Patrick Steinhardt <ps@xxxxxx> writes:
> 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.
>> 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.
>> 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@xxxxxxxxx/
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.
>> 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.
