[PATCH] remote.c: use shorten_unambiguous_ref

[PATCH] remote.c: use shorten_unambiguous_ref

[Michael J Gruber]

Use the new shorten_unambiguous_ref() for simplifying the output of
upstream branch names. This affects status and checkout.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
I intentionally didn't combine the line with the previous one (see
context) to make things a bit clearer.

I haven't seen more obvious place for using shorten_unambiguous_ref().
prettify_ref() is a natural candidate but is mostly used for
prettyfying refs on the remote side. git branch is covered by Jeff's
patch already.

 remote.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index a06761a..10af722 100644
--- a/remote.c
+++ b/remote.c
@@ -1461,11 +1461,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 		return 0;
 
 	base = branch->merge[0]->dst;
-	if (!prefixcmp(base, "refs/remotes/")) {
-		base += strlen("refs/remotes/");
-	} else if (!prefixcmp(base, "refs/heads/")) {
-		base += strlen("refs/heads/");
-	}
+	base = shorten_unambiguous_ref(base);
 	if (!num_theirs)
 		strbuf_addf(sb, "Your branch is ahead of '%s' "
 			    "by %d commit%s.\n",
-- 
1.6.2.2.646.gb214

Re: [PATCH] remote.c: use shorten_unambiguous_ref

[Jeff King]

On Thu, Apr 09, 2009 at 05:33:02PM +0200, Michael J Gruber wrote:

> Use the new shorten_unambiguous_ref() for simplifying the output of
> upstream branch names. This affects status and checkout.

Yeah, this is the spot I was thinking about when I mentioned "use it in
other places" earlier in the thread. So

Acked-by: Jeff King <peff@peff.net>

> I haven't seen more obvious place for using shorten_unambiguous_ref().
> prettify_ref() is a natural candidate but is mostly used for
> prettyfying refs on the remote side. git branch is covered by Jeff's
> patch already.

Hmm. I was thinking we might be able to just do away with prettify_ref,
but I didn't consider the fact that we need to prettify remote things. I
think you could still unambiguously prettify the local half of those
callsites, though.

Given that the two functions are closely related, should we perhaps
rename them to

  const char *shorten_ref(const char *);
  const char *shorten_ref_unambiguous(const char *);

? The implementations are quite different, with prettify_ref not really
respecting the ref lookup rules, but rather just considering a few
pre-determined bits of the hierarchy as uninteresting. It shouldn't be
that hard to have them both use the same implementation, like:

  const char *shorten_ref(const char *, int unambiguous);

-Peff

Re: [PATCH] remote.c: use shorten_unambiguous_ref

[Michael J Gruber]

Jeff King venit, vidit, dixit 10.04.2009 19:14:
> On Thu, Apr 09, 2009 at 05:33:02PM +0200, Michael J Gruber wrote:
> 
>> Use the new shorten_unambiguous_ref() for simplifying the output of
>> upstream branch names. This affects status and checkout.
> 
> Yeah, this is the spot I was thinking about when I mentioned "use it in
> other places" earlier in the thread. So
> 
> Acked-by: Jeff King <peff@peff.net>
> 
>> I haven't seen more obvious place for using shorten_unambiguous_ref().
>> prettify_ref() is a natural candidate but is mostly used for
>> prettyfying refs on the remote side. git branch is covered by Jeff's
>> patch already.
> 
> Hmm. I was thinking we might be able to just do away with prettify_ref,
> but I didn't consider the fact that we need to prettify remote things. I
> think you could still unambiguously prettify the local half of those
> callsites, though.
> 
> Given that the two functions are closely related, should we perhaps
> rename them to
> 
>   const char *shorten_ref(const char *);
>   const char *shorten_ref_unambiguous(const char *);
> 
> ? The implementations are quite different, with prettify_ref not really
> respecting the ref lookup rules, but rather just considering a few
> pre-determined bits of the hierarchy as uninteresting. It shouldn't be
> that hard to have them both use the same implementation, like:
> 
>   const char *shorten_ref(const char *, int unambiguous);
> 
> -Peff

Should I rebase this on top of Bert's newer patch (which has the
signature you suggest)? Currently I don't see any of them in.

