Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values

[Junio C Hamano]

Phillip Wood  writes:

> Keeping the permission bits makes sense (I'd not thought of them when
> I created the patch) as we want to check that the file has the correct
> permissions. As for the all-zero object name, is it really worth
> leaving it in - if a file has been created or deleted then we'll still
> have /dev/null as the file name for one side or the other and the diff
> lines will show it as well. As these tests are just to check the state
> of the index then I'm not sure the hash values add anything. How do
> you feel about a filter like
>
> sed "/^   index/s/ [0-9a-f][0-9a-f]*\.\.[0-9a-f][0-9a-f]*/x/"

Something like that, with perhaps the single 'x' replaced with
something more normal looking and the all-zero thing special cased,
was what I had in mind.

Special casing all-zero matters.  I know that the current code uses
all-zero on the missing side.  The thing is, tests are about
protecting the correctness we currently have, and we want to catch
the next idiot that breaks the code to stop showing all-zero when
talking about the missing side.

Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values

[Phillip Wood]

On 20/02/18 17:39, Junio C Hamano wrote:

Phillip Wood  writes:


From: Phillip Wood 

Purge the index lines from diffs so we're not hard coding sha1 hash
values in the expected output.


The motivation of this patch is clear, but all-zero object name for
missing side of deletion or creation patch should not change when we
transition to any hash function.  Neither the permission bits shown
in the output (and whether the index line has the bits are shown on
it in the first place, i.e. the index line of a creation patch does
not, whilethe one in a modification patch does).

So I am a bit ambivalent about this change.

Perhaps have a filter that redacts, instead of removes, selected
pieces of information that are likely to change while hash
transition, and use that consistently to filter both the expected
output and the actual output before comparing?

Keeping the permission bits makes sense (I'd not thought of them when I 
created the patch) as we want to check that the file has the correct 
permissions. As for the all-zero object name, is it really worth leaving 
it in - if a file has been created or deleted then we'll still have 
/dev/null as the file name for one side or the other and the diff lines 
will show it as well. As these tests are just to check the state of the 
index then I'm not sure the hash values add anything. How do you feel 
about a filter like


sed "/^index/s/ [0-9a-f][0-9a-f]*\.\.[0-9a-f][0-9a-f]*/x/"

Best Wishes

Phillip

Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> Purge the index lines from diffs so we're not hard coding sha1 hash
> values in the expected output.

The motivation of this patch is clear, but all-zero object name for
missing side of deletion or creation patch should not change when we
transition to any hash function.  Neither the permission bits shown
in the output (and whether the index line has the bits are shown on
it in the first place, i.e. the index line of a creation patch does
not, whilethe one in a modification patch does).

So I am a bit ambivalent about this change.

Perhaps have a filter that redacts, instead of removes, selected
pieces of information that are likely to change while hash
transition, and use that consistently to filter both the expected
output and the actual output before comparing?

Re: [PATCH v2 4/9] t3701: don't hard code sha1 hash values

[Eric Sunshine]

On Mon, Feb 19, 2018 at 6:29 AM, Phillip Wood  wrote:
> Purge the index lines from diffs so we're not hard coding sha1 hash
> values in the expected output.

Perhaps the commit message could provide a bit more explanation about
why this is a good idea. For instance, briefly mention that doing so
will ease Git's transition from SHA1 to some newer hash function.

> Signed-off-by: Phillip Wood 

[PATCH v2 4/9] t3701: don't hard code sha1 hash values

[Phillip Wood]

From: Phillip Wood 

Purge the index lines from diffs so we're not hard coding sha1 hash
values in the expected output.

Signed-off-by: Phillip Wood 
---
 t/t3701-add-interactive.sh | 32 
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 
b73a9598ab3eaed074735e99f3dcbc8f88d86f4c..70748393f28c93f4bc5d43b07bd96bd208aba3e9
 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -24,7 +24,6 @@ test_expect_success 'status works (initial)' '
 test_expect_success 'setup expected' '
cat >expected <<-EOF
new file mode 100644
-   index 000..d95f3ad
--- /dev/null
+++ b/file
@@ -0,0 +1 @@
@@ -34,7 +33,7 @@ test_expect_success 'setup expected' '
 
 test_expect_success 'diff works (initial)' '
