Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin

Johan Herland [off-list ref] writes:

On Mon, May 6, 2013 at 7:52 PM, Junio C Hamano [off-list ref] wrote:

quoted

Johan Herland [off-list ref] writes:

quoted

... there is AFAICS _no_ way for sscanf() - having
already done one or more format extractions - to indicate to its caller
that the input fails to match the trailing part of the format string.

Yeah, we can detect when we did not have enough, but we cannot tell
where it stopped matching.

It is interesting that this bug has stayed so long with us, which
may indicate that nobody actually uses the feature at all.

I don't know if people really care about whether
"refs/remotes/origin/HEAD" shortens to "origin/HEAD" or "origin". I'm
guessing that people _do_ depend on the reverse - having "origin"
expand into "refs/remotes/origin/HEAD", so we probably cannot rip out
the "refs/remotes/%.*s/HEAD" rule altogether...

Oh, no doubt about that reverse conversion.

The real reason nobody cared about refs/remotes/origin/HEAD is that
nobody sane has anything but non-symbolic ref there.  Your t1514
does this:

	...
	git update-ref refs/master master_d &&
	test_commit master_e
	git update-ref refs/remotes/origin/HEAD master_e &&
	...

Nowhere in the set-up sequence, you see anything that does

	git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/master

or any other branch we copied from the remote.

And the shortening is done after dereferencing the synbolic ref.
Because of this, refs/remotes/origin/HEAD usually resolves to
origin/master, not origin.

 t/t1514-rev-parse-shorten-unambiguous-ref.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t1514-rev-parse-shorten-unambiguous-ref.sh b/t/t1514-rev-parse-shorten-unambiguous-ref.sh
index fd87ce3..556ad16 100755
--- a/t/t1514-rev-parse-shorten-unambiguous-ref.sh
+++ b/t/t1514-rev-parse-shorten-unambiguous-ref.sh

@@ -76,4 +76,11 @@ test_expect_success 'shortening refnames in loose mode' '
 	test_shortname refs/tags/master loose tags/master master_c
 '
 
+test_expect_success 'shortening is done after dereferencing a symref' '
+	git update-ref refs/remotes/frotz/master master_e &&
+	git symbolic-ref refs/remotes/frotz/HEAD refs/remotes/frotz/master &&
+	test_shortname refs/remotes/frotz/HEAD strict frotz/master master_e &&
+	test_shortname refs/remotes/frotz/HEAD loose frotz/master master_e
+'
+
 test_done

On Tue, May 7, 2013 at 11:31 PM, Junio C Hamano [off-list ref] wrote:

Johan Herland [off-list ref] writes:

quoted

On Mon, May 6, 2013 at 7:52 PM, Junio C Hamano [off-list ref] wrote:

quoted

It is interesting that this bug has stayed so long with us, which
may indicate that nobody actually uses the feature at all.

I don't know if people really care about whether
"refs/remotes/origin/HEAD" shortens to "origin/HEAD" or "origin". I'm
guessing that people _do_ depend on the reverse - having "origin"
expand into "refs/remotes/origin/HEAD", so we probably cannot rip out
the "refs/remotes/%.*s/HEAD" rule altogether...

Oh, no doubt about that reverse conversion.

The real reason nobody cared about refs/remotes/origin/HEAD is that
nobody sane has anything but non-symbolic ref there.  Your t1514
does this:

        ...
        git update-ref refs/master master_d &&
        test_commit master_e

...oops, I see I forgot the trailing && on this line. Do you want a
resend, or fix up yourself?

        git update-ref refs/remotes/origin/HEAD master_e &&
        ...

Nowhere in the set-up sequence, you see anything that does

        git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/master

or any other branch we copied from the remote.

Correct. I first did a "git remote set-head origin master", but
quickly discovered that rev-parse resolved the symref as part of
--abbrev-ref, so I had to fake up a non-symref to trigger the
shortening logic I wanted to test.

True. I'm not sure whether that's a feature or a bug in --abbrev-ref,
probably a feature.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net