Re: [PATCH 1/3] t3404: preserve test_tick state across short SHA-1 collision test
On Sun, Aug 25, 2013 at 1:55 AM, Jonathan Nieder jrnie...@gmail.com wrote: Hi, Eric Sunshine wrote: The short SHA-1 collision test requires carefully crafted commits in order to ensure a collision at rebase time. Yeah, this breaks the usual rule that tests should be independent of hashing function. But it's the best we can do, I think. [...] --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -994,17 +994,23 @@ test_expect_success 'short SHA-1 setup' ' test_when_finished git checkout master git checkout --orphan collide git rm -rf . + ( unset test_tick test_commit collide1 collide test_commit --notick collide2 collide test_commit --notick collide3 115158b5 collide collide3 collide3 + ) Would be clearer if the code in a subshell were indented: ( unset test_tick test_commit ... ) I considered it, but decided against it for a couple reasons: * In this script, there already is a mix between the two styles: indented vs. unindented. * In this particular patch, the test_commit line creating commit3 wrapped beyond 80 columns when indented. In v2 of the series (for which you also made the same observation), the collide3 test_commit line is shorter, so I could have indented, however, I left it alone since nobody complained about it (and because there already is the mix of styles). Should this be worth a re-roll? [...] test_expect_success 'short SHA-1 collide' ' test_when_finished reset_rebase git checkout master git checkout collide + ( + unset test_tick + test_tick FAKE_COMMIT_MESSAGE=collide2 815200e \ FAKE_LINES=reword 1 2 git rebase -i HEAD~2 + ) Likewise. Hope that helps, Jonathan -- 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 1/3] t3404: preserve test_tick state across short SHA-1 collision test
Eric Sunshine wrote: On Sun, Aug 25, 2013 at 1:55 AM, Jonathan Nieder jrnie...@gmail.com wrote: Would be clearer if the code in a subshell were indented: ( unset test_tick test_commit ... ) I considered it, but decided against it for a couple reasons: * In this script, there already is a mix between the two styles: indented vs. unindented. * In this particular patch, the test_commit line creating commit3 wrapped beyond 80 columns when indented. I'm just one person, but I find the indented form way more readable. Long lines or lines with \ continuation are not a big deal. [...] Should this be worth a re-roll? Since the file already has a mixture of styles, if there's no other reason to reroll, I'd suggest leaving it alone. The next time it bugs me or someone else, that person can write a patch that cleans up the whole file on top. :) Thanks, Jonathan -- 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 1/3] t3404: preserve test_tick state across short SHA-1 collision test
Jonathan Nieder jrnie...@gmail.com writes: Eric Sunshine wrote: On Sun, Aug 25, 2013 at 1:55 AM, Jonathan Nieder jrnie...@gmail.com wrote: Would be clearer if the code in a subshell were indented: ( unset test_tick test_commit ... ) I considered it, but decided against it for a couple reasons: * In this script, there already is a mix between the two styles: indented vs. unindented. * In this particular patch, the test_commit line creating commit3 wrapped beyond 80 columns when indented. I'm just one person, but I find the indented form way more readable. Long lines or lines with \ continuation are not a big deal. [...] Should this be worth a re-roll? Since the file already has a mixture of styles, if there's no other reason to reroll, I'd suggest leaving it alone. The next time it bugs me or someone else, that person can write a patch that cleans up the whole file on top. :) Yeah, I think we saw style modernization patches to some test scripts during the last cycle. This one and the rest may want to go through a similar modernization, but it can be done after a real change like the proposed one matures. 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 1/3] t3404: preserve test_tick state across short SHA-1 collision test
Hi, Eric Sunshine wrote: The short SHA-1 collision test requires carefully crafted commits in order to ensure a collision at rebase time. Yeah, this breaks the usual rule that tests should be independent of hashing function. But it's the best we can do, I think. [...] --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -994,17 +994,23 @@ test_expect_success 'short SHA-1 setup' ' test_when_finished git checkout master git checkout --orphan collide git rm -rf . + ( unset test_tick test_commit collide1 collide test_commit --notick collide2 collide test_commit --notick collide3 115158b5 collide collide3 collide3 + ) Would be clearer if the code in a subshell were indented: ( unset test_tick test_commit ... ) [...] test_expect_success 'short SHA-1 collide' ' test_when_finished reset_rebase git checkout master git checkout collide + ( + unset test_tick + test_tick FAKE_COMMIT_MESSAGE=collide2 815200e \ FAKE_LINES=reword 1 2 git rebase -i HEAD~2 + ) Likewise. Hope that helps, Jonathan -- 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
[PATCH 1/3] t3404: preserve test_tick state across short SHA-1 collision test
The short SHA-1 collision test requires carefully crafted commits in order to ensure a collision at rebase time. This involves managing state which impacts the resulting SHA-1, including commit time. To accomplish this, test_tick is set to a known state for the short SHA-1 collision test. Unfortunately, doing so throws away the existing state of test_tick, which may be problematic for subsequently added tests. Fix this by preserving the existing state of test_tick across the short SHA-1 collision test. Signed-off-by: Eric Sunshine sunsh...@sunshineco.com --- t/t3404-rebase-interactive.sh | 6 ++ 1 file changed, 6 insertions(+) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index b65b774..6be97ba 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -994,17 +994,23 @@ test_expect_success 'short SHA-1 setup' ' test_when_finished git checkout master git checkout --orphan collide git rm -rf . + ( unset test_tick test_commit collide1 collide test_commit --notick collide2 collide test_commit --notick collide3 115158b5 collide collide3 collide3 + ) ' test_expect_success 'short SHA-1 collide' ' test_when_finished reset_rebase git checkout master git checkout collide + ( + unset test_tick + test_tick FAKE_COMMIT_MESSAGE=collide2 815200e \ FAKE_LINES=reword 1 2 git rebase -i HEAD~2 + ) ' test_done -- 1.8.4.rc4.499.gfb33910 -- 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
