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

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, May 28, 2015 at 9:10 AM, Paul Tan <pyokagan@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.lespinet@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'.