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

[Stefan Beller <sbeller@google.com>]

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?

Thanks,
Stefan