[PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"

[PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"

[Siddharth Kannan]

This is as per our discussion[1]. The patches and commit messages are based on
Junio's patches that were posted as a reply to
<20170212184132.12375-1-gitster@pobox.com>.

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}

For the other kinds of suffixes, such as -^ or -~ or -~N, the suffix
information is first extracted and then, the function get_sha1_1 is called with
name="-^" and len=1 (which is the reason for the changed condition inside Patch
4 of this series).

For -@{yesterday} kind of queries, the functions dwim_log,
interpret_branch_name and interpret_nth_prior_checkout are called.

1. A nice way to solve this would be to extend the replacement of "-" with
"@{-1}" one step further. Using strbuf, instead of replacing the whole string
with "@{-1}" we would simply replace "-" with "@{-1}" expanding the string
appropriately. This will ensure that all the code is inside the function
get_sha1_1. The code to do this is in the cover section of the 4th patch in this
series.

2. we could go down the dwim_log codepath, and find another suitable place to
make the same "-" -> "@{-1}" replacement. In the time that I spent till now, it
seems that the suffix information (i.e.  @{yesterday} or @{2.days.ago}) is
extracted _after_ the branch information has been extracted, so I suspect that
we will have to keep that part intact even in this solution.  (I am not too
sure about this. If this is the preferred solution, then I will dig deeper and
find the right place as I did for the first part of this patch)

Matthieu: Thanks a lot for your comments on the tests! test_commit has made the
tests a lot cleaner!

[1]: <xmqqh941ippo.fsf@gitster.mtv.corp.google.com>

Siddharth Kannan (4):
  revision.c: do not update argv with unknown option
  revision.c: swap if/else blocks
  revision.c: args starting with "-" might be a revision
  sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"

 revision.c               |  15 ++++---
 sha1_name.c              |   5 +++
 t/t4214-log-shorthand.sh | 106 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 120 insertions(+), 6 deletions(-)
 create mode 100755 t/t4214-log-shorthand.sh

-- 
2.1.4

[PATCH 1/4 v4] revision.c: do not update argv with unknown option

[Siddharth Kannan]

handle_revision_opt() tries to recognize and handle the given argument. If an
option was unknown to it, it used to add the option to unkv[(*unkc)++].  This
increment of unkc causes the variable in the caller to change.

Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
This is now the responsibility of the caller.

There are two callers of this function:

1. setup_revision: Changes have been made so that setup_revision will now
update the unknown option in argv

2. parse_revision_opt: No changes are required here. This function throws an
error whenever the option provided as argument was unknown to
handle_revision_opt().

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

diff --git a/revision.c b/revision.c
index b37dbec..5674a9a 100644
--- a/revision.c
+++ b/revision.c
@@ -2016,8 +2016,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->ignore_missing = 1;
 	} else {
 		int opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);
-		if (!opts)
-			unkv[(*unkc)++] = arg;
 		return opts;
 	}
 	if (revs->graph && revs->track_linear)
@@ -2234,6 +2232,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			}
 			if (opts < 0)
 				exit(128);
+			/* arg is an unknown option */
+			argv[left++] = arg;
 			continue;
 		}
 
-- 
2.1.4

Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option

[Matthieu Moy]

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

> handle_revision_opt() tries to recognize and handle the given argument. If an
> option was unknown to it, it used to add the option to unkv[(*unkc)++].  This
> increment of unkc causes the variable in the caller to change.
>
> Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
> This is now the responsibility of the caller.
>
> There are two callers of this function:
>
> 1. setup_revision: Changes have been made so that setup_revision will now
> update the unknown option in argv

You're writting "Changes have been made", but I did not see any up to
this point in the series.

We write patch series so that they are bisectable, i.e. each commit
should be correct (compileable, pass tests, consistent
documentation, ...). Here, it seems you are introducing a breakage to
repair it later.

Other that bisectability, this makes review harder: at this point the
reader knows it's broken, guesses that it will be repaired later, but
does not know in which patch.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option

[Junio C Hamano]

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

> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>
>> handle_revision_opt() tries to recognize and handle the given argument. If an
>> option was unknown to it, it used to add the option to unkv[(*unkc)++].  This
>> increment of unkc causes the variable in the caller to change.
>>
>> Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
>> This is now the responsibility of the caller.
>>
>> There are two callers of this function:
>>
>> 1. setup_revision: Changes have been made so that setup_revision will now
>> update the unknown option in argv
>
> You're writting "Changes have been made", but I did not see any up to
> this point in the series.

Actually, I think you misread the patch and explanation.
handle_revision_opt() used to be responsible for stuffing unknown
ones to unkv[] array passed from the caller even when it returns 0
(i.e. "I do not know what they are" case, as opposed to "I know what
they are, I am not handling them here and leaving them in unkv[]"
case--the latter returns non-zero).  The first hunk makes the
function stop doing so, and to compensate, the second hunk, which is
in setup_revisions() that calls the function, now makes the caller
do the equivalent "argv[left++] = arg" there after it receives 0.

So "Changes have been made" to setup_revisions() to compensate for
the change of behaviour in the called function.

The enumerated point 2. (not in your response) explains why such a
corresponding compensatory change is not there for the other caller
of this function whose behaviour has changed.

> We write patch series so that they are bisectable, i.e. each commit
> should be correct (compileable, pass tests, consistent
> documentation, ...). Here, it seems you are introducing a breakage to
> repair it later.

That is a very good point to stress, but 1. is exactly to avoid
breakage in this individual step (and 2. is an explanation why the
change does not break the other caller).

Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>>
>>> handle_revision_opt() tries to recognize and handle the given argument. If an
>>> option was unknown to it, it used to add the option to unkv[(*unkc)++].  This
>>> increment of unkc causes the variable in the caller to change.
>>>
>>> Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
>>> This is now the responsibility of the caller.
>>>
>>> There are two callers of this function:
>>>
>>> 1. setup_revision: Changes have been made so that setup_revision will now
>>> update the unknown option in argv
>>
>> You're writting "Changes have been made", but I did not see any up to
>> this point in the series.
>
> Actually, I think you misread the patch and explanation.
> handle_revision_opt() used to be responsible for stuffing unknown
> ones to unkv[] array passed from the caller even when it returns 0
> (i.e. "I do not know what they are" case, as opposed to "I know what
> they are, I am not handling them here and leaving them in unkv[]"
> case--the latter returns non-zero).  The first hunk makes the
> function stop doing so, and to compensate, the second hunk, which is
> in setup_revisions()

Indeed, I misread the patch. The explanation could be a little bit more
"tired-reviewer-proof" by not using a past tone, perhaps

1. setup_revision, which is changed to ...

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option

[Siddharth Kannan]

Hey Matthieu,

On 16 February 2017 at 23:52, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>
> Indeed, I misread the patch. The explanation could be a little bit more
> "tired-reviewer-proof" by not using a past tone, perhaps
>
> 1. setup_revision, which is changed to ...

Oh, okay! Sorry about the confusion!

Yes, I used the past perfect tense to refer to changes that were made
in this particular patch!

I will change the message in the next version to something that's in
present tense.

>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/



-- 

Best Regards,

- Siddharth.

[PATCH 2/4 v4] revision.c: swap if/else blocks

[Siddharth Kannan]

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: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
 revision.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index 5674a9a..8d4ddae 100644
--- a/revision.c
+++ b/revision.c
@@ -2238,7 +2238,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		}
 
 
-		if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+		if (!handle_revision_arg(arg, revs, flags, revarg_opt))
+			got_rev_arg = 1;
+		else {
 			int j;
 			if (seen_dashdash || *arg == '^')
 				die("bad revision '%s'", arg);
@@ -2255,8 +2257,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			append_prune_data(&prune_data, argv + i);
 			break;
 		}
-		else
-			got_rev_arg = 1;
 	}
 
 	if (prune_data.nr) {
-- 
2.1.4

[PATCH 3/4 v4] revision.c: args starting with "-" might be a revision

[Siddharth Kannan]

setup_revisions used to consider any argument starting with "-" to be either a
valid or an unknown option.

Teach setup_revisions to check if an argument is a revision before adding it as
an unknown option (something that setup_revisions didn't understand) to argv,
and moving on to the next argument.

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

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

diff --git a/revision.c b/revision.c
index 8d4ddae..5470c33 100644
--- a/revision.c
+++ b/revision.c
@@ -2203,6 +2203,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	read_from_stdin = 0;
 	for (left = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
+		int maybe_opt = 0;
 		if (*arg == '-') {
 			int opts;
 
@@ -2232,15 +2233,17 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			}
 			if (opts < 0)
 				exit(128);
-			/* arg is an unknown option */
-			argv[left++] = arg;
-			continue;
+			maybe_opt = 1;
 		}
 
 
 		if (!handle_revision_arg(arg, revs, flags, revarg_opt))
 			got_rev_arg = 1;
-		else {
+		else if (maybe_opt) {
+			/* arg is an unknown option */
+			argv[left++] = arg;
+			continue;
+		} else {
 			int j;
 			if (seen_dashdash || *arg == '^')
 				die("bad revision '%s'", arg);
-- 
2.1.4

[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 <kannan.siddharth12@gmail.com>
---

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(&changed_argument, "@{-1}");
    strbuf_addstr(&changed_argument, name + 1);

    strbuf_setlen(&changed_argument, strlen(name) + 4);

    name = strbuf_detach(&changed_argument, NULL);
  }
}

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

[1]: <xmqqtw882n08.fsf@gitster.mtv.corp.google.com>

 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 0000000..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 &&
+	test 0 -eq $(git log -...- | wc -l) &&
+	test 0 -eq $(git log -..- | wc -l)
+'
+
+test_expect_success 'suffixes to - should work' '
+	git checkout testing-2 &&
+	git checkout master &&
+	git log -~ >expect.1 &&
+	git log @{-1}~ >actual.1 &&
+	git log -~2 >expect.2 &&
+	git log @{-1}~2 >actual.2 &&
+	git log -^ >expect.3 &&
+	git log @{-1}^ >actual.3 &&
+	git log -@{yesterday} >expect.4 &&
+	git log @{-1}@{yesterday} >actual.4 &&
+	test_cmp expect.1 actual.1 &&
+	test_cmp expect.2 actual.2 &&
+	test_cmp expect.3 actual.3 &&
+	test_cmp expect.4 actual.4
+'
+
+test_done
-- 
2.1.4

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

[Junio C Hamano]

Siddharth Kannan <kannan.siddharth12@gmail.com> 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(&changed_argument, "@{-1}");
>     strbuf_addstr(&changed_argument, name + 1);
>
>     strbuf_setlen(&changed_argument, strlen(name) + 4);
>
>     name = strbuf_detach(&changed_argument, NULL);
>   }
> }
>
> Junio's comments on a previous version of the patch which used this same
> approach but inside setup_revisions [1]
>
> [1]: <xmqqtw882n08.fsf@gitster.mtv.corp.google.com>

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 "@{-<num>}" as a string, and the first half parses
the <num> 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.

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 <gitster@pobox.com> 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 <kannan.siddharth12@gmail.com> 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 <vpqa89mnl4z.fsf@anie.imag.fr>]


>
> 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 <kannan.siddharth12@gmail.com> writes:

> On 17 February 2017 at 00:38, Junio C Hamano <gitster@pobox.com> 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 21 February 2017 at 02:00, Junio C Hamano <gitster@pobox.com> wrote:
> Siddharth Kannan <kannan.siddharth12@gmail.com> 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 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"

[Matthieu Moy]

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

> This is as per our discussion[1]. The patches and commit messages are based on
> Junio's patches that were posted as a reply to
> <20170212184132.12375-1-gitster@pobox.com>.
>
> 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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"

[Junio C Hamano]

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

> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>
>> This is as per our discussion[1]. The patches and commit messages are based on
>> Junio's patches that were posted as a reply to
>> <20170212184132.12375-1-gitster@pobox.com>.
>>
>> 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.

The branch we were on before would be a ref, and the ref may know
where it was yesterday?  If @{-1}@{1.day} works it would be natural
to expect -@{1.day} to, too, but there probably is some disambiguity
or other reasons that they cannot or should not work that way I am
missing, in which case it is fine ("too much work for too obscure
feature that is not expected to be used often" is also an acceptable
reason) to punt or deliberately not support it, as long as it is
explained in the log and/or doc (future developers need to know if
we are simply punting, or if we found a case where it would hurt end
user experience if we supported the feature), and as long as it does
not do a wrong thing (dying with "we do not support it" is OK,
segfaulting or doing random other things is not).

Thanks.

Re: [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"

[Siddharth Kannan]

Hey Junio and Matthieu,

On 17 February 2017 at 00:19, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>>
>>> This is as per our discussion[1]. The patches and commit messages are based on
>>> Junio's patches that were posted as a reply to
>>> <20170212184132.12375-1-gitster@pobox.com>.
>>>
>>> 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.
>
> The branch we were on before would be a ref, and the ref may know
> where it was yesterday?  If @{-1}@{1.day} works it would be natural
> to expect -@{1.day} to, too, but there probably is some disambiguity
> or other reasons that they cannot or should not work that way I am
> missing, in which case it is fine ("too much work for too obscure
> feature that is not expected to be used often" is also an acceptable
> reason) to punt or deliberately not support it, as long as it is
> explained in the log and/or doc (future developers need to know if
> we are simply punting, or if we found a case where it would hurt end
> user experience if we supported the feature), and as long as it does
> not do a wrong thing (dying with "we do not support it" is OK,
> segfaulting or doing random other things is not).
>

Right now, these commands die with an "fatal: unrecognized argument:
-@{yesterday}" or a "fatal: unrecognized argument: -@{2.days.ago}".
So, it is definitely not doing anything "random" :)

I will wait for consensus on whether these should or should not be
supported.

-- 

Best Regards,

- Siddharth.