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

On Fri, Oct 20, 2017 at 7:56 PM, Kaartic Sivaraam
[off-list ref] wrote:

On Fri, 2017-10-20 at 14:50 -0700, Stefan Beller wrote:

quoted

On Mon, Sep 25, 2017 at 1:20 AM, Kaartic Sivaraam
[off-list ref] wrote:

quoted

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