Namhyung Kim <namhyung.kim@xxxxxxx> 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.
> 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.
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.
> 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, '/');
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.
> while (o && o->type == OBJ_TAG) {
> @@ -113,12 +114,15 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
> if (!prefixcmp(path, "refs/heads/"))
> path = path + 11;
> else if (data->tags_only
> - && data->name_only
> && !prefixcmp(path, "refs/tags/"))
> path = path + 10;
> else if (!prefixcmp(path, "refs/"))
> path = path + 5;
>
> + if (data->ref_filter && prefixcmp(data->ref_filter, "refs/")
> + && fnmatch(data->ref_filter, path, 0))
> + return 0;
> name_rev(commit, xstrdup(path), 0, 0, deref);
> }
> return 0;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html