Bug? "git branch" failing to list all branches

Bug? "git branch" failing to list all branches

[Simo Melenius]

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/

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

[Jonathan Nieder]

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.

>                 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

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

[Simo Melenius]

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?

> 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/

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

[Jonathan Nieder]

Simo Melenius wrote:

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

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

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

[Simo Melenius]

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

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

[Jonathan Nieder]

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.

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

> @@ -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;
 };
-- 

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

[Simo Melenius]

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

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

[Sverre Rabbelier]

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

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

[Jonathan Nieder]

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;
 }
-- 

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

[Simo Melenius]

On 4 June 2010 05:43, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Except as pointed out by Sverre[1],
>  Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks, I'll now resend the polished versions of these patches.


Simo

-- 
() Today is the car of the cdr of your life.
/\ http://arc.pasp.de/

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

[Simo Melenius]

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, 19 insertions(+), 6 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 6cf7e72..72a486c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -257,9 +257,15 @@ 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;
@@ -293,8 +299,10 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 	commit = NULL;
 	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);
+		if (!commit) {
+			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 +492,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 +505,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 +538,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 +692,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

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

[Simo Melenius]

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 72a486c..2371ca0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -301,7 +301,7 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 		commit = lookup_commit_reference_gently(sha1, 1);
 		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 */
@@ -539,6 +539,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");
+
 	return cb.ret;
 }
 
-- 
1.7.0.4