[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
