[PATCH] show-ref: Allow --head to work with --verify

[PATCH] show-ref: Allow --head to work with --verify

[Vladimir Panteleev]

Previously, when --verify was specified, --head was being ignored, and
"show-ref --verify --head HEAD" would always print "fatal: 'HEAD' -
not a valid ref". As such, when using show-ref to look up any
ref (including HEAD) precisely by name, one would have to special-case
looking up the HEAD ref.

This patch adds --head support to show-ref's --verify logic, by
explicitly checking if the "HEAD" ref is specified when --head is
present.

Signed-off-by: Vladimir Panteleev <git@thecybershadow.net>
---
 builtin/show-ref.c  | 2 ++
 t/t1403-show-ref.sh | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 6d4e66900..ee5078604 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -207,6 +207,8 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
 				if (!quiet)
 					show_one(*pattern, &oid);
 			}
+			else if (show_head && !strcmp(*pattern, "HEAD"))
+				head_ref(show_ref, NULL);
 			else if (!quiet)
 				die("'%s' - not a valid ref", *pattern);
 			else
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 7e10bcfe3..de64ebfb7 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -164,4 +164,12 @@ test_expect_success 'show-ref --heads, --tags, --head, pattern' '
 	test_cmp expect actual
 '
 
+test_expect_success 'show-ref --verify --head' '
+	{
+		echo $(git rev-parse HEAD) HEAD
+	} >expect &&
+	git show-ref --verify --head HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.11.0

Re: [PATCH] show-ref: Allow --head to work with --verify

[Junio C Hamano]

Vladimir Panteleev <git@thecybershadow.net> writes:

> This patch adds --head support to show-ref's --verify logic, by
> explicitly checking if the "HEAD" ref is specified when --head is
> present.

> @@ -207,6 +207,8 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
>  				if (!quiet)
>  					show_one(*pattern, &oid);
>  			}
> +			else if (show_head && !strcmp(*pattern, "HEAD"))
> +				head_ref(show_ref, NULL);
>  			else if (!quiet)
>  				die("'%s' - not a valid ref", *pattern);
>  			else

The context around here look like this:

		while (*pattern) {
			struct object_id oid;

			if (starts_with(*pattern, "refs/") &&
			    !read_ref(*pattern, oid.hash)) {
				if (!quiet)
					show_one(*pattern, &oid);
			}
			else if (!quiet)
				die("'%s' - not a valid ref", *pattern);
			else
				return 1;
			pattern++;
		}

and viewed in the wider context, I notice that quiet is not honored
in the added code.  I think that is easily fixable by replacing this
hunk with something like:

-	if (starts_with(*pattern, "refs/") &&
+	if (to_show_ref(*pattern) &&

and then another hunk that implements to_show_ref() helper function,
perhaps like

	static int to_show_ref(const char *pattern)
	{
		if (starts_with(pattern, "refs/"))
			return 1;
		if (show_head && !strcmp(pattern, "HEAD"))
			return 1;
		return 0;
	}

or something.

Having said all that, using --verify on HEAD does not make much
sense, because if HEAD is missing in .git/, I do not think Git
considers that directory as a Git repository to begin with.  So from
that point of view, I am not sure what value this change adds to the
system, even though the change is almost correct (modulo the "quiet"
thing).

Re: [PATCH] show-ref: Allow --head to work with --verify

[Vladimir Panteleev]

Hi Junio,

On 2017-01-20 19:03, Junio C Hamano wrote:
> Having said all that, using --verify on HEAD does not make much
> sense, because if HEAD is missing in .git/, I do not think Git
> considers that directory as a Git repository to begin with.  So from
> that point of view, I am not sure what value this change adds to the
> system, even though the change is almost correct (modulo the "quiet"
> thing).

My use case was the following series of steps:

Q1: How do I resolve a full ref name to a commit SHA1?
A1: Use show-ref <full-ref-name>.

Q2: How to make git show-ref also work when HEAD is specified as the 
reference?
A2: Add --head.

Q3: How do I make git show-ref only look for the exact full ref name 
specified, instead of doing a pattern/substring search, and thus output 
at most one result?
A3: Add --verify.

However, A2 and A3 are incompatible. Thus, there doesn't seem to be a 
way to e.g. make a simple alias which looks up a ref only by its full 
ref name, where said ref might or might not be HEAD. The obvious 
workaround is to check if the ref is HEAD in the rev-parse caller, 
however it seemed more logical to fix it in git instead.

The documentation for show-ref also makes no mention that --head is 
ignored if --verify is specified, and the combination was not covered by 
any tests, therefore this seemed to me as a simple omission in 
--verify's logic.

There is also rev-parse, which also has a --verify switch, however it 
does something else, and I don't see a way to ask rev-parse to only 
consider full refs.

-- 
Best regards,
  Vladimir

Re: [PATCH] show-ref: Allow --head to work with --verify

[Vladimir Panteleev]

And to clarify:

On 2017-01-20 20:26, Vladimir Panteleev wrote:
> On 2017-01-20 19:03, Junio C Hamano wrote:
>> Having said all that, using --verify on HEAD does not make much
>> sense, because if HEAD is missing in .git/, I do not think Git
>> considers that directory as a Git repository to begin with.

The behavior of --verify I am interested in is not to check that the ref 
exists, but to get its SHA1 while avoiding the pattern search. This 
avoids accidentally matching refs via substring (e.g. "git show-ref 
--head HEAD" will print HEAD, but also e.g. refs/remotes/origin/HEAD), 
and for performance reasons (looking up a ref by exact name can be MUCH 
faster than matching all refs by pattern search, e.g. in one of my 
projects where I use git as an object store, --verify makes a difference 
of 21 milliseconds vs. over 5 minutes).

-- 
Best regards,
  Vladimir

Re: [PATCH] show-ref: Allow --head to work with --verify

[Junio C Hamano]

Vladimir Panteleev <thecybershadow@gmail.com> writes:

> Hi Junio,
>
> On 2017-01-20 19:03, Junio C Hamano wrote:
>> Having said all that, using --verify on HEAD does not make much
>> sense, because if HEAD is missing in .git/, I do not think Git
>> considers that directory as a Git repository to begin with.  So from
>> that point of view, I am not sure what value this change adds to the
>> system, even though the change is almost correct (modulo the "quiet"
>> thing).
>
> My use case was the following series of steps:
>
> Q1: How do I resolve a full ref name to a commit SHA1?
> A1: Use show-ref <full-ref-name>.
>
> Q2: How to make git show-ref also work when HEAD is specified as the
> reference?
> A2: Add --head.
>
> Q3: How do I make git show-ref only look for the exact full ref name
> specified, instead of doing a pattern/substring search, and thus
> output at most one result?
> A3: Add --verify.
>
> However, A2 and A3 are incompatible. Thus, there doesn't seem to be a
> way to e.g. make a simple alias which looks up a ref only by its full
> ref name, where said ref might or might not be HEAD. The obvious
> workaround is to check if the ref is HEAD in the rev-parse caller,
> however it seemed more logical to fix it in git instead.
>
> The documentation for show-ref also makes no mention that --head is
> ignored if --verify is specified, and the combination was not covered
> by any tests, therefore this seemed to me as a simple omission in
> --verify's logic.
>
> There is also rev-parse, which also has a --verify switch, however it
> does something else, and I don't see a way to ask rev-parse to only
> consider full refs.

Your log message for the patch needs to be updated by summarizing
the above better.  I couldn't read the motivation behind the change
fully from the original (even though I guessed it correctly).

Re: [PATCH] show-ref: Allow --head to work with --verify

[Junio C Hamano]

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

>> My use case was the following series of steps:
>> ... long and readable if a bit too verbose description ...
> Your log message for the patch needs to be updated by summarizing
> the above better.

That raises the number of things to improve in the patch to 3 (so
far):

 * the log message clarification.

 * the code change I mentioned already to hook into the existing
   "only read the fully specified ref" part, sharing the actual
   action (i.e. read_ref() to read it, honor quiet and show it by
   calling show_one()), instead of adding another clause that does
   it differently (i.e. you didn't do read_ref/quiet?/show_one)

 * add a test to make sure that the command also works sensibly when
   --quiet is used.

With all of the above, I think the change makes sense.

Re: [PATCH] show-ref: Allow --head to work with --verify

[Junio C Hamano]

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

> Your log message for the patch needs to be updated by summarizing
> the above better.

Here is my attempt.

     "git show-ref HEAD" used with "--verify" (because the user is
     not interested in seeing refs/remotes/origin/HEAD), and used
     with "--head" (because the user does not want HEAD to be
     filtered out), i.e. "git show-ref --head --verify HEAD", did
     not work as expected.

     Instead of insisting that the input begins with "refs/", allow
     "HEAD" when "--head" is given in the codepath that handles
     "--verify", so that all valid full refnames including HEAD are
     passed to the same output machinery.

Re: [PATCH] show-ref: Allow --head to work with --verify

[Vladimir Panteleev]

On 2017-01-20 19:03, Junio C Hamano wrote:
> and viewed in the wider context, I notice that quiet is not honored
> in the added code.  I think that is easily fixable by replacing this
> hunk with something like:

--quiet will still work correctly with the current patch, because 
show_ref already checks quiet. Granted, the original --verify code used 
show_one and not show_ref; however, I don't see a meaningful difference 
between calling show_ref and show_one for HEAD, other than a bit of 
overhead, so adding a new function may not be worthwhile. I will still 
add tests for this; however, in light of this, would you still like me 
to perform the change you requested?

-- 
Best regards,
  Vladimir

Re: [PATCH] show-ref: Allow --head to work with --verify

[Junio C Hamano]

Vladimir Panteleev <thecybershadow@gmail.com> writes:

> --quiet will still work correctly with the current patch, because
> show_ref already checks quiet. Granted, the original --verify code
> used show_one and not show_ref; however, I don't see a meaningful
> difference between calling show_ref and show_one for HEAD, other than
> a bit of overhead, so adding a new function may not be worthwhile. I
> will still add tests for this; however, in light of this, would you
> still like me to perform the change you requested?

If two codepaths are called "I don't see a meaningful difference",
then it is really better to share the same code.  Today, they may
happen to behave identically.  When we need to update the behaviour
of one, we'd be forced to update the other one to match.

IOW, something along this line, perhaps (not even compile tested so
take it with grain of salt).

Thanks.

 builtin/show-ref.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 6d4e669002..57491152b7 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -202,7 +202,8 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
 		while (*pattern) {
 			struct object_id oid;
 
-			if (starts_with(*pattern, "refs/") &&
+			if (((show_head && !strcmp(*pattern, "HEAD")) ||
+			     starts_with(*pattern, "refs/")) &&
 			    !read_ref(*pattern, oid.hash)) {
 				if (!quiet)
 					show_one(*pattern, &oid);

Re: [PATCH] show-ref: Allow --head to work with --verify

[Junio C Hamano]

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

> If two codepaths are called "I don't see a meaningful difference",
> then it is really better to share the same code.  Today, they may
> happen to behave identically.  When we need to update the behaviour
> of one, we'd be forced to update the other one to match.
>
> IOW, something along this line, perhaps (not even compile tested so
> take it with grain of salt).

By the way, I have no strong preference between "read-ref, check
quiet and show-one" and "show-ref", so if you make --verify to
consistently call "show_ref()" for both refs/heads/master and HEAD,
that is also perfectly fine.

I just do not want to see the same feature/codepath to call two
different implementations that happens to do the same thing today.

Thanks.

Re: [PATCH] show-ref: Allow --head to work with --verify

[Vladimir Panteleev]

On 2017-01-20 23:29, Junio C Hamano wrote:
> By the way, I have no strong preference between "read-ref, check
> quiet and show-one" and "show-ref", so if you make --verify to
> consistently call "show_ref()" for both refs/heads/master and HEAD,
> that is also perfectly fine.

This sounds like a good idea, as it will also allow -d and some other 
options to work with --verify (whatever use case there might be for 
that). Will resubmit with that.

> I just do not want to see the same feature/codepath to call two
> different implementations that happens to do the same thing today.

Understood and agreed.

-- 
Best regards,
  Vladimir