[PATCH v2 0/2] Fix invalid revision error messages

[PATCH v2 0/2] Fix invalid revision error messages

[Ramkumar Ramachandra]

As Junio pointed out in [0/2], this is not for 1.8.3; it's just a
regular "enhacement".

In [1/2], I've extended the commit message with the justification I
wrote out for Junio.

In [2/2], I've made sure to print the "correct" error message
everytime: I missed the detached HEAD case last time.  I'm not in
favor of anything "prettier", as I already explained in my email.

Thanks.

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

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

-- 
1.8.3.rc3.10.g6f8d616

[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?  So, these callers can never hit the codepath touched by this
   patch, and the change does not affect them.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 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"));
 	if (!upstream->merge || !upstream->merge[0]->dst) {
 		if (!ref_exists(upstream->refname))
-			return error(_("No such branch: '%s'"), cp);
-		if (!upstream->merge)
-			return error(_("No upstream configured for branch '%s'"),
-				     upstream->name);
-		return error(
+			die(_("No such branch: '%s'"), cp);
+		if (!upstream->merge) {
+			die(_("No upstream configured for branch '%s'"),
+				upstream->name);
+		}
+		die(
 			_("Upstream branch '%s' not stored as a remote-tracking branch"),
 			upstream->merge[0]->src);
 	}
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.10.g6f8d616

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> 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@gmail.com>
> ---
>  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.

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

[Ramkumar Ramachandra]

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.

[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 failing 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 416a673..b7e008a 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.10.g6f8d616

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

[Eric Sunshine]

On Wed, May 22, 2013 at 6:39 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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 failing to convey

s/failing/fails/

> 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>

[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 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.

[PATCH 0/2] Fix invalid revision error messages for 1.8.3

[Ramkumar Ramachandra]

Hi,

Seeing other patches on the list, I decided that I should do something
for 1.8.3 as well (as opposed to constantly writing new features).  So
here's my contribution.

The first error message has annoyed me endlessly, and I took this
opportunity to fix it.  Interested people can sprinkle in some advice
later.  The second one is a low-hanging "while we're there".

Thanks.

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

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

-- 
1.8.3.rc3.6.ga9126d5.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
for the current branch doesn't go back far enough, we get errors like:

  $ 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.
  ...

The empty string '' looks ugly and inconsistent with the output of
<branch>@{<N>}.  Replace it with the string 'current branch'.

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

diff --git a/sha1_name.c b/sha1_name.c
index 416a673..683b4bd 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -517,6 +517,10 @@ 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) {
+				str = "current branch";
+				len = strlen("current branch");
+			}
 			if (at_time)
 				warning("Log for '%.*s' only goes "
 					"back to %s.", len, str,
-- 
1.8.3.rc3.6.ga9126d5.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
> for the current branch doesn't go back far enough, we get errors like:
>
>   $ 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.
>   ...
>
> The empty string '' looks ugly and inconsistent with the output of
> <branch>@{<N>}.  Replace it with the string 'current branch'.

Wouldn't that be '*the* current branch'?

More importantly, doesn't "real_ref" have the name of the branch?

Suppose the user said "git show @{10000}" instead of "git show
master@{10000}" while on 'master'.

It could be argued that it may look nicer to say "your current
branch does not have enough update history" instead of saying
"master does not..." (i.e. different input to ask for the same
thing, different output depending on the way the user asked).  It
also could be argued that they should produce the same diagnosis
that is more informative.

I am slightly leaning toward the latter.

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  sha1_name.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 416a673..683b4bd 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -517,6 +517,10 @@ 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) {
> +				str = "current branch";
> +				len = strlen("current branch");
> +			}
>  			if (at_time)
>  				warning("Log for '%.*s' only goes "
>  					"back to %s.", len, str,

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

[Kevin Bracey]

On 21/05/2013 19:52, Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> The empty string '' looks ugly and inconsistent with the output of
>> <branch>@{<N>}.  Replace it with the string 'current branch'.
> Wouldn't that be '*the* current branch'?
>
> More importantly, doesn't "real_ref" have the name of the branch?
>
> Suppose the user said "git show @{10000}" instead of "git show
> master@{10000}" while on 'master'.
>
> It could be argued that it may look nicer to say "your current
> branch does not have enough update history" instead of saying
> "master does not..." (i.e. different input to ask for the same
> thing, different output depending on the way the user asked).  It
> also could be argued that they should produce the same diagnosis
> that is more informative.
>
> I am slightly leaning toward the latter.
That would also avoid the complaint I was about to make that putting 
'current branch' in scare quotes would be annoying.

Kevin

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> More importantly, doesn't "real_ref" have the name of the branch?
>
> Suppose the user said "git show @{10000}" instead of "git show
> master@{10000}" while on 'master'.

My stupidity, sorry.

> It could be argued that it may look nicer to say "your current
> branch does not have enough update history" instead of saying
> "master does not..." (i.e. different input to ask for the same
> thing, different output depending on the way the user asked).  It
> also could be argued that they should produce the same diagnosis
> that is more informative.

Yeah, I wanted to discuss this: the problem is that even something as
low-level as rev-list will print this "pretty" error.  It's certainly
useful for porcelain.  How do we achieve this?  An extra
"is-porcelain" argument?