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? -- Kaartic
