[PATCH] diff: allow --recurse-submodules as an synonym for --submodule

[PATCH] diff: allow --recurse-submodules as an synonym for --submodule

[Stefan Beller]

Many commands have flags to recurse into submodules, which is named
--recurse-submodules. The diff family also has a submodule recursion flag,
but that is named differently. Add a synonym --recurse-submodules, which
means the same as the --submodule flag, such that across all git commands
supporting submodules we have the --recurse-submodules flag available.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/diff-options.txt | 1 +
 diff.c                         | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 0378cd574eb..694c97338c9 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -227,6 +227,7 @@ linkgit:git-config[1]).
 	of the `--diff-filter` option on what the status letters mean.
 
 --submodule[=<format>]::
+--recurse-submodules[=<format>]::
 	Specify how differences in submodules are shown.  When specifying
 	`--submodule=short` the 'short' format is used.  This format just
 	shows the names of the commits at the beginning and end of the range.
diff --git a/diff.c b/diff.c
index 145cfbae592..d3d5a989bd1 100644
--- a/diff.c
+++ b/diff.c
@@ -5023,6 +5023,8 @@ int diff_opt_parse(struct diff_options *options,
 		handle_ignore_submodules_arg(options, arg);
 	} else if (skip_to_optional_arg_default(arg, "--submodule", &arg, "log"))
 		return parse_submodule_opt(options, arg);
+	else if (skip_to_optional_arg_default(arg, "--recurse-submodules", &arg, "log"))
+		return parse_submodule_opt(options, arg);
 	else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
 		return parse_ws_error_highlight_opt(options, arg);
 	else if (!strcmp(arg, "--ita-invisible-in-index"))
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule

[Jonathan Nieder]

Stefan Beller wrote:

> Many commands have flags to recurse into submodules, which is named
> --recurse-submodules. The diff family also has a submodule recursion flag,
> but that is named differently. Add a synonym --recurse-submodules, which
> means the same as the --submodule flag, such that across all git commands
> supporting submodules we have the --recurse-submodules flag available.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/diff-options.txt | 1 +
>  diff.c                         | 2 ++
>  2 files changed, 3 insertions(+)

Makes sense.

[...]
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -227,6 +227,7 @@ linkgit:git-config[1]).
>  	of the `--diff-filter` option on what the status letters mean.
>  
>  --submodule[=<format>]::
> +--recurse-submodules[=<format>]::
>  	Specify how differences in submodules are shown.  When specifying
>  	`--submodule=short` the 'short' format is used.  This format just
>  	shows the names of the commits at the beginning and end of the range.
> diff --git a/diff.c b/diff.c
> index 145cfbae592..d3d5a989bd1 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -5023,6 +5023,8 @@ int diff_opt_parse(struct diff_options *options,
>  		handle_ignore_submodules_arg(options, arg);
>  	} else if (skip_to_optional_arg_default(arg, "--submodule", &arg, "log"))
>  		return parse_submodule_opt(options, arg);
> +	else if (skip_to_optional_arg_default(arg, "--recurse-submodules", &arg, "log"))
> +		return parse_submodule_opt(options, arg);

It seems like various commands are gaining --recurse-submodules options
taking different kinds of arguments:

- clone takes --recurse-submodules=<pathspec>
- fetch takes --recurse-submodules=<mode>
- after this patch, diff takes --recurse-submodules=<mode>

Is there a unifying principle?  Can Documentation/gitsubmodules.txt
say a word or two about what kind of argument values the user should
expect to be accepted by these options?

Thanks,
Jonathan

Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule

[Martin Ågren]

On Thu, 6 Sep 2018 at 00:59, Stefan Beller <sbeller@google.com> wrote:
>
>  --submodule[=<format>]::

Maybe drop `--submodule` here ...

> +--recurse-submodules[=<format>]::
>         Specify how differences in submodules are shown.  When specifying
>         `--submodule=short` the 'short' format is used.  This format just

... and use `--recurse-submodules` here ...

>         shows the names of the commits at the beginning and end of the range.

... and mention `--submodule` here as a historical alias? Maybe
deprecate it? I suppose the implementation of the aliasing is easy
enough that we can carry `--submodule` around forever, though.

> diff --git a/diff.c b/diff.c
> index 145cfbae592..d3d5a989bd1 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -5023,6 +5023,8 @@ int diff_opt_parse(struct diff_options *options,
>                 handle_ignore_submodules_arg(options, arg);
>         } else if (skip_to_optional_arg_default(arg, "--submodule", &arg, "log"))
>                 return parse_submodule_opt(options, arg);
> +       else if (skip_to_optional_arg_default(arg, "--recurse-submodules", &arg, "log"))
> +               return parse_submodule_opt(options, arg);

How about (whitespace-damaged)

} else if (skip_to_optional_arg_default(arg, "--submodule", &arg, "log") ||
           skip_to_optional_arg_default(arg, "--recurse-submodules",
&arg, "log"))
        return parse_submodule_opt(options, arg);

to make this future-proof? Sure, they're close enough that one should
notice the two instances, and any future work work would supposedly
happen in `parse_submodule_opt()` or anywhere else but here, but still.

Just a few thoughts.

Martin

[PATCH] diff: allow --recurse-submodules as an synonym for --submodule

[Stefan Beller]

Many commands have flags to recurse into submodules, which is named
--recurse-submodules. The diff family also has a submodule recursion flag,
but that is named differently. Add a synonym --recurse-submodules, which
means the same as the --submodule flag, such that across all git commands
supporting submodules we have the --recurse-submodules flag available.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/diff-options.txt | 21 +++++++++++----------
 diff.c                         |  3 ++-
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 0378cd574eb..28c6c7f939f 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -226,17 +226,18 @@ linkgit:git-config[1]).
 	Show only names and status of changed files. See the description
 	of the `--diff-filter` option on what the status letters mean.
 
---submodule[=<format>]::
+--recurse-submodules[=<format>]::
 	Specify how differences in submodules are shown.  When specifying
-	`--submodule=short` the 'short' format is used.  This format just
-	shows the names of the commits at the beginning and end of the range.
-	When `--submodule` or `--submodule=log` is specified, the 'log'
-	format is used.  This format lists the commits in the range like
-	linkgit:git-submodule[1] `summary` does.  When `--submodule=diff`
-	is specified, the 'diff' format is used.  This format shows an
-	inline diff of the changes in the submodule contents between the
-	commit range.  Defaults to `diff.submodule` or the 'short' format
-	if the config option is unset.
+	`--recurse-submodules=short` the 'short' format is used.  This format
+	just shows the names of the commits at the beginning and end of the
+	range. When `--recurse-submodules` or `--recurse-submodules=log` is
+	specified, the 'log' format is used.  This format lists the commits
+	in the range like linkgit:git-submodule[1] `summary` does.
+	When `--recurse-submodules=diff` is specified, the 'diff' format
+	is used. This format shows an inline diff of the changes in the
+	submodule contents between the commit range.  Defaults to
+	`diff.submodule` or the 'short' format if the config option is unset.
+	`--submodule[=<format>]` is a historic synonym for this option.
 
 --color[=<when>]::
 	Show colored diff.
diff --git a/diff.c b/diff.c
index 145cfbae592..b874f166c00 100644
--- a/diff.c
+++ b/diff.c
@@ -5021,7 +5021,8 @@ int diff_opt_parse(struct diff_options *options,
 	else if (skip_to_optional_arg_default(arg, "--ignore-submodules", &arg, "all")) {
 		options->flags.override_submodule_config = 1;
 		handle_ignore_submodules_arg(options, arg);
-	} else if (skip_to_optional_arg_default(arg, "--submodule", &arg, "log"))
+	} else if (skip_to_optional_arg_default(arg, "--submodule", &arg, "log") ||
+	    skip_to_optional_arg_default(arg, "--recurse-submodules", &arg, "log"))
 		return parse_submodule_opt(options, arg);
 	else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
 		return parse_ws_error_highlight_opt(options, arg);
