threads / patch / 33700

v2, 9 partsA natural solution to the @ -> HEAD problem

Subject: [PATCH v2 0/9] A natural solution to the @ -> HEAD problem

## tl;dr

24 messages between May 2, 2013 and May 2, 2013. Diffs are folded; open one to read it.

replies: 23people: 3as markdown or json

Ramkumar Ramachandra· May 2, 2013, 13:39 UTC · lore
Hi,

So, we're back with v2 of the series. Six patches add comprehensive tests to at-combinations, guarding against any future bugs. [7/9] is the most important part, where I've fixed branch_get() with a long justification. [8/9] and [9/9] are almost trivial.

As Felipe pointed out, [9/9] might require more documentation, but I don't know where to put it. Help in the area is appreciated.

I've excluded all @-parsing simplification in this series. It will come as a later series based on this one: I'm currently integrating Felipe's work with mine.

Felipe: can I have your sign-off for the first three parts?
Thanks.
Felipe Contreras (3):
  t1508 (at-combinations): simplify setup
  t1508 (at-combinations): test branches separately
  t1508 (at-combinations): improve nonsense()
Ramkumar Ramachandra (6):
  t1508 (at-combinations): increase coverage
  t1508 (at-combinations): document @{N} versus HEAD@{N}
  t1508 (at-combinations): test with symbolic refs
  remote.c: teach branch_get() to treat symrefs other than HEAD
  sha1_name.c: fix @-parsing bug in interpret_branch_name()
  refs.c: make @ a pseudo-ref alias to HEAD
 Documentation/git-check-ref-format.txt |  2 +
 Documentation/revisions.txt            |  8 +++-
 refs.c                                 | 12 +++++-
 remote.c                               | 23 ++++++++++--
 sha1_name.c                            |  2 +-
 t/t1400-update-ref.sh                  |  3 ++
 t/t1508-at-combinations.sh             | 68 ++++++++++++++++++++++++++--------
 7 files changed, 94 insertions(+), 24 deletions(-)
-- 
1.8.3.rc0.40.g09a0447
Ramkumar Ramachandra· May 2, 2013, 13:39 UTC · re: Ramkumar Ramachandra · lore

[PATCH 1/9] t1508 (at-combinations): simplify setup

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

Instead of setting the configuration variables branch.<name>.merge and branch.<name>.remote by hand, use the shortcut 'git branch -u', which is exactly equivalent. It's much more readable now.

[rr: commit message]
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1508-at-combinations.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
Show changes to t/t1508-at-combinations.sh +2 −4
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.40.g09a0447
Ramkumar Ramachandra· May 2, 2013, 13:39 UTC · re: Ramkumar Ramachandra · lore

[PATCH 2/9] t1508 (at-combinations): test branches separately

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

In the tests involving @{-1} and @{u} as the final component, what we really want to check is if it's pointing to the right ref. We currently check the tip commit of the ref, but we can clarify this by separating out checking for commits versus checking for refs at check().

[rr: commit message, fix arguments in check()]
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1508-at-combinations.sh | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)
Show changes to t/t1508-at-combinations.sh +16 −11
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 46e3f16..cacb2d0 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:-$2}" "
+	if [ '$2' == 'commit' ]; then
+		echo '$3' >expect &&
+		git log -1 --format=%s '$1' >actual
+	else
+		echo '${3:-$2}' >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 refs/heads/new-branch
+check "@{1}" commit new-one
+check "@{-1}" refs/heads/old-branch
+check "@{-1}@{1}" commit old-one
+check "@{u}" refs/heads/upstream-branch
+check "@{u}@{1}" commit upstream-one
+check "@{-1}@{u}" refs/heads/master
+check "@{-1}@{u}@{1}" commit master-one
 nonsense "@{u}@{-1}"
 nonsense "@{1}@{u}"
 
-- 
1.8.3.rc0.40.g09a0447
Felipe Contreras· May 2, 2013, 17:23 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 2/9] t1508 (at-combinations): test branches separately

