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

> 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

> 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.

> >         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)?