Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Look for branches that are literally the word main #1981

Conversation

william-richard
Copy link
Contributor

Description

When finding the default git branch, use a regex that matches only the word main, instead of any branch that has the substring main in it.

Motivation and Context

I have a repo with many other branches that contain the string main,
but our default branch is still master. This grep was seeing those other branches
and deciding that my default branch was main.

How Has This Been Tested?

I tested this change on the repository I described above, and it correctly decided my default branch was master again.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.
    • I didn't see any existing tests for the git aliases (or any aliases). Should I add a new test file?

I have a repo with many other branches that contain the string `main`,
but our default branch is still master.  This grep was seeing those other branches
and deciding that my default branch was `main`.  This tighter regex
fixes that behavior for me.
Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @william-richard
Thanks for the PR!

Regarding the tests- we sadly don't have a full test coverage of the repo. I you have time and want to contribute, I will gladly accept PRs which add test coverage 😄

@william-richard
Copy link
Contributor Author

Heh, my bash-fu is not very strong... I'm not sure I'm the right person to take on that job. Sorry 😢

@davidpfarrell
Copy link
Contributor

Hi All !

The proposed regex appears to be broken relative to the actual output of git branch.

when i do git branch in my local

$ cd $BASH_IT
$ git branch

* (HEAD detached at origin/master)
  davidpfarrell/master
  docs/development_rst
  help
  list
  master

try the grep (adapted for master)

$ git branch | grep -q '^master$' && echo "FOUND IT" || echo "NOPE"

NOPE

The regex could probably work as:

$ git branch | grep -q '\bmaster\b' && echo "FOUND IT" || echo "NOPE"

FOUND IT

And, adapted to just fetch the master vs main branch in one go:

thanks stack overflow

$ git branch | grep -o -m1 "\b\(master\|main\)\b"

master

There's (currently) no great way to solve the "default branch" problem, but I think that a more general approach could be supporting a user-definable config property. ie something like :

# Set default branch for this repo
$ git config core.defaultBranch main

# Query default branch 
$ git config --get core.defaultBranch

Copy link
Contributor

@davidpfarrell davidpfarrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex appears to be broken (per my previous comment)

@NoahGorny
Copy link
Member

@davidpfarrell is indeed correct. we need to change the regex to \wmain\w instead. Great catch!

@davidpfarrell
Copy link
Contributor

@davidpfarrell is indeed correct. we need to change the regex to \wmain\w instead. Great catch!

I don't know if \w is going to do it:

$ git branch

* (HEAD detached at origin/master)
  davidpfarrell/master
  docs/development_rst
  help
  list
  master

with \w

$ git branch | grep -q '\wmaster\w' && echo "FOUND IT" || echo "NOPE"

NOPE

with \b

git branch | grep -q '\bmaster\b' && echo "FOUND IT" || echo "NOPE"
FOUND IT

@NoahGorny
Copy link
Member

@davidpfarrell is indeed correct. we need to change the regex to \wmain\w instead. Great catch!

I don't know if \w is going to do it:

$ git branch

* (HEAD detached at origin/master)
  davidpfarrell/master
  docs/development_rst
  help
  list
  master

with \w

$ git branch | grep -q '\wmaster\w' && echo "FOUND IT" || echo "NOPE"

NOPE

with \b

git branch | grep -q '\bmaster\b' && echo "FOUND IT" || echo "NOPE"
FOUND IT

whoops, I do not even know how I wrote `\w', I meant '\b'.... brain bug probably
thanks for correcting me @davidpfarrell 😄

@william-richard
Copy link
Contributor Author

Ok, I've updated the PR to use /b - thanks for that suggestion! That's a much better appoarch.

I just remembered that I have a hacky script that checks out the default branch of a repo - a lot of the codebases at my company use develop as the default branch, and I use this to determine what the default branch is:

DEFAULT_BRANCH=$(git symbolic-ref refs/remotes/origin/HEAD | sed 's@^refs/remotes/origin/@@')

That said, should gcom checkout a default branch that doesn't start with an m? Or is its intended behavior to check out the default branch, and since that usually starts with an m, we name the alias with an m?

\b matches things like - and _, which is often used in branch names
@davidpfarrell
Copy link
Contributor

That said, should gcom checkout a default branch that doesn't start with an m?

I thin the spirit of the g*m aliases (gcom, gswm, etc) are meant to target the "default" branch.
The issue is that there isn't a great standard way to determine this in git right now.

I think I saw versions of your script in my stack overflow discussion I linked in my previous comment.

I'm contemplating submitting a request to have git create a branch.default property when initializing a repo, and/or at least try to get community adoption for the property. Then tools can look there first and the user can have control over the value in a standard way.

Copy link
Contributor

@davidpfarrell davidpfarrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi again.
Q: Did you test this?

adapted for master

$ cd $BASH_IT

# with \b
$git branch | grep -q '\bmaster\b' && echo "FOUND IT" || echo "NOPE"
FOUND IT

# with \s
$ git branch | grep -q '\smaster\s' && echo "FOUND IT" || echo "NOPE"
NOPE

So git branch names are (currently) not allowed to contain whitespace, so the \b solution is likely fine.

\smain$ might be a bit better, or we could go all-out with :

  • '^. master$'

Which will match the whole line

@william-richard
Copy link
Contributor Author

william-richard commented Oct 27, 2021

Hi David,

Yep, I've been running with my branch for the last week, and this has been working pretty reliably for me. But, I also don't regularly use any repos that use main as their default branch, so I'm realizing that my tests are incomplete. I just tested on a repo with main as the default, and you're right that the regex I committed did not work as expected.

\b did not work for me because I have many branches that look something like foo-main-bar, so \b matches the -, and that gives me a false positive. Also, the code I'm running is trying to match main, as opposed to your test which is matching master.

I think your suggestion to try to match the entire line is a good one - I'll take another stab at this, and make sure to test in both flavors of repos before I request reviews again.

Sorry for my incomplete testing - I do appreciate your time and effort, and I'll do the best to make better use of your time in the future. 😸

@davidpfarrell
Copy link
Contributor

Hi @william-richard,

I don't think you caught all of my previous comment.

I believe I showed that \smain\s does not work, and was waiting for you to update the PR.

I think the "full line" matcher is the way to go ...

@william-richard
Copy link
Contributor Author

Apologies @davidpfarrell - I thought I had pushed up the full line match commit, but didn't notice the error message and moved onto something else.

I've been running with the full line match version for about 2 weeks now, and I agree, I think it's the way to go forward.

Copy link
Contributor

@davidpfarrell davidpfarrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that the \s* is needed,

BUT I am convinced that this works in my local, so ...

Approved and thanks for contributing to Bash-It !

@NoahGorny NoahGorny merged commit 447c89a into Bash-it:master Nov 16, 2021
@william-richard william-richard deleted the make-default-branch-match-exactly-main branch November 18, 2021 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants