Re: [PATCH v2 GSoC RFC] diff: allow "-" as a short-hand for "last branch"

[Siddharth Kannan]

On Fri, Mar 10, 2017 at 04:52:07AM +, mash wrote:
> Maybe you can reuse the diff tests. I'll do another microproject then.

Yeah, definitely. If there are more tests required, then I will reuse
your ones!
> 
> mash
> 
> The original message doesn't seem to cc the mailing list:

Thanks! It was rather daft of me to not realise this. I was waiting
for it to appear on public-inbox.

I re-sent it with the CC. The timestamp is a little bit
skewed, but I think it should make sense.

Thanks,
Siddharth.

[PATCH v2 GSoC RFC] diff: allow "-" as a short-hand for "last branch"

[Siddharth Kannan]

Hey, I have already worked on this, and I made the change inside
sha1_name.c.

The final version of my patch is here[1].

> Handling the dash in sha1_name:get_sha1_basic is not an issue but
> git
> was designed with the dash in mind for options not for this weird
> short-hand so as long as there's no decision made that git should
> actually have this short-hand everywhere it does not seem like a
> good
> idea to change anything in there because it would probably have
> unwanted side-effects.

Actually, this was discussed even when I was working on this patch.

I said [2]

> Making a change in sha1_name.c will touch a lot of commands
> (setup_revisions is called from everywhere in the codebase), so, I
> am
> still trying to figure out how to do this such that the rest of the
> codepath remains unchanged.

Matthieu replied to this [3]

> I don't have strong opinion on this: I tend to favor consistency and
> supporting "-" everywhere goes in this direction, but I think the
> downsides should be considered too. A large part of the exercice
> here
> is to write a good commit message!

>From the discussion over the different versions of my patch, I get
the feeling that enabling this shorthand for all the commands is the
direction that git wants to move in.

Sorry about the time you spent on this patch.

[1]: 
http://public-inbox.org/git/1488007487-12965-1-git-send-email-kannan.siddhart...@gmail.com/
[2]: 
https://public-inbox.org/git/20170207191450.GA5569@ubuntu-512mb-blr1-01.localdomain/
[3]: https://public-inbox.org/git/vpqh944eof7@anie.imag.fr/

Thanks,
Siddharth.

P.S. This message was sent _before_ 1cmcxh-nd...@crossperf.com but
I didn't CC The mailing list in that message. I am sending it with the
mailing list cc-ed to ensure that the conversation makes sense.

Re: [PATCH GSoC] Allow "-" as a short-hand for "@{-1}" in branch deletions

[Siddharth Kannan]

Hey Shuyang,
On Thu, Mar 09, 2017 at 09:47:12AM -0800, Stefan Beller wrote:
> > The "-" shorthand that stands for "the branch we were previously on",
> > like we did for "git merge -" sometime after we introduced "git checkout -".
> > Now I am introducing this shorthand to branch delete, i.e.
> > "git branch -d -".
> >
> > More reference:
> >   https://public-inbox.org/git/7vppuewl6h@alter.siamese.dyndns.org/
> 

1. I have already worked on this project, and my patch is in the
"Needs review" section in "What's cooking". It implements this change
inside sha1_name.c and doesn't touch git branch. So, your patch is
mutually exclusive to my previous patch.

2. Matthieu made an argument against enabling commands like "git
branch -D -" even by mistake [1]. The way that I have implemented
ensured that not a lot of "rm"-like commands were enabled.

My patch that would enable this shorthand for other projects is
here[2].

[1]: http://public-inbox.org/git/vpqh944eof7@anie.imag.fr/
[2]: 
http://public-inbox.org/git/1488007487-12965-5-git-send-email-kannan.siddhart...@gmail.com/

Thanks,
Siddharth.

[PATCH 1/6 v5] 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.siddhart...@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(>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

[PATCH 1/6 v5] 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.siddhart...@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(>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

[PATCH 2/6 v5] 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.siddhart...@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(_data, argv + i);
break;
}
-   else
-   got_rev_arg = 1;
}
 
