Re: [PATCH] remote.c: use shorten_unambiguous_ref
On Tue, Apr 14, 2009 at 09:55:08AM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> 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.
Show 19 quoted lines
> 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
Michael J Gruber