(echo d; echo 1) | git add -i >output &&
-   sed -ne "/new file/,/content/p" diff &&
+   sed -ne /^index/d -e "/new file/,/content/p" diff &&
test_cmp expected diff
 '
 test_expect_success 'revert works (initial)' '
@@ -60,7 +59,6 @@ test_expect_success 'status works (commit)' '
 
 test_expect_success 'setup expected' '
cat >expected <<-EOF
-   index 180b47c..b6f2c08 100644
--- a/file
+++ b/file
@@ -1 +1,2 @@
@@ -71,7 +69,7 @@ test_expect_success 'setup expected' '
 
 test_expect_success 'diff works (commit)' '
(echo d; echo 1) | git add -i >output &&
-   sed -ne "/^index/,/content/p" diff &&
+   sed -ne "/^---/,/content/p" diff &&
test_cmp expected diff
 '
 test_expect_success 'revert works (commit)' '
@@ -90,7 +88,7 @@ test_expect_success 'setup expected' '
 test_expect_success 'dummy edit works' '
test_set_editor : &&
(echo e; echo a) | git add -p &&
-   git diff > diff &&
+   git diff | sed /^index/d >diff &&
test_cmp expected diff
 '
 
@@ -145,7 +143,6 @@ test_expect_success 'setup patch' '
 test_expect_success 'setup expected' '
cat >expected <<-EOF
diff --git a/file b/file
-   index b5dd6c9..f910ae9 100644
--- a/file
+++ b/file
@@ -1,4 +1,4 @@
@@ -159,7 +156,7 @@ test_expect_success 'setup expected' '
 
 test_expect_success 'real edit works' '
(echo e; echo n; echo d) | git add -p &&
-   git diff >output &&
+   git diff | sed /^index/d >output &&
test_cmp expected output
 '
 
@@ -168,10 +165,10 @@ test_expect_success 'skip files similarly as commit -a' '
echo file >.gitignore &&
echo changed >file &&
echo y | git add -p file &&
-   git diff >output &&
+   git diff | sed /^index/d >output &&
git reset &&
git commit -am commit &&
-   git diff >expected &&
+   git diff | sed /^index/d >expected &&
test_cmp expected output &&
git reset --hard HEAD^
 '
@@ -217,7 +214,6 @@ test_expect_success 'setup again' '
 # Write the patch file with a new line at the top and bottom
 test_expect_success 'setup patch' '
cat >patch <<-EOF
-   index 180b47c..b6f2c08 100644
--- a/file
+++ b/file
@@ -1,2 +1,4 @@
@@ -232,7 +228,6 @@ test_expect_success 'setup patch' '
 test_expect_success 'setup expected' '
cat >expected <<-EOF
diff --git a/file b/file
-   index b6f2c08..61b9053 100755
--- a/file
+++ b/file
@@ -1,2 +1,4 @@
@@ -248,15 +243,14 @@ test_expect_success 'add first line works' '
git commit -am "clear local changes" &&
git apply patch &&
(echo s; echo y; echo y) | git add -p file &&
-   git diff --cached > diff &&
+   git diff --cached | sed /^index/d >diff &&
test_cmp expected diff
 '
 
 test_expect_success 'setup expected' '
cat >expected <<-EOF
diff --git a/non-empty b/non-empty
deleted file mode 100644
-   index d95f3ad..000
--- a/non-empty
+++ /dev/null
@@ -1 +0,0 @@
@@ -271,15 +265,14 @@ test_expect_success 'deleting a non-empty file' '
git commit -m non-empty &&
rm non-empty &&
echo y | git add -p non-empty &&
-   git diff --cached >diff &&
+   git diff --cached | sed /^index/d >diff &&
test_cmp expected diff
 '
 
 test_expect_success 'setup expected' '
cat >expected <<-EOF
diff --git a/empty b/empty
deleted file mode 100644
-   index e69de29..000
EOF
 '
 
@@ -290,7 +283,7 @@ test_expect_success 'deleting an empty file' '
git commit -m empty &&
rm empty &&
echo y | git add -p empty &&
-   git diff --cached >diff &&
+   git diff --cached | sed /^index/d >diff &&
test_cmp expected diff
 '
 
@@ -317,7 +310,7 @@ test_expect_success 'split hunk "add -p (edit)"' '
#exits.
printf "%s\n" s e q n q q |
EDITOR=: git add -p &&
-   git