Re: [PATCH v2] tag, branch, for-each-ref: add --ignore-case for sorting and filtering

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@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 <pclouds@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).