{"thread":{"id":"23981","subject":"Bug? \"git branch\" failing to list all branches","startedAt":"2010-06-02T14:47:41Z","lastAt":"2010-06-04T02:43:45Z","messageCount":9,"participants":["Simo Melenius","Jonathan Nieder","Sverre Rabbelier"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"142819","messageId":"AANLkTimTOucIfzSxsYNvmML7MALwj0E3BUASIIKIN1lN@mail.gmail.com","threadId":"23981","inReplyTo":null,"subject":"Bug? \"git branch\" failing to list all branches","fromName":"Simo Melenius","fromEmail":"simo.melenius@iki.fi","sentAt":"2010-06-02T14:47:41Z","receivedAt":"2010-06-02T14:47:41Z","isPatch":false,"sender":{"key":"simo.melenius@iki.fi","avatar":null},"body":"I consider this a bug and wrote a fix. I would now like to ask the git\nmaintainers' opinion with regard to it.\n\nWhen listing branches, \"git branch\" will in certain cases terminate\niteration at the first broken ref that doesn't point to a commit. This\nwill silently hide any remaining refs from the output listing.\nHowever, this failure is not communicated upwards either, so I think\nappend_ref() goes wrong to terminate the whole loop because of this.\n\nI noticed this because \"git branch -a\" and \"git branch -av\"\nunexpectedly gave a very different output.\n\n\nSimo\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6cf7e72..1a8e3d3 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -294,7 +294,10 @@ static int append_ref(const char *refname, const\nunsigned char *sha1, int flags,\n        if (ref_list->verbose || ref_list->with_commit || merge_filter\n!= NO_FILTER) {\n                commit = lookup_commit_reference_gently(sha1, 1);\n                if (!commit)\n-                       return error(\"branch '%s' does not point at a\ncommit\", refname);\n+               {\n+                       error(\"branch '%s' does not point at a\ncommit\", refname);\n+                       return 0;\n+               }\n\n                /* Filter with with_commit if specified */\n                if (!is_descendant_of(commit, ref_list->with_commit))\n\n-- \n() Today is the car of the cdr of your life.\n/\\ http://arc.pasp.de/\n"},{"id":"142856","messageId":"20100603042218.GA21254@progeny.tock","threadId":"23981","inReplyTo":"AANLkTimTOucIfzSxsYNvmML7MALwj0E3BUASIIKIN1lN@mail.gmail.com","subject":"Re: Bug? \"git branch\" failing to list all branches","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-06-03T04:22:18Z","receivedAt":"2010-06-03T04:22:18Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Simo,\n\nSimo Melenius wrote:\n\n> I noticed this because \"git branch -a\" and \"git branch -av\"\n> unexpectedly gave a very different output.\n\nHmm --- so the error message must not have been very visible...\n\n> When listing branches, \"git branch\" will in certain cases terminate\n> iteration at the first broken ref that doesn't point to a commit.\n\nEven in a broken repository, the full branch list would be useful for\ngetting one’s bearings.  Thanks.\n\n>                 commit = lookup_commit_reference_gently(sha1, 1);\n>                 if (!commit)\n> -                       return error(\"branch '%s' does not point at a\n> commit\", refname);\n> +               {\n> +                       error(\"branch '%s' does not point at a\n> commit\", refname);\n> +                       return 0;\n> +               }\n\nWill this make ‘git branch’ exit with status zero?  Scripts and people\nwith fancy prompts benefit from a nonzero exit status.\n\nIf I have 37 branches and an error is encountered looking up one of\nthem, with this patch the error message will scroll off the screen.\nIs this worth worrying about?  It depends on what the usual causes for\nbroken branch refs are and whether they require attention or can be\nsafely ignored.\n\nOne other thought: this patch is line-wrapped, which means it cannot\nbe mechanically applied.  Documentation/SubmittingPatches has some\ntips on sending a patch unmangled (and please also see the section\nlabelled \"Sign your work\").\n\nCheers,\nJonathan\n"},{"id":"142859","messageId":"AANLkTinCNgsIf_MsiysfphKm3MCW2Uio_xiemfDZraH7@mail.gmail.com","threadId":"23981","inReplyTo":"20100603042218.GA21254@progeny.tock","subject":"Re: Bug? \"git branch\" failing to list all branches","fromName":"Simo Melenius","fromEmail":"simo.melenius@iki.fi","sentAt":"2010-06-03T06:00:14Z","receivedAt":"2010-06-03T06:00:14Z","isPatch":false,"sender":{"key":"simo.melenius@iki.fi","avatar":null},"body":"On 3 June 2010 07:22, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>> I noticed this because \"git branch -a\" and \"git branch -av\"\n>> unexpectedly gave a very different output.\n> Hmm --- so the error message must not have been very visible...\n\nI have been working with such set of repositories that most of them\nhave one or two broken refs. I probably saw it but didn't care because\nit was a known issue. If I had called git branch from a script and\npiped the output somewhere while relying on exit status, I wouldn't\nhave noticed anything.\n\n> Will this make ‘git branch’ exit with status zero?  Scripts and people\n> with fancy prompts benefit from a nonzero exit status.\n\nMy change doesn't change the current behaviour. At least git 1.7.0.4\ndidn't give a nonzero exit status either. It would be good if it did.\n\nI could add that to my patch. I'm, however, unsure of what's the best\nway to communicate the error from append_ref() to cmd_branch(). A\nstatic variable in branch.c would of course do.\n\nHowever, if the git codebase has somewhere a global mechanism for\nsignalling errors by, for example, raising some flag when error() is\ncalled, using that mechanism would be better, right?\n\n> If I have 37 branches and an error is encountered looking up one of\n> them, with this patch the error message will scroll off the screen.\n> Is this worth worrying about?  It depends on what the usual causes for\n> broken branch refs are and whether they require attention or can be\n> safely ignored.\n\nSince this only concerns the printing of branches, often for\ninteractive viewing or bash completion, and does not affect any of the\noperations that modify the repository, I think it's sufficient that\nthe error message is still readable from stderr for those who are\ninterested.\n\n> One other thought: this patch is line-wrapped, which means it cannot\n> be mechanically applied.  Documentation/SubmittingPatches has some\n> tips on sending a patch unmangled (and please also see the section\n> labelled \"Sign your work\").\n\nYeah, I have a kosher patch locally. I just copypasted the diff part\nhere for discussion.\n\n\nSimo\n\n-- \n() Today is the car of the cdr of your life.\n/\\ http://arc.pasp.de/\n"},{"id":"142860","messageId":"20100603065527.GA23439@progeny.tock","threadId":"23981","inReplyTo":"AANLkTinCNgsIf_MsiysfphKm3MCW2Uio_xiemfDZraH7@mail.gmail.com","subject":"Re: Bug? \"git branch\" failing to list all branches","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-06-03T06:55:28Z","receivedAt":"2010-06-03T06:55:28Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Simo Melenius wrote:\n\n> My change doesn't change the current behaviour. At least git 1.7.0.4\n> didn't give a nonzero exit status either. It would be good if it did.\n> \n> I could add that to my patch. I'm, however, unsure of what's the best\n> way to communicate the error from append_ref() to cmd_branch(). A\n> static variable in branch.c would of course do.\n\nI see.  You can use the cb_data argument to pass a result back:\n\n struct append_ref_cb {\n \tstruct ref_list *ref_list;\n \tint err;\n };\n\nA static variable might be simpler.\n\nBut that is a separate topic (for a separate patch).  I can pick it up\nif you don’t.\n\n>> If I have 37 branches and an error is encountered looking up one of\n>> them, with this patch the error message will scroll off the screen.\n>> Is this worth worrying about?  It depends on what the usual causes for\n>> broken branch refs are and whether they require attention or can be\n>> safely ignored.\n>\n> Since this only concerns the printing of branches, often for\n> interactive viewing or bash completion, and does not affect any of the\n> operations that modify the repository, I think it's sufficient that\n> the error message is still readable from stderr for those who are\n> interested.\n\nSorry, I must have been unclear.  Let me illustrate with an example:\n\n $ git branch\n * (no branch)\n   cc/sequencer-rebase-i\n   db/svn-fe\n error: branch 'dk/hash' does not point at a commit.\n   gp/debian-pu\n   gp/sid-patches\n   jl/gitk-submodule\n   jk/pull-rebase-message\n   jn/debian-build-depends\n   js/grep-open\n   ks/gitk-notes\n   nd/gitbox\n   nd/setup\n   rr/svn-remote\n   sb/sequencer-dev\n   sb/sequencer-rfc\n ...\n\nIf this scrolls on for too long, I will not see the message.  Now if\nthe error is due to some kind of corruption or a broken script, I\nwould want to know about it right away, even if I can carry on with\nmy work without.  So in that case, it would make sense to add at the\nend:\n\n fatal: some refs could not be read.\n\nOn the other hand, maybe there is some harmless process that often\ncreates these broken refs and such a message would be a nuisance.\n\nHoping that is clearer,\nJonathan\n"},{"id":"142862","messageId":"1275551282-21557-1-git-send-email-simo.melenius@iki.fi","threadId":"23981","inReplyTo":"20100603065527.GA23439@progeny.tock","subject":"[PATCH 1/2] branch: exit status now reflects if branch listing finds an error","fromName":"Simo Melenius","fromEmail":"simo.melenius@iki.fi","sentAt":"2010-06-03T07:48:01Z","receivedAt":"2010-06-03T07:48:01Z","isPatch":true,"sender":{"key":"simo.melenius@iki.fi","avatar":null},"body":"If some refs could not be read when listing branches, this can now be\nobserved in the exit status of the \"git branch\" command.\n\nSigned-off-by: Simo Melenius <simo.melenius@iki.fi>\n---\n builtin/branch.c |   25 ++++++++++++++++++++-----\n 1 files changed, 20 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6cf7e72..46ca59c 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -257,9 +257,16 @@ static char *resolve_symref(const char *src, const char *prefix)\n \treturn xstrdup(dst);\n }\n \n+struct append_ref_cb\n+{\n+\tstruct ref_list *ref_list;\n+\tint ret;\n+};\n+\n static int append_ref(const char *refname, const unsigned char *sha1, int flags, void *cb_data)\n {\n-\tstruct ref_list *ref_list = (struct ref_list*)(cb_data);\n+\tstruct append_ref_cb *cb = (struct append_ref_cb*)(cb_data);\n+\tstruct ref_list *ref_list = cb->ref_list;\n \tstruct ref_item *newitem;\n \tstruct commit *commit;\n \tint kind, i;\n@@ -294,7 +301,10 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,\n \tif (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {\n \t\tcommit = lookup_commit_reference_gently(sha1, 1);\n \t\tif (!commit)\n-\t\t\treturn error(\"branch '%s' does not point at a commit\", refname);\n+\t\t{\n+\t\t\tcb->ret = error(\"branch '%s' does not point at a commit\", refname);\n+\t\t\treturn cb->ret;\n+\t\t}\n \n \t\t/* Filter with with_commit if specified */\n \t\tif (!is_descendant_of(commit, ref_list->with_commit))\n@@ -484,9 +494,10 @@ static void show_detached(struct ref_list *ref_list)\n \t}\n }\n \n-static void print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit)\n+static int print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit)\n {\n \tint i;\n+\tstruct append_ref_cb cb;\n \tstruct ref_list ref_list;\n \n \tmemset(&ref_list, 0, sizeof(ref_list));\n@@ -496,7 +507,9 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str\n \tref_list.with_commit = with_commit;\n \tif (merge_filter != NO_FILTER)\n \t\tinit_revisions(&ref_list.revs, NULL);\n-\tfor_each_rawref(append_ref, &ref_list);\n+\tcb.ref_list = &ref_list;\n+\tcb.ret = 0;\n+\tfor_each_rawref(append_ref, &cb);\n \tif (merge_filter != NO_FILTER) {\n \t\tstruct commit *filter;\n \t\tfilter = lookup_commit_reference_gently(merge_filter_ref, 0);\n@@ -527,6 +540,8 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str\n \t}\n \n \tfree_ref_list(&ref_list);\n+\n+\treturn cb.ret;\n }\n \n static void rename_branch(const char *oldname, const char *newname, int force)\n@@ -679,7 +694,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tif (delete)\n \t\treturn delete_branches(argc, argv, delete > 1, kinds);\n \telse if (argc == 0)\n-\t\tprint_ref_list(kinds, detached, verbose, abbrev, with_commit);\n+\t\treturn print_ref_list(kinds, detached, verbose, abbrev, with_commit);\n \telse if (rename && (argc == 1))\n \t\trename_branch(head, argv[0], rename > 1);\n \telse if (rename && (argc == 2))\n-- \n1.7.0.4\n"},{"id":"142863","messageId":"1275551282-21557-2-git-send-email-simo.melenius@iki.fi","threadId":"23981","inReplyTo":"20100603065527.GA23439@progeny.tock","subject":"[PATCH 2/2] branch: don't fail listing branches if one of the commits wasn't found","fromName":"Simo Melenius","fromEmail":"simo.melenius@iki.fi","sentAt":"2010-06-03T07:48:02Z","receivedAt":"2010-06-03T07:48:02Z","isPatch":true,"sender":{"key":"simo.melenius@iki.fi","avatar":null},"body":"When listing branches with ref lookups, if one of the known raw refs\ndoesn't point to a commit then \"git branch\" would return error(),\nterminating the whole for_each_rawref() iteration and possibly hiding\nany remaining refs.\n\nSigned-off-by: Simo Melenius <simo.melenius@iki.fi>\n---\n builtin/branch.c |    5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 46ca59c..2242743 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -303,7 +303,7 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,\n \t\tif (!commit)\n \t\t{\n \t\t\tcb->ret = error(\"branch '%s' does not point at a commit\", refname);\n-\t\t\treturn cb->ret;\n+\t\t\treturn 0;\n \t\t}\n \n \t\t/* Filter with with_commit if specified */\n@@ -541,6 +541,9 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n \n \tfree_ref_list(&ref_list);\n \n+\tif (cb.ret)\n+\t\terror(\"some refs could not be read, review stderr\");\n+\n \treturn cb.ret;\n }\n \n-- \n1.7.0.4\n"},{"id":"142914","messageId":"AANLkTinD2OLZllialwyAwuEroCDo9w56Zu_3bhKbf-gg@mail.gmail.com","threadId":"23981","inReplyTo":"1275551282-21557-2-git-send-email-simo.melenius@iki.fi","subject":"Re: [PATCH 2/2] branch: don't fail listing branches if one of the commits wasn't found","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-06-03T17:42:01Z","receivedAt":"2010-06-03T17:42:01Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Thu, Jun 3, 2010 at 09:48, Simo Melenius <simo.melenius@iki.fi> wrote:\n> +       if (cb.ret)\n> +               error(\"some refs could not be read, review stderr\");\n\nI don't think there's any precedence for an error message like this,\nperhaps in git-svn's \"the above error message is nothing to worry\nabout, move along\", which I think is silly as well. I think it's best\nto just s/, review stderr// here.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"142940","messageId":"20100604022405.GA26071@progeny.tock","threadId":"23981","inReplyTo":"1275551282-21557-1-git-send-email-simo.melenius@iki.fi","subject":"Re: [PATCH 1/2] branch: exit status now reflects if branch listing finds an error","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-06-04T02:24:06Z","receivedAt":"2010-06-04T02:24:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Simo Melenius wrote:\n\n> If some refs could not be read when listing branches, this can now be\n> observed in the exit status of the \"git branch\" command.\n\nA noble cause.  Thanks.\n\n> +struct append_ref_cb\n> +{\n> +\tstruct ref_list *ref_list;\n> +\tint ret;\n> +};\n\nnitpick: the brace should go on the same line to match the other\nstructs in builtin/branch.c and elsewhere in git.\n\n> @@ -496,7 +507,9 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str\n>  \tref_list.with_commit = with_commit;\n>  \tif (merge_filter != NO_FILTER)\n>  \t\tinit_revisions(&ref_list.revs, NULL);\n> -\tfor_each_rawref(append_ref, &ref_list);\n> +\tcb.ref_list = &ref_list;\n> +\tcb.ret = 0;\n> +\tfor_each_rawref(append_ref, &cb);\n>  \tif (merge_filter != NO_FILTER) {\n>  \t\tstruct commit *filter;\n>  \t\tfilter = lookup_commit_reference_gently(merge_filter_ref, 0);\n\nThis can be simplified by \"ret = for_each_rawref(append_ref, ...\"\nbut the above would have to be added back anyway for patch 2/2.  So\nI’m happy with this patch as is.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 46ca59c..77ae444 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -257,8 +257,7 @@ static char *resolve_symref(const char *src, const char *prefix)\n \treturn xstrdup(dst);\n }\n \n-struct append_ref_cb\n-{\n+struct append_ref_cb {\n \tstruct ref_list *ref_list;\n \tint ret;\n };\n-- \n"},{"id":"142941","messageId":"20100604024344.GB26311@progeny.tock","threadId":"23981","inReplyTo":"1275551282-21557-2-git-send-email-simo.melenius@iki.fi","subject":"Re: [PATCH 2/2] branch: don't fail listing branches if one of the commits wasn't found","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-06-04T02:43:45Z","receivedAt":"2010-06-04T02:43:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Simo Melenius wrote:\n\n> Signed-off-by: Simo Melenius <simo.melenius@iki.fi>\n\nExcept as pointed out by Sverre[1],\n\n  Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/148253/focus=148350\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 2242743..d2d3c26 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -542,7 +542,7 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n \tfree_ref_list(&ref_list);\n \n \tif (cb.ret)\n-\t\terror(\"some refs could not be read, review stderr\");\n+\t\terror(\"some refs could not be read\");\n \n \treturn cb.ret;\n }\n-- \n"}]}