Michael

Re: [PATCH] remote.c: use shorten_unambiguous_ref

[Jeff King]

On Tue, Apr 14, 2009 at 02:55:14PM +0200, Michael J Gruber wrote:

> >   const char *shorten_ref(const char *);
> >   const char *shorten_ref_unambiguous(const char *);
> > 
> > ? The implementations are quite different, with prettify_ref not really
> > respecting the ref lookup rules, but rather just considering a few
> > pre-determined bits of the hierarchy as uninteresting. It shouldn't be
> > that hard to have them both use the same implementation, like:
> > 
> >   const char *shorten_ref(const char *, int unambiguous);
> 
> Should I rebase this on top of Bert's newer patch (which has the
> signature you suggest)? Currently I don't see any of them in.

It has the same signature, but the meanings are different. My flag meant
"should we bother looking at ambiguity at all?" whereas Bert's flag is
about "how strictly should we be counting ambiguity?".

So really there are three levels, which implies an enum (with none,
loose, and strict). However, calling it with a tri-state flag would be
less convenient, since your logic for the flag is:

  if (we are shortening something non-local)
    flag = none;
  else if (core.warn_ambiguous_refs is not set)
    flag = loose;
  else
    flag = strict;

So you really want to pass the two bits of information separately.

-Peff

Re: [PATCH] remote.c: use shorten_unambiguous_ref

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hmm. I was thinking we might be able to just do away with prettify_ref,
> but I didn't consider the fact that we need to prettify remote things. I
> think you could still unambiguously prettify the local half of those
> callsites, though.
>
> Given that the two functions are closely related, should we perhaps
> rename them to
>
>   const char *shorten_ref(const char *);
>   const char *shorten_ref_unambiguous(const char *);
>
> ? The implementations are quite different, with prettify_ref not really
> respecting the ref lookup rules, but rather just considering a few
> pre-determined bits of the hierarchy as uninteresting. It shouldn't be
> that hard to have them both use the same implementation, like:
>
>   const char *shorten_ref(const char *, int unambiguous);

I was hoping that a single "shorten" function that does not even take
"unambiguous" parameter would be used by almost everybody.  As far as I
can see, Bert's "rev-parse --abbrev-ref" RFC is the only caller that might
need to use a value different from warn_ambiguous_refs, and all the other
existing callers (including fill_tracking_info() for "upstream" report by
git-branch) do not have to pass "0" but can use the default.  IOW, we can
have:

	const char *shorten_ref_unambiguous(const char *ref, int strict);
	const char *shorten_ref(const char *ref)
        {
        	return shorten_ref_unambiguous(ref, warn_ambiguous_refs);
	}

and only specialized callers that really care use shorten_ref_unambiguous
(without Bert's [PATCH-RFC 3/2] we do not have any such specialized
caller, I think).

But I am not sure how well prettify_ref() fits into this picture.  It is
called only from transport and is meant to deal with refs that exist on
the remote side, so ambiguity check against our local namespace would not
make much sense.  We could:

	const char *shorten_ref_internal(const char *ref, int mode);
	const char *shorten_ref(const char *ref)
        {
		unsigned mode = warn_ambiguous_refs ? SHORTEN_STRICT : 0;
        	return shorten_ref_internal(ref, mode);
	}
	const char *prettify_ref(const char *ref)
        {
        	return shorten_ref_internal(ref, SHORTEN_PREFIX_ONLY);
        }

and have the SHORTEN_PREFIX_ONLY logic inherit from what the current
prettify_ref() does, but at that point it I do not think it makes sense
anymore.

Re: [PATCH] remote.c: use shorten_unambiguous_ref

[Bert Wesarg]

