Jeff King wrote: > On Mon, Jun 12, 2017 at 06:38:17PM -0700, Jonathan Nieder wrote: >> Brandon Williams wrote: >>> On 06/12, Jonathan Nieder wrote:
>>>> Alternatively, could this patch rename git_config_with_options? That >>>> way any other patch in flight that calls git_config_with_options would >>>> conflict with this patch, giving us an opportunity to make sure it >>>> also sets git_dir. As another nice side benefit it would make it easy >>>> for someone reading the patch to verify it didn't miss any callers. >> [...] >>> And I don't know if I agree with renaming a function just to rename it. >> >> I forgot to say: another way to accomplish the same thing can be to >> reorder the function's arguments. The relevant thing is to make code >> that calls the function without being aware of the new requirements >> fail to compile. > > If the parameter is now required, then it might make sense for it to > become an actual function parameter instead of being stuffed into the > config_options struct. That would give you your breaking change, plus > make it more obvious to the reader that it is not optional. I like it. I also like that in your proposed patch the no longer necessary local git_dir goes away. What's the next step? The discussion of get_git_common_dir() reveals that switching from git_path() to mkpathdup() loses the adjust_git_path() step and everything becomes complicated. So perhaps adding opts.git_common_dir and using that instead of git_path should happen as a preliminary patch, making this patch afterward a more straightforward refactoring. Thanks, Jonathan
