Re: [GSoC][RFC PATCH v3 1/3] builtin/refs: add list subcommand
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 24, 2025, 16:01 UTC
- Message-ID
- <xmqq4iv1k26x.fsf@gitster.g>
- In-Reply-To
- <aIHLdkhdVNy72Yf-@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 9 quoted lines
> On Wed, Jul 23, 2025 at 12:13:11PM +0530, Meet Soni wrote: >> diff --git a/Documentation/git-for-each-ref.adoc b/Documentation/git-for-each-ref.adoc >> index 5ef89fc0fe..f7bbc1902a 100644 >> --- a/Documentation/git-for-each-ref.adoc >> +++ b/Documentation/git-for-each-ref.adoc > > Tiny nit, not worth a reroll by itself: it would have been nice to move > the extraction of the common options from our docs into a separate, > preparatory commit.
True.
Show 24 quoted lines
>> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
>> index 3d2207ec77..d7d8279049 100644
>> --- a/builtin/for-each-ref.c
>> +++ b/builtin/for-each-ref.c
>> @@ -16,11 +16,27 @@ static char const * const for_each_ref_usage[] = {
>> NULL
>> };
>>
>> +#define REFS_LIST_USAGE \
>> + N_("git refs list [--count=<count>] [--shell|--perl|--python|--tcl]\n" \
>> + " [(--sort=<key>)...] [--format=<format>]\n" \
>> + " [--include-root-refs] [ --stdin | <pattern>... ]\n" \
>> + " [--points-at=<object>]\n" \
>> + " [--merged[=<object>]] [--no-merged[=<object>]]\n" \
>> + " [--contains[=<object>]] [--no-contains[=<object>]]\n" \
>> + " [--exclude=<pattern> ...]")
>> +
>> +static char const * const refs_list_usage[] = {
>> + REFS_LIST_USAGE,
>> + NULL
>> +};
>
> Shouldn't the usage strings for git-for-each-ref(1) and git-refs-list(1)
> be the same, except for the command name?A great observation, but where would it lead us? Something like
#define COMMON_USAGE_FOR_EACH_REF \
" [--count=<count>] [--shell|--perl|--python|--tcl]\n" \
" [(--sort=<key>)...] [--format=<format>]\n" \
... \
" [--exclude=<pattern> ...]" #define REFS_LIST_USAGE \
N_("git refs list" COMMON_USAGE_FOR_EACH_REF);
#define FOR_EACH_REF_USAGE \
N_("git for-each-ref" COMMON_USAGE_FOR_EACH_REF);is very much desirable because we do not want the options of these two commands drift apart, but it does not quite work with the current usage_with_options() infrastructure, as it leaves the indentation of second and subsequent lines up to the caller, and the display widths of "refs list" and "for-each-ref" are different.
Show 14 quoted lines
>> int cmd_for_each_ref(int argc,
>> const char **argv,
>> const char *prefix,
>> struct repository *repo)
>> {
>> + int cmd_is_refs_list = !strcmp(argv[0], "refs list");
>> + const char *const *opt_usage = cmd_is_refs_list ? refs_list_usage : for_each_ref_usage;
>> struct ref_sorting *sorting;
>> struct string_list sorting_options = STRING_LIST_INIT_DUP;
>> int icase = 0, include_root_refs = 0, from_stdin = 0;
>
> This follows the same pattern we have in "builtin/blame.c". It's not
> exactly pretty that git-for-each-ref(1) is aware of git-refs(1) now, but
> I think it's the pragmatic thing to do.Although it is yucky. argv[0] can never be "refs list" unless you are calling this as if it were a subroutine by hand, which is in general a no-no.
cf. https://lore.kernel.org/git/20171010040604.26029-1-gitster@pobox.com/
In this particular case, it is not like calling cmd_merge() from within a loop in cmd_rebase(), so many reasons against such program structure would not apply, but it would be prudent to leave an in-code comment to warn others not to mimic this pattern in general case.
Show 24 quoted lines
>> diff --git a/builtin/refs.c b/builtin/refs.c
>> index 998d2a2c1c..41e29d1b5f 100644
>> --- a/builtin/refs.c
>> +++ b/builtin/refs.c
>> @@ -13,6 +14,15 @@
>> #define REFS_VERIFY_USAGE \
>> N_("git refs verify [--strict] [--verbose]")
>>
>> +#define REFS_LIST_USAGE \
>> + N_("git refs list [--count=<count>] [--shell|--perl|--python|--tcl]\n" \
>> + " [(--sort=<key>)...] [--format=<format>]\n" \
>> + " [--include-root-refs] [ --stdin | <pattern>... ]\n" \
>> + " [--points-at=<object>]\n" \
>> + " [--merged[=<object>]] [--no-merged[=<object>]]\n" \
>> + " [--contains[=<object>]] [--no-contains[=<object>]]\n" \
>> + " [--exclude=<pattern> ...]")
>> +
>> static int cmd_refs_migrate(int argc, const char **argv, const char *prefix,
>> struct repository *repo UNUSED)
>> {
>
> Hm, this one is a bit unfortunate though, as it feels like it's just a
> matter of time before the two `REFS_LIST_USAGE` defines drift apart.
> Might be worth it to move them to a shared place.Yes, but see above.
> Alternatively, we could pull out the logic of `cmd_for_each_ref()` into > a separate function that also receives the usage array. Not sure whether > that is worth the hassle though.
I was also wondering about such restructuring of the existing program. Before adding a single line to support "refs list", can we move much of what cmd_for_each_ref() does that is common between it and upcoming "refs list" into a helper function, and make cmd_for_each_ref() to call it, as a preparatory step? Then a new "refs list" implementation does not have to touch the implementation of cmd_for_each_ref(), or call cmd_for_each_ref(). Which would give us much cleaner implementation, and we do not have to leave an ugly comment "we are doing this because we want to introduce 'refs list' synonym that behaves identically and calling cmd_for_each_ref() is the easiest way, but do not mimick it! It is an unacceptable pattern you should not follow in more general cases".
Thanks.