| Message ID | 20200810085343.43717-1-ray@ameretat.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t4069: test diff behavior with i-t-a paths | expand |
"Raymond E. Pasco" <ray@ameretat.dev> writes: > Add a small test suite to test the behavior of diff with intent-to-add > paths. Specifically, the diff between an i-t-a entry and a file in the > worktree should be a "new file" diff, and the diff between an i-t-a > entry and no file in the worktree should be a "deleted file" diff. > However, if --ita-visible-in-index is passed, the former should instead > be a diff from the empty blob. > > Signed-off-by: Raymond E. Pasco <ray@ameretat.dev> > --- > t/t4069-diff-intent-to-add.sh | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > create mode 100644 t/t4069-diff-intent-to-add.sh It indeed is that "add -N" appears only once in our test suite and tests around it is lacking, but I'd prefer to see i-t-a to be taken as just one of the normal things by not adding a special test for it. I wonder if there is a reason why these are not part of say t4013 (diff-various)? By adjusting and adding to existing test, we'd avoid a mistake of adding a test script that is not executable (didn't your "make DEVELOPER=1 test" catch the error?) ;-) Thanks. > diff --git a/t/t4069-diff-intent-to-add.sh b/t/t4069-diff-intent-to-add.sh > new file mode 100644 > index 0000000000..85c1a35ca7 > --- /dev/null > +++ b/t/t4069-diff-intent-to-add.sh > @@ -0,0 +1,30 @@ > +#!/bin/sh > + > +test_description='behavior of diff with intent-to-add entries' > + > +. ./test-lib.sh > + > +test_expect_success setup ' > + test_write_lines 1 2 3 4 5 >blueprint > +' > + > +test_expect_success 'diff between i-t-a and file should be new file' ' > + cat blueprint >test-file && > + git add -N test-file && > + git diff >output && > + grep "new file mode 100644" output > +' > + > +test_expect_success 'diff between i-t-a and no file should be deletion' ' > + rm -f test-file && > + git diff >output && > + grep "deleted file mode 100644" output > +' > + > +test_expect_success '--ita-visible-in-index diff should be from empty blob' ' > + cat blueprint >test-file && > + git diff --ita-visible-in-index >output && > + grep "index e69de29" output > +' > + > +test_done
On Mon, Aug 10, 2020 at 4:54 AM Raymond E. Pasco <ray@ameretat.dev> wrote: > Add a small test suite to test the behavior of diff with intent-to-add > paths. Specifically, the diff between an i-t-a entry and a file in the > worktree should be a "new file" diff, and the diff between an i-t-a > entry and no file in the worktree should be a "deleted file" diff. > However, if --ita-visible-in-index is passed, the former should instead > be a diff from the empty blob. > > Signed-off-by: Raymond E. Pasco <ray@ameretat.dev> > --- > diff --git a/t/t4069-diff-intent-to-add.sh b/t/t4069-diff-intent-to-add.sh > @@ -0,0 +1,30 @@ > +test_expect_success 'diff between i-t-a and file should be new file' ' > + cat blueprint >test-file && > + git add -N test-file && > + git diff >output && > + grep "new file mode 100644" output > +' If someone comes along and inserts new tests above this one and those new tests make their own changes to the index or worktree, how can this test be sure that the "new file mode" line is about 'test-file' rather than some other entry? It might be better to tighten this test, perhaps like this: git diff -- test-file && Same comment applies to the other tests. > +test_expect_success 'diff between i-t-a and no file should be deletion' ' > + rm -f test-file && > + git diff >output && > + grep "deleted file mode 100644" output > +' > + > +test_expect_success '--ita-visible-in-index diff should be from empty blob' ' > + cat blueprint >test-file && > + git diff --ita-visible-in-index >output && > + grep "index e69de29" output > +' The hard-coded SHA-1 value in the "index" line is going to cause the test to fail when the test suite is configured to run with SHA-256. You could fix it by preparing two hash values -- one for SHA-1 and one for SHA-256 -- and then looking up the value with test_oid() for use with grep. On the other hand, if you're not interested in the exact value, but care only that _some_ hash value is present, then you could just grep for a hex-string. But what is this test actually checking? In my experiments, this grep expression will also successfully match the output from the test preceding this one, which means that the conditions of this test are too loose. To tighten this test, perhaps it makes sense to take a different approach and check the exact output rather than merely grepping for a particular string. In other words, something like this might be better (typed in email, so untested): cat >expect <<-\EOF && diff --git a/test-file b/test-file index HEX..HEX HEX --- a/test-file +++ b/test-file EOF cat blueprint >test-file && git diff --ita-visible-in-index -- test-file >raw && sed "s/[0-9a-f][0-9a-f]*/HEX/g' raw >actual && test_cmp expect actual In fact, this likely would be a good model to use for all the tests.
On Mon, Aug 10, 2020 at 12:23 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > To tighten this test, perhaps it makes sense to take a different > approach and check the exact output rather than merely grepping for a > particular string. In other words, something like this might be better > (typed in email, so untested): > > cat >expect <<-\EOF && > diff --git a/test-file b/test-file > index HEX..HEX HEX > --- a/test-file > +++ b/test-file > EOF > cat blueprint >test-file && > git diff --ita-visible-in-index -- test-file >raw && > sed "s/[0-9a-f][0-9a-f]*/HEX/g' raw >actual && > test_cmp expect actual This can be improved by taking advantage of the OID_REGEX variable defined by the test suite for matching an OID. So something like this would be even better: cat >expect <<-\EOF && diff --git a/test-file b/test-file index OID..OID 100644 --- a/test-file +++ b/test-file EOF cat blueprint >test-file && git diff --ita-visible-in-index -- test-file >raw && sed "s/$OID_REGEX/OID/g" raw >actual && test_cmp expect actual
Eric Sunshine <sunshine@sunshineco.com> writes: > On Mon, Aug 10, 2020 at 12:23 PM Eric Sunshine <sunshine@sunshineco.com> wrote: >> To tighten this test, perhaps it makes sense to take a different >> approach and check the exact output rather than merely grepping for a >> particular string. In other words, something like this might be better >> (typed in email, so untested): >> >> cat >expect <<-\EOF && >> diff --git a/test-file b/test-file >> index HEX..HEX HEX >> --- a/test-file >> +++ b/test-file >> EOF >> cat blueprint >test-file && >> git diff --ita-visible-in-index -- test-file >raw && >> sed "s/[0-9a-f][0-9a-f]*/HEX/g' raw >actual && >> test_cmp expect actual > > This can be improved by taking advantage of the OID_REGEX variable > defined by the test suite for matching an OID. So something like this > would be even better: > > cat >expect <<-\EOF && > diff --git a/test-file b/test-file > index OID..OID 100644 > --- a/test-file > +++ b/test-file > EOF > cat blueprint >test-file && > git diff --ita-visible-in-index -- test-file >raw && > sed "s/$OID_REGEX/OID/g" raw >actual && > test_cmp expect actual OID_REGEX is [0-9a-f]{40} while what is used here is [0-9a-f]{1,}. Unless --full-index is in use, they mean different things, no?
On Mon, Aug 10, 2020 at 6:09 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > This can be improved by taking advantage of the OID_REGEX variable > > defined by the test suite for matching an OID. So something like this > > would be even better: > > > > cat >expect <<-\EOF && > > diff --git a/test-file b/test-file > > index OID..OID 100644 > > --- a/test-file > > +++ b/test-file > > EOF > > cat blueprint >test-file && > > git diff --ita-visible-in-index -- test-file >raw && > > sed "s/$OID_REGEX/OID/g" raw >actual && > > test_cmp expect actual > > OID_REGEX is [0-9a-f]{40} while what is used here is [0-9a-f]{1,}. > Unless --full-index is in use, they mean different things, no? You're right, of course. The regex in the original example I gave was too loose, matching even single hex letters in words like "index" and "test-file", so I wanted to tighten it up, but I botched it with OID_REGEX. Anyhow, I hope my examples convey the general idea (which needs a bit of tweaking from what I showed).
Eric Sunshine <sunshine@sunshineco.com> writes: > On Mon, Aug 10, 2020 at 6:09 PM Junio C Hamano <gitster@pobox.com> wrote: >> Eric Sunshine <sunshine@sunshineco.com> writes: >> > This can be improved by taking advantage of the OID_REGEX variable >> > defined by the test suite for matching an OID. So something like this >> > would be even better: >> > >> > cat >expect <<-\EOF && >> > diff --git a/test-file b/test-file >> > index OID..OID 100644 >> > --- a/test-file >> > +++ b/test-file >> > EOF >> > cat blueprint >test-file && >> > git diff --ita-visible-in-index -- test-file >raw && >> > sed "s/$OID_REGEX/OID/g" raw >actual && >> > test_cmp expect actual >> >> OID_REGEX is [0-9a-f]{40} while what is used here is [0-9a-f]{1,}. >> Unless --full-index is in use, they mean different things, no? > > You're right, of course. The regex in the original example I gave was > too loose, matching even single hex letters in words like "index" and > "test-file", so I wanted to tighten it up, but I botched it with > OID_REGEX. Anyhow, I hope my examples convey the general idea (which > needs a bit of tweaking from what I showed). Yeah, perhaps along the lines of HEX_RX="_x05*" sed -e "s/index $HEX_RX..$HEX_RX /index OID..OID /"
On Mon Aug 10, 2020 at 12:23 PM EDT, Eric Sunshine wrote: > The hard-coded SHA-1 value in the "index" line is going to cause the > test to fail when the test suite is configured to run with SHA-256. > You could fix it by preparing two hash values -- one for SHA-1 and one > for SHA-256 -- and then looking up the value with test_oid() for use > with grep. On the other hand, if you're not interested in the exact > value, but care only that _some_ hash value is present, then you could > just grep for a hex-string. Loosely, all this test cares about is that it's not "index 0000000"; or perhaps, taking another tack, that it doesn't have a "new file" line. More rigidly, it's nice to confirm that it's a diff from the empty blob and not from some other random blob.
On Mon, Aug 10, 2020 at 7:09 PM Raymond E. Pasco <ray@ameretat.dev> wrote: > More rigidly, it's nice to confirm that it's a diff from the empty blob > and not from some other random blob. The tests can get access to the correct OID of the empty blob without hardcoding it either via $(test_oid empty_blob) or just as $EMPTY_BLOB, however, that's the full length hex string, so you'd still need to do some tweaking.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Mon, Aug 10, 2020 at 7:09 PM Raymond E. Pasco <ray@ameretat.dev> wrote: >> More rigidly, it's nice to confirm that it's a diff from the empty blob >> and not from some other random blob. > > The tests can get access to the correct OID of the empty blob without > hardcoding it either via $(test_oid empty_blob) or just as > $EMPTY_BLOB, however, that's the full length hex string, so you'd > still need to do some tweaking. If the tests do truly care about these exact objects, then using "--full-index" would be a good way to compare with $EMPTY_BLOB and/or $ZERO_OID and such.
diff --git a/t/t4069-diff-intent-to-add.sh b/t/t4069-diff-intent-to-add.sh new file mode 100644 index 0000000000..85c1a35ca7 --- /dev/null +++ b/t/t4069-diff-intent-to-add.sh @@ -0,0 +1,30 @@ +#!/bin/sh + +test_description='behavior of diff with intent-to-add entries' + +. ./test-lib.sh + +test_expect_success setup ' + test_write_lines 1 2 3 4 5 >blueprint +' + +test_expect_success 'diff between i-t-a and file should be new file' ' + cat blueprint >test-file && + git add -N test-file && + git diff >output && + grep "new file mode 100644" output +' + +test_expect_success 'diff between i-t-a and no file should be deletion' ' + rm -f test-file && + git diff >output && + grep "deleted file mode 100644" output +' + +test_expect_success '--ita-visible-in-index diff should be from empty blob' ' + cat blueprint >test-file && + git diff --ita-visible-in-index >output && + grep "index e69de29" output +' + +test_done
Add a small test suite to test the behavior of diff with intent-to-add paths. Specifically, the diff between an i-t-a entry and a file in the worktree should be a "new file" diff, and the diff between an i-t-a entry and no file in the worktree should be a "deleted file" diff. However, if --ita-visible-in-index is passed, the former should instead be a diff from the empty blob. Signed-off-by: Raymond E. Pasco <ray@ameretat.dev> --- t/t4069-diff-intent-to-add.sh | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 t/t4069-diff-intent-to-add.sh