[PATCH/RFC 0/4] Adding '-' notation as @{-1} (pu, d40f108)

[PATCH/RFC 0/4] Adding '-' notation as @{-1} (pu, d40f108)

[Kenny Lee Sin Cheong]

This is an attempt to allow '-' everywhere a revision is normally allowed.
I previously attempted this  as a microproject and the subject was disscussed at : http://article.gmane.org/gmane.comp.version-control.git/265672

Currently, something like '-~2' does not work. I tried tracing the execution of, say 'log -~2' vs 'log master -~2' and noticed when calling dwim_ref() with '-~2', it returns 0 (no refs found) whereas when given 'master~2', it returned non-zero. However I'm not sure how exactly dwim_ref() works.

Kenny Lee Sin Cheong (4):
  Add "-" as @{-1} support for the rev-parse command
  t1505: add tests for '-' notation in rev-parse
  Handle arg as revision first, then option.
  t0102: add tests for '-' notation

 builtin/rev-parse.c           | 37 +++++++++++++-------------
 revision.c                    | 61 +++++++++++++++++++++++--------------------
 sha1_name.c                   |  2 +-
 t/t0102-previous-shorthand.sh | 40 ++++++++++++++++++++++++++++
 t/t1505-rev-parse-last.sh     | 12 ++++++---
 5 files changed, 101 insertions(+), 51 deletions(-)
 create mode 100644 t/t0102-previous-shorthand.sh

-- 
2.3.3.203.g8ffb468.dirty

[PATCH/RFC 1/4] Add "-" as @{-1} support for the rev-parse command

[Kenny Lee Sin Cheong]

Allows the use of the "-" shorthand notation, including
use with revision ranges. If we plan to allow "-" as a stand in every
where a revision is allowed, then "-" would also need to be usable in
plumbing commands, for writing tests, for example.

Checks if the argument can be interpreted as a revision range first
before checking for flags. This saves us from having to check that
something that begins with "-" does not get checked as a possible flag.

Signed-off-by: Kenny Lee Sin Cheong <kenny.lee28@gmail.com>
---
 builtin/rev-parse.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 3626c61..8da95b5 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -553,6 +553,25 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
