[PATCH] branch: return error when --list finds no matches

[PATCH] branch: return error when --list finds no matches

[Josh Hunt via GitGitGadget]

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.

Signed-off-by: Josh Hunt <johunt@akamai.com>
---
    branch: return error when --list finds no matches
    
    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

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-892%2Fjoshuahunt%2Fjohunt%2Fgit-branch-list-return-error-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-892/joshuahunt/johunt/git-branch-list-return-error-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/892

 builtin/branch.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index bcc00bcf182d..5573175734fe 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -426,6 +426,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 	memset(&array, 0, sizeof(array));
 
 	filter_refs(&array, filter, filter->kind | FILTER_REFS_INCLUDE_BROKEN);
+	if (!array.nr)
+		die(_("no branches found"));
 
 	if (filter->verbose)
 		maxwidth = calc_maxwidth(&array, strlen(remote_prefix));

base-commit: ec125d1bc1053df7e4103f71ebd24fe48dd624a2
-- 
gitgitgadget

Re: [PATCH] branch: return error when --list finds no matches

[Junio C Hamano]

"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.

Re: [PATCH] branch: return error when --list finds no matches

[Josh Hunt]

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