{"thread":{"id":"49287","subject":"[PATCH] diff: allow --recurse-submodules as an synonym for --submodule","startedAt":"2018-09-05T22:58:34Z","lastAt":"2018-09-07T09:53:55Z","messageCount":9,"participants":["Stefan Beller","Jonathan Nieder","Martin Ågren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"357491","messageId":"20180905225828.17782-1-sbeller@google.com","threadId":"49287","inReplyTo":null,"subject":"[PATCH] diff: allow --recurse-submodules as an synonym for --submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-05T22:58:28Z","receivedAt":"2018-09-05T22:58:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Many commands have flags to recurse into submodules, which is named\n--recurse-submodules. The diff family also has a submodule recursion flag,\nbut that is named differently. Add a synonym --recurse-submodules, which\nmeans the same as the --submodule flag, such that across all git commands\nsupporting submodules we have the --recurse-submodules flag available.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/diff-options.txt | 1 +\n diff.c | 2 ++\n 2 files changed, 3 insertions(+)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 0378cd574eb..694c97338c9 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -227,6 +227,7 @@ linkgit:git-config[1]).\n \tof the `--diff-filter` option on what the status letters mean.\n \n --submodule[=]::\n+--recurse-submodules[=]::\n \tSpecify how differences in submodules are shown. When specifying\n \t`--submodule=short` the 'short' format is used. This format just\n \tshows the names of the commits at the beginning and end of the range.\ndiff --git a/diff.c b/diff.c\nindex 145cfbae592..d3d5a989bd1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5023,6 +5023,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\thandle_ignore_submodules_arg(options, arg);\n \t} else if (skip_to_optional_arg_default(arg, \"--submodule\", &arg, \"log\"))\n \t\treturn parse_submodule_opt(options, arg);\n+\telse if (skip_to_optional_arg_default(arg, \"--recurse-submodules\", &arg, \"log\"))\n+\t\treturn parse_submodule_opt(options, arg);\n \telse if (skip_prefix(arg, \"--ws-error-highlight=\", &arg))\n \t\treturn parse_ws_error_highlight_opt(options, arg);\n \telse if (!strcmp(arg, \"--ita-invisible-in-index\"))\n-- \n2.19.0.rc2.392.g5ba43deb5a-goog\n\n"},{"id":"357493","messageId":"20180905231258.GD120842@aiede.svl.corp.google.com","threadId":"49287","inReplyTo":"20180905225828.17782-1-sbeller@google.com","subject":"Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-05T23:12:58Z","receivedAt":"2018-09-05T23:13:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Many commands have flags to recurse into submodules, which is named\n> --recurse-submodules. The diff family also has a submodule recursion flag,\n> but that is named differently. Add a synonym --recurse-submodules, which\n> means the same as the --submodule flag, such that across all git commands\n> supporting submodules we have the --recurse-submodules flag available.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> Documentation/diff-options.txt | 1 +\n> diff.c | 2 ++\n> 2 files changed, 3 insertions(+)\n\nMakes sense.\n\n[...]\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -227,6 +227,7 @@ linkgit:git-config[1]).\n> \tof the `--diff-filter` option on what the status letters mean.\n> \n> --submodule[=]::\n> +--recurse-submodules[=]::\n> \tSpecify how differences in submodules are shown. When specifying\n> \t`--submodule=short` the 'short' format is used. This format just\n> \tshows the names of the commits at the beginning and end of the range.\n> diff --git a/diff.c b/diff.c\n> index 145cfbae592..d3d5a989bd1 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -5023,6 +5023,8 @@ int diff_opt_parse(struct diff_options *options,\n> \t\thandle_ignore_submodules_arg(options, arg);\n> \t} else if (skip_to_optional_arg_default(arg, \"--submodule\", &arg, \"log\"))\n> \t\treturn parse_submodule_opt(options, arg);\n> +\telse if (skip_to_optional_arg_default(arg, \"--recurse-submodules\", &arg, \"log\"))\n> +\t\treturn parse_submodule_opt(options, arg);\n\nIt seems like various commands are gaining --recurse-submodules options\ntaking different kinds of arguments:\n\n- clone takes --recurse-submodules=\n- fetch takes --recurse-submodules=\n- after this patch, diff takes --recurse-submodules=\n\nIs there a unifying principle? Can Documentation/gitsubmodules.txt\nsay a word or two about what kind of argument values the user should\nexpect to be accepted by these options?\n\nThanks,\nJonathan\n"},{"id":"357515","messageId":"CAN0heSpOG8c5En_YVvtkcwBmOrnS72cTXSw9YRJP4FG-M8dWag@mail.gmail.com","threadId":"49287","inReplyTo":"20180905225828.17782-1-sbeller@google.com","subject":"Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-09-06T06:19:05Z","receivedAt":"2018-09-06T06:19:22Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 6 Sep 2018 at 00:59, Stefan Beller wrote:\n>\n> --submodule[=]::\n\nMaybe drop `--submodule` here ...\n\n> +--recurse-submodules[=]::\n> Specify how differences in submodules are shown. When specifying\n> `--submodule=short` the 'short' format is used. This format just\n\n... and use `--recurse-submodules` here ...\n\n> shows the names of the commits at the beginning and end of the range.\n\n... and mention `--submodule` here as a historical alias? Maybe\ndeprecate it? I suppose the implementation of the aliasing is easy\nenough that we can carry `--submodule` around forever, though.\n\n> diff --git a/diff.c b/diff.c\n> index 145cfbae592..d3d5a989bd1 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -5023,6 +5023,8 @@ int diff_opt_parse(struct diff_options *options,\n> handle_ignore_submodules_arg(options, arg);\n> } else if (skip_to_optional_arg_default(arg, \"--submodule\", &arg, \"log\"))\n> return parse_submodule_opt(options, arg);\n> + else if (skip_to_optional_arg_default(arg, \"--recurse-submodules\", &arg, \"log\"))\n> + return parse_submodule_opt(options, arg);\n\nHow about (whitespace-damaged)\n\n} else if (skip_to_optional_arg_default(arg, \"--submodule\", &arg, \"log\") ||\n skip_to_optional_arg_default(arg, \"--recurse-submodules\",\n&arg, \"log\"))\n return parse_submodule_opt(options, arg);\n\nto make this future-proof? Sure, they're close enough that one should\nnotice the two instances, and any future work work would supposedly\nhappen in `parse_submodule_opt()` or anywhere else but here, but still.\n\nJust a few thoughts.\n\nMartin\n"},{"id":"357558","messageId":"20180906182353.114453-1-sbeller@google.com","threadId":"49287","inReplyTo":"CAN0heSpOG8c5En_YVvtkcwBmOrnS72cTXSw9YRJP4FG-M8dWag@mail.gmail.com","subject":"[PATCH] diff: allow --recurse-submodules as an synonym for --submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-06T18:23:53Z","receivedAt":"2018-09-06T18:24:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Many commands have flags to recurse into submodules, which is named\n--recurse-submodules. The diff family also has a submodule recursion flag,\nbut that is named differently. Add a synonym --recurse-submodules, which\nmeans the same as the --submodule flag, such that across all git commands\nsupporting submodules we have the --recurse-submodules flag available.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/diff-options.txt | 21 +++++++++++----------\n diff.c | 3 ++-\n 2 files changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 0378cd574eb..28c6c7f939f 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -226,17 +226,18 @@ linkgit:git-config[1]).\n \tShow only names and status of changed files. See the description\n \tof the `--diff-filter` option on what the status letters mean.\n \n---submodule[=]::\n+--recurse-submodules[=]::\n \tSpecify how differences in submodules are shown. When specifying\n-\t`--submodule=short` the 'short' format is used. This format just\n-\tshows the names of the commits at the beginning and end of the range.\n-\tWhen `--submodule` or `--submodule=log` is specified, the 'log'\n-\tformat is used. This format lists the commits in the range like\n-\tlinkgit:git-submodule[1] `summary` does. When `--submodule=diff`\n-\tis specified, the 'diff' format is used. This format shows an\n-\tinline diff of the changes in the submodule contents between the\n-\tcommit range. Defaults to `diff.submodule` or the 'short' format\n-\tif the config option is unset.\n+\t`--recurse-submodules=short` the 'short' format is used. This format\n+\tjust shows the names of the commits at the beginning and end of the\n+\trange. When `--recurse-submodules` or `--recurse-submodules=log` is\n+\tspecified, the 'log' format is used. This format lists the commits\n+\tin the range like linkgit:git-submodule[1] `summary` does.\n+\tWhen `--recurse-submodules=diff` is specified, the 'diff' format\n+\tis used. This format shows an inline diff of the changes in the\n+\tsubmodule contents between the commit range. Defaults to\n+\t`diff.submodule` or the 'short' format if the config option is unset.\n+\t`--submodule[=]` is a historic synonym for this option.\n \n --color[=]::\n \tShow colored diff.\ndiff --git a/diff.c b/diff.c\nindex 145cfbae592..b874f166c00 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5021,7 +5021,8 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (skip_to_optional_arg_default(arg, \"--ignore-submodules\", &arg, \"all\")) {\n \t\toptions->flags.override_submodule_config = 1;\n \t\thandle_ignore_submodules_arg(options, arg);\n-\t} else if (skip_to_optional_arg_default(arg, \"--submodule\", &arg, \"log\"))\n+\t} else if (skip_to_optional_arg_default(arg, \"--submodule\", &arg, \"log\") ||\n+\t skip_to_optional_arg_default(arg, \"--recurse-submodules\", &arg, \"log\"))\n \t\treturn parse_submodule_opt(options, arg);\n \telse if (skip_prefix(arg, \"--ws-error-highlight=\", &arg))\n \t\treturn parse_ws_error_highlight_opt(options, arg);\n-- \n2.19.0.rc2.392.g5ba43deb5a-goog\n\n"},{"id":"357559","messageId":"CAGZ79kbBC9OxiMhOCjB61pQXaMa-_tXJdkPq_6JnzC+gDzRuAw@mail.gmail.com","threadId":"49287","inReplyTo":"20180905231258.GD120842@aiede.svl.corp.google.com","subject":"Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-06T18:27:37Z","receivedAt":"2018-09-06T18:27:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 5, 2018 at 4:13 PM Jonathan Nieder wrote:\n>\n> Stefan Beller wrote:\n>\n> > Many commands have flags to recurse into submodules, which is named\n> > --recurse-submodules. The diff family also has a submodule recursion flag,\n> > but that is named differently. Add a synonym --recurse-submodules, which\n> > means the same as the --submodule flag, such that across all git commands\n> > supporting submodules we have the --recurse-submodules flag available.\n> >\n> > Signed-off-by: Stefan Beller \n> > ---\n> > Documentation/diff-options.txt | 1 +\n> > diff.c | 2 ++\n> > 2 files changed, 3 insertions(+)\n>\n> Makes sense.\n>\n> [...]\n> > --- a/Documentation/diff-options.txt\n> > +++ b/Documentation/diff-options.txt\n> > @@ -227,6 +227,7 @@ linkgit:git-config[1]).\n> > of the `--diff-filter` option on what the status letters mean.\n> >\n> > --submodule[=]::\n> > +--recurse-submodules[=]::\n> > Specify how differences in submodules are shown. When specifying\n> > `--submodule=short` the 'short' format is used. This format just\n> > shows the names of the commits at the beginning and end of the range.\n> > diff --git a/diff.c b/diff.c\n> > index 145cfbae592..d3d5a989bd1 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -5023,6 +5023,8 @@ int diff_opt_parse(struct diff_options *options,\n> > handle_ignore_submodules_arg(options, arg);\n> > } else if (skip_to_optional_arg_default(arg, \"--submodule\", &arg, \"log\"))\n> > return parse_submodule_opt(options, arg);\n> > + else if (skip_to_optional_arg_default(arg, \"--recurse-submodules\", &arg, \"log\"))\n> > + return parse_submodule_opt(options, arg);\n>\n> It seems like various commands are gaining --recurse-submodules options\n> taking different kinds of arguments:\n>\n> - clone takes --recurse-submodules=\n> - fetch takes --recurse-submodules=\n> - after this patch, diff takes --recurse-submodules=\n>\n> Is there a unifying principle? Can Documentation/gitsubmodules.txt\n> say a word or two about what kind of argument values the user should\n> expect to be accepted by these options?\n\nI don't think there is a unifying principle deep down.\nEverything but clone is specifying a mode (you could imagine\nthat we extend \"git reset --recurse-submodules\" as well to take\nsome sort of mode, either the hard/soft differentiation or how to treat\ndirty submodules or a mode that could differentiate between superprojects\nsha1 and branch of the same name)\n\nCare to send a patch on top to talk about these issues in\nDocumentation/gitsubmodules.txt ?\n\nThanks,\nStefan\n"},{"id":"357583","messageId":"xmqqefe6z5ws.fsf@gitster-ct.c.googlers.com","threadId":"49287","inReplyTo":"20180905231258.GD120842@aiede.svl.corp.google.com","subject":"Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-06T21:12:51Z","receivedAt":"2018-09-06T21:12:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> It seems like various commands are gaining --recurse-submodules options\n> taking different kinds of arguments:\n>\n> - clone takes --recurse-submodules=\n> - fetch takes --recurse-submodules=\n> - after this patch, diff takes --recurse-submodules=\n>\n> Is there a unifying principle? Can Documentation/gitsubmodules.txt\n> say a word or two about what kind of argument values the user should\n> expect to be accepted by these options?\n\nI am not sure if the above is rhetorical. The only thing such a\ndocument can say about status-quo is that the user cannot make an\neducated guess, as there is no consistency. Some take an option to\nclarify which subset of submodules to act on, others take an option\nto specify what variant of operation to be made on the submodules.\n\nIn the ideal world, the users ought to be able to give these two\nindependently, i.e. \"fetch\" should be able to say \"only fetch these\nsubmodules\" with pathspec while \"run the fetch in all of these\nsubmodules specified (with the pathspec) as necessary\" with\n\"on-demand\" mode, for example.\n\nIt may mean that it is too early to add \"diff --recurse-submodules\"\nas a synonym for \"diff --submodule\", before what we can do to\nimprove the situation for commands that already take that\n\"--recurse-submodules\" option. A solution for _that_ problem might\nresult in splitting it into two options (an option to select which\nsubmodule to recurse into, and another to specify the action that\nhappens in these submodules)---we'd regret if we realize that the\nexisting \"diff --submodule\" that specifies the action (i.e. how the\nchanges in submodules are presented, not which submodules' changes\nare shown) does not match the one we choose for --recurse-submodules\noption (e.g. we may say --recurse-submodules is for selection, and a\nnew --action-in-submodules is for action, in which case \"diff\"'s\n\"--submodule\" option is closer to the latter).\n"},{"id":"357586","messageId":"CAGZ79kbFGdtGL7jpYRrw_KaxJ5NY4HZNauOy-y3sxqnNDkHfWg@mail.gmail.com","threadId":"49287","inReplyTo":"xmqqefe6z5ws.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-06T21:25:01Z","receivedAt":"2018-09-06T21:25:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 6, 2018 at 2:12 PM Junio C Hamano wrote:\n>\n> Jonathan Nieder writes:\n>\n> > It seems like various commands are gaining --recurse-submodules options\n> > taking different kinds of arguments:\n> >\n> > - clone takes --recurse-submodules=\n> > - fetch takes --recurse-submodules=\n> > - after this patch, diff takes --recurse-submodules=\n> >\n> > Is there a unifying principle? Can Documentation/gitsubmodules.txt\n> > say a word or two about what kind of argument values the user should\n> > expect to be accepted by these options?\n>\n> I am not sure if the above is rhetorical. The only thing such a\n> document can say about status-quo is that the user cannot make an\n> educated guess, as there is no consistency. Some take an option to\n> clarify which subset of submodules to act on, others take an option\n> to specify what variant of operation to be made on the submodules.\n>\n> In the ideal world, the users ought to be able to give these two\n> independently, i.e. \"fetch\" should be able to say \"only fetch these\n> submodules\" with pathspec while \"run the fetch in all of these\n> submodules specified (with the pathspec) as necessary\" with\n> \"on-demand\" mode, for example.\n>\n> It may mean that it is too early to add \"diff --recurse-submodules\"\n> as a synonym for \"diff --submodule\", before what we can do to\n> improve the situation for commands that already take that\n> \"--recurse-submodules\" option.\n\nGood point. So let's retreat that patch for now?\n\nStefan\n"},{"id":"357610","messageId":"20180907064026.GB172953@aiede.svl.corp.google.com","threadId":"49287","inReplyTo":"xmqqefe6z5ws.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-07T06:40:26Z","receivedAt":"2018-09-07T06:40:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> It seems like various commands are gaining --recurse-submodules options\n>> taking different kinds of arguments:\n>>\n>> - clone takes --recurse-submodules=\n>> - fetch takes --recurse-submodules=\n>> - after this patch, diff takes --recurse-submodules=\n>>\n>> Is there a unifying principle? Can Documentation/gitsubmodules.txt\n>> say a word or two about what kind of argument values the user should\n>> expect to be accepted by these options?\n>\n> I am not sure if the above is rhetorical. The only thing such a\n> document can say about status-quo is that the user cannot make an\n> educated guess, as there is no consistency. Some take an option to\n> clarify which subset of submodules to act on, others take an option\n> to specify what variant of operation to be made on the submodules.\n\nIt's not rhetorical: I really do want to find out what our plan is for\nthe future of --recurse-submodules.\n\nOne possibility (A) would be \"accept pathspec everywhere\", as you\nmentioned. For a command like fetch that already accepts , that\nis problematic and would involve a migration. If we already have to\nmigrate fetch, migrating diff as well does not seem too bad.\n\nAnother possibility (B) would be \"accept pathspec in clone only\". A\nclone is a bit of a special case in that it is setting up the set of\nactive submodules; perhaps we want other commands to always respect\nthat set of active submodules without a method for overriding and only\nacting on a subset. After all, \"git checkout \" doesn't have a\nway to only checkout on a subset of the worktree; why should\n\"git checkout --recurse-submodules \" be any different?\n\nWhen I think about it this way, I suspect that (B) will provide a\nbetter experience than (A), so this diff change doesn't seem like a\nstep in the wrong direction. Except: it took me a long time to think\nthis through. Some documentation really would help, since it would\nmean that the next person can understand what the *intention* behind\nthese options are and save some time thinking through other not-chosen\nalternatives.\n\nHmm?\n\nThanks,\nJonathan\n"},{"id":"357618","messageId":"xmqqh8j1y6og.fsf@gitster-ct.c.googlers.com","threadId":"49287","inReplyTo":"20180907064026.GB172953@aiede.svl.corp.google.com","subject":"Re: [PATCH] diff: allow --recurse-submodules as an synonym for --submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-07T09:53:51Z","receivedAt":"2018-09-07T09:53:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> When I think about it this way, I suspect that (B) will provide a\n> better experience than (A), so this diff change doesn't seem like a\n> step in the wrong direction.\n\nOK, that's fair.\n"}]}