Re: about rev-parse's quietness

<stefan.naewe@atlas-elektronik.com> writes:

Yup, that is unfortunate. The above two pass different codepaths in sha1_name.c::get_sha1_basic().

 * The code first notices "master@{anything}" has @{anything}
   suffix, which could be @{123} (counted reflog), @{1.hour.ago}
   (timed reflog), @{-1} (prior checkout---valid only when nothing
   comes before "@{...}") or @{upstream}/@{push}.

 * The first two goes through dwim_log(), together with
   'master@{xyz}' to locate the reflog of 'master' (i.e. what comes
   before that @{anything}) and identifies that we need to look at
   the log for 'refs/heads/master', looks up an appropriate entry,
   and the processing ends there, returning success.  However, 'xyz'
   is not a valid specification for either counted or timed reflog,
   so the function returns failure for 'master@{xyz}', letting the
   remainder of the callchain to take care of it.
 
 * The third one goes to interpret_nth_prior_checkout() and the
   processing ends there, returning success.

 * The last one goes to dwim_ref(), together with just 'master'
   without any @{anything}.  It tries to turn master@{upstream} or
   master@{push} to refs/remotes/origin/master and then it is used
   to grab the object name pointed by it.

The helper function dwim_ref() calls to turn master@{upstream} to refs/remotes/origin/master, interpret_branch_name(), is designed to die() when fed a ref without upstream (or @{push} when there is no such thing).

Naively, one would imagine that the die() can be demoted to an internal error return to the caller (just like 'master@{xyz}' is rejected silently in the dwim_log() codepath) and everything else should go well. It is unfortunately not sufficient, though, as the other callers to interpret_branch_name() rely on the function to die and are not prepared to show the error messages themselves.

So I think this is fixable and the right approach to fix it is as hinted above, i.e.

 * audit callers of interpret_branch_name() and find the ones that
   depend on the function to die (i.e. it does not handle the case
   where interpret_branch_name() notices that 'master@{upstream}'
   does not exist).  Change them to die with an appropriate message
   themselves.

 * change interpret_branch_mark(), which is actually where the die()
   happens, so that it can notice a request to use 'master@{upstream}' 
   when there is no such upstream and signal an error to the caller.

I am not saying that you should be the one to fix it, though ;-) The above is to help those who may be interested to look into the codepath to do so.

Thanks.