On Thu, May 2, 2013 at 8:39 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 37 quoted lines
> From: Felipe Contreras <felipe.contreras@gmail.com>
>
> In the tests involving @{-1} and @{u} as the final component, what we
> really want to check is if it's pointing to the right ref.  We
> currently check the tip commit of the ref, but we can clarify this by
> separating out checking for commits versus checking for refs at
> check().
>
> [rr: commit message, fix arguments in check()]
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@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..cacb2d0 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:-$2}" "
> +       if [ '$2' == 'commit' ]; then
> +               echo '$3' >expect &&
> +               git log -1 --format=%s '$1' >actual
> +       else
> +               echo '${3:-$2}' >expect &&
> +               git rev-parse --symbolic-full-name '$1' >actual
> +       fi &&
>         test_cmp expect actual
>  "
>  }

I'm not sure about this. If we introduce a check that fails, we would have to do:

check HEAD refs/heads/new-branch "" failure

Which doesn't seem clean. Perhaps it makes more sense to always add the type of check:

check HEAD ref refs/heads/new-branch
-- 
Felipe Contreras
Ramkumar Ramachandra· May 2, 2013, 17:28 UTC · re: Felipe Contreras · lore

Re: [PATCH 2/9] t1508 (at-combinations): test branches separately

Felipe Contreras wrote:
Show 9 quoted lines
> I'm not sure about this. If we introduce a check that fails, we would
> have to do:
>
> check HEAD refs/heads/new-branch "" failure
>
> Which doesn't seem clean. Perhaps it makes more sense to always add
> the type of check:
>
> check HEAD ref refs/heads/new-branch
I think you misunderstood.  Failure looks like this:
    check "@@{u}" ref refs/heads/upstream-branch failure
And corresponding success like this:
    check "@@{u}" refs/heads/upstream-branch
We can make the "ref" compulsory if you like.  I thought about it too.
Felipe Contreras· May 2, 2013, 17:30 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 2/9] t1508 (at-combinations): test branches separately

On Thu, May 2, 2013 at 12:28 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 20 quoted lines
> Felipe Contreras wrote:
>> I'm not sure about this. If we introduce a check that fails, we would
>> have to do:
>>
>> check HEAD refs/heads/new-branch "" failure
>>
>> Which doesn't seem clean. Perhaps it makes more sense to always add
>> the type of check:
>>
>> check HEAD ref refs/heads/new-branch
>
> I think you misunderstood.  Failure looks like this:
>
>     check "@@{u}" ref refs/heads/upstream-branch failure
>
> And corresponding success like this:
>
>     check "@@{u}" refs/heads/upstream-branch
>
> We can make the "ref" compulsory if you like.  I thought about it too.
I think it's less surprising.
-- 
Felipe Contreras
Ramkumar Ramachandra· May 2, 2013, 13:39 UTC · re: Ramkumar Ramachandra · lore

[PATCH 3/9] t1508 (at-combinations): improve nonsense()

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

nonsense() is checking at the wrong abstraction layer: it doesn't need to invoke a 'git log' at all; it is sufficient to check that 'git rev-parse' can't resolve the argument.

[rr: commit message, minor modification]
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1508-at-combinations.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t1508-at-combinations.sh +1 −1
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index cacb2d0..55fdd0f 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -17,7 +17,7 @@ test_expect_${4:-success} "$1 = ${3:-$2}" "
 }
 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.40.g09a0447
Ramkumar Ramachandra· May 2, 2013, 13:39 UTC · re: Ramkumar Ramachandra · lore

[PATCH 4/9] t1508 (at-combinations): increase coverage

Add more tests exercising documented functionality.

It is worth nothing that HEAD@{-<n>} is senseless because @{-<n>} cannot be used with anything other than HEAD anyway.

[fc: contribute a couple of tests]
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1508-at-combinations.sh | 8 ++++++++
 1 file changed, 8 insertions(+)
Show changes to t/t1508-at-combinations.sh +8 −0
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 55fdd0f..eaa5337 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -42,13 +42,21 @@ test_expect_success 'setup' '
 
 check HEAD 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}" refs/heads/old-branch
+check "@{-1}@{0}" commit old-two
 check "@{-1}@{1}" commit old-one
 check "@{u}" refs/heads/upstream-branch
+check "HEAD@{u}" refs/heads/upstream-branch
 check "@{u}@{1}" commit upstream-one
 check "@{-1}@{u}" 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.40.g09a0447
Eric Sunshine· May 2, 2013, 14:26 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 4/9] t1508 (at-combinations): increase coverage

On Thu, May 2, 2013 at 9:39 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Add more tests exercising documented functionality.
>
> It is worth nothing that HEAD@{-<n>} is senseless because @{-<n>}
s/nothing/noting/
Show 5 quoted lines
> cannot be used with anything other than HEAD anyway.
>
> [fc: contribute a couple of tests]
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Ramkumar Ramachandra· May 2, 2013, 13:39 UTC · re: Ramkumar Ramachandra · lore

[PATCH 5/9] t1508 (at-combinations): document @{N} versus HEAD@{N}

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.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1508-at-combinations.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Show changes to t/t1508-at-combinations.sh +14 −0
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index eaa5337..29ffd73 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -59,4 +59,18 @@ nonsense "@{1}@{u}"
 nonsense "HEAD@{-1}"
 nonsense "@{-1}@{-1}"
 
+# Document differences between @{N} and HEAD@{N}.  The former resolves
+# HEAD to a branch and looks up the logs for that branch, while the
+# latter looks for the logs of HEAD.
+check "HEAD@{3}" commit old-two
+nonsense "@{3}"
+
+test_expect_success 'switch to old-branch' '
+	git checkout old-branch
+'
+
+check HEAD refs/heads/old-branch
+check "HEAD@{1}" commit new-two
+check "@{1}" commit old-one
+
 test_done
-- 
1.8.3.rc0.40.g09a0447
Felipe Contreras· May 2, 2013, 17:27 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 5/9] t1508 (at-combinations): document @{N} versus HEAD@{N}

On Thu, May 2, 2013 at 8:39 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 18 quoted lines
> 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.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t1508-at-combinations.sh | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
> index eaa5337..29ffd73 100755
> --- a/t/t1508-at-combinations.sh
> +++ b/t/t1508-at-combinations.sh
> @@ -59,4 +59,18 @@ nonsense "@{1}@{u}"
>  nonsense "HEAD@{-1}"
>  nonsense "@{-1}@{-1}"
>
> +# Document differences between @{N} and HEAD@{N}.

That's enough explanation, except that we not documenting them, we are testing them.

Show 5 quoted lines
> The former resolves
> +# HEAD to a branch and looks up the logs for that branch, while the
> +# latter looks for the logs of HEAD.
> +check "HEAD@{3}" commit old-two
> +nonsense "@{3}"
Show 7 quoted lines
> +test_expect_success 'switch to old-branch' '
> +       git checkout old-branch
> +'
> +
> +check HEAD refs/heads/old-branch
> +check "HEAD@{1}" commit new-two
> +check "@{1}" commit old-one
And here we are testing something completely different.
-- 
Felipe Contreras
Ramkumar Ramachandra· May 2, 2013, 17:30 UTC · re: Felipe Contreras · lore

Re: [PATCH 5/9] t1508 (at-combinations): document @{N} versus HEAD@{N}

Felipe Contreras wrote:
Show 9 quoted lines
>> +test_expect_success 'switch to old-branch' '
>> +       git checkout old-branch
>> +'
>> +
>> +check HEAD refs/heads/old-branch
>> +check "HEAD@{1}" commit new-two
>> +check "@{1}" commit old-one
>
> And here we are testing something completely different.
How?  I'm showing that HEAD@{1} can be different from @{1}.
Felipe Contreras· May 2, 2013, 17:34 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 5/9] t1508 (at-combinations): document @{N} versus HEAD@{N}

On Thu, May 2, 2013 at 12:30 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 12 quoted lines
> Felipe Contreras wrote:
>>> +test_expect_success 'switch to old-branch' '
>>> +       git checkout old-branch
>>> +'
>>> +
>>> +check HEAD refs/heads/old-branch
>>> +check "HEAD@{1}" commit new-two
>>> +check "@{1}" commit old-one
>>
>> And here we are testing something completely different.
>
> How?  I'm showing that HEAD@{1} can be different from @{1}.
Never mind, I thought you were testing detached HEAD checkouts.
-- 
Felipe Contreras
Ramkumar Ramachandra· May 2, 2013, 17:38 UTC · re: Felipe Contreras · lore

Re: [PATCH 5/9] t1508 (at-combinations): document @{N} versus HEAD@{N}

Felipe Contreras wrote:
>> +# Document differences between @{N} and HEAD@{N}.
>
> That's enough explanation, except that we not documenting them, we are
> testing them.

The difference is subtle, and I couldn't find this documented clearly anywhere else. If someone comes asking what the difference is, just point to this comment along with the tests.

Ramkumar Ramachandra· May 2, 2013, 13:39 UTC · re: Ramkumar Ramachandra · lore

[PATCH 6/9] t1508 (at-combinations): test with symbolic refs

Symbolic refs don't work with @{u}. Document this failure. Also create a symbolic-ref @ (which is a valid ref name), and test everything with it to make sure that there is no bug in the @-parser.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t1508-at-combinations.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)
Show changes to t/t1508-at-combinations.sh +13 −0
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 29ffd73..bb86c79 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -59,6 +59,19 @@ nonsense "@{1}@{u}"
 nonsense "HEAD@{-1}"
 nonsense "@{-1}@{-1}"
 
+# Symbolic refs
+git symbolic-ref H HEAD
+check "H@{1}" commit new-one
+check "H@{now}" commit new-two
+check "H@{u}" ref refs/heads/upstream-branch failure
+
+# To make sure that the @-parser isn't buggy, check things with the
+# symbolic-ref @
+git symbolic-ref @ HEAD
+check "@@{1}" commit new-one
+check "@@{now}" commit new-two
+check "@@{u}" ref refs/heads/upstream-branch failure
+
 # Document differences between @{N} and HEAD@{N}.  The former resolves
 # HEAD to a branch and looks up the logs for that branch, while the
 # latter looks for the logs of HEAD.
-- 
1.8.3.rc0.40.g09a0447
Ramkumar Ramachandra· May 2, 2013, 13:39 UTC · re: Ramkumar Ramachandra · lore

[PATCH 7/9] remote.c: teach branch_get() to treat symrefs other than HEAD

Broadly, there are two ways to resolve a command-line argument. The first approach is to treat it as a revision and resolve it using the revision-parsing mechanism get_sha1(). The other approach is to treat it as a ref, and use a reduced mechanism like branch_get().

Compare and contrast the implementations of @{N} and @{u} in sha1_name.c to see where exactly branch_get() is useful:

In <...>@{N}, the <...> is passed through dwim_log() which first calls substitute_branch_name() and then resolve_ref_unsafe() on that. The resolved ref could be any ref (symbolic/ non-symbolic), and not just a refs/heads/* ref (branch).

However, in the <...>@{u} case, the <...> can only be a ref/heads/* ref or a symbolic ref pointing to a refs/heads/* ref. It uses branch_get() to find the relevant ref.

Unfortunately, branch_get() doesn't call resolve_ref_unsafe() on what is passed to it at all. Instead it hard-codes the special four-letter word "HEAD" (which is resolved by read_config() which is called first), and rejects any other symbolic ref. This is a historical mistake because of which some callers of branch_get() suffer. To put it another way, the commands that accept "HEAD" (and resolve it into a branch) can benefit from accepting any other symbolic ref that resolves to a branch. For example, the following fail:

    $ git symbolic-ref M refs/heads/master
    $ git show M@{u}
    $ git branch -u ram/master M
This patch fixes branch_get() directly, making these work.

Notice that branch_get() calls read_config(), which in turn preemptively calls resolve_ref_unsafe() on "HEAD", when the caller might not even be requesting the current branch. After copying out two critical tasks that are required for make_remote() to work properly:

    git_config(handle_config, NULL);
    alias_all_urls();

we can remove read_config() and call resolve_ref_unsafe() on the argument that is passed ourselves. HEAD is therefore not a special case anymore.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 remote.c                   | 23 ++++++++++++++++++++---
 t/t1508-at-combinations.sh |  2 +-
 2 files changed, 21 insertions(+), 4 deletions(-)
Show changes to 2 files +21 −4

remote.c, t/t1508-at-combinations.sh

diff --git a/remote.c b/remote.c
index 68eb99b..3ac302f 100644
--- a/remote.c
+++ b/remote.c
@@ -1463,13 +1463,30 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 
 struct branch *branch_get(const char *name)
 {
-	struct branch *ret;
+	struct branch *ret = NULL;
+
+	/* Setup */
+	git_config(handle_config, NULL);
+	alias_all_urls();
 
-	read_config();
 	if (!name || !*name || !strcmp(name, "HEAD"))
-		ret = current_branch;
+		name = "HEAD";
 	else
 		ret = make_branch(name, 0);
