v2, 2 partsFix invalid revision error messages

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

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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.

OK.

OK.

OK, but I would not add extra {} if I were doing this change.

OK, but I would fix the indentation while at it if I were doing this change.

Much nicer.

Junio C Hamano wrote:

I'll mention that a simple grep for "@{u}" and "@{upstream}" found nothing.

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

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}"?

But my Emacs reports that the indentation is correct.  Did you mean:

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(+)

On Wed, May 22, 2013 at 6:39 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

s/failing/fails/