Ramkumar Ramachandra <artagnon@xxxxxxxxx> writes:
> 2. Callers calling in with programmatic data, and expecting the function
> to return and not die(). In this case, why would anyone ever
> construct a string containing "@{u}" programmatically in the first
> place?
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.
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().
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.
> Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
> ---
> sha1_name.c | 13 +++++++------
> t/t1507-rev-parse-upstream.sh | 15 +++++----------
> 2 files changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 3820f28..416a673 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -1033,14 +1033,15 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
> * points to something different than a branch.
> */
> if (!upstream)
> - return error(_("HEAD does not point to a branch"));
> + die(_("HEAD does not point to a branch"));
OK.
> if (!upstream->merge || !upstream->merge[0]->dst) {
> if (!ref_exists(upstream->refname))
> + die(_("No such branch: '%s'"), cp);
OK.
> + if (!upstream->merge) {
> + die(_("No upstream configured for branch '%s'"),
> + upstream->name);
> + }
OK, but I would not add extra {} if I were doing this change.
> + 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.
> diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
> index b27a720..2a19e79 100755
> --- a/t/t1507-rev-parse-upstream.sh
> +++ b/t/t1507-rev-parse-upstream.sh
> @@ -129,8 +129,7 @@ test_expect_success 'branch@{u} works when tracking a local branch' '
>
> test_expect_success 'branch@{u} error message when no upstream' '
> cat >expect <<-EOF &&
> - error: No upstream configured for branch ${sq}non-tracking${sq}
> - fatal: Needed a single revision
> + fatal: No upstream configured for branch ${sq}non-tracking${sq}
Much nicer.
--
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