git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] range-diff: allow to diff files regardless submodule
@ 2018-10-10 15:09 Lucas De Marchi
  2018-10-11  0:02 ` brian m. carlson
  2018-10-11  7:42 ` Ævar Arnfjörð Bjarmason
  0 siblings, 2 replies; 13+ messages in thread
From: Lucas De Marchi @ 2018-10-10 15:09 UTC (permalink / raw)
  To: git
  Cc: Johannes Schindelin, Thomas Gummerer, Eric Sunshine,
	Junio C Hamano, lucas.de.marchi

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);
 	spec->data = (char *)p;
 	spec->size = strlen(p);
 	spec->should_munmap = 0;
-- 
2.19.1.1.g8c3cf03f71


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] range-diff: allow to diff files regardless submodule
  2018-10-10 15:09 [PATCH] range-diff: allow to diff files regardless submodule Lucas De Marchi
@ 2018-10-11  0:02 ` brian m. carlson
  2018-10-11  7:50   ` Lucas De Marchi
  2018-10-11  7:42 ` Ævar Arnfjörð Bjarmason
  1 sibling, 1 reply; 13+ messages in thread
From: brian m. carlson @ 2018-10-11  0:02 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: git, Johannes Schindelin, Thomas Gummerer, Eric Sunshine,
	Junio C Hamano, lucas.de.marchi

[-- Attachment #1: Type: text/plain, Size: 1134 bytes --]

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?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] range-diff: allow to diff files regardless submodule
  2018-10-10 15:09 [PATCH] range-diff: allow to diff files regardless submodule Lucas De Marchi
  2018-10-11  0:02 ` brian m. carlson
@ 2018-10-11  7:42 ` Ævar Arnfjörð Bjarmason
  2018-10-11  8:14   ` Lucas De Marchi
  2018-10-11  8:25   ` [PATCH] " Junio C Hamano
  1 sibling, 2 replies; 13+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-10-11  7:42 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: git, Johannes Schindelin, Thomas Gummerer, Eric Sunshine,
	Junio C Hamano, lucas.de.marchi


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.

> 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;

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] range-diff: allow to diff files regardless submodule
  2018-10-11  0:02 ` brian m. carlson
@ 2018-10-11  7:50   ` Lucas De Marchi
  2018-10-12  9:24     ` Johannes Schindelin
  0 siblings, 1 reply; 13+ messages in thread
From: Lucas De Marchi @ 2018-10-11  7:50 UTC (permalink / raw)
  To: sandals, Lucas De Marchi, git, johannes.schindelin, t.gummerer,
	sunshine, gitster

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.

Lucas De Marchi
> --
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204



-- 
Lucas De Marchi

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] range-diff: allow to diff files regardless submodule
  2018-10-11  7:42 ` Ævar Arnfjörð Bjarmason
@ 2018-10-11  8:14   ` Lucas De Marchi
  2018-10-11  8:17     ` [PATCH v2] " Lucas De Marchi
  2018-10-11  8:25   ` [PATCH] " Junio C Hamano
  1 sibling, 1 reply; 13+ messages in thread
From: Lucas De Marchi @ 2018-10-11  8:14 UTC (permalink / raw)
  To: avarab
  Cc: Lucas De Marchi, git, johannes.schindelin, t.gummerer, sunshine, gitster

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

>
> > 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2] range-diff: allow to diff files regardless submodule
  2018-10-11  8:14   ` Lucas De Marchi
@ 2018-10-11  8:17     ` " Lucas De Marchi
  2018-10-12  9:26       ` Johannes Schindelin
  0 siblings, 1 reply; 13+ messages in thread
From: Lucas De Marchi @ 2018-10-11  8:17 UTC (permalink / raw)
  To: lucas.de.marchi
  Cc: avarab, git, gitster, johannes.schindelin, lucas.demarchi,
	sunshine, t.gummerer

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(-)

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] range-diff: allow to diff files regardless submodule
  2018-10-11  7:42 ` Ævar Arnfjörð Bjarmason
  2018-10-11  8:14   ` Lucas De Marchi
@ 2018-10-11  8:25   ` " Junio C Hamano
  2018-10-23 14:07     ` Lucas De Marchi
  1 sibling, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2018-10-11  8:25 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Lucas De Marchi, git, Johannes Schindelin, Thomas Gummerer,
	Eric Sunshine, lucas.de.marchi

Æ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.

Thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] range-diff: allow to diff files regardless submodule
  2018-10-11  7:50   ` Lucas De Marchi
