[PATCH v3 0/2] Replacement for rr/die-on-missing-upstream

[PATCH v3 0/2] Replacement for rr/die-on-missing-upstream

[Ramkumar Ramachandra]

[1/2] clarifies the commit message to say that a grep didn't find
"@{u}" or "@{upstream}" hard-coded by any callers.  Thanks to Junio.

[2/2] fixes a small grammar error in the commit message.  Thanks to
Eric Sunshine.

Ramkumar Ramachandra (2):
  sha1_name: fix error message for @{u}
  sha1_name: fix error message for @{<N>}, @{<date>}

 sha1_name.c                   | 21 +++++++++++++++------
 t/t1507-rev-parse-upstream.sh | 15 +++++----------
 2 files changed, 20 insertions(+), 16 deletions(-)

-- 
1.8.3.rc3.17.gd95ec6c.dirty

[PATCH 1/2] sha1_name: fix error message for @{u}

[Ramkumar Ramachandra]

Currently, when no (valid) upstream is configured for a branch, you get
an error like:

  $ git show @{u}
  error: No upstream configured for branch 'upstream-error'
  error: No upstream configured for branch 'upstream-error'
  fatal: ambiguous argument '@{u}': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions, like this:
  'git <command> [<revision>...] -- [<file>...]'

The "error: " line actually appears twice, and the rest of the error
message is useless.  In sha1_name.c:interpret_branch_name(), there is
really no point in processing further if @{u} couldn't be resolved, and
we might as well die() instead of returning an error().  After making
this change, you get:

  $ git show @{u}
  fatal: No upstream configured for branch 'upstream-error'

Also tweak a few tests in t1507 to expect this output.

To justify that this change is safe, consider that all callers of
interpret_branch_name() have to fall in two categories:

1. Direct end-user facing applications like [rev-parse, show] calling in
   with end-user data (in which case the data can contain "@{u}").
   Failing immediately is the right thing to do: the only difference is
   that the die() happens in interpret_branch_name() instead of
   die_verify_filename(), and this is desirable.

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?  A grep reveals that no part of the code hard-codes either
   "@{u}" or "@{upstream}".  So, these callers will never hit the
   codepath touched by the patch.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sha1_name.c                   | 11 +++++------
 t/t1507-rev-parse-upstream.sh | 15 +++++----------
 2 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 3820f28..61f5a34 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1033,15 +1033,14 @@ 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"));
 	if (!upstream->merge || !upstream->merge[0]->dst) {
 		if (!ref_exists(upstream->refname))
-			return error(_("No such branch: '%s'"), cp);
+			die(_("No such branch: '%s'"), cp);
 		if (!upstream->merge)
-			return error(_("No upstream configured for branch '%s'"),
-				     upstream->name);
-		return error(
-			_("Upstream branch '%s' not stored as a remote-tracking branch"),
+			die(_("No upstream configured for branch '%s'"),
+				upstream->name);
+		die(_("Upstream branch '%s' not stored as a remote-tracking branch"),
 			upstream->merge[0]->src);
 	}
 	free(cp);
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}
 	EOF
 	error_message non-tracking@{u} 2>actual &&
 	test_i18ncmp expect actual
@@ -138,8 +137,7 @@ test_expect_success 'branch@{u} error message when no upstream' '
 
 test_expect_success '@{u} error message when no upstream' '
 	cat >expect <<-EOF &&
-	error: No upstream configured for branch ${sq}master${sq}
-	fatal: Needed a single revision
+	fatal: No upstream configured for branch ${sq}master${sq}
 	EOF
 	test_must_fail git rev-parse --verify @{u} 2>actual &&
 	test_i18ncmp expect actual
@@ -147,8 +145,7 @@ test_expect_success '@{u} error message when no upstream' '
 
 test_expect_success 'branch@{u} error message with misspelt branch' '
 	cat >expect <<-EOF &&
-	error: No such branch: ${sq}no-such-branch${sq}
-	fatal: Needed a single revision
+	fatal: No such branch: ${sq}no-such-branch${sq}
 	EOF
 	error_message no-such-branch@{u} 2>actual &&
 	test_i18ncmp expect actual
@@ -156,8 +153,7 @@ test_expect_success 'branch@{u} error message with misspelt branch' '
 
 test_expect_success '@{u} error message when not on a branch' '
 	cat >expect <<-EOF &&
-	error: HEAD does not point to a branch
-	fatal: Needed a single revision
+	fatal: HEAD does not point to a branch
 	EOF
 	git checkout HEAD^0 &&
 	test_must_fail git rev-parse --verify @{u} 2>actual &&
@@ -166,8 +162,7 @@ test_expect_success '@{u} error message when not on a branch' '
 
 test_expect_success 'branch@{u} error message if upstream branch not fetched' '
 	cat >expect <<-EOF &&
-	error: Upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch
-	fatal: Needed a single revision
+	fatal: Upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch
 	EOF
 	error_message bad-upstream@{u} 2>actual &&
 	test_i18ncmp expect actual
-- 
1.8.3.rc3.17.gd95ec6c.dirty

[PATCH 2/2] sha1_name: fix error message for @{<N>}, @{<date>}

[Ramkumar Ramachandra]

Currently, when we try to resolve @{<N>} or @{<date>} when the reflog
doesn't go back far enough, we get errors like:

  # on branch master
  $ git show @{10000}
  fatal: Log for '' only has 7 entries.

  $ git show @{10000.days.ago}
  warning: Log for '' only goes back to Tue, 21 May 2013 14:14:45 +0530.
  ...

  # detached HEAD case
  $ git show @{10000}
  fatal: Log for '' only has 2005 entries.

  $ git show master@{10000}
  fatal: Log for 'master' only has 7 entries.

The empty string '' is ugly, inconsistent, and fails to convey
information about whose logs we are inspecting.  Change this so that we
get:

  # on branch master
  $ git show @{10000}
  fatal: Log for 'master' only has 7 entries.

  $ git show @{10000.days.ago}
  warning: Log for 'master' only goes back to Tue, 21 May 2013 14:14:45 +0530.
  ...

  # detached HEAD case
  $ git show @{10000}
  fatal: Log for 'HEAD' only has 2005 entries.

  $ git show master@{10000}
  fatal: Log for 'master' only has 7 entries.

Simple, consistent, and informative; suitable for output even from
plumbing commands like rev-parse.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sha1_name.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/sha1_name.c b/sha1_name.c
index 61f5a34..6928cc7 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -517,6 +517,16 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		}
 		if (read_ref_at(real_ref, at_time, nth, sha1, NULL,
 				&co_time, &co_tz, &co_cnt)) {
+			if (!len) {
+				if (!prefixcmp(real_ref, "refs/heads/")) {
+					str = real_ref + 11;
+					len = strlen(real_ref + 11);
+				} else {
+					/* detached HEAD */
+					str = "HEAD";
+					len = 4;
+				}
+			}
 			if (at_time)
 				warning("Log for '%.*s' only goes "
 					"back to %s.", len, str,
-- 
1.8.3.rc3.17.gd95ec6c.dirty

Re: [PATCH 2/2] sha1_name: fix error message for @{<N>}, @{<date>}

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Currently, when we try to resolve @{<N>} or @{<date>} when the reflog
> doesn't go back far enough, we get errors like:
>
>   # on branch master
>   $ git show @{10000}
>   fatal: Log for '' only has 7 entries.
>
>   $ git show @{10000.days.ago}
>   warning: Log for '' only goes back to Tue, 21 May 2013 14:14:45 +0530.
>   ...
>
>   # detached HEAD case
>   $ git show @{10000}
>   fatal: Log for '' only has 2005 entries.
>
>   $ git show master@{10000}
>   fatal: Log for 'master' only has 7 entries.
>
> The empty string '' is ugly, inconsistent, and fails to convey
> information about whose logs we are inspecting.  Change this so that we

What is this meant to update?  I recall rewriting this part on
purpose.

Even though it appears unusual and invites confusion, it is very
consistent to say '' when the user asks for @... as we say 'master'
when the user asks for master@...; "inconsistent" is a very bad
label for it.

Re: [PATCH 2/2] sha1_name: fix error message for @{<N>}, @{<date>}

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> What is this meant to update?  I recall rewriting this part on
> purpose.

I was being verbose to show that I handle the detached HEAD case too,
which I missed last time.

> Even though it appears unusual and invites confusion, it is very
> consistent to say '' when the user asks for @... as we say 'master'
> when the user asks for master@...; "inconsistent" is a very bad
> label for it.

The inconsistency refers to these two:

>>   # on branch master
>>   $ git show @{10000}
>>   fatal: Log for '' only has 7 entries.

>>   # detached HEAD case
>>   $ git show @{10000}
>>   fatal: Log for '' only has 2005 entries.

Is the commit message unclear?

Re: [PATCH 2/2] sha1_name: fix error message for @{<N>}, @{<date>}

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> What is this meant to update?  I recall rewriting this part on
>> purpose.
>
> I was being verbose to show that I handle the detached HEAD case too,
> which I missed last time.
>
>> Even though it appears unusual and invites confusion, it is very
>> consistent to say '' when the user asks for @... as we say 'master'
>> when the user asks for master@...; "inconsistent" is a very bad
>> label for it.
>
> The inconsistency refers to these two:
>
>>>   # on branch master
>>>   $ git show @{10000}
>>>   fatal: Log for '' only has 7 entries.
>
>>>   # detached HEAD case
>>>   $ git show @{10000}
>>>   fatal: Log for '' only has 2005 entries.
>
> Is the commit message unclear?

Very unclear.  You said

    The empty string '' is ugly, inconsistent, and fails to convey
    information...

I was reacting to "inconsistent" by pointing out that showing an
empty string is very consistent.

I was _not_ saying that "'' must be kept because it is consistent",
of course; that is a consistency that is not useful.  Which means
inconsistency that does not exist is a reason to rewrite it, nor the
rewrite is to gain consistency.  It is to make the result more useful.

That is why I rewrote it like so:

    The empty string '' is confusing and does not convey information
    about whose logs we are inspecting.  Change this so that...

Re: [PATCH 2/2] sha1_name: fix error message for @{<N>}, @{<date>}

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> That is why I rewrote it like so:
>
>     The empty string '' is confusing and does not convey information
>     about whose logs we are inspecting.  Change this so that...

Ah, I didn't notice the rewrite in pu.  Thanks.