Junio C Hamano wrote:
> If you have to ask why, and cannot answer the question yourself,
> then you would not know if such a caller exists. After a code
> audit, I know there is no such caller that appends @{u} but if you
> were writing a quick-and-dirty caller, I would not be surprised if
> you find it more convenient to form a textual extended SHA-1
> expression and have get_sha1() do its work, instead of asking the
> same question programmatically.
I'll mention that a simple grep for "@{u}" and "@{upstream}" found nothing.
> In this case, I think you already checked there is no such problem,
> and it is a more straight-forward justification to say that you did
> a code-audit and made sure that all the callers that used to hit one
> of these errors() want to die().
It's not about callers eventually wanting to die(); it's about whether
callers want to die() without doing anything useful after getting this
-1. And that is impossible for me to say with confidence, unless I do
a _very_ extensive code review (which I didn't do).
> Also such a caller, if existed, would either
>
> (1) want to die itself, in which case these error() messages are
> superfluous; or
>
> (2) want to continue (possibly dying with its own message), in
> which case these error() messages are unwanted.
>
> Because you are changing only the existing call sites of error()
> into die(), and not changing silent -1 returns to die(), this change
> is safe for both kinds of such callers, I think.
Take the example of git branch (-vv) output: let's imagine a universe
in which it determines upstream by calling in with a hard-coded "@{u}"
string. Should the entire program die() and stop printing the rest of
the branches? Ofcourse not. Is your argument that no caller should
do this in the first place, because of spurious error() messages
polluting the output (of git branch)? How is this argument stronger
than my grep for "@{u}"?
>> + die(
>> _("Upstream branch '%s' not stored as a remote-tracking branch"),
>> upstream->merge[0]->src);
>
> OK, but I would fix the indentation while at it if I were doing this change.
But my Emacs reports that the indentation is correct. Did you mean:
diff --git a/sha1_name.c b/sha1_name.c
index b7e008a..b00ea0f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1051,8 +1051,7 @@ int interpret_branch_name(const char *name,
struct strbuf *buf)
die(_("No upstream configured for branch '%s'"),
upstream->name);
}
- die(
- _("Upstream branch '%s' not stored as a remote-tracking branch"),
+ die(_("Upstream branch '%s' not stored as a remote-tracking branch"),
upstream->merge[0]->src);
}
free(cp);
Yeah, I'll do this.
--
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