+
+	if (!ret || !ret->remote_name) {
+		/* Is this a symbolic-ref like HEAD, pointing to a
+		 * valid branch?
+		 */
+		const char *this_ref;
+		unsigned char sha1[20];
+		int flag;
+
+		this_ref = resolve_ref_unsafe(name, sha1, 0, &flag);
+		if (this_ref && (flag & REF_ISSYMREF) &&
+			!prefixcmp(this_ref, "refs/heads/"))
+			ret = make_branch(this_ref + strlen("refs/heads/"), 0);
+	}
 	if (ret && ret->remote_name) {
 		ret->remote = remote_get(ret->remote_name);
 		if (ret->merge_nr) {
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index bb86c79..424caf5 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -63,7 +63,7 @@ nonsense "@{-1}@{-1}"
 git symbolic-ref H HEAD
 check "H@{1}" commit new-one
 check "H@{now}" commit new-two
-check "H@{u}" ref refs/heads/upstream-branch failure
+check "H@{u}" refs/heads/upstream-branch
 
 # To make sure that the @-parser isn't buggy, check things with the
 # symbolic-ref @
-- 
1.8.3.rc0.40.g09a0447
Ramkumar Ramachandra· May 2, 2013, 14:37 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 7/9] remote.c: teach branch_get() to treat symrefs other than HEAD

Ramkumar Ramachandra wrote:
> [...]

So sorry about this, but this breaks some tests in t1507 (rev-parse-upstream). I'm looking into this now.

In the meantime, reviewers can focus on the commit message.
Ramkumar Ramachandra· May 2, 2013, 15:43 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 7/9] remote.c: teach branch_get() to treat symrefs other than HEAD

Ramkumar Ramachandra wrote:
> So sorry about this, but this breaks some tests in t1507
> (rev-parse-upstream).  I'm looking into this now.
>
> In the meantime, reviewers can focus on the commit message.
So, it turns out that some callers expect it to read_config().

If we're still adamant about not touching branch_get(), the alternative is to call resolve_ref_unsafe() before the branch_get() call in interpret_branch_name(). We'll still retain the H@{u}, but we'll lose the 'git branch -u ram/master M'.

Felipe Contreras· May 2, 2013, 16:57 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 7/9] remote.c: teach branch_get() to treat symrefs other than HEAD

On Thu, May 2, 2013 at 8:39 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 5 quoted lines
>     $ git symbolic-ref M refs/heads/master
>     $ git show M@{u}
>     $ git branch -u ram/master M
>
> This patch fixes branch_get() directly, making these work.

It's not a fix, it's implementing brand new functionality, and changing old behaviors.

