{"thread":{"id":"49526","subject":"[PATCH] range-diff: allow to diff files regardless submodule","startedAt":"2018-10-10T15:09:48Z","lastAt":"2018-10-24T19:46:40Z","messageCount":17,"participants":["Lucas De Marchi","brian m. carlson","Ævar Arnfjörð Bjarmason","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"360034","messageId":"20181010150916.4295-1-lucas.demarchi@intel.com","threadId":"49526","inReplyTo":null,"subject":"[PATCH] range-diff: allow to diff files regardless submodule","fromName":"Lucas De Marchi","fromEmail":"lucas.demarchi@intel.com","sentAt":"2018-10-10T15:09:16Z","receivedAt":"2018-10-10T15:09:48Z","isPatch":true,"sender":{"key":"lucas.demarchi@intel.com","avatar":null},"body":"Do like it's done in grep so mode doesn't end up as\n0160000, which means range-diff doesn't work if one has\n\"submodule.diff = log\" in the configuration. Without this\nwhile using range-diff I only get a\n\n    Submodule a 0000000...0000000 (new submodule)\n\ninstead of the diff between the revisions.\n\nSigned-off-by: Lucas De Marchi <lucas.demarchi@intel.com>\n---\n range-diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 60edb2f518..bd8083f2d1 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n {\n \tstruct diff_filespec *spec = alloc_filespec(name);\n \n-\tfill_filespec(spec, &null_oid, 0, 0644);\n+\tfill_filespec(spec, &null_oid, 0, 0100644);\n \tspec->data = (char *)p;\n \tspec->size = strlen(p);\n \tspec->should_munmap = 0;\n-- \n2.19.1.1.g8c3cf03f71\n\n"},{"id":"360114","messageId":"20181011000233.GX432229@genre.crustytoothpaste.net","threadId":"49526","inReplyTo":"20181010150916.4295-1-lucas.demarchi@intel.com","subject":"Re: [PATCH] range-diff: allow to diff files regardless submodule","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-10-11T00:02:33Z","receivedAt":"2018-10-11T00:02:42Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Oct 10, 2018 at 08:09:16AM -0700, Lucas De Marchi wrote:\n> Do like it's done in grep so mode doesn't end up as\n> 0160000, which means range-diff doesn't work if one has\n> \"submodule.diff = log\" in the configuration. Without this\n> while using range-diff I only get a\n> \n>     Submodule a 0000000...0000000 (new submodule)\n> \n> instead of the diff between the revisions.\n> \n> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>\n> ---\n>  range-diff.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/range-diff.c b/range-diff.c\n> index 60edb2f518..bd8083f2d1 100644\n> --- a/range-diff.c\n> +++ b/range-diff.c\n> @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n>  {\n>  \tstruct diff_filespec *spec = alloc_filespec(name);\n>  \n> -\tfill_filespec(spec, &null_oid, 0, 0644);\n> +\tfill_filespec(spec, &null_oid, 0, 0100644);\n\nIf we have a system that has different mode values from the common Unix\nones, is this still correct or does it need to change?\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"360145","messageId":"87in29dj55.fsf@evledraar.gmail.com","threadId":"49526","inReplyTo":"20181010150916.4295-1-lucas.demarchi@intel.com","subject":"Re: [PATCH] range-diff: allow to diff files regardless submodule","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-11T07:42:14Z","receivedAt":"2018-10-11T07:42:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 10 2018, Lucas De Marchi wrote:\n\n> Do like it's done in grep so mode doesn't end up as\n> 0160000, which means range-diff doesn't work if one has\n> \"submodule.diff = log\" in the configuration. Without this\n> while using range-diff I only get a\n>\n>     Submodule a 0000000...0000000 (new submodule)\n\nI'm not familiar enough with this to tell what the real problem is\nthat's being solved from the commit message, but if it means that now\nrange-diff works in some configuration, presumably that can be reduced\nto a simple set of commands that didn't work before but now does, and\ntherefore a test in t/t3206-range-diff.sh.\n\n> instead of the diff between the revisions.\n>\n> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>\n> ---\n>  range-diff.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/range-diff.c b/range-diff.c\n> index 60edb2f518..bd8083f2d1 100644\n> --- a/range-diff.c\n> +++ b/range-diff.c\n> @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n>  {\n>  \tstruct diff_filespec *spec = alloc_filespec(name);\n>\n> -\tfill_filespec(spec, &null_oid, 0, 0644);\n> +\tfill_filespec(spec, &null_oid, 0, 0100644);\n>  \tspec->data = (char *)p;\n>  \tspec->size = strlen(p);\n>  \tspec->should_munmap = 0;\n"},{"id":"360147","messageId":"CAKi4VALL5nnLfo4ZxtifKE1JmcmUQ6F-9GSAQEMxXLjKsCtSmg@mail.gmail.com","threadId":"49526","inReplyTo":"20181011000233.GX432229@genre.crustytoothpaste.net","subject":"Re: [PATCH] range-diff: allow to diff files regardless submodule","fromName":"Lucas De Marchi","fromEmail":"lucas.de.marchi@gmail.com","sentAt":"2018-10-11T07:50:36Z","receivedAt":"2018-10-11T07:50:51Z","isPatch":true,"sender":{"key":"lucas.de.marchi@gmail.com","avatar":null},"body":"On Wed, Oct 10, 2018 at 5:02 PM brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n>\n> On Wed, Oct 10, 2018 at 08:09:16AM -0700, Lucas De Marchi wrote:\n> > Do like it's done in grep so mode doesn't end up as\n> > 0160000, which means range-diff doesn't work if one has\n> > \"submodule.diff = log\" in the configuration. Without this\n> > while using range-diff I only get a\n> >\n> >     Submodule a 0000000...0000000 (new submodule)\n> >\n> > instead of the diff between the revisions.\n> >\n> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>\n> > ---\n> >  range-diff.c | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/range-diff.c b/range-diff.c\n> > index 60edb2f518..bd8083f2d1 100644\n> > --- a/range-diff.c\n> > +++ b/range-diff.c\n> > @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n> >  {\n> >       struct diff_filespec *spec = alloc_filespec(name);\n> >\n> > -     fill_filespec(spec, &null_oid, 0, 0644);\n> > +     fill_filespec(spec, &null_oid, 0, 0100644);\n>\n> If we have a system that has different mode values from the common Unix\n> ones, is this still correct or does it need to change?\n\nFrom what I can see this would still be correct, or at least git-grep\nimplementation would be broken.\n\nLucas De Marchi\n> --\n> brian m. carlson: Houston, Texas, US\n> OpenPGP: https://keybase.io/bk2204\n\n\n\n-- \nLucas De Marchi\n"},{"id":"360148","messageId":"CAKi4VAL+qSObOABeNHVXGytE9aB3sUJtecE5jYF6_ZfDA+J0ZQ@mail.gmail.com","threadId":"49526","inReplyTo":"87in29dj55.fsf@evledraar.gmail.com","subject":"Re: [PATCH] range-diff: allow to diff files regardless submodule","fromName":"Lucas De Marchi","fromEmail":"lucas.de.marchi@gmail.com","sentAt":"2018-10-11T08:14:37Z","receivedAt":"2018-10-11T08:14:52Z","isPatch":true,"sender":{"key":"lucas.de.marchi@gmail.com","avatar":null},"body":"On Thu, Oct 11, 2018 at 12:42 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Wed, Oct 10 2018, Lucas De Marchi wrote:\n>\n> > Do like it's done in grep so mode doesn't end up as\n> > 0160000, which means range-diff doesn't work if one has\n> > \"submodule.diff = log\" in the configuration. Without this\n> > while using range-diff I only get a\n> >\n> >     Submodule a 0000000...0000000 (new submodule)\n>\n> I'm not familiar enough with this to tell what the real problem is\n> that's being solved from the commit message, but if it means that now\n> range-diff works in some configuration, presumably that can be reduced\n> to a simple set of commands that didn't work before but now does, and\n> therefore a test in t/t3206-range-diff.sh.\n\n$ git config --global diff.submodule log\n$ git range-diff\n\nThis produces the output above\n\n$ git config --global diff.submodule short\n$ git range-diff\n\nThis blocks forever in a wait4() call and prints this when terminated:\nfatal: exec 'diff': cd to 'a' failed: Not a directory\n\nLucas De Marchi\n\n>\n> > instead of the diff between the revisions.\n> >\n> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>\n> > ---\n> >  range-diff.c | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/range-diff.c b/range-diff.c\n> > index 60edb2f518..bd8083f2d1 100644\n> > --- a/range-diff.c\n> > +++ b/range-diff.c\n> > @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n> >  {\n> >       struct diff_filespec *spec = alloc_filespec(name);\n> >\n> > -     fill_filespec(spec, &null_oid, 0, 0644);\n> > +     fill_filespec(spec, &null_oid, 0, 0100644);\n> >       spec->data = (char *)p;\n> >       spec->size = strlen(p);\n> >       spec->should_munmap = 0;\n\n\n\n-- \nLucas De Marchi\n"},{"id":"360150","messageId":"20181011081750.24240-1-lucas.demarchi@intel.com","threadId":"49526","inReplyTo":"CAKi4VAL+qSObOABeNHVXGytE9aB3sUJtecE5jYF6_ZfDA+J0ZQ@mail.gmail.com","subject":"[PATCH v2] range-diff: allow to diff files regardless submodule","fromName":"Lucas De Marchi","fromEmail":"lucas.demarchi@intel.com","sentAt":"2018-10-11T08:17:50Z","receivedAt":"2018-10-11T08:18:17Z","isPatch":true,"sender":{"key":"lucas.demarchi@intel.com","avatar":null},"body":"Do like it's done in grep so mode doesn't end up as\n0160000, which means range-diff doesn't work if one has\n\"submodule.diff = log\" in the configuration. Without this\nwhile using range-diff I only get a\n\n    Submodule a 0000000...0000000 (new submodule)\n\ninstead of the diff between the revisions.\n\nSigned-off-by: Lucas De Marchi <lucas.demarchi@intel.com>\n---\n range-diff.c          |  2 +-\n t/t3206-range-diff.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 60edb2f518..bd8083f2d1 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n {\n \tstruct diff_filespec *spec = alloc_filespec(name);\n \n-\tfill_filespec(spec, &null_oid, 0, 0644);\n+\tfill_filespec(spec, &null_oid, 0, 0100644);\n \tspec->data = (char *)p;\n \tspec->size = strlen(p);\n \tspec->should_munmap = 0;\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 045aca1c18..6aae364171 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -122,6 +122,35 @@ test_expect_success 'changed commit' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'changed commit with sm config' '\n+\tgit range-diff --no-color --submodule=log topic...changed >actual &&\n+\tcat >expected <<-EOF &&\n+\t1:  4de457d = 1:  a4b3333 s/5/A/\n+\t2:  fccce22 = 2:  f51d370 s/4/A/\n+\t3:  147e64e ! 3:  0559556 s/11/B/\n+\t    @@ -10,7 +10,7 @@\n+\t      9\n+\t      10\n+\t     -11\n+\t    -+B\n+\t    ++BB\n+\t      12\n+\t      13\n+\t      14\n+\t4:  a63e992 ! 4:  d966c5c s/12/B/\n+\t    @@ -8,7 +8,7 @@\n+\t     @@\n+\t      9\n+\t      10\n+\t    - B\n+\t    + BB\n+\t     -12\n+\t     +B\n+\t      13\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'no commits on one side' '\n \tgit commit --amend -m \"new message\" &&\n \tgit range-diff master HEAD@{1} HEAD\n-- \n2.19.1.1.g8c3cf03f71\n\n"},{"id":"360152","messageId":"xmqqmurkgaap.fsf@gitster-ct.c.googlers.com","threadId":"49526","inReplyTo":"87in29dj55.fsf@evledraar.gmail.com","subject":"Re: [PATCH] range-diff: allow to diff files regardless submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-11T08:25:02Z","receivedAt":"2018-10-11T08:25:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Wed, Oct 10 2018, Lucas De Marchi wrote:\n>\n>> Do like it's done in grep so mode doesn't end up as\n>> 0160000, which means range-diff doesn't work if one has\n>> \"submodule.diff = log\" in the configuration. Without this\n>> while using range-diff I only get a\n>>\n>>     Submodule a 0000000...0000000 (new submodule)\n>\n> I'm not familiar enough with this to tell what the real problem is\n> that's being solved from the commit message, but if it means that now\n> range-diff works in some configuration, presumably that can be reduced\n> to a simple set of commands that didn't work before but now does, and\n> therefore a test in t/t3206-range-diff.sh.\n\nYes, I agree on both counts (i.e. it was totally unclear what\nproblem is being solved and what the root cause of the problem is,\nand we would want a new test to protect this \"fix\" from getting\nbroken in the future.\n\nThanks.\n"},{"id":"360287","messageId":"nycvar.QRO.7.76.6.1810121122260.45@tvgsbejvaqbjf.bet","threadId":"49526","inReplyTo":"CAKi4VALL5nnLfo4ZxtifKE1JmcmUQ6F-9GSAQEMxXLjKsCtSmg@mail.gmail.com","subject":"Re: [PATCH] range-diff: allow to diff files regardless submodule","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-12T09:24:43Z","receivedAt":"2018-10-12T09:25:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\n\nOn Thu, 11 Oct 2018, Lucas De Marchi wrote:\n\n> On Wed, Oct 10, 2018 at 5:02 PM brian m. carlson\n> <sandals@crustytoothpaste.net> wrote:\n> >\n> > On Wed, Oct 10, 2018 at 08:09:16AM -0700, Lucas De Marchi wrote:\n> > > Do like it's done in grep so mode doesn't end up as\n> > > 0160000, which means range-diff doesn't work if one has\n> > > \"submodule.diff = log\" in the configuration. Without this\n> > > while using range-diff I only get a\n> > >\n> > >     Submodule a 0000000...0000000 (new submodule)\n> > >\n> > > instead of the diff between the revisions.\n> > >\n> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>\n> > > ---\n> > >  range-diff.c | 2 +-\n> > >  1 file changed, 1 insertion(+), 1 deletion(-)\n> > >\n> > > diff --git a/range-diff.c b/range-diff.c\n> > > index 60edb2f518..bd8083f2d1 100644\n> > > --- a/range-diff.c\n> > > +++ b/range-diff.c\n> > > @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n> > >  {\n> > >       struct diff_filespec *spec = alloc_filespec(name);\n> > >\n> > > -     fill_filespec(spec, &null_oid, 0, 0644);\n> > > +     fill_filespec(spec, &null_oid, 0, 0100644);\n> >\n> > If we have a system that has different mode values from the common Unix\n> > ones, is this still correct or does it need to change?\n> \n> From what I can see this would still be correct, or at least git-grep\n> implementation would be broken.\n\nAs you can see from the Windows port: we are stuck with the simplistic\nPOSIX permissions in Git, and platforms that have a different permission\nsystem have to emulate it.\n\nWe only use preciously few bit masks, anyway. For example, we do not\nreally use the lower 12 bits for anything but \"executable or not?\"\n\nSo Lucas' patch is correct, AFAICT.\n\nCiao,\nDscho\n\n> \n> Lucas De Marchi\n> > --\n> > brian m. carlson: Houston, Texas, US\n> > OpenPGP: https://keybase.io/bk2204\n> \n> \n> \n> -- \n> Lucas De Marchi\n> \n"},{"id":"360288","messageId":"nycvar.QRO.7.76.6.1810121124570.45@tvgsbejvaqbjf.bet","threadId":"49526","inReplyTo":"20181011081750.24240-1-lucas.demarchi@intel.com","subject":"Re: [PATCH v2] range-diff: allow to diff files regardless submodule","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-12T09:26:29Z","receivedAt":"2018-10-12T09:26:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Lucas,\n\nOn Thu, 11 Oct 2018, Lucas De Marchi wrote:\n\n> Do like it's done in grep so mode doesn't end up as\n> 0160000, which means range-diff doesn't work if one has\n> \"submodule.diff = log\" in the configuration. Without this\n> while using range-diff I only get a\n> \n>     Submodule a 0000000...0000000 (new submodule)\n> \n> instead of the diff between the revisions.\n> \n> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>\n\nThank you for this contribution, which I am glad to ACK.\n\nI am especially happy that you added a regression test so that we are\nconfident not to break this again.\n\nCiao,\nDscho\n\n> ---\n>  range-diff.c          |  2 +-\n>  t/t3206-range-diff.sh | 29 +++++++++++++++++++++++++++++\n>  2 files changed, 30 insertions(+), 1 deletion(-)\n> \n> diff --git a/range-diff.c b/range-diff.c\n> index 60edb2f518..bd8083f2d1 100644\n> --- a/range-diff.c\n> +++ b/range-diff.c\n> @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n>  {\n>  \tstruct diff_filespec *spec = alloc_filespec(name);\n>  \n> -\tfill_filespec(spec, &null_oid, 0, 0644);\n> +\tfill_filespec(spec, &null_oid, 0, 0100644);\n>  \tspec->data = (char *)p;\n>  \tspec->size = strlen(p);\n>  \tspec->should_munmap = 0;\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index 045aca1c18..6aae364171 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -122,6 +122,35 @@ test_expect_success 'changed commit' '\n>  \ttest_cmp expected actual\n>  '\n>  \n> +test_expect_success 'changed commit with sm config' '\n> +\tgit range-diff --no-color --submodule=log topic...changed >actual &&\n> +\tcat >expected <<-EOF &&\n> +\t1:  4de457d = 1:  a4b3333 s/5/A/\n> +\t2:  fccce22 = 2:  f51d370 s/4/A/\n> +\t3:  147e64e ! 3:  0559556 s/11/B/\n> +\t    @@ -10,7 +10,7 @@\n> +\t      9\n> +\t      10\n> +\t     -11\n> +\t    -+B\n> +\t    ++BB\n> +\t      12\n> +\t      13\n> +\t      14\n> +\t4:  a63e992 ! 4:  d966c5c s/12/B/\n> +\t    @@ -8,7 +8,7 @@\n> +\t     @@\n> +\t      9\n> +\t      10\n> +\t    - B\n> +\t    + BB\n> +\t     -12\n> +\t     +B\n> +\t      13\n> +\tEOF\n> +\ttest_cmp expected actual\n> +'\n> +\n>  test_expect_success 'no commits on one side' '\n>  \tgit commit --amend -m \"new message\" &&\n>  \tgit range-diff master HEAD@{1} HEAD\n> -- \n> 2.19.1.1.g8c3cf03f71\n> \n> \n"},{"id":"360360","messageId":"20181012231742.GZ432229@genre.crustytoothpaste.net","threadId":"49526","inReplyTo":"nycvar.QRO.7.76.6.1810121122260.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] range-diff: allow to diff files regardless submodule","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-10-12T23:17:43Z","receivedAt":"2018-10-12T23:18:03Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Oct 12, 2018 at 11:24:43AM +0200, Johannes Schindelin wrote:\n> \n> \n> On Thu, 11 Oct 2018, Lucas De Marchi wrote:\n> \n> > On Wed, Oct 10, 2018 at 5:02 PM brian m. carlson\n> > <sandals@crustytoothpaste.net> wrote:\n> > >\n> > > On Wed, Oct 10, 2018 at 08:09:16AM -0700, Lucas De Marchi wrote:\n> > > > Do like it's done in grep so mode doesn't end up as\n> > > > 0160000, which means range-diff doesn't work if one has\n> > > > \"submodule.diff = log\" in the configuration. Without this\n> > > > while using range-diff I only get a\n> > > >\n> > > >     Submodule a 0000000...0000000 (new submodule)\n> > > >\n> > > > instead of the diff between the revisions.\n> > > >\n> > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>\n> > > > ---\n> > > >  range-diff.c | 2 +-\n> > > >  1 file changed, 1 insertion(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/range-diff.c b/range-diff.c\n> > > > index 60edb2f518..bd8083f2d1 100644\n> > > > --- a/range-diff.c\n> > > > +++ b/range-diff.c\n> > > > @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n> > > >  {\n> > > >       struct diff_filespec *spec = alloc_filespec(name);\n> > > >\n> > > > -     fill_filespec(spec, &null_oid, 0, 0644);\n> > > > +     fill_filespec(spec, &null_oid, 0, 0100644);\n> > >\n> > > If we have a system that has different mode values from the common Unix\n> > > ones, is this still correct or does it need to change?\n> > \n> > From what I can see this would still be correct, or at least git-grep\n> > implementation would be broken.\n> \n> As you can see from the Windows port: we are stuck with the simplistic\n> POSIX permissions in Git, and platforms that have a different permission\n> system have to emulate it.\n\nI think I may not have explained myself well.  There are a small number\nof POSIXy systems which have mode bits that differ from the common ones\n(e.g., a plain file is something other than 0100000).  I think one\nperson mentioned on the list that they have a homebrew Unix that works\nthis way, and I think I may have heard of some minor commercial Unices\nthat work this way as well.\n\nMy question was intended to ask whether we should be using an\nOS-provided constant (e.g., S_IFREG) that represented that value\ndifferently because it was a system value or whether it was the internal\nGit representation.\n\nI hadn't intended to inquire about Windows, as I was fairly confident\nthat this syntax does indeed work there through our compatibility layers\n(because it has in the past even when we've had these kinds of issues on\nother Unices).  But I'm glad that you chimed in and confirmed that it\ndoes.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"361288","messageId":"20181023140710.GA7073@ldmartin-desk.jf.intel.com","threadId":"49526","inReplyTo":"xmqqmurkgaap.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] range-diff: allow to diff files regardless submodule","fromName":"Lucas De Marchi","fromEmail":"lucas.demarchi@intel.com","sentAt":"2018-10-23T14:07:10Z","receivedAt":"2018-10-23T14:07:13Z","isPatch":true,"sender":{"key":"lucas.demarchi@intel.com","avatar":null},"body":"On Thu, Oct 11, 2018 at 05:25:02PM +0900, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n> \n> > On Wed, Oct 10 2018, Lucas De Marchi wrote:\n> >\n> >> Do like it's done in grep so mode doesn't end up as\n> >> 0160000, which means range-diff doesn't work if one has\n> >> \"submodule.diff = log\" in the configuration. Without this\n> >> while using range-diff I only get a\n> >>\n> >>     Submodule a 0000000...0000000 (new submodule)\n> >\n> > I'm not familiar enough with this to tell what the real problem is\n> > that's being solved from the commit message, but if it means that now\n> > range-diff works in some configuration, presumably that can be reduced\n> > to a simple set of commands that didn't work before but now does, and\n> > therefore a test in t/t3206-range-diff.sh.\n> \n> Yes, I agree on both counts (i.e. it was totally unclear what\n> problem is being solved and what the root cause of the problem is,\n> and we would want a new test to protect this \"fix\" from getting\n> broken in the future.\n\nhave you seen I sent a v2 with proper test?\n\nthanks\nLucas De Marchi\n\n> \n> Thanks.\n"},{"id":"361352","messageId":"xmqqh8hcqeho.fsf@gitster-ct.c.googlers.com","threadId":"49526","inReplyTo":"20181023140710.GA7073@ldmartin-desk.jf.intel.com","subject":"Re: [PATCH] range-diff: allow to diff files regardless submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T02:12:51Z","receivedAt":"2018-10-24T03:14:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lucas De Marchi <lucas.demarchi@intel.com> writes:\n\n>> Yes, I agree on both counts (i.e. it was totally unclear what\n>> problem is being solved and what the root cause of the problem is,\n>> and we would want a new test to protect this \"fix\" from getting\n>> broken in the future.\n>\n> have you seen I sent a v2 with proper test?\n\nNo, otherwise I wouln't have said it needs tests, and no, because I\nhaven't seen the v2, I do not know if it came with proper test or\nother issues pointed out and fixes suggested in the review round\nwere addressed in v2.  Sorry.\n\nWhen you ask such a question, please accompany it with \"this is the\nmessage-id\" to avoid the receiver of the question locating a wrong\nversion of your patch from the archive.\n\nThanks.\n"},{"id":"361357","messageId":"20181024024309.GA22379@ldmartin-desk.jf.intel.com","threadId":"49526","inReplyTo":"xmqqh8hcqeho.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] range-diff: allow to diff files regardless submodule","fromName":"Lucas De Marchi","fromEmail":"lucas.demarchi@intel.com","sentAt":"2018-10-24T02:43:09Z","receivedAt":"2018-10-24T03:41:42Z","isPatch":true,"sender":{"key":"lucas.demarchi@intel.com","avatar":null},"body":"On Wed, Oct 24, 2018 at 11:12:51AM +0900, Junio C Hamano wrote:\n> Lucas De Marchi <lucas.demarchi@intel.com> writes:\n> \n> >> Yes, I agree on both counts (i.e. it was totally unclear what\n> >> problem is being solved and what the root cause of the problem is,\n> >> and we would want a new test to protect this \"fix\" from getting\n> >> broken in the future.\n> >\n> > have you seen I sent a v2 with proper test?\n> \n> No, otherwise I wouln't have said it needs tests, and no, because I\n> haven't seen the v2, I do not know if it came with proper test or\n> other issues pointed out and fixes suggested in the review round\n> were addressed in v2.  Sorry.\n\nYour reply arrived just a little after I sent the v2, so I thought it\nwas just the race and you would end up seeing the unread email in the\nsame thread. Sorry for not including the msg id:\n20181011081750.24240-1-lucas.demarchi@intel.com\n\nthanks\nLucas De Marchi\n\n> \n> When you ask such a question, please accompany it with \"this is the\n> message-id\" to avoid the receiver of the question locating a wrong\n> version of your patch from the archive.\n> \n> Thanks.\n"},{"id":"361363","messageId":"xmqqtvlcnd1w.fsf@gitster-ct.c.googlers.com","threadId":"49526","inReplyTo":"20181024024309.GA22379@ldmartin-desk.jf.intel.com","subject":"Re: [PATCH] range-diff: allow to diff files regardless submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T05:12:11Z","receivedAt":"2018-10-24T05:12:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lucas De Marchi <lucas.demarchi@intel.com> writes:\n\n> Your reply arrived just a little after I sent the v2, so I thought it\n> was just the race and you would end up seeing the unread email in the\n> same thread. Sorry for not including the msg id:\n> 20181011081750.24240-1-lucas.demarchi@intel.com\n\nOK, then I am not surprised that I do not recall seeing such an old\nmessage ;-)  Let me take a look.\n\nThanks.\n"},{"id":"361364","messageId":"xmqqk1m7orbg.fsf@gitster-ct.c.googlers.com","threadId":"49526","inReplyTo":"xmqqtvlcnd1w.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] range-diff: allow to diff files regardless submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T05:18:43Z","receivedAt":"2018-10-24T05:18:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Lucas De Marchi <lucas.demarchi@intel.com> writes:\n>\n>> Your reply arrived just a little after I sent the v2, so I thought it\n>> was just the race and you would end up seeing the unread email in the\n>> same thread. Sorry for not including the msg id:\n>> 20181011081750.24240-1-lucas.demarchi@intel.com\n>\n> OK, then I am not surprised that I do not recall seeing such an old\n> message ;-)  Let me take a look.\n\nHeh, it turns out that that is the version that has been queued in\n'next' for about a week already.\n\nOne issue that was pointed out in v1 was that the log message was\nunclear; it seems v2 didn't address that at all, though.\n\nThanks.\n\n"},{"id":"361431","messageId":"20181024171940.GA2921@ldmartin-desk.jf.intel.com","threadId":"49526","inReplyTo":"xmqqk1m7orbg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] range-diff: allow to diff files regardless submodule","fromName":"Lucas De Marchi","fromEmail":"lucas.demarchi@intel.com","sentAt":"2018-10-24T17:19:41Z","receivedAt":"2018-10-24T17:19:43Z","isPatch":true,"sender":{"key":"lucas.demarchi@intel.com","avatar":null},"body":"On Wed, Oct 24, 2018 at 02:18:43PM +0900, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Lucas De Marchi <lucas.demarchi@intel.com> writes:\n> >\n> >> Your reply arrived just a little after I sent the v2, so I thought it\n> >> was just the race and you would end up seeing the unread email in the\n> >> same thread. Sorry for not including the msg id:\n> >> 20181011081750.24240-1-lucas.demarchi@intel.com\n> >\n> > OK, then I am not surprised that I do not recall seeing such an old\n> > message ;-)  Let me take a look.\n> \n> Heh, it turns out that that is the version that has been queued in\n> 'next' for about a week already.\n> \n> One issue that was pointed out in v1 was that the log message was\n> unclear; it seems v2 didn't address that at all, though.\n\nok, let me try to expand it.\n\nthanks\nLucas De Marchi\n\n> \n> Thanks.\n> \n"},{"id":"361433","messageId":"20181024194617.425-1-lucas.demarchi@intel.com","threadId":"49526","inReplyTo":"xmqqk1m7orbg.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v3] range-diff: allow to diff files regardless of submodule config","fromName":"Lucas De Marchi","fromEmail":"lucas.demarchi@intel.com","sentAt":"2018-10-24T19:46:17Z","receivedAt":"2018-10-24T19:46:40Z","isPatch":true,"sender":{"key":"lucas.demarchi@intel.com","avatar":null},"body":"If we have `submodule.diff = log' in the configuration file\nor `--submodule=log' is given as argument, range-diff fails\nto compare both diffs and we only get the following output:\n\n    Submodule a 0000000...0000000 (new submodule)\n\nEven if the repository doesn't have any submodule.\n\nThat's because the mode in diff_filespec is not correct and when\nflushing the diff, down in builtin_diff() we will enter the condition:\n\n\tif (o->submodule_format == DIFF_SUBMODULE_LOG &&\n\t    (!one->mode || S_ISGITLINK(one->mode)) &&\n\t    (!two->mode || S_ISGITLINK(two->mode))) {\n\t\tshow_submodule_summary(o, one->path ? one->path : two->path,\n\t\t\t\t&one->oid, &two->oid,\n\t\t\t\ttwo->dirty_submodule);\n\t\treturn;\n\nIt turns out that S_ISGITLINK will return true (mode == 0160000 here).\nSimilar thing happens if submodule.diff is \"diff\".\n\nDo like it's done in grep.c when calling fill_filespec() and force it to\nbe recognized as a file by adding S_IFREG to the mode.\n\nSigned-off-by: Lucas De Marchi <lucas.demarchi@intel.com>\nAcked-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n\nv2: Add test to make sure we don't regress\n\nv3: Extend commit mesage with better explanation and base it on maint branch\n    since it's a bug fix\n\n range-diff.c          |  2 +-\n t/t3206-range-diff.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex b6b9abac26..3d6aa2330a 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -334,7 +334,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)\n {\n \tstruct diff_filespec *spec = alloc_filespec(name);\n \n-\tfill_filespec(spec, &null_oid, 0, 0644);\n+\tfill_filespec(spec, &null_oid, 0, 0100644);\n \tspec->data = (char *)p;\n \tspec->size = strlen(p);\n \tspec->should_munmap = 0;\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 2237c7f4af..518c9a527d 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -122,6 +122,35 @@ test_expect_success 'changed commit' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'changed commit with sm config' '\n+\tgit range-diff --no-color --submodule=log topic...changed >actual &&\n+\tcat >expected <<-EOF &&\n+\t1:  4de457d = 1:  a4b3333 s/5/A/\n+\t2:  fccce22 = 2:  f51d370 s/4/A/\n+\t3:  147e64e ! 3:  0559556 s/11/B/\n+\t    @@ -10,7 +10,7 @@\n+\t      9\n+\t      10\n+\t     -11\n+\t    -+B\n+\t    ++BB\n+\t      12\n+\t      13\n+\t      14\n+\t4:  a63e992 ! 4:  d966c5c s/12/B/\n+\t    @@ -8,7 +8,7 @@\n+\t     @@\n+\t      9\n+\t      10\n+\t    - B\n+\t    + BB\n+\t     -12\n+\t     +B\n+\t      13\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'changed message' '\n \tgit range-diff --no-color topic...changed-message >actual &&\n \tsed s/Z/\\ /g >expected <<-EOF &&\n-- \n2.19.1.543.g4e5b40e2e8\n\n"}]}