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

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

[Rubén Justo via GitGitGadget]

From: rjusto <rjusto@gmail.com>

Align "branch" with the intuitive use of "-" as a short-hand
for "@{-1}", like in "checkout" and "merge" commands.

$ git branch -d -      # short-hand for: "git branch -d @{-1}"
$ git branch -D -      # short-hand for: "git branch -D @{-1}"

Signed-off-by: rjusto <rjusto@gmail.com>
---
    branch: allow "-" as a short-hand for "previous branch"
    
    Align "branch" with the intuitive use of "-" as a short-hand for
    "@{-1}", like in "checkout" and "merge" commands.
    
    $ git branch -d - # short-hand for: "git branch -d @{-1}" $ git branch
    -D - # short-hand for: "git branch -D @{-1}"
    
    Signed-off-by: rjusto rjusto@gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1315%2Frjusto%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1315/rjusto/master-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1315

 builtin/branch.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e998..59c19f38d2e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -241,6 +241,10 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			die(_("Couldn't look up commit object for HEAD"));
 	}
 
+	if ((argc == 1) && !strcmp(argv[0], "-")) {
+		argv[0] = "@{-1}";
+	}
+
 	for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
 		char *target = NULL;
 		int flags = 0;

base-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9
-- 
gitgitgadget

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2868 bytes --]

Hi Rubén,

On Sun, 7 Aug 2022, Rubén Justo via GitGitGadget wrote:

> From: rjusto <rjusto@gmail.com>
>
> Align "branch" with the intuitive use of "-" as a short-hand
> for "@{-1}", like in "checkout" and "merge" commands.
>
> $ git branch -d -      # short-hand for: "git branch -d @{-1}"
> $ git branch -D -      # short-hand for: "git branch -D @{-1}"

A valuable goal!

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 55cd9a6e998..59c19f38d2e 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -241,6 +241,10 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,

Touching only the `delete_branches()` function suggests that other
commands are left as before, e.g. `git branch --unset-upstream -` would
probably fail.

That's fine, but the commit message claims that the `"-"` special-casing
is introduced for the `git branch` command, not just for `git branch -d`.

>  			die(_("Couldn't look up commit object for HEAD"));
>  	}

At this stage, we already handled the `--remotes` flag, therefore I think
that this patch does not do the intended thing for this command-line:

	git branch -d --remotes -

>
> +	if ((argc == 1) && !strcmp(argv[0], "-")) {
> +		argv[0] = "@{-1}";
> +	}

This means that we only handle `git branch -d -`, but not `git branch -d
some-branch - some-other-branch`.

Could you fix that?

My thinking is that this probably should be a sibling of the `@{-1}`
handling, most likely somewhat like this (I only compile-tested this
patch, please take it from here):

-- snip --
diff --git a/object-name.c b/object-name.c
index 4d2746574cd..ae6c2ed7b83 100644
--- a/object-name.c
+++ b/object-name.c
@@ -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] != '-')
@@ -1432,6 +1438,8 @@ static int interpret_nth_prior_checkout(struct repository *r,
 		return -1;
 	if (nth <= 0)
 		return -1;
+
+find_nth_checkout:
 	cb.remaining = nth;
 	cb.sb = buf;

-- snap --

Naturally, this has much bigger ramifications than just `git branch -d -`,
and might even obsolete some `-` special-casing elsewhere; I have not
looked to see if there is any such special-casing, and would like to ask
you to see whether you can find those and remove them in separate commits
after implementing (and testing) the above
`interpret_nth_prior_checkout()` approach.

Thanks,
Johannes

> +
>  	for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
>  		char *target = NULL;
>  		int flags = 0;
>
> base-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9
> --
> gitgitgadget
>

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

[Junio C Hamano]

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

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.

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

[Rubén Justo]

On Mon, Aug 8, 2022 at 6:06 PM Junio C Hamano <gitster@pobox.com 
<mailto:gitster@pobox.com>> wrote:
 >
 > Johannes Schindelin <Johannes.Schindelin@gmx.de 
<mailto: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}".  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.
 >
 > 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.

Agree. To make a substitution in the command line and to consider "-"
in interpret_nth_prior_checkout, I see them as two very different games.

Previous to this, I thought about making also a "git diff -",
https://github.com/gitgitgadget/git/pull/1314 
<https://github.com/gitgitgadget/git/pull/1314>. Suddenly there was 5
commands with this substitution (checkout, merge, rebase, branch, diff)
so I follow a little the path now Johannes suggests, making the
substitution "- ~ @{-1}" deep in the system. For me, the implications,
error cases, test cases... to consider was not worth the change to
get what I was looking for: align the workflow "checkout/merge/branch
-d".

Also discarded the "git diff -" change, because of so many flags and
conditions "diff" has. So I only sent the "branch -" patch.

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

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> Previous to this, I thought about making also a "git diff -",
> ...
> Also discarded the "git diff -" change, because of so many flags and
> conditions "diff" has. So I only sent the "branch -" patch.

Yeah, that is why the approach Johannes showed, i.e. instead of
making "in this context but not in that context, '-' means
'previous'" changes to random commands, teaching anybody that takes
@{-1} to understand that '-' means 'previous' everywhere, appears
very tempting.

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3519 bytes --]

Hi Junio,

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

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

> 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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 4019 bytes --]

Hi Junio & Rubén,

On Tue, 16 Aug 2022, Johannes Schindelin wrote:

> On Mon, 8 Aug 2022, Junio C Hamano wrote:
>
> > 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.
>
> [...]
>
> 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 patch does that:

