[PATCH 1/3] t4150-am: refactor and clean common setup
Eric Sunshine sunsh...@sunshineco.com writes: Paul Tan pyoka...@gmail.com writes: Remi LESPINET remi.lespi...@ensimag.grenoble-inp.fr writes: + tmp_name=${2-temporary} I don't think the quotes are required. Also, I don't feel good about swapping the order of the arguments to git-checkout. (or making $2 an optional argument). As the patch stands, if I see setup_temporary_branch lorem I will think: so we are creating a temporary branch lorem. But nope, we are creating a temporary branch temporary that branches from lorem. I don't think writing setup_temporary_branch temporary lorem explicitly is that bad. In fact, the second argument is never used in any of the three patches, so it seems to be wasted functionality (at this time). If so, then an even more appropriate name might be new_temp_branch_from(). Clearly, then: new_temp_branch_from lorem Considering your two comments about the name of the function I think removing the possibility of using the second argument and renaming the function new_temp_branch_from gets everyone to agree since it has not the possible confusion with git-checkout problem. This is confusing. The commit message seems to advertise this patch as primarily a refactoring change (pulling boilerplate out of tests and into a new function), but the operation of that new function is surprisingly different from the boilerplate it's replacing: The old code did not create a branch, the new function does. If your intention really is to create new branches in tests which previously did not need throwaway branches, then the commit message should state that as its primary purpose and explain why doing so is desirable, since it is not clear from this patch what you gain from doing so. Moreover, typically, you should only either refactor or change behavior in a single patch, not both. For instance, patch 1 would add the new function and update tests to call it in place of the boilerplate; and patch 2 would change behavior (such as creating a temporary branch). On the other hand, if these tests really don't benefit from having a throw-away branch, then this change seems suspect and obscures the intent of the test rather than clarifying or simplifying. The purpose of this patch was originally to eliminate some test dependancies introduced by the checkouts relative to HEAD (which caused cascade failure) and avoid creating branches, whose name can't be reused, but you're right, that may not benefit as much as I expected... Perhaps I should have make tags from branches used as start points, which would have done the same effect as these temporary branches. sed -n -e 3,\$p msg file head -n 9 msg file git add file @@ -303,10 +297,8 @@ test_expect_success 'am -3 -p0 can read --no-prefix patch' ' ' test_expect_success 'am can rename a file' ' + setup_temporary_branch lorem grep ^rename from rename.patch - rm -fr .git/rebase-apply - git reset --hard - git checkout lorem^0 In other cases, you insert the setup_temporary_branch() invocation where the old code resided. Why the difference in this test (and others following)? This doesn't affect the result of the test (assuming setup_temporary_branch doesn't fail). I preferred to add the setup before anything else in the test. It affects efficiency in case the rename patch has not the correct syntax. So it may be better to put the grep as the first instruction to avoid evaluating all the test in case the syntax of the patch is not correct. Thanks a lot for your comments, I'll correct the patch asap ! -- 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] t4150-am: refactor and clean common setup
On Tue, May 26, 2015 at 5:32 PM, Remi Lespinet remi.lespi...@ensimag.grenoble-inp.fr wrote: Add new functions to keep the setup cleaner: - setup_temporary_branch: creates a new branch, check it out and automatically delete it after the test is over - setup_fixed_branch: creates a fixed branch, which can be re-used in later tests See below for review comments beyond those already provided by Paul... Signed-off-by: Remi Lespinet remi.lespi...@ensimag.grenoble-inp.fr --- diff --git a/t/t4150-am.sh b/t/t4150-am.sh index 306e6f3..8370951 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -4,6 +4,20 @@ test_description='git am running' . ./test-lib.sh +setup_temporary_branch () { + tmp_name=${2-temporary} + git reset --hard + rm -fr .git/rebase-apply + test_when_finished git checkout $1 git branch -D $tmp_name + git checkout -b $tmp_name $1 +} + +setup_fixed_branch () { + git reset --hard + rm -fr .git/rebase-apply + git checkout -b $1 $2 +} + test_expect_success 'setup: messages' ' cat msg -\EOF second @@ -143,9 +157,7 @@ test_expect_success setup ' ' test_expect_success 'am applies patch correctly' ' - rm -fr .git/rebase-apply - git reset --hard - git checkout first + setup_temporary_branch first This is confusing. The commit message seems to advertise this patch as primarily a refactoring change (pulling boilerplate out of tests and into a new function), but the operation of that new function is surprisingly different from the boilerplate it's replacing: The old code did not create a branch, the new function does. If your intention really is to create new branches in tests which previously did not need throwaway branches, then the commit message should state that as its primary purpose and explain why doing so is desirable, since it is not clear from this patch what you gain from doing so. Moreover, typically, you should only either refactor or change behavior in a single patch, not both. For instance, patch 1 would add the new function and update tests to call it in place of the boilerplate; and patch 2 would change behavior (such as creating a temporary branch). On the other hand, if these tests really don't benefit from having a throw-away branch, then this change seems suspect and obscures the intent of the test rather than clarifying or simplifying. test_tick git am patch1 test_path_is_missing .git/rebase-apply @@ -275,9 +273,7 @@ test_expect_success 'am --keep-non-patch really keeps the non-patch part' ' ' test_expect_success 'am -3 falls back to 3-way merge' ' - rm -fr .git/rebase-apply - git reset --hard - git checkout -b lorem2 master2 + setup_fixed_branch lorem2 master2 sed -n -e 3,\$p msg file head -n 9 msg file git add file @@ -289,9 +285,7 @@ test_expect_success 'am -3 falls back to 3-way merge' ' ' test_expect_success 'am -3 -p0 can read --no-prefix patch' ' - rm -fr .git/rebase-apply - git reset --hard - git checkout -b lorem3 master2 + setup_temporary_branch lorem2 Unlike the test prior to this one which creates a fixed branch (via setup_fixed_branch) named 'lorem2' which is used by other tests, the 'lorem3' branch in this test is never used elsewhere, so setup_temporary_branch is used instead. Makes sense. sed -n -e 3,\$p msg file head -n 9 msg file git add file @@ -303,10 +297,8 @@ test_expect_success 'am -3 -p0 can read --no-prefix patch' ' ' test_expect_success 'am can rename a file' ' + setup_temporary_branch lorem grep ^rename from rename.patch - rm -fr .git/rebase-apply - git reset --hard - git checkout lorem^0 In other cases, you insert the setup_temporary_branch() invocation where the old code resided. Why the difference in this test (and others following)? git am rename.patch test_path_is_missing .git/rebase-apply git update-index --refresh @@ -349,11 +335,9 @@ test_expect_success 'am -3 -q is quiet' ' ' test_expect_success 'am pauses on conflict' ' - rm -fr .git/rebase-apply - git reset --hard - git checkout lorem2^^ + setup_temporary_branch lorem2^^ test_must_fail git am lorem-move.patch - test -d .git/rebase-apply + test_path_is_dir .git/rebase-apply Sneaking in unrelated change. This sort of cleanup should go in a patch of its own. ' test_expect_success 'am --skip works' ' -- 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] t4150-am: refactor and clean common setup
On Thu, May 28, 2015 at 3:09 PM, Eric Sunshine sunsh...@sunshineco.com wrote: On Tue, May 26, 2015 at 5:32 PM, Remi Lespinet remi.lespi...@ensimag.grenoble-inp.fr wrote: +setup_temporary_branch () { + tmp_name=${2-temporary} I forgot to mention the broken -chain here. Although the missing doesn't actively hurt the function today, someone may someday insert code above the 'tmp_name=' line without noticing the lack of , and the test won't notice a failure in that newly added code. Thus, it's better to keep the -chain intact throughout. + git reset --hard + rm -fr .git/rebase-apply + test_when_finished git checkout $1 git branch -D $tmp_name + git checkout -b $tmp_name $1 +} -- 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] t4150-am: refactor and clean common setup
On Thu, May 28, 2015 at 9:10 AM, Paul Tan pyoka...@gmail.com wrote: Take these comments/suggestions with a pinch of salt because I'm not that experienced with the code base as well ;-). I agree with pretty much all of your review comments. See below for a minor addenda. On Wed, May 27, 2015 at 5:32 AM, Remi Lespinet remi.lespi...@ensimag.grenoble-inp.fr wrote: +setup_temporary_branch () { Maybe name this checkout_temp_branch() or something, since it more or less is just a wrapper around git-checkout. checkout_temp_branch() doesn't give any indication that a new branch is being created, so it may not be an improvement over setup_temporary_branch(). A more apt name might be something like new_temp_branch(). + tmp_name=${2-temporary} I don't think the quotes are required. Also, I don't feel good about swapping the order of the arguments to git-checkout. (or making $2 an optional argument). As the patch stands, if I see setup_temporary_branch lorem I will think: so we are creating a temporary branch lorem. But nope, we are creating a temporary branch temporary that branches from lorem. I don't think writing setup_temporary_branch temporary lorem explicitly is that bad. In fact, the second argument is never used in any of the three patches, so it seems to be wasted functionality (at this time). If so, then an even more appropriate name might be new_temp_branch_from(). Clearly, then: new_temp_branch_from lorem creates a throw-away branch based upon 'lorem'. -- 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] t4150-am: refactor and clean common setup
Hi, Take these comments/suggestions with a pinch of salt because I'm not that experienced with the code base as well ;-). On Wed, May 27, 2015 at 5:32 AM, Remi Lespinet remi.lespi...@ensimag.grenoble-inp.fr wrote: Add new functions to keep the setup cleaner: - setup_temporary_branch: creates a new branch, check it out and automatically delete it after the test is over Agreed. This removes a lot of boilerplate from the tests. Another positive effect is that we can be sure that any commits made on that throwaway branch will not affect the other parts of the test suite, which makes understanding and editing the test suite much easier. - setup_fixed_branch: creates a fixed branch, which can be re-used in later tests Looking at the patch, I see that setup_fixed_branch() is only used in 2 places, so I don't think it is a common pattern that would require its own function. Also, see below. Signed-off-by: Remi Lespinet remi.lespi...@ensimag.grenoble-inp.fr --- t/t4150-am.sh | 138 -- 1 file changed, 47 insertions(+), 91 deletions(-) diff --git a/t/t4150-am.sh b/t/t4150-am.sh index 306e6f3..8370951 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -4,6 +4,20 @@ test_description='git am running' . ./test-lib.sh +setup_temporary_branch () { Maybe name this checkout_temp_branch() or something, since it more or less is just a wrapper around git-checkout. + tmp_name=${2-temporary} I don't think the quotes are required. Also, I don't feel good about swapping the order of the arguments to git-checkout. (or making $2 an optional argument). As the patch stands, if I see setup_temporary_branch lorem I will think: so we are creating a temporary branch lorem. But nope, we are creating a temporary branch temporary that branches from lorem. I don't think writing setup_temporary_branch temporary lorem explicitly is that bad. This is just personal preference though. + git reset --hard + rm -fr .git/rebase-apply + test_when_finished git checkout $1 git branch -D $tmp_name I think you should use git checkout -f $1 as if the working tree is dirty the test will fail at cleanup with no error message at all, which is annoying for debugging. Furthermore, the test_when_finished should be shifted below the git checkout -b below, as git branch -D will fail if the branch does not exist. + git checkout -b $tmp_name $1 If you use git checkout -f here then there's no need for the git reset --hard above. +} + +setup_fixed_branch () { + git reset --hard + rm -fr .git/rebase-apply + git checkout -b $1 $2 Again, by using git checkout -f we can drop the git reset --hard. But that reduces the function to 2 lines, and thus I don't think that this usage pattern needs its own function. +} + test_expect_success 'setup: messages' ' cat msg -\EOF second @@ -143,9 +157,7 @@ test_expect_success setup ' ' I haven't looked at the rest of the patch in detail because I'm not that well-acquainted with t4150 yet, but it looks okay. Thanks, Paul -- 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] t4150-am: refactor and clean common setup
Add new functions to keep the setup cleaner: - setup_temporary_branch: creates a new branch, check it out and automatically delete it after the test is over - setup_fixed_branch: creates a fixed branch, which can be re-used in later tests Signed-off-by: Remi Lespinet remi.lespi...@ensimag.grenoble-inp.fr --- t/t4150-am.sh | 138 -- 1 file changed, 47 insertions(+), 91 deletions(-) diff --git a/t/t4150-am.sh b/t/t4150-am.sh index 306e6f3..8370951 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -4,6 +4,20 @@ test_description='git am running' . ./test-lib.sh +setup_temporary_branch () { + tmp_name=${2-temporary} + git reset --hard + rm -fr .git/rebase-apply + test_when_finished git checkout $1 git branch -D $tmp_name + git checkout -b $tmp_name $1 +} + +setup_fixed_branch () { + git reset --hard + rm -fr .git/rebase-apply + git checkout -b $1 $2 +} + test_expect_success 'setup: messages' ' cat msg -\EOF second @@ -143,9 +157,7 @@ test_expect_success setup ' ' test_expect_success 'am applies patch correctly' ' - rm -fr .git/rebase-apply - git reset --hard - git checkout first + setup_temporary_branch first test_tick git am patch1 test_path_is_missing .git/rebase-apply @@ -155,9 +167,7 @@ test_expect_success 'am applies patch correctly' ' ' test_expect_success 'am applies patch e-mail not in a mbox' ' - rm -fr .git/rebase-apply - git reset --hard - git checkout first + setup_temporary_branch first git am patch1.eml test_path_is_missing .git/rebase-apply git diff --exit-code second @@ -166,9 +176,7 @@ test_expect_success 'am applies patch e-mail not in a mbox' ' ' test_expect_success 'am applies patch e-mail not in a mbox with CRLF' ' - rm -fr .git/rebase-apply - git reset --hard - git checkout first + setup_temporary_branch first git am patch1-crlf.eml test_path_is_missing .git/rebase-apply git diff --exit-code second @@ -177,9 +185,7 @@ test_expect_success 'am applies patch e-mail not in a mbox with CRLF' ' ' test_expect_success 'am applies patch e-mail with preceding whitespace' ' - rm -fr .git/rebase-apply - git reset --hard - git checkout first + setup_temporary_branch first git am patch1-ws.eml test_path_is_missing .git/rebase-apply git diff --exit-code second @@ -203,9 +209,7 @@ compare () { test_expect_success 'am changes committer and keeps author' ' test_tick - rm -fr .git/rebase-apply - git reset --hard - git checkout first + setup_temporary_branch first git am patch2 test_path_is_missing .git/rebase-apply test $(git rev-parse master^^) = $(git rev-parse HEAD^^) @@ -218,9 +222,7 @@ test_expect_success 'am changes committer and keeps author' ' ' test_expect_success 'am --signoff adds Signed-off-by: line' ' - rm -fr .git/rebase-apply - git reset --hard - git checkout -b master2 first + setup_fixed_branch master2 first git am --signoff patch2 printf %s\n $signoff expected echo Signed-off-by: $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL expected @@ -255,9 +257,7 @@ test_expect_success 'am without --keep removes Re: and [PATCH] stuff' ' ' test_expect_success 'am --keep really keeps the subject' ' - rm -fr .git/rebase-apply - git reset --hard - git checkout HEAD^ + setup_temporary_branch master2^ git am --keep patch4 test_path_is_missing .git/rebase-apply git cat-file commit HEAD actual @@ -265,9 +265,7 @@ test_expect_success 'am --keep really keeps the subject' ' ' test_expect_success 'am --keep-non-patch really keeps the non-patch part' ' - rm -fr .git/rebase-apply - git reset --hard - git checkout HEAD^ + setup_temporary_branch master2^ git am --keep-non-patch patch4 test_path_is_missing .git/rebase-apply git cat-file commit HEAD actual @@ -275,9 +273,7 @@ test_expect_success 'am --keep-non-patch really keeps the non-patch part' ' ' test_expect_success 'am -3 falls back to 3-way merge' ' - rm -fr .git/rebase-apply - git reset --hard - git checkout -b lorem2 master2 + setup_fixed_branch lorem2 master2 sed -n -e 3,\$p msg file head -n 9 msg file git add file @@ -289,9 +285,7 @@ test_expect_success 'am -3 falls back to 3-way merge' ' ' test_expect_success 'am -3 -p0 can read --no-prefix patch' ' - rm -fr .git/rebase-apply - git reset --hard - git checkout -b lorem3 master2 + setup_temporary_branch lorem2 sed -n -e 3,\$p msg file head -n 9 msg file git add file @@ -303,10