v3, 2 partsWIP: allow "-" as a shorthand for "previous branch"

Thanks a lot, Matthieu, for your comments on an earlier version of this patch! [1]

After the discussion there, I have considered the changes that have been made and I broke them into two separate commits.

I have included the list of commands that are touched by this patch series in the second commit message. (idea from the v1 discussion [2]) Reproduced here:

  * builtin/blame.c
  * builtin/diff.c
  * builtin/diff-files.c
  * builtin/diff-index.c
  * builtin/diff-tree.c
  * builtin/log.c
  * builtin/rev-list.c
  * builtin/shortlog.c
  * builtin/fast-export.c
  * builtin/fmt-merge-msg.c
  builtin/add.c
  builtin/checkout.c
  builtin/commit.c
  builtin/merge.c
  builtin/pack-objects.c
  builtin/revert.c

  * marked commands are information-only.

I have added the WIP tag because I am still unsure if the tests that I have added (for git-log) are sufficient for this patch or more comprehensive tests need to be added. So, please help me with some feedback on that.

I have removed the "log:" tag from the subject line because this patch now affects commands other than log.

I have run the test suite locally and on Travis CI! [3]

[1]: https://public-inbox.org/git/vpqh944eof7.fsf@anie.imag.fr/#t [2]: https://public-inbox.org/git/CAN-3QhoZN_wYvqbVdU_c1h4vUOaT5FOBFL7k+FemNpqkxjWDDA@mail.gmail.com/ [3]: https://travis-ci.org/icyflame/git/builds/200431159

Siddharth Kannan (2):
  revision.c: args starting with "-" might be a revision
  sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"

 revision.c               | 12 ++++++--
 sha1_name.c              |  5 ++++
 t/t4214-log-shorthand.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+), 2 deletions(-)
 create mode 100755 t/t4214-log-shorthand.sh

setup_revisions used to consider any argument starting with "-" to be either a valid option or nothing at all. This patch will teach it to check if the argument is a revision before declaring that it is nothing at all.

Before this patch, handle_revision_arg was not called for arguments starting with "-" and once for arguments that didn't start with "-". Now, it will be called once per argument.

This patch prepares the addition of "-" as a shorthand for "previous branch".

Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
 revision.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

This patch introduces "-" as a method to refer to a revision, and adds tests to test that git-log works with this shorthand.

This change will touch the following commands (through revision.c:setup_revisions):

* builtin/blame.c
* builtin/diff.c
* builtin/diff-files.c
* builtin/diff-index.c
* builtin/diff-tree.c
* builtin/log.c
* builtin/rev-list.c
* builtin/shortlog.c
* builtin/fast-export.c
* builtin/fmt-merge-msg.c
builtin/add.c
builtin/checkout.c
builtin/commit.c
builtin/merge.c
builtin/pack-objects.c
builtin/revert.c

* marked commands are information-only.

As most commands in this list are not of the rm-variety, (i.e a command that would delete something), this change does not make it easier for people to delete. (eg: "git branch -d -" is *not* enabled by this patch)

Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
 sha1_name.c              |  5 ++++
 t/t4214-log-shorthand.sh | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)
 create mode 100755 t/t4214-log-shorthand.sh

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

One drawback of this approach is that further error messages will be given from the "@{-1}" string that the user never typed.

After you do that, the existing "turn - into @{-1}" pieces of code become useless and you should remove it (probably in a further patch).

There are at least:

