Re: [PATCH/RFC v2 2/6] branch: add copy branch option

[Sahil Dua <sahildua2305@gmail.com>]

On Thu, Jun 1, 2017 at 3:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Sahil Dua <sahildua2305@gmail.com> writes:
>
>> Adds copy branch option available using -c or -C (forcefully).
>>
>> Includes a lot of function renames and their signature changes in order
>> to introduce a new function parameter - flag 'copy' which determines
>> whether those functions should do operation copy or move.
>>
>> Additionally, this changes a lot of other files wherever the renamed
>> functions were used. By default copy=0 is passed at all those places so
>> that they keep behaving the way they were, before these changes.
>
> Things like rename_branch() that is narrowly confined inside a
> single program (i.e. builtin/branch.c), if renaming and copying
> shares a lot of logic and there is only a single caller to rename,
> it may be OK to rename the function to rename_or_copy_branch() and
> pass a new "are we doing copy or move?" parameter, but for lower
> level infrastructure like config_rename_section(), I am afraid to
> say that such a change is totally unacceptable.  When the current
> callers are content with rename_section(), and have no need to ever
> copy, why should they be forced tocall copy-or-rename with copy set
> to 0?
>
> When the original code looks like:
>
>
>     == caller (there are many) ==
>
>     rename_it(a, b);
>
>     == implementation (only one) ==
>
>     int rename_it(src, dst) {
>         ... logic to create dst by copying src ...
>         ... logic to remove src ...
>     }
>
> You could introduce a common helper
>
>     == implementation ==
>
>     int rename_or_copy_it(src, dst, copy?) {
>         ... logic to create dst by copying src ...
>         if (!copy?) {
>             ... logic to remove src ...
>         }
>     }
>
> but to help the current code (and possibly code somebody _else_ is
> developing elsewhere), you can also do it in a much less disruptive
> way.
>
>     == implementation ==
>
>     static int rename_or_copy_it(src, dst, copy?) {
>         ... logic to create dst by copying src ...
>         if (!copy?) {
>             ... logic to remove src ...
>         }
>     }
>
>     int rename_it(src, dst) {
>         return rename_or_copy_it(src, dst, 0);
>     }
>
>     int copy_it(src, dst) {
>         return rename_or_copy_it(src, dst, 1);
>     }
>
> Existing callers of "rename" that are not interested in your new
> "copy" thing can be left oblivious to it if you did it that way.
>

Thanks for your comments. I was already a little sceptic about making
changes in all the callers for config_rename_section function. I will
make the changes to have a helper function so that the existing
callers of this method aren't affected at all.