Re: [PATCH] Re-re-re-fix common tail optimization

Jeff King [off-list ref] writes:

This would probably work better as 'cat'.

Yeah, I amended it without adding another "re-" to the title ;-)  The
result has been already pushed out.

quoted

+test_expect_success 'diff -U0' '
+
+	git diff -U0 | sed -e "/^index/d" -e "s/$z2047/Z/g" >actual &&
+	diff -u expect actual

Aren't we using "git diff" for the second diff there nowadays?

Some people seem to think that is a good idea, but I generally do not
like using "git diff" between expect and actual (both untracked) inside
tests.  The last "diff" is about validating what git does and using "git
diff" there would make the test meaningless when "git diff" itself is
broken.

This is especially so because comparison between untracked files is a
bolted-on afterthought and I am least confident about among the
codepaths in the whole "git diff" (it is not even my nor Linus's code).

On Sun, Dec 16, 2007 at 02:23:27PM -0800, Junio C Hamano wrote:

Yeah, I amended it without adding another "re-" to the title ;-)  The
result has been already pushed out.

OK. Too late, but it has my ack. ;)

quoted

Aren't we using "git diff" for the second diff there nowadays?

Some people seem to think that is a good idea, but I generally do not
like using "git diff" between expect and actual (both untracked) inside
tests.  The last "diff" is about validating what git does and using "git
diff" there would make the test meaningless when "git diff" itself is
broken.

I think that is a valid concern. But ISTR that were some issues with
using GNU diff. Commit 5bd74506 mentions getting rid of the dependency
in all existing tests, but gives no reason.

-Peff

El 16/12/2007, a las 23:29, Jeff King escribió:

On Sun, Dec 16, 2007 at 02:23:27PM -0800, Junio C Hamano wrote:

quoted

Yeah, I amended it without adding another "re-" to the title ;-)  The
result has been already pushed out.

OK. Too late, but it has my ack. ;)

quoted

quoted

Aren't we using "git diff" for the second diff there nowadays?

Some people seem to think that is a good idea, but I generally do not
like using "git diff" between expect and actual (both untracked)  
inside
tests.  The last "diff" is about validating what git does and using  
"git
diff" there would make the test meaningless when "git diff" itself is
broken.

I think that is a valid concern. But ISTR that were some issues with
using GNU diff. Commit 5bd74506 mentions getting rid of the dependency
in all existing tests, but gives no reason.

I'd say it's safe and sensible to use "git diff" in all tests *except*  
for tests of "git diff" itself.

Wincent

Hi,

On Mon, 17 Dec 2007, Wincent Colaiuta wrote:

El 16/12/2007, a las 23:29, Jeff King escribi?:

quoted

On Sun, Dec 16, 2007 at 02:23:27PM -0800, Junio C Hamano wrote:

quoted

quoted

Aren't we using "git diff" for the second diff there nowadays?

Some people seem to think that is a good idea, but I generally do 
not like using "git diff" between expect and actual (both untracked) 
inside tests.  The last "diff" is about validating what git does and 
using "git diff" there would make the test meaningless when "git 
diff" itself is broken.

I think that is a valid concern. But ISTR that were some issues with 
using GNU diff. Commit 5bd74506 mentions getting rid of the dependency 
in all existing tests, but gives no reason.

I'd say it's safe and sensible to use "git diff" in all tests *except* 
for tests of "git diff" itself.

To the contrary.  It has to test "git diff", so it must use "git diff".  
As for the reference output: we include the expected diffs as texts, and 
therefore do not really have to rely on having GNU diff installed.

Besides, we cannot even test the goodies like "rename from" by comparing 
to GNU diff's output.

Ciao,
Dscho

El 17/12/2007, a las 11:39, Johannes Schindelin escribió:

Hi,

On Mon, 17 Dec 2007, Wincent Colaiuta wrote:

quoted

El 16/12/2007, a las 23:29, Jeff King escribi?:

quoted

On Sun, Dec 16, 2007 at 02:23:27PM -0800, Junio C Hamano wrote:

quoted

quoted

Aren't we using "git diff" for the second diff there nowadays?

Some people seem to think that is a good idea, but I generally do
not like using "git diff" between expect and actual (both  
untracked)
inside tests.  The last "diff" is about validating what git does  
and
using "git diff" there would make the test meaningless when "git
diff" itself is broken.

I think that is a valid concern. But ISTR that were some issues with
using GNU diff. Commit 5bd74506 mentions getting rid of the  
dependency
in all existing tests, but gives no reason.

I'd say it's safe and sensible to use "git diff" in all tests  
*except*
for tests of "git diff" itself.

To the contrary.  It has to test "git diff", so it must use "git  
diff".

Obviously, you can only test "git diff" by actually running it.

As for the reference output: we include the expected diffs as texts,  
and
therefore do not really have to rely on having GNU diff installed.

Besides, we cannot even test the goodies like "rename from" by  
comparing
to GNU diff's output.

Sorry, I didn't make myself clear. That's not what I was proposing at  
all. I was talking about this kind of example:

+	git diff -U0 | sed -e "/^index/d" -e "s/$z2047/Z/g" >actual &&
+	diff -u expect actual

First line uses "git diff", if the second line uses "git diff" as well  
and "git diff" happens to be broken then you're using a broken tool to  
test a broken tool, as Junio already pointed out. I presumed that if  
you had read the whole thread then that would be obvious (look at the  
quoted section from Junio above).

In the example you're not interested in the details of the output  
format, only in the exit status, so it is appropriate to use diff  
instead of "git diff".

Wincent

Hi,

On Mon, 17 Dec 2007, Wincent Colaiuta wrote:

El 17/12/2007, a las 11:39, Johannes Schindelin escribi?:

quoted

On Mon, 17 Dec 2007, Wincent Colaiuta wrote:

quoted

El 16/12/2007, a las 23:29, Jeff King escribi?:

quoted

On Sun, Dec 16, 2007 at 02:23:27PM -0800, Junio C Hamano wrote:

quoted

quoted

Aren't we using "git diff" for the second diff there nowadays?

Some people seem to think that is a good idea, but I generally 
do not like using "git diff" between expect and actual (both 
untracked) inside tests.  The last "diff" is about validating 
what git does and using "git diff" there would make the test 
meaningless when "git diff" itself is broken.

I think that is a valid concern. But ISTR that were some issues 
with using GNU diff. Commit 5bd74506 mentions getting rid of the 
dependency in all existing tests, but gives no reason.

I'd say it's safe and sensible to use "git diff" in all tests 
*except* for tests of "git diff" itself.

To the contrary.  It has to test "git diff", so it must use "git 
diff".

Obviously, you can only test "git diff" by actually running it.

Sorry, I should have made clear that I meant this as funny:

	;-)

quoted

As for the reference output: we include the expected diffs as texts, 
and therefore do not really have to rely on having GNU diff installed.

Besides, we cannot even test the goodies like "rename from" by 
comparing to GNU diff's output.

Sorry, I didn't make myself clear. That's not what I was proposing at 
all. I was talking about this kind of example:

quoted

+ git diff -U0 | sed -e "/^index/d" -e "s/$z2047/Z/g" >actual &&
+ diff -u expect actual

First line uses "git diff", if the second line uses "git diff" as well 
and "git diff" happens to be broken then you're using a broken tool to 
test a broken tool, as Junio already pointed out.

Hmm.  There is some chicken-and-egg problem here (I read the thread, but 
did not really see a problem, as I assumed that _other_ tests would assure 
that "git diff --no-index" works as expected).

But as at least one released version of GNU diff has a pretty serious bug, 
I would rather not rely too much on diff.  (BTW this was the reason I 
wanted --no-index so badly.)

So yeah, the second "diff" cannot be "git diff".  Maybe "cmp", but not 
"git diff".

Ciao,
Dscho

El 17/12/2007, a las 12:57, Johannes Schindelin escribió:

Hmm.  There is some chicken-and-egg problem here (I read the thread,  
but
did not really see a problem, as I assumed that _other_ tests would  
assure
that "git diff --no-index" works as expected).

But as at least one released version of GNU diff has a pretty  
serious bug,
I would rather not rely too much on diff.  (BTW this was the reason I
wanted --no-index so badly.)

So yeah, the second "diff" cannot be "git diff".  Maybe "cmp", but not
"git diff".

Well cmp would be fine as well, seeing all we want is a boolean "is  
this the same or not" answer. (I'm not familiar with the GNU diff bug  
you speak of, but was it so bad that it couldn't even get *that*  
answer right?)

Cheers,
Wincent

On Mon, Dec 17, 2007 at 01:08:45PM +0100, Wincent Colaiuta wrote:

quoted

So yeah, the second "diff" cannot be "git diff".  Maybe "cmp", but not
"git diff".

Well cmp would be fine as well, seeing all we want is a boolean "is this the 
same or not" answer. (I'm not familiar with the GNU diff bug you speak of, 
but was it so bad that it couldn't even get *that* answer right?)

Personally I find it useful to generate the diff output when viewing
with "-v". But if there is a real reason to use cmp over diff in the
script, one can always manually go into the trash directory and run
diff.

-Peff

Wincent Colaiuta schrieb:

El 17/12/2007, a las 12:57, Johannes Schindelin escribió:

quoted

Hmm.  There is some chicken-and-egg problem here (I read the thread, but
did not really see a problem, as I assumed that _other_ tests would
assure
that "git diff --no-index" works as expected).

But as at least one released version of GNU diff has a pretty serious
bug,
I would rather not rely too much on diff.  (BTW this was the reason I
wanted --no-index so badly.)

So yeah, the second "diff" cannot be "git diff".  Maybe "cmp", but not
"git diff".

Well cmp would be fine as well, seeing all we want is a boolean "is this
the same or not" answer. (I'm not familiar with the GNU diff bug you
speak of, but was it so bad that it couldn't even get *that* answer right?)

Heh, there's at least one distribution out there (Suse 10.1) that comes with
a *cmp* that doesn't get that answer right if its output is connected to
/dev/null, which is the case when you simply 'make test'.

-- Hannes

Hi,

On Mon, 17 Dec 2007, Johannes Sixt wrote:

Wincent Colaiuta schrieb:

quoted

El 17/12/2007, a las 12:57, Johannes Schindelin escribi?:

quoted

Hmm.  There is some chicken-and-egg problem here (I read the thread, but
did not really see a problem, as I assumed that _other_ tests would
assure
that "git diff --no-index" works as expected).

But as at least one released version of GNU diff has a pretty serious
bug,
I would rather not rely too much on diff.  (BTW this was the reason I
wanted --no-index so badly.)

So yeah, the second "diff" cannot be "git diff".  Maybe "cmp", but not
"git diff".

Well cmp would be fine as well, seeing all we want is a boolean "is 
this the same or not" answer. (I'm not familiar with the GNU diff bug 
you speak of, but was it so bad that it couldn't even get *that* 
answer right?)

Heh, there's at least one distribution out there (Suse 10.1) that comes 
with a *cmp* that doesn't get that answer right if its output is 
connected to /dev/null, which is the case when you simply 'make test'.

Yeah.  That's what it was.  I even posted a patch to GNU diff, only to 
find out that it was already fixed in CVS.  Sigh.

Ciao,
Dscho