-- 
2.19.0.rc2.392.g5ba43deb5a-goog

Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule

[Stefan Beller]

On Wed, Sep 5, 2018 at 4:13 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Stefan Beller wrote:
>
> > Many commands have flags to recurse into submodules, which is named
> > --recurse-submodules. The diff family also has a submodule recursion flag,
> > but that is named differently. Add a synonym --recurse-submodules, which
> > means the same as the --submodule flag, such that across all git commands
> > supporting submodules we have the --recurse-submodules flag available.
> >
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
> >  Documentation/diff-options.txt | 1 +
> >  diff.c                         | 2 ++
> >  2 files changed, 3 insertions(+)
>
> Makes sense.
>
> [...]
> > --- a/Documentation/diff-options.txt
> > +++ b/Documentation/diff-options.txt
> > @@ -227,6 +227,7 @@ linkgit:git-config[1]).
> >       of the `--diff-filter` option on what the status letters mean.
> >
> >  --submodule[=<format>]::
> > +--recurse-submodules[=<format>]::
> >       Specify how differences in submodules are shown.  When specifying
> >       `--submodule=short` the 'short' format is used.  This format just
> >       shows the names of the commits at the beginning and end of the range.
> > diff --git a/diff.c b/diff.c
> > index 145cfbae592..d3d5a989bd1 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -5023,6 +5023,8 @@ int diff_opt_parse(struct diff_options *options,
> >               handle_ignore_submodules_arg(options, arg);
> >       } else if (skip_to_optional_arg_default(arg, "--submodule", &arg, "log"))
> >               return parse_submodule_opt(options, arg);
> > +     else if (skip_to_optional_arg_default(arg, "--recurse-submodules", &arg, "log"))
> > +             return parse_submodule_opt(options, arg);
>
> It seems like various commands are gaining --recurse-submodules options
> taking different kinds of arguments:
>
> - clone takes --recurse-submodules=<pathspec>
> - fetch takes --recurse-submodules=<mode>
> - after this patch, diff takes --recurse-submodules=<mode>
>
> Is there a unifying principle?  Can Documentation/gitsubmodules.txt
> say a word or two about what kind of argument values the user should
> expect to be accepted by these options?

I don't think there is a unifying principle deep down.
Everything but clone is specifying a mode (you could imagine
that we extend "git reset --recurse-submodules" as well to take
some sort of mode, either the hard/soft differentiation or how to treat
dirty submodules or a mode that could differentiate between superprojects
sha1 and branch of the same name)

Care to send a patch on top to talk about these issues in
Documentation/gitsubmodules.txt ?

Thanks,
Stefan

Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> It seems like various commands are gaining --recurse-submodules options
> taking different kinds of arguments:
>
> - clone takes --recurse-submodules=<pathspec>
> - fetch takes --recurse-submodules=<mode>
> - after this patch, diff takes --recurse-submodules=<mode>
>
> Is there a unifying principle?  Can Documentation/gitsubmodules.txt
> say a word or two about what kind of argument values the user should
> expect to be accepted by these options?

I am not sure if the above is rhetorical.  The only thing such a
document can say about status-quo is that the user cannot make an
educated guess, as there is no consistency.  Some take an option to
clarify which subset of submodules to act on, others take an option
to specify what variant of operation to be made on the submodules.

In the ideal world, the users ought to be able to give these two
independently, i.e. "fetch" should be able to say "only fetch these
submodules" with pathspec while "run the fetch in all of these
submodules specified (with the pathspec) as necessary" with
"on-demand" mode, for example.

