for-each-ref optimizations & usability improvements #1609
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,7 +45,6 @@ static const char *head; | |
| static struct object_id head_oid; | ||
| static int recurse_submodules = 0; | ||
| static int submodule_propagate_branches = 0; | ||
|
|
||
| static int branch_use_color = -1; | ||
| static char branch_colors[][COLOR_MAXLEN] = { | ||
|
|
@@ -438,8 +437,6 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin | |
| { | ||
|
There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This consolidates much of the code used to filter and format refs in
> 'builtin/for-each-ref.c', 'builtin/tag.c', and 'builtin/branch.c', reducing
> duplication and simplifying the future changes needed to optimize the filter
> & format process.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> builtin/branch.c | 33 +++++++++++++++++----------------
> builtin/for-each-ref.c | 27 +--------------------------
> builtin/tag.c | 23 +----------------------
> ref-filter.c | 35 +++++++++++++++++++++++++++++++++++
> ref-filter.h | 14 ++++++++++++++
> 5 files changed, 68 insertions(+), 64 deletions(-)
The amount of existing duplication of code is rather surprising, and
this patch nicely refactors to improve. Good.
Thanks. |
||
| int i; | ||
| struct ref_array array; | ||
| int maxwidth = 0; | ||
| const char *remote_prefix = ""; | ||
| char *to_free = NULL; | ||
|
|
@@ -469,24 +466,27 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin | |
| filter_ahead_behind(the_repository, format, &array); | ||
| ref_array_sort(sorting, &array); | ||
|
|
||
| if (column_active(colopts)) { | ||
| struct strbuf out = STRBUF_INIT, err = STRBUF_INIT; | ||
|
|
||
| assert(!filter->verbose && "--column and --verbose are incompatible"); | ||
|
|
||
| for (i = 0; i < array.nr; i++) { | ||
| strbuf_reset(&err); | ||
| strbuf_reset(&out); | ||
| if (format_ref_array_item(array.items[i], format, &out, &err)) | ||
| die("%s", err.buf); | ||
|
|
||
| /* format to a string_list to let print_columns() do its job */ | ||
| string_list_append(output, out.buf); | ||
| } | ||
|
|
||
| strbuf_release(&err); | ||
| strbuf_release(&out); | ||
| } else { | ||
| print_formatted_ref_array(&array, format); | ||
| } | ||
|
|
||
| ref_array_clear(&array); | ||
| free(to_free); | ||
| } | ||
|
|
@@ -737,7 +737,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) | |
| OPT_BIT('D', NULL, &delete, N_("delete branch (even if not merged)"), 2), | ||
| OPT_BIT('m', "move", &rename, N_("move/rename a branch and its reflog"), 1), | ||
| OPT_BIT('M', NULL, &rename, N_("move/rename a branch, even if target exists"), 2), | ||
| OPT_BOOL(0, "omit-empty", &format.array_opts.omit_empty, | ||
| N_("do not output a newline after empty formatted refs")), | ||
| OPT_BIT('c', "copy", ©, N_("copy a branch and its reflog"), 1), | ||
| OPT_BIT('C', NULL, ©, N_("copy a branch, even if target exists"), 2), | ||
|
|
@@ -767,7 +767,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix) | |
| if (argc == 2 && !strcmp(argv[1], "-h")) | ||
|
There was a problem hiding this comment. On the Git mailing list, Patrick Steinhardt wrote (reply to this): 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 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.
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> builtin/branch.c | 6 ++++
> builtin/for-each-ref.c | 3 ++
> builtin/ls-remote.c | 10 ++----
> builtin/tag.c | 6 ++++
> ref-filter.c | 19 ++----------
> t/t3200-branch.sh | 68 +++++++++++++++++++++++++++++++++++++++--
> t/t6300-for-each-ref.sh | 21 +++++++++++++
> t/t7004-tag.sh | 45 +++++++++++++++++++++++++++
> 8 files changed, 152 insertions(+), 26 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index e7ee9bd0f15..d67738bbcaa 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -767,7 +767,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage_with_options(builtin_branch_usage, options);
>
> + /*
> + * Try to set sort keys from config. If config does not set any,
> + * fall back on default (refname) sorting.
> + */
> git_config(git_branch_config, &sorting_options);
> + if (!sorting_options.nr)
> + string_list_append(&sorting_options, "refname");
>
> track = git_branch_track;
>
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 350bfa6e811..93b370f550b 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -67,6 +67,9 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>
> git_config(git_default_config, NULL);
>
> + /* Set default (refname) sorting */
> + string_list_append(&sorting_options, "refname");
> +
> parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0);
> if (maxcount < 0) {
> error("invalid --count argument: `%d'", maxcount);
> 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?
> for (i = 0; i < ref_array.nr; i++) {
> const struct ref_array_item *ref = ref_array.items[i];
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 3918eacbb57..64f3196cd4c 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -501,7 +501,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>
> setup_ref_filter_porcelain_msg();
>
> + /*
> + * Try to set sort keys from config. If config does not set any,
> + * fall back on default (refname) sorting.
> + */
> git_config(git_tag_config, &sorting_options);
> + if (!sorting_options.nr)
> + string_list_append(&sorting_options, "refname");
>
> memset(&opt, 0, sizeof(opt));
> filter.lines = -1;
> diff --git a/ref-filter.c b/ref-filter.c
> index e4d3510e28e..7250089b7c6 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -3142,7 +3142,8 @@ void ref_sorting_set_sort_flags_all(struct ref_sorting *sorting,
>
> void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
> {
> - QSORT_S(array->items, array->nr, compare_refs, sorting);
> + if (sorting)
> + QSORT_S(array->items, array->nr, compare_refs, sorting);
> }
>
> static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)
> @@ -3248,18 +3249,6 @@ static int parse_sorting_atom(const char *atom)
> return res;
> }
>
> -/* If no sorting option is given, use refname to sort as default */
> -static struct ref_sorting *ref_default_sorting(void)
> -{
> - static const char cstr_name[] = "refname";
> -
> - struct ref_sorting *sorting = xcalloc(1, sizeof(*sorting));
> -
> - sorting->next = NULL;
> - sorting->atom = parse_sorting_atom(cstr_name);
> - return sorting;
> -}
> -
> static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
> {
> struct ref_sorting *s;
> @@ -3283,9 +3272,7 @@ struct ref_sorting *ref_sorting_options(struct string_list *options)
> struct string_list_item *item;
> struct ref_sorting *sorting = NULL, **tail = &sorting;
>
> - if (!options->nr) {
> - sorting = ref_default_sorting();
> - } else {
> + if (options->nr) {
> for_each_string_list_item(item, options)
> parse_ref_sorting(tail, item->string);
> }
> 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
> @@ -1570,9 +1570,10 @@ test_expect_success 'tracking with unexpected .fetch refspec' '
>
> test_expect_success 'configured committerdate sort' '
> git init -b main sort &&
> + test_config -C sort branch.sort "committerdate" &&
> +
> (
> cd sort &&
> - git config branch.sort committerdate &&
> test_commit initial &&
> git checkout -b a &&
> test_commit a &&
> @@ -1592,9 +1593,10 @@ test_expect_success 'configured committerdate sort' '
> '
>
> test_expect_success 'option override configured sort' '
> + test_config -C sort branch.sort "committerdate" &&
> +
> (
> cd sort &&
> - git config branch.sort committerdate &&
> git branch --sort=refname >actual &&
> cat >expect <<-\EOF &&
> a
> @@ -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`.
> + cat >expect <<-\EOF &&
> + a
> + * b
> + c
> + main
> + EOF
> + test_cmp expect actual
> + )
> +
> +'
> +
> +test_expect_success '--no-sort cancels command line sort keys' '
> + (
> + cd sort &&
> +
> + # objecttype is identical for all of them, so sort falls back on
> + # default (ascending refname)
> + git branch \
> + --sort="-refname" \
> + --no-sort \
> + --sort="objecttype" >actual &&
> + cat >expect <<-\EOF &&
> + a
> + * b
> + c
> + main
> + EOF
> + test_cmp expect actual
> + )
> +'
> +
> +test_expect_success '--no-sort without subsequent --sort prints expected branches' '
> + (
> + cd sort &&
> +
> + # Sort the results with `sort` for a consistent comparison
> + # against expected
> + git branch --no-sort | sort >actual &&
> + cat >expect <<-\EOF &&
> + a
> + c
> + main
> + * b
> + EOF
> + test_cmp expect actual
> + )
> +'
> +
> test_expect_success 'invalid sort parameter in configuration' '
> + test_config -C sort branch.sort "v:notvalid" &&
> +
> (
> cd sort &&
> - git config branch.sort "v:notvalid" &&
>
> # this works in the "listing" mode, so bad sort key
> # is a dying offence.
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 00a060df0b5..0613e5e3623 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -1335,6 +1335,27 @@ test_expect_success '--no-sort cancels the previous sort keys' '
> test_cmp expected actual
> '
>
> +test_expect_success '--no-sort without subsequent --sort prints expected refs' '
> + cat >expected <<-\EOF &&
> + refs/tags/multi-ref1-100000-user1
> + refs/tags/multi-ref1-100000-user2
> + refs/tags/multi-ref1-200000-user1
> + refs/tags/multi-ref1-200000-user2
> + refs/tags/multi-ref2-100000-user1
> + refs/tags/multi-ref2-100000-user2
> + refs/tags/multi-ref2-200000-user1
> + refs/tags/multi-ref2-200000-user2
> + EOF
> +
> + # Sort the results with `sort` for a consistent comparison against
> + # expected
> + git for-each-ref \
> + --format="%(refname)" \
> + --no-sort \
> + "refs/tags/multi-*" | sort >actual &&
> + test_cmp expected actual
> +'
> +
> test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
> test_when_finished "git checkout main" &&
> git for-each-ref --format="%(HEAD) %(refname:short)" refs/heads/ >actual &&
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index e689db42929..b41a47eb943 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1862,6 +1862,51 @@ test_expect_success 'option override configured sort' '
> test_cmp expect actual
> '
>
> +test_expect_success '--no-sort cancels config sort keys' '
> + test_config tag.sort "-refname" &&
> +
> + # objecttype is identical for all of them, so sort falls back on
> + # default (ascending refname)
> + git tag -l \
> + --no-sort \
> + --sort="objecttype" \
> + "foo*" >actual &&
> + cat >expect <<-\EOF &&
> + foo1.10
> + foo1.3
> + foo1.6
> + EOF
> + test_cmp expect actual
> +'
Same question here.
Patrick
> +test_expect_success '--no-sort cancels command line sort keys' '
> + # objecttype is identical for all of them, so sort falls back on
> + # default (ascending refname)
> + git tag -l \
> + --sort="-refname" \
> + --no-sort \
> + --sort="objecttype" \
> + "foo*" >actual &&
> + cat >expect <<-\EOF &&
> + foo1.10
> + foo1.3
> + foo1.6
> + EOF
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--no-sort without subsequent --sort prints expected tags' '
> + # Sort the results with `sort` for a consistent comparison against
> + # expected
> + git tag -l --no-sort "foo*" | sort >actual &&
> + cat >expect <<-\EOF &&
> + foo1.10
> + foo1.3
> + foo1.6
> + EOF
> + test_cmp expect actual
> +'
> +
> test_expect_success 'invalid sort parameter on command line' '
> test_must_fail git tag -l --sort=notvalid "foo*" >actual
> '
> --
> gitgitgadget
>
> There was a problem hiding this comment. On the Git mailing list, Victoria Dye wrote (reply to this): 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.
There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Victoria Dye <vdye@github.com>
>
> When '--no-sort' is passed to 'for-each-ref', 'tag', and 'branch', the
> printed refs are still sorted by ascending refname. Change the handling of
> sort options in these commands so that '--no-sort' to truly disables
> sorting.
"to truly disables" -> "truly disables" I think?
> '--no-sort' does not disable sorting in these commands is because their
"'--no-sort' does not" -> "The reason why '--no-sort' does not", or
"is because" -> "because".
> option parsing does not distinguish between "the absence of '--sort'"
> (and/or values for tag.sort & branch.sort) and '--no-sort'. Both result in
> an empty 'sorting_options' string list, which is parsed by
> 'ref_sorting_options()' to create the 'struct ref_sorting *' for the
> command. If the string list is empty, 'ref_sorting_options()' interprets
> that as "the absence of '--sort'" and returns the default ref sorting
> structure (equivalent to "refname" sort).
>
> To handle '--no-sort' properly while preserving the "refname" sort in the
> "absence of --sort'" case, first explicitly add "refname" to the string list
> *before* parsing options. This alone 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.
>
> Now that the string list is populated by default, '--no-sort' is the only
> way to empty the 'sorting_options' string list. Update
> 'ref_sorting_options()' to return a NULL 'struct ref_sorting *' if the
> string list is empty, and add a condition to 'ref_array_sort()' to skip the
> sort altogether if the sort structure is NULL. Note that other functions
> using 'struct ref_sorting *' do not need any changes because they already
> ignore NULL values.
Nice.
> Finally, remove the condition around sorting in 'ls-remote', since it's no
> longer necessary. Unlike 'for-each-ref' et. al., it does *not* do any
> sorting by default. This default is preserved by simply leaving its sort key
> string list empty before parsing options; if no additional sort keys are
> set, 'struct ref_sorting *' is NULL and sorting is skipped.
Doubly nice.
> diff --git a/ref-filter.c b/ref-filter.c
> index e4d3510e28e..7250089b7c6 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -3142,7 +3142,8 @@ void ref_sorting_set_sort_flags_all(struct ref_sorting *sorting,
>
> void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
> {
> - QSORT_S(array->items, array->nr, compare_refs, sorting);
> + if (sorting)
> + QSORT_S(array->items, array->nr, compare_refs, sorting);
> }
Nice. We do allow passing NULL to ref_sorting_release(), and we can
return NULL from ref_sorting_options(), and allowing NULL to be
passed to this function makes it easier for the callers to deal with
the case where no sorting is specified.
> 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
> @@ -1570,9 +1570,10 @@ test_expect_success 'tracking with unexpected .fetch refspec' '
>
> test_expect_success 'configured committerdate sort' '
> git init -b main sort &&
> + test_config -C sort branch.sort "committerdate" &&
> +
> (
> cd sort &&
> - git config branch.sort committerdate &&
> test_commit initial &&
> git checkout -b a &&
> test_commit a &&
> @@ -1592,9 +1593,10 @@ test_expect_success 'configured committerdate sort' '
> '
>
> test_expect_success 'option override configured sort' '
> + test_config -C sort branch.sort "committerdate" &&
> +
> (
> cd sort &&
> - git config branch.sort committerdate &&
> git branch --sort=refname >actual &&
> cat >expect <<-\EOF &&
> a
The above two are not strictly necessary for the purpose of this
patch, in that the tests that come after these tests do not care if
the branch.sort configuration variable is set in the "sort"
repository, as they set their own value before doing their test.
But of course, cleaning up after yourself with test_config and
friends is a good idea regardless, and a handful of new tests added
after this point follow the same pattern. Good.
> @@ -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)
Interesting.
> + git branch \
> + --no-sort \
> + --sort="objecttype" >actual &&
> + cat >expect <<-\EOF &&
> + a
> + * b
> + c
> + main
> + EOF
> + test_cmp expect actual
> + )
> +
> +'
> +
> +test_expect_success '--no-sort cancels command line sort keys' '
> + (
> + cd sort &&
> +
> + # objecttype is identical for all of them, so sort falls back on
> + # default (ascending refname)
> + git branch \
> + --sort="-refname" \
> + --no-sort \
> + --sort="objecttype" >actual &&
OK, this exercises the same "--no-sort cleans the slate" as before,
and for this one it is essential that we lack branch.sort after the
previous step is done, which is ensured thanks to the use of
test_config in the previous one. Nice.
> + cat >expect <<-\EOF &&
> + a
> + * b
> + c
> + main
> + EOF
> + test_cmp expect actual
> + )
> +'
> +
> +test_expect_success '--no-sort without subsequent --sort prints expected branches' '
> + (
> + cd sort &&
> +
> + # Sort the results with `sort` for a consistent comparison
> + # against expected
> + git branch --no-sort | sort >actual &&
> + cat >expect <<-\EOF &&
> + a
> + c
> + main
> + * b
> + EOF
> + test_cmp expect actual
> + )
> +'
> +
> test_expect_success 'invalid sort parameter in configuration' '
> + test_config -C sort branch.sort "v:notvalid" &&
> +
> (
> cd sort &&
> - git config branch.sort "v:notvalid" &&
>
> # this works in the "listing" mode, so bad sort key
> # is a dying offence.
With such an invalid configuration value set, running the command
with "--no-sort" would stop the command from failing? Is that worth
protecting with a new test, I wonder.
Overall very nicely done.
Thanks. |
||
| usage_with_options(builtin_branch_usage, options); | ||
|
|
||
| /* | ||
| * Try to set sort keys from config. If config does not set any, | ||
| * fall back on default (refname) sorting. | ||
| */ | ||
| git_config(git_branch_config, &sorting_options); | ||
| if (!sorting_options.nr) | ||
| string_list_append(&sorting_options, "refname"); | ||
|
|
||
| track = git_branch_track; | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2764,8 +2764,6 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname) | |
| struct ref_filter_cbdata { | ||
|
There was a problem hiding this comment. On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Tue, Nov 07, 2023 at 01:25:56AM +0000, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
>
> Move the 'contains_cache' and 'no_contains_cache' used in filter_refs into
> an 'internal' struct of the 'struct ref_filter'. In later patches, the
> 'struct ref_filter *' will be a common data structure across multiple
> filtering functions. These caches are part of the common functionality the
> filter struct will support, so they are updated to be internally accessible
> wherever the filter is used.
>
> The design used here is mirrors what was introduced in 576de3d956
Nit: s/is //
Patrick
> (unpack_trees: start splitting internal fields from public API, 2023-02-27)
> for 'unpack_trees_options'.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> ref-filter.c | 14 ++++++--------
> ref-filter.h | 6 ++++++
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 7250089b7c6..5129b6986c9 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2764,8 +2764,6 @@ static int filter_ref_kind(struct ref_filter *filter, const char *refname)
> struct ref_filter_cbdata {
> struct ref_array *array;
> struct ref_filter *filter;
> - struct contains_cache contains_cache;
> - struct contains_cache no_contains_cache;
> };
>
> /*
> @@ -2816,11 +2814,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
> return 0;
> /* We perform the filtering for the '--contains' option... */
> if (filter->with_commit &&
> - !commit_contains(filter, commit, filter->with_commit, &ref_cbdata->contains_cache))
> + !commit_contains(filter, commit, filter->with_commit, &filter->internal.contains_cache))
> return 0;
> /* ...or for the `--no-contains' option */
> if (filter->no_commit &&
> - commit_contains(filter, commit, filter->no_commit, &ref_cbdata->no_contains_cache))
> + commit_contains(filter, commit, filter->no_commit, &filter->internal.no_contains_cache))
> return 0;
> }
>
> @@ -2989,8 +2987,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
> save_commit_buffer_orig = save_commit_buffer;
> save_commit_buffer = 0;
>
> - init_contains_cache(&ref_cbdata.contains_cache);
> - init_contains_cache(&ref_cbdata.no_contains_cache);
> + init_contains_cache(&filter->internal.contains_cache);
> + init_contains_cache(&filter->internal.no_contains_cache);
>
> /* Simple per-ref filtering */
> if (!filter->kind)
> @@ -3014,8 +3012,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
> head_ref(ref_filter_handler, &ref_cbdata);
> }
>
> - clear_contains_cache(&ref_cbdata.contains_cache);
> - clear_contains_cache(&ref_cbdata.no_contains_cache);
> + clear_contains_cache(&filter->internal.contains_cache);
> + clear_contains_cache(&filter->internal.no_contains_cache);
>
> /* Filters that need revision walking */
> reach_filter(array, &filter->reachable_from, INCLUDE_REACHED);
> diff --git a/ref-filter.h b/ref-filter.h
> index d87d61238b7..0db3ff52889 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -7,6 +7,7 @@
> #include "commit.h"
> #include "string-list.h"
> #include "strvec.h"
> +#include "commit-reach.h"
>
> /* Quoting styles */
> #define QUOTE_NONE 0
> @@ -75,6 +76,11 @@ struct ref_filter {
> lines;
> int abbrev,
> verbose;
> +
> + struct {
> + struct contains_cache contains_cache;
> + struct contains_cache no_contains_cache;
> + } internal;
> };
>
> struct ref_format {
> --
> gitgitgadget
>
> |
||
| struct ref_array *array; | ||
| struct ref_filter *filter; | ||
| }; | ||
|
|
||
| /* | ||
|
|
@@ -2816,11 +2814,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid, | |
| return 0; | ||
| /* We perform the filtering for the '--contains' option... */ | ||
| if (filter->with_commit && | ||
| !commit_contains(filter, commit, filter->with_commit, &filter->internal.contains_cache)) | ||
| return 0; | ||
| /* ...or for the `--no-contains' option */ | ||
| if (filter->no_commit && | ||
| commit_contains(filter, commit, filter->no_commit, &filter->internal.no_contains_cache)) | ||
| return 0; | ||
| } | ||
|
|
||
|
|
@@ -2989,8 +2987,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int | |
| save_commit_buffer_orig = save_commit_buffer; | ||
| save_commit_buffer = 0; | ||
|
|
||
| init_contains_cache(&filter->internal.contains_cache); | ||
| init_contains_cache(&filter->internal.no_contains_cache); | ||
|
|
||
| /* Simple per-ref filtering */ | ||
| if (!filter->kind) | ||
|
|
@@ -3014,8 +3012,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int | |
| head_ref(ref_filter_handler, &ref_cbdata); | ||
| } | ||
|
|
||
| clear_contains_cache(&filter->internal.contains_cache); | ||
| clear_contains_cache(&filter->internal.no_contains_cache); | ||
|
|
||
| /* Filters that need revision walking */ | ||
| reach_filter(array, &filter->reachable_from, INCLUDE_REACHED); | ||
|
|
@@ -3025,6 +3023,18 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int | |
| return ret; | ||
| } | ||
|
|
||
| void filter_and_format_refs(struct ref_filter *filter, unsigned int type, | ||
| struct ref_sorting *sorting, | ||
| struct ref_format *format) | ||
| { | ||
| struct ref_array array = { 0 }; | ||
| filter_refs(&array, filter, type); | ||
| filter_ahead_behind(the_repository, format, &array); | ||
| ref_array_sort(sorting, &array); | ||
| print_formatted_ref_array(&array, format); | ||
| ref_array_clear(&array); | ||
| } | ||
|
|
||
| static int compare_detached_head(struct ref_array_item *a, struct ref_array_item *b) | ||
| { | ||
| if (!(a->kind ^ b->kind)) | ||
|
|
@@ -3142,7 +3152,8 @@ void ref_sorting_set_sort_flags_all(struct ref_sorting *sorting, | |
|
|
||
| void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array) | ||
| { | ||
| if (sorting) | ||
| QSORT_S(array->items, array->nr, compare_refs, sorting); | ||
| } | ||
|
|
||
| static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state) | ||
|
|
@@ -3213,6 +3224,29 @@ int format_ref_array_item(struct ref_array_item *info, | |
| return 0; | ||
| } | ||
|
|
||
| void print_formatted_ref_array(struct ref_array *array, struct ref_format *format) | ||
| { | ||
| int total; | ||
| struct strbuf output = STRBUF_INIT, err = STRBUF_INIT; | ||
|
|
||
| total = format->array_opts.max_count; | ||
| if (!total || array->nr < total) | ||
| total = array->nr; | ||
| for (int i = 0; i < total; i++) { | ||
| strbuf_reset(&err); | ||
| strbuf_reset(&output); | ||
| if (format_ref_array_item(array->items[i], format, &output, &err)) | ||
| die("%s", err.buf); | ||
| if (output.len || !format->array_opts.omit_empty) { | ||
| fwrite(output.buf, 1, output.len, stdout); | ||
| putchar('\n'); | ||
| } | ||
| } | ||
|
|
||
| strbuf_release(&err); | ||
| strbuf_release(&output); | ||
| } | ||
|
|
||
| void pretty_print_ref(const char *name, const struct object_id *oid, | ||
| struct ref_format *format) | ||
| { | ||
|
|
@@ -3248,18 +3282,6 @@ static int parse_sorting_atom(const char *atom) | |
| return res; | ||
| } | ||
|
|
||
| static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg) | ||
| { | ||
| struct ref_sorting *s; | ||
|
|
@@ -3283,9 +3305,7 @@ struct ref_sorting *ref_sorting_options(struct string_list *options) | |
| struct string_list_item *item; | ||
| struct ref_sorting *sorting = NULL, **tail = &sorting; | ||
|
|
||
| if (options->nr) { | ||
| for_each_string_list_item(item, options) | ||
| parse_ref_sorting(tail, item->string); | ||
| } | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Øystein Walle wrote (reply to this):