Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"

[Siddharth Kannan]

On 21 February 2017 at 02:00, Junio C Hamano  wrote:
> Siddharth Kannan  writes:
> > So, is it okay to stop with just supporting "-" and not support things
> > like "-@{yesterday}"?
>
> If the approach to turn "-" into "@{-1}" at that spot you did will
> cause "-@{yesterday}" to barf, then I'd say so be it for now ;-).
> We can later spread the understanding of "-" to functions deeper in
> the callchain and add support for that, no?

Yes, this can be done later. I will send these patches again, with
only the changes that are discussed here.

I will keep the tests for "-@{yesterday}" as failing tests, if that
would help in finding this again and fixing it later.

Thanks for your review, Junio!

-- 

Best Regards,

- Siddharth Kannan.

Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"

[Junio C Hamano]

Siddharth Kannan  writes:

> On 17 February 2017 at 00:38, Junio C Hamano  wrote:
>> Having said all that, I do not think the remainder of the code is
>> prepared to take "-", not yet anyway [*1*], so turning "-" into
>> "@{-1}" this patch does before it calls get_sha1_basic(), while it
>> is not an ideal final state, is probably an acceptable milestone to
>> stop at.
>
> So, is it okay to stop with just supporting "-" and not support things
> like "-@{yesterday}"?

If the approach to turn "-" into "@{-1}" at that spot you did will
cause "-@{yesterday}" to barf, then I'd say so be it for now ;-).
We can later spread the understanding of "-" to functions deeper in
the callchain and add support for that, no?

>> It is a separate matter if this patch is sufficient to produce
>> correct results, though.  I haven't studied the callers of this
>> change to make sure yet, and may find bugs in this approach later.

Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"

[Siddharth Kannan]

On 17 February 2017 at 00:38, Junio C Hamano  wrote:
> Having said all that, I do not think the remainder of the code is
> prepared to take "-", not yet anyway [*1*], so turning "-" into
> "@{-1}" this patch does before it calls get_sha1_basic(), while it
> is not an ideal final state, is probably an acceptable milestone to
> stop at.

So, is it okay to stop with just supporting "-" and not support things
like "-@{yesterday}"?

Matthieu's comments on the matter:

Siddharth Kannan  writes:

> As per Matthieu's comments, I have updated the tests, but there
is still one
> thing that is not working: log -@{yesterday} or log -@{2.days.ago}

Note that I did not request that these things work, just that they seem
to be relevant tests: IMHO it's OK to reject them, but for example we
don't want them to segfault. And having a test is a good hint that you
thought about what could happen and to document it.

[Quoted from email ]


>
> It is a separate matter if this patch is sufficient to produce
> correct results, though.  I haven't studied the callers of this
> change to make sure yet, and may find bugs in this approach later.
>

-- 

Best Regards,

- Siddharth Kannan.

Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"

[Junio C Hamano]

Siddharth Kannan  writes:

> Instead of replacing the whole string, we would expand it accordingly using:
>
> if (*name == '-') {
>   if (len == 1) {
> name = "@{-1}";
> len = 5;
>   } else {
> struct strbuf changed_argument = STRBUF_INIT;
>
> strbuf_addstr(_argument, "@{-1}");
> strbuf_addstr(_argument, name + 1);
>
> strbuf_setlen(_argument, strlen(name) + 4);
>
> name = strbuf_detach(_argument, NULL);
>   }
> }
>
> Junio's comments on a previous version of the patch which used this same
> approach but inside setup_revisions [1]
>
> [1]: 

What I said is that when we know we got "-", there is no reason to
replace it with and textually parse "@{-1}".

> + if (*name == '-' && len == 1) {
> + name = "@{-1}";
> + len = 5;
> + }
> +
>   ret = get_sha1_basic(name, len, sha1, lookup_flags);

If we look at get_sha1_basic(), it obviously is not prepared to
understand "-" as "@{-1}", and the primary obstacle is that the
underlying interpret_nth_prior_checkout() does two things.  It
expects to take "@{-}" as a string, and the first half parses
the  into "long nth".  The latter half then finds the nth prior
checkout.  We probably should factor out the latter half into a
separate function find_nth_prior_checkout() that takes "long nth" as
input, and call it from interpret_nth_prior_checkout(), as a
preparatory step.  Once it is done, get_sha1_basic() can notice that
it was fed (len == 1 && str[0] == '-') and make a direct call to
find_nth_prior_checkout() without going through the "pass '@{-1}' as
text, have interpret_nth_prior_checkout() to parse it to recover 1",
which is a roundabout way to do what you want to do.

Having said all that, I do not think the remainder of the code is
prepared to take "-", not yet anyway [*1*], so turning "-" into
"@{-1}" this patch does before it calls get_sha1_basic(), while it
is not an ideal final state, is probably an acceptable milestone to
stop at.