if (prune_data.nr) {
-- 
2.1.4

[PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions

[Siddharth Kannan]

revert.c:run_sequencer calls setup_revisions right after replacing "-" with
"@{-1}" for this shorthand. A previous patch taught setup_revisions to handle
this shorthand by doing the required replacement inside revision.c:get_sha1_1.

Hence, the code here is redundant and has been removed.

This patch also adds a test to check that revert recognizes the "-" shorthand.

Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com>
---
 builtin/revert.c|  2 --
 t/t3514-revert-shorthand.sh | 25 +
 2 files changed, 25 insertions(+), 2 deletions(-)
 create mode 100755 t/t3514-revert-shorthand.sh

diff --git a/builtin/revert.c b/builtin/revert.c
index 4ca5b51..0bc6657 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -155,8 +155,6 @@ static int run_sequencer(int argc, const char **argv, 
struct replay_opts *opts)
opts->revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
if (argc < 2)
usage_with_options(usage_str, options);
-   if (!strcmp(argv[1], "-"))
-   argv[1] = "@{-1}";
memset(_r_opt, 0, sizeof(s_r_opt));
s_r_opt.assume_dashdash = 1;
argc = setup_revisions(argc, argv, opts->revs, _r_opt);
diff --git a/t/t3514-revert-shorthand.sh b/t/t3514-revert-shorthand.sh
new file mode 100755
index 000..51f8c81d
--- /dev/null
+++ b/t/t3514-revert-shorthand.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+
+test_description='log can show previous branch using shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+   test_commit first
+'
+
+test_expect_success 'setup branches' '
+echo "hello" >hello &&
+cat hello >expect &&
+git add hello &&
+git commit -m "hello first commit" &&
+echo "world" >>hello &&
+git commit -am "hello second commit" &&
+git checkout -b testing-1 &&
+git checkout master &&
+git revert --no-edit - &&
+cat hello >actual &&
+test_cmp expect actual
+'
+
+test_done
-- 
2.1.4

[PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1

[Siddharth Kannan]

The callchain for handling each argument contains the function
revision.c:get_sha1 where the shorthand for "-" ~ "@{-1}" has already been
implemented in a previous patch; the complete callchain leading to that
function is:

1. merge.c:collect_parents
2. commit.c:get_merge_parent : this function calls revision.c:get_sha1

This patch also adds a test for checking that the shorthand works properly

Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com>
---
 builtin/merge.c   |  2 --
 t/t3035-merge-hyphen-shorthand.sh | 33 +
 2 files changed, 33 insertions(+), 2 deletions(-)
 create mode 100755 t/t3035-merge-hyphen-shorthand.sh

diff --git a/builtin/merge.c b/builtin/merge.c
index a96d4fb..36ff420 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1228,8 +1228,6 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
argc = setup_with_upstream();
else
die(_("No commit specified and merge.defaultToUpstream 
not set."));
-   } else if (argc == 1 && !strcmp(argv[0], "-")) {
-   argv[0] = "@{-1}";
}
 
if (!argc)
diff --git a/t/t3035-merge-hyphen-shorthand.sh 
b/t/t3035-merge-hyphen-shorthand.sh
new file mode 100755
index 000..fd37ff9
--- /dev/null
+++ b/t/t3035-merge-hyphen-shorthand.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='merge uses the 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 'setup branches' '
+git checkout master &&
+git checkout -b testing-2 &&
+git checkout -b testing-1 &&
+test_commit eigth &&
+test_commit ninth
+'
+
+test_expect_success 'merge - should work' '
+git checkout testing-2 &&
+git merge - &&
+git rev-parse HEAD HEAD^^ | sort >actual &&
+git rev-parse master testing-1 | sort >expect &&
+test_cmp expect actual
+'
+
+test_done
-- 
2.1.4

[PATCH 4/6 v5] 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)

Suffixes like "-@{yesterday}" and "-@{2.days.ago}" are not enabled by this
patch. This is something that needs to be fixed later by making changes deeper
down the callchain.

Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.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 000..8be2de1
--- /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 e

[PATCH 3/6 v5] 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.siddhart...@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 0/6 v5] allow "-" as a shorthand for "previous branch"

[Siddharth Kannan]

An updated version of the patch [1]. Discussion here[1] has been taken into
account. The test for "-@{yesterday}" is there inside the log-shorthand test,
it is commented out for now.

I have removed the redundant pieces of code in merge.c and revert.c as mentioned
by Matthieu in [2]. As analysed by Junio[3], the same type of code inside
checkout.c and worktree.c can not be removed because the appropriate functions
inside revision.c are not called in their codepaths.

Thanks for your review of the previous versions, Junio and Matthieu!

[1]: 1487258054-32292-1-git-send-email-kannan.siddhart...@gmail.com
[2]: vpqbmu768on@anie.imag.fr
[3]: xmqq1sv1euob@gitster.mtv.corp.google.com

Siddharth Kannan (6):
  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}"
  merge.c: delegate handling of "-" shorthand to revision.c:get_sha1
  revert.c: delegate handling of "-" shorthand to setup_revisions

 builtin/merge.c   |   2 -
 builtin/revert.c  |   2 -
 revision.c|  15 +++---
 sha1_name.c   |   5 ++
 t/t3035-merge-hyphen-shorthand.sh |  33 
 t/t3514-revert-shorthand.sh   |  25 +
 t/t4214-log-shorthand.sh  | 106 ++
 7 files changed, 178 insertions(+), 10 deletions(-)
 create mode 100755 t/t3035-merge-hyphen-shorthand.sh
 create mode 100755 t/t3514-revert-shorthand.sh
 create mode 100755 t/t4214-log-shorthand.sh

-- 
2.1.4

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 <gits...@pobox.com> wrote:
> Siddharth Kannan <kannan.siddhart...@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 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 <gits...@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.siddhart...@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@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 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 <gits...@pobox.com> wrote:
> Matthieu Moy <matthieu@grenoble-inp.fr> writes:
>
>> Siddharth Kannan <kannan.siddhart...@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-gits...@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.

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  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 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-gits...@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@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.siddhart...@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(>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

[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.siddhart...@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.siddhart...@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(_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]: <xmqqtw882n08@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 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 &&
+

[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.siddhart...@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(_data, argv + i);
break;
}
-   else
-   got_rev_arg = 1;
}
 
if (prune_data.nr) {
-- 
2.1.4

Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision

[Siddharth Kannan]

Hey Junio,
> 
> 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.

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.

Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision

[Siddharth Kannan]

On Sat, Feb 11, 2017 at 01:08:09PM -0800, Junio C Hamano wrote:
> Siddharth Kannan <kannan.siddhart...@gmail.com> writes:
> 
> > Um, I am sorry, but I feel that decrementing left, and incrementing it 
> > again is
> > also confusing.
> 
> Yes, but it is no more confusing than your original "left--".
> ...
> 
>  * 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.
> 
> 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.

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

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)

diff --git a/revision.c b/revision.c
index b37dbec..8c0acea 100644
--- a/revision.c
+++ b/revision.c
@@ -2203,11 +2203,11 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
if (seen_dashdash)
revarg_opt |= REVARG_CANNOT_BE_FILENAME;
read_from_stdin = 0;
+
for (left = i = 1; i < argc; i++) {
const char *arg = argv[i];
+   int opts;
if (*arg == '-') {
-   int opts;
-
opts = handle_revision_pseudo_opt(submodule,
revs, argc - i, argv + i,
);
@@ -2226,7 +2226,11 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
read_revisions_from_stdin(revs, _data);
continue;
}
+   }
 
+   if (!handle_revision_arg(arg, revs, flags, revarg_opt))
+   got_rev_arg = 1;
+   else if (*arg == '-') {
opts = handle_revision_opt(revs, argc - i, argv + i, 
, argv);
if (opts > 0) {
i += opts - 1;
@@ -2234,11 +2238,7 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
}
if (opts < 0)
exit(128);
-   continue;
-   }
-
-
-   if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+   } else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
@@ -2255,8 +2255,6 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
append_prune_data(_data, argv + i);
break;
}
-   else
-   got_rev_arg = 1;
}
 
if (prune_data.nr) {


The "if (*arg =='-')" line is repeated. On analysing the resulting
revision.c:setup_revisions function, I feel that the codepath is still as
easily followable as it was earlier, and there is definitely no confusion
because of a mysterious decrement. Also, the repeated condition doesn't make it
any har

Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"

[Siddharth Kannan]

Hey Matthieu,
On Sun, Feb 12, 2017 at 10:48:56AM +0100, Matthieu Moy wrote:
> Siddharth Kannan <kannan.siddhart...@gmail.com> writes:
> 
> >  sha1_name.c  |  5 
> >  t/t4214-log-shorthand.sh | 73 
> > 
> >  2 files changed, 78 insertions(+)
> >  create mode 100755 t/t4214-log-shorthand.sh
> >
> > diff --git a/sha1_name.c b/sha1_name.c
> > index 73a915f..d774e46 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 (!strcmp(name, "-")) {
> > +   name = "@{-1}";
> > +   len = 5;
> > +   }
> 
> After you do that, the existing "turn - into @{-1}" pieces of code
> become useless and you should remove it (probably in a further patch).

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)
> 
> > diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh
> > ...
> > +test_expect_success 'setup' '
> > +   echo hello >world &&
> > +   git add world &&
> > +   git commit -m initial &&
> > +   echo "hello second time" >>world &&
> > ...
> 
> You may use test_commit to save a few lines of code.

Oh, yeah! I will use that. I need to work on improving the tests, as
well as adding the documentation.
> 
> > +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 &&
> 
> Nitpick: we stick the > and the filename (as you did in most places
> already).
Sorry, slipped my mind!
> 
> It may be worth adding tests for more cases like
> 
> * Check what happens with suffixes, i.e. -^, -@{yesterday} and -~.

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!

> 
> * -..- -> 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".

Yeah, will add these tests.

> 
> The last two may be overkill, but the first one is probably important.
> 
> -- 
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

--
Regards,

Siddharth Kannan.

Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision

[Siddharth Kannan]

) {
> + got_rev_arg = 1;
> + } else if (maybe_rev) {
> + left++; /* it turns out that it was "unknown opt" */
> + continue;
> + } else {
>   int j;
>   if (seen_dashdash || *arg == '^')
>   die("bad revision '%s'", arg);
> @@ -2255,8 +2261,6 @@ int setup_revisions(int argc, const char **argv, struct 
> rev_info *revs, struct s
>   append_prune_data(_data, argv + i);
>   break;
>   }
> - else
> - got_rev_arg = 1;
>   }
>  
>   if (prune_data.nr) {

Thanks Junio, for the time you spent analysing and writing the above version of
the patch!

Regards,

- Siddharth Kannan

[PATCH 2/2 v3] sha1_name: 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.siddhart...@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

diff --git a/sha1_name.c b/sha1_name.c
index 73a915f..d774e46 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 (!strcmp(name, "-")) {
+   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..dec966c
--- /dev/null
+++ b/t/t4214-log-shorthand.sh
@@ -0,0 +1,73 @@
+#!/bin/sh
+
+test_description='log can show previous branch using shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+   echo hello >world &&
+   git add world &&
+   git commit -m initial &&
+   echo "hello second time" >>world &&
+   git add world &&
+   git commit -m second &&
+   echo "hello other file" >>planet &&
+   git add planet &&
+   git commit -m third &&
+   echo "hello yet another file" >>city &&
+   git add city &&
+   git commit -m fourth
+'
+
+test_expect_success '"log -" should not work initially' '
+   test_must_fail git log -
+'
+
+test_expect_success '"log -" should work' '
+   git checkout -b testing-1 master^ &&
+   git checkout -b testing-2 master~2 &&
+   git checkout master &&
+
+   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_done
-- 
2.1.4

[PATCH 1/2 v3] 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 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.siddhart...@gmail.com>
---
 revision.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index b37dbec..4131ad5 100644
--- a/revision.c
+++ b/revision.c
@@ -2205,6 +2205,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 handle_rev_arg_called = 0, args;
if (*arg == '-') {
int opts;
 
@@ -2234,11 +2235,18 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
}
if (opts < 0)
exit(128);
-   continue;
+
+   args = handle_revision_arg(arg, revs, flags, 
revarg_opt);
+   handle_rev_arg_called = 1;
+   if (args)
+   continue;
+   else
+   --left;
}
 
 
-   if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+   if ((handle_rev_arg_called && args) ||
+   handle_revision_arg(arg, revs, flags, 
revarg_opt)) {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
-- 
2.1.4

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

[Siddharth Kannan]

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@anie.imag.fr/#t
[2]: 
https://public-inbox.org/git/can-3qhozn_wyvqbvdu_c1h4vuoat5fobfl7k+femnpqkxjw...@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

-- 
2.1.4

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Siddharth Kannan]

On 9 February 2017 at 17:55, Matthieu Moy  wrote:
>
>> [...]
>> As you might notice, in this list, most commands are not of the `rm` variety,
>> i.e. something that would delete stuff.
>
> OK, I think I'm convinced.

I am glad! :)

>
> Keep the arguments in mind when polishing the commit message.

I will definitely do that. I am working on a good commit message for
this by looking at some past changes to sha1_name.c which have
affected multiple commands.

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

-- 

Best Regards,

- Siddharth.

Re: GSoC 2017: application open, deadline = February 9, 2017

[Siddharth Kannan]

On 9 February 2017 at 15:45, Matthieu Moy  wrote:
>
> A non-quoted but yet important part of my initial email was:
>
> | So, as much as possible, I'd like to avoid being an org admin this
> | year. It's not a lot of work (much, much less than being a mentor!),
> | but if I manage to get some time to work for Git, I'd rather do that
> | on coding and reviewing this year.
>
> and for now, no one stepped in to admin.

I would like to point everyone to this reply from Jeff King on the
original post: [1]
(In case this was lost in the midst of other emails) It sounds like
Jeff King is okay
with taking up the "admin" role.

I do not mind doing the administrative stuff.  But the real work is in
polishing up the ideas list and microprojects page. So I think the first
step, if people are interested in GSoC, is not just to say "yes, let's
do it", but to actually flesh out these pages:

>
>> Someone steps in to do what exactly?
>
> First we need an admin. Then as you said a bit of janitoring work on
> the web pages.


[1]: 
https://public-inbox.org/git/20170125204504.ebw2sa4uokfww...@sigill.intra.peff.net/

-- 

Best Regards,

- Siddharth.

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Siddharth Kannan]

Hello Matthieu,

On 8 February 2017 at 20:10, Matthieu Moy  wrote:
> In a previous discussion, I made an analogy with "cd -" (which is the
> source of inspiration of this shorthand AFAIK): "-" did not magically
> become "the last visited directory" for all Unix commands, just for
> "cd". And in this case, I'm happy with it. For example, I never need
> "mkdir -", and I'm happy I can't "rm -fr -" by mistake.
>
> So, it's debatable whether it's a good thing to have all commands
> support "-". For example, forcing users to explicitly type "git branch
> -d @{1}" and not providing them with a shortcut might be a good thing.

builtin/branch.c does not call setup_revisions and remains unaffected
by this patch :)

In my original patch post, I was not explicit about what files call
setup_revisions.
I would like to rectify that with this (grep-ed) list:

* 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 only show information, and don't change anything.

As you might notice, in this list, most commands are not of the `rm` variety,
i.e. something that would delete stuff.

In the next version of this patch, I will definitely include the list
of commands
which are "rm-ish" and affected by this patch.

>
> I don't have strong opinion on this: I tend to favor consistency and
> supporting "-" everywhere goes in this direction, but I think the
> downsides should be considered too. A large part of the exercice here is
> to write a good commit message!

Yes, I prefer consistency very much as well! Having "-" mean the same thing
across a lot of commands is better than having that shorthand only in a few
commands, as it is now. Unless there is a specific confusion that might arise
because of this shorthand inclusion, I think that this shorthand
should be supported
across the board.
(I especially like typing `git checkout - ` which is very handy!)

>
> Another issue with this is: - is also a common way to say "use stdin
> instead of a file", so before enabling - for "previous branch", we need
> to make sure it does not introduce any ambiguity. Git does not seem to
> use "- for stdin" much (most commands able to read from stdin have an
> explicit --stdin option for that), a quick grep in the docs shows only
> "git blame --contents -" which is OK because a revision wouldn't make
> sense here anyway.

Yes, just to jog your memory, this was discussed here [1]

Junio said:

As long as the addition is carefully prepared so that we know it
will not conflict (or be confused by users) with possible other uses
of "-", I do not think we would mind "git branch -D -" and other
commands to learn "-" as a synonym for @{-1}.

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

Thanks a lot for the review on this patch, Matthieu!

-- 

Best Regards,

- Siddharth.

[1]: https://public-inbox.org/git/7vmwpitb6k@alter.siamese.dyndns.org/

[PATCH/RFC v2] WIP: log: allow "-" as a short-hand for "previous branch"

[Siddharth Kannan]

Teach revision.c:setup_revisions that an argument starting with "-" can be an
argument also. `left` variable needs to be incremented only when the supplied
arg is neither an argument, nor an option.

Teach sha1_name.c:get_sha1_1 that "-" is equivalent to "@{-1}"

Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com>
---
I have run the test suite locally and on Travis CI. [1]

Whenever the argument begins with a "-" then the function "handle_revision_arg" 
is called twice. I can fix this using a variable that would store whether the
function has been called earlier or not. For doing that I have to investigate
some more on what the valid return values for "handle_revision_arg" are. (Or
a simpler approach would be to use an integer flag, this would also not be
affected if in case "handle_revision_arg" is changed in the future)

I have also written a very basic test for git-log only. I have based this on the
tests that were added in 696acf4 (checkout: implement "-" abbreviation, add docs
and tests, 2009-01-17). It tests revisions, revision ranges, and open-ended
revision ranges (where the start or the finish argument is inferred) If the
code in this patch is okay, or close to okay, then please tell me if further
tests need to be added for git-log and/or other commands.

This change touches a few commands, other than log. notably, git-format-patch,
git-whatchanged and git-show, all of which are defined inside builtin/log.c. In
general, it affects commands that call setup_revisions at some point in their
codepath. (eg: git-diff-index)

Thanks a lot, Junio, for your comments on the previous version of this patch and
further discussion on that thread!

[1]: https://travis-ci.org/icyflame/git/builds/199350136

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

diff --git a/revision.c b/revision.c
index b37dbec..e14f62c 100644
--- a/revision.c
+++ b/revision.c
@@ -2206,7 +2206,7 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
for (left = i = 1; i < argc; i++) {
const char *arg = argv[i];
if (*arg == '-') {
-   int opts;
+   int opts, args;
 
opts = handle_revision_pseudo_opt(submodule,
revs, argc - i, argv + i,
@@ -2234,7 +2234,12 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
}
if (opts < 0)
exit(128);
-   continue;
+
+   args = handle_revision_arg(arg, revs, flags, 
revarg_opt);
+   if (args)
+   continue;
+   else
+   --left;
}
 
 
diff --git a/sha1_name.c b/sha1_name.c
index 73a915f..d774e46 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 (!strcmp(name, "-")) {
+   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..95cf2d4
--- /dev/null
+++ b/t/t4214-log-shorthand.sh
@@ -0,0 +1,62 @@
+#!/bin/sh
+
+test_description='log can show previous branch using shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+   echo hello >world &&
+   git add world &&
+   git commit -m initial &&
+   echo "hello second time" >>world &&
+   git add world &&
+   git commit -m second &&
+   echo "hello other file" >>planet &&
+   git add planet &&
+   git commit -m third &&
+   echo "hello yet another file" >>city &&
+   git add city &&
+   git commit -m fourth
+'
+
+test_expect_success '"log -" should not work initially' '
+   test_must_fail git log -
+'
+
+test_expect_success '"log -" should work' '
+   git checkout -b testing-1 master^ &&
+   git checkout -b testing-2 master~2 &&
+   git checkout master &&
+
+   git log testing-2 >expect &&
+   git log - >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'revision range should work when one end is left empty' '
+   git checkout testi

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Siddharth Kannan]

On Mon, Feb 06, 2017 at 03:09:47PM -0800, Junio C Hamano wrote:
> The focus of GSoC being mentoring those who are new to the open
> source development, and hopefully retain them in the community after
> GSoC is over, we do expect microprojects to be suitable for those
> who are new to the codebase.

Okay, understood! Since I have spent time here anyway, I guess I will
continue on this instead of going over to a new micro project.

> 
> > (c) -> Else look for "r1^-"
> > ...
> > Case (c) is a bit confusing. This could be something like "-^-", and
> > something like "^-" could mean "Not commits on previous branch" or it
> > could mean "All commits on this branch except for the parent of HEAD"
> 
> Do you mean:
> 
> "git rev-parse ^-" does not mean "git rev-parse HEAD^-", but we
> probably would want to, and if that is what is going to happen,
> "^-" should mean "HEAD^-", and cannot be used for "^@{-1}"?
> 
> It's friend "^!" does not mean "HEAD^!", and "^@" does not mean
> "HEAD^@", either (the latter is somewhat borked, though, and "^@"
> translates to "^HEAD" because confusingly "@" stands for "HEAD"
> sometimes).  

Yes, I meant that whether we should use ^- as ^@{-1} or HEAD^-.

Oh! So, that's why running `git log ^@` leads to an empty set!
> 
> So my gut feeling is that it is probably OK to make "^-" mean
> "^@{-1}"; it may be prudent to at least initially keep "^-" an error
> like it currently is already, though.

I agree with your gut feeling, and would like to _not_ exclude only
this case. This way, across the code and implementation, there
wouldn't be any particular cases which would have to be excluded.

> > So, this patch reduces to the following 2 tasks:
> > 
> > 1. Teach setup_revisions that something starting with "-" can be
> > an
> > argument as well
> > 2. Teach get_sha1_basic that "-" means the tip of the previous
> > branch
> > perhaps by replacing it with "@{-1}" just before the reflog
> > parsing is
> > done

Making a change in sha1_name.c will touch a lot of commands
(setup_revisions is called from everywhere in the codebase), so, I am
still trying to figure out how to do this such that the rest of the
codepath remains unchanged.

I hope that you do not mind this side-effect, but rather, you intended
for this to happen, right? More commands will start supporting this
shorthand, suddenly.  (such as format-patch, whatchanged, diff to name
a very few).

Best Regards,

Siddharth.

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Siddharth Kannan]

Hey Junio, I did some more digging into the codepath:

On Sun, Feb 05, 2017 at 04:15:03PM -0800, Junio C Hamano wrote:
> 
> A correct solution needs to know if the argument is at the position
> where a revision (or revision range) is expected and then give the
> tip of the previous branch when it sees "-" (and other combinations
> this patch tries to cover).  In other words, the parser always knows
> what it is parsing, and if and only if it is parsing a rev, react to
> "-" and think "ah, the user wants me to use the tip of the previous
> branch".
> 
> But the code that knows that it expects to see a revision already
> exists, and it is the real parser.  In the above snippet,
> setup_revisions() is the one that does the real parsing of argv[].
> The code there knows when it wants to see a rev, and takes argv[i]
> and turns into an object to call add_pending_object().  That codepath
> may not yet know that "-" means the tip of the previous branch, and
> that is where the change needs to go.

Inside setup_revisions, it tries to parse arguments and options. In
there, is this line of code:

if (*arg == '-') {

Once control enters this branch, it will either parse the argument as
an option / pseudo-option, or simply leave this argument as is in the
argv[] array and move forward with the other arguments.

So, first I need to teach setup_revisions that something starting with
a "-" might be a revision or a revision range.

After this, going further down the codepath, in
revision.c:handle_revision_arg: 

The argument is parsed to find out if it is of the form
rev1...rev2 or rev1..rev2 or just rev1, etc.

(a) -> If `..` or `...` was found, then two pointers "this" and "next"
now hold the from and to revisions, and the function
get_sha1_committish is called on them. In case both were found to be
committish, then the char pointers now hold the sha1 in them, they are
parsed into objects.

(b) -> Else look for "r1^@", "r1^!" (This could be "-^@", "-^!") To
get r1, again the function get_sha1_committish is called with only r1
as the parameter.

(c) -> Else look for "r1^-"

(d) -> Else look for the argument using the same get_sha1_committish
function (It any "^" was present in it, it has already been noted and
removed)

Cases (a), (b) and (d) can be handled by putting this inside
get_sha1_committish. (Further discussion about that below)

Case (c) is a bit confusing. This could be something like "-^-", and
something like "^-" could mean "Not commits on previous branch" or it
could mean "All commits on this branch except for the parent of HEAD"
Please tell me if this is confusing or undesired altogether.
Personally, I feel that people who have been using "^-" would be
very confused if it's behaviour changed.

So, all the code till now points at adding the patch for "-" = "@{-1}"
inside get_sha1_committish or downstream from there.

get_sha1_committish 
-> get_sha1_with_context 
-> get_sha1_with_context_1
-> get_sha1_1 
  -> peel_onion -> calling get_sha1_basic again with the ref
  only (after peeling) 
  -> get_sha1_basic -> includes parsing of "@{-N}" type revs. So, 
  this indicates that if we can convert the "-" appropriately 
  before this point, then it would be good.
  -> get_short_sha1

So, this patch reduces to the following 2 tasks:

1. Teach setup_revisions that something starting with "-" can be an
argument as well
2. Teach get_sha1_basic that "-" means the tip of the previous branch
perhaps by replacing it with "@{-1}" just before the reflog parsing is
done

> A correct solution will be a lot more involved, of course, and I
> think it will be larger than a reasonable microproject for people
> new to the codebase.

So true :) I had spent a fair bit of time already on my previous patch,
and I thought I might as well complete my research into this, and send
this write-up to the mailing list, so that I could write a patch some
time later. In case you would prefer for me to not work on this
anymore because I am new to the codebase, I will leave it at this.

- Siddharth Kannan

Re: [PATCH v3] parse-remote: remove reference to unused op_prep

[Siddharth Kannan]

Hey Pranit,
On Sun, Feb 05, 2017 at 02:45:46AM +0530, Pranit Bauva wrote:
> Hey Siddharth,
> 
> On Sat, Feb 4, 2017 at 8:01 PM, Siddharth Kannan
> <kannan.siddhart...@gmail.com> wrote:
> > The error_on_missing_default_upstream helper function learned to
> > take op_prep argument with 15a147e618 ("rebase: use @{upstream}
> > if no upstream specified", 2011-02-09), but as of 045fac5845
> > ("i18n: git-parse-remote.sh: mark strings for translation",
> >  2016-04-19), the argument is no longer used.  Remove it.
> >
> > Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com>
> 
> This looks good to me! Thanks :)
> 
> Regards,
> Pranit Bauva

Should I send this patch with "To:" set to Junio and "Cc:" set to the
mailing list, as mentioend in the SubmittingPatches document?

- Siddharth Kannan

Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Siddharth Kannan]

Hey Junio,
On Sun, Feb 05, 2017 at 04:15:03PM -0800, Junio C Hamano wrote:
> Siddharth Kannan <kannan.siddhart...@gmail.com> writes:
> 
> > @@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char 
> > **argv, const char *prefix,
> >
> > if (quiet)
> > rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;
> > +
> > +   /*
> > +* Check if any argument has a "-" in it, which has been referred to as 
> > a
> > +* shorthand for @{-1}.  Handles methods that might be used to list 
> > commits
> > +* as mentioned in git rev-list --help
> > +*/
> > +
> > +   for(i = 0; i < argc; ++i) {
> > +   if (!strcmp(argv[i], "-")) {
> > +   argv[i] = "@{-1}";
> > +   } else if (!strcmp(argv[i], "^-")) {
> > +   argv[i] = "^@{-1}";
> > +   } else if (strlen(argv[i]) >= 4) {
> > +
> > +   ...
> > +   }
> > +   }
> > +
> > argc = setup_revisions(argc, argv, rev, opt);
> 
> "Turn '-' to '@{-1}' before we do the real parsing" can never be a
> reasonable strategy to implement the desired "'-' means the tip of
> the previous branch" correctly.  To understand why, you only need to
> imagine what happens to this command:
> 
> $ git log --grep '^-'
> 
> Turning it into "git log --grep '^@{-1}'" obviously is not what the
> end-users want, so that is an immediate bug in the version of Git
> with this patch applied.
> 
> Even if this were not a patch for the "log" command but for some
> other command, a change with the above approach is very much
> unwelcome, even if that other command does not currently have any
> option that takes arbitrary string the user may want to specify
> (like "find commit with a line that matches this pattern" option
> called "--grep" the "log" command has).  That is because it will
> make it impossible to enhance the command by adding such an option
> in the future.  So it is also adding the problems to future
> developers (and users) of Git.

Understood!
> 
> A correct solution needs to know if the argument is at the position
> where a revision (or revision range) is expected and then give the
> tip of the previous branch when it sees "-" (and other combinations
> this patch tries to cover).  In other words, the parser always knows
> what it is parsing, and if and only if it is parsing a rev, react to
> "-" and think "ah, the user wants me to use the tip of the previous
> branch".

Ah, okay. I will do another one of the suggestions as my micro project
but continue to look into this part of the code and try to find the
right place to write the code to implement the present patch.