Show 10 quoted lines
> --- a/remote.c
> +++ b/remote.c
> @@ -1463,13 +1463,30 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>
>  struct branch *branch_get(const char *name)
>  {
> -       struct branch *ret;
> +       struct branch *ret = NULL;
> +
> +       /* Setup */
I thought the preferred style for these kinds of comments was:
  /* setup */
Show 5 quoted lines
> +       git_config(handle_config, NULL);
> +       alias_all_urls();
>
> -       read_config();
>         if (!name || !*name || !strcmp(name, "HEAD"))

I still see "HEAD" there. Didn't you say your patch makes "HEAD" less of a special ref? Why is it still here?

Show 9 quoted lines
> -               ret = current_branch;
> +               name = "HEAD";
>         else
>                 ret = make_branch(name, 0);
> +
> +       if (!ret || !ret->remote_name) {
> +               /* Is this a symbolic-ref like HEAD, pointing to a
> +                * valid branch?
> +                */
The style is:
  /*
   * This is the preferred style for multi-line
   * comments in the Linux kernel source code.
   * Please use it consistently.
   *
   * Description:  A column of asterisks on the left side,
   * with beginning and ending almost-blank lines.
   */
-- 
Felipe Contreras
Ramkumar Ramachandra· May 2, 2013, 13:39 UTC · re: Ramkumar Ramachandra · lore

[PATCH 8/9] sha1_name.c: fix @-parsing bug in interpret_branch_name()

Symbolic refs work in the general case, but making @ a symbolic-ref doesn't work due to a parsing bug in interpret_branch_name(). Update the function to parse "@{", not '@' (since '@' is a valid symbolic ref).

This makes a failing test in t1508 (at-combinations) pass. In other words, you can now do:

    $ git symbolic-ref @ HEAD
And expect the following to work:
    $ git rev-parse @@{u}
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sha1_name.c                | 2 +-
 t/t1508-at-combinations.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Show changes to 2 files +2 −2

sha1_name.c, t/t1508-at-combinations.sh

diff --git a/sha1_name.c b/sha1_name.c
index 3820f28..850e6d7 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1019,7 +1019,7 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 		return ret - used + len;
 	}
 
-	cp = strchr(name, '@');
+	cp = strstr(name, "@{");
 	if (!cp)
 		return -1;
 	tmp_len = upstream_mark(cp, namelen - (cp - name));
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 424caf5..6a69f9e 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -70,7 +70,7 @@ check "H@{u}" refs/heads/upstream-branch
 git symbolic-ref @ HEAD
 check "@@{1}" commit new-one
 check "@@{now}" commit new-two
-check "@@{u}" ref refs/heads/upstream-branch failure
+check "@@{u}" refs/heads/upstream-branch
 
 # Document differences between @{N} and HEAD@{N}.  The former resolves
 # HEAD to a branch and looks up the logs for that branch, while the
-- 
1.8.3.rc0.40.g09a0447
Ramkumar Ramachandra· May 2, 2013, 13:39 UTC · re: Ramkumar Ramachandra · lore

[PATCH 9/9] refs.c: make @ a pseudo-ref alias to HEAD

First, make sure that check_refname_format() rejects the a refname beginning with a '@'. Add a test to t1400 (update-ref) demonstrating that update-ref forbids the user from updating a ref named "@".

Now, resolve_ref_unsafe() is built to resolve any refs that have a corresponding file inside $GITDIR. Our "@" ref is a special pseudo-ref and does not have a filesystem counterpart. So, hard-interpret "@" as "HEAD" and resolve .git/HEAD as usual. This means that we can drop the 'git symbolic-ref @ HEAD' line in t1508 (at-combinations), and everything will continue working as usual.

If the user does manage to create a '.git/@' unsafely (via symbolic-ref or otherwise), it will be ignored.

In practice, this means that you will now be able to do:
    $ git show @~1
    $ git log @^2
Advertise these features in the tests and documentation.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-check-ref-format.txt |  2 ++
 Documentation/revisions.txt            |  8 ++++++--
 refs.c                                 | 12 ++++++++++--
 t/t1400-update-ref.sh                  |  3 +++
 t/t1508-at-combinations.sh             |  4 +---
 5 files changed, 22 insertions(+), 7 deletions(-)
Show changes to 5 files +22 −7

Documentation/git-check-ref-format.txt, Documentation/revisions.txt, refs.c, t/t1400-update-ref.sh, t/t1508-at-combinations.sh

diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
index ec1739a..3de9adc 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -52,6 +52,8 @@ Git imposes the following rules on how references are named:
 
 . They cannot end with a dot `.`.
 
+. They cannot be the single character `@`.
+
 . They cannot contain a sequence `@{`.
 
 . They cannot contain a `\`.
diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index d477b3f..02bfc90 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -27,6 +27,10 @@ blobs contained in a commit.
   When ambiguous, a '<refname>' is disambiguated by taking the
   first match in the following rules:
 
+  . '@' is a synonym for 'HEAD'.  An '@' followed by '\{' has no
+    relationship to this and means something entirely different (see
+    below).
+
   . If '$GIT_DIR/<refname>' exists, that is what you mean (this is usually
     useful only for 'HEAD', 'FETCH_HEAD', 'ORIG_HEAD', 'MERGE_HEAD'
     and 'CHERRY_PICK_HEAD');
@@ -93,7 +97,7 @@ some output processing may assume ref names in UTF-8.
   refers to the branch that the branch specified by branchname is set to build on
   top of.  A missing branchname defaults to the current one.
 
-'<rev>{caret}', e.g. 'HEAD{caret}, v1.5.1{caret}0'::
+'<rev>{caret}', e.g. '@{caret}2', 'HEAD{caret}', 'v1.5.1{caret}0'::
   A suffix '{caret}' to a revision parameter means the first parent of
   that commit object.  '{caret}<n>' means the <n>th parent (i.e.
   '<rev>{caret}'
@@ -101,7 +105,7 @@ some output processing may assume ref names in UTF-8.
   '<rev>{caret}0' means the commit itself and is used when '<rev>' is the
   object name of a tag object that refers to a commit object.
 
-'<rev>{tilde}<n>', e.g. 'master{tilde}3'::
+'<rev>{tilde}<n>', e.g. '@{tilde}1', 'master{tilde}3'::
   A suffix '{tilde}<n>' to a revision parameter means the commit
   object that is the <n>th generation ancestor of the named
   commit object, following only the first parents.  I.e. '<rev>{tilde}3' is
diff --git a/refs.c b/refs.c
index de2d8eb..6a75f77 100644
--- a/refs.c
+++ b/refs.c
@@ -72,6 +72,9 @@ int check_refname_format(const char *refname, int flags)
 {
 	int component_len, component_count = 0;
 
+	if (!strcmp(refname, "@"))
+		return -1;
+
 	while (1) {
 		/* We are at the start of a path component. */
 		component_len = check_refname_component(refname, flags);
@@ -1093,8 +1096,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 	if (flag)
 		*flag = 0;
 
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
-		return NULL;
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) < 0) {
+		/* Handle the pseudo-ref @ */
+		if (!strcmp(refname, "@"))
+			refname = "HEAD";
+		else
+			return NULL;
+	}
 
 	for (;;) {
 		char path[PATH_MAX];
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index e415ee0..ee93979 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -127,6 +127,9 @@ test_expect_success '(not) change HEAD with wrong SHA1' "
 test_expect_success "(not) changed .git/$m" "
 	! test $B"' = $(cat .git/'"$m"')
 '
+test_expect_success 'disallow creating a ref with name @' '
+	test_must_fail git update-ref @ HEAD
+'
 rm -f .git/$m
 
 : a repository with working tree always has reflog these days...
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index 6a69f9e..c50c3f7 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -65,9 +65,7 @@ check "H@{1}" commit new-one
 check "H@{now}" commit new-two
 check "H@{u}" refs/heads/upstream-branch
 
-# To make sure that the @-parser isn't buggy, check things with the
-# symbolic-ref @
-git symbolic-ref @ HEAD
+# Check everything with pseudo-ref @
 check "@@{1}" commit new-one
 check "@@{now}" commit new-two
 check "@@{u}" refs/heads/upstream-branch
-- 
1.8.3.rc0.40.g09a0447
Ramkumar Ramachandra· May 2, 2013, 17:18 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH v2 0/9] A natural solution to the @ -> HEAD problem

Ramkumar Ramachandra wrote:
Show 9 quoted lines
> Felipe Contreras (3):
>   t1508 (at-combinations): simplify setup
>   t1508 (at-combinations): test branches separately
>   t1508 (at-combinations): improve nonsense()
>
> Ramkumar Ramachandra (6):
>   t1508 (at-combinations): increase coverage
>   t1508 (at-combinations): document @{N} versus HEAD@{N}
>   t1508 (at-combinations): test with symbolic refs
Can we just okay these six now, and drop the rest?
Thanks.
Felipe Contreras· May 2, 2013, 17:25 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH v2 0/9] A natural solution to the @ -> HEAD problem

On Thu, May 2, 2013 at 12:18 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 12 quoted lines
> Ramkumar Ramachandra wrote:
>> Felipe Contreras (3):
>>   t1508 (at-combinations): simplify setup
>>   t1508 (at-combinations): test branches separately
>>   t1508 (at-combinations): improve nonsense()
>>
>> Ramkumar Ramachandra (6):
>>   t1508 (at-combinations): increase coverage
>>   t1508 (at-combinations): document @{N} versus HEAD@{N}
>>   t1508 (at-combinations): test with symbolic refs
>
> Can we just okay these six now, and drop the rest?

I'm generally OK with them, but the one for 'symbolic refs' is relying on a symbolic ref named '@', so it would immediately conflict with my patch series.

-- 
Felipe Contreras
Ramkumar Ramachandra· May 2, 2013, 17:34 UTC · re: Felipe Contreras · lore

Re: [PATCH v2 0/9] A natural solution to the @ -> HEAD problem

Felipe Contreras wrote:
> I'm generally OK with them, but the one for 'symbolic refs' is relying
> on a symbolic ref named '@', so it would immediately conflict with my
> patch series.
Right.  I'll re-roll when we've finished with the rest.

← back to recent threads