It may mean that it is too early to add "diff --recurse-submodules"
as a synonym for "diff --submodule", before what we can do to
improve the situation for commands that already take that
"--recurse-submodules" option.  A solution for _that_ problem might
result in splitting it into two options (an option to select which
submodule to recurse into, and another to specify the action that
happens in these submodules)---we'd regret if we realize that the
existing "diff --submodule" that specifies the action (i.e. how the
changes in submodules are presented, not which submodules' changes
are shown) does not match the one we choose for --recurse-submodules
option (e.g. we may say --recurse-submodules is for selection, and a
new --action-in-submodules is for action, in which case "diff"'s
"--submodule" option is closer to the latter).

Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule

[Stefan Beller]

On Thu, Sep 6, 2018 at 2:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
> > It seems like various commands are gaining --recurse-submodules options
> > taking different kinds of arguments:
> >
> > - clone takes --recurse-submodules=<pathspec>
> > - fetch takes --recurse-submodules=<mode>
> > - after this patch, diff takes --recurse-submodules=<mode>
> >
> > Is there a unifying principle?  Can Documentation/gitsubmodules.txt
> > say a word or two about what kind of argument values the user should
> > expect to be accepted by these options?
>
> I am not sure if the above is rhetorical.  The only thing such a
> document can say about status-quo is that the user cannot make an
> educated guess, as there is no consistency.  Some take an option to
> clarify which subset of submodules to act on, others take an option
> to specify what variant of operation to be made on the submodules.
>
> In the ideal world, the users ought to be able to give these two
> independently, i.e. "fetch" should be able to say "only fetch these
> submodules" with pathspec while "run the fetch in all of these
> submodules specified (with the pathspec) as necessary" with
> "on-demand" mode, for example.
>
> It may mean that it is too early to add "diff --recurse-submodules"
> as a synonym for "diff --submodule", before what we can do to
> improve the situation for commands that already take that
> "--recurse-submodules" option.

Good point. So let's retreat that patch for now?

Stefan

Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> It seems like various commands are gaining --recurse-submodules options
>> taking different kinds of arguments:
>>
>> - clone takes --recurse-submodules=<pathspec>
>> - fetch takes --recurse-submodules=<mode>
>> - after this patch, diff takes --recurse-submodules=<mode>
>>
>> Is there a unifying principle?  Can Documentation/gitsubmodules.txt
>> say a word or two about what kind of argument values the user should
>> expect to be accepted by these options?
>
> I am not sure if the above is rhetorical.  The only thing such a
> document can say about status-quo is that the user cannot make an
> educated guess, as there is no consistency.  Some take an option to
> clarify which subset of submodules to act on, others take an option
> to specify what variant of operation to be made on the submodules.

It's not rhetorical: I really do want to find out what our plan is for
the future of --recurse-submodules.

One possibility (A) would be "accept pathspec everywhere", as you
mentioned.  For a command like fetch that already accepts <mode>, that
is problematic and would involve a migration.  If we already have to
migrate fetch, migrating diff as well does not seem too bad.

Another possibility (B) would be "accept pathspec in clone only".  A
clone is a bit of a special case in that it is setting up the set of
active submodules; perhaps we want other commands to always respect
that set of active submodules without a method for overriding and only
acting on a subset.  After all, "git checkout <branch>" doesn't have a
way to only checkout <branch> on a subset of the worktree; why should
"git checkout --recurse-submodules <branch>" be any different?

When I think about it this way, I suspect that (B) will provide a
better experience than (A), so this diff change doesn't seem like a
step in the wrong direction.  Except: it took me a long time to think
this through.  Some documentation really would help, since it would
mean that the next person can understand what the *intention* behind
these options are and save some time thinking through other not-chosen
alternatives.

Hmm?

Thanks,
Jonathan

Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> When I think about it this way, I suspect that (B) will provide a
> better experience than (A), so this diff change doesn't seem like a
> step in the wrong direction.

OK, that's fair.