Re: [PATCH] branch: allow "-" as a short-hand for "previous branch"

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 16, 2022, 09:49 UTC
Message-ID: <5194s6qn-570s-6053-2104-9s22qo1874sn@tzk.qr>
In-Reply-To: <xmqqk07iu3c3.fsf@gitster.g>

Hi Junio,

On Mon, 8 Aug 2022, Junio C Hamano wrote:

Show 19 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > @@ -1420,6 +1420,12 @@ static int interpret_nth_prior_checkout(struct repository *r,
> >  	const char *brace;
> >  	char *num_end;
> >
> > +	if (namelen == 1 && *name == '-') {
> > +		brace = name;
> > +		nth = 1;
> > +		goto find_nth_checkout;
> > +	}
> > +
> >  	if (namelen < 4)
> >  		return -1;
> >  	if (name[0] != '@' || name[1] != '{' || name[2] != '-')
>
> If a solution along this line works, it would be far cleaner design
> than the various hacks we have done in the past, noticing "-" and
> replacing with "@{-1}".

Indeed, but it does not work as-is: `interpret_nth_prior_checkout()` is used on prefixes of a rev, and for the special handling of `-` we cannot have that.

To illustrate what I mean: `-` should not be idempotent to `@{-1}` because we want to allow things like `@{-1}^2~15` but we do not want `-^2~15` to be a synonym for that.

Therefore, the layer where this `-` handling needs to happen is somewhere above `interpret_nth_prior_checkout()`, but still well below `delete_branches()`.

Show 11 quoted lines

> For one thing, we wouldn't be receiving a "-" from the end user on the
> command line and in response say @{-1} does not make sense in the
> context in an error message.  That alone makes the above approach to
> deal with it at the lowest level quite attractive.
>
> In the list archive, however, you may be able to find a few past
> discussions on why this is not a good idea (some of which I may no
> longer agree with).  One thing that still worries me a bit is that
> we often disambiguate the command line arguments by seeing "is this
> (still) a rev, or is this a file, or can it be interpreted as both?"
> and "-" is not judged to be a "rev", IIRC.

I haven't had the time to perform a thorough analysis (and hoped that Rubén would rise up to the challenge), but I have not seen a lot of places where `-` would be ambiguous, especially when taking into account that revision and file name arguments can be separated via `--`.

One thing we could do, however, would be to patch only `repo_interpret_branch_name()`, i.e. only allow `-` to imply the previous branch name in invocations where a branch name is asked for _explicitly_. I.e. not any random revision, but specifically a branch name.

This would address all of the `git branch` operations we care about, and leave invocations like `git diff -` unaddressed (which might be more confusing than we want it to be).

Show 11 quoted lines

> Luckily, not many commands we have take "-" as if it were a file and
> make it read from the standard input stream, but if there were (or
> if we were to add a command to behave like so), treating "-" to mean
> the same thing as "@{-1}" everywhere may require the "does this look
> like a rev?"  heuristics (which is used by the "earlier ones must be
> rev and not file, later ones must be file and cannot be interpreted
> as rev, for you to omit '--' from the command line" logic) to be
> taught that a lone "-" can be a rev.
>
> So it is quite a lot of thing that the new code needs to get right
> before getting there.

I am not claiming that it will be easy to perform that analysis. It will be worth the effort, though, I am sure.

And it will be necessary because the current approach of special-special-casing `git branch -d -` is just too narrow, and a recipe for totally valid complaints by users.

Ciao, Dscho