Re: [PATCH v3 1/1] diff: --patch-{modifies,grep} arg names for -S and -G

[Junio C Hamano <gitster@pobox.com>]

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.

>  `-S<string>`::
> +`--patch-modifies=<string>`::

Good.  I am looking at 'git-commit.txt' as an example, and we seem
to give shorthand first and then longhand, which matches what we see
here.

> @@ -657,18 +658,19 @@ renamed entries cannot appear if detection for those types is disabled.
>  It is useful when you're looking for an exact block of code (like a
>  struct), and want to know the history of that block since it first
>  came into being: use the feature iteratively to feed the interesting
> -block in the preimage back into `-S`, and keep going until you get the
> -very first version of the block.
> +block in the preimage back into `--patch-modifies`, and keep going until
> +you get the very first version of the block.

If this paragraph _were_ written with the longhand from the
beginning, I would not have minded too much, but I personally find
it unnecessary to churn the existing document like this.

>  `-G<regex>`::
> +`--patch-grep=<regex>`::

Same two paragraphs from the above apply here, and ...

>  `--find-object=<object-id>`::
>  `--pickaxe-all`::
>  `--pickaxe-regex`::

... all of the above.

> diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt

Ditto.

> 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.

> 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.

> -		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.

> -			       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.

> -			  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.

>  		OPT_BIT_F(0, "pickaxe-regex", &options->pickaxe_opts,
> -			  N_("treat <string> in -S as extended POSIX regular expression"),
> +			  N_("treat <string> in -S/--patch-modifies as extended POSIX regular expression"),

Ditto.

Thanks.