{"thread":{"id":"56130","subject":"'git range-diff' does not detect submodule changes if 'diff.submodule=log'","startedAt":"2021-07-19T23:11:22Z","lastAt":"2021-07-22T22:42:38Z","messageCount":3,"participants":["Philippe Blain","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"430555","messageId":"e469038c-d78c-cd4b-0214-7094746b9281@gmail.com","threadId":"56130","inReplyTo":null,"subject":"'git range-diff' does not detect submodule changes if 'diff.submodule=log'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-07-19T20:43:54Z","receivedAt":"2021-07-19T23:11:22Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Johannes,\n\nI noticed that 'git range-diff' silently \"drops\" submodule changes if\n'diff.submodule=log' is set in the config. This is because the 'diff --git'\nheader is not shown in that case, and the code in range-diff.c::read_patches\nexpects that header to be found to detect changes.\n\nIf 'diff.submodule' is instead set to 'diff', the range-diff outright errors with\n(at least in my case):\n\nerror: git apply: bad git-diff - inconsistent old filename on line 1\nerror: could not parse git header 'diff --git path/to/submodule/and/some/file/within\n'\nerror: could not parse log for '@{u}..@{1}'\n\n\nI think it would make sense to force '--submodule=short' for range-diff, something like:\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 1a4471fe4c..d74b9c7a55 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -54,7 +54,7 @@ static int read_patches(const char *range, struct string_list *list,\n\n         strvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\", \"--no-merges\",\n                      \"--reverse\", \"--date-order\", \"--decorate=no\",\n-                    \"--no-prefix\",\n+                    \"--no-prefix\", \"--submodule=short\",\n                      /*\n                       * Choose indicators that are not used anywhere\n                       * else in diffs, but still look reasonable\n\n\nWhat do you think ?\n\nThanks,\nPhilippe.\n\nP.S. As an aside,\nI'm  not sure why you chose to skip the 'diff --git' header with '--submodule=log'\n(I did not search the list), but I think that in general 'git diff' and friends,\nit would be nice to be able to still see this header even with --submodule=log...\n"},{"id":"430929","messageId":"nycvar.QRO.7.76.6.2107221516240.55@tvgsbejvaqbjf.bet","threadId":"56130","inReplyTo":"e469038c-d78c-cd4b-0214-7094746b9281@gmail.com","subject":"Re: 'git range-diff' does not detect submodule changes if 'diff.submodule=log'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-07-22T13:19:01Z","receivedAt":"2021-07-22T13:19:05Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Philippe,\n\nOn Mon, 19 Jul 2021, Philippe Blain wrote:\n\n> I noticed that 'git range-diff' silently \"drops\" submodule changes if\n> 'diff.submodule=log' is set in the config. This is because the 'diff --git'\n> header is not shown in that case, and the code in range-diff.c::read_patches\n> expects that header to be found to detect changes.\n>\n> If 'diff.submodule' is instead set to 'diff', the range-diff outright errors\n> with\n> (at least in my case):\n>\n> error: git apply: bad git-diff - inconsistent old filename on line 1\n> error: could not parse git header 'diff --git\n> error: path/to/submodule/and/some/file/within\n> '\n> error: could not parse log for '@{u}..@{1}'\n>\n>\n> I think it would make sense to force '--submodule=short' for range-diff,\n> something like:\n>\n> diff --git a/range-diff.c b/range-diff.c\n> index 1a4471fe4c..d74b9c7a55 100644\n> --- a/range-diff.c\n> +++ b/range-diff.c\n> @@ -54,7 +54,7 @@ static int read_patches(const char *range, struct\n> string_list *list,\n>\n>         strvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\", \"--no-merges\",\n>                      \"--reverse\", \"--date-order\", \"--decorate=no\",\n> -                    \"--no-prefix\",\n> +                    \"--no-prefix\", \"--submodule=short\",\n>                      /*\n>                       * Choose indicators that are not used anywhere\n>                       * else in diffs, but still look reasonable\n>\n>\n> What do you think ?\n\nSure. I never thought that `range-diff` would be useful in the context of\nsubmodules. But then, I am an anti-fan of submodules anyway, so don't put\ntoo much stock into my opinion about anything submodule-related.\n\n> P.S. As an aside,\n> I'm  not sure why you chose to skip the 'diff --git' header with\n> '--submodule=log'\n> (I did not search the list), but I think that in general 'git diff' and\n> friends,\n> it would be nice to be able to still see this header even with\n> --submodule=log...\n\nI don't remember the details. It might be that `git-tbdiff` did it that\nway, and I copied it without questioning.\n\n*clicketyclick*\n\nYep, that seems plausible:\nhttps://github.com/trast/tbdiff/blob/047d1c79dfada57522a42f307cd4b0ddcb098934/git-tbdiff.py#L48\n\nCiao,\nDscho\n"},{"id":"430972","messageId":"98bdd571-e4f1-a9a5-d7e6-81b1b6c89b45@gmail.com","threadId":"56130","inReplyTo":"nycvar.QRO.7.76.6.2107221516240.55@tvgsbejvaqbjf.bet","subject":"Re: 'git range-diff' does not detect submodule changes if 'diff.submodule=log'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-07-22T22:42:33Z","receivedAt":"2021-07-22T22:42:38Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Dscho,\n\nLe 2021-07-22 à 09:19, Johannes Schindelin a écrit :\n> Hi Philippe,\n> \n> On Mon, 19 Jul 2021, Philippe Blain wrote:\n> \n>> I noticed that 'git range-diff' silently \"drops\" submodule changes if\n>> 'diff.submodule=log' is set in the config. This is because the 'diff --git'\n>> header is not shown in that case, and the code in range-diff.c::read_patches\n>> expects that header to be found to detect changes.\n>>\n>> If 'diff.submodule' is instead set to 'diff', the range-diff outright errors\n>> with\n>> (at least in my case):\n>>\n>> error: git apply: bad git-diff - inconsistent old filename on line 1\n>> error: could not parse git header 'diff --git\n>> error: path/to/submodule/and/some/file/within\n>> '\n>> error: could not parse log for '@{u}..@{1}'\n>>\n>>\n>> I think it would make sense to force '--submodule=short' for range-diff,\n>> something like:\n>>\n>> diff --git a/range-diff.c b/range-diff.c\n>> index 1a4471fe4c..d74b9c7a55 100644\n>> --- a/range-diff.c\n>> +++ b/range-diff.c\n>> @@ -54,7 +54,7 @@ static int read_patches(const char *range, struct\n>> string_list *list,\n>>\n>>          strvec_pushl(&cp.args, \"log\", \"--no-color\", \"-p\", \"--no-merges\",\n>>                       \"--reverse\", \"--date-order\", \"--decorate=no\",\n>> -                    \"--no-prefix\",\n>> +                    \"--no-prefix\", \"--submodule=short\",\n>>                       /*\n>>                        * Choose indicators that are not used anywhere\n>>                        * else in diffs, but still look reasonable\n>>\n>>\n>> What do you think ?\n> \n> Sure. I never thought that `range-diff` would be useful in the context of\n> submodules. But then, I am an anti-fan of submodules anyway, so don't put\n> too much stock into my opinion about anything submodule-related.\n\nOK. I'll try to find the time to send a patch for that.\n\n> \n>> P.S. As an aside,\n>> I'm  not sure why you chose to skip the 'diff --git' header with\n>> '--submodule=log'\n>> (I did not search the list), but I think that in general 'git diff' and\n>> friends,\n>> it would be nice to be able to still see this header even with\n>> --submodule=log...\n> \n> I don't remember the details. It might be that `git-tbdiff` did it that\n> way, and I copied it without questioning.\n> \n> *clicketyclick*\n> \n> Yep, that seems plausible:\n> https://github.com/trast/tbdiff/blob/047d1c79dfada57522a42f307cd4b0ddcb098934/git-tbdiff.py#L48\n\nI meant when you implemented '--submodule=log' back in the days, why did you choose\nto skip the 'diff --git' header. In fact I dig up the thread and keeping that header\nis something Jens Lehmann actually suggested [1] (near the end of the message).\nI for one would find it useful; it's easy to miss submodules changes among a sea of other\nchanges if the submodule is updated with just a few commits... yet another thing to\nadd to my list :)\n\nCheers,\n\nPhilippe.\n\n[1] https://lore.kernel.org/git/4AC9D6EB.8090002@web.de/\n"}]}