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

[Remi LESPINET <remi.lespinet@ensimag.grenoble-inp.fr>]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Paul Tan <pyokagan@gmail.com> writes:
>
> >> Remi LESPINET <remi.lespinet@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 !