Nguyễn Thái Ngọc Duy <pclo...@gmail.com> writes:
> This option makes sorting ignore case, which is great when you have
> branches named bug-12-do-something, Bug-12-do-some-more and
> BUG-12-do-what and want to group them together. Sorting externally may
> not be an option because we lose coloring and column layout from
> git-branch and git-tag.
>
> The same could be said for filtering, but it's probably less important
> because you can always go with the ugly pattern [bB][uU][gG]-* if you're
> desperate.
But of course --ignore-case is of course much easier.
> You can't have case-sensitive filtering and case-insensitive sorting (or
> the other way around) with this though. But who would want that?
I do not feel uncomfortable declaring that the answer to that
question is "nobody" ;-)
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
> ---
> v2 has a different approach, and I think it's a better one even with
> that unanswered question above.
Yeah, I think this would be easier to use.
> @@ -512,15 +512,6 @@ static void print_ref_list(struct ref_filter *filter,
> struct ref_sorting *sortin
> if (filter->verbose)
> maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
>
> - /*
> - * If no sorting parameter is given then we default to sorting
> - * by 'refname'. This would give us an alphabetically sorted
> - * array with the 'HEAD' ref at the beginning followed by
> - * local branches 'refs/heads/...' and finally remote-tacking
> - * branches 'refs/remotes/...'.
> - */
> - if (!sorting)
> - sorting = ref_default_sorting();
So it is now a BUG() to give sorting==NULL to this function, which
is OK and I do not think we even need an assert() for it (i.e. the
code with the patch looks good).
> @@ -744,6 +739,16 @@ int cmd_branch(int argc, const char **argv, const char
> *prefix)
> if ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)
> filter.kind |= FILTER_REFS_DETACHED_HEAD;
> filter.name_patterns = argv;
> + /*
> + * If no sorting parameter is given then we default to sorting
> + * by 'refname'. This would give us an alphabetically sorted
> + * array with the 'HEAD' ref at the beginning followed by
> + * local branches 'refs/heads/...' and finally remote-tacking
> + * branches 'refs/remotes/...'.
> + */
> + if (!sorting)
> + sorting = ref_default_sorting();
> + sorting->ignore_case = icase;
> print_ref_list(&filter, sorting);
> print_columns(&output, colopts, NULL);
> string_list_clear(&output, 0);
... and the fallback is moved to the caller, which makes sense.
> diff --git a/ref-filter.c b/ref-filter.c
> index f5f7a70..bd98010 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1231,8 +1231,14 @@ static int commit_contains(struct ref_filter *filter,
> struct commit *commit)
> * matches a pattern "refs/heads/mas") or a wildcard (e.g. the same ref
> * matches "refs/heads/mas*", too).
> */
> -static int match_pattern(const char **patterns, const char *refname)
> +static int match_pattern(const struct ref_filter *filter, const char
> *refname)
> {
> + const char **patterns = filter->name_patterns;
> + unsigned flags = 0;
> +
> + if (filter->ignore_case)
> + flags |= WM_CASEFOLD;
> +
Ahh, OK. My reading stuttered when seeing "sorting and filtering"
in the option description for "git tag", but this makes perfect
sense. Good job.
> @@ -1255,9 +1261,15 @@ static int match_pattern(const char **patterns, const
> char *refname)
> * matches a pattern "refs/heads/" but not "refs/heads/m") or a
> * wildcard (e.g. the same ref matches "refs/heads/m*", too).
> */
> -static int match_name_as_path(const char **pattern, const char *refname)
> +static int match_name_as_path(const struct ref_filter *filter, const char
> *refname)
> {
> + const char **pattern = filter->name_patterns;
> int namelen = strlen(refname);
> + unsigned flags = WM_PATHNAME;
> +
> + if (filter->ignore_case)
> + flags |= WM_CASEFOLD;
> +
Likewise. Very simple and nicely done.
> @@ -1536,18 +1548,20 @@ static int cmp_ref_sorting(struct ref_sorting *s,
> struct ref_array_item *a, stru
> struct atom_value *va, *vb;
> int cmp;
> cmp_type cmp_type = used_atom[s->atom].type;
> + int (*cmp_fn)(const char *, const char *);
>
> get_ref_atom_value(a, s->atom, &va);
> get_ref_atom_value(b, s->atom, &vb);
> + cmp_fn = s->ignore_case ? strcasecmp : strcmp;
> if (s->version)
> cmp = versioncmp(va->s, vb->s);
> else if (cmp_type == FIELD_STR)
> - cmp = strcmp(va->s, vb->s);
> + cmp = cmp_fn(va->s, vb->s);
> else {
> if (va->ul < vb->ul)
> cmp = -1;
> else if (va->ul == vb->ul)
> - cmp = strcmp(a->refname, b->refname);
> + cmp = cmp_fn(a->refname, b->refname);
> else
> cmp = 1;
> }
OK.
> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index c6a3ccb..fad79e8 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -89,6 +89,11 @@ test_expect_success 'git branch --list -v pattern shows
> branch summaries' '
> awk "{print \$NF}" <tmp >actual &&
> test_cmp expect actual
> '
> +test_expect_success 'git branch --ignore-case --list -v pattern shows branch
> summaries' '
> + git branch --list --ignore-case -v BRANCH* >tmp &&
> + awk "{print \$NF}" <tmp >actual &&
> + test_cmp expect actual
> +'
The way the test ensures it found only branch-one and branch-two
looks very sloppy, but that was inherited from the existing one
before this new one, so I'll let it pass.
> @@ -196,4 +201,21 @@ test_expect_success 'local-branch symrefs shortened
> properly' '
> test_cmp expect actual
> '
>
> +test_expect_success 'sort branches, ignore case' '
> + (
> + git init sort-icase &&
> + cd sort-icase &&
> + test_commit initial &&
> + git branch branch-one &&
> + git branch BRANCH-two &&
> + git branch --list -i | awk "{print \$NF}" >actual &&
> + cat >expected <<-\EOF &&
> + branch-one
> + BRANCH-two
> + master
> + EOF
> + test_cmp expected actual
> + )
> +'
Is there an existing test that uses refs with mixed cases, i.e. the
result of listing sorts differently with and without the -i option?
If not, this one should test output from both cases to ensure that
the command run without -i stays case sensitive.
> test_done
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 8b0f71a..2d9cae3 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -27,6 +27,23 @@ test_expect_success 'listing all tags in an empty tree
> should output nothing' '
> test $(git tag | wc -l) -eq 0
> '
>
> +test_expect_success 'sort tags, ignore case' '
> + (
> + git init sort &&
> + cd sort &&
> + test_commit initial &&
> + git tag tag-one &&
> + git tag TAG-two &&
> + git tag -l -i >actual &&
> + cat >expected <<-\EOF &&
> + initial
> + tag-one
> + TAG-two
> + EOF
> + test_cmp expected actual
> + )
> +'
Ditto.
> test_expect_success 'looking for a tag in an empty tree should fail' \
> '! (tag_exists mytag)'
>
> @@ -81,6 +98,9 @@ test_expect_success 'listing all tags if one exists should
> output that tag' '
> test_expect_success 'listing a tag using a matching pattern should succeed' \
> 'git tag -l mytag'
>
> +test_expect_success 'listing a tag using a matching pattern should succeed' \
> + 'git tag -l --ignore-case MYTAG'
The existing one before this one merely says that "git tag -l" must
exit with 0 status code, no?
IOW, even "git tag -l no-such-tag-anywhere && echo OK" yields OK.
So there is not much point replicating it with "-i", unless you want
to say that "git tag -i -l" also must exit with 0 status code.
> test_expect_success \
> 'listing a tag using a matching pattern should output that tag' \
> 'test $(git tag -l mytag) = mytag'
I think the new one would want to mimic this one instead. Look for
MYTAG with the -i option and see it output mytag (in lowercase).
