Re: [PATCH 1/9] ref-filter.c: really don't sort when using --no-sort

[Victoria Dye <vdye@github.com>]

Patrick Steinhardt wrote:
> On Tue, Nov 07, 2023 at 01:25:53AM +0000, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <vdye@github.com>
>>
>> Update 'ref_sorting_options()' to return a NULL 'struct ref_sorting *' if
>> the string list provided to it is empty, rather than returning the default
>> refname sort structure. Also update 'ref_array_sort()' to explicitly skip
>> sorting if its 'struct ref_sorting *' arg is NULL. Other functions using
>> 'struct ref_sorting *' do not need any changes because they already properly
>> ignore NULL values.
>>
>> The goal of this change is to have the '--no-sort' option truly disable
>> sorting in commands like 'for-each-ref, 'tag', and 'branch'. Right now,
>> '--no-sort' will still trigger refname sorting by default in 'for-each-ref',
>> 'tag', and 'branch'.
>>
>> To match existing behavior as closely as possible, explicitly add "refname"
>> to the list of sort keys in 'for-each-ref', 'tag', and 'branch' before
>> parsing options (if no config-based sort keys are set). This ensures that
>> sorting will only be fully disabled if '--no-sort' is provided as an option;
>> otherwise, "refname" sorting will remain the default. Note: this also means
>> that even when sort keys are provided on the command line, "refname" will be
>> the final sort key in the sorting structure. This doesn't actually change
>> any behavior, since 'compare_refs()' already falls back on comparing
>> refnames if two refs are equal w.r.t all other sort keys.
>>
>> Finally, remove the condition around sorting in 'ls-remote', since it's no
>> longer necessary. Unlike 'for-each-ref' et. al., it does *not* set any sort
>> keys by default. The default empty list of sort keys will produce a NULL
>> 'struct ref_sorting *', which causes the sorting to be skipped in
>> 'ref_array_sort()'.
> 
> I found the order in this commit message a bit funny because you first
> explain what you're doing, then explain the goal, and then jump into the
> changes again. The message might be a bit easier to read if the goal was
> stated up front.

I'll try to restructure it.

> 
> I was also briefly wondering whether it would make sense to split up
> this commit, as you're doing two different things:
> 
>     - Refactor how git-for-each-ref(1), git-tag(1) and git-branch(1) set
>       up their default sorting.
> 
>     - Change `ref_array_sort()` to not sort when its sorting option is
>       `NULL`.
> 
> If this was split up into two commits, then the result might be a bit
> easier to reason about. But I don't feel strongly about this.

The addition of "refname" to the sorting defaults really only makes sense in
the context of needing it to update 'ref_array_sort()', though. While you
can convey some of that in a commit message, when reading through commits
(mine and others') I find it much easier to contextualize small refactors
with their associated behavior change if they're done in a single patch.
There's a limit to that, of course; even within this series I have a lot of
"this will make sense later" commit messages (more than I'd like really)
because the refactors are large & varied enough that they'd be overwhelming
if squashed into a single patch.

So, while I definitely see where you're coming from, I think this patch is
better off not being split.

>> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
>> index fc765754305..436249b720c 100644
>> --- a/builtin/ls-remote.c
>> +++ b/builtin/ls-remote.c
>> @@ -58,6 +58,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>>  	struct transport *transport;
>>  	const struct ref *ref;
>>  	struct ref_array ref_array;
>> +	struct ref_sorting *sorting;
>>  	struct string_list sorting_options = STRING_LIST_INIT_DUP;
>>  
>>  	struct option options[] = {
>> @@ -141,13 +142,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>>  		item->symref = xstrdup_or_null(ref->symref);
>>  	}
>>  
>> -	if (sorting_options.nr) {
>> -		struct ref_sorting *sorting;
>> -
>> -		sorting = ref_sorting_options(&sorting_options);
>> -		ref_array_sort(sorting, &ref_array);
>> -		ref_sorting_release(sorting);
>> -	}
>> +	sorting = ref_sorting_options(&sorting_options);
>> +	ref_array_sort(sorting, &ref_array);
> 
> We stopped calling `ref_sorting_release()`. Doesn't that cause us to
> leak memory?

Nice catch, thanks! It should have been moved to the end of this function
(right before the 'ref_array_clear()').

>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> index 3182abde27f..9918ba05dec 100755
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -1606,10 +1608,70 @@ test_expect_success 'option override configured sort' '
>>  	)
>>  '
>>  
>> +test_expect_success '--no-sort cancels config sort keys' '
>> +	test_config -C sort branch.sort "-refname" &&
>> +
>> +	(
>> +		cd sort &&
>> +
>> +		# objecttype is identical for all of them, so sort falls back on
>> +		# default (ascending refname)
>> +		git branch \
>> +			--no-sort \
>> +			--sort="objecttype" >actual &&
> 
> This test is a bit confusing to me. Shouldn't we in fact ignore the
> configured sorting order as soon as we pass `--sort=` anyway? In other
> words, I would expect the `--no-sort` option to not make a difference
> here. What should make a difference is if you _only_ passed `--no-sort`.

The existing behavior (as demonstrated by this test) is that the command
line sort keys append to, rather than replace, the config-based sort keys. I
don't see any evidence in the commit history to indicate that this was an
intentional design decision, but it's not necessarily incorrect either.

For one, it's not universal in string list options that the command line
replaces the config. There are examples of both approaches to string list
options in other commands:

- in 'git push', specifying '--push-option' on the command line even once
  will remove any values set by 'push.pushoption'
- in 'git blame', any values specified with '--ignore-revs-file' are
  appended to those set by 'blame.ignorerevsfile'

In the case of 'git (tag|branch)', I can see why users might not want
command line sort keys to completely remove config-based ones. The only time
the config-based keys will come into play is when two entries are identical
w.r.t _all_ of the command line sort keys. In that scenario, I'd expect a
user would want to use their configured defaults to "break the tie" instead
of the hardcoded ascending refname sort. If they do actually want to remove
the config keys, they can set '--no-sort' before their other sort keys.