Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule
Jonathan Nieder 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.
Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule
Junio C Hamano wrote: > Jonathan Nieder writes: >> It seems like various commands are gaining --recurse-submodules options >> taking different kinds of arguments: >> >> - clone takes --recurse-submodules= >> - fetch takes --recurse-submodules= >> - after this patch, diff takes --recurse-submodules= >> >> 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 , 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 " doesn't have a way to only checkout on a subset of the worktree; why should "git checkout --recurse-submodules " 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
On Thu, Sep 6, 2018 at 2:12 PM Junio C Hamano wrote: > > Jonathan Nieder writes: > > > It seems like various commands are gaining --recurse-submodules options > > taking different kinds of arguments: > > > > - clone takes --recurse-submodules= > > - fetch takes --recurse-submodules= > > - after this patch, diff takes --recurse-submodules= > > > > 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 writes: > It seems like various commands are gaining --recurse-submodules options > taking different kinds of arguments: > > - clone takes --recurse-submodules= > - fetch takes --recurse-submodules= > - after this patch, diff takes --recurse-submodules= > > 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
On Wed, Sep 5, 2018 at 4:13 PM Jonathan Nieder 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 > > --- > > 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[=]:: > > +--recurse-submodules[=]:: > > 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", , > > "log")) > > return parse_submodule_opt(options, arg); > > + else if (skip_to_optional_arg_default(arg, "--recurse-submodules", > > , "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= > - fetch takes --recurse-submodules= > - after this patch, diff takes --recurse-submodules= > > 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
[PATCH] diff: allow --recurse-submodules as an synonym for --submodule
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
---
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[=]::
+--recurse-submodules[=]::
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[=]` is a historic synonym for this option.
--color[=]::
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", ,
"all")) {
options->flags.override_submodule_config = 1;
handle_ignore_submodules_arg(options, arg);
- } else if (skip_to_optional_arg_default(arg, "--submodule", ,
"log"))
+ } else if (skip_to_optional_arg_default(arg, "--submodule", ,
"log") ||
+ skip_to_optional_arg_default(arg, "--recurse-submodules", ,
"log"))
return parse_submodule_opt(options, arg);
else if (skip_prefix(arg, "--ws-error-highlight=", ))
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
On Thu, 6 Sep 2018 at 00:59, Stefan Beller wrote: > > --submodule[=]:: Maybe drop `--submodule` here ... > +--recurse-submodules[=]:: > 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", , > "log")) > return parse_submodule_opt(options, arg); > + else if (skip_to_optional_arg_default(arg, "--recurse-submodules", > , "log")) > + return parse_submodule_opt(options, arg); How about (whitespace-damaged) } else if (skip_to_optional_arg_default(arg, "--submodule", , "log") || skip_to_optional_arg_default(arg, "--recurse-submodules", , "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
Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule
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 > --- > 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[=]:: > +--recurse-submodules[=]:: > 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", , > "log")) > return parse_submodule_opt(options, arg); > + else if (skip_to_optional_arg_default(arg, "--recurse-submodules", > , "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= - fetch takes --recurse-submodules= - after this patch, diff takes --recurse-submodules= 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
[PATCH] diff: allow --recurse-submodules as an synonym for --submodule
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 --- 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[=]:: +--recurse-submodules[=]:: 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", , "log")) return parse_submodule_opt(options, arg); + else if (skip_to_optional_arg_default(arg, "--recurse-submodules", , "log")) + return parse_submodule_opt(options, arg); else if (skip_prefix(arg, "--ws-error-highlight=", )) return parse_ws_error_highlight_opt(options, arg); else if (!strcmp(arg, "--ita-invisible-in-index")) -- 2.19.0.rc2.392.g5ba43deb5a-goog
