Re: [PATCH] name-rev: Allow to omit refs/tags/ part in --refs option when --tags used

[Namhyung Kim <namhyung.kim@lge.com>]

Hi Junio,

2013-06-18 AM 12:27, Junio C Hamano wrote:
> Namhyung Kim <namhyung.kim@lge.com> writes:
>
>> In its current form, when an user wants to filter specific ref using
>> --refs option, she needs to give something like --refs=refs/tags/v1.*.
>>
>> This is not intuitive as users might think it's enough to give just
>> actual tag name part like --refs=v1.*.
>
> I do not think "Users might think" is not particularly a good
> justification, but I agree that it would be useful to allow
> --refs=v1.\* to match refs/heads/v1.4-maint and refs/tags/v1.4.0; it
> is easy for the users to disambiguate with longer prefix if they
> wanted to.

Right.  I just failed to find right words. :)

>
>> It applies to refs other than
>> just tags too.  Change it for users to be able to use --refs=sth or
>> --refs=remotes/sth.
>>
>> Also remove the leading 'tags/' part in the output when --tags option
>> was given since the option restricts to work with tags only.
>
> This part is questionable, as it changes the output people's scripts
> have been reading from the command since eternity ago.

True.

>
> If the pattern asks to match with v1.* (not tags/v1.* or
> refs/tags/v1.*) and you find refs/tags/v1.*, it might be acceptable
> to strip "refs/tags/" part.  Existing users are _expected_ to feed a
> pattern with full refname starting with refs/, so they will not be
> negatively affected by such a usability enhancement on the output
> side.

This is what I wanted to do exactly. :)

>
>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
>> index 6238247..446743b 100644
>> --- a/builtin/name-rev.c
>> +++ b/builtin/name-rev.c
>> @@ -97,7 +97,8 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
>>   	if (data->tags_only && prefixcmp(path, "refs/tags/"))
>>   		return 0;
>>
>> -	if (data->ref_filter && fnmatch(data->ref_filter, path, 0))
>> +	if (data->ref_filter && !prefixcmp(data->ref_filter, "refs/")
>> +	    && fnmatch(data->ref_filter, path, 0))
>>   		return 0;
>
> What does this mean?  "When --refs is specified, if it begins with
> refs/ then do not show unmatching path, but let any path be subject
> to the following if --refs does not begin with refs/" sounds like a
> broken logic, unless you add another fnmatch() later in the codepath
> to compensate.  And you indeed do so, but then at that point, do we
> still need this "if(...) return 0" at all?
>
> I think it can and should be improved here, and then the one in the
> main logic you added can be removed.
>
> Wouldn't it make more sense to see if the given pattern matches a
> tail substring of the ref, instead of using the hardcoded "strip
> refs/heads/, refs/tags or refs/, and then match once" logic?  That
> way, --refs=origin/* can find refs/remotes/origin/master by running
> fnmatch of origin/* against its substrings, i.e.
>
> 	refs/remotes/origin/master
>          remotes/origin/master
>          origin/master
>
> and find that the pattern matches it.
>
> Perhaps it is just the matter of adding something like:
>
> 	static int subpath_matches(const char *path, const char	*filter)
> 	{
> 		const char *subpath = path;
> 		while (subpath) {
>                  	if (!fnmatch(data->ref_filter, subpath, 0))
> 				return subpath - path;
> 			subpath = strchr(path, '/');
   					 subpath

>                          if (subpath)
> 	                        subpath++;
> 		}
> 		return -1;
> 	}
>
> and then at the beginning of name_ref() do this:
>
> 	int can_abbreviate_output = data->name_only;
>
> 	if (data->tags_only && prefixcmp(path, "refs/tags/"))
> 		return 0;
> 	if (data->ref_filter) {
>          	switch (subpath_matches(path, data->ref_filter)) {
> 		case -1: /* did not match */
> 			return 0;
> 		default: /* matched subpath */
> 			can_abbreviate_output = 1;
> 			break;
> 		case 0: /* matched fully */
>                  	break;
> 		}
> 	}
>
> The logic before calling name_rev() will be kept as "only decide how
> the output looks like", without mixing the unrelated "decide if we
> want to use it" logic in.

Looks good to me with the little change above!

I'll resend v2 with changes in this and your other reply.

Thanks,
Namhyung