# [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch" 16 messages from 2017-02-16 to 2017-02-22. Participants: Siddharth Kannan, Matthieu Moy, Junio C Hamano. Thread: https://gitlist.dev/t/45153 ## Siddharth Kannan, 2017-02-16 15:14 Subject: [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch" Message-ID: <1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com> URL: https://gitlist.dev/e/1487258054-32292-1-git-send-email-kannan.siddharth12%40gmail.com ``` 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]: 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 ``` ## Siddharth Kannan, 2017-02-16 15:14 Subject: [PATCH 1/4 v4] revision.c: do not update argv with unknown option Message-ID: <1487258054-32292-2-git-send-email-kannan.siddharth12@gmail.com> URL: https://gitlist.dev/e/1487258054-32292-2-git-send-email-kannan.siddharth12%40gmail.com In-Reply-To: <1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com> ``` 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 --- 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 ``` ## Siddharth Kannan, 2017-02-16 15:14 Subject: [PATCH 2/4 v4] revision.c: swap if/else blocks Message-ID: <1487258054-32292-3-git-send-email-kannan.siddharth12@gmail.com> URL: https://gitlist.dev/e/1487258054-32292-3-git-send-email-kannan.siddharth12%40gmail.com In-Reply-To: <1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com> ``` 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 --- 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 ``` ## Siddharth Kannan, 2017-02-16 15:14 Subject: [PATCH 3/4 v4] revision.c: args starting with "-" might be a revision Message-ID: <1487258054-32292-4-git-send-email-kannan.siddharth12@gmail.com> URL: https://gitlist.dev/e/1487258054-32292-4-git-send-email-kannan.siddharth12%40gmail.com In-Reply-To: <1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com> ``` 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 --- 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 ``` ## Siddharth Kannan, 2017-02-16 15:14 Subject: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" Message-ID: <1487258054-32292-5-git-send-email-kannan.siddharth12@gmail.com> URL: https://gitlist.dev/e/1487258054-32292-5-git-send-email-kannan.siddharth12%40gmail.com In-Reply-To: <1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com> ``` 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(&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]: 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 ``` ## Matthieu Moy, 2017-02-16 16:41 Subject: Re: [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch" Message-ID: URL: https://gitlist.dev/e/vpqa89mnl4z.fsf%40anie.imag.fr In-Reply-To: <1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com> ``` Siddharth Kannan 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/ ``` ## Matthieu Moy, 2017-02-16 16:48 Subject: Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option Message-ID: URL: https://gitlist.dev/e/vpqwpcqm69k.fsf%40anie.imag.fr In-Reply-To: <1487258054-32292-2-git-send-email-kannan.siddharth12@gmail.com> ``` Siddharth Kannan 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/ ``` ## Junio C Hamano, 2017-02-16 18:11 Subject: Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option Message-ID: URL: https://gitlist.dev/e/xmqqwpcqxay0.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Matthieu Moy writes: > Siddharth Kannan 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). ``` ## Matthieu Moy, 2017-02-16 18:22 Subject: Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option Message-ID: URL: https://gitlist.dev/e/vpqwpcqgfmw.fsf%40anie.imag.fr In-Reply-To: ``` Junio C Hamano writes: > Matthieu Moy writes: > >> Siddharth Kannan 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/ ``` ## Junio C Hamano, 2017-02-16 18:49 Subject: Re: [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch" Message-ID: URL: https://gitlist.dev/e/xmqqinoax96u.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Matthieu Moy writes: > Siddharth Kannan 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. ``` ## Junio C Hamano, 2017-02-16 19:08 Subject: Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" Message-ID: URL: https://gitlist.dev/e/xmqq8tp6x8b6.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1487258054-32292-5-git-send-email-kannan.siddharth12@gmail.com> ``` 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(&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]: 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. ``` ## Siddharth Kannan, 2017-02-16 19:39 Subject: Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option Message-ID: URL: https://gitlist.dev/e/CAN-3QhrO2FDDNfXLZJm-3DO5fw6m0Ea0QjW4Cu%2BCeo9aJJBPWg%40mail.gmail.com In-Reply-To: ``` Hey Matthieu, On 16 February 2017 at 23:52, Matthieu Moy 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. ``` ## Siddharth Kannan, 2017-02-16 19:43 Subject: Re: [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch" Message-ID: URL: https://gitlist.dev/e/CAN-3Qhok0WVZHBc-tFgTCNebGKRH87jPGe-rbRwTrsh1qLTfDQ%40mail.gmail.com In-Reply-To: ``` Hey Junio and Matthieu, On 17 February 2017 at 00:19, Junio C Hamano wrote: > Matthieu Moy writes: > >> Siddharth Kannan 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. ``` ## Siddharth Kannan, 2017-02-20 14:21 Subject: Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" Message-ID: URL: https://gitlist.dev/e/CAN-3QhoXBnLWyfuUsuvvRMYNnoupMrQHxE_G%3DysyA_14KX4Yrw%40mail.gmail.com In-Reply-To: ``` 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. ``` ## Junio C Hamano, 2017-02-20 20:30 Subject: Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" Message-ID: URL: https://gitlist.dev/e/xmqqshn8ip0j.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` 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. ``` ## Siddharth Kannan, 2017-02-22 06:27 Subject: Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" Message-ID: URL: https://gitlist.dev/e/CAN-3QhrAYJaVNf-4LKpT2uZQr1ubyxpd1Cpo-hVhGL8dh%2B_SXA%40mail.gmail.com In-Reply-To: ``` 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. ```