| Message ID | pull.1315.git.1659910949556.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | branch: allow "-" as a short-hand for "previous branch" | expand |
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 >
"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.
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.
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...
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 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.
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.
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
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
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.
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
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
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.
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.
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 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) {
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;