On Thu, May 28, 2015 at 9:10 AM, Paul Tan <pyokagan@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html