git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch"
@ 2017-02-25  7:24 Siddharth Kannan
  2017-02-25  7:24 ` [PATCH 1/6 v5] revision.c: do not update argv with unknown option Siddharth Kannan
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25  7:24 UTC (permalink / raw)
  To: git
  Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
	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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/6 v5] revision.c: do not update argv with unknown option
  2017-02-25  7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
@ 2017-02-25  7:24 ` Siddharth Kannan
  2017-02-25  7:24 ` [PATCH 2/6 v5] revision.c: swap if/else blocks Siddharth Kannan
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25  7:24 UTC (permalink / raw)
  To: git
  Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
	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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/6 v5] revision.c: swap if/else blocks
  2017-02-25  7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
  2017-02-25  7:24 ` [PATCH 1/6 v5] revision.c: do not update argv with unknown option Siddharth Kannan
@ 2017-02-25  7:24 ` Siddharth Kannan
  2017-02-25  7:24 ` [PATCH 3/6 v5] revision.c: args starting with "-" might be a revision Siddharth Kannan
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25  7:24 UTC (permalink / raw)
  To: git
  Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
	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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/6 v5] revision.c: args starting with "-" might be a revision
  2017-02-25  7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
  2017-02-25  7:24 ` [PATCH 1/6 v5] revision.c: do not update argv with unknown option Siddharth Kannan
  2017-02-25  7:24 ` [PATCH 2/6 v5] revision.c: swap if/else blocks Siddharth Kannan
@ 2017-02-25  7:24 ` Siddharth Kannan
  2017-02-25  7:24 ` [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" Siddharth Kannan
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25  7:24 UTC (permalink / raw)
  To: git
  Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
	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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
  2017-02-25  7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
                   ` (2 preceding siblings ...)
  2017-02-25  7:24 ` [PATCH 3/6 v5] revision.c: args starting with "-" might be a revision Siddharth Kannan
@ 2017-02-25  7:24 ` Siddharth Kannan
  2017-03-14  2:10   ` [PATCH 6/6 v5] sha1_name.c: avoid parsing @{-1} unnecessarily mash
  2017-02-25  7:24 ` [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1 Siddharth Kannan
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25  7:24 UTC (permalink / raw)
  To: git
  Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
	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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1
  2017-02-25  7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
                   ` (3 preceding siblings ...)
  2017-02-25  7:24 ` [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" Siddharth Kannan
@ 2017-02-25  7:24 ` Siddharth Kannan
  2017-03-01 22:49   ` Junio C Hamano
  2017-02-25  7:24 ` [PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions Siddharth Kannan
  2017-02-25  7:32 ` [PATCH 1/6 v5] revision.c: do not update argv with unknown option Siddharth Kannan
  6 siblings, 1 reply; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25  7:24 UTC (permalink / raw)
  To: git
  Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
	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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions
  2017-02-25  7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
                   ` (4 preceding siblings ...)
  2017-02-25  7:24 ` [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1 Siddharth Kannan
@ 2017-02-25  7:24 ` Siddharth Kannan
  2017-03-01 23:18   ` Junio C Hamano
  2017-02-25  7:32 ` [PATCH 1/6 v5] revision.c: do not update argv with unknown option Siddharth Kannan
  6 siblings, 1 reply; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25  7:24 UTC (permalink / raw)
  To: git
  Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
	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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 1/6 v5] revision.c: do not update argv with unknown option
  2017-02-25  7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
                   ` (5 preceding siblings ...)
  2017-02-25  7:24 ` [PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions Siddharth Kannan
@ 2017-02-25  7:32 ` Siddharth Kannan
  6 siblings, 0 replies; 12+ messages in thread
From: Siddharth Kannan @ 2017-02-25  7:32 UTC (permalink / raw)
  To: git
  Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
	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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1
  2017-02-25  7:24 ` [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1 Siddharth Kannan
@ 2017-03-01 22:49   ` Junio C Hamano
  2017-03-01 23:05     ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2017-03-01 22:49 UTC (permalink / raw)
  To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1
  2017-03-01 22:49   ` Junio C Hamano
@ 2017-03-01 23:05     ` Junio C Hamano
  0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2017-03-01 23:05 UTC (permalink / raw)
  To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals

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.






^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions
  2017-02-25  7:24 ` [PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions Siddharth Kannan
@ 2017-03-01 23:18   ` Junio C Hamano
  0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2017-03-01 23:18 UTC (permalink / raw)
  To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals

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.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 6/6 v5] sha1_name.c: avoid parsing @{-1} unnecessarily
  2017-02-25  7:24 ` [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" Siddharth Kannan
@ 2017-03-14  2:10   ` mash
  0 siblings, 0 replies; 12+ messages in thread
From: mash @ 2017-03-14  2:10 UTC (permalink / raw)
  To: Siddharth Kannan
  Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
	Siddharth Kannan, Git Mailing List, Stefan Beller

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-03-14  2:10 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-25  7:24 [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch" Siddharth Kannan
2017-02-25  7:24 ` [PATCH 1/6 v5] revision.c: do not update argv with unknown option Siddharth Kannan
2017-02-25  7:24 ` [PATCH 2/6 v5] revision.c: swap if/else blocks Siddharth Kannan
2017-02-25  7:24 ` [PATCH 3/6 v5] revision.c: args starting with "-" might be a revision Siddharth Kannan
2017-02-25  7:24 ` [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}" Siddharth Kannan
2017-03-14  2:10   ` [PATCH 6/6 v5] sha1_name.c: avoid parsing @{-1} unnecessarily mash
2017-02-25  7:24 ` [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1 Siddharth Kannan
2017-03-01 22:49   ` Junio C Hamano
2017-03-01 23:05     ` Junio C Hamano
2017-02-25  7:24 ` [PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions Siddharth Kannan
2017-03-01 23:18   ` Junio C Hamano
2017-02-25  7:32 ` [PATCH 1/6 v5] revision.c: do not update argv with unknown option Siddharth Kannan

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).