Re: [PATCH v5 9/9] status: unit tests for --porcelain=v2
On 08/10/2016 06:41 PM, Junio C Hamano wrote: Jeff Hostetler writes: Having said all that, it is OK to fix their titles after the current 9-patch series lands on 'next'; incremental refinements are easier on reviewers than having to review too many rerolls. I'll change the test titles to have all that info. OK. As I said, if the retitling is the only change, then doing so as a follow-up to the existing 9-patch series would be easier to review. If there are other changes needed, a reroll of the full set is probably better. I've changed the titles and added 5 or 6 more tests to cover your previous questions/comments, but no code changes. This is a small point, but doesn't the lowercase 'u' somehow look ugly, especially because the status letters that immediately follow it are all uppercase? Since we are inventing a new format and my column 1 is completely new I didn't think it mattered. Wrong assumption ;-) We want the resulting code to be consistent. And I used a lowercase 'u' to distinguish it from the "U" in the X and Y columns since they mean different things. I thought they both mean "Unmerged"; that is why I thought seeing a lowercase there was very strange. For unmerged items, the X & Y columns in the original porcelain format, can have various combinations of "A", "D", and "U" values. The implication being that "U" means modified/edited in some way. Yes, it is labeled "unmerged", but in a "DU" or "UD" conflict, it means that one side deleted it and one side modified it in some way. A "UU" conflict means both sides edited it and auto-merge failed. All I wanted the lowercase 'u' to indicate was that the file was in an unmerged state (without specifying why). That is, we would have (the 7 valid combinations of) "u [ADU][ADU]" and it would be clear(er?) that the little 'u' was different from the other 2. I know it is a subtle point and we can change it if you want, but that was the rationale. Thanks, Jeff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 9/9] status: unit tests for --porcelain=v2
Jeff Hostetler writes: > In my brief testing, the existing porcelain status reports it as "AM" > (for both a file with content and an empty file). > > The V2 code outputs the following: > 1 AM N... 00 100644 100644 OK, so they are consistent, which is good. >> Having said all that, it is OK to fix their titles after the current >> 9-patch series lands on 'next'; incremental refinements are easier >> on reviewers than having to review too many rerolls. > > I'll change the test titles to have all that info. OK. As I said, if the retitling is the only change, then doing so as a follow-up to the existing 9-patch series would be easier to review. If there are other changes needed, a reroll of the full set is probably better. >> This is a small point, but doesn't the lowercase 'u' somehow look >> ugly, especially because the status letters that immediately follow >> it are all uppercase? >> > > Since we are inventing a new format and my column 1 is completely new > I didn't think it mattered. Wrong assumption ;-) We want the resulting code to be consistent. > And I used a lowercase 'u' to distinguish > it from the "U" in the X and Y columns since they mean different > things. I thought they both mean "Unmerged"; that is why I thought seeing a lowercase there was very strange. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 9/9] status: unit tests for --porcelain=v2
On 08/08/2016 01:07 PM, Junio C Hamano wrote: Jeff Hostetler writes: +test_expect_success pre_initial_commit_0 ' + ... + git status --porcelain=v2 --branch --untracked-files=normal >actual && + test_cmp expect actual +' + + +test_expect_success pre_initial_commit_1 ' + git add file_x file_y file_z dir1 && + ... + cat >expect <<-EOF && + # branch.oid (initial) + # branch.head master + 1 A. N... 00 100644 100644 $_z40 $OID_A dir1/file_a + ... This makes one wonder what would/should be shown on the "A." column if one of these files are added with "-N" (intent-to-add). I do not see any test for such entries in this patch, but I think it should. I must admit that I don't use -N, so I'm open to recommendations here. In my brief testing, the existing porcelain status reports it as "AM" (for both a file with content and an empty file). The V2 code outputs the following: 1 AM N... 00 100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 intent.add Which I think makes sense. I'll add a test to exercise that. "Try -z on the above" can and should be in the test title to help ... Having said all that, it is OK to fix their titles after the current 9-patch series lands on 'next'; incremental refinements are easier on reviewers than having to review too many rerolls. I'll change the test titles to have all that info. This is probably a good place to see what happens to these untracked files and branch info if we do not ask the command to show them. I'll add some cases to cycle thru the options and confirm there's no output when not requested. +## +## Ignore a file +## + +test_expect_success ignore_file_0 ' + echo x.ign >.gitignore && + echo "ignore me" >x.ign && + H1=$(git rev-parse HEAD) && + + ## ignored file SHOULD NOT appear in output when --ignored is not used. + ... + git status --porcelain=v2 --branch --untracked-files=all >actual && + test_cmp expect actual && + ... + git status --porcelain=v2 --branch --ignored --untracked-files=all >actual && + rm x.ign && + rm .gitignore && Arrange these files to be cleaned before you create them by having test_when_finished "rm -f x.ign .gitignore" && at the very beginning of this test before they are created. Otherwise, if any step before these removal fail, later test that assume they are gone will be affected. You already do so correctly in the upstream_fields_0 test below. Missed a few. Got it. Thanks! + + cat >expect <<-EOF && + # branch.oid $HM + # branch.head AA_M + u AA N... 00 100644 100644 100644 $_z40 $OID_AA_B $OID_AA_A conflict.txt + EOF This is a small point, but doesn't the lowercase 'u' somehow look ugly, especially because the status letters that immediately follow it are all uppercase? Since we are inventing a new format and my column 1 is completely new I didn't think it mattered. And I used a lowercase 'u' to distinguish it from the "U" in the X and Y columns since they mean different things. But we can change it if you'd prefer. + git status --porcelain=v2 --branch --untracked-files=all >actual && + git reset --hard && This "reset" also may be a candidate for test_when_finished clean-up (I won't repeat the comment but there probably are many others). Got it. And I hopefully caught the rest. +test_expect_success 'upstream_fields_0' ' + git checkout master && + test_when_finished rm -rf sub_repo && The "when-finished" argument are usually quoted like this, I think: test_when_finished "rm -rf sub_repo" && Got it. Thanks. I have the test changes ready. As suggested, I'll send a single commit patch after my 9-patch series lands on 'next'. Thanks, Jeff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 9/9] status: unit tests for --porcelain=v2
Jeff Hostetler writes: > +test_expect_success pre_initial_commit_0 ' > + ... > + git status --porcelain=v2 --branch --untracked-files=normal >actual && > + test_cmp expect actual > +' > + > + > +test_expect_success pre_initial_commit_1 ' > + git add file_x file_y file_z dir1 && > + ... > + cat >expect <<-EOF && > + # branch.oid (initial) > + # branch.head master > + 1 A. N... 00 100644 100644 $_z40 $OID_A dir1/file_a > + ... This makes one wonder what would/should be shown on the "A." column if one of these files are added with "-N" (intent-to-add). I do not see any test for such entries in this patch, but I think it should. > + git status --porcelain=v2 --branch --untracked-files=all >actual && > + test_cmp expect actual > +' > +## Try -z on the above > +test_expect_success pre_initial_commit_2 ' > + lf_to_nul >expect <<-EOF && > + ... > + git status -z --porcelain=v2 --branch --untracked-files=all >actual && > + test_cmp expect actual > +' "Try -z on the above" can and should be in the test title to help those who are watching the test run. Instead of trying to clarify code with comments, clarify them by letting the code speak for themselves. I would have named the above three perhaps like this: test_expect_success 'before first commit, nothing added' test_expect_success 'before first commit, some files added' test_expect_success 'before first commit, some files added (-z)' "pre-initial-commit-$number" is not very interesting; it does not convey a more interesting aspect of these tests, like (a) _0 is distinct (nothing added) among the three, (b) _1 and _2 are about the same state, just expressed differently, and (c) _1 is LF terminated, _2 is NUL terminated. The same comment applies to the remainder of the test. For example: > +## > +## Create second commit. > +## > + > +test_expect_success second_commit_0 ' This "_0" does not tell us anything, but you are testing "When fully committed, we only see untracked files (if we ask) and branch info (if we ask). Having said all that, it is OK to fix their titles after the current 9-patch series lands on 'next'; incremental refinements are easier on reviewers than having to review too many rerolls. This is probably a good place to see what happens to these untracked files and branch info if we do not ask the command to show them. Otherwise, it tests exactly the same as "initial_commit_0" and is not all that interesting, no? > +## > +## Ignore a file > +## > + > +test_expect_success ignore_file_0 ' > + echo x.ign >.gitignore && > + echo "ignore me" >x.ign && > + H1=$(git rev-parse HEAD) && > + > + ## ignored file SHOULD NOT appear in output when --ignored is not used. > + ... > + git status --porcelain=v2 --branch --untracked-files=all >actual && > + test_cmp expect actual && > + ... > + git status --porcelain=v2 --branch --ignored --untracked-files=all > >actual && > + rm x.ign && > + rm .gitignore && Arrange these files to be cleaned before you create them by having test_when_finished "rm -f x.ign .gitignore" && at the very beginning of this test before they are created. Otherwise, if any step before these removal fail, later test that assume they are gone will be affected. You already do so correctly in the upstream_fields_0 test below. > +## > +## Create some conflicts. > +## > + > +test_expect_success conflict_AA ' > + git branch AA_A master && > + git checkout AA_A && > + echo "Branch AA_A" >conflict.txt && > + OID_AA_A=$(git hash-object -t blob -- conflict.txt) && > + git add conflict.txt && > + git commit -m "branch aa_a" && > + > + git branch AA_B master && > + git checkout AA_B && > + echo "Branch AA_B" >conflict.txt && > + OID_AA_B=$(git hash-object -t blob -- conflict.txt) && > + git add conflict.txt && > + git commit -m "branch aa_b" && > + > + git branch AA_M AA_B && > + git checkout AA_M && > + test_must_fail git merge AA_A && > + > + HM=$(git rev-parse HEAD) && > + > + cat >expect <<-EOF && > + # branch.oid $HM > + # branch.head AA_M > + u AA N... 00 100644 100644 100644 $_z40 $OID_AA_B $OID_AA_A > conflict.txt > + EOF This is a small point, but doesn't the lowercase 'u' somehow look ugly, especially because the status letters that immediately follow it are all uppercase? > + git status --porcelain=v2 --branch --untracked-files=all >actual && > + git reset --hard && This "reset" also may be a candidate for test_when
[PATCH v5 9/9] status: unit tests for --porcelain=v2
From: Jeff Hostetler Test porcelain v2 status format. Signed-off-by: Jeff Hostetler --- t/t7064-wtstatus-pv2.sh | 597 1 file changed, 597 insertions(+) create mode 100755 t/t7064-wtstatus-pv2.sh diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh new file mode 100755 index 000..edc65ce4 --- /dev/null +++ b/t/t7064-wtstatus-pv2.sh @@ -0,0 +1,597 @@ +#!/bin/sh + +test_description='git status --porcelain=v2 + +This test exercises porcelain V2 output for git status.' + + +. ./test-lib.sh + +test_expect_success setup ' + test_tick && + git config --local core.autocrlf false && + echo x >file_x && + echo y >file_y && + echo z >file_z && + mkdir dir1 && + echo a >dir1/file_a && + echo b >dir1/file_b +' + + +## +## Confirm output prior to initial commit. +## + +test_expect_success pre_initial_commit_0 ' + cat >expect <<-EOF && + # branch.oid (initial) + # branch.head master + ? actual + ? dir1/ + ? expect + ? file_x + ? file_y + ? file_z + EOF + + git status --porcelain=v2 --branch --untracked-files=normal >actual && + test_cmp expect actual +' + + +test_expect_success pre_initial_commit_1 ' + git add file_x file_y file_z dir1 && + OID_A=$(git hash-object -t blob -- dir1/file_a) && + OID_B=$(git hash-object -t blob -- dir1/file_b) && + OID_X=$(git hash-object -t blob -- file_x) && + OID_Y=$(git hash-object -t blob -- file_y) && + OID_Z=$(git hash-object -t blob -- file_z) && + + cat >expect <<-EOF && + # branch.oid (initial) + # branch.head master + 1 A. N... 00 100644 100644 $_z40 $OID_A dir1/file_a + 1 A. N... 00 100644 100644 $_z40 $OID_B dir1/file_b + 1 A. N... 00 100644 100644 $_z40 $OID_X file_x + 1 A. N... 00 100644 100644 $_z40 $OID_Y file_y + 1 A. N... 00 100644 100644 $_z40 $OID_Z file_z + ? actual + ? expect + EOF + + git status --porcelain=v2 --branch --untracked-files=all >actual && + test_cmp expect actual +' + +## Try -z on the above +test_expect_success pre_initial_commit_2 ' + lf_to_nul >expect <<-EOF && + # branch.oid (initial) + # branch.head master + 1 A. N... 00 100644 100644 $_z40 $OID_A dir1/file_a + 1 A. N... 00 100644 100644 $_z40 $OID_B dir1/file_b + 1 A. N... 00 100644 100644 $_z40 $OID_X file_x + 1 A. N... 00 100644 100644 $_z40 $OID_Y file_y + 1 A. N... 00 100644 100644 $_z40 $OID_Z file_z + ? actual + ? expect + EOF + + git status -z --porcelain=v2 --branch --untracked-files=all >actual && + test_cmp expect actual +' + +## +## Create first commit. Confirm commit oid in new track header. +## Then make some changes on top of it. +## + +test_expect_success initial_commit_0 ' + git commit -m initial && + H0=$(git rev-parse HEAD) && + cat >expect <<-EOF && + # branch.oid $H0 + # branch.head master + ? actual + ? expect + EOF + + git status --porcelain=v2 --branch --untracked-files=all >actual && + test_cmp expect actual +' + + +test_expect_success initial_commit_1 ' + echo x >>file_x && + OID_X1=$(git hash-object -t blob -- file_x) && + rm file_z && + H0=$(git rev-parse HEAD) && + + cat >expect <<-EOF && + # branch.oid $H0 + # branch.head master + 1 .M N... 100644 100644 100644 $OID_X $OID_X file_x + 1 .D N... 100644 100644 00 $OID_Z $OID_Z file_z + ? actual + ? expect + EOF + + git status --porcelain=v2 --branch --untracked-files=all >actual && + test_cmp expect actual +' + + +test_expect_success initial_commit_2 ' + git add file_x && + git rm file_z && + H0=$(git rev-parse HEAD) && + + cat >expect <<-EOF && + # branch.oid $H0 + # branch.head master + 1 M. N... 100644 100644 100644 $OID_X $OID_X1 file_x + 1 D. N... 100644 00 00 $OID_Z $_z40 file_z + ? actual + ? expect + EOF + + git status --porcelain=v2 --branch --untracked-files=all >actual && + test_cmp expect actual +' + + +test_expect_success initial_commit_3 ' + git mv file_y renamed_y && + H0=$(git rev-parse HEAD) && + + q_to_tab >expect <<-EOF && + # branch.oid $H0 + # branch.head master + 1 M. N... 100644 100644 100644 $OID_X $OID_X1 file_x + 1 D. N... 100644 00 00 $OID_Z $_z40 file_z + 2 R. N... 100644 100644 100644 $OID_Y $OID_Y R100 renamed_yQfile_y + ? actual + ?
