{"thread":{"id":"18805","subject":"[PATCH] remote.c: use shorten_unambiguous_ref","startedAt":"2009-04-09T15:33:02Z","lastAt":"2009-04-15T08:12:46Z","messageCount":7,"participants":["Michael J Gruber","Jeff King","Junio C Hamano","Bert Wesarg"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"110923","messageId":"1239291182-12860-1-git-send-email-git@drmicha.warpmail.net","threadId":"18805","inReplyTo":null,"subject":"[PATCH] remote.c: use shorten_unambiguous_ref","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-04-09T15:33:02Z","receivedAt":"2009-04-09T15:33:02Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Use the new shorten_unambiguous_ref() for simplifying the output of\nupstream branch names. This affects status and checkout.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\nI intentionally didn't combine the line with the previous one (see\ncontext) to make things a bit clearer.\n\nI haven't seen more obvious place for using shorten_unambiguous_ref().\nprettify_ref() is a natural candidate but is mostly used for\nprettyfying refs on the remote side. git branch is covered by Jeff's\npatch already.\n\n remote.c |    6 +-----\n 1 files changed, 1 insertions(+), 5 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex a06761a..10af722 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1461,11 +1461,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)\n \t\treturn 0;\n \n \tbase = branch->merge[0]->dst;\n-\tif (!prefixcmp(base, \"refs/remotes/\")) {\n-\t\tbase += strlen(\"refs/remotes/\");\n-\t} else if (!prefixcmp(base, \"refs/heads/\")) {\n-\t\tbase += strlen(\"refs/heads/\");\n-\t}\n+\tbase = shorten_unambiguous_ref(base);\n \tif (!num_theirs)\n \t\tstrbuf_addf(sb, \"Your branch is ahead of '%s' \"\n \t\t\t    \"by %d commit%s.\\n\",\n-- \n1.6.2.2.646.gb214\n"},{"id":"111015","messageId":"20090410171458.GA26478@sigill.intra.peff.net","threadId":"18805","inReplyTo":"1239291182-12860-1-git-send-email-git@drmicha.warpmail.net","subject":"Re: [PATCH] remote.c: use shorten_unambiguous_ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-10T17:14:58Z","receivedAt":"2009-04-10T17:14:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 09, 2009 at 05:33:02PM +0200, Michael J Gruber wrote:\n\n> Use the new shorten_unambiguous_ref() for simplifying the output of\n> upstream branch names. This affects status and checkout.\n\nYeah, this is the spot I was thinking about when I mentioned \"use it in\nother places\" earlier in the thread. So\n\nAcked-by: Jeff King <peff@peff.net>\n\n> I haven't seen more obvious place for using shorten_unambiguous_ref().\n> prettify_ref() is a natural candidate but is mostly used for\n> prettyfying refs on the remote side. git branch is covered by Jeff's\n> patch already.\n\nHmm. I was thinking we might be able to just do away with prettify_ref,\nbut I didn't consider the fact that we need to prettify remote things. I\nthink you could still unambiguously prettify the local half of those\ncallsites, though.\n\nGiven that the two functions are closely related, should we perhaps\nrename them to\n\n  const char *shorten_ref(const char *);\n  const char *shorten_ref_unambiguous(const char *);\n\n? The implementations are quite different, with prettify_ref not really\nrespecting the ref lookup rules, but rather just considering a few\npre-determined bits of the hierarchy as uninteresting. It shouldn't be\nthat hard to have them both use the same implementation, like:\n\n  const char *shorten_ref(const char *, int unambiguous);\n\n-Peff\n"},{"id":"111282","messageId":"49E487B2.5030606@drmicha.warpmail.net","threadId":"18805","inReplyTo":"20090410171458.GA26478@sigill.intra.peff.net","subject":"Re: [PATCH] remote.c: use shorten_unambiguous_ref","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-04-14T12:55:14Z","receivedAt":"2009-04-14T12:55:14Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 10.04.2009 19:14:\n> On Thu, Apr 09, 2009 at 05:33:02PM +0200, Michael J Gruber wrote:\n> \n>> Use the new shorten_unambiguous_ref() for simplifying the output of\n>> upstream branch names. This affects status and checkout.\n> \n> Yeah, this is the spot I was thinking about when I mentioned \"use it in\n> other places\" earlier in the thread. So\n> \n> Acked-by: Jeff King <peff@peff.net>\n> \n>> I haven't seen more obvious place for using shorten_unambiguous_ref().\n>> prettify_ref() is a natural candidate but is mostly used for\n>> prettyfying refs on the remote side. git branch is covered by Jeff's\n>> patch already.\n> \n> Hmm. I was thinking we might be able to just do away with prettify_ref,\n> but I didn't consider the fact that we need to prettify remote things. I\n> think you could still unambiguously prettify the local half of those\n> callsites, though.\n> \n> Given that the two functions are closely related, should we perhaps\n> rename them to\n> \n>   const char *shorten_ref(const char *);\n>   const char *shorten_ref_unambiguous(const char *);\n> \n> ? The implementations are quite different, with prettify_ref not really\n> respecting the ref lookup rules, but rather just considering a few\n> pre-determined bits of the hierarchy as uninteresting. It shouldn't be\n> that hard to have them both use the same implementation, like:\n> \n>   const char *shorten_ref(const char *, int unambiguous);\n> \n> -Peff\n\nShould I rebase this on top of Bert's newer patch (which has the\nsignature you suggest)? Currently I don't see any of them in.\n\nMichael\n"},{"id":"111295","messageId":"7vprff41lf.fsf@gitster.siamese.dyndns.org","threadId":"18805","inReplyTo":"20090410171458.GA26478@sigill.intra.peff.net","subject":"Re: [PATCH] remote.c: use shorten_unambiguous_ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-14T16:55:08Z","receivedAt":"2009-04-14T16:55:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Hmm. I was thinking we might be able to just do away with prettify_ref,\n> but I didn't consider the fact that we need to prettify remote things. I\n> think you could still unambiguously prettify the local half of those\n> callsites, though.\n>\n> Given that the two functions are closely related, should we perhaps\n> rename them to\n>\n>   const char *shorten_ref(const char *);\n>   const char *shorten_ref_unambiguous(const char *);\n>\n> ? The implementations are quite different, with prettify_ref not really\n> respecting the ref lookup rules, but rather just considering a few\n> pre-determined bits of the hierarchy as uninteresting. It shouldn't be\n> that hard to have them both use the same implementation, like:\n>\n>   const char *shorten_ref(const char *, int unambiguous);\n\nI was hoping that a single \"shorten\" function that does not even take\n\"unambiguous\" parameter would be used by almost everybody.  As far as I\ncan see, Bert's \"rev-parse --abbrev-ref\" RFC is the only caller that might\nneed to use a value different from warn_ambiguous_refs, and all the other\nexisting callers (including fill_tracking_info() for \"upstream\" report by\ngit-branch) do not have to pass \"0\" but can use the default.  IOW, we can\nhave:\n\n\tconst char *shorten_ref_unambiguous(const char *ref, int strict);\n\tconst char *shorten_ref(const char *ref)\n        {\n        \treturn shorten_ref_unambiguous(ref, warn_ambiguous_refs);\n\t}\n\nand only specialized callers that really care use shorten_ref_unambiguous\n(without Bert's [PATCH-RFC 3/2] we do not have any such specialized\ncaller, I think).\n\nBut I am not sure how well prettify_ref() fits into this picture.  It is\ncalled only from transport and is meant to deal with refs that exist on\nthe remote side, so ambiguity check against our local namespace would not\nmake much sense.  We could:\n\n\tconst char *shorten_ref_internal(const char *ref, int mode);\n\tconst char *shorten_ref(const char *ref)\n        {\n\t\tunsigned mode = warn_ambiguous_refs ? SHORTEN_STRICT : 0;\n        \treturn shorten_ref_internal(ref, mode);\n\t}\n\tconst char *prettify_ref(const char *ref)\n        {\n        \treturn shorten_ref_internal(ref, SHORTEN_PREFIX_ONLY);\n        }\n\nand have the SHORTEN_PREFIX_ONLY logic inherit from what the current\nprettify_ref() does, but at that point it I do not think it makes sense\nanymore.\n"},{"id":"111298","messageId":"36ca99e90904141118w425a059v6bd22a7e6400f78f@mail.gmail.com","threadId":"18805","inReplyTo":"7vprff41lf.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] remote.c: use shorten_unambiguous_ref","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2009-04-14T18:18:45Z","receivedAt":"2009-04-14T18:18:45Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Tue, Apr 14, 2009 at 18:55, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> Hmm. I was thinking we might be able to just do away with prettify_ref,\n>> but I didn't consider the fact that we need to prettify remote things. I\n>> think you could still unambiguously prettify the local half of those\n>> callsites, though.\n>>\n>> Given that the two functions are closely related, should we perhaps\n>> rename them to\n>>\n>>   const char *shorten_ref(const char *);\n>>   const char *shorten_ref_unambiguous(const char *);\n>>\n>> ? The implementations are quite different, with prettify_ref not really\n>> respecting the ref lookup rules, but rather just considering a few\n>> pre-determined bits of the hierarchy as uninteresting. It shouldn't be\n>> that hard to have them both use the same implementation, like:\n>>\n>>   const char *shorten_ref(const char *, int unambiguous);\n>\n> I was hoping that a single \"shorten\" function that does not even take\n> \"unambiguous\" parameter would be used by almost everybody.  As far as I\n> can see, Bert's \"rev-parse --abbrev-ref\" RFC is the only caller that might\n> need to use a value different from warn_ambiguous_refs, and all the other\n> existing callers (including fill_tracking_info() for \"upstream\" report by\n> git-branch) do not have to pass \"0\" but can use the default.  IOW, we can\n> have:\n>\n>        const char *shorten_ref_unambiguous(const char *ref, int strict);\n>        const char *shorten_ref(const char *ref)\n>        {\n>                return shorten_ref_unambiguous(ref, warn_ambiguous_refs);\n>        }\nThere's too much confusion: Whatever input you gave to\nshorten_unambiguous_ref(), if you pass the result to dwim_ref() you\nget the input again. That is, all return values are unambiguous. The\nonly small different (and here comes a little word-confusion) is, that\nin strict mode you wont get any warning for 'ambiguous' refs. So the\nnames \"shorten_ref_unambiguous\" and \"shorten_rer\" are misleading,\nbecause it looks like the former returns unambiguous refs and the\nlatter not. Which is wrong.\n\nI can't think about this further, no time sorry.\n\nBert\n"},{"id":"111344","messageId":"20090415080301.GD23332@coredump.intra.peff.net","threadId":"18805","inReplyTo":"49E487B2.5030606@drmicha.warpmail.net","subject":"Re: [PATCH] remote.c: use shorten_unambiguous_ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-15T08:03:02Z","receivedAt":"2009-04-15T08:03:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 14, 2009 at 02:55:14PM +0200, Michael J Gruber wrote:\n\n> >   const char *shorten_ref(const char *);\n> >   const char *shorten_ref_unambiguous(const char *);\n> > \n> > ? The implementations are quite different, with prettify_ref not really\n> > respecting the ref lookup rules, but rather just considering a few\n> > pre-determined bits of the hierarchy as uninteresting. It shouldn't be\n> > that hard to have them both use the same implementation, like:\n> > \n> >   const char *shorten_ref(const char *, int unambiguous);\n> \n> Should I rebase this on top of Bert's newer patch (which has the\n> signature you suggest)? Currently I don't see any of them in.\n\nIt has the same signature, but the meanings are different. My flag meant\n\"should we bother looking at ambiguity at all?\" whereas Bert's flag is\nabout \"how strictly should we be counting ambiguity?\".\n\nSo really there are three levels, which implies an enum (with none,\nloose, and strict). However, calling it with a tri-state flag would be\nless convenient, since your logic for the flag is:\n\n  if (we are shortening something non-local)\n    flag = none;\n  else if (core.warn_ambiguous_refs is not set)\n    flag = loose;\n  else\n    flag = strict;\n\nSo you really want to pass the two bits of information separately.\n\n-Peff\n"},{"id":"111345","messageId":"20090415081246.GE23332@coredump.intra.peff.net","threadId":"18805","inReplyTo":"7vprff41lf.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] remote.c: use shorten_unambiguous_ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-04-15T08:12:46Z","receivedAt":"2009-04-15T08:12:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 14, 2009 at 09:55:08AM -0700, Junio C Hamano wrote:\n\n> I was hoping that a single \"shorten\" function that does not even take\n> \"unambiguous\" parameter would be used by almost everybody.  As far as I\n> can see, Bert's \"rev-parse --abbrev-ref\" RFC is the only caller that might\n> need to use a value different from warn_ambiguous_refs, and all the other\n> existing callers (including fill_tracking_info() for \"upstream\" report by\n> git-branch) do not have to pass \"0\" but can use the default.  IOW, we can\n> have:\n> \n> \tconst char *shorten_ref_unambiguous(const char *ref, int strict);\n> \tconst char *shorten_ref(const char *ref)\n>         {\n>         \treturn shorten_ref_unambiguous(ref, warn_ambiguous_refs);\n> \t}\n> \n> and only specialized callers that really care use shorten_ref_unambiguous\n> (without Bert's [PATCH-RFC 3/2] we do not have any such specialized\n> caller, I think).\n\nI think that is a sensible approach; I also thought when reading Bert's\npatch that the parameter seemed like it would not be used in most\nsituations.\n\n> But I am not sure how well prettify_ref() fits into this picture.  It is\n> called only from transport and is meant to deal with refs that exist on\n> the remote side, so ambiguity check against our local namespace would not\n> make much sense.  We could:\n> \n> \tconst char *shorten_ref_internal(const char *ref, int mode);\n> \tconst char *shorten_ref(const char *ref)\n>         {\n> \t\tunsigned mode = warn_ambiguous_refs ? SHORTEN_STRICT : 0;\n>         \treturn shorten_ref_internal(ref, mode);\n> \t}\n> \tconst char *prettify_ref(const char *ref)\n>         {\n>         \treturn shorten_ref_internal(ref, SHORTEN_PREFIX_ONLY);\n>         }\n> \n> and have the SHORTEN_PREFIX_ONLY logic inherit from what the current\n> prettify_ref() does, but at that point it I do not think it makes sense\n> anymore.\n\nThere are three things wrong with prettify_ref:\n\n  1. It takes a ref struct instead of a string with a refname (but only\n     looks at ref->name). This is easily fixed.\n\n  2. It does the same thing as shorten_ref_unambiguous, but without any\n     ambiguity check, so the names should be related. That is easily\n     changed, too, once we settle on the name (either it is shorten_ref\n     to the other's _unambiguous form, or the unambiguous one becomes\n     shorten_ref, and this becomes shorten_ref_remote or something).\n\n  3. It uses its own \"skip these random things rules\" instead of being\n     based on the usual ref lookup rules. I think this can be folded\n     into the unambiguous case by simply bailing on the first textual\n     match.  I don't know in practice if it matters that much.\n\n-Peff\n"}]}