threads / bug / 23981

Bug? "git branch" failing to list all branches

Subject: Bug? "git branch" failing to list all branches

## tl;dr

9 messages between Jun 2, 2010 and Jun 4, 2010.

replies: 8people: 3as markdown or json

Simo Melenius· Jun 2, 2010, 14:47 UTC · lore

I consider this a bug and wrote a fix. I would now like to ask the git maintainers' opinion with regard to it.

When listing branches, "git branch" will in certain cases terminate iteration at the first broken ref that doesn't point to a commit. This will silently hide any remaining refs from the output listing. However, this failure is not communicated upwards either, so I think append_ref() goes wrong to terminate the whole loop because of this.

I noticed this because "git branch -a" and "git branch -av" unexpectedly gave a very different output.

Simo
diff --git a/builtin/branch.c b/builtin/branch.c
index 6cf7e72..1a8e3d3 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -294,7 +294,10 @@ static int append_ref(const char *refname, const
unsigned char *sha1, int flags,
        if (ref_list->verbose || ref_list->with_commit || merge_filter
!= NO_FILTER) {
                commit = lookup_commit_reference_gently(sha1, 1);
                if (!commit)
-                       return error("branch '%s' does not point at a
commit", refname);
+               {
+                       error("branch '%s' does not point at a
commit", refname);
+                       return 0;
+               }

                /* Filter with with_commit if specified */
                if (!is_descendant_of(commit, ref_list->with_commit))
-- 
() Today is the car of the cdr of your life.
/\ http://arc.pasp.de/
Jonathan Nieder· Jun 3, 2010, 04:22 UTC · re: Simo Melenius · lore

Re: Bug? "git branch" failing to list all branches

Hi Simo,
Simo Melenius wrote:
> I noticed this because "git branch -a" and "git branch -av"
> unexpectedly gave a very different output.
Hmm --- so the error message must not have been very visible...
> When listing branches, "git branch" will in certain cases terminate
> iteration at the first broken ref that doesn't point to a commit.

Even in a broken repository, the full branch list would be useful for getting one’s bearings. Thanks.

Show 9 quoted lines
>                 commit = lookup_commit_reference_gently(sha1, 1);
>                 if (!commit)
> -                       return error("branch '%s' does not point at a
> commit", refname);
> +               {
> +                       error("branch '%s' does not point at a
> commit", refname);
> +                       return 0;
> +               }

Will this make ‘git branch’ exit with status zero? Scripts and people with fancy prompts benefit from a nonzero exit status.

If I have 37 branches and an error is encountered looking up one of them, with this patch the error message will scroll off the screen. Is this worth worrying about? It depends on what the usual causes for broken branch refs are and whether they require attention or can be safely ignored.

One other thought: this patch is line-wrapped, which means it cannot be mechanically applied. Documentation/SubmittingPatches has some tips on sending a patch unmangled (and please also see the section labelled "Sign your work").

Cheers, Jonathan

Simo Melenius· Jun 3, 2010, 06:00 UTC · re: Jonathan Nieder · lore

Re: Bug? "git branch" failing to list all branches

On 3 June 2010 07:22, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> I noticed this because "git branch -a" and "git branch -av"
>> unexpectedly gave a very different output.
> Hmm --- so the error message must not have been very visible...

I have been working with such set of repositories that most of them have one or two broken refs. I probably saw it but didn't care because it was a known issue. If I had called git branch from a script and piped the output somewhere while relying on exit status, I wouldn't have noticed anything.

> Will this make ‘git branch’ exit with status zero?  Scripts and people
> with fancy prompts benefit from a nonzero exit status.

My change doesn't change the current behaviour. At least git 1.7.0.4 didn't give a nonzero exit status either. It would be good if it did.

I could add that to my patch. I'm, however, unsure of what's the best way to communicate the error from append_ref() to cmd_branch(). A static variable in branch.c would of course do.

However, if the git codebase has somewhere a global mechanism for signalling errors by, for example, raising some flag when error() is called, using that mechanism would be better, right?

Show 5 quoted lines
> If I have 37 branches and an error is encountered looking up one of
> them, with this patch the error message will scroll off the screen.
> Is this worth worrying about?  It depends on what the usual causes for
> broken branch refs are and whether they require attention or can be
> safely ignored.

Since this only concerns the printing of branches, often for interactive viewing or bash completion, and does not affect any of the operations that modify the repository, I think it's sufficient that the error message is still readable from stderr for those who are interested.

> One other thought: this patch is line-wrapped, which means it cannot
> be mechanically applied.  Documentation/SubmittingPatches has some
> tips on sending a patch unmangled (and please also see the section
> labelled "Sign your work").

Yeah, I have a kosher patch locally. I just copypasted the diff part here for discussion.

Simo
-- 
() Today is the car of the cdr of your life.
/\ http://arc.pasp.de/
Jonathan Nieder· Jun 3, 2010, 06:55 UTC · re: Simo Melenius · lore

Re: Bug? "git branch" failing to list all branches

Simo Melenius wrote:
Show 6 quoted lines
> My change doesn't change the current behaviour. At least git 1.7.0.4
> didn't give a nonzero exit status either. It would be good if it did.
> 
> I could add that to my patch. I'm, however, unsure of what's the best
> way to communicate the error from append_ref() to cmd_branch(). A
> static variable in branch.c would of course do.
I see.  You can use the cb_data argument to pass a result back:
 struct append_ref_cb {
 	struct ref_list *ref_list;
 	int err;
 };
A static variable might be simpler.

But that is a separate topic (for a separate patch). I can pick it up if you don’t.

Show 11 quoted lines
>> If I have 37 branches and an error is encountered looking up one of
>> them, with this patch the error message will scroll off the screen.
>> Is this worth worrying about?  It depends on what the usual causes for
>> broken branch refs are and whether they require attention or can be
>> safely ignored.
>
> Since this only concerns the printing of branches, often for
> interactive viewing or bash completion, and does not affect any of the
> operations that modify the repository, I think it's sufficient that
> the error message is still readable from stderr for those who are
> interested.
Sorry, I must have been unclear.  Let me illustrate with an example:
 $ git branch
 * (no branch)
   cc/sequencer-rebase-i
   db/svn-fe
 error: branch 'dk/hash' does not point at a commit.
   gp/debian-pu
   gp/sid-patches
   jl/gitk-submodule
   jk/pull-rebase-message
   jn/debian-build-depends
   js/grep-open
   ks/gitk-notes
   nd/gitbox
   nd/setup
   rr/svn-remote
   sb/sequencer-dev
   sb/sequencer-rfc
 ...

If this scrolls on for too long, I will not see the message. Now if the error is due to some kind of corruption or a broken script, I would want to know about it right away, even if I can carry on with my work without. So in that case, it would make sense to add at the end:

 fatal: some refs could not be read.

On the other hand, maybe there is some harmless process that often creates these broken refs and such a message would be a nuisance.

Hoping that is clearer, Jonathan

Simo Melenius· Jun 3, 2010, 07:48 UTC · re: Jonathan Nieder · lore

[PATCH 1/2] branch: exit status now reflects if branch listing finds an error

If some refs could not be read when listing branches, this can now be observed in the exit status of the "git branch" command.

Signed-off-by: Simo Melenius <simo.melenius@iki.fi>
---
 builtin/branch.c |   25 ++++++++++++++++++++-----
 1 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 6cf7e72..46ca59c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -257,9 +257,16 @@ static char *resolve_symref(const char *src, const char *prefix)
 	return xstrdup(dst);
 }
 
+struct append_ref_cb
+{
+	struct ref_list *ref_list;
+	int ret;
+};
+
 static int append_ref(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
 {
-	struct ref_list *ref_list = (struct ref_list*)(cb_data);
+	struct append_ref_cb *cb = (struct append_ref_cb*)(cb_data);
+	struct ref_list *ref_list = cb->ref_list;
 	struct ref_item *newitem;
 	struct commit *commit;
 	int kind, i;
@@ -294,7 +301,10 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 	if (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {
 		commit = lookup_commit_reference_gently(sha1, 1);
 		if (!commit)
-			return error("branch '%s' does not point at a commit", refname);
+		{
+			cb->ret = error("branch '%s' does not point at a commit", refname);
+			return cb->ret;
+		}
 
 		/* Filter with with_commit if specified */
 		if (!is_descendant_of(commit, ref_list->with_commit))
@@ -484,9 +494,10 @@ static void show_detached(struct ref_list *ref_list)
 	}
 }
 
-static void print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit)
+static int print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit)
 {
 	int i;
+	struct append_ref_cb cb;
 	struct ref_list ref_list;
 
 	memset(&ref_list, 0, sizeof(ref_list));
@@ -496,7 +507,9 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str
 	ref_list.with_commit = with_commit;
 	if (merge_filter != NO_FILTER)
 		init_revisions(&ref_list.revs, NULL);
-	for_each_rawref(append_ref, &ref_list);
+	cb.ref_list = &ref_list;
+	cb.ret = 0;
+	for_each_rawref(append_ref, &cb);
 	if (merge_filter != NO_FILTER) {
 		struct commit *filter;
 		filter = lookup_commit_reference_gently(merge_filter_ref, 0);
@@ -527,6 +540,8 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str
 	}
 
 	free_ref_list(&ref_list);
+
+	return cb.ret;
 }
 
 static void rename_branch(const char *oldname, const char *newname, int force)
@@ -679,7 +694,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (delete)
 		return delete_branches(argc, argv, delete > 1, kinds);
 	else if (argc == 0)
-		print_ref_list(kinds, detached, verbose, abbrev, with_commit);
+		return print_ref_list(kinds, detached, verbose, abbrev, with_commit);
 	else if (rename && (argc == 1))
 		rename_branch(head, argv[0], rename > 1);
 	else if (rename && (argc == 2))
-- 
1.7.0.4
Jonathan Nieder· Jun 4, 2010, 02:24 UTC · re: Simo Melenius · lore

Re: [PATCH 1/2] branch: exit status now reflects if branch listing finds an error

Simo Melenius wrote:
> If some refs could not be read when listing branches, this can now be
> observed in the exit status of the "git branch" command.
A noble cause.  Thanks.
Show 5 quoted lines
> +struct append_ref_cb
> +{
> +	struct ref_list *ref_list;
> +	int ret;
> +};

nitpick: the brace should go on the same line to match the other structs in builtin/branch.c and elsewhere in git.

Show 11 quoted lines
> @@ -496,7 +507,9 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str
>  	ref_list.with_commit = with_commit;
>  	if (merge_filter != NO_FILTER)
>  		init_revisions(&ref_list.revs, NULL);
> -	for_each_rawref(append_ref, &ref_list);
> +	cb.ref_list = &ref_list;
> +	cb.ret = 0;
> +	for_each_rawref(append_ref, &cb);
>  	if (merge_filter != NO_FILTER) {
>  		struct commit *filter;
>  		filter = lookup_commit_reference_gently(merge_filter_ref, 0);

This can be simplified by "ret = for_each_rawref(append_ref, ..." but the above would have to be added back anyway for patch 2/2. So I’m happy with this patch as is.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
diff --git a/builtin/branch.c b/builtin/branch.c
index 46ca59c..77ae444 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -257,8 +257,7 @@ static char *resolve_symref(const char *src, const char *prefix)
 	return xstrdup(dst);
 }
 
-struct append_ref_cb
-{
+struct append_ref_cb {
 	struct ref_list *ref_list;
 	int ret;
 };
Simo Melenius· Jun 3, 2010, 07:48 UTC · re: Jonathan Nieder · lore

[PATCH 2/2] branch: don't fail listing branches if one of the commits wasn't found

When listing branches with ref lookups, if one of the known raw refs doesn't point to a commit then "git branch" would return error(), terminating the whole for_each_rawref() iteration and possibly hiding any remaining refs.

Signed-off-by: Simo Melenius <simo.melenius@iki.fi>
---
 builtin/branch.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 46ca59c..2242743 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -303,7 +303,7 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 		if (!commit)
 		{
 			cb->ret = error("branch '%s' does not point at a commit", refname);
-			return cb->ret;
+			return 0;
 		}
 
 		/* Filter with with_commit if specified */
@@ -541,6 +541,9 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 
 	free_ref_list(&ref_list);
 
+	if (cb.ret)
+		error("some refs could not be read, review stderr");
+
 	return cb.ret;
 }
 
-- 
1.7.0.4
Sverre Rabbelier· Jun 3, 2010, 17:42 UTC · re: Simo Melenius · lore

Re: [PATCH 2/2] branch: don't fail listing branches if one of the commits wasn't found

Heya,
On Thu, Jun 3, 2010 at 09:48, Simo Melenius <simo.melenius@iki.fi> wrote:
> +       if (cb.ret)
> +               error("some refs could not be read, review stderr");

I don't think there's any precedence for an error message like this, perhaps in git-svn's "the above error message is nothing to worry about, move along", which I think is silly as well. I think it's best to just s/, review stderr// here.

-- 
Cheers,

Sverre Rabbelier
Jonathan Nieder· Jun 4, 2010, 02:43 UTC · re: Simo Melenius · lore

Re: [PATCH 2/2] branch: don't fail listing branches if one of the commits wasn't found

Simo Melenius wrote:
> Signed-off-by: Simo Melenius <simo.melenius@iki.fi>
Except as pointed out by Sverre[1],
  Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
[1] http://thread.gmane.org/gmane.comp.version-control.git/148253/focus=148350
diff --git a/builtin/branch.c b/builtin/branch.c
index 2242743..d2d3c26 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -542,7 +542,7 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 	free_ref_list(&ref_list);
 
 	if (cb.ret)
-		error("some refs could not be read, review stderr");
+		error("some refs could not be read");
 
 	return cb.ret;
 }

← back to recent threads