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

[Stefan Beller <sbeller@google.com>]

On Fri, Oct 20, 2017 at 7:56 PM, Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
> On Fri, 2017-10-20 at 14:50 -0700, Stefan Beller wrote:
>> On Mon, Sep 25, 2017 at 1:20 AM, Kaartic Sivaraam
>> <kaartic.sivaraam@gmail.com> wrote:
>> > 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.
>> >
>> > So, re-order the arguments to keep the related arguments close to each
>> > other.
>>
>> Thanks for taking a lot at the code quality in detail.
>>
>> In my currently checked out version of Git, there are two occurrences
>> of create_branch in builtin/branch.c, this patch converts only one occurrence.
>>
>> (So you'd need to convert that second one, too. Oh wait; it is converted
>> implicitly as the arguments are both '0':
>>     create_branch(branch->name, new_upstream, 0, 0, 0, \
>>         quiet, BRANCH_TRACK_OVERRIDE);
>> )
>>
>> This leads to the generic problem of this patch: Assume there are other
>> contributors that write patches. They may introduce new calls to
>> `create_branch`, but using the order of parameters as they may
>> be unaware of this patch and they'd test without this patch.
>>
>> As the signature of the function call doesn't change the compiler
>> doesn't assist us in finding such a potential race between patches.
>>
>> I am not aware of any other patches in flight, so we *might* be fine
>> with this patch. But hope is rarely a good strategy.
>>
>> Can we change the function signature (the name or another order
>> of arguments that also makes sense, or making one of the
>> parameters an enum) such that a potential race can be detected
>> easier?
>>
>
> I don't have a great interest in keeping this patch in case it might
> conflict with other patches. Anyways, I guess we could avoid the issue
> by making the last 'enum' parameter as the third parameter. It pretty
> well changes the order by moving the flag-like parameters to the last
> but it doesn't change the signature very strongly as you can pass
> integers in the place of enums. (I guess that also obviates the
> suggestion of making one parameter an enum)
>
> So, the only way around is to rename the function which is something I
> wouldn't like to do myself unless other people like the idea. So,
> should I drop this patch or should I rename the function?

Well let's keep the patch and closely watch next/pu to see if there might
topics that conflict, then. I do really like these small fixes to make the
code more readable. e.g.

  $ git log --oneline origin/master..origin/pu  -G create_branch