> 
> But the code that knows that it expects to see a revision already
> exists, and it is the real parser.  In the above snippet,
> setup_revisions() is the one that does the real parsing of argv[].
> The code there knows when it wants to see a rev, and takes argv[i]
> and turns into an object to call add_pending_object().  That codepath
> may not yet know that "-" means the tip of the previous branch, and
> that is where the change needs to go.
> 
> Such a properly-done update does not need to textually replace "-"
> with "@{-1}" in argv[]; the codepath is where it understands what
> any textual representation of a rev the user gave it means, and it
> understands "@{-1}" there.  It would be the matter of updating it to
> also understand what "-" means.
> 
> A correct solution will be a lot more involved, of course, and I
> think it will be larger than a reasonable microproject for people
> new to the codebase.
> 
> I didn't check the microproject ideas page myself; whether it says
> that turning "-" unconditionally to "@{-1}" is a good idea, or it
> hints that supporting "-" as "the tip of the previous branch" in
> more commands is a reasonable byte-sized microproject, I think it is
> misleading and misguided.  Can somebody remove that entry so that we
> won't waste time of new developers (which would lead to discouraging
> them)?  Thanks.

Thanks a lot for writing this detailed reply! I will definitely take
into account all of the points mentioned here in the future patches I
send.

- Siddharth Kannan

[PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"

[Siddharth Kannan]

Search and replace "-" (written in the context of a branch name) in the argument
list with "@{-1}". As per the help text of git rev-list, this includes the 
following four
cases:

  a. "-"
  b. "^-"
  c. "-..other-branch-name" or "other-branch-name..-"
  d. "-...other-branch-name" or "other-branch-name...-"

(a) and (b) have been implemented as in the previous implementations of
this abbreviation. Namely, 696acf45 (checkout: implement "-" abbreviation, add
docs and tests, 2009-01-17), 182d7dc4 (cherry-pick: allow "-" as
abbreviation of '@{-1}', 2013-09-05) and 4e8115ff (merge: allow "-" as a
short-hand for "previous branch", 2011-04-07)

(c) and (d) have been implemented by using the strbuf API, growing it to the
right size and placing "@{-1}" instead of "-"

Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com>
---
This is a patch for one of the microprojects of SoC 2017. [1]

I have implemented this using multiple methods, that I have re-written again and
again for better versions ([2]). The present version I feel is the closest that
I could get to the existing code in the repository. This patch only uses
functions that are commonly used in the rest of the codebase.

I still have to write tests, as well as update documentation as done in 696acf45
(checkout: implement "-" abbreviation, add docs and tests, 2009-01-17).

I request your comments on this patch. Also, I have the following questions
regarding this patch:

1. Is the approach that I have used to solve this problem fine?
2. Is the code I am writing in the right function? (I have put it right
before the revisions data structure is setup, thus these changes affect only
git-log)

[1]: https://git.github.io/SoC-2017-Microprojects/
[2]: https://github.com/git/git/compare/6e3a7b3...icyflame:7e286c9.patch (Uses
strbufs for the starting 4 characters, and last 4 characters and compares those
to the appropriate strings for case (c) and case (d). I edited this patch to use
strstr instead, which avoids all the strbuf declarations)

 builtin/log.c | 47 ++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 55d20cc..a5aac99 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -132,7 +132,7 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
 struct rev_info *rev, struct setup_revision_opt *opt)
 {
struct userformat_want w;
-   int quiet = 0, source = 0, mailmap = 0;
+   int quiet = 0, source = 0, mailmap = 0, i = 0;
static struct line_opt_callback_data line_cb = {NULL, NULL, 
STRING_LIST_INIT_DUP};

const struct option builtin_log_options[] = {
@@ -158,6 +158,51 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,

if (quiet)
rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;
+
+   /*
+* Check if any argument has a "-" in it, which has been referred to as 
a
+* shorthand for @{-1}.  Handles methods that might be used to list 
commits
+* as mentioned in git rev-list --help
+*/
+
+   for(i = 0; i < argc; ++i) {
+   if (!strcmp(argv[i], "-")) {
+   argv[i] = "@{-1}";
+   } else if (!strcmp(argv[i], "^-")) {
+   argv[i] = "^@{-1}";
+   } else if (strlen(argv[i]) >= 4) {
+
+   if (strstr(argv[i], "-...") == argv[i] || 
strstr(argv[i], "-..") == argv[i]) {
+   struct strbuf changed_argument = STRBUF_INIT;
+
+   strbuf_addstr(_argument, "@{-1}");
+   strbuf_addstr(_argument, argv[i] + 1);
+
+   strbuf_setlen(_argument, 
strlen(argv[i]) + 4);
+
+   argv[i] = strbuf_detach(_argument, 
NULL);
+   }
+
+   /*
+* Find the first occurence, and add the size to it and 
proceed if
+* the resulting value is NULL
+*/
+   if (!(strstr(argv[i], "...-") + 4)  ||
+   !(strstr(argv[i], "..-") + 3)) {
+   struct strbuf changed_argument = STRBUF_INIT;
+
+   strbuf_addstr(_argument, argv[i]);
+
+   strbuf_grow(_argument, strlen(argv[i]) 
+ 4);
+   strbuf_setlen(_argument, 
strlen(argv[i]) + 4);
+
+   strbuf_splice(_argument, 
strlen(argv[i]) - 1, 5, "@{-1}", 5);
+
+   argv[i] = strbuf_detach(_argument, 
NULL);
+   }
+   }
+   }
+
argc = setup_revisions(argc, argv, rev, opt);

/* Any arguments at this point are not recognized */
--
2.1.4

[PATCH v3] parse-remote: remove reference to unused op_prep

[Siddharth Kannan]

The error_on_missing_default_upstream helper function learned to
take op_prep argument with 15a147e618 ("rebase: use @{upstream}
if no upstream specified", 2011-02-09), but as of 045fac5845
("i18n: git-parse-remote.sh: mark strings for translation",
 2016-04-19), the argument is no longer used.  Remove it.

Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com>
---
Thanks a lot for the review, Pranit and Junio! I have made the appropriate
changes, and the edit to the file inside contrib/examples/ has been removed from
this patch.

 git-parse-remote.sh | 3 +--
 git-rebase.sh   | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index d3c3998..9698a05 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -56,8 +56,7 @@ get_remote_merge_branch () {
 error_on_missing_default_upstream () {
cmd="$1"
op_type="$2"
-   op_prep="$3" # FIXME: op_prep is no longer used
-   example="$4"
+   example="$3"
branch_name=$(git symbolic-ref -q HEAD)
display_branch_name="${branch_name#refs/heads/}"
# If there's only one remote, use that in the suggestion
diff --git a/git-rebase.sh b/git-rebase.sh
index 04f6e44..b89f960 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -448,7 +448,7 @@ then
then
. git-parse-remote
error_on_missing_default_upstream "rebase" "rebase" \
-   "against" "git rebase $(gettext '')"
+   "git rebase $(gettext '')"
fi
 
test "$fork_point" = auto && fork_point=t
-- 
2.1.4

[PATCH v2] parse-remote: Remove reference to unused op_prep

[Siddharth Kannan]

The error_on_missing_default_upstream helper function learned to
take op_prep argument with 15a147e618 ("rebase: use @{upstream}
if no upstream specified", 2011-02-09), but as of 045fac5845
("i18n: git-parse-remote.sh: mark strings for translation",
 2016-04-19), the argument is no longer used.  Remove it.

Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com>
---
Thanks a lot, Pranit and Junio for your reviews on the first version of this
patch. I have changed the messages accordingly.

 contrib/examples/git-pull.sh | 2 +-
 git-parse-remote.sh  | 3 +--
 git-rebase.sh| 2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/contrib/examples/git-pull.sh b/contrib/examples/git-pull.sh
index 6b3a03f..1d51dc3 100755
--- a/contrib/examples/git-pull.sh
+++ b/contrib/examples/git-pull.sh
@@ -267,7 +267,7 @@ error_on_no_merge_candidates () {
echo "for your current branch, you must specify a branch on the 
command line."
elif [ -z "$curr_branch" -o -z "$upstream" ]; then
. git-parse-remote
-   error_on_missing_default_upstream "pull" $op_type $op_prep \
+   error_on_missing_default_upstream "pull" $op_type \
"git pull  "
else
echo "Your configuration specifies to $op_type $op_prep the ref 
'${upstream#refs/heads/}'"
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index d3c3998..9698a05 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -56,8 +56,7 @@ get_remote_merge_branch () {
 error_on_missing_default_upstream () {
cmd="$1"
op_type="$2"
-   op_prep="$3" # FIXME: op_prep is no longer used
-   example="$4"
+   example="$3"
branch_name=$(git symbolic-ref -q HEAD)
display_branch_name="${branch_name#refs/heads/}"
# If there's only one remote, use that in the suggestion
diff --git a/git-rebase.sh b/git-rebase.sh
index 04f6e44..b89f960 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -448,7 +448,7 @@ then
then
. git-parse-remote
error_on_missing_default_upstream "rebase" "rebase" \
-   "against" "git rebase $(gettext '')"
+   "git rebase $(gettext '')"
fi
 
test "$fork_point" = auto && fork_point=t
-- 
2.1.4

[PATCH] git-parse-remote.sh: Remove op_prep argument

[Siddharth Kannan]

- Remove the third argument of error_on_missing_default_upstream that is no
  longer required
- FIXME to remove this argument was added in commit 045fac5845
- Run "grep" on the rest of the codebase to find and remove occurences of
  the third argument and fix the function calls appropriately

Signed-off-by: Siddharth Kannan <kannan.siddhart...@gmail.com>
---
The contrib/examples/git-pull.sh file also has a variable op_prep which is used
in one of the messages shown the user. Should I remove this variable as well?

 contrib/examples/git-pull.sh | 2 +-
 git-parse-remote.sh  | 3 +--
 git-rebase.sh| 2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/contrib/examples/git-pull.sh b/contrib/examples/git-pull.sh
index 6b3a03f..1d51dc3 100755
--- a/contrib/examples/git-pull.sh
+++ b/contrib/examples/git-pull.sh
@@ -267,7 +267,7 @@ error_on_no_merge_candidates () {
echo "for your current branch, you must specify a branch on the 
command line."
elif [ -z "$curr_branch" -o -z "$upstream" ]; then
. git-parse-remote
-   error_on_missing_default_upstream "pull" $op_type $op_prep \
+   error_on_missing_default_upstream "pull" $op_type \
"git pull  "
else
echo "Your configuration specifies to $op_type $op_prep the ref 
'${upstream#refs/heads/}'"
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index d3c3998..9698a05 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -56,8 +56,7 @@ get_remote_merge_branch () {
 error_on_missing_default_upstream () {
cmd="$1"
op_type="$2"
-   op_prep="$3" # FIXME: op_prep is no longer used
-   example="$4"
+   example="$3"
branch_name=$(git symbolic-ref -q HEAD)
display_branch_name="${branch_name#refs/heads/}"
# If there's only one remote, use that in the suggestion
diff --git a/git-rebase.sh b/git-rebase.sh
index 04f6e44..b89f960 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -448,7 +448,7 @@ then
then
. git-parse-remote
error_on_missing_default_upstream "rebase" "rebase" \
-   "against" "git rebase $(gettext '')"
+   "git rebase $(gettext '')"
fi
 
test "$fork_point" = auto && fork_point=t
-- 
2.1.4