allow "-" as a shorthand for "previous branch"

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]: [email protected] [2]: [email protected]
[3]: [email protected]

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

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

---
 revision.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index b37dbec..5674a9a 100644
@@ -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

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

---
 revision.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index b37dbec..5674a9a 100644
@@ -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

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.

---
 revision.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 5674a9a..8d4ddae 100644
@@ -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

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

---
 revision.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

index 8d4ddae..5470c33 100644
@@ -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

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.

---
 sha1_name.c              |   5 +++
 t/t4214-log-shorthand.sh | 106 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+)
 create mode 100755 t/t4214-log-shorthand.sh

index 73a915f..2f86bc9 100644
@@ -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;

2.1.4

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

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

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

index 2f86bc9..363bbe7 100644
@@ -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

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

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

index a96d4fb..36ff420 100644
@@ -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)

new file mode 100755
index 0000000..fd37ff9
@@ -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

This breaks "git merge".

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

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.

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.

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

index 4ca5b51..0bc6657 100644
@@ -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);

new file mode 100755
index 0000000..51f8c81d
@@ -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

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.