It is a separate matter if this patch is sufficient to produce
correct results, though.  I haven't studied the callers of this
change to make sure yet, and may find bugs in this approach later.


[Footnote]

*1* For example, the existing callsite in get_sha1_basic() that
calls interpret_nth_prior_checkout() does not replace "str" with
what was returned when the HEAD is not detached.  The callpath
then depends on dwim_ref() to also understand "@{-1}" it got
from the caller.  If we really want to keep what came from the
end user as-is so that error message can include it, we'd need
to teach dwim_ref() about the new "-" convention.  The extent of
necessary change will become a lot larger.  On the other hand,
if we allow error messages and reports to use a real refname
instead of parrotting exactly what the user gave us, I think we
may be able to arrange to replace str/len in get_sha1_basic()
when we call interpret/find_nth_prior_checkout() and get a ref,
without having to teach the new "-" convention all over the
place.

[PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"

[Siddharth Kannan]

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

Instead of replacing the whole string, we would expand it accordingly using:

if (*name == '-') {
  if (len == 1) {
name = "@{-1}";
len = 5;
  } else {
struct strbuf changed_argument = STRBUF_INIT;

strbuf_addstr(_argument, "@{-1}");
strbuf_addstr(_argument, name + 1);

strbuf_setlen(_argument, strlen(name) + 4);

name = strbuf_detach(_argument, NULL);
  }
}

Junio's comments on a previous version of the patch which used this same
approach but inside setup_revisions [1]

[1]: 

 sha1_name.c  |   5 +++
 t/t4214-log-shorthand.sh | 106 +++
 2 files changed, 111 insertions(+)
 create mode 100755 t/t4214-log-shorthand.sh

diff --git a/sha1_name.c b/sha1_name.c
index 73a915f..2f86bc9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, unsigned 
char *sha1, unsigned l
if (!ret)
return 0;
 
+   if (*name == '-' && len == 1) {
+   name = "@{-1}";
+   len = 5;
+   }
+
ret = get_sha1_basic(name, len, sha1, lookup_flags);
if (!ret)
return 0;
diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh
new file mode 100755
index 000..659b100
--- /dev/null
+++ b/t/t4214-log-shorthand.sh
@@ -0,0 +1,106 @@
+#!/bin/sh
+
+test_description='log can show previous branch using shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+   test_commit first &&
+   test_commit second &&
+   test_commit third &&
+   test_commit fourth &&
+   test_commit fifth &&
+   test_commit sixth &&
+   test_commit seventh
+'
+
+test_expect_success '"log -" should not work initially' '
+   test_must_fail git log -
+'
+
+test_expect_success 'setup branches for testing' '
+   git checkout -b testing-1 master^ &&
+   git checkout -b testing-2 master~2 &&
+   git checkout master
+'
+
+test_expect_success '"log -" should work' '
+   git log testing-2 >expect &&
+   git log - >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'symmetric revision range should work when one end is left 
empty' '
+   git checkout testing-2 &&
+   git checkout master &&
+   git log ...@{-1} >expect.first_empty &&
+   git log @{-1}... >expect.last_empty &&
+   git log ...- >actual.first_empty &&
+   git log -... >actual.last_empty &&
+   test_cmp expect.first_empty actual.first_empty &&
+   test_cmp expect.last_empty actual.last_empty
+'
+
+test_expect_success 'asymmetric revision range should work when one end is 
left empty' '
+   git checkout testing-2 &&
+   git checkout master &&
+   git log ..@{-1} >expect.first_empty &&
+   git log @{-1}.. >expect.last_empty &&
+   git log ..- >actual.first_empty &&
+   git log -.. >actual.last_empty &&
+   test_cmp expect.first_empty actual.first_empty &&
+   test_cmp expect.last_empty actual.last_empty
+'
+
+test_expect_success 'symmetric revision range should work when both ends are 
given' '
+   git checkout testing-2 &&
+   git checkout master &&
+   git log -...testing-1 >expect &&
+   git log testing-2...testing-1 >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'asymmetric revision range should work when both ends are 
given' '
+   git checkout testing-2 &&
+   git checkout master &&
+   git log -..testing-1 >expect &&
+   git log testing-2..testing-1 >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'multiple separate arguments should be handled properly' '
+   git checkout testing-2 &&
+   git checkout master &&
+   git log - - >expect.1 &&
+   git log @{-1} @{-1} >actual.1 &&
+   git log - HEAD >expect.2 &&
+   git log @{-1} HEAD >actual.2 &&
+   test_cmp expect.1 actual.1 &&
+   test_cmp expect.2 actual.2
+'
+
+test_expect_success 'revision ranges with same start and end should be empty' '
+   git checkout testing-2 &&
+   git checkout master &&