[PATCH 1/3] t4150-am: refactor and clean common setup

Eric Sunshine [off-list ref] writes:

Paul Tan [off-list ref] writes:

quoted

quoted

Remi LESPINET [off-list ref] 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.

quoted

        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 !