$ git grep -n -A1 'strcmp.*"-"' | grep -B 1 '@\{1\}' builtin/checkout.c:975: if (!strcmp(arg, "-")) builtin/checkout.c-976- arg = "@{-1}"; -- builtin/merge.c:1231: } else if (argc == 1 && !strcmp(argv[0], "-")) { builtin/merge.c-1232- argv[0] = "@{-1}"; -- builtin/revert.c:158: if (!strcmp(argv[1], "-")) builtin/revert.c-159- argv[1] = "@{-1}"; -- builtin/worktree.c:344: if (!strcmp(branch, "-")) builtin/worktree.c-345- branch = "@{-1}";

In the final version, obviously the same "refactoring" (specific command -> git-wide) should be done for documentation (it should be in this patch to avoid letting not-up-to-date documentation even for a single commit).

You may use test_commit to save a few lines of code.

I'd have split this into a "setup branches" and a '"log -" should work' test (to actually see where "setup branches" happen in the output, and to allow running the setup step separately if needed). Not terribly important.

Nitpick: we stick the > and the filename (as you did in most places
already).

It may be worth adding tests for more cases like

* Check what happens with suffixes, i.e. -^, -@{yesterday} and -~.

* -..- -> to make sure you handle the presence of two - properly.

* multiple separate arguments to make sure you handle them all, e.g.
  "git log - -", "git log HEAD -", "git log - HEAD".

The last two may be overkill, but the first one is probably important.

Hey Matthieu, On Sun, Feb 12, 2017 at 10:48:56AM +0100, Matthieu Moy wrote:

Yeah, this is currently also implemented in checkout, apart from the grepped list that you have supplied here. I will find all the instances, and ensure that they work, and remove them. (This will require some more digging into the codepath the commands, to ensure that get_sha1_1 is called somewhere down the line)

Oh, yeah! I will use that. I need to work on improving the tests, as well as adding the documentation.

Sorry, slipped my mind!

These do not work right now. The first and last cases here are handled by peel_onion, if I remember correctly. I have to find out why exactly these are not working. Thanks for mentioning this!

Yeah, will add these tests.

-- Regards,

Siddharth Kannan.

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Right.

I didn't check the surrounding context to be sure, but I think this "- to @{-1}" conversion cannot be delegated down to revision parsing that eventually wants to return a 40-hex as the result.

We do want a branch _name_ sometimes when we say "@{-1}"; "checkout master" (i.e. checkout by name) and "checkout master^0" (i.e. the same commit object, but not by name) do different things.

These should be safe to delegate down.

I do not know about this one, but it smells like a branch name that wants to be used before it gets turned into 40-hex.

Junio C Hamano <gitster@pobox.com> writes:

FYI, the "@{-<number>} to branch name" translation happens in interpret_branch_name(). I do not offhand recall if any callers protect their calls to the function with conditionals that assume the thing must begin with "@{" or cannot begin with "-" (the latter of which is similar to the topic of patch 1/2 of this series), but I suspect that teaching the function that "-" means the same as "@{-1}" would bring us closer to where we want to go.

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

Naively I would have expected that removing the "continue" at the end and letting the control go to the existing

	if (handle_revision_arg(arg, revs, flags, revarg_opt)) {

would be all that is needed. The latter half of the patch is an artifact of having ane xtra "handle_revision_arg() calls inside the "if it begins with dash" block to avoid calling it twice.

So the difference is just "--left" (by the way, our codebase seem to prefer "left--" when there is no difference between pre- or post- decrement/increment) that adjusts the slot in argv[] where the next unknown argument is stuffed to.

The adjustment is needed as the call to handle_revision_opt() that is before the pre-context of this hunk stuffed the unknown thing that begins with "-" into argv[left++]; if that thing turns out to be a valid rev, then you would need to take it back, because after all, that is not an unknown command line argument.

I am wondering if writing it like the following is easier to understand. I had a hard time figuring out what you are trying to do, partly because "args" is quite a misnomer---implying "how many arguments did we see" that is similar to opts that does mean "how many options did handle_revision_opts() see?" The variable means means "yes we saw a valid rev" when it is zero. The rewrite below may avoid such a confusion. I dunno.

 revision.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Hey Junio, On Fri, Feb 10, 2017 at 03:35:47PM -0800, Junio C Hamano wrote:

Understood, I will use post decrement.

Um, okay, I see that "args" is very confusing. Would it help if this variable was called "arg_not_rev"? Because the value that is returned from handle_revision_arg is 1 when it is not a revision, and 0 when it is a revision. The changed block of code would look like this:

---
 revision.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

Not really. If we absolutely need to have one variable that is meant to escape the "if it begins with a dash" block and to affect what comes next, I think the variable should mean "we know we saw a revision and you do not have to call it again". IOW the code that needs to do "handle_rev_arg_called && arg_not_rev" is just being silly. At that point in the codeflow, I do not see why the code needs to take two bits of information and combine them; the one that sets these two variables should have done the work for it.

And that would make the if statement slightly easier to read compared to the original. I am however not suggesting to do that; read on.

The function follows the convention to return 0 for success, -1 for error/unexpected, by the way.

Yes, but it is no more confusing than your original "left--".

If we want to make the flow of logic easier to follow, we need to step back and view what the codepath _wants_ to do at the higher level, which is:

 * If it is an option known to us, handle it and go to the next arg.

 * If it is an option that we do not understand, stuff it in
   argv[left++] and go to the next arg.

 * If it is a rev, handle it, and note that fact in got_rev_arg.

 * If it is not a rev and we haven't seen dashdash, verify that it
   and everything that follows it are pathnames (which is an inexact
   but a cheap way to avoid ambiguity), make all them the prune_data
   and conclude.

Because the second step currently is implemented by calling handle_opt(), which not just tells if it is an option we understand or not, but also mucks with argv[left++], you need to undo it once you start making it possible for a valid "rev" to begin with a dash. That is what your left-- was, and that is what "decrement and then increment when it turns out it was an unknown option after all" is.

The first step to a saner flow _could_ be to stop passing the unkv and unkc to handle_revision_opt() and instead make the caller responsible for doing that. That would express what your patch wanted to do in the most natural way, i.e.

 * If it is an option known to us, handle it and go to the next arg.

 * If it is a rev, handle it, and note that fact in got_rev_arg
   (this change of order enables you to allow a rev that begins with
   a dash, which would have been misrecognised as a possible unknown
   option).

 * If it looks like an option (i.e. "begins with a dash"), then we
   already know that it is not something we understand, because the
   first step would have caught it already.  Stuff it in
   argv[left++] and go to the next arg.

 * If it is not a rev and we haven't seen dashdash, verify that it
   and everything that follows it are pathnames (which is an inexact
   but a cheap way to avoid ambiguity), make all them the prune_data
   and conclude.

Such a change to handle_revision_opt() unfortunately affects other callers of the function, so it may not be worth it, but I think "decrement and then increment, because this codepath wants to check to see something that may ordinarily be clasified as an unknown option if it is a rev" is an ugly workaround, just like your left-- was. But I think the resulting code flow is much closer to the above ideal.

Junio C Hamano <gitster@pobox.com> writes:

Having re-analysed the codepath like so, I realize that the new variable I introduced was misnamed. Its purpose is to let the "if arg begins with dash, do this" block communicate that what the later part of the code is told to inspect in "arg" may be an option that we do not recognise. So I shouldn't have called it maybe_rev; the message from the former to the latter is "this may be an unknown option" and I should have called it "maybe_unknown_opt".

It turns out that telling handle_revision_opt() not to molest argv[left++] does not have heavy fallout.

Junio C Hamano (3):
  handle_revision_opt(): do not update argv[left++] with an unknown arg
  setup_revisions(): swap if/else bodies to make the next step more readable
  setup_revisions(): allow a rev that begins with a dash

 revision.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

In future steps, we will make it possible for a rev or a revision range (i.e. what is understood by handle_revision_arg() helper) to begin with a dash. The setup_revisions() function however currently considers anything that begins with a dash to be:

 - an option it itself understands and handles (some take effect by
   setting fields in the revision structure, some others are left
   in the argv[left++] to be handled in later steps);
 - an option handle_revision_opt() understands and tells us to skip;
 - an option handle_revision_opt() found to be incorrect; or
 - an option handle_revision_opt() did not understand, which is
   stuffed in argv[left++].

and does not give a chance to handle_revision_arg() to inspect it. The handle_revision_opt() function returns a positive count, a negative count or zero to allow the caller to tell the latter three cases apart. A rev that begins with a dash would be thrown into the last category.

Teach handle_revision_opt() not to touch argv[left++] in the last case. Because the other one among the two callers of this function immediately errors out with the usage string when it returns zero (i.e. the last case above), there is no negative effect to that caller.

In setup_revisions(), which is the other caller of this function, we need to stuff the unknown arg to argv[left++] in this case, to preserve the current behaviour.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Swap the condition and bodies of an "if (A) do_A else do_B" in setup_revisions() to "if (!A) do_B else do A", to make the change in the the next step easier to read.

No behaviour change is intended in this step.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Now all the preparatory pieces are in place, it is a matter of handling a truly unknown option _after_ handle_revision_arg() decides that arg is not a rev.

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

 We _could_ do without a new variable maybe_opt and instead check if
 arg begins with a dash one more time, but it is cleaner to do it
 the way this patch does to avoid writing the same check twice.  We
 may be hit with a desire similar to but an opposite of the current
 topic (which wants to allow a rev that begins with a dash), to
 start allowing an option that does not begin with a dash someday.

 revision.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

On Sat, Feb 11, 2017 at 01:08:09PM -0800, Junio C Hamano wrote:

So, our problem here is that the function handle_revision_opt is opaquely also incrementing "left", which we need to decrement somehow.

Or: we could change the flow of the code so that this incrementing
will happen only when we have decided that the argument is not a
revision.

This "changing the order" gave me the idea to change the flow. I tried to implement the above steps without touching the function handle_revision_opt. By inserting the handle_revision_arg call just before calling handle_revision_opt.

The decrementing then incrementing or "left--" things have now been removed. (But there is still one thing which doesn't look good)

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

Changing the order is changing the order of the function calls, i.e. changing the flow. So at the idea level we are on the same page.

I was shooting for not having to duplicate calls to handle_revision_arg().

See the 3-patch series I just sent out. I didn't think it through very carefully (especially the error message the other caller produces), but the whole thing _smells_ correct to me.

Hey Junio,

Okay, got it! I will write-up those changes, and make sure nothing bad happens. (Also, the one other function that calls handle_revision_opt, parse_revision_opt needs to be fixed for any changes in handle_revision_opt.)

I will do all of this in the next week (Unfortunately, exams!) and submit a new version of this patch (Also, I need to update tests, add documentation, and remove code that does this shorthand stuff for other commands as per Matthieu's comments)

-- Best Regards,

Siddharth Kannan.