@ 2018-10-12  9:24     ` Johannes Schindelin
  2018-10-12 23:17       ` brian m. carlson
  0 siblings, 1 reply; 13+ messages in thread
From: Johannes Schindelin @ 2018-10-12  9:24 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: sandals, Lucas De Marchi, git, t.gummerer, sunshine, gitster



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.

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

> 
> Lucas De Marchi
> > --
> > brian m. carlson: Houston, Texas, US
> > OpenPGP: https://keybase.io/bk2204
> 
> 
> 
> -- 
> Lucas De Marchi
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] range-diff: allow to diff files regardless submodule
  2018-10-11  8:17     ` [PATCH v2] " Lucas De Marchi
@ 2018-10-12  9:26       ` Johannes Schindelin
  0 siblings, 0 replies; 13+ messages in thread
From: Johannes Schindelin @ 2018-10-12  9:26 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: lucas.de.marchi, avarab, git, gitster, sunshine, t.gummerer

Hi Lucas,

On Thu, 11 Oct 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)
> 
> 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

> ---
>  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
> 
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] range-diff: allow to diff files regardless submodule
  2018-10-12  9:24     ` Johannes Schindelin
@ 2018-10-12 23:17       ` brian m. carlson
  0 siblings, 0 replies; 13+ messages in thread
From: brian m. carlson @ 2018-10-12 23:17 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Lucas De Marchi, Lucas De Marchi, git, t.gummerer, sunshine, gitster

[-- Attachment #1: Type: text/plain, Size: 2756 bytes --]

On Fri, Oct 12, 2018 at 11:24:43AM +0200, Johannes Schindelin wrote:
> 
> 
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] range-diff: allow to diff files regardless submodule
  2018-10-11  8:25   ` [PATCH] " Junio C Hamano
@ 2018-10-23 14:07     ` Lucas De Marchi
  2018-10-24  2:12       ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Lucas De Marchi @ 2018-10-23 14:07 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Ævar Arnfjörð Bjarmason, git, Johannes Schindelin,
	Thomas Gummerer, Eric Sunshine, lucas.de.marchi

On Thu, Oct 11, 2018 at 05:25:02PM +0900, Junio C Hamano wrote:
> Æ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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] range-diff: allow to diff files regardless submodule
  2018-10-23 14:07     ` Lucas De Marchi
@ 2018-10-24  2:12       ` Junio C Hamano
  2018-10-24  2:43         ` Lucas De Marchi
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2018-10-24  2:12 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: Ævar Arnfjörð Bjarmason, git, Johannes Schindelin,
	Thomas Gummerer, Eric Sunshine, lucas.de.marchi

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.

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] range-diff: allow to diff files regardless submodule
  2018-10-24  2:12       ` Junio C Hamano
@ 2018-10-24  2:43         ` Lucas De Marchi
  0 siblings, 0 replies; 13+ messages in thread
From: Lucas De Marchi @ 2018-10-24  2:43 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Ævar Arnfjörð Bjarmason, git, Johannes Schindelin,
	Thomas Gummerer, Eric Sunshine, lucas.de.marchi

On Wed, Oct 24, 2018 at 11:12:51AM +0900, Junio C Hamano wrote:
> 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

> 
> 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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-10 15:09 [PATCH] range-diff: allow to diff files regardless submodule Lucas De Marchi
2018-10-11  0:02 ` brian m. carlson
2018-10-11  7:50   ` Lucas De Marchi
2018-10-12  9:24     ` Johannes Schindelin
2018-10-12 23:17       ` brian m. carlson
2018-10-11  7:42 ` Ævar Arnfjörð Bjarmason
2018-10-11  8:14   ` Lucas De Marchi
2018-10-11  8:17     ` [PATCH v2] " Lucas De Marchi
2018-10-12  9:26       ` Johannes Schindelin
2018-10-11  8:25   ` [PATCH] " Junio C Hamano
2018-10-23 14:07     ` Lucas De Marchi
2018-10-24  2:12       ` Junio C Hamano
2018-10-24  2:43         ` Lucas De Marchi

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox