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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 1, 2017, 01:50 UTC
Message-ID: <xmqqfufk8oik.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <0102015c60dcf680-4a036e9e-c152-41d2-a932-b44c1a5bf68e-000000@eu-west-1.amazonses.com>

Sahil Dua <sahildua2305@gmail.com> writes:

Show 9 quoted lines

> 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.