threads / patch / 49526

patchrange-diff: allow to diff files regardless submodule

Subject: [PATCH] range-diff: allow to diff files regardless submodule

## tl;dr

17 messages between Oct 10, 2018 and Oct 24, 2018. Diffs are folded; open one to read it.

replies: 16people: 6as markdown or json

Lucas De Marchi· Oct 10, 2018, 15:09 UTC · lore

Do like it's done in grep so mode doesn't end up as 0160000, which means range-diff doesn't work if one has "submodule.diff = log" in the configuration. Without this while using range-diff I only get a

    Submodule a 0000000...0000000 (new submodule)
instead of the diff between the revisions.
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to range-diff.c +1 −1
diff --git a/range-diff.c b/range-diff.c
index 60edb2f518..bd8083f2d1 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
 {
 	struct diff_filespec *spec = alloc_filespec(name);
 
-	fill_filespec(spec, &null_oid, 0, 0644);
+	fill_filespec(spec, &null_oid, 0, 0100644);
 	spec->data = (char *)p;
 	spec->size = strlen(p);
 	spec->should_munmap = 0;

-- 
2.19.1.1.g8c3cf03f71
brian m. carlson· Oct 11, 2018, 00:02 UTC · re: Lucas De Marchi · lore

Re: [PATCH] range-diff: allow to diff files regardless submodule

On Wed, Oct 10, 2018 at 08:09:16AM -0700, Lucas De Marchi wrote:
Show 24 quoted lines
> Do like it's done in grep so mode doesn't end up as
> 0160000, which means range-diff doesn't work if one has
> "submodule.diff = log" in the configuration. Without this
> while using range-diff I only get a
> 
>     Submodule a 0000000...0000000 (new submodule)
> 
> instead of the diff between the revisions.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  range-diff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/range-diff.c b/range-diff.c
> index 60edb2f518..bd8083f2d1 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
>  {
>  	struct diff_filespec *spec = alloc_filespec(name);
>  
> -	fill_filespec(spec, &null_oid, 0, 0644);
> +	fill_filespec(spec, &null_oid, 0, 0100644);

If we have a system that has different mode values from the common Unix ones, is this still correct or does it need to change?

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Lucas De Marchi· Oct 11, 2018, 07:50 UTC · re: brian m. carlson · lore

Re: [PATCH] range-diff: allow to diff files regardless submodule

On Wed, Oct 10, 2018 at 5:02 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 29 quoted lines
>
> On Wed, Oct 10, 2018 at 08:09:16AM -0700, Lucas De Marchi wrote:
> > Do like it's done in grep so mode doesn't end up as
> > 0160000, which means range-diff doesn't work if one has
> > "submodule.diff = log" in the configuration. Without this
> > while using range-diff I only get a
> >
> >     Submodule a 0000000...0000000 (new submodule)
> >
> > instead of the diff between the revisions.
> >
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > ---
> >  range-diff.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/range-diff.c b/range-diff.c
> > index 60edb2f518..bd8083f2d1 100644
> > --- a/range-diff.c
> > +++ b/range-diff.c
> > @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
> >  {
> >       struct diff_filespec *spec = alloc_filespec(name);
> >
> > -     fill_filespec(spec, &null_oid, 0, 0644);
> > +     fill_filespec(spec, &null_oid, 0, 0100644);
>
> If we have a system that has different mode values from the common Unix
> ones, is this still correct or does it need to change?

From what I can see this would still be correct, or at least git-grep implementation would be broken.

Lucas De Marchi
> --
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204
-- 
Lucas De Marchi
Johannes Schindelin· Oct 12, 2018, 09:24 UTC · re: Lucas De Marchi · lore

Re: [PATCH] range-diff: allow to diff files regardless submodule

On Thu, 11 Oct 2018, Lucas De Marchi wrote:
Show 34 quoted lines
> On Wed, Oct 10, 2018 at 5:02 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> >
> > On Wed, Oct 10, 2018 at 08:09:16AM -0700, Lucas De Marchi wrote:
> > > Do like it's done in grep so mode doesn't end up as
> > > 0160000, which means range-diff doesn't work if one has
> > > "submodule.diff = log" in the configuration. Without this
> > > while using range-diff I only get a
> > >
> > >     Submodule a 0000000...0000000 (new submodule)
> > >
> > > instead of the diff between the revisions.
> > >
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > >  range-diff.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/range-diff.c b/range-diff.c
> > > index 60edb2f518..bd8083f2d1 100644
> > > --- a/range-diff.c
> > > +++ b/range-diff.c
> > > @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
> > >  {
> > >       struct diff_filespec *spec = alloc_filespec(name);
> > >
> > > -     fill_filespec(spec, &null_oid, 0, 0644);
> > > +     fill_filespec(spec, &null_oid, 0, 0100644);
> >
> > If we have a system that has different mode values from the common Unix
> > ones, is this still correct or does it need to change?
> 
> From what I can see this would still be correct, or at least git-grep
> implementation would be broken.

As you can see from the Windows port: we are stuck with the simplistic POSIX permissions in Git, and platforms that have a different permission system have to emulate it.

We only use preciously few bit masks, anyway. For example, we do not really use the lower 12 bits for anything but "executable or not?"

So Lucas' patch is correct, AFAICT.

Ciao, Dscho

Show 11 quoted lines
> 
> Lucas De Marchi
> > --
> > brian m. carlson: Houston, Texas, US
> > OpenPGP: https://keybase.io/bk2204
> 
> 
> 
> -- 
> Lucas De Marchi
>
brian m. carlson· Oct 12, 2018, 23:17 UTC · re: Johannes Schindelin · lore

Re: [PATCH] range-diff: allow to diff files regardless submodule

On Fri, Oct 12, 2018 at 11:24:43AM +0200, Johannes Schindelin wrote:
Show 42 quoted lines
> 
> 
> On Thu, 11 Oct 2018, Lucas De Marchi wrote:
> 
> > On Wed, Oct 10, 2018 at 5:02 PM brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> > >
> > > On Wed, Oct 10, 2018 at 08:09:16AM -0700, Lucas De Marchi wrote:
> > > > Do like it's done in grep so mode doesn't end up as
> > > > 0160000, which means range-diff doesn't work if one has
> > > > "submodule.diff = log" in the configuration. Without this
> > > > while using range-diff I only get a
> > > >
> > > >     Submodule a 0000000...0000000 (new submodule)
> > > >
> > > > instead of the diff between the revisions.
> > > >
> > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > ---
> > > >  range-diff.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/range-diff.c b/range-diff.c
> > > > index 60edb2f518..bd8083f2d1 100644
> > > > --- a/range-diff.c
> > > > +++ b/range-diff.c
> > > > @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
> > > >  {
> > > >       struct diff_filespec *spec = alloc_filespec(name);
> > > >
> > > > -     fill_filespec(spec, &null_oid, 0, 0644);
> > > > +     fill_filespec(spec, &null_oid, 0, 0100644);
> > >
> > > If we have a system that has different mode values from the common Unix
> > > ones, is this still correct or does it need to change?
> > 
> > From what I can see this would still be correct, or at least git-grep
> > implementation would be broken.
> 
> As you can see from the Windows port: we are stuck with the simplistic
> POSIX permissions in Git, and platforms that have a different permission
> system have to emulate it.

I think I may not have explained myself well. There are a small number of POSIXy systems which have mode bits that differ from the common ones (e.g., a plain file is something other than 0100000). I think one person mentioned on the list that they have a homebrew Unix that works this way, and I think I may have heard of some minor commercial Unices that work this way as well.

My question was intended to ask whether we should be using an OS-provided constant (e.g., S_IFREG) that represented that value differently because it was a system value or whether it was the internal Git representation.

I hadn't intended to inquire about Windows, as I was fairly confident that this syntax does indeed work there through our compatibility layers (because it has in the past even when we've had these kinds of issues on other Unices). But I'm glad that you chimed in and confirmed that it does.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Ævar Arnfjörð Bjarmason· Oct 11, 2018, 07:42 UTC · re: Lucas De Marchi · lore

Re: [PATCH] range-diff: allow to diff files regardless submodule

On Wed, Oct 10 2018, Lucas De Marchi wrote:
Show 6 quoted lines
> Do like it's done in grep so mode doesn't end up as
> 0160000, which means range-diff doesn't work if one has
> "submodule.diff = log" in the configuration. Without this
> while using range-diff I only get a
>
>     Submodule a 0000000...0000000 (new submodule)

I'm not familiar enough with this to tell what the real problem is that's being solved from the commit message, but if it means that now range-diff works in some configuration, presumably that can be reduced to a simple set of commands that didn't work before but now does, and therefore a test in t/t3206-range-diff.sh.

Show 20 quoted lines
> instead of the diff between the revisions.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  range-diff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/range-diff.c b/range-diff.c
> index 60edb2f518..bd8083f2d1 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
>  {
>  	struct diff_filespec *spec = alloc_filespec(name);
>
> -	fill_filespec(spec, &null_oid, 0, 0644);
> +	fill_filespec(spec, &null_oid, 0, 0100644);
>  	spec->data = (char *)p;
>  	spec->size = strlen(p);
>  	spec->should_munmap = 0;
Lucas De Marchi· Oct 11, 2018, 08:14 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] range-diff: allow to diff files regardless submodule

On Thu, Oct 11, 2018 at 12:42 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 16 quoted lines
>
>
> On Wed, Oct 10 2018, Lucas De Marchi wrote:
>
> > Do like it's done in grep so mode doesn't end up as
> > 0160000, which means range-diff doesn't work if one has
> > "submodule.diff = log" in the configuration. Without this
> > while using range-diff I only get a
> >
> >     Submodule a 0000000...0000000 (new submodule)
>
> I'm not familiar enough with this to tell what the real problem is
> that's being solved from the commit message, but if it means that now
> range-diff works in some configuration, presumably that can be reduced
> to a simple set of commands that didn't work before but now does, and
> therefore a test in t/t3206-range-diff.sh.

$ git config --global diff.submodule log $ git range-diff

This produces the output above

$ git config --global diff.submodule short $ git range-diff

This blocks forever in a wait4() call and prints this when terminated: fatal: exec 'diff': cd to 'a' failed: Not a directory

Lucas De Marchi
Show 21 quoted lines
>
> > instead of the diff between the revisions.
> >
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > ---
> >  range-diff.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/range-diff.c b/range-diff.c
> > index 60edb2f518..bd8083f2d1 100644
> > --- a/range-diff.c
> > +++ b/range-diff.c
> > @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
> >  {
> >       struct diff_filespec *spec = alloc_filespec(name);
> >
> > -     fill_filespec(spec, &null_oid, 0, 0644);
> > +     fill_filespec(spec, &null_oid, 0, 0100644);
> >       spec->data = (char *)p;
> >       spec->size = strlen(p);
> >       spec->should_munmap = 0;
-- 
Lucas De Marchi
Lucas De Marchi· Oct 11, 2018, 08:17 UTC · re: Lucas De Marchi · lore

[PATCH v2] range-diff: allow to diff files regardless submodule

Do like it's done in grep so mode doesn't end up as 0160000, which means range-diff doesn't work if one has "submodule.diff = log" in the configuration. Without this while using range-diff I only get a

    Submodule a 0000000...0000000 (new submodule)
instead of the diff between the revisions.
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 range-diff.c          |  2 +-
 t/t3206-range-diff.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)
Show changes to 2 files +30 −1

range-diff.c, t/t3206-range-diff.sh

diff --git a/range-diff.c b/range-diff.c
index 60edb2f518..bd8083f2d1 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
 {
 	struct diff_filespec *spec = alloc_filespec(name);
 
-	fill_filespec(spec, &null_oid, 0, 0644);
+	fill_filespec(spec, &null_oid, 0, 0100644);
 	spec->data = (char *)p;
 	spec->size = strlen(p);
 	spec->should_munmap = 0;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 045aca1c18..6aae364171 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -122,6 +122,35 @@ test_expect_success 'changed commit' '
 	test_cmp expected actual
 '
 
+test_expect_success 'changed commit with sm config' '
+	git range-diff --no-color --submodule=log topic...changed >actual &&
+	cat >expected <<-EOF &&
+	1:  4de457d = 1:  a4b3333 s/5/A/
+	2:  fccce22 = 2:  f51d370 s/4/A/
+	3:  147e64e ! 3:  0559556 s/11/B/
+	    @@ -10,7 +10,7 @@
+	      9
+	      10
+	     -11
+	    -+B
+	    ++BB
+	      12
+	      13
+	      14
+	4:  a63e992 ! 4:  d966c5c s/12/B/
+	    @@ -8,7 +8,7 @@
+	     @@
+	      9
+	      10
+	    - B
+	    + BB
+	     -12
+	     +B
+	      13
+	EOF
+	test_cmp expected actual
+'
+
 test_expect_success 'no commits on one side' '
 	git commit --amend -m "new message" &&
 	git range-diff master HEAD@{1} HEAD

-- 
2.19.1.1.g8c3cf03f71
Johannes Schindelin· Oct 12, 2018, 09:26 UTC · re: Lucas De Marchi · lore

Re: [PATCH v2] range-diff: allow to diff files regardless submodule

Hi Lucas,
On Thu, 11 Oct 2018, Lucas De Marchi wrote:
Show 10 quoted lines
> Do like it's done in grep so mode doesn't end up as
> 0160000, which means range-diff doesn't work if one has
> "submodule.diff = log" in the configuration. Without this
> while using range-diff I only get a
> 
>     Submodule a 0000000...0000000 (new submodule)
> 
> instead of the diff between the revisions.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Thank you for this contribution, which I am glad to ACK.

I am especially happy that you added a regression test so that we are confident not to break this again.

Ciao, Dscho

Show 62 quoted lines
> ---
>  range-diff.c          |  2 +-
>  t/t3206-range-diff.sh | 29 +++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/range-diff.c b/range-diff.c
> index 60edb2f518..bd8083f2d1 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -354,7 +354,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
>  {
>  	struct diff_filespec *spec = alloc_filespec(name);
>  
> -	fill_filespec(spec, &null_oid, 0, 0644);
> +	fill_filespec(spec, &null_oid, 0, 0100644);
>  	spec->data = (char *)p;
>  	spec->size = strlen(p);
>  	spec->should_munmap = 0;
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 045aca1c18..6aae364171 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -122,6 +122,35 @@ test_expect_success 'changed commit' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'changed commit with sm config' '
> +	git range-diff --no-color --submodule=log topic...changed >actual &&
> +	cat >expected <<-EOF &&
> +	1:  4de457d = 1:  a4b3333 s/5/A/
> +	2:  fccce22 = 2:  f51d370 s/4/A/
> +	3:  147e64e ! 3:  0559556 s/11/B/
> +	    @@ -10,7 +10,7 @@
> +	      9
> +	      10
> +	     -11
> +	    -+B
> +	    ++BB
> +	      12
> +	      13
> +	      14
> +	4:  a63e992 ! 4:  d966c5c s/12/B/
> +	    @@ -8,7 +8,7 @@
> +	     @@
> +	      9
> +	      10
> +	    - B
> +	    + BB
> +	     -12
> +	     +B
> +	      13
> +	EOF
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'no commits on one side' '
>  	git commit --amend -m "new message" &&
>  	git range-diff master HEAD@{1} HEAD
> -- 
> 2.19.1.1.g8c3cf03f71
> 
>
Junio C Hamano· Oct 11, 2018, 08:25 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] range-diff: allow to diff files regardless submodule

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 14 quoted lines
> On Wed, Oct 10 2018, Lucas De Marchi wrote:
>
>> Do like it's done in grep so mode doesn't end up as
>> 0160000, which means range-diff doesn't work if one has
>> "submodule.diff = log" in the configuration. Without this
>> while using range-diff I only get a
>>
>>     Submodule a 0000000...0000000 (new submodule)
>
> I'm not familiar enough with this to tell what the real problem is
> that's being solved from the commit message, but if it means that now
> range-diff works in some configuration, presumably that can be reduced
> to a simple set of commands that didn't work before but now does, and
> therefore a test in t/t3206-range-diff.sh.

Yes, I agree on both counts (i.e. it was totally unclear what problem is being solved and what the root cause of the problem is, and we would want a new test to protect this "fix" from getting broken in the future.

Thanks.
Lucas De Marchi· Oct 23, 2018, 14:07 UTC · re: Junio C Hamano · lore

Re: [PATCH] range-diff: allow to diff files regardless submodule

On Thu, Oct 11, 2018 at 05:25:02PM +0900, Junio C Hamano wrote:
Show 21 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > On Wed, Oct 10 2018, Lucas De Marchi wrote:
> >
> >> Do like it's done in grep so mode doesn't end up as
> >> 0160000, which means range-diff doesn't work if one has
> >> "submodule.diff = log" in the configuration. Without this
> >> while using range-diff I only get a
> >>
> >>     Submodule a 0000000...0000000 (new submodule)
> >
> > I'm not familiar enough with this to tell what the real problem is
> > that's being solved from the commit message, but if it means that now
> > range-diff works in some configuration, presumably that can be reduced
> > to a simple set of commands that didn't work before but now does, and
> > therefore a test in t/t3206-range-diff.sh.
> 
> Yes, I agree on both counts (i.e. it was totally unclear what
> problem is being solved and what the root cause of the problem is,
> and we would want a new test to protect this "fix" from getting
> broken in the future.
have you seen I sent a v2 with proper test?

thanks Lucas De Marchi

> 
> Thanks.
Junio C Hamano· Oct 24, 2018, 02:12 UTC · re: Lucas De Marchi · lore

Re: [PATCH] range-diff: allow to diff files regardless submodule

Lucas De Marchi <lucas.demarchi@intel.com> writes:
Show 6 quoted lines
>> Yes, I agree on both counts (i.e. it was totally unclear what
>> problem is being solved and what the root cause of the problem is,
>> and we would want a new test to protect this "fix" from getting
>> broken in the future.
>
> have you seen I sent a v2 with proper test?

No, otherwise I wouln't have said it needs tests, and no, because I haven't seen the v2, I do not know if it came with proper test or other issues pointed out and fixes suggested in the review round were addressed in v2. Sorry.

When you ask such a question, please accompany it with "this is the message-id" to avoid the receiver of the question locating a wrong version of your patch from the archive.

Thanks.
Lucas De Marchi· Oct 24, 2018, 02:43 UTC · re: Junio C Hamano · lore

Re: [PATCH] range-diff: allow to diff files regardless submodule

On Wed, Oct 24, 2018 at 11:12:51AM +0900, Junio C Hamano wrote:
Show 13 quoted lines
> Lucas De Marchi <lucas.demarchi@intel.com> writes:
> 
> >> Yes, I agree on both counts (i.e. it was totally unclear what
> >> problem is being solved and what the root cause of the problem is,
> >> and we would want a new test to protect this "fix" from getting
> >> broken in the future.
> >
> > have you seen I sent a v2 with proper test?
> 
> No, otherwise I wouln't have said it needs tests, and no, because I
> haven't seen the v2, I do not know if it came with proper test or
> other issues pointed out and fixes suggested in the review round
> were addressed in v2.  Sorry.

Your reply arrived just a little after I sent the v2, so I thought it was just the race and you would end up seeing the unread email in the same thread. Sorry for not including the msg id: 20181011081750.24240-1-lucas.demarchi@intel.com

thanks Lucas De Marchi

Show 6 quoted lines
> 
> When you ask such a question, please accompany it with "this is the
> message-id" to avoid the receiver of the question locating a wrong
> version of your patch from the archive.
> 
> Thanks.
Junio C Hamano· Oct 24, 2018, 05:12 UTC · re: Lucas De Marchi · lore

Re: [PATCH] range-diff: allow to diff files regardless submodule

Lucas De Marchi <lucas.demarchi@intel.com> writes:
> Your reply arrived just a little after I sent the v2, so I thought it
> was just the race and you would end up seeing the unread email in the
> same thread. Sorry for not including the msg id:
> 20181011081750.24240-1-lucas.demarchi@intel.com

OK, then I am not surprised that I do not recall seeing such an old message ;-) Let me take a look.

Thanks.
Junio C Hamano· Oct 24, 2018, 05:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] range-diff: allow to diff files regardless submodule

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> Lucas De Marchi <lucas.demarchi@intel.com> writes:
>
>> Your reply arrived just a little after I sent the v2, so I thought it
>> was just the race and you would end up seeing the unread email in the
>> same thread. Sorry for not including the msg id:
>> 20181011081750.24240-1-lucas.demarchi@intel.com
>
> OK, then I am not surprised that I do not recall seeing such an old
> message ;-)  Let me take a look.

Heh, it turns out that that is the version that has been queued in 'next' for about a week already.

One issue that was pointed out in v1 was that the log message was unclear; it seems v2 didn't address that at all, though.

Thanks.
Lucas De Marchi· Oct 24, 2018, 17:19 UTC · re: Junio C Hamano · lore

Re: [PATCH] range-diff: allow to diff files regardless submodule

On Wed, Oct 24, 2018 at 02:18:43PM +0900, Junio C Hamano wrote:
Show 17 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Lucas De Marchi <lucas.demarchi@intel.com> writes:
> >
> >> Your reply arrived just a little after I sent the v2, so I thought it
> >> was just the race and you would end up seeing the unread email in the
> >> same thread. Sorry for not including the msg id:
> >> 20181011081750.24240-1-lucas.demarchi@intel.com
> >
> > OK, then I am not surprised that I do not recall seeing such an old
> > message ;-)  Let me take a look.
> 
> Heh, it turns out that that is the version that has been queued in
> 'next' for about a week already.
> 
> One issue that was pointed out in v1 was that the log message was
> unclear; it seems v2 didn't address that at all, though.
ok, let me try to expand it.

thanks Lucas De Marchi

> 
> Thanks.
>
Lucas De Marchi· Oct 24, 2018, 19:46 UTC · re: Junio C Hamano · lore

[PATCH v3] range-diff: allow to diff files regardless of submodule config

If we have `submodule.diff = log' in the configuration file or `--submodule=log' is given as argument, range-diff fails to compare both diffs and we only get the following output:

    Submodule a 0000000...0000000 (new submodule)
Even if the repository doesn't have any submodule.

That's because the mode in diff_filespec is not correct and when flushing the diff, down in builtin_diff() we will enter the condition:

	if (o->submodule_format == DIFF_SUBMODULE_LOG &&
	    (!one->mode || S_ISGITLINK(one->mode)) &&
	    (!two->mode || S_ISGITLINK(two->mode))) {
		show_submodule_summary(o, one->path ? one->path : two->path,
				&one->oid, &two->oid,
				two->dirty_submodule);
		return;

It turns out that S_ISGITLINK will return true (mode == 0160000 here). Similar thing happens if submodule.diff is "diff".

Do like it's done in grep.c when calling fill_filespec() and force it to be recognized as a file by adding S_IFREG to the mode.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
v2: Add test to make sure we don't regress
v3: Extend commit mesage with better explanation and base it on maint branch
    since it's a bug fix
 range-diff.c          |  2 +-
 t/t3206-range-diff.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)
Show changes to 2 files +30 −1

range-diff.c, t/t3206-range-diff.sh

diff --git a/range-diff.c b/range-diff.c
index b6b9abac26..3d6aa2330a 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -334,7 +334,7 @@ static struct diff_filespec *get_filespec(const char *name, const char *p)
 {
 	struct diff_filespec *spec = alloc_filespec(name);
 
-	fill_filespec(spec, &null_oid, 0, 0644);
+	fill_filespec(spec, &null_oid, 0, 0100644);
 	spec->data = (char *)p;
 	spec->size = strlen(p);
 	spec->should_munmap = 0;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 2237c7f4af..518c9a527d 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -122,6 +122,35 @@ test_expect_success 'changed commit' '
 	test_cmp expected actual
 '
 
+test_expect_success 'changed commit with sm config' '
+	git range-diff --no-color --submodule=log topic...changed >actual &&
+	cat >expected <<-EOF &&
+	1:  4de457d = 1:  a4b3333 s/5/A/
+	2:  fccce22 = 2:  f51d370 s/4/A/
+	3:  147e64e ! 3:  0559556 s/11/B/
+	    @@ -10,7 +10,7 @@
+	      9
+	      10
+	     -11
+	    -+B
+	    ++BB
+	      12
+	      13
+	      14
+	4:  a63e992 ! 4:  d966c5c s/12/B/
+	    @@ -8,7 +8,7 @@
+	     @@
+	      9
+	      10
+	    - B
+	    + BB
+	     -12
+	     +B
+	      13
+	EOF
+	test_cmp expected actual
+'
+
 test_expect_success 'changed message' '
 	git range-diff --no-color topic...changed-message >actual &&
 	sed s/Z/\ /g >expected <<-EOF &&

-- 
2.19.1.543.g4e5b40e2e8

← back to recent threads