Re: [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments

[Kaartic Sivaraam <kaartic.sivaraam@gmail.com>]

On Mon, 2017-11-06 at 11:06 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> > From: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
> > 
> > The ad-hoc patches to add new arguments to a function when needed
> > resulted in the related arguments not being close to each other.
> > This misleads the person reading the code to believe that there isn't
> > much relation between those arguments while it's not the case.
> 
> I do not get what this wants to say.  "I am sending this ad-hoc
> patch that scrambles the order of parameters for no real reason" is
> certainly not how you are selling this step.
> 
> > So, re-order the arguments to keep the related arguments close to each
> > other.
> 
> This sentence (without "So,", obviously, because I do not get the
> previous paragraph) I do understand and agree with as a goal.
> 

I've tried to improve it, does the following paragraph sound clear
enough?

    branch: group related arguments of create_branch()
        
    New arguments were added to create_branch() whenever the need
    arised and they were added to tail of the argument list. This
    resulted in the related arguments not being close to each other.
    This misleads the person reading the code to believe that
    there isn't much relation between those arguments while it is
    not the case.
        
    So, re-order the arguments to keep the related arguments close
    to each other.


> I think the only two things that should be kept together are "force"
> and "clobber_head_ok" because the previous 1/4 changed the meaning
> of "clobber_head" to "it is OK if I am renaming the currently
> checked-out branch", i.e. closer to what "force" means.
> 
> I certainly would not mind the order used in the result of this
> patch (in other words, if somebody posted a patch to add
> create_branch() function to the codebase that lacked it, with its
> parameters listed in the order this patch uses, I wouldn't
> complain), but it would have equally been OK if "reflog" and "force"
> were swapped without making any other change this patch makes.
> 

Makes sense. The unwanted shuffling was a consequence of my attempt to
see if the signature of the function did change when the position of
the 'enum' was changed. It seems there isn't change in its signature as
it is possible to use integers for enums and vice versa due to liberal
checking for misuses.

I've changed the signature back to keep alone "force" and
"clobber_head_ok" together.


Thanks,
Kaartic