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

[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 1/2] sha1_name: fix error message for @{u}

[Ramkumar Ramachandra]

Currently, when no (valid) upstream is configured for a branch, we 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.

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.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 0/2] Fix invalid revision error messages for 1.8.3

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Seeing other patches on the list, I decided that I should do something
> for 1.8.3 as well

Fixes to something that are broken the same way between 'master' and
older release versions are the same as enhancements (which you can
view as "fix to lack of feature").  They are not regression fixes
and not for 1.8.3 at this point in the cycle, deep into -rc.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Currently, when no (valid) upstream is configured for a branch, we 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.

Does a failure in interpret-branch-name that issue these error
messages always followed by die() in the caller?  I know you looked
at the cases you noticed as an end-user (like the above "git show @{u}"
example), but if some codepaths did this:

	if (interpret-branch-name()) {
        	you do not seem to have upstream defined,
	        so I will helpfully do something else that
                you probably have meant.
	}

this patch will break that codepath you did not look.

I do not offhand know if there is such a codepath, so if you did a
code audit and know this patch is regression-free, please say that
in the log message.  "I ran all the tests and they passed" is not
good enough.

Other than that, the idea sounds OK.

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

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 0/2] Fix invalid revision error messages for 1.8.3

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Fixes to something that are broken the same way between 'master' and
> older release versions are the same as enhancements (which you can
> view as "fix to lack of feature").  They are not regression fixes
> and not for 1.8.3 at this point in the cycle, deep into -rc.

If we view them as enhancements, well and good.  Let's polish them
until we're really happy with them: they're written with the "minimal,
but correct" philosophy, because the -rc3 window is too small for a
review.

Just to share opinion, they looked like "bugs" to me, because it's not
about "improving" the error messages; it's about correcting a defect.
The author could not have possibly intended two "error: " lines in the
first one, or an empty string in the second one.  At some point in the
past, the behavior must have been different (a "feature" must have
introduced these problems: like implicit HEAD for @{<N>}): the
"regression" was introduced in the version after that.  So, is it
because that version was too long ago that we don't consider it a
regression (do we backport fixes)?

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Does a failure in interpret-branch-name that issue these error
> messages always followed by die() in the caller?  I know you looked
> at the cases you noticed as an end-user (like the above "git show @{u}"
> example), but if some codepaths did this:
>
>         if (interpret-branch-name()) {
>                 you do not seem to have upstream defined,
>                 so I will helpfully do something else that
>                 you probably have meant.
>         }
>
> this patch will break that codepath you did not look.

How can that ever happen in a non end-user case?  That failure
requires a string containing "@{u}" to be constructed and passed as an
argument.  Why would we ever programmatically construct "@{u}" to find
the upstream?

To put it another way: unless an end-user facing application finds an
"@{u}" while parsing argv and passes it on to interpret-branch-name,
isn't it impossible for an "@{u}" to end up in the argument?

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> So, is it
> because that version was too long ago that we don't consider it a
> regression (do we backport fixes)?

The "regression fixes" pre-release -rc period is for is to make sure
to avoid unwanted/unintended behaviour changes between releases.

People have _already_ seen and lived with these issues in released
versions.  Changing it may or may not be getting it back to the
state to that of an even older release, but at that point the
differences do not matter.  It is a "fix", too late for the kind of
regression fixes we focus during _this_ -rc period, which is about
regressions between v1.8.2 and 'master'.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> Does a failure in interpret-branch-name that issue these error
>> messages always followed by die() in the caller?  I know you looked
>> at the cases you noticed as an end-user (like the above "git show @{u}"
>> example), but if some codepaths did this:
>>
>>         if (interpret-branch-name()) {
>>                 you do not seem to have upstream defined,
>>                 so I will helpfully do something else that
>>                 you probably have meant.
>>         }
>>
>> this patch will break that codepath you did not look.
>
> How can that ever happen in a non end-user case?  That failure
> requires a string containing "@{u}" to be constructed and passed as an
> argument.  Why would we ever programmatically construct "@{u}" to find
> the upstream?
>
> To put it another way: unless an end-user facing application finds an
> "@{u}" while parsing argv and passes it on to interpret-branch-name,
> isn't it impossible for an "@{u}" to end up in the argument?

