On Fri, Oct 20, 2017 at 7:56 PM, Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> 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@xxxxxxxxx> 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
