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

branch: return error when --list finds no matches #892

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joshuahunt
Copy link

@joshuahunt joshuahunt commented Mar 3, 2021

Currently git branch --list foo always returns an exit status of 0 even
when the branch being searched for does not exist. Now an error is printed
and returns a non-zero exit status.

Signed-off-by: Josh Hunt johunt@akamai.com

Thanks for taking the time to contribute to Git! Please be advised that the
Git community does not use github.com for their contributions. Instead, we use
a mailing list (git@vger.kernel.org) for code submissions, code reviews, and
bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.

Please read the "guidelines for contributing" linked above!

cc: Josh Hunt johunt@akamai.com

Currently git branch --list foo always returns an exit status of 0 even
when the branch being searched for does not exist. Now an error is printed
and returns a non-zero exit status.

Signed-off-by: Josh Hunt <johunt@akamai.com>
@gitgitgadget
Copy link

gitgitgadget bot commented Mar 3, 2021

Welcome to GitGitGadget

Hi @joshuahunt, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the FreeNode IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Freenode. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

@gitgitgadget gitgitgadget bot added the new user label Mar 3, 2021
@dscho
Copy link
Member

dscho commented Mar 3, 2021

/allow

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 3, 2021

User joshuahunt is now allowed to use GitGitGadget.

WARNING: joshuahunt has no public email address set on GitHub

@joshuahunt
Copy link
Author

I've added my public email.

@dscho
Copy link
Member

dscho commented Mar 3, 2021

I've added my public email.

Good! Now try /preview, and if everything works all right, /submit.

@joshuahunt
Copy link
Author

/preview

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 3, 2021

Preview email sent as pull.892.git.1614792649003.gitgitgadget@gmail.com

@joshuahunt
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 3, 2021

Submitted as pull.892.git.1614793491538.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-892/joshuahunt/johunt/git-branch-list-return-error-v1

To fetch this version to local tag pr-892/joshuahunt/johunt/git-branch-list-return-error-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-892/joshuahunt/johunt/git-branch-list-return-error-v1

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 4, 2021

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Josh Hunt via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Josh Hunt <johunt@akamai.com>
>
> Currently git branch --list foo always returns an exit status of 0 even
> when the branch being searched for does not exist. Now an error is printed
> and returns a non-zero exit status.

Explaining what happens in the current code upfront is a good thing
and is in line with the convention used in our project, which is
good.  But drop "currently" from there.

Strictly speaking, it is not "always".  In a corrupt repository, it
is likely to show a proper error message and die.

Also explaining what you want to happen before the end of the log
message is good.

But the proposed log message lacks why it is a good idea to make
such a change, which is the most important part.

If you ask me, I would say that the command was asked to show any
branches, if exist, that match the given pattern, and did what it
was asked to do without encountering any error---it just happened to
have seen 0 branch that matched.  So I think returning non-zero
status would be a bug.

Thanks.



@gitgitgadget
Copy link

gitgitgadget bot commented Mar 5, 2021

On the Git mailing list, Josh Hunt wrote (reply to this):

On 3/3/21 5:27 PM, Junio C Hamano wrote:
> "Josh Hunt via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Josh Hunt <johunt@akamai.com>
>>
>> Currently git branch --list foo always returns an exit status of 0 even
>> when the branch being searched for does not exist. Now an error is printed
>> and returns a non-zero exit status.
> 

Junio

Thanks so much for the review.

> Explaining what happens in the current code upfront is a good thing
> and is in line with the convention used in our project, which is
> good.  But drop "currently" from there.
> 
> Strictly speaking, it is not "always".  In a corrupt repository, it
> is likely to show a proper error message and die.
> 
> Also explaining what you want to happen before the end of the log
> message is good.

Thank you. I will make the above changes to the commit message if this 
moves forward.

> 
> But the proposed log message lacks why it is a good idea to make
> such a change, which is the most important part.

OK sure. To provide better context I came across this when using 'git 
branch --list foo' in a script and there was an expectation (possibly 
incorrect) when the search was not successful it would return an 
non-zero exit status.

I don't know if there's a convention that git follows, but I have the 
following examples where a similar type of command does return a 
non-zero exit status:

johunt@johunt-ThinkPad-T480s:~$ ls foo
ls: cannot access 'foo': No such file or directory
johunt@johunt-ThinkPad-T480s:~$ echo $?
2

johunt@johunt-ThinkPad-T480s:~$ touch foo
johunt@johunt-ThinkPad-T480s:~$ grep bar foo
johunt@johunt-ThinkPad-T480s:~$ echo $?
1

johunt@johunt-ThinkPad-T480s:~$ grep . foo
grep: foo: No such file or directory
johunt@johunt-ThinkPad-T480s:~$ echo $?
2

Not just these tools, but even in git itself:

johunt@johunt-ThinkPad-T480s:~$ git init foo
Initialized empty Git repository in /home/johunt/foo/.git/
johunt@johunt-ThinkPad-T480s:~/foo$ touch bar
johunt@johunt-ThinkPad-T480s:~/foo$ git add bar
johunt@johunt-ThinkPad-T480s:~/foo$ git commit -am 'initial commit'
[master (root-commit) 771510f] initial commit
  1 file changed, 0 insertions(+), 0 deletions(-)
  create mode 100644 bar
johunt@johunt-ThinkPad-T480s:~/foo$ git log foo
fatal: ambiguous argument 'foo': unknown revision or path not in the 
working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
johunt@johunt-ThinkPad-T480s:~/foo$ echo $?
128

johunt@johunt-ThinkPad-T480s:~/foo$ git grep foo .
johunt@johunt-ThinkPad-T480s:~/foo$ echo $?
1

However it does seem like 'tag --list' follows the branch behavior:

johunt@johunt-ThinkPad-T480s:~/foo$ git tag --list foo
johunt@johunt-ThinkPad-T480s:~/foo$ echo $?
0

There are likely other examples of git commands showing both behaviors. 
Is it that branch and tag are a certain type of command which allows 
them to behave differently than say log or grep?

> 
> If you ask me, I would say that the command was asked to show any
> branches, if exist, that match the given pattern, and did what it
> was asked to do without encountering any error---it just happened to
> have seen 0 branch that matched.  So I think returning non-zero
> status would be a bug.

Interesting. Going back to the scripting context then your suggestion 
would just be to check if output is NULL? This is what I've converted my 
scripts to do for now, but still feel like if --list can't find the 
branch I'm looking for then it should return non-zero as it matches 
behavior of other tools :)

Josh

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 5, 2021

User Josh Hunt <johunt@akamai.com> has been added to the cc: list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants