[PATCH v2 00/11] sha1_name: improvements

[PATCH v2 00/11] sha1_name: improvements

[Felipe Contreras]

Hi,

While trying to add support for the @ shortcut lots of cleanups arised. Here
they are in a single series.

Felipe Contreras (7):
  tests: at-combinations: simplify setup
  tests: at-combinations: check ref names directly
  tests: at-combinations: improve nonsense()
  sha1_name: remove no-op
  sha1_name: remove unnecessary braces
  sha1_name: avoid Yoda conditions
  sha1_name: reorganize get_sha1_basic()

Ramkumar Ramachandra (4):
  tests: at-combinations: increase coverage
  tests: at-combinations: @{N} versus HEAD@{N}
  sha1_name: don't waste cycles in the @-parsing loop
  sha1_name: check @{-N} errors sooner

 sha1_name.c                | 42 +++++++++++++++++++---------------
 t/t1508-at-combinations.sh | 56 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 64 insertions(+), 34 deletions(-)

-- 
1.8.3.rc0.401.g45bba44

[PATCH v2 01/11] tests: at-combinations: simplify setup

[Felipe Contreras]

The test is setting up an upstream branch, but there's a much simpler
way of doing that: git branch -u.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t1508-at-combinations.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index d5d6244..46e3f16 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -31,10 +31,8 @@ test_expect_success 'setup' '
 	git checkout -b new-branch &&
 	test_commit new-one &&
 	test_commit new-two &&
-	git config branch.old-branch.remote . &&
-	git config branch.old-branch.merge refs/heads/master &&
-	git config branch.new-branch.remote . &&
-	git config branch.new-branch.merge refs/heads/upstream-branch
+	git branch -u master old-branch &&
+	git branch -u upstream-branch new-branch
 '
 
 check HEAD new-two
-- 
1.8.3.rc0.401.g45bba44

[PATCH v2 02/11] tests: at-combinations: check ref names directly

[Felipe Contreras]

Some committishes might point to the same commit, but through a
different ref, that's why it's better to check directly for the ref,
rather than the commit message.

We can do that by calling rev-parse --symbolic-full-name, and to
differentiate the old from the new behavior we add an extra argument to
the check() helper.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t1508-at-combinations.sh | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 46e3f16..bd2d2fe 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -4,9 +4,14 @@ test_description='test various @{X} syntax combinations together'
 . ./test-lib.sh
 
 check() {
-test_expect_${3:-success} "$1 = $2" "
-	echo '$2' >expect &&
-	git log -1 --format=%s '$1' >actual &&
+test_expect_${4:-success} "$1 = $3" "
+	if [ '$2' == 'commit' ]; then
+		echo '$3' >expect &&
+		git log -1 --format=%s '$1' >actual
+	else
+		echo '$3' >expect &&
+		git rev-parse --symbolic-full-name '$1' >actual
+	fi &&
 	test_cmp expect actual
 "
 }
@@ -35,14 +40,14 @@ test_expect_success 'setup' '
 	git branch -u upstream-branch new-branch
 '
 
-check HEAD new-two
-check "@{1}" new-one
-check "@{-1}" old-two
-check "@{-1}@{1}" old-one
-check "@{u}" upstream-two
-check "@{u}@{1}" upstream-one
-check "@{-1}@{u}" master-two
-check "@{-1}@{u}@{1}" master-one
+check HEAD ref refs/heads/new-branch
+check "@{1}" commit new-one
+check "@{-1}" ref refs/heads/old-branch
+check "@{-1}@{1}" commit old-one
+check "@{u}" ref refs/heads/upstream-branch
+check "@{u}@{1}" commit upstream-one
+check "@{-1}@{u}" ref refs/heads/master
+check "@{-1}@{u}@{1}" commit master-one
 nonsense "@{u}@{-1}"
 nonsense "@{1}@{u}"
 
-- 
1.8.3.rc0.401.g45bba44

Re: [PATCH v2 02/11] tests: at-combinations: check ref names directly

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Some committishes might point to the same commit, but through a
> different ref, that's why it's better to check directly for the ref,
> rather than the commit message.
>
> We can do that by calling rev-parse --symbolic-full-name, and to
> differentiate the old from the new behavior we add an extra argument to
> the check() helper.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---

It is signed-off by Ram first but who is the author?  You, or him?

>  t/t1508-at-combinations.sh | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
> index 46e3f16..bd2d2fe 100755
> --- a/t/t1508-at-combinations.sh
> +++ b/t/t1508-at-combinations.sh
> @@ -4,9 +4,14 @@ test_description='test various @{X} syntax combinations together'
>  . ./test-lib.sh
>  
>  check() {
> -test_expect_${3:-success} "$1 = $2" "
> -	echo '$2' >expect &&
> -	git log -1 --format=%s '$1' >actual &&
> +test_expect_${4:-success} "$1 = $3" "
> +	if [ '$2' == 'commit' ]; then
> +		echo '$3' >expect &&
> +		git log -1 --format=%s '$1' >actual
> +	else
> +		echo '$3' >expect &&
> +		git rev-parse --symbolic-full-name '$1' >actual
> +	fi &&

Move the echo outside of if, and match the overall style.

	echo '$3' >expect &&
        if test '$2' = commit
	then
		git log ...
	else
		git rev-parse ...
	fi &&


>  	test_cmp expect actual
>  "
>  }
> @@ -35,14 +40,14 @@ test_expect_success 'setup' '
>  	git branch -u upstream-branch new-branch
>  '
>  
> -check HEAD new-two
> -check "@{1}" new-one
> -check "@{-1}" old-two
> -check "@{-1}@{1}" old-one
> -check "@{u}" upstream-two
> -check "@{u}@{1}" upstream-one
> -check "@{-1}@{u}" master-two
> -check "@{-1}@{u}@{1}" master-one
> +check HEAD ref refs/heads/new-branch
> +check "@{1}" commit new-one
> +check "@{-1}" ref refs/heads/old-branch
> +check "@{-1}@{1}" commit old-one
> +check "@{u}" ref refs/heads/upstream-branch
> +check "@{u}@{1}" commit upstream-one
> +check "@{-1}@{u}" ref refs/heads/master
> +check "@{-1}@{u}@{1}" commit master-one
>  nonsense "@{u}@{-1}"
>  nonsense "@{1}@{u}"

Re: [PATCH v2 02/11] tests: at-combinations: check ref names directly

[Felipe Contreras]

On Wed, May 8, 2013 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Some committishes might point to the same commit, but through a
>> different ref, that's why it's better to check directly for the ref,
>> rather than the commit message.
>>
>> We can do that by calling rev-parse --symbolic-full-name, and to
>> differentiate the old from the new behavior we add an extra argument to
>> the check() helper.
>>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>
> It is signed-off by Ram first but who is the author?  You, or him?

I am, but he sent the patch first.

>>  t/t1508-at-combinations.sh | 27 ++++++++++++++++-----------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
>> index 46e3f16..bd2d2fe 100755
>> --- a/t/t1508-at-combinations.sh
>> +++ b/t/t1508-at-combinations.sh
>> @@ -4,9 +4,14 @@ test_description='test various @{X} syntax combinations together'
>>  . ./test-lib.sh
>>
>>  check() {
>> -test_expect_${3:-success} "$1 = $2" "
>> -     echo '$2' >expect &&
>> -     git log -1 --format=%s '$1' >actual &&
>> +test_expect_${4:-success} "$1 = $3" "
>> +     if [ '$2' == 'commit' ]; then
>> +             echo '$3' >expect &&
>> +             git log -1 --format=%s '$1' >actual
>> +     else
>> +             echo '$3' >expect &&
>> +             git rev-parse --symbolic-full-name '$1' >actual
>> +     fi &&
>
> Move the echo outside of if, and match the overall style.

Right.

-- 
Felipe Contreras

[PATCH v2 03/11] tests: at-combinations: improve nonsense()

[Felipe Contreras]

In some circumstances 'git log' might fail, but not because the @
parsing failed. For example: 'git rev-parse' might succeed and return a
bad object, and then 'git log' would fail.

The layer we want to test is revision parsing, so let's test that
directly.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t1508-at-combinations.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index bd2d2fe..2ea735e 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -17,7 +17,7 @@ test_expect_${4:-success} "$1 = $3" "
 }
 nonsense() {
 test_expect_${2:-success} "$1 is nonsensical" "
-	test_must_fail git log -1 '$1'
+	test_must_fail git rev-parse '$1'
 "
 }
 fail() {
-- 
1.8.3.rc0.401.g45bba44

Re: [PATCH v2 03/11] tests: at-combinations: improve nonsense()

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> In some circumstances 'git log' might fail, but not because the @
> parsing failed. For example: 'git rev-parse' might succeed and return a
> bad object, and then 'git log' would fail.
>
> The layer we want to test is revision parsing, so let's test that
> directly.

Hmph, but

	git rev-parse Makefile

would happily succeed if there happens to be Makefile in the
directory.

Are we expecting that they are always object names?  If that is the
case, perhaps

	git rev-parse --verify "$1"

would express the intention better.

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t1508-at-combinations.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
> index bd2d2fe..2ea735e 100755
> --- a/t/t1508-at-combinations.sh
> +++ b/t/t1508-at-combinations.sh
> @@ -17,7 +17,7 @@ test_expect_${4:-success} "$1 = $3" "
>  }
>  nonsense() {
>  test_expect_${2:-success} "$1 is nonsensical" "
> -	test_must_fail git log -1 '$1'
> +	test_must_fail git rev-parse '$1'
>  "
>  }
>  fail() {

Re: [PATCH v2 03/11] tests: at-combinations: improve nonsense()

[Felipe Contreras]

On Wed, May 8, 2013 at 12:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> In some circumstances 'git log' might fail, but not because the @
>> parsing failed. For example: 'git rev-parse' might succeed and return a
>> bad object, and then 'git log' would fail.
>>
>> The layer we want to test is revision parsing, so let's test that
>> directly.
>
> Hmph, but
>
>         git rev-parse Makefile
>
> would happily succeed if there happens to be Makefile in the
> directory.
>
> Are we expecting that they are always object names?  If that is the
> case, perhaps
>
>         git rev-parse --verify "$1"
>
> would express the intention better.

Probably, although the same would fail before this patch.

-- 
Felipe Contreras

[PATCH v2 04/11] tests: at-combinations: increase coverage

[Felipe Contreras]

From: Ramkumar Ramachandra <artagnon@gmail.com>

Add more tests exercising documented functionality.

[fc: commit message and extra tests]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t1508-at-combinations.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 2ea735e..58cd1fe 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -42,13 +42,21 @@ test_expect_success 'setup' '
 
 check HEAD ref refs/heads/new-branch
 check "@{1}" commit new-one
+check "HEAD@{1}" commit new-one
+check "@{now}" commit new-two
+check "HEAD@{now}" commit new-two
 check "@{-1}" ref refs/heads/old-branch
+check "@{-1}@{0}" commit old-two
 check "@{-1}@{1}" commit old-one
 check "@{u}" ref refs/heads/upstream-branch
+check "HEAD@{u}" ref refs/heads/upstream-branch
 check "@{u}@{1}" commit upstream-one
 check "@{-1}@{u}" ref refs/heads/master
 check "@{-1}@{u}@{1}" commit master-one
 nonsense "@{u}@{-1}"
+nonsense "@{0}@{0}"
 nonsense "@{1}@{u}"
+nonsense "HEAD@{-1}"
+nonsense "@{-1}@{-1}"
 
 test_done
-- 
1.8.3.rc0.401.g45bba44

[PATCH v2 05/11] tests: at-combinations: @{N} versus HEAD@{N}

[Felipe Contreras]

From: Ramkumar Ramachandra <artagnon@gmail.com>

All the tests so far check that @{N} is the same as HEAD@{N} (for
positive N). However, this is not always the case; write a couple of
tests for this.

[fc: simplify some wording]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t1508-at-combinations.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 58cd1fe..13b0372 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -59,4 +59,17 @@ nonsense "@{1}@{u}"
 nonsense "HEAD@{-1}"
 nonsense "@{-1}@{-1}"
 
+# @{N} versus HEAD@{N}
+
+check "HEAD@{3}" commit old-two
+nonsense "@{3}"
+
+test_expect_success 'switch to old-branch' '
+	git checkout old-branch
+'
+
+check HEAD ref refs/heads/old-branch
+check "HEAD@{1}" commit new-two
+check "@{1}" commit old-one
+
 test_done
-- 
1.8.3.rc0.401.g45bba44

[PATCH v2 06/11] sha1_name: remove no-op

[Felipe Contreras]

'at' is always 0, since we can reach this point only if
!len && reflog_len, and len=at when reflog is assigned.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 sha1_name.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index 3820f28..01e49a9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -464,7 +464,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		struct strbuf buf = STRBUF_INIT;
 		int ret;
 		/* try the @{-N} syntax for n-th checkout */
-		ret = interpret_branch_name(str+at, &buf);
+		ret = interpret_branch_name(str, &buf);
 		if (ret > 0) {
 			/* substitute this branch name and restart */
 			return get_sha1_1(buf.buf, buf.len, sha1, 0);
-- 
1.8.3.rc0.401.g45bba44

[PATCH v2 07/11] sha1_name: remove unnecessary braces

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 sha1_name.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 01e49a9..6530ddd 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -465,12 +465,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		int ret;
 		/* try the @{-N} syntax for n-th checkout */
 		ret = interpret_branch_name(str, &buf);
-		if (ret > 0) {
+		if (ret > 0)
 			/* substitute this branch name and restart */
 			return get_sha1_1(buf.buf, buf.len, sha1, 0);
-		} else if (ret == 0) {
+		else if (ret == 0)
 			return -1;
-		}
 		/* allow "@{...}" to mean the current branch reflog */
 		refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
 	} else if (reflog_len)
-- 
1.8.3.rc0.401.g45bba44

[PATCH v2 08/11] sha1_name: avoid Yoda conditions

[Felipe Contreras]

Speak in reverse we shall not; compare variable with constant, not
constant with variable.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 sha1_name.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 6530ddd..93c4e8c 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -996,9 +996,9 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 
 	if (!len)
 		return len; /* syntax Ok, not enough switches */
-	if (0 < len && len == namelen)
+	if (len > 0 && len == namelen)
 		return len; /* consumed all */
-	else if (0 < len) {
+	else if (len > 0) {
 		/* we have extra data, which might need further processing */
 		struct strbuf tmp = STRBUF_INIT;
 		int used = buf->len;
-- 
1.8.3.rc0.401.g45bba44

[PATCH v2 09/11] sha1_name: don't waste cycles in the @-parsing loop

[Felipe Contreras]

From: Ramkumar Ramachandra <artagnon@gmail.com>

The @-parsing loop unnecessarily checks for the sequence "@{" from len -
2 unnecessarily. We can safely check from len - 4.

[fc: remove cruft]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 sha1_name.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index 93c4e8c..0acc6e0 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -445,7 +445,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 	/* basic@{time or number or -number} format to query ref-log */
 	reflog_len = at = 0;
 	if (len && str[len-1] == '}') {
-		for (at = len-2; at >= 0; at--) {
+		for (at = len-4; at >= 0; at--) {
 			if (str[at] == '@' && str[at+1] == '{') {
 				if (!upstream_mark(str + at, len - at)) {
 					reflog_len = (len-1) - (at+2);
-- 
1.8.3.rc0.401.g45bba44

[PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()

[Felipe Contreras]

Through the years the functionality to handle @{-N} and @{u} has moved
around the code, and as a result, code that once made sense, doesn't any
more.

There is no need to call this function recursively with the branch of
@{-N} substituted because dwim_{ref,log} already replaces it.

However, there's one corner-case where @{-N} resolves to a detached
HEAD, in which case we wouldn't get any ref back.

So we parse the nth-prior manually, and deal with it depending on
weather it's a SHA-1, or a ref.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 sha1_name.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 0acc6e0..c512c69 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -431,13 +431,14 @@ static inline int upstream_mark(const char *string, int len)
 }
 
 static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);
+static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf);
 
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 {
 	static const char *warn_msg = "refname '%.*s' is ambiguous.";
 	char *real_ref = NULL;
 	int refs_found = 0;
-	int at, reflog_len;
+	int at, reflog_len, nth_prior = 0;
 
 	if (len == 40 && !get_sha1_hex(str, sha1))
 		return 0;
@@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 	if (len && str[len-1] == '}') {
 		for (at = len-4; at >= 0; at--) {
 			if (str[at] == '@' && str[at+1] == '{') {
+				if (at == 0 && str[2] == '-') {
+					nth_prior = 1;
+					continue;
+				}
 				if (!upstream_mark(str + at, len - at)) {
 					reflog_len = (len-1) - (at+2);
 					len = at;
@@ -460,19 +465,22 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 	if (len && ambiguous_path(str, len))
 		return -1;
 
-	if (!len && reflog_len) {
+	if (nth_prior) {
 		struct strbuf buf = STRBUF_INIT;
-		int ret;
-		/* try the @{-N} syntax for n-th checkout */
-		ret = interpret_branch_name(str, &buf);
-		if (ret > 0)
-			/* substitute this branch name and restart */
-			return get_sha1_1(buf.buf, buf.len, sha1, 0);
-		else if (ret == 0)
-			return -1;
+		int detached;
+
+		if (interpret_nth_prior_checkout(str, &buf) > 0) {
+			detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));
+			strbuf_release(&buf);
+			if (detached)
+				return 0;
+		}
+	}
+
+	if (!len && reflog_len)
 		/* allow "@{...}" to mean the current branch reflog */
 		refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
-	} else if (reflog_len)
+	else if (reflog_len)
 		refs_found = dwim_log(str, len, sha1, &real_ref);
 	else
 		refs_found = dwim_ref(str, len, sha1, &real_ref);
-- 
1.8.3.rc0.401.g45bba44

Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> ---
>  sha1_name.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)

How has this changed since my eyeballing of the previous version?  An
inter-diff would be nice: having spent a significant amount of time
looking at this area, I can confirm that the patch is Correct.

Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()

[Felipe Contreras]

On Wed, May 8, 2013 at 2:39 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> ---
>>  sha1_name.c | 30 +++++++++++++++++++-----------
>>  1 file changed, 19 insertions(+), 11 deletions(-)
>
> How has this changed since my eyeballing of the previous version?  An
> inter-diff would be nice: having spent a significant amount of time
> looking at this area, I can confirm that the patch is Correct.

You mean this compared to v4? It hasn't changed.

-- 
Felipe Contreras

Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Through the years the functionality to handle @{-N} and @{u} has moved
> around the code, and as a result, code that once made sense, doesn't any
> more.
>
> There is no need to call this function recursively with the branch of
> @{-N} substituted because dwim_{ref,log} already replaces it.
>
> However, there's one corner-case where @{-N} resolves to a detached
> HEAD, in which case we wouldn't get any ref back.
>
> So we parse the nth-prior manually, and deal with it depending on
> weather it's a SHA-1, or a ref.
> ...

s/weather/whether/;

> @@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>  	if (len && str[len-1] == '}') {
>  		for (at = len-4; at >= 0; at--) {
>  			if (str[at] == '@' && str[at+1] == '{') {
> +				if (at == 0 && str[2] == '-') {
> +					nth_prior = 1;
> +					continue;
> +				}

Does this have to be inside the loop?

> @@ -460,19 +465,22 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>  	if (len && ambiguous_path(str, len))
>  		return -1;
>  
> -	if (!len && reflog_len) {
> +	if (nth_prior) {
>  		struct strbuf buf = STRBUF_INIT;
> -		int ret;
> -		/* try the @{-N} syntax for n-th checkout */
> -		ret = interpret_branch_name(str, &buf);
> -		if (ret > 0)
> -			/* substitute this branch name and restart */
> -			return get_sha1_1(buf.buf, buf.len, sha1, 0);
> -		else if (ret == 0)
> -			return -1;
> +		int detached;
> +
> +		if (interpret_nth_prior_checkout(str, &buf) > 0) {
> +			detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));
> +			strbuf_release(&buf);
> +			if (detached)
> +				return 0;
> +		}
> +	}

Earlier, if @{-N} resolved to a detached head, we just fed it to
get_sha1_1().  If it resolved to a concrete refname, we also fed it
to get_sha1_1().  We ended up calling ourselves again and did the
right thing either way.

The new code bypasses the recursive call when we get a detached head
back, because we know that calling get_sha1_1() with the 40-hex will
eventually take us back to this codepath, and immediately return
when it sees get_sha1_hex() succeeds.

What happens when str @{-N} leaves a concrete refname in buf.buf?
The branch name is lost with strbuf_release(), and then where do we
go from here?  Continuing down from here would run dwim_ref/log on
str which is still @{-N}, no?

Ahh, OK, the new code will now let dwim_ref/log to process @{-N}
again (the log message hints this but it wasn't all that clear), so
even though we already learned the branch name in buf here and
discard it, we will eventually discover the same information later.

That is somewhat contrived, and I am not so sure if that is a good
reorganization.

Also, a few points this patch highlights in the code before the
change:

 - If we were on a branch with 40-hex name at nth prior checkout,
   would we mistake it as being detached at the commit?

 - If we were on a branch 'foo' at nth prior checkout, would our
   previous get_sha1_1() have made us mistake it as referring to a
   tag 'foo' with the same name if it exists?

The former needs a fix to either writing of reflog or reading by
interpret_nth_prior_checkout() so that we can tell these cases apart
more reliably.  Then the latter can be solved by splicing
refs/heads/ in front of the branch name before recursively calling
get_sha1_1() in the original code (and similar fix can be
forward-ported to this patch).

Incidentally, I think if we prefix refs/heads/ in front and feed
that to dwim_ref/log, we would also avoid running @{-N} twice (which
I suspect might be more expensive than a simple recursion, as it
needs to go through the reflog file), which may be a nice side
effect of such a fix we would get for free.

> +
> +	if (!len && reflog_len)
>  		/* allow "@{...}" to mean the current branch reflog */
>  		refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
> -	} else if (reflog_len)
> +	else if (reflog_len)
>  		refs_found = dwim_log(str, len, sha1, &real_ref);
>  	else
>  		refs_found = dwim_ref(str, len, sha1, &real_ref);

Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()

[Junio C Hamano]

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

> Also, a few points this patch highlights in the code before the
> change:
>
>  - If we were on a branch with 40-hex name at nth prior checkout,
>    would we mistake it as being detached at the commit?
>
>  - If we were on a branch 'foo' at nth prior checkout, would our
>    previous get_sha1_1() have made us mistake it as referring to a
>    tag 'foo' with the same name if it exists?
>
> The former needs a fix to either writing of reflog or reading by
> interpret_nth_prior_checkout() so that we can tell these cases apart
> more reliably.  Then the latter can be solved by splicing
> refs/heads/ in front of the branch name before recursively calling
> get_sha1_1() in the original code (and similar fix can be
> forward-ported to this patch).
>
> Incidentally, I think if we prefix refs/heads/ in front and feed
> that to dwim_ref/log, we would also avoid running @{-N} twice (which
> I suspect might be more expensive than a simple recursion, as it
> needs to go through the reflog file), which may be a nice side
> effect of such a fix we would get for free.

Here is the first step (i.e. more reliable interpret_nth_prior).

I looked at all the existing callers of interpret_branch_name() and
it appears to me that most of them currently assume they are getting
a branch name, because they want to work on a ref, and some of them
do not care, because they are working on a revision.  For the
former, they can (and should) error out instead of relying on not
having refs/heads/$detached_SHA1 that will prevent them from working
on a ref which is what they currently do, if they had the "detached"
information.  For the latter, if we give "detached" information,
they can either prefix "refs/heads/" (if the result is "not
detached") to call resolve_ref(), or call get_sha1_hex (if the
result is "detached"), which would be the solution for the second
issue I noticed in the message I am replying to.

The next step on top of this patch may be to expose the "detached"
bit up in the API chain to let callers of interpret_branch_name() to
know about the distinction.

 sha1_name.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 3820f28..3dd6667 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -862,6 +862,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
 struct grab_nth_branch_switch_cbdata {
 	int remaining;
 	struct strbuf buf;
+	int detached;
 };
 
 static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
@@ -880,9 +881,14 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
 	if (!match || !target)
 		return 0;
 	if (--(cb->remaining) == 0) {
+		unsigned char sha1[20];
+
 		len = target - match;
 		strbuf_reset(&cb->buf);
 		strbuf_add(&cb->buf, match, len);
+		cb->detached = (len == 40 &&
+				!get_sha1_hex(match, sha1) &&
+				!hashcmp(osha1, sha1));
 		return 1; /* we are done */
 	}
 	return 0;
@@ -892,7 +898,7 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
  * Parse @{-N} syntax, return the number of characters parsed
  * if successful; otherwise signal an error with negative value.
  */
-static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)
+static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf, int *detached)
 {
 	long nth;
 	int retval;
@@ -917,6 +923,8 @@ static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)
 	if (0 < for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch, &cb)) {
 		strbuf_reset(buf);
 		strbuf_add(buf, cb.buf.buf, cb.buf.len);
+		if (detached)
+			*detached = cb.detached;
 		retval = brace - name + 1;
 	}
 
@@ -992,7 +1000,7 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 	char *cp;
 	struct branch *upstream;
 	int namelen = strlen(name);
-	int len = interpret_nth_prior_checkout(name, buf);
+	int len = interpret_nth_prior_checkout(name, buf, NULL);
 	int tmp_len;
 
 	if (!len)

Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()

[Felipe Contreras]

On Wed, May 8, 2013 at 1:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Through the years the functionality to handle @{-N} and @{u} has moved
>> around the code, and as a result, code that once made sense, doesn't any
>> more.
>>
>> There is no need to call this function recursively with the branch of
>> @{-N} substituted because dwim_{ref,log} already replaces it.
>>
>> However, there's one corner-case where @{-N} resolves to a detached
>> HEAD, in which case we wouldn't get any ref back.
>>
>> So we parse the nth-prior manually, and deal with it depending on
>> weather it's a SHA-1, or a ref.
>> ...
>
> s/weather/whether/;
>
>> @@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>>       if (len && str[len-1] == '}') {
>>               for (at = len-4; at >= 0; at--) {
>>                       if (str[at] == '@' && str[at+1] == '{') {
>> +                             if (at == 0 && str[2] == '-') {
>> +                                     nth_prior = 1;
>> +                                     continue;
>> +                             }
>
> Does this have to be inside the loop?

Yes, the whole purpose is to avoid reflog_len to be set.

>> @@ -460,19 +465,22 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>>       if (len && ambiguous_path(str, len))
>>               return -1;
>>
>> -     if (!len && reflog_len) {
>> +     if (nth_prior) {
>>               struct strbuf buf = STRBUF_INIT;
>> -             int ret;
>> -             /* try the @{-N} syntax for n-th checkout */
>> -             ret = interpret_branch_name(str, &buf);
>> -             if (ret > 0)
>> -                     /* substitute this branch name and restart */
>> -                     return get_sha1_1(buf.buf, buf.len, sha1, 0);
>> -             else if (ret == 0)
>> -                     return -1;
>> +             int detached;
>> +
>> +             if (interpret_nth_prior_checkout(str, &buf) > 0) {
>> +                     detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));
>> +                     strbuf_release(&buf);
>> +                     if (detached)
>> +                             return 0;
>> +             }
>> +     }
>
> Earlier, if @{-N} resolved to a detached head, we just fed it to
> get_sha1_1().  If it resolved to a concrete refname, we also fed it
> to get_sha1_1().  We ended up calling ourselves again and did the
> right thing either way.
>
> The new code bypasses the recursive call when we get a detached head
> back, because we know that calling get_sha1_1() with the 40-hex will
> eventually take us back to this codepath, and immediately return
> when it sees get_sha1_hex() succeeds.
>
> What happens when str @{-N} leaves a concrete refname in buf.buf?
> The branch name is lost with strbuf_release(), and then where do we
> go from here?  Continuing down from here would run dwim_ref/log on
> str which is still @{-N}, no?
>
> Ahh, OK, the new code will now let dwim_ref/log to process @{-N}
> again (the log message hints this but it wasn't all that clear),

I thought it was clear we would let dwim_{ref,log} do the job:

---
There is no need to call this function recursively with the branch of
@{-N} substituted because dwim_{ref,log} already replaces it.
---

> That is somewhat contrived, and I am not so sure if that is a good
> reorganization.

But much less contrived than before, because the code that deals with
@{-N} is in one place, instead of sprinkled all over as many
corner-cases, and there's no recursion.

> Also, a few points this patch highlights in the code before the
> change:
>
>  - If we were on a branch with 40-hex name at nth prior checkout,
>    would we mistake it as being detached at the commit?
>
>  - If we were on a branch 'foo' at nth prior checkout, would our
>    previous get_sha1_1() have made us mistake it as referring to a
>    tag 'foo' with the same name if it exists?

I don't know, but I suspect there's no change after this patch.

-- 
Felipe Contreras

Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Wed, May 8, 2013 at 1:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> Through the years the functionality to handle @{-N} and @{u} has moved
>>> around the code, and as a result, code that once made sense, doesn't any
>>> more.
>>>
>>> There is no need to call this function recursively with the branch of
>>> @{-N} substituted because dwim_{ref,log} already replaces it.
>>>
>>> However, there's one corner-case where @{-N} resolves to a detached
>>> HEAD, in which case we wouldn't get any ref back.
>>>
>>> So we parse the nth-prior manually, and deal with it depending on
>>> weather it's a SHA-1, or a ref.
>>> ...
>>
>> s/weather/whether/;
>>
>>> @@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>>>       if (len && str[len-1] == '}') {
>>>               for (at = len-4; at >= 0; at--) {
>>>                       if (str[at] == '@' && str[at+1] == '{') {
>>> +                             if (at == 0 && str[2] == '-') {
>>> +                                     nth_prior = 1;
>>> +                                     continue;
>>> +                             }
>>
>> Does this have to be inside the loop?
>
> Yes, the whole purpose is to avoid reflog_len to be set.

What I meant was the "<nothing>@{-" check, which happens only at==0.

	if (!memcmp(str, "@{-", 3) && len > 3)
		nth_prior = 1;
	else
		for (at = len - 4; at; at--) {
			... look for and break at the first "@{" ...
		}

or something.

>> Ahh, OK, the new code will now let dwim_ref/log to process @{-N}
>> again (the log message hints this but it wasn't all that clear),
>
> I thought it was clear we would let dwim_{ref,log} do the job:

Yes, the reason I did not immediately think of that is because I
knew @{-N} was expensive (need to read reflog backwards) and didn't
think anybody would redo the code to deliberately do that twice ;-)

>> Also, a few points this patch highlights in the code before the
>> change:
>>
>>  - If we were on a branch with 40-hex name at nth prior checkout,
>>    would we mistake it as being detached at the commit?
>>
>>  - If we were on a branch 'foo' at nth prior checkout, would our
>>    previous get_sha1_1() have made us mistake it as referring to a
>>    tag 'foo' with the same name if it exists?
>
> I don't know, but I suspect there's no change after this patch.

Yes, didn't I say "the code before the change" above?

These two correctness issues look more important issues to me, with
or without the restructure patch (in other words, they are
independent).

Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()

[Felipe Contreras]

On Wed, May 8, 2013 at 4:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Wed, May 8, 2013 at 1:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> Through the years the functionality to handle @{-N} and @{u} has moved
>>>> around the code, and as a result, code that once made sense, doesn't any
>>>> more.
>>>>
>>>> There is no need to call this function recursively with the branch of
>>>> @{-N} substituted because dwim_{ref,log} already replaces it.
>>>>
>>>> However, there's one corner-case where @{-N} resolves to a detached
>>>> HEAD, in which case we wouldn't get any ref back.
>>>>
>>>> So we parse the nth-prior manually, and deal with it depending on
>>>> weather it's a SHA-1, or a ref.
>>>> ...
>>>
>>> s/weather/whether/;
>>>
>>>> @@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>>>>       if (len && str[len-1] == '}') {
>>>>               for (at = len-4; at >= 0; at--) {
>>>>                       if (str[at] == '@' && str[at+1] == '{') {
>>>> +                             if (at == 0 && str[2] == '-') {
>>>> +                                     nth_prior = 1;
>>>> +                                     continue;
>>>> +                             }
>>>
>>> Does this have to be inside the loop?
>>
>> Yes, the whole purpose is to avoid reflog_len to be set.
>
> What I meant was the "<nothing>@{-" check, which happens only at==0.
>
>         if (!memcmp(str, "@{-", 3) && len > 3)
>                 nth_prior = 1;
>         else
>                 for (at = len - 4; at; at--) {
>                         ... look for and break at the first "@{" ...
>                 }
>
> or something.

That's doable, but would screw up the next patch.

>>> Ahh, OK, the new code will now let dwim_ref/log to process @{-N}
>>> again (the log message hints this but it wasn't all that clear),
>>
>> I thought it was clear we would let dwim_{ref,log} do the job:
>
> Yes, the reason I did not immediately think of that is because I
> knew @{-N} was expensive (need to read reflog backwards) and didn't
> think anybody would redo the code to deliberately do that twice ;-)

But that's what the commit message said.

>>> Also, a few points this patch highlights in the code before the
>>> change:
>>>
>>>  - If we were on a branch with 40-hex name at nth prior checkout,
>>>    would we mistake it as being detached at the commit?
>>>
>>>  - If we were on a branch 'foo' at nth prior checkout, would our
>>>    previous get_sha1_1() have made us mistake it as referring to a
>>>    tag 'foo' with the same name if it exists?
>>
>> I don't know, but I suspect there's no change after this patch.
>
> Yes, didn't I say "the code before the change" above?
>
> These two correctness issues look more important issues to me, with
> or without the restructure patch (in other words, they are
> independent).

Right, if you are interested in correctness, you might want to try
@{-1}{0}; it resolves to @{-1} currently, and it fails correctly with
my patch.

Cheers.

-- 
Felipe Contreras

[PATCH v2 11/11] sha1_name: check @{-N} errors sooner

[Felipe Contreras]

From: Ramkumar Ramachandra <artagnon@gmail.com>

It's trivial to check for them in the @{N} parsing loop.

[fc: style]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 sha1_name.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index c512c69..db965af 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -448,7 +448,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 	if (len && str[len-1] == '}') {
 		for (at = len-4; at >= 0; at--) {
 			if (str[at] == '@' && str[at+1] == '{') {
-				if (at == 0 && str[2] == '-') {
+				if (str[at+2] == '-') {
+					if (at != 0)
+						/* @{-N} not at start */
+						return -1;
 					nth_prior = 1;
 					continue;
 				}
@@ -497,10 +500,6 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		unsigned long co_time;
 		int co_tz, co_cnt;
 
-		/* a @{-N} placed anywhere except the start is an error */
-		if (str[at+2] == '-')
-			return -1;
-
 		/* Is it asking for N-th entry, or approxidate? */
 		for (i = nth = 0; 0 <= nth && i < reflog_len; i++) {
 			char ch = str[at+2+i];
-- 
1.8.3.rc0.401.g45bba44

Re: [PATCH v2 00/11] sha1_name: improvements

[Felipe Contreras]

On Tue, May 7, 2013 at 4:55 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> While trying to add support for the @ shortcut lots of cleanups arised. Here
> they are in a single series.
>
> Felipe Contreras (7):
>   tests: at-combinations: simplify setup
>   tests: at-combinations: check ref names directly
>   tests: at-combinations: improve nonsense()
>   sha1_name: remove no-op
>   sha1_name: remove unnecessary braces
>   sha1_name: avoid Yoda conditions
>   sha1_name: reorganize get_sha1_basic()
>
> Ramkumar Ramachandra (4):
>   tests: at-combinations: increase coverage
>   tests: at-combinations: @{N} versus HEAD@{N}
>   sha1_name: don't waste cycles in the @-parsing loop
>   sha1_name: check @{-N} errors sooner

When merging this series to the @ shortcut one, there will be
conflicts, this is how I propose fixing them:

                return len; /* syntax Ok, not enough switches */
-       if (0 < len && len == namelen)
+       if (len > 0 && len == namelen)
                return len; /* consumed all */
-       else if (0 < len)
...
++      else if (len > 0)
 +              return reinterpret(name, namelen, len, buf);

- check "@" new-two
- check "@@{u}" upstream-two
...
++check "@" ref refs/heads/new-branch
++check "@@{u}" ref refs/heads/upstream-branch

If that creates some kind of problem I would rather throw away this
series rather than the other one.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2 00/11] sha1_name: improvements

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> When merging this series to the @ shortcut one, there will be
> conflicts, this is how I propose fixing them:
>
>                 return len; /* syntax Ok, not enough switches */
> -       if (0 < len && len == namelen)
> +       if (len > 0 && len == namelen)
>                 return len; /* consumed all */
> -       else if (0 < len)
> ...
> ++      else if (len > 0)
>  +              return reinterpret(name, namelen, len, buf);
>
> - check "@" new-two
> - check "@@{u}" upstream-two
> ...
> ++check "@" ref refs/heads/new-branch
> ++check "@@{u}" ref refs/heads/upstream-branch

The resolution for the tests wrapper that acquired an extra
parameter matches what I did locally.  Thanks for a merge sanity
check.

I didn't see any conflicts on the sha1_name.c side, but I applied
the Yoda thing slightly differently to result in a slightly more
streamlined codeflow:

	if (!len) {
		return len;
	} else if (len > 0) {
		if (len == namelen)
                	return len;
		else
			return reinterpret(...);
	}

which I think shows the choices better.

Although I haven't looked at the largest one (10/11) carefully,
everything else looked quite straightforward and readable.

I am not very happy about how $<n> parameters are quoted in t1508,
but that suboptimal quoting were there before this series, and I'd
consider it outside of the scope for now.

Will queue.  Thanks.

Re: [PATCH v2 00/11] sha1_name: improvements

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Will queue.  Thanks.

Nit: you might want to add my s-o-b on patches 73027d and b018e8 queued in pu.