threads / patch / 49287

patchdiff: allow --recurse-submodules as an synonym for --submodule

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

## tl;dr

9 messages between Sep 5, 2018 and Sep 7, 2018. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Stefan Beller· Sep 5, 2018, 22:58 UTC · lore

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(+)

Show changes to 2 files +3 −0

Documentation/diff-options.txt, diff.c

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

Jonathan Nieder· Sep 5, 2018, 23:12 UTC · re: Stefan Beller · lore

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

Stefan Beller wrote:

Show 11 quoted lines

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

[...]

Show 20 quoted lines

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

Stefan Beller· Sep 6, 2018, 18:27 UTC · re: Jonathan Nieder · lore

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

On Wed, Sep 5, 2018 at 4:13 PM Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 49 quoted lines

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

Junio C Hamano· Sep 6, 2018, 21:12 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 10 quoted lines

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

Stefan Beller· Sep 6, 2018, 21:25 UTC · re: Junio C Hamano · lore

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

On Thu, Sep 6, 2018 at 2:12 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 30 quoted lines

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

Jonathan Nieder· Sep 7, 2018, 06:40 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:

Show 16 quoted lines

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

Junio C Hamano· Sep 7, 2018, 09:53 UTC · re: Jonathan Nieder · lore

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

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.

Martin Ågren· Sep 6, 2018, 06:19 UTC · re: Stefan Beller · lore

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

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.

Show 10 quoted lines

> 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

Stefan Beller· Sep 6, 2018, 18:23 UTC · re: Martin Ågren · lore

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

Show changes to 2 files +13 −10

Documentation/diff-options.txt, diff.c

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