branch: allow "-" as a short-hand for "previous branch" #1315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Welcome to GitGitGadgetHi @rjusto, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that your Pull Request has a good description, as it will be used as cover letter. You can CC potential reviewers by adding a footer to the PR description with the following syntax: Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
/allow |
|
User rjusto is now allowed to use GitGitGadget. WARNING: rjusto has no public email address set on GitHub; |
|
/preview |
|
Error: Could not determine full name of rjusto |
|
/submit |
|
Error: Could not determine full name of rjusto |
|
/preview |
|
Preview email sent as pull.1315.git.1659910686222.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1315.git.1659910949556.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): 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
> |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "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. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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. |
|
On the Git mailing list, Rubén Justo wrote (reply to this): 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...
|
|
User |
|
On the Git mailing list, Rubén Justo wrote (reply to this): 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.
|
|
On the Git mailing list, Rubén Justo wrote (reply to this): 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. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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.
|
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): 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 |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): 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 |
|
/preview |
|
Preview email sent as pull.1315.v2.git.1660668436362.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1315.v2.git.1660669912043.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Rubén Justo wrote (reply to this): 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. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "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. |
|
/preview |
|
Preview email sent as pull.1315.v3.git.1660677298033.gitgitgadget@gmail.com |
|
/preview |
|
Preview email sent as pull.1315.v3.git.1660683273554.gitgitgadget@gmail.com |
|
On the Git mailing list, Rubén Justo wrote (reply to this): 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 |
|
User |
|
On the Git mailing list, Rubén Justo wrote (reply to this): 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. |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): 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 |
|
User |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): 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 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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. |
|
On the Git mailing list, Rubén Justo wrote (reply to this): 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. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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).
|
|
On the Git mailing list, Rubén Justo wrote (reply to this): 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)
{ |
|
On the Git mailing list, Rubén Justo wrote (reply to this): 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 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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. |
|
On the Git mailing list, Rubén Justo wrote (reply to this): 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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
cc: Rubén Justo rjusto@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de