public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Vladimir Panteleev <git@thecybershadow•net>
Cc: git@vger•kernel.org
Subject: Re: [PATCH v2 3/4] show-ref: Optimize show_ref a bit
Date: Sun, 22 Jan 2017 14:47:47 -0800	[thread overview]
Message-ID: <xmqqa8aivhik.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170121010821.25046-4-git@thecybershadow.net> (Vladimir Panteleev's message of "Sat, 21 Jan 2017 01:08:20 +0000")

Vladimir Panteleev <git@thecybershadow•net> writes:

> The inner `if (verify)' check was not being used before the preceding
> commit, as show_ref was never being called from a code path where
> verify was non-zero.
>
> Adding a `!verify' check to the outer if skips an unnecessary string
> comparison when verify is non-zero, and show_ref is already called
> with a reference exactly matching pattern.
>
> Signed-off-by: Vladimir Panteleev <git@thecybershadow•net>
> ---
>  builtin/show-ref.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> index bcdc1a95e..3cf344d47 100644
> --- a/builtin/show-ref.c
> +++ b/builtin/show-ref.c
> @@ -43,7 +43,7 @@ static int show_ref(const char *refname, const struct object_id *oid,
>  		if (!match)
>  			return 0;
>  	}
> -	if (pattern) {
> +	if (pattern && !verify) {
>  		int reflen = strlen(refname);
>  		const char **p = pattern, *m;
>  		while ((m = *p++) != NULL) {
> @@ -54,9 +54,6 @@ static int show_ref(const char *refname, const struct object_id *oid,
>  				continue;
>  			if (len == reflen)
>  				goto match;
> -			/* "--verify" requires an exact match */
> -			if (verify)
> -				continue;
>  			if (refname[reflen - len - 1] == '/')
>  				goto match;
>  		}

Having to do this change probably is an indication that the division
of labour between show_ref() and show_one() up to this step needs to
be rethought.

Conceptually, "git show-ref" works in two ways:

 * When in --verify mode, the end user gives which refnames to
   consider showing.

 * Otherwise the end user gives pattern and the command infers which
   refnames to consider showing using the pattern.

And for the refnames that are considered for showing, we may do
various things, like -d to deref and --quiet to be silent.  We want
this actual "output" step to be the same between two modes of
operation.

So a better division of labour would be:

 * Make show_ref() about "using pattern, enumerate what refs to
   show" and call show_one().

 * Update show_one() and teach _it_ to handle quiet and deref_tags.

 * From cmd_show_ref(), in --verify mode, make sure the ref exists
   and call show_one(), because we do not do the "using pattern,
   enumerate what refs to show" at all.

And from that point of view, I think 4/4 is going in the wrong
direction.

I also think that "git show-ref --verify HEAD" should work; it is OK
that the command accepts "--head" in that case, but when in --verify
mode, the end user gives which refnames to consider showing, and
HEAD given from the command line is a signal enough that that
psuedo-ref is what the end user wants to be shown.  "--head" is
about not filtering in "enumerate the ones that match the given
patterns" mode, and it feels unnecessary to require it in "--verify"
mode.

Thanks.

next prev parent reply	other threads:[~2017-01-22 22:47 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-21  1:08 [PATCH v2] show-ref: Allow -d, --head to work with --verify Vladimir Panteleev
2017-01-21  1:08 ` [PATCH v2 1/4] show-ref: Allow " Vladimir Panteleev
2017-01-21  1:08 ` [PATCH v2 2/4] show-ref: Allow -d " Vladimir Panteleev
2017-01-21  1:08 ` [PATCH v2 3/4] show-ref: Optimize show_ref a bit Vladimir Panteleev
2017-01-22 22:47   ` Junio C Hamano [this message]
2017-01-22 22:55     ` Junio C Hamano
2017-01-21  1:08 ` [PATCH v2 4/4] show-ref: Inline show_one Vladimir Panteleev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqa8aivhik.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=git@thecybershadow$(echo .)net \
    --cc=git@vger$(echo .)kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox