Re: [PATCH] remote.c: use shorten_unambiguous_ref

[Jeff King <peff@peff.net>]

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