On Tue, Apr 14, 2009 at 18:55, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> Hmm. I was thinking we might be able to just do away with prettify_ref,
>> but I didn't consider the fact that we need to prettify remote things. I
>> think you could still unambiguously prettify the local half of those
>> callsites, though.
>>
>> Given that the two functions are closely related, should we perhaps
>> rename them to
>>
>>   const char *shorten_ref(const char *);
>>   const char *shorten_ref_unambiguous(const char *);
>>
>> ? The implementations are quite different, with prettify_ref not really
>> respecting the ref lookup rules, but rather just considering a few
>> pre-determined bits of the hierarchy as uninteresting. It shouldn't be
>> that hard to have them both use the same implementation, like:
>>
>>   const char *shorten_ref(const char *, int unambiguous);
>
> I was hoping that a single "shorten" function that does not even take
> "unambiguous" parameter would be used by almost everybody.  As far as I
> can see, Bert's "rev-parse --abbrev-ref" RFC is the only caller that might
> need to use a value different from warn_ambiguous_refs, and all the other
> existing callers (including fill_tracking_info() for "upstream" report by
> git-branch) do not have to pass "0" but can use the default.  IOW, we can
> have:
>
>        const char *shorten_ref_unambiguous(const char *ref, int strict);
>        const char *shorten_ref(const char *ref)
>        {
>                return shorten_ref_unambiguous(ref, warn_ambiguous_refs);
>        }
There's too much confusion: Whatever input you gave to
shorten_unambiguous_ref(), if you pass the result to dwim_ref() you
get the input again. That is, all return values are unambiguous. The
only small different (and here comes a little word-confusion) is, that
in strict mode you wont get any warning for 'ambiguous' refs. So the
names "shorten_ref_unambiguous" and "shorten_rer" are misleading,
because it looks like the former returns unambiguous refs and the
latter not. Which is wrong.

I can't think about this further, no time sorry.

Bert

Re: [PATCH] remote.c: use shorten_unambiguous_ref

[Jeff King]

On Tue, Apr 14, 2009 at 09:55:08AM -0700, Junio C Hamano wrote:

> I was hoping that a single "shorten" function that does not even take
> "unambiguous" parameter would be used by almost everybody.  As far as I
> can see, Bert's "rev-parse --abbrev-ref" RFC is the only caller that might
> need to use a value different from warn_ambiguous_refs, and all the other
> existing callers (including fill_tracking_info() for "upstream" report by
> git-branch) do not have to pass "0" but can use the default.  IOW, we can
> have:
> 
> 	const char *shorten_ref_unambiguous(const char *ref, int strict);
> 	const char *shorten_ref(const char *ref)
>         {
>         	return shorten_ref_unambiguous(ref, warn_ambiguous_refs);
> 	}
> 
> and only specialized callers that really care use shorten_ref_unambiguous
> (without Bert's [PATCH-RFC 3/2] we do not have any such specialized
> caller, I think).

I think that is a sensible approach; I also thought when reading Bert's
patch that the parameter seemed like it would not be used in most
situations.

> But I am not sure how well prettify_ref() fits into this picture.  It is
> called only from transport and is meant to deal with refs that exist on
> the remote side, so ambiguity check against our local namespace would not
> make much sense.  We could:
> 
> 	const char *shorten_ref_internal(const char *ref, int mode);
> 	const char *shorten_ref(const char *ref)
>         {
> 		unsigned mode = warn_ambiguous_refs ? SHORTEN_STRICT : 0;
>         	return shorten_ref_internal(ref, mode);
> 	}
> 	const char *prettify_ref(const char *ref)
>         {
>         	return shorten_ref_internal(ref, SHORTEN_PREFIX_ONLY);
>         }
> 
> and have the SHORTEN_PREFIX_ONLY logic inherit from what the current
> prettify_ref() does, but at that point it I do not think it makes sense
> anymore.

There are three things wrong with prettify_ref:

  1. It takes a ref struct instead of a string with a refname (but only
     looks at ref->name). This is easily fixed.

  2. It does the same thing as shorten_ref_unambiguous, but without any
     ambiguity check, so the names should be related. That is easily
     changed, too, once we settle on the name (either it is shorten_ref
     to the other's _unambiguous form, or the unambiguous one becomes
     shorten_ref, and this becomes shorten_ref_remote or something).

  3. It uses its own "skip these random things rules" instead of being
     based on the usual ref lookup rules. I think this can be folded
     into the unambiguous case by simply bailing on the first textual
     match.  I don't know in practice if it matters that much.

-Peff