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

STALE3467d

9 messages, 2 authors, 2017-03-14 · open the first message on its own page

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

From: Siddharth Kannan <hidden>
Date: 2017-02-25 07:25:02

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 3/6 v5] revision.c: args starting with "-" might be a revision

From: Siddharth Kannan <hidden>
Date: 2017-02-25 07:25:23

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 <redacted>
---
 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 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions

From: Siddharth Kannan <hidden>
Date: 2017-02-25 07:25:26

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

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

From: Siddharth Kannan <hidden>
Date: 2017-02-25 07:25:30

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

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

From: Siddharth Kannan <hidden>
Date: 2017-02-25 07:25:32

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 <redacted>
---
 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 2/6 v5] revision.c: swap if/else blocks

From: Siddharth Kannan <hidden>
Date: 2017-02-25 07:31:14

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 <redacted>
---
 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 1/6 v5] revision.c: do not update argv with unknown option

From: Siddharth Kannan <hidden>
Date: 2017-02-25 07:32:23

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 <redacted>
---
 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 1/6 v5] revision.c: do not update argv with unknown option

From: Siddharth Kannan <hidden>
Date: 2017-02-25 07:32:24

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 <redacted>
---
 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 6/6 v5] sha1_name.c: avoid parsing @{-1} unnecessarily

From: mash <hidden>
Date: 2017-03-14 02:10:46

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help