On 2/6/25 12:59, Junio C Hamano wrote:
> Illia Bobyr <illia.bobyr@gmail.com> writes:
>
>> Most arguments have both short and long versions. Long versions are
>> easier to read, especially in scripts and command history.
>>
>> Tests that check just the option parsing are duplicated to check both
>> short and long argument options. But more complex tests are updated to
>> use the long argument in order to improve the test readability.
>
> While checking both may be a prudent thing to do, because the "-S"
> option and the "-G" option have been there with us almost since the
> beginning of time, the swapping all existing use of them with the
> longhand is rather unwelcome and needless churn, I would have to
> say.
My thinking is that as long version names improve readability, it also applies to the test code. When I see a short option, I often have to check the manual to remember what exactly does it do. Even for "-S"/"-G" I find myself sometimes confused as to which of the two does what exactly. While the "grep" mnemonic helps, I do not always remember it.
But, I think, I understand your point of view as well.
In v5, patch 5 contains a relatively minimum amount of changes that add long alternatives for "-S" and "-G" just to the command line parsing.
I do not have your experience with assessing the churn, but if my argument about the readability changes your mind, I've moved the rest of the updates into separate patches, at the end of the chain. Patches 8 through 10. Making it easier to discuss them in smaller chunks, if you wish so. But also, I assume, it should be easy for you to ignore those, if you do not want to include them?
> [...]
>
>> diff --git a/Documentation/gitdiffcore.txt
b/Documentation/gitdiffcore.txt
>> index 642c5..e4b18 100644
>> --- a/Documentation/gitdiffcore.txt
>> +++ b/Documentation/gitdiffcore.txt
>> @@ -245,33 +245,35 @@ diffcore-pickaxe: For Detecting
Addition/Deletion of Specified String
>>
>> This transformation limits the set of filepairs to those that change
>> specified strings between the preimage and the postimage in a certain
>> -way. -S<block-of-text> and -G<regular-expression> options are used to
>> +way. --patch-modifies=<block-of-text> and
>> +--patch-grep=<regular-expression> options are used to specify
>> +different ways these strings are sought.
>
> This is worse. Here is the first part that describes the pickaxe,
> so mentioning both may be more appropriate; showing only the
> longhand nobody is familiar with (yet) does not make any sense.
>
> ... certain way. `--patch-modifies=<block-of-text>`
> (`-S<block-of-text>` for short) and
`--patch-grep=<regular-expression>`
> (`-G<regular-expression>` for short) are used to ...
>
> Once establishing the equivalence between the longhand and the
> shorthand for these two options, we do not have to churn the
> existing text at all.
Applied your suggestion.
I guess one difference in the way you look at it, is that you default to the short version when you can. While I default to the long one, as I assume it is easier to understand. Someone not that intimately familiar with git might need to go to the previous paragraph to recall what "-S" and "-G" are, while if they are spelled as "--patch-modifies" and "--patch-grep", it might be less necessary. So, the argument is that while we are reducing the diff, we might also be reducing the improvement in readability.
Being the author, I could also be biased when assessing how much more readable "--patch-modifies" and "--patch-grep" are compared to "-S" and "-G".
>> diff --git a/diff.c b/diff.c
>> index d28b41..09beb 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -4877,15 +4877,17 @@ void diff_setup_done(struct diff_options
*options)
>>
>> if (HAS_MULTI_BITS(options->pickaxe_opts &
DIFF_PICKAXE_KINDS_MASK))
>> die(_("options '%s', '%s', and '%s' cannot be used together"),
>> - "-G", "-S", "--find-object");
>> + "-G/--patch-grep", "-S/--patch-modifies", "--find-object");
>>
>> if (HAS_MULTI_BITS(options->pickaxe_opts &
DIFF_PICKAXE_KINDS_G_REGEX_MASK))
>> die(_("options '%s' and '%s' cannot be used together, use
'%s' with '%s'"),
>> - "-G", "--pickaxe-regex", "--pickaxe-regex", "-S");
>> + "-G/--patch-grep", "--pickaxe-regex",
>> + "--pickaxe-regex", "-S/--patch-modifies");
>>
>> if (HAS_MULTI_BITS(options->pickaxe_opts &
DIFF_PICKAXE_KINDS_ALL_OBJFIND_MASK))
>> die(_("options '%s' and '%s' cannot be used together, use
'%s' with '%s' and '%s'"),
>> - "--pickaxe-all", "--find-object", "--pickaxe-all",
"-G", "-S");
>> + "--pickaxe-all", "--find-object",
>> + "--pickaxe-all", "-G/--patch-grep",
"-S/--patch-modifies");
>
> The message change looks fine; the indentation is broken.
>
> .git/rebase-apply/patch:184: indent with spaces.
> "--pickaxe-regex", "-S/--patch-modifies");
> .git/rebase-apply/patch:190: indent with spaces.
> "--pickaxe-all", "-G/--patch-grep",
"-S/--patch-modifies");
> warning: 2 lines applied after fixing whitespace errors.
> Applying: diff: --patch-{modifies,grep} arg names for -S and -G
>
> These alone do not require a new iteration, as "git am --whitespace=fix"
> already corrected them.Sorry about this. I did check the indentation manually, but did not use a tool. Reconfigured my editor to use tabs now.
>> - OPT_CALLBACK_F('S', NULL, options, N_("<string>"),
>> + OPT_CALLBACK_F('S', "patch-modifies", options, N_("<string>"),
>> - OPT_CALLBACK_F('G', NULL, options, N_("<regex>"),
>> + OPT_CALLBACK_F('G', "patch-grep", options, N_("<regex>"),
>
> OK. NOte that this says <regex>. We may want to have a separate
clean-up
> patch so that Documentation/gitdifcore.txt that used <regular-expression>
> and the placeholder used here match.Makes sense. I've added this fix as patch 5 in v5. I've reformatted paragraphs in gitdifcore.adoc that were affected. Let me know if you do not want me to reformat it, and just keep shorter lines as is.
>> - N_("look for differences that change the number
of occurrences of the specified regex"),
>> + N_("look for differences where a patch contains
the specified regex"),
>
> This is an unrelated change that should not be in this patch. If
> you want to modify it, please do it in a separate clean-up patch,
> just like the above <regex> vs <regular-expression> change.Split it into patch 2 in v5.
>> - N_("show all changes in the changeset with -S or -G"),
>> + N_("show all changes in the changeset with
-S/--patch-modifies or -G/--patch-grep"),
>
> This line is meant to be shown when the user requests list of
> options and their meanings. Growing the message from 47 columns or
> so to 78 columns would make it wider than terminals when these
> messages are indented. Because earlier entries in this array have
> already established the equivalence between the shorthand and the
> longhand, I do not think the output is understandable without this
> change.A description for "-S" is already 81 characters long:
N_("look for differences that change the number of occurrences of the specified regex"),
So I was assuming if I grow another description to 77 characters, it is still OK. While one can find the correspondence between "-S" and "--patch-modifies" by reading the "-S" description, in my mind, the same argument applies as to the test readability - it just makes it a bit easier for the reader.
This change is now part of a much smaller patch 9 in v5, which is only about adding longer alternatives to the CLI help messages that currently contain only "-G" and "-S". This way you can decide if you want it or not as a complete unit. Or if you want me to change it in some way, we can discuss it separately from the rest of the changes.
By the way, I must admit I can not find a way to look at a help message generated from these strings. Running `git diff -h` shows a message from
`diff.h` and running `git diff --help` shows the man page.
Thank you.
Illia Bobyr