Hi Junio,
On Mon, 8 Aug 2022, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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()`.
> 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).
> 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
