On 3 June 2010 07:22, Jonathan Nieder <jrnieder@xxxxxxxxx> 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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html