-- snip --
diff --git a/object-name.c b/object-name.c
index 4d2746574cd..a2732be3b71 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1616,6 +1616,20 @@ int repo_interpret_branch_name(struct repository *r,
 	if (!namelen)
 		namelen = strlen(name);

+	if (namelen == 1 && *name == '-') {
+		struct grab_nth_branch_switch_cbdata cb = {
+			.remaining = 1,
+			.sb = buf
+		};
+
+		if (refs_for_each_reflog_ent_reverse(get_main_ref_store(r),
+						     "HEAD",
+						     grab_nth_branch_switch,
+						     &cb) <= 0)
+			return -1;
+		return namelen;
+	}
+
 	if (!options->allowed || (options->allowed & INTERPRET_BRANCH_LOCAL)) {
 		len = interpret_nth_prior_checkout(r, name, namelen, buf);
 		if (!len) {
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 993a6b5eff7..5acbef95913 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -67,6 +67,22 @@ test_expect_success 'delete branch via @{-1}' '
 	expect_deleted previous-del
 '

+test_expect_success 'delete branch via -' '
+	git checkout -b previous-del &&
+	git checkout - &&
+
+	git branch -d - &&
+	expect_deleted previous-del &&
+
+	git branch previous-del2 &&
+	git checkout -b previous-del &&
+	git checkout - &&
+
+	git branch -d previous-del2 - &&
+	expect_deleted previous-del &&
+	expect_deleted previous-del2
+'
+
 test_expect_success 'delete branch via local @{upstream}' '
 	git branch local-del &&
 	git branch --set-upstream-to=local-del &&
-- snap --

This adds support for things like `git branch -d -`, and it even verifies
that that works.

What does not work with this patch is `git show -`. I am not sure whether
we want to make that work, although I have to admit that I would use it.
Often. And this patch would make it work, the test suite even passes with
it:

-- snip --
diff --git a/revision.c b/revision.c
index f4eee11cc8b..207b554aef1 100644
--- a/revision.c
+++ b/revision.c
@@ -2802,7 +2802,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		revarg_opt |= REVARG_CANNOT_BE_FILENAME;
 	for (left = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
-		if (!seen_end_of_options && *arg == '-') {
+		if (!seen_end_of_options && *arg == '-' && arg[1]) {
 			int opts;

 			opts = handle_revision_pseudo_opt(
-- snap --

That latter patch obviously needs at least one accompanying test case, and
the patch series would then need to drop the special-casings we already
have for "-".

Rubén, do you want to take this a bit further?

Ciao,
Dscho

P.S.: For convenient fetching, I opened a draft PR here:
https://github.com/gitgitgadget/git/pull/1329

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> This patch does that:
>
> -- snip --
> diff --git a/object-name.c b/object-name.c
> index 4d2746574cd..a2732be3b71 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -1616,6 +1616,20 @@ int repo_interpret_branch_name(struct repository *r,
>  	if (!namelen)
>  		namelen = strlen(name);
>
> +	if (namelen == 1 && *name == '-') {
> +		struct grab_nth_branch_switch_cbdata cb = {
> +			.remaining = 1,
> +			.sb = buf
> +		};
> +
> +		if (refs_for_each_reflog_ent_reverse(get_main_ref_store(r),
> +						     "HEAD",
> +						     grab_nth_branch_switch,
> +						     &cb) <= 0)
> +			return -1;
> +		return namelen;
> +	}
> +

This is very reasonable.  

Anywhere we take '@{-1}', 'main', or 'js/dash-is-previous', nobody
would be surprised if we take '-' and interpreted as '@{-1}' in
addition.

However, as I said earlier, we have command line disambiguation that
wants to tell dashed options, revs, and paths apart.  We need to
find places that need adjusting and adjust them, *AND* then make
sure that such tweaks do not introduce unintended side effect.
These, especially the last one, take a careful work I would rather
not to see unexperienced developer perform alone and take the
finding by them blindly.

> What does not work with this patch is `git show -`. I am not sure whether
> we want to make that work, although I have to admit that I would use it.
> Often. And this patch would make it work, the test suite even passes with
> it:

Exactly my above point.  Nobody including our tests expect that a
single '-' to be taken as a rev when we disambiguate command line
arguments, so it is very unlikely that it is tested to ensure that a
single '-' ends the revs and is checked for its "path-ness".  It is
not surprising that the tests do not fail ;-).

> -- snip --
> diff --git a/revision.c b/revision.c
> index f4eee11cc8b..207b554aef1 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2802,7 +2802,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  		revarg_opt |= REVARG_CANNOT_BE_FILENAME;
>  	for (left = i = 1; i < argc; i++) {
>  		const char *arg = argv[i];
> -		if (!seen_end_of_options && *arg == '-') {
> +		if (!seen_end_of_options && *arg == '-' && arg[1]) {
>  			int opts;
>
>  			opts = handle_revision_pseudo_opt(
> -- snap --

Thanks.  These two patch snippets in the message I am responding to
are promising and exciting.

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

[Rubén Justo]

Hi,

On 8/19/22 3:05 PM, Johannes Schindelin wrote:
> 
> Rubén, do you want to take this a bit further?
> 

Just wanted to delete the previous branch, I didn't want to enter in a 
deep change... but here we are :-)

Allow the "-" in setup_revisions:

diff --git a/revision.c b/revision.c
index f4eee11cc8..65e7eb85d8 100644
--- a/revision.c
+++ b/revision.c
@@ -2802,7 +2802,7 @@ int setup_revisions(int argc, const char **argv, 
struct rev_info *revs, struct s
                 revarg_opt |= REVARG_CANNOT_BE_FILENAME;
         for (left = i = 1; i < argc; i++) {
                 const char *arg = argv[i];
-               if (!seen_end_of_options && *arg == '-') {
+               if (!seen_end_of_options && *arg == '-' && 
!strchr(".^~:@", arg[1])) {
                         int opts;

Then, consider "-" as nth_prior, just like @{-1}:

diff --git a/object-name.c b/object-name.c
index 4d2746574c..87b4c33cce 100644
--- a/object-name.c
+++ b/object-name.c
@@ -934,6 +934,9 @@ static int get_oid_basic(struct repository *r, const 
char *str, int len,
                 }
         }

+        if ((len == 1) && (str[0] == '-'))
+                nth_prior = 1;
+
         /* Accept only unambiguous ref paths. */
         if (len && ambiguous_path(str, len))
                 return -1;

diff --git a/object-name.c b/object-name.c
index 4d2746574c..87b4c33cce 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1420,18 +1423,24 @@ static int interpret_nth_prior_checkout(struct 
repository *r,
         const char *brace;
         char *num_end;

-       if (namelen < 4)
-               return -1;
-       if (name[0] != '@' || name[1] != '{' || name[2] != '-')
-               return -1;
-       brace = memchr(name, '}', namelen);
-       if (!brace)
-               return -1;
-       nth = strtol(name + 3, &num_end, 10);
-       if (num_end != brace)
-               return -1;
-       if (nth <= 0)
-               return -1;
+        if (name[0] == '-' && strchr(".^~:@", name[1])) {
+                nth = 1;
+                brace = name;
+        } else {
+                if (namelen < 4)
+                        return -1;
+                if (name[0] != '@' || name[1] != '{' || name[2] != '-')
+                        return -1;
+                brace = memchr(name, '}', namelen);
+                if (!brace)
+                        return -1;
+                nth = strtol(name + 3, &num_end, 10);
+                if (num_end != brace)
+                        return -1;
+                if (nth <= 0)
+                        return -1;
+        }
+
         cb.remaining = nth;

Two checks needs to be adjusted:

diff --git a/refs.c b/refs.c
index 90bcb27168..0ed9f99ccc 100644
--- a/refs.c
+++ b/refs.c
@@ -198,6 +198,11 @@ static int check_or_sanitize_refname(const char 
*refname, int flags,
                 else
                         return -1;
         }
+
+       if (component_len == 1 && refname[0] == '-') {
+                return -1;
+       }
+

diff --git a/object-name.c b/object-name.c
index 4d2746574c..87b4c33cce 100644
@@ -1684,7 +1693,7 @@ int strbuf_check_branch_ref(struct strbuf *sb, 
const char *name)
          */
         strbuf_splice(sb, 0, 0, "refs/heads/", 11);

-       if (*name == '-' ||
+       if ((*name == '-' && name[1]) ||
             !strcmp(sb->buf, "refs/heads/HEAD"))
                 return -1;


I know this changes open the possibility of having things like "-^2" or 
-@{yesterday} that you said was not desiable. But, why wouldn't we want 
that? Having parse_opt_result to handle that:

diff --git a/parse-options.c b/parse-options.c
index edf55d3ef5..2757bd94c1 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -740,7 +740,7 @@ enum parse_opt_result parse_options_step(struct 
parse_opt_ctx_t *ctx,
                     ctx->argc != ctx->total)
                         break;

-               if (*arg != '-' || !arg[1]) {
+               if (*arg != '-' || strchr(".^~:@", arg[1])) {
                         if (parse_nodash_opt(ctx, arg, options) == 0)
                                 continue;
                         if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)


With this changes, all the current uses of "-", with the hacks already 
removed, keep working and fixes the weird cases:

$ git merge branch - other_branch
$ git branch -d branch - other_branch


Also, I've checked that work:
$ git diff -
$ git show -
$ git blame -
and branch -d :-)

I've checked the current tests and added new ones for this, all passes. ie:

diff --git a/t/t1505-rev-parse-last.sh b/t/t1505-rev-parse-last.sh
index 4a5758f08a..231457df50 100755
--- a/t/t1505-rev-parse-last.sh
+++ b/t/t1505-rev-parse-last.sh
@@ -40,10 +40,18 @@ test_expect_success '@{-1} works' '
         test_cmp_rev side @{-1}
  '

+test_expect_success '- works' '
+       test_cmp_rev side -
+'
+
  test_expect_success '@{-1}~2 works' '
         test_cmp_rev side~2 @{-1}~2
  '

+test_expect_success '-~2 works' '
+       test_cmp_rev side~2 -~2
+'
+
  test_expect_success '@{-1}^2 works' '
         test_cmp_rev side^2 @{-1}^2
  '

Still needs some work, for example: git log shows "-" in the warning:
$ ../git/git log "-@{10000 minutes ago}"
warning: log for '-' only goes back to Wed, 24 Aug 2022 14:16:17 +0200

What do you think, is it worth the change?

I've created a PR with all the changes and tests.
https://github.com/gitgitgadget/git/pull/1338

Thanks.

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

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> struct rev_info *revs, struct s
>                 revarg_opt |= REVARG_CANNOT_BE_FILENAME;
>         for (left = i = 1; i < argc; i++) {
>                 const char *arg = argv[i];
> -               if (!seen_end_of_options && *arg == '-') {
> +               if (!seen_end_of_options && *arg == '-' &&
> !strchr(".^~:@", arg[1])) {

Yuck.  I really didn't want to see that strchr() or any other logic
that _knows_ what letter is allowed after a "rev".  It will be
impossible to keep it in sync with the real code paths that needs to
know and parses these syntactical constructs, and folks new to the
codebase will never be able to tell at a first glance if the above
is sufficient (or what the strchr() is doing).

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

[Rubén Justo]

On 8/25/22 6:23 PM, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
>> struct rev_info *revs, struct s
>>                  revarg_opt |= REVARG_CANNOT_BE_FILENAME;
>>          for (left = i = 1; i < argc; i++) {
>>                  const char *arg = argv[i];
>> -               if (!seen_end_of_options && *arg == '-') {
>> +               if (!seen_end_of_options && *arg == '-' &&
>> !strchr(".^~:@", arg[1])) {
> 
> Yuck.  I really didn't want to see that strchr() or any other logic
> that _knows_ what letter is allowed after a "rev".  It will be
> impossible to keep it in sync with the real code paths that needs to
> know and parses these syntactical constructs, and folks new to the
> codebase will never be able to tell at a first glance if the above
> is sufficient (or what the strchr() is doing).
> 

Some logic is needed to disambiguate from options. It is difficult than 
that set of chars changes, they are all around the code. And if any new 
is added should be reviewed and hopefully some test will be broken.

Maybe a more centralized approach?

diff --git a/parse-options.c b/parse-options.c
index 2757bd94c1..303854e8a4 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -740,7 +741,7 @@ enum parse_opt_result parse_options_step(struct 
parse_opt_ctx_t *ctx,
                     ctx->argc != ctx->total)
                         break;

-               if (*arg != '-' || strchr(".^~:@", arg[1])) {
+               if (*arg != '-' || 
!check_refchar_component_special(arg[1])) {
                         if (parse_nodash_opt(ctx, arg, options) == 0)
                                 continue;
                         if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)


diff --git a/refs.c b/refs.c
index 0ed9f99ccc..5327a8ec1f 100644
--- a/refs.c
+++ b/refs.c
@@ -159,6 +159,32 @@ static int check_refname_component(const char 
*refname, int *flags,
         return cp - refname;
  }

+int check_refchar_component_special(char refchar)
+{
+        int ch = refchar & 255;
+        unsigned char disp = refname_disposition[ch];
+
+        switch (disp) {
+        case 1:
+                /* end of component */
+                return 0;
+        case 2:
+                /* ".." components */
+                return 0;
+        case 3:
+                /* "@{" components */
+                return 0;
+        case 4:
+                /* forbidden char */
+                return 0;
+        case 5:
+                /* pattern */
+                return 0;
+        }
+
+        return -1;
+}
+
  static int check_or_sanitize_refname(const char *refname, int flags,
                                      struct strbuf *sanitized)
  {

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

[Rubén Justo]

Hi Johannes,

Thanks for the /allow!

On Mon, Aug 8, 2022 at 3:26 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de>  wrote:

> Touching only the `delete_branches()` function suggests that other
> commands are left as before, e.g. `git branch --unset-upstream -` would
> probably fail.
>
> That's fine, but the commit message claims that the `"-"` special-casing
> is introduced for the `git branch` command, not just for `git branch -d`.
>
>>                        die(_("Couldn't look up commit object for HEAD"));
>>        }
> At this stage, we already handled the `--remotes` flag, therefore I think
> that this patch does not do the intended thing for this command-line:
>
>          git branch -d --remotes -
>
>> +     if ((argc == 1) && !strcmp(argv[0], "-")) {
>> +             argv[0] = "@{-1}";
>> +     }
> This means that we only handle `git branch -d -`, but not `git branch -d
> some-branch - some-other-branch`.
>
> Could you fix that?

I did it on purpose, to be interpreted in the context of "git branch
-d/D" with just one branch: "previous branch". I agree the commit
message does not suggest this, I can fix it.

My intention is a short-hand for "delete previous branch", the same
way "git merge -" is "merge previous branch".

The workflow to address is just allow doing:

$ git checkout work_to_review
$ git checkout -
$ git merge -
$ git branch -d -

Instead of:

$ git checkout work_to_review
$ git checkout -
$ git merge -
$ git branch -d work_to_review

The syntax @{-1} is hard for me to write, and I feel intuitive "-",
like in "cd -".

Make sense to me...

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

[Junio C Hamano]

"Rubén Justo via GitGitGadget"  <gitgitgadget@gmail.com> writes:

> From: rjusto <rjusto@gmail.com>
>
> Align "branch" with the intuitive use of "-" as a short-hand
> for "@{-1}", like in "checkout" and "merge" commands.
>
> $ git branch -d -      # short-hand for: "git branch -d @{-1}"
> $ git branch -D -      # short-hand for: "git branch -D @{-1}"

The "-d" and "-D" options being the more detructive ones among other
operation modes of the command, I am not sure if this change is even
desirable.  Even if it were, the implementation to special case a
single argument case like this ...

> +	if ((argc == 1) && !strcmp(argv[0], "-")) {
> +		argv[0] = "@{-1}";
> +	}

... (by the way, we don't write braces around a single statement
block) would invite cries from confused users why none of these ...

 $ git branch -m - new-name
 $ git branch new-branch -
 $ git branch --set-upstream-to=<upstream> -
    
work and "-" works only for deletion.

So, I dunno.

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

[Rubén Justo]

On Mon, Aug 8, 2022 at 4:47 PM Junio C Hamano<gitster@pobox.com>  wrote:

> The "-d" and "-D" options being the more detructive ones among other
> operation modes of the command, I am not sure if this change is even
> desirable.  Even if it were, the implementation to special case a
> single argument case like this ...
>
>> +     if ((argc == 1) && !strcmp(argv[0], "-")) {
>> +             argv[0] = "@{-1}";
>> +     }
> ... (by the way, we don't write braces around a single statement
> block) would invite cries from confused users why none of these ...
>
>   $ git branch -m - new-name
>   $ git branch new-branch -
>   $ git branch --set-upstream-to=<upstream> -
>
> work and "-" works only for deletion.

Agree. But the approach is to ease the deletion of previous branch,
aligned with merge:

$ git merge - -
merge: - - not something we can merge
$ git merge - old-branch
merge: - - not something we can merge

In fact, I think it is a bit confuse to allow use it that way, and
probably induces to error.

Haven't think about -m, -c. If you think it is a good addition, I can do it.

I can fix the braces around that single statement block, sorry.

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1419 bytes --]

Hi Rubén,

On Sat, 13 Aug 2022, Rubén Justo wrote:

> On Mon, Aug 8, 2022 at 4:47 PM Junio C Hamano<gitster@pobox.com>  wrote:
>
> > The "-d" and "-D" options being the more detructive ones among other
> > operation modes of the command, I am not sure if this change is even
> > desirable.  Even if it were, the implementation to special case a
> > single argument case like this ...
> >
> > > +     if ((argc == 1) && !strcmp(argv[0], "-")) {
> > > +             argv[0] = "@{-1}";
> > > +     }
> > ... (by the way, we don't write braces around a single statement
> > block) would invite cries from confused users why none of these ...
> >
> >   $ git branch -m - new-name
> >   $ git branch new-branch -
> >   $ git branch --set-upstream-to=<upstream> -
> >
> > work and "-" works only for deletion.
>
> Agree. But the approach is to ease the deletion of previous branch,
> aligned with merge:
>
> $ git merge - -
> merge: - - not something we can merge
> $ git merge - old-branch
> merge: - - not something we can merge

This is confusing me: how is the patch supporting `git branch -d -`
aligned with the presented `git merge` invocations?

In any case, you now have two sets of feedback that say that
special-casing one particular command-line and leaving all other
invocations using `-` unchanged is undesirable, if you needed more than
one such feedback.

Ciao,
Dscho

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

[Rubén Justo]

Hi Johannes,

On 8/16/22 11:31 AM, Johannes Schindelin wrote:

>> $ git merge - old-branch
>> merge: - - not something we can merge
> 
> This is confusing me: how is the patch supporting `git branch -d -`
> aligned with the presented `git merge` invocations?

"merge" supports multiple objects to be specified, but "-" only is 
accepted if just one argument is specified, as Junio did it in:

commit 4e8115fff135a09f75020083f51722e7e35eb6e9
Author: Junio C Hamano <gitster@pobox.com>
Date:   Thu Apr 7 15:57:57 2011 -0700

     merge: allow "-" as a short-hand for "previous branch"

     Just like "git checkout -" is a short-hand for "git checkout @{-1}" to
     conveniently switch back to the previous branch, "git merge -" is a
     short-hand for "git merge @{-1}" to conveniently merge the previous 
branch.

     It will allow me to say:

         $ git checkout -b au/topic
         $ git am -s ./+au-topic.mbox
         $ git checkout pu
         $ git merge -

     which is an extremely typical and repetitive operation during my 
git day.

     Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/merge.c b/builtin/merge.c
index d54e7ddbb1..0bdd19a137 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1062,9 +1062,12 @@ int cmd_merge(int argc, const char **argv, const 
char *prefix)
         if (!allow_fast_forward && fast_forward_only)
                 die(_("You cannot combine --no-ff with --ff-only."));

-       if (!argc && !abort_current_merge && default_to_upstream)
-               argc = setup_with_upstream(&argv);
-
+       if (!abort_current_merge) {
+               if (!argc && default_to_upstream)
+                       argc = setup_with_upstream(&argv);
+               else if (argc == 1 && !strcmp(argv[0], "-"))
+                       argv[0] = "@{-1}";
+       }
         if (!argc)
                 usage_with_options(builtin_merge_usage,
                         builtin_merge_options);



So I aligned "branch -d" (or "delete-branch") with that.

The other two commands that already support "-", also works the same way:

$ git checkout -B - default
fatal: '-' is not a valid branch name

$ git rebase default -
fatal: no such branch/commit '-'

To summarize, my goal is to allow:

$ git checkout work_to_review
$ git checkout -
$ git merge - # or git rebase -
$ git branch -d -

Makes sense to me...

I've updated the commit message with a more specific message and removed 
the braces, jic.

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 4523 bytes --]

Hi Rubén,

On Tue, 16 Aug 2022, Rubén Justo wrote:

> On 8/16/22 11:31 AM, Johannes Schindelin wrote:
>
> > > $ git merge - old-branch
> > > merge: - - not something we can merge
> >
> > This is confusing me: how is the patch supporting `git branch -d -`
> > aligned with the presented `git merge` invocations?
>
> "merge" supports multiple objects to be specified, but "-" only is accepted if
> just one argument is specified, as Junio did it in:
>
> commit 4e8115fff135a09f75020083f51722e7e35eb6e9
> Author: Junio C Hamano <gitster@pobox.com>
> Date:   Thu Apr 7 15:57:57 2011 -0700
>
>     merge: allow "-" as a short-hand for "previous branch"
>
>     Just like "git checkout -" is a short-hand for "git checkout @{-1}" to
>     conveniently switch back to the previous branch, "git merge -" is a
>     short-hand for "git merge @{-1}" to conveniently merge the previous
> branch.
>
>     It will allow me to say:
>
>         $ git checkout -b au/topic
>         $ git am -s ./+au-topic.mbox
>         $ git checkout pu
>         $ git merge -
>
>     which is an extremely typical and repetitive operation during my git day.
>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index d54e7ddbb1..0bdd19a137 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1062,9 +1062,12 @@ int cmd_merge(int argc, const char **argv, const char
> *prefix)
>         if (!allow_fast_forward && fast_forward_only)
>                 die(_("You cannot combine --no-ff with --ff-only."));
>
> -       if (!argc && !abort_current_merge && default_to_upstream)
> -               argc = setup_with_upstream(&argv);
> -
> +       if (!abort_current_merge) {
> +               if (!argc && default_to_upstream)
> +                       argc = setup_with_upstream(&argv);
> +               else if (argc == 1 && !strcmp(argv[0], "-"))
> +                       argv[0] = "@{-1}";
> +       }
>         if (!argc)
>                 usage_with_options(builtin_merge_usage,
>                         builtin_merge_options);

Ah, the vagaries of being a maintainer and everybody following your lead,
even if you have a bad day and are grumpy, or as in this case just want to
get a quick fix in that supports your workflow better, and then move on.

If you read the commit message carefully, you will note that there is no
justification for restricting it to the `argc == 1` case.

I assume that the implicit rationale is that it was just simpler to do it
this way.

The alternative would have been to modify `collect_parents()`, or even
`get_merge_parent()` (which has many more callers), and at some stage the
investigation would have been as involved as it will be in this here
thread.

However, it is one thing to integrate such a patch as a one-off, or do it
two times, or three.

It is another thing to do this again and again and again and seeing that
we're not getting anywhere and only piling hack upon hack.

It is this latter stage that we have arrived at.

> So I aligned "branch -d" (or "delete-branch") with that.
>
> The other two commands that already support "-", also works the same way:
>
> $ git checkout -B - default
> fatal: '-' is not a valid branch name
>
> $ git rebase default -
> fatal: no such branch/commit '-'
>
> To summarize, my goal is to allow:
>
> $ git checkout work_to_review
> $ git checkout -
> $ git merge - # or git rebase -
> $ git branch -d -
>
> Makes sense to me...

There are different qualities at play with these commands, though. `git
checkout` cannot support more than a single revision argument. With `git
merge`, technically we do support more than a single revision argument
(via octopus merges), but support for it is limited (for example, we do
not even support recursive octopus merges). You might say that it is
discouraged to call `git merge` with more than one revision argument.

With `git branch -d` or with `git branch --list`, we are in a different
league. Those commands are commonly called with more than just a single
branch name.

And then there are the other commands that would benefit from support for
`-` and that accept many more than one revision argument, too, such as
`log`, `rev-parse`, `merge-base`, etc.

Sure, we can accept one more one-off hack to support a single `-` argument
to refer to the previous branch. The sum of those hacks, however, becomes
a burden.

Ciao,
Dscho

[PATCH v2] allow "-" as short-hand for "@{-1}" in "branch -d"

[Rubén Justo via GitGitGadget]

From: rjusto <rjusto@gmail.com>

Align "branch -d" with the intuitive use of "-" as a short-hand
for "@{-1}", like in "checkout", "rebase" and "merge" commands.

$ git branch -d -      # short-hand for: "git branch -d @{-1}"
$ git branch -D -      # short-hand for: "git branch -D @{-1}"

So I can do:

$ git checkout work_to_review
$ git checkout -
$ git merge - # or git rebase -
$ git branch -d -

Signed-off-by: rjusto <rjusto@gmail.com>
---
    branch: allow "-" as a short-hand for "previous branch"
    
    Align "branch" with the intuitive use of "-" as a short-hand for
    "@{-1}", like in "checkout" and "merge" commands.
    
    $ git branch -d - # short-hand for: "git branch -d @{-1}" $ git branch
    -D - # short-hand for: "git branch -D @{-1}"
    
    Signed-off-by: rjusto rjusto@gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1315%2Frjusto%2Fmaster-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1315/rjusto/master-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1315

Range-diff vs v1:

 1:  3ba75097ea0 ! 1:  0fe48ada15b branch: allow "-" as a short-hand for "previous branch"
     @@ Metadata
      Author: rjusto <rjusto@gmail.com>
      
       ## Commit message ##
     -    branch: allow "-" as a short-hand for "previous branch"
     +    allow "-" as short-hand for "@{-1}" in "branch -d"
      
     -    Align "branch" with the intuitive use of "-" as a short-hand
     -    for "@{-1}", like in "checkout" and "merge" commands.
     +    Align "branch -d" with the intuitive use of "-" as a short-hand
     +    for "@{-1}", like in "checkout", "rebase" and "merge" commands.
      
          $ git branch -d -      # short-hand for: "git branch -d @{-1}"
          $ git branch -D -      # short-hand for: "git branch -D @{-1}"
      
     +    So I can do:
     +
     +    $ git checkout work_to_review
     +    $ git checkout -
     +    $ git merge - # or git rebase -
     +    $ git branch -d -
     +
          Signed-off-by: rjusto <rjusto@gmail.com>
      
       ## builtin/branch.c ##
     @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int fo
       			die(_("Couldn't look up commit object for HEAD"));
       	}
       
     -+	if ((argc == 1) && !strcmp(argv[0], "-")) {
     ++	if ((argc == 1) && !strcmp(argv[0], "-"))
      +		argv[0] = "@{-1}";
     -+	}
      +
       	for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
       		char *target = NULL;


 builtin/branch.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e998..7f7589bd4a8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -241,6 +241,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			die(_("Couldn't look up commit object for HEAD"));
 	}
 
+	if ((argc == 1) && !strcmp(argv[0], "-"))
+		argv[0] = "@{-1}";
+
 	for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
 		char *target = NULL;
 		int flags = 0;

base-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9
-- 
gitgitgadget

Re: [PATCH v2] allow "-" as short-hand for "@{-1}" in "branch -d"

[Junio C Hamano]

"Rubén Justo via GitGitGadget"  <gitgitgadget@gmail.com> writes:

> From: rjusto <rjusto@gmail.com>

Documentation/SubmittingPatches:

    Also notice that a real name is used in the `Signed-off-by`
    trailer. Please don't hide your real name.

>      -    branch: allow "-" as a short-hand for "previous branch"
>      +    allow "-" as short-hand for "@{-1}" in "branch -d"

The "branch:" prefix is lost here, which is not an improvement.  The
"<area>:" prefix is what makes it easier to locate a particular
change in "git shortlog --no-merges v2.37.0..v2.38.0".

As to the implementation, there is nothing to complain about, but as
we already discussed during the reivew of the first iteration, I am
not sure if the goal is sound in the first place.

Thanks.

Re: [PATCH v2] allow "-" as short-hand for "@{-1}" in "branch -d"

[Rubén Justo]

On 8/16/22 8:55 PM, Junio C Hamano wrote:
> 
>> From: rjusto <rjusto@gmail.com>
> 
> Documentation/SubmittingPatches:
> 
>      Also notice that a real name is used in the `Signed-off-by`
>      trailer. Please don't hide your real name.
> 

Fixed.

>>       -    branch: allow "-" as a short-hand for "previous branch"
>>       +    allow "-" as short-hand for "@{-1}" in "branch -d"
> 
> The "branch:" prefix is lost here, which is not an improvement.  The
> "<area>:" prefix is what makes it easier to locate a particular
> change in "git shortlog --no-merges v2.37.0..v2.38.0".

Just want to reduce the confusion, as Johannes suggested, that could 
apply to the whole command. But ok, I've put it back.

> 
> As to the implementation, there is nothing to complain about, but as
> we already discussed during the reivew of the first iteration, I am
> not sure if the goal is sound in the first place.
> 

Sorry, I don't want to be annoying, I just think the effort is worth of it.

The change rounds the workflow: "checkout/merge/branch -d" and does not 
introduce any new confusion or new circumstance. The commands that 
already support the substitution doesn't support it in all combinations, 
complex use cases can (probably, should) use the @{-1} syntax and 
definitely those uses are not the candidates to use the short-hands.

> Thanks.
> 

Thank you.

Rubén.

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

[Rubén Justo]

From: Rubén Justo <rjusto@gmail.com>

Align "branch -d" with the intuitive use of "-" as a short-hand
for "@{-1}", like in "checkout", "rebase" and "merge" commands.

$ git branch -d -      # short-hand for: "git branch -d @{-1}"
$ git branch -D -      # short-hand for: "git branch -D @{-1}"

So I can do:

$ git checkout work_to_review
$ git checkout -
$ git merge - # or git rebase -
$ git branch -d -

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
     branch: allow "-" as a short-hand for "previous branch"

     Align "branch -d" with the intuitive use of "-" as a short-hand for
     "@{-1}", like in "checkout", "rebase" and "merge" commands.

     $ git branch -d - # short-hand for: "git branch -d @{-1}" $ git branch
     -D - # short-hand for: "git branch -D @{-1}"

     So I can do:

     $ git checkout work_to_review $ git checkout - $ git merge - # or git
     rebase - $ git branch -d -

     Signed-off-by: Rubén Justo rjusto@gmail.com

Published-As: 
https://github.com/gitgitgadget/git/releases/tag/pr-1315%2Frjusto%2Fmaster-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-1315/rjusto/master-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1315

Range-diff vs v2:

  1:  0fe48ada15b ! 1:  cc28546aea9 allow "-" as short-hand for "@{-1}" 
in "branch -d"
      @@
        ## Metadata ##
      -Author: rjusto <rjusto@gmail.com>
      +Author: Rubén Justo <rjusto@gmail.com>

        ## Commit message ##
      -    allow "-" as short-hand for "@{-1}" in "branch -d"
      +    branch: allow "-" as short-hand for "@{-1}" in "branch -d"

           Align "branch -d" with the intuitive use of "-" as a short-hand
           for "@{-1}", like in "checkout", "rebase" and "merge" commands.
      @@ Commit message
           $ git merge - # or git rebase -
           $ git branch -d -

      -    Signed-off-by: rjusto <rjusto@gmail.com>
      +    Signed-off-by: Rubén Justo <rjusto@gmail.com>

        ## builtin/branch.c ##
       @@ builtin/branch.c: static int delete_branches(int argc, const 
char **argv, int force, int kinds,


  builtin/branch.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e998..7f7589bd4a8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -241,6 +241,9 @@ static int delete_branches(int argc, const char 
**argv, int force, int kinds,
  			die(_("Couldn't look up commit object for HEAD"));
  	}

+	if ((argc == 1) && !strcmp(argv[0], "-"))
+		argv[0] = "@{-1}";
+
  	for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
  		char *target = NULL;
  		int flags = 0;

base-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9
-- 
2.36.1

[PATCH v4] branch: allow "-" as a shortcut for "previous branch"

[Rubén Justo]

Teach "git branch" the use of "-" as a shortcut for "@{-1}", like in "checkout
-", "merge -" and other commands.

Usage examples:

	$ git branch -d -              # git branch -d @{-1}
	$ git branch -M some-branch -  # git branch -M some-branch @{-1}
	$ git branch -c - new-branch   # git branch -c @{-1} new-branch

Possible workflow:

	$ git checkout work_to_review
	$ git checkout -
	$ git merge -  # or git rebase -
	$ git branch -d -


This change will be unnecessary if "-" ends up being supported globally as a
shortcut for "previous branch".  The usage examples and the tests will remain
valid.

Previous-work-by: Kenny Lee Sin Cheong <kenny.lee28@gmail.com>
Previous-work-by: Dinesh <dpdineshp2@gmail.com>
Previous-work-by: Elena Petrashen <elena.petrashen@gmail.com>
Previous-work-by: Aaron Greenberg <p@aaronjgreenberg.com>
Previous-work-by: Ivan Tham <pickfire@riseup.net>
Previous-work-by: Erlend E. Aasland <erlend.aasland@innova.no>
Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

Let's recap this.

We have discussed two topics, related but somewhat different:
  1.- teach "git branch" to support "-" as a shortcut of "previous-branch"
  2.- teach "git" to support "-" as a shortcut of "previous-branch" _anywhere a
      branch is expected_

I'll refer to the former as "local support" and the latter as "global support".

Teach "git branch -d" about "-" as a shortcut of "previous-branch" is the main
intention of the patch that originates this thread.

Teach "git" about the shortcut makes unnecessary, at first, the change in "git
branch".  It makes "-" work in "git branch" and everywhere else as well.   And
this is what makes tempting to skip the "local support" and go for the "global
support".

Attempts of "global support" can be tracked back to Jan 2009 [2], just days
after the syntax "@{-1}" was presented [1] or the "git checkout -" (the real
originator of all of this) patch was committed.  Each of this attempts has
received some concerns and not many reviews and/or iterations to allow the
curation and integration of the change.  Meanwhile, since "checkout -" was
introduced: "merge -", "cherry-pick -", "rebase -" and "worktree -", have
gained support for "-" on its own, via a local: if (!strcmp(argv[i], "-"))
argv[i] = "@{-1}".

Since 2016 there have been, at least, five patch series (see below) to make a
similar (some equal) change in "git branch -d".  The motivations suggested each
time have been the same: fulfill the intuitive use of "-" when deleting
branches, following the path of, initially, "cd -" and then "checkout -", 
"merge -",.. not any other function of "git branch".  As well as the attempts 
for the "global support", each of this attempts has also received some
concerns, reviews and done some iterations.

Let's have a look at the reviews and/or concerns about the "local support" 
first, and then the ones about the "global support".

Reviews and/or concerns about "git branch -d -":

 * /if (!strcmp(argv[i], "-")) argv[i] = "@{-1}/ is somewhat of a hack
 
   https://lore.kernel.org/git/7s9s8p38-r22n-opnn-9219-0p49onrro70s@tzk.qr/
 
   > Sure, we can accept one more one-off hack to support a single `-` argument
   > to refer to the previous branch. The sum of those hacks, however, becomes
   > a burden.
 
   Do we want a more elaborated way of the hack? ie:

        +       const char* handle_dash_alias_argv(const char* arg)
        +       {
        +           if (arg && !strcmp(argv[i], "-"))
        +                   return "@{-1}";
        +
        +               return arg;
        +       }
        ...
        -       if (!strcmp(arg, "-"))
        -               arg = "@{-1}";
        +       arg = handle_dash_alias_argv(arg);
        ...
        -       } else if (argc == 1 && !strcmp(argv[0], "-")) {
        -               argv[0] = "@{-1}";
        +       } else if (argc == 1) {
        +               argv[0] = handle_dash_alias(argv[0]);

   Or, do we want to keep it simple and expect that the "global support" will
   allow this hacks vanish easily?

   The "global support" resolves automatically this.  Will make all of this
   hacks unnecessary.  So, isn't this more of an argument to defend the work
   in the "global support" than a concern for this patch?

 
 * The patch only considers "git branch -d/-D", not any other options like
 * "-m", "-c", "-u", "--unset-upstream", "--edit-description"

   First things first.  This review has already been productive: some of those
   uncovered options by the patch weren't even supporting "@{-N}", which is 
   documented to be supported everywhere.  So, thank you.  A patch to fix this
   has already been submitted [E].

   On the other hand, an argument can be used here: the operations that
   currently support the shortcut (ie: merge, rebase, workdir...) do not do it
   in all of its capabilities, and this can considered a good thing.  Allowing
   the previously commented hack to stay simple by not covering the
   corner-cases no one has needed yet (maybe never will).  Eventually,
   if the "global support" is achieved, there would be no need to do
   anything to complete that currently missing support.  Will be completed
   automatically.
 

 * "git branch -d -" is a destructive operation unlike the other operations
 * currently supporting the "-" alias (ie: checkout, merge,...)
  
   There is a reasonable twist for this argument: is this a concern for this
   patch or a spark to think about the destructive aspects of "git branch -d"?
   Maybe the reflog can be gc'ed and not deleted [F]? Better or new advices [7]?
   ...

   Also an interesting argument to consider:
 
   https://lore.kernel.org/git/xmqqvadidbo7.fsf@gitster-ct.c.googlers.com/
 
   >     $ git checkout somelongtopicname
   >     $ work work work
   >     $ git checkout master && git merge -
   >     $ git branch -d -
   >
   > would be a lot less error-prone than the user being forced to write
   > last step in longhand
   >
   >     $ git branch -d someotherlongtopicname
   >
   > and destroying an unrelated but similarly named branch.

   More about this, below in the concerns about "global support" where
   isolated "-" in the command line are described.

 
 * Confusing error message: "error: branch "@{-1}" not found." received, but
 * "-" was specified.
 
   This is a legit concern and can be argued that it is already happening in
   the commands that already support the shortcut:
 
       $ git merge -
       merge: @{-1} - not something we can merge

   This can be reasoned because solving it will require /dig deeper/ in the
   hack discussed above and shouldn't be creating a lot of confusion.

   Current WIP patches for "global support" show that this problem will vanish
   along with the hacks.  So, again, this becomes more of an argument for
   having "global support" than a complain about this patch.  Thought in this
   case, it is not just the _burden in the code_ but the experience of the
   user, which has repercussion so it requires special consideration.


Reviews and/or concerns about global support of the shortcut:

 * '-' is also known as a synonym for stdin

   https://lore.kernel.org/git/20200429190013.GG83442@syl.local/
    
   > > > +To delete the previous branch::
   > > > ++
   > > > +------------
   > > > +$ git branch -D -
   > >
   > > ... so this suggests that the command, when used like this:
   > >
   > > $ echo "branch_name" | git branch -D -
   > >
   > > will delete "branch_name" rather than some "previous" branch, whatever
   > > that means.
   > >
   > > Is this short-cut /that/ important to create yet another confusion?
   >
   > I think that it may be causing more confusion now than it would be after
   > Ivan's patch. 'git checkout', for example, also treats '-' as a synonym
   > for '@{-1}'.
   >
   > In my opinion, it is fairly clear that 'git branch -D -' means "delete
   > the last branch", and not "delete a list of branches from stdin.


 * Isolated "-" is a risk when misspelling command lines

   https://lore.kernel.org/git/20200429195745.GC3920@syl.local/
   
   > > BTW, what about mistyping:
   > >
   > > $ git branch -d - f my_branch
   > >
   > > for
   > >
   > > $ git branch -d -f my_branch
   > >
   > > or some such?
   > >
   > > No, it still doesn't look like a good idea to use isolated '-' as
   > > suggested by the patch.
   >
   > Frankly, I do not find this compelling. Does that mean that '/' as a
   > directory separator is dangerous, too, because you can accidentally
   > write 'rm -rf / foo/bar/baz'?


 * "-" must be a synonym of "@{-1}", but not making "-^2~15" a synonym of
   "@{-1}^2~15"
   
   https://lore.kernel.org/git/5194s6qn-570s-6053-2104-9s22qo1874sn@tzk.qr/

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

   I cannot find any argument for this.  "-" should or shouldn't be idempotent
   to "@{-1}", for me it needs more thought and exploration.  But an agreement
   to left it restricted but open to consideration sounds plausible.


After all of this, if you are still here, sounds reasonable to say:
    - there is no clear concern or review that justifies why "git branch"
      hasn't been taught to support the "-" shortcut
    - "global support" of "-" as a shortcut for "@{-1}" or "previous-branch"
      deserves some work


This is the v4 of the patch to support the shortcut "-" in "branch -d".  The
changes are:
   - Handle "-" when deleting multiple branches
   - Add support for -m/-c.
   - New tests
   - Commit message, to reflect changes and include names of people
     who previously worked on this.

The switches "--edit-description", "--set-upstream-to" and "--unset-upstream"
can adopt the hack, if necessary, whenever the patch [E] that fixes supporting
"@{-1}" is merged.  It is not necessary, and it is better this way, to link 
the fix with the support of "-", which is not clear if it will be.


I won't send a v5, promise :-)... but an RFC v1 for the "global support"?:

    diff --git a/setup.c b/setup.c
    index cefd5f63c4..de86bb2d98 100644
    --- a/setup.c
    +++ b/setup.c
    @@ -266,8 +266,6 @@ void verify_filename(const char *prefix,
                         const char *arg,
    ?                    int diagnose_misspelt_rev)
     {
    ?       if (*arg == '-')
    ?               die(_("option '%s' must come before non-option arguments"), arg);
            if (looks_like_pathspec(arg) || check_filename(prefix, arg))


Thank you all for making this not only useful but also interesting and enriching.


Pd. A chronology:

15 Jan 2009 -      [1] - Patch to introduce "checkout '-'" that itself
                         introduces the notation @{-N}

17 Jan 2009 - ae5a6c36 - checkout: implement "@{-N}" shortcut name for N-th
                         last branch

17 Jan 2009 - 696acf45 - checkout: implement "-" abbreviation, add docs and
                         tests

13 Feb 2009 - 8415d5c7 - Teach the "@{-1} syntax to "git branch"

13 Feb 2009 - c9717ee9 - Teach @{-1} to git merge

21 Mar 2009 -      [2] - 1st attempt to have "-" as an universal alias of
                         @{-1}

 7 Apr 2011 - 4e8115ff - merge: allow "-" as a short-hand for "previous
                         branch"

 5 Sep 2013 - 182d7dc4 - cherry-pick: allow "-" as abbreviation of '@{-1}'

19 Jul 2013 -      [3] - [RFC] Delete current branch

19 Mar 2014 - 4f407407 - rebase: allow "-" short-hand for the previous branch

10 Mar 2015 -      [4] - [JFF] "-" and "@{-1}" on various program

16 Mar 2015 -      [5] - [PATCH/RFC 0/2][GSoC] revision.c: Allow "-" as
                         stand-in for "@{-1}" everywhere a branch is allowed

 5 Mar 2016 -      [6] - [PATCH] branch.c: Allow "-" as a short-hand for
                         "@{-1}" in "git branch -d @{-1}"

18 Mar 2016 -      [7] - [PATCH][Outreachy] branch: allow - as abbreviation of
                         '@{-1}'

27 May 2016 - 1a450e2f - worktree: allow "-" short-hand for @{-1} in add
                         command

 5 Feb 2017 -      [8] - [PATCH/RFC] WIP: log: allow "-" as a short-hand for
                         "previous branch"

 8 Mar 2017 -      [9] - [PATCH] diff: allow "-" as a short-hand for "last
                         branch"

21 Mar 2018 -      [A] - [PATCH] branch: implement shortcut to delete last
                         branch

29 Abr 2020 -      [B] - [PATCH] branch: add '-' to delete previous branch

16 Feb 2022 -      [C] - [PATCH] branch: delete now accepts '-' as branch name

 8 Ago 2022 -      [D] - [PATCH] branch: allow "-" as a short-hand for
                         "previous branch"


[1] 1231977976-8739-1-git-send-email-trast@student.ethz.ch
[2] 7vskl69xw3.fsf_-_@gitster.siamese.dyndns.org
[3] CALkWK0=8q4J2yi2to_+41kJSA5E59CBwkG69Hj7MmTPgUnSh5Q@mail.gmail.com
[4] xmqqy4n4zjst.fsf@gitster.dls.corp.google.com
[5] 1426518703-15785-1-git-send-email-kenny.lee28@gmail.com
[6] 1457176366-14952-1-git-send-email-dpdineshp2@gmail.com
[7] 1458305231-2333-1-git-send-email-elena.petrashen@gmail.com
[8] 1486299439-2859-1-git-send-email-kannan.siddharth12@gmail.com
[9] 1clZj4-0006vN-9q@crossperf.com
[A] 1521770966-18383-1-git-send-email-p@aaronjgreenberg.com
[B] 20200429130133.520981-1-pickfire@riseup.net
[C] pull.1217.git.git.1645020495014.gitgitgadget@gmail.com
[D] pull.1315.git.1659910949556.gitgitgadget@gmail.com
[E] 7abdb5a9-5707-7897-4196-8d2892beeb81@gmail.com
[F] 20120719213225.GA20311@sigill.intra.peff.net



 builtin/branch.c                      | 11 +++++++++++
 t/t3204-branch-name-interpretation.sh | 19 ++++++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 55cd9a6e99..66503d18d1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -245,6 +245,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		char *target = NULL;
 		int flags = 0;
 
+		if (argv[i] && !strcmp(argv[i], "-"))
+			argv[i] = "@{-1}";
+
 		strbuf_branchname(&bname, argv[i], allowed_interpret);
 		free(name);
 		name = mkpathdup(fmt, bname.buf);
@@ -527,6 +530,14 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
 			die(_("cannot rename the current branch while not on any."));
 	}
 
+	if (oldname && !strcmp(oldname, "-")) {
+		oldname = "@{-1}";
+	}
+
+	if (newname && !strcmp(newname, "-")) {
+		newname = "@{-1}";
+	}
+
 	if (strbuf_check_branch_ref(&oldref, oldname)) {
 		/*
 		 * Bad name --- this could be an attempt to rename a
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 993a6b5eff..84b646e1f7 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -57,14 +57,15 @@ test_expect_success 'create branch with pseudo-qualified name' '
 	expect_branch refs/heads/refs/heads/qualified two
 '
 
-test_expect_success 'delete branch via @{-1}' '
-	git branch previous-del &&
+test_expect_success 'delete branch via @{-N} and -' '
+	git checkout -b previous-one-del &&
+	git checkout -b previous-two-del &&
 
-	git checkout previous-del &&
 	git checkout main &&
 
-	git branch -D @{-1} &&
-	expect_deleted previous-del
+	git branch -D - @{-2} &&
+	expect_deleted previous-one-del &&
+	expect_deleted previous-two-del
 '
 
 test_expect_success 'delete branch via local @{upstream}' '
@@ -133,4 +134,12 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
 	expect_branch HEAD one
 '
 
+test_expect_success 'rename and copy branches via -' '
+	git checkout -b one-branch &&
+	git checkout - &&
+	git branch -c - two-branch &&
+	git branch -m - three-branch &&
+	git branch -m three-branch -
+'
+
 test_done
-- 
2.36.1

Re: [PATCH v4] branch: allow "-" as a shortcut for "previous branch"

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> Teach "git branch" the use of "-" as a shortcut for "@{-1}", like in "checkout
> -", "merge -" and other commands.

I am sorry that this is now at v4, but let's not do this.

"git checkout -" was started purely because "cd -" is a fairly well
known way to say "go back to the previous directory" (it is "cd ~-"
in some shells).

No shells accept "mv - newname" to rename the directory we were
previously in, or "rmdir -" to remove it.  And "diff -r - ." does
not compare the previous and the current directory recursively.
But all of these can be done (with some shells) if you use a proper
syntax, "mv ~- newname", "rmdir ~-", or "diff -r ~- .".

Yes, we may have by mistake added "git merge -" as well, but it is
perfectly fine to say that we should admit it as a mistake and plan
to possibly deprecate it in the longer term.  We shouldn't use it as
an excuse to make things even more confusing.

Thanks.  I think your other "git branch -d @{-1}" thing is sound,
though.

Re: [PATCH v4] branch: allow "-" as a shortcut for "previous branch"

[Rubén Justo]

Junio C Hamano wrote:

> I am sorry that this is now at v4, but let's not do this.

Fair enough.

> Thanks.  I think your other "git branch -d @{-1}" thing is sound,
> though.
 
Just to be sure. I'm assuming you're referring to the patch for the other
branch options: "-edit-description", "-set-upstream-to" and
"-unset-upstream". Routing their arguments through strbuf_branchname to
allow the expansion of @{-N}, @{u}...  Option "-d" is fine with this.
Otherwise please let me know.


Rubén.