[PATCH 0/6 v5] allow "-" as a shorthand for "previous branch"

[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.siddharth12@gmail.com
[2]: vpqbmu768on.fsf@anie.imag.fr
[3]: xmqq1sv1euob.fsf@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

[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.siddharth12@gmail.com>
---
 revision.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

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

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

[PATCH 3/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.siddharth12@gmail.com>
---
 revision.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

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

[PATCH 4/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.siddharth12@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 0000000..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 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

[PATCH 6/6 v5] sha1_name.c: avoid parsing @{-1} unnecessarily

[mash]

Move dash is previous branch check to get_sha1_basic.
Introduce helper function that gets nth prior branch switch from reflog.

Signed-off-by: mash <mash+git@crossperf.com>
---
RE: [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
> +	if (*name == '-' && len == 1) {
> +		name = "@{-1}";
> +		len = 5;
> +	}

We could avoid parsing @{-1} unnecessarily with something like this patch.

Forgive me I don't understand how the patch numbering works just yet. This is
6/6 because format-patch made it 6/6 with however I got the patches applied on
my end. This should apply cleanly on pu anyways.

Thanks to Stefan since he suggested that I might want to review this.

mash

 sha1_name.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 2f86bc9..363bbe7 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -568,6 +568,7 @@ static inline int push_mark(const char *string, int len)
 }
 
 static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);
+static int get_branch_switch(int nth, struct strbuf *buf);
 static int interpret_nth_prior_checkout(const char *name, int namelen, struct strbuf *buf);
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1,
@@ -628,11 +629,12 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,
 	if (len && ambiguous_path(str, len))
 		return -1;
 
-	if (nth_prior) {
+	if (nth_prior || !strcmp(str, "-")) {
 		struct strbuf buf = STRBUF_INIT;
 		int detached;
 
-		if (interpret_nth_prior_checkout(str, len, &buf) > 0) {
+		if (nth_prior ? interpret_nth_prior_checkout(str, len, &buf) > 0
+			      : get_branch_switch(1, &buf) > 0) {
 			detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));
 			strbuf_release(&buf);
 			if (detached)
@@ -1078,6 +1080,25 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
 	return 0;
 }
 
+static int get_branch_switch(int nth, struct strbuf *buf)
+{
+	int retval;
+	struct grab_nth_branch_switch_cbdata cb;
+
+	cb.remaining = nth;
+	strbuf_init(&cb.buf, 20);
+
+	retval = for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch,
+					     &cb);
+	if (0 < retval) {
+		strbuf_reset(buf);
+		strbuf_addbuf(buf, &cb.buf);
+	}
+
+	strbuf_release(&cb.buf);
+	return retval;
+}
+
 /*
  * Parse @{-N} syntax, return the number of characters parsed
  * if successful; otherwise signal an error with negative value.
@@ -1086,8 +1107,6 @@ static int interpret_nth_prior_checkout(const char *name, int namelen,
 					struct strbuf *buf)
 {
 	long nth;
-	int retval;
-	struct grab_nth_branch_switch_cbdata cb;
 	const char *brace;
 	char *num_end;
 
@@ -1103,18 +1122,8 @@ static int interpret_nth_prior_checkout(const char *name, int namelen,
 		return -1;
 	if (nth <= 0)
 		return -1;
-	cb.remaining = nth;
-	strbuf_init(&cb.buf, 20);
 
-	retval = 0;
-	if (0 < for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch, &cb)) {
-		strbuf_reset(buf);
-		strbuf_addbuf(buf, &cb.buf);
-		retval = brace - name + 1;
-	}
-
-	strbuf_release(&cb.buf);
-	return retval;
+	return 0 < get_branch_switch(nth, buf) ? brace - name + 1 : 0;
 }
 
 int get_oid_mb(const char *name, struct object_id *oid)
-- 
2.9.3

[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.siddharth12@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(&argv);
 		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 0000000..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

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

[Junio C Hamano]

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

> 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

This breaks "git merge".

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

This test is not sufficient to catch a regression I seem to be
seeing.

	$ git checkout side
	$ git checkout pu
	$ git merge -

used to say "Merge branch 'side' into pu".  With this series merged,
I seem to be getting "Merge commit '-' into pu".

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

[Junio C Hamano]

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

> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>
>> 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
>
> This breaks "git merge".
>
>> +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
>
> This test is not sufficient to catch a regression I seem to be
> seeing.
>
> 	$ git checkout side
> 	$ git checkout pu
> 	$ git merge -
>
> used to say "Merge branch 'side' into pu".  With this series merged,
> I seem to be getting "Merge commit '-' into pu".

You stopped at get_sha1_1() in your 3817cebabc ("sha1_name.c: teach
get_sha1_1 "-" shorthand for "@{-1}"", 2017-02-25), instead of going
down to get_sha1_basic() and teaching it that "-" means the same
thing as "@{-1}", which would in turn require you to teach
dwim_ref() that "-" is the same thing as "@{-1}".  As dwim_ref()
does not know about "-" and does not expand it to the refname like
it expands "@{-1}", it would break and that is why 3817cebabc punts
at a bit higher in the callchain.

The breakage by this patch to "git merge" happens for the same
reason.  cmd_merge() calls collect_parents() which annotates the
commits that are merged with their textual name, which used to be
"@{-1}" without this patch but now "-" is passed as-is.  This
annotation will be given to merge_name(), and the first thing it
does is dwim_ref().  The function knows what to do with "@{-1}",
but it does not know what to do with "-", and that is why you end up
producing "Merge commit '-' into ...".

Dropping this patch from the series would make things consistent
with what was done in 3817cebabc and I think that is a sensible
place to stop.  After the dust settles, We _can_ later dig further
by teaching dwim_ref() and friends what "-" means, and when it is
done, this patch would become useful.

Thanks.

[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.siddharth12@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(&s_r_opt, 0, sizeof(s_r_opt));
 		s_r_opt.assume_dashdash = 1;
 		argc = setup_revisions(argc, argv, opts->revs, &s_r_opt);
diff --git a/t/t3514-revert-shorthand.sh b/t/t3514-revert-shorthand.sh
new file mode 100755
index 0000000..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

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

[Junio C Hamano]

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

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

Unlike "merge" [*1*], I think this one is OK because "git revert
$commit" does not try to say _how_ the commit was given, and most
importantly, it does not say what branch the reverted thing was.

Thanks.

[Footnote]

*1* Probably "checkout" would exhibit the same issue as we saw in
    5/6 for "git merge" if you remove the "- to @{-1}" conversion
    from it.

[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.siddharth12@gmail.com>
---
 revision.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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