So did you or did you not audit the codepath?

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> So did you or did you not audit the codepath?

No; I was explaining why I didn't in the first place.  Going through it now.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> So did you or did you not audit the codepath?
>
> No; I was explaining why I didn't in the first place.  Going through it now.

I did not mean "You must do so or we should discard the patch".  I
just wanted to make sure the log messages say how firmly the change
is backed.

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?

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> People have _already_ seen and lived with these issues in released
> versions.  Changing it may or may not be getting it back to the
> state to that of an even older release, but at that point the
> differences do not matter.  It is a "fix", too late for the kind of
> regression fixes we focus during _this_ -rc period, which is about
> regressions between v1.8.2 and 'master'.

Makes sense.

On a related note, I really wonder why people run anything < master
git; it's so easy to compile and use from ~.  The idea isn't insane at
all: most people run a -p ruby from ~ using things like rbenv (yes, it
compiles from source).

(ofcourse servers have to run a release)

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

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> Junio C Hamano wrote:
>> So did you or did you not audit the codepath?
>
> No; I was explaining why I didn't in the first place.  Going through it now.

So, this is what I have:

interpret_branch_name -> interpret_branch_name (recursion)
                      -> get_sha1_basic -> get_sha1 [context] (end-user data)
                      -> substitute_branch_name -> dwim (end-user data)
		      -> strbuf_branchname (callers pass a branch name; no @{u})
		      -> revision.c:add_pending_object [with_mode] (end-user data)

[die_]verify_filename -> builtin/rev-parse.c (end-user)
		      -> builtin/reset.c (end-user)
		      -> builtin/grep.c:cmd_grep (end-user)
		      -> revision.c:setup_revisions (end-user data)

We used to die in die_verify_filename() earlier, but we die in
interpret_branch_name() after the patch.  Do we have to dig deeper?

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Ramkumar Ramachandra wrote:
>> Junio C Hamano wrote:
>>> So did you or did you not audit the codepath?
>>
>> No; I was explaining why I didn't in the first place.  Going through it now.
>
> So, this is what I have:
>
> interpret_branch_name -> interpret_branch_name (recursion)
>                       -> get_sha1_basic -> get_sha1 [context] (end-user data)
>                       -> substitute_branch_name -> dwim (end-user data)
> 		      -> strbuf_branchname (callers pass a branch name; no @{u})
> 		      -> revision.c:add_pending_object [with_mode] (end-user data)
>
> [die_]verify_filename -> builtin/rev-parse.c (end-user)
> 		      -> builtin/reset.c (end-user)
> 		      -> builtin/grep.c:cmd_grep (end-user)
> 		      -> revision.c:setup_revisions (end-user data)

It seems that you are digging in the wrong direction?  I was worried
about the callers of interpret_branch_name().

But whatever.

I looked at the callers myself while waiting for the test suite to
pass for five integration branches and I think the patch is safe.
There were some silent error returns from the function but your
patch did not touch them (which is good).

> We used to die in die_verify_filename() earlier, but we die in
> interpret_branch_name() after the patch.

I think that is a desired outcome.  Thanks.

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>> interpret_branch_name -> interpret_branch_name (recursion)
>>                       -> get_sha1_basic -> get_sha1 [context] (end-user data)
>>                       -> substitute_branch_name -> dwim (end-user data)
>>                     -> strbuf_branchname (callers pass a branch name; no @{u})
>>                     -> revision.c:add_pending_object [with_mode] (end-user data)
>>
>> [die_]verify_filename -> builtin/rev-parse.c (end-user)
>>                     -> builtin/reset.c (end-user)
>>                     -> builtin/grep.c:cmd_grep (end-user)
>>                     -> revision.c:setup_revisions (end-user data)
>
> It seems that you are digging in the wrong direction?  I was worried
> about the callers of interpret_branch_name().

Um, aren't interpret_branch_name, get_sha1_basic,
substitute_branch_name, strbuf_branchname, and add_pending_object the
five callers of interpret_branch_name?  I've tried to show how they
are called with either end-user data or programmatic data without a
"@{u}".  What am I missing?

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> "@{u}".  What am I missing?

You draw the arrow the other way around, that is what made the text
confusing.

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