+		/* Not a flag argument */
+		if (try_difference(arg))
+			continue;
+		if (try_parent_shorthands(arg))
+			continue;
+		name = arg;
+		type = NORMAL;
+		if (*arg == '^') {
+			name++;
+			type = REVERSED;
+		}
+		if (!get_sha1_with_context(name, flags, sha1, &unused)) {
+			if (verify)
+				revs_count++;
+			else
+				show_rev(type, sha1, name);
+			continue;
+		}
+
 		if (*arg == '-') {
 			if (!strcmp(arg, "--")) {
 				as_is = 2;
@@ -810,24 +829,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		/* Not a flag argument */
-		if (try_difference(arg))
-			continue;
-		if (try_parent_shorthands(arg))
-			continue;
-		name = arg;
-		type = NORMAL;
-		if (*arg == '^') {
-			name++;
-			type = REVERSED;
-		}
-		if (!get_sha1_with_context(name, flags, sha1, &unused)) {
-			if (verify)
-				revs_count++;
-			else
-				show_rev(type, sha1, name);
-			continue;
-		}
 		if (verify)
 			die_no_single_rev(quiet);
 		if (has_dashdash)
-- 
2.3.3.203.g8ffb468.dirty

[PATCH/RFC 2/4] t1505: add tests for '-' notation in rev-parse

[Kenny Lee Sin Cheong]

Signed-off-by: Kenny Lee Sin Cheong <kenny.lee28@gmail.com>
---
 t/t1505-rev-parse-last.sh | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/t1505-rev-parse-last.sh b/t/t1505-rev-parse-last.sh
index 4969edb..a1976ad 100755
--- a/t/t1505-rev-parse-last.sh
+++ b/t/t1505-rev-parse-last.sh
@@ -33,19 +33,23 @@ test_expect_success 'setup' '
 # and 'side' should be the last branch
 
 test_expect_success '@{-1} works' '
-	test_cmp_rev side @{-1}
+	test_cmp_rev side @{-1} &&
+	test_cmp_rev side -
 '
 
 test_expect_success '@{-1}~2 works' '
-	test_cmp_rev side~2 @{-1}~2
+	test_cmp_rev side~2 @{-1}~2 &&
+	test_cmp_rev side~2 -~2
 '
 
 test_expect_success '@{-1}^2 works' '
-	test_cmp_rev side^2 @{-1}^2
+	test_cmp_rev side^2 @{-1}^2 &&
+	test_cmp_rev side^2 -^2
 '
 
 test_expect_success '@{-1}@{1} works' '
-	test_cmp_rev side@{1} @{-1}@{1}
+	test_cmp_rev side@{1} @{-1}@{1} &&
+	test_cmp_rev side@{1} -@{1}
 '
 
 test_expect_success '@{-2} works' '
-- 
2.3.3.203.g8ffb468.dirty

[PATCH/RFC 3/4] Handle arg as revision first, then option.

[Kenny Lee Sin Cheong]

Check the argument as a revision at first. If it fails, then tries to
check it as an option, and finally as a pathspec.

Returns -1 when we have an ambiguous revision range, such as
"master..next", to allow the argument to get checked as an option before
calling die() from verify_non_filename(). This is because we are
allowing "-" to be given in a revision range, but making the revision
check first. Otherwise, an ambiguous argument that starts with
"-" (let's say an option) would die even though its normal behaviour is
to silently return. Instead we check for ambiguity in a revision after
making sure that the argument cannot be parsed as an option.

This problem is discussed in:
http://article.gmane.org/gmane.comp.version-control.git/265672

Signed-off-by: Kenny Lee Sin Cheong <kenny.lee28@gmail.com>
---
 revision.c  | 61 +++++++++++++++++++++++++++++++++----------------------------
 sha1_name.c |  2 +-
 2 files changed, 34 insertions(+), 29 deletions(-)

diff --git a/revision.c b/revision.c
index 570945a..1ea290f 100644
--- a/revision.c
+++ b/revision.c
@@ -1516,7 +1516,10 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
 
 			if (!cant_be_filename) {
 				*dotdot = '.';
-				verify_non_filename(revs->prefix, arg);
+				if (is_inside_work_tree() && !is_inside_git_dir() &&
+				    check_filename(revs->prefix, arg)) {
+					return -1;
+				}
 			}
 
 			a_obj = parse_object(from_sha1);
@@ -2198,40 +2201,39 @@ 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];
-		if (arg[0] == '-' && arg[1] && !starts_with(arg + 1, "..")) {
-			int opts;
-
-			opts = handle_revision_pseudo_opt(submodule,
-						revs, argc - i, argv + i,
-						&flags);
-			if (opts > 0) {
-				i += opts - 1;
-				continue;
-			}
+		if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+			if (arg[0] == '-' && arg[1] && !starts_with(arg + 1, "..")) {
+				int opts;
+
+				opts = handle_revision_pseudo_opt(submodule,
+								  revs, argc - i, argv + i,
+								  &flags);
+				if (opts > 0) {
+					i += opts - 1;
+					continue;
+				}
 
-			if (!strcmp(arg, "--stdin")) {
-				if (revs->disable_stdin) {
-					argv[left++] = arg;
+				if (!strcmp(arg, "--stdin")) {
+					if (revs->disable_stdin) {
+						argv[left++] = arg;
+						continue;
+					}
+					if (read_from_stdin++)
+						die("--stdin given twice?");
+					read_revisions_from_stdin(revs, &prune_data);
 					continue;
 				}
-				if (read_from_stdin++)
-					die("--stdin given twice?");
-				read_revisions_from_stdin(revs, &prune_data);
-				continue;
-			}
 
-			opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv);
-			if (opts > 0) {
-				i += opts - 1;
+				opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv);
+				if (opts > 0) {
+					i += opts - 1;
+					continue;
+				}
+				if (opts < 0)
+					exit(128);
 				continue;
 			}
-			if (opts < 0)
-				exit(128);
-			continue;
-		}
-
 
-		if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
 			int j;
 			if (seen_dashdash || *arg == '^')
 				die("bad revision '%s'", arg);
@@ -2249,6 +2251,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			break;
 		}
 		else
+			/* Make sure that a filename doesn't get interpreted as a revision */
+			if (!seen_dashdash)
+				verify_non_filename(revs->prefix, arg);
 			got_rev_arg = 1;
 	}
 
diff --git a/sha1_name.c b/sha1_name.c
index 7a621ba..b99b1dc 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -483,7 +483,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1,
 				break;
 			}
 		}
-	} else if (len == 1 && str[0] == '-') {
+	} else if (len == 1 && str[0] == '-' && !str[1]) {
 		nth_prior = 1;
 	}
 
-- 
2.3.3.203.g8ffb468.dirty

[PATCH/RFC 4/4] t0102: add tests for '-' notation

[Kenny Lee Sin Cheong]

Signed-off-by: Kenny Lee Sin Cheong <kenny.lee28@gmail.com>
---
 t/t0102-previous-shorthand.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 t/t0102-previous-shorthand.sh

diff --git a/t/t0102-previous-shorthand.sh b/t/t0102-previous-shorthand.sh
new file mode 100644
index 0000000..919b055
--- /dev/null
+++ b/t/t0102-previous-shorthand.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+test_description='previous branch syntax @{-n}'
+
+. ./test-lib.sh
+
+test_expect_success 'branch -d -' '
+	test_commit A &&
+	git checkout -b junk2 &&
+	git checkout - &&
+	test "$(git symbolic-ref HEAD)" = refs/heads/master &&
+	git branch -d - &&
+	test_must_fail git rev-parse --verify refs/heads/junk2
+'
+
+test_expect_success 'merge -' '
+	git checkout A &&
+	test_commit B &&
+	git checkout A &&
+	test_commit C &&
+	test_commit D &&
+	git branch -f master B &&
+	git branch -f other &&
+	git checkout other &&
+	git checkout master &&
+	git merge - &&
+	git cat-file commit HEAD | grep "Merge branch '\''other'\''"
+'
+
+test_expect_success 'merge -~1' '
+	git checkout master &&
+	git reset --hard B &&
+	git checkout other &&
+	git checkout master &&
+	git merge -~1 &&
+	git cat-file commit HEAD >actual &&
+	grep "Merge branch '\''other'\''" actual
+'
+
+test_done
-- 
2.3.3.203.g8ffb468.dirty

Re: [PATCH/RFC 1/4] Add "-" as @{-1} support for the rev-parse command

[Junio C Hamano]

Kenny Lee Sin Cheong <kenny.lee28@gmail.com> writes:

> Allows the use of the "-" shorthand notation, including
> use with revision ranges. If we plan to allow "-" as a stand in every
> where a revision is allowed, then "-" would also need to be usable in
> plumbing commands, for writing tests, for example.
>
> Checks if the argument can be interpreted as a revision range first
> before checking for flags. This saves us from having to check that
> something that begins with "-" does not get checked as a possible flag.

Doesn't that mean -<something> that is a valid flag can no longer be
recognised as a flag if the same string can be an extended SHA-1
whose formulation starts from "the previous branch"?  It sounds like
a regression to me.

Hmmm.

After all, "we often call for the previous branch, so let's give a
short-and-sweet '-' as an even shorter short-hand than '@{-1}'" and
"allow '-' anywhere" are two quite different things.  We may do "git
checkout -" very often to go back to what we were working on, but I
do not think "git log -.." or "git log ..-" are something we want to
do very often.

I think what I am saying is that it may be perfectly fine if we said
"'-' can be used for '@{-1}' only by itself; no ranges, no
parent-traversals, no other uses", if it makes it less likely for
mistakes and confusions to happen.

Re: [PATCH/RFC 2/4] t1505: add tests for '-' notation in rev-parse

[Torsten Bögershausen]

On 03/30/2015 07:41 PM, Kenny Lee Sin Cheong wrote:
> Signed-off-by: Kenny Lee Sin Cheong <kenny.lee28@gmail.com>
> ---
>   t/t1505-rev-parse-last.sh | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1505-rev-parse-last.sh b/t/t1505-rev-parse-last.sh
> index 4969edb..a1976ad 100755
> --- a/t/t1505-rev-parse-last.sh
> +++ b/t/t1505-rev-parse-last.sh
> @@ -33,19 +33,23 @@ test_expect_success 'setup' '
>   # and 'side' should be the last branch
>   
>   test_expect_success '@{-1} works' '
> -	test_cmp_rev side @{-1}
> +	test_cmp_rev side @{-1} &&
> +	test_cmp_rev side -
>   '
(Beside that "-" is often used for "stdin" in many unix-like tools,
and my favorite would be "-1" ):

I think the test heading should be updated as well:

test_expect_success '@{-1} or - works' '
	test_cmp_rev side @{-1} &&
	test_cmp_rev side -
  '


>   
>   test_expect_success '@{-1}~2 works' '
> -	test_cmp_rev side~2 @{-1}~2
> +	test_cmp_rev side~2 @{-1}~2 &&
> +	test_cmp_rev side~2 -~2
>   '
>   
>   test_expect_success '@{-1}^2 works' '
> -	test_cmp_rev side^2 @{-1}^2
> +	test_cmp_rev side^2 @{-1}^2 &&
> +	test_cmp_rev side^2 -^2
>   '
>   
>   test_expect_success '@{-1}@{1} works' '
> -	test_cmp_rev side@{1} @{-1}@{1}
> +	test_cmp_rev side@{1} @{-1}@{1} &&
> +	test_cmp_rev side@{1} -@{1}
>   '
>   
>   test_expect_success '@{-2} works' '