{"thread":{"id":"34172","subject":"[PATCH] name-rev: Allow to omit refs/tags/ part in --refs option when --tags used","startedAt":"2013-06-17T07:53:56Z","lastAt":"2013-06-18T12:24:53Z","messageCount":5,"participants":["Namhyung Kim","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"221038","messageId":"1371455636-1378-1-git-send-email-namhyung.kim@lge.com","threadId":"34172","inReplyTo":null,"subject":"[PATCH] name-rev: Allow to omit refs/tags/ part in --refs option when --tags used","fromName":"Namhyung Kim","fromEmail":"namhyung.kim@lge.com","sentAt":"2013-06-17T07:53:56Z","receivedAt":"2013-06-17T07:53:56Z","isPatch":true,"sender":{"key":"namhyung@gmail.com","avatar":"https://avatars.githubusercontent.com/u/48503?v=4"},"body":"In its current form, when an user wants to filter specific ref using\n--refs option, she needs to give something like --refs=refs/tags/v1.*.\n\nThis is not intuitive as users might think it's enough to give just\nactual tag name part like --refs=v1.*.  It applies to refs other than\njust tags too.  Change it for users to be able to use --refs=sth or\n--refs=remotes/sth.\n\nAlso remove the leading 'tags/' part in the output when --tags option\nwas given since the option restricts to work with tags only.  This is\nwhat we have if --name-only option was given also.\n\nSigned-off-by: Namhyung Kim <namhyung.kim@lge.com>\n---\n builtin/name-rev.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex 6238247..446743b 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -97,7 +97,8 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void\n \tif (data->tags_only && prefixcmp(path, \"refs/tags/\"))\n \t\treturn 0;\n \n-\tif (data->ref_filter && fnmatch(data->ref_filter, path, 0))\n+\tif (data->ref_filter && !prefixcmp(data->ref_filter, \"refs/\")\n+\t    && fnmatch(data->ref_filter, path, 0))\n \t\treturn 0;\n \n \twhile (o && o->type == OBJ_TAG) {\n@@ -113,12 +114,15 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void\n \t\tif (!prefixcmp(path, \"refs/heads/\"))\n \t\t\tpath = path + 11;\n \t\telse if (data->tags_only\n-\t\t    && data->name_only\n \t\t    && !prefixcmp(path, \"refs/tags/\"))\n \t\t\tpath = path + 10;\n \t\telse if (!prefixcmp(path, \"refs/\"))\n \t\t\tpath = path + 5;\n \n+\t\tif (data->ref_filter && prefixcmp(data->ref_filter, \"refs/\")\n+\t\t    && fnmatch(data->ref_filter, path, 0))\n+\t\t\treturn 0;\n+\n \t\tname_rev(commit, xstrdup(path), 0, 0, deref);\n \t}\n \treturn 0;\n-- \n1.7.11.7\n"},{"id":"221066","messageId":"7vip1chi50.fsf@alter.siamese.dyndns.org","threadId":"34172","inReplyTo":"1371455636-1378-1-git-send-email-namhyung.kim@lge.com","subject":"Re: [PATCH] name-rev: Allow to omit refs/tags/ part in --refs option when --tags used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-17T15:27:39Z","receivedAt":"2013-06-17T15:27:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Namhyung Kim <namhyung.kim@lge.com> writes:\n\n> In its current form, when an user wants to filter specific ref using\n> --refs option, she needs to give something like --refs=refs/tags/v1.*.\n>\n> This is not intuitive as users might think it's enough to give just\n> actual tag name part like --refs=v1.*.\n\nI do not think \"Users might think\" is not particularly a good\njustification, but I agree that it would be useful to allow\n--refs=v1.\\* to match refs/heads/v1.4-maint and refs/tags/v1.4.0; it\nis easy for the users to disambiguate with longer prefix if they\nwanted to.\n\n> It applies to refs other than\n> just tags too.  Change it for users to be able to use --refs=sth or\n> --refs=remotes/sth.\n>\n> Also remove the leading 'tags/' part in the output when --tags option\n> was given since the option restricts to work with tags only.\n\nThis part is questionable, as it changes the output people's scripts\nhave been reading from the command since eternity ago.\n\nIf the pattern asks to match with v1.* (not tags/v1.* or\nrefs/tags/v1.*) and you find refs/tags/v1.*, it might be acceptable\nto strip \"refs/tags/\" part.  Existing users are _expected_ to feed a\npattern with full refname starting with refs/, so they will not be\nnegatively affected by such a usability enhancement on the output\nside.\n\n> diff --git a/builtin/name-rev.c b/builtin/name-rev.c\n> index 6238247..446743b 100644\n> --- a/builtin/name-rev.c\n> +++ b/builtin/name-rev.c\n> @@ -97,7 +97,8 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void\n>  \tif (data->tags_only && prefixcmp(path, \"refs/tags/\"))\n>  \t\treturn 0;\n>  \n> -\tif (data->ref_filter && fnmatch(data->ref_filter, path, 0))\n> +\tif (data->ref_filter && !prefixcmp(data->ref_filter, \"refs/\")\n> +\t    && fnmatch(data->ref_filter, path, 0))\n>  \t\treturn 0;\n\nWhat does this mean?  \"When --refs is specified, if it begins with\nrefs/ then do not show unmatching path, but let any path be subject\nto the following if --refs does not begin with refs/\" sounds like a\nbroken logic, unless you add another fnmatch() later in the codepath\nto compensate.  And you indeed do so, but then at that point, do we\nstill need this \"if(...) return 0\" at all?\n\nI think it can and should be improved here, and then the one in the\nmain logic you added can be removed.\n\nWouldn't it make more sense to see if the given pattern matches a\ntail substring of the ref, instead of using the hardcoded \"strip\nrefs/heads/, refs/tags or refs/, and then match once\" logic?  That\nway, --refs=origin/* can find refs/remotes/origin/master by running\nfnmatch of origin/* against its substrings, i.e.\n\n\trefs/remotes/origin/master\n        remotes/origin/master\n        origin/master\n\nand find that the pattern matches it.\n\nPerhaps it is just the matter of adding something like:\n\n\tstatic int subpath_matches(const char *path, const char\t*filter)\n\t{        \n\t\tconst char *subpath = path;\n\t\twhile (subpath) {\n                \tif (!fnmatch(data->ref_filter, subpath, 0))\n\t\t\t\treturn subpath - path;\n\t\t\tsubpath = strchr(path, '/');\n                        if (subpath)\n\t                        subpath++;\n\t\t}\n\t\treturn -1;\n\t}\n\nand then at the beginning of name_ref() do this:\n\n\tint can_abbreviate_output = data->name_only;\n\n\tif (data->tags_only && prefixcmp(path, \"refs/tags/\"))\n\t\treturn 0;\n\tif (data->ref_filter) {\n        \tswitch (subpath_matches(path, data->ref_filter)) {\n\t\tcase -1: /* did not match */\n\t\t\treturn 0;\n\t\tdefault: /* matched subpath */\n\t\t\tcan_abbreviate_output = 1;\n\t\t\tbreak;\n\t\tcase 0: /* matched fully */\n                \tbreak;\n\t\t}\n\t}\n\nThe logic before calling name_rev() will be kept as \"only decide how\nthe output looks like\", without mixing the unrelated \"decide if we\nwant to use it\" logic in.\n\n>  \twhile (o && o->type == OBJ_TAG) {\n> @@ -113,12 +114,15 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void\n>  \t\tif (!prefixcmp(path, \"refs/heads/\"))\n>  \t\t\tpath = path + 11;\n>  \t\telse if (data->tags_only\n> -\t\t    && data->name_only\n>  \t\t    && !prefixcmp(path, \"refs/tags/\"))\n>  \t\t\tpath = path + 10;\n>  \t\telse if (!prefixcmp(path, \"refs/\"))\n>  \t\t\tpath = path + 5;\n>  \n> +\t\tif (data->ref_filter && prefixcmp(data->ref_filter, \"refs/\")\n> +\t\t    && fnmatch(data->ref_filter, path, 0))\n> +\t\t\treturn 0;\n>  \t\tname_rev(commit, xstrdup(path), 0, 0, deref);\n>  \t}\n>  \treturn 0;\n"},{"id":"221067","messageId":"7vehc0hgy6.fsf@alter.siamese.dyndns.org","threadId":"34172","inReplyTo":"7vip1chi50.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] name-rev: Allow to omit refs/tags/ part in --refs option when --tags used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-17T15:53:21Z","receivedAt":"2013-06-17T15:53:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Wouldn't it make more sense to see if the given pattern matches a\n> tail substring of the ref, instead of using the hardcoded \"strip\n> refs/heads/, refs/tags or refs/, and then match once\" logic?  That\n> way, --refs=origin/* can find refs/remotes/origin/master by running\n> fnmatch of origin/* against its substrings, i.e.\n>\n> \trefs/remotes/origin/master\n>         remotes/origin/master\n>         origin/master\n>\n> and find that the pattern matches it.\n>\n> Perhaps it is just the matter of adding something like:\n> ...\n> and then at the beginning of name_ref() do this:\n>\n> \tint can_abbreviate_output = data->name_only;\n>\n> \tif (data->tags_only && prefixcmp(path, \"refs/tags/\"))\n> \t\treturn 0;\n> \tif (data->ref_filter) {\n>         \tswitch (subpath_matches(path, data->ref_filter)) {\n> \t\tcase -1: /* did not match */\n> \t\t\treturn 0;\n> \t\tdefault: /* matched subpath */\n> \t\t\tcan_abbreviate_output = 1;\n> \t\t\tbreak;\n> \t\tcase 0: /* matched fully */\n>                 \tbreak;\n> \t\t}\n> \t}\n>\n> The logic before calling name_rev() will be kept as \"only decide how\n> the output looks like\", without mixing the unrelated \"decide if we\n> want to use it\" logic in.\n\n... which may make the \"call name_rev with this abbreviated path\"\nlogic look something like this:\n\n\tif (o && o->type == OBJ_COMMIT) {\n        \tif (can_abbreviate_output)\n\t\t\tpath = shorten_unambiguous_ref(path, 0);\n\t\telse if (!prefixcmp(path, \"refs/heads/\"))\n\t\t\tpath = path + 11;\n\t\telse if (data->tags_only\n\t\t    && data->name_only\n\t\t    && !prefixcmp(path, \"refs/tags/\"))\n\t\t\tpath = path + 10;\n\t\telse if (!prefixcmp(path, \"refs/\"))\n\t\t\tpath = path + 5;\n\n\t\tname_rev((struct commit *) o, xstrdup(path), 0, 0, deref);\n\t}\n"},{"id":"221189","messageId":"51C04ACD.1000003@lge.com","threadId":"34172","inReplyTo":"7vip1chi50.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] name-rev: Allow to omit refs/tags/ part in --refs option when --tags used","fromName":"Namhyung Kim","fromEmail":"namhyung.kim@lge.com","sentAt":"2013-06-18T11:55:57Z","receivedAt":"2013-06-18T11:55:57Z","isPatch":true,"sender":{"key":"namhyung@gmail.com","avatar":"https://avatars.githubusercontent.com/u/48503?v=4"},"body":"Hi Junio,\n\n2013-06-18 AM 12:27, Junio C Hamano wrote:\n> Namhyung Kim <namhyung.kim@lge.com> writes:\n>\n>> In its current form, when an user wants to filter specific ref using\n>> --refs option, she needs to give something like --refs=refs/tags/v1.*.\n>>\n>> This is not intuitive as users might think it's enough to give just\n>> actual tag name part like --refs=v1.*.\n>\n> I do not think \"Users might think\" is not particularly a good\n> justification, but I agree that it would be useful to allow\n> --refs=v1.\\* to match refs/heads/v1.4-maint and refs/tags/v1.4.0; it\n> is easy for the users to disambiguate with longer prefix if they\n> wanted to.\n\nRight.  I just failed to find right words. :)\n\n>\n>> It applies to refs other than\n>> just tags too.  Change it for users to be able to use --refs=sth or\n>> --refs=remotes/sth.\n>>\n>> Also remove the leading 'tags/' part in the output when --tags option\n>> was given since the option restricts to work with tags only.\n>\n> This part is questionable, as it changes the output people's scripts\n> have been reading from the command since eternity ago.\n\nTrue.\n\n>\n> If the pattern asks to match with v1.* (not tags/v1.* or\n> refs/tags/v1.*) and you find refs/tags/v1.*, it might be acceptable\n> to strip \"refs/tags/\" part.  Existing users are _expected_ to feed a\n> pattern with full refname starting with refs/, so they will not be\n> negatively affected by such a usability enhancement on the output\n> side.\n\nThis is what I wanted to do exactly. :)\n\n>\n>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c\n>> index 6238247..446743b 100644\n>> --- a/builtin/name-rev.c\n>> +++ b/builtin/name-rev.c\n>> @@ -97,7 +97,8 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void\n>>   \tif (data->tags_only && prefixcmp(path, \"refs/tags/\"))\n>>   \t\treturn 0;\n>>\n>> -\tif (data->ref_filter && fnmatch(data->ref_filter, path, 0))\n>> +\tif (data->ref_filter && !prefixcmp(data->ref_filter, \"refs/\")\n>> +\t    && fnmatch(data->ref_filter, path, 0))\n>>   \t\treturn 0;\n>\n> What does this mean?  \"When --refs is specified, if it begins with\n> refs/ then do not show unmatching path, but let any path be subject\n> to the following if --refs does not begin with refs/\" sounds like a\n> broken logic, unless you add another fnmatch() later in the codepath\n> to compensate.  And you indeed do so, but then at that point, do we\n> still need this \"if(...) return 0\" at all?\n>\n> I think it can and should be improved here, and then the one in the\n> main logic you added can be removed.\n>\n> Wouldn't it make more sense to see if the given pattern matches a\n> tail substring of the ref, instead of using the hardcoded \"strip\n> refs/heads/, refs/tags or refs/, and then match once\" logic?  That\n> way, --refs=origin/* can find refs/remotes/origin/master by running\n> fnmatch of origin/* against its substrings, i.e.\n>\n> \trefs/remotes/origin/master\n>          remotes/origin/master\n>          origin/master\n>\n> and find that the pattern matches it.\n>\n> Perhaps it is just the matter of adding something like:\n>\n> \tstatic int subpath_matches(const char *path, const char\t*filter)\n> \t{\n> \t\tconst char *subpath = path;\n> \t\twhile (subpath) {\n>                  \tif (!fnmatch(data->ref_filter, subpath, 0))\n> \t\t\t\treturn subpath - path;\n> \t\t\tsubpath = strchr(path, '/');\n   \t\t\t\t\t subpath\n\n>                          if (subpath)\n> \t                        subpath++;\n> \t\t}\n> \t\treturn -1;\n> \t}\n>\n> and then at the beginning of name_ref() do this:\n>\n> \tint can_abbreviate_output = data->name_only;\n>\n> \tif (data->tags_only && prefixcmp(path, \"refs/tags/\"))\n> \t\treturn 0;\n> \tif (data->ref_filter) {\n>          \tswitch (subpath_matches(path, data->ref_filter)) {\n> \t\tcase -1: /* did not match */\n> \t\t\treturn 0;\n> \t\tdefault: /* matched subpath */\n> \t\t\tcan_abbreviate_output = 1;\n> \t\t\tbreak;\n> \t\tcase 0: /* matched fully */\n>                  \tbreak;\n> \t\t}\n> \t}\n>\n> The logic before calling name_rev() will be kept as \"only decide how\n> the output looks like\", without mixing the unrelated \"decide if we\n> want to use it\" logic in.\n\nLooks good to me with the little change above!\n\nI'll resend v2 with changes in this and your other reply.\n\nThanks,\nNamhyung\n"},{"id":"221199","messageId":"51C05195.7000403@lge.com","threadId":"34172","inReplyTo":"7vehc0hgy6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] name-rev: Allow to omit refs/tags/ part in --refs option when --tags used","fromName":"Namhyung Kim","fromEmail":"namhyung.kim@lge.com","sentAt":"2013-06-18T12:24:53Z","receivedAt":"2013-06-18T12:24:53Z","isPatch":true,"sender":{"key":"namhyung@gmail.com","avatar":"https://avatars.githubusercontent.com/u/48503?v=4"},"body":"2013-06-18 AM 12:53, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Wouldn't it make more sense to see if the given pattern matches a\n>> tail substring of the ref, instead of using the hardcoded \"strip\n>> refs/heads/, refs/tags or refs/, and then match once\" logic?  That\n>> way, --refs=origin/* can find refs/remotes/origin/master by running\n>> fnmatch of origin/* against its substrings, i.e.\n>>\n>> \trefs/remotes/origin/master\n>>          remotes/origin/master\n>>          origin/master\n>>\n>> and find that the pattern matches it.\n>>\n>> Perhaps it is just the matter of adding something like:\n>> ...\n>> and then at the beginning of name_ref() do this:\n>>\n>> \tint can_abbreviate_output = data->name_only;\n>>\n>> \tif (data->tags_only && prefixcmp(path, \"refs/tags/\"))\n>> \t\treturn 0;\n>> \tif (data->ref_filter) {\n>>          \tswitch (subpath_matches(path, data->ref_filter)) {\n>> \t\tcase -1: /* did not match */\n>> \t\t\treturn 0;\n>> \t\tdefault: /* matched subpath */\n>> \t\t\tcan_abbreviate_output = 1;\n>> \t\t\tbreak;\n>> \t\tcase 0: /* matched fully */\n>>                  \tbreak;\n>> \t\t}\n>> \t}\n>>\n>> The logic before calling name_rev() will be kept as \"only decide how\n>> the output looks like\", without mixing the unrelated \"decide if we\n>> want to use it\" logic in.\n>\n> ... which may make the \"call name_rev with this abbreviated path\"\n> logic look something like this:\n>\n> \tif (o && o->type == OBJ_COMMIT) {\n>          \tif (can_abbreviate_output)\n> \t\t\tpath = shorten_unambiguous_ref(path, 0);\n> \t\telse if (!prefixcmp(path, \"refs/heads/\"))\n> \t\t\tpath = path + 11;\n> \t\telse if (data->tags_only\n> \t\t    && data->name_only\n> \t\t    && !prefixcmp(path, \"refs/tags/\"))\n> \t\t\tpath = path + 10;\n> \t\telse if (!prefixcmp(path, \"refs/\"))\n> \t\t\tpath = path + 5;\n>\n> \t\tname_rev((struct commit *) o, xstrdup(path), 0, 0, deref);\n> \t}\n>\n>\n\nHmm.. I thought about it twice.\n\nThis will affects the output of `--name-only \n--refs=refs/remotes/origin/*` case.  (AFAIK it only affected to tags so \nfar)  As the name_only always sets can_abbreviate_output, it'll shorten \nthe name of remote ref even if it's fully matched.\n\n  $ ./git name-rev --refs=refs/remotes/origin/* a2055c2\n  a2055c2 remotes/origin/maint~642\n\n  $ ./git name-rev --refs=refs/remotes/origin/* --name-only a2055c2\n  origin/maint~642\n\nI think it should be 'data->tags_only && data->name_only' for \ncompatibility reason.\n\nI also see that the 3rd condition of 'tags_only && name_only' turned out \nto be useless for the similar reason.  When name_only set, it'll take \nthe first case so 3rd case cannot be reached. When it's not set it \ncannot take the third case too obviously.  So I'll just remove it.\n\nThanks,\nNamhyung\n"}]}