From: Junio C Hamano <gitster@pobox•com>
To: Sahil Dua <sahildua2305@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH/RFC v2 2/6] branch: add copy branch option
Date: Thu, 01 Jun 2017 10:50:27 +0900 [thread overview]
Message-ID: <xmqqfufk8oik.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <0102015c60dcf680-4a036e9e-c152-41d2-a932-b44c1a5bf68e-000000@eu-west-1.amazonses.com> (Sahil Dua's message of "Wed, 31 May 2017 23:35:12 +0000")
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.
next prev parent reply other threads:[~2017-06-01 1:50 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-28 22:56 [PATCH/RFC] branch: add tests for new copy branch feature Sahil Dua
2017-05-28 23:30 ` Ævar Arnfjörð Bjarmason
2017-05-29 20:41 ` Sahil Dua
2017-05-29 20:50 ` Ævar Arnfjörð Bjarmason
2017-05-29 22:23 ` Sahil Dua
2017-06-13 17:55 ` Jonathan Nieder
2017-06-13 18:01 ` Ævar Arnfjörð Bjarmason
2017-06-13 18:08 ` Jonathan Nieder
2017-05-29 2:09 ` Junio C Hamano
2017-05-29 19:39 ` Sahil Dua
2017-05-31 23:35 ` [PATCH/RFC v2 1/6] " Sahil Dua
2017-05-31 23:35 ` [PATCH/RFC v2 6/6] branch: don't copy or rename config when same branch name Sahil Dua
2017-05-31 23:35 ` [PATCH/RFC v2 4/6] config: modify function signature to include copy argument Sahil Dua
2017-05-31 23:35 ` [PATCH/RFC v2 5/6] config: add copy config section logic Sahil Dua
2017-05-31 23:35 ` [PATCH/RFC v2 2/6] branch: add copy branch option Sahil Dua
2017-06-01 1:50 ` Junio C Hamano [this message]
2017-06-01 16:09 ` Sahil Dua
2017-05-31 23:35 ` [PATCH/RFC v2 3/6] config: abstract out create section from key logic Sahil Dua
2017-06-01 18:35 ` [PATCH/RFC v3 1/3] branch: add tests for new copy branch feature Sahil Dua
2017-06-01 18:35 ` [PATCH/RFC v3 2/3] config: abstract out create section from key logic Sahil Dua
2017-06-01 18:35 ` [PATCH/RFC v3 3/3] branch: add copy branch feature implementation Sahil Dua
2017-06-01 18:59 ` Ævar Arnfjörð Bjarmason
2017-06-01 22:05 ` Sahil Dua
2017-06-05 20:40 ` [PATCH/RFC v4 1/3] branch: add tests for new copy branch feature Sahil Dua
2017-06-05 20:40 ` [PATCH/RFC v4 2/3] config: abstract out create section from key logic Sahil Dua
2017-06-05 20:40 ` [PATCH/RFC v4 3/3] branch: add copy branch feature implementation Sahil Dua
2017-06-05 20:52 ` Sahil Dua
2017-06-06 0:10 ` Junio C Hamano
2017-06-06 0:14 ` Junio C Hamano
2017-06-06 7:39 ` Ævar Arnfjörð Bjarmason
2017-06-06 10:13 ` Sahil Dua
2017-06-06 12:03 ` Junio C Hamano
2017-06-13 16:17 ` [PATCH 1/3] config: create a function to format section headers Sahil Dua
2017-06-13 16:17 ` [PATCH 2/3] branch: add test for -m renaming multiple config sections Sahil Dua
2017-06-13 17:10 ` Junio C Hamano
2017-06-13 17:31 ` Ævar Arnfjörð Bjarmason
2017-06-13 17:39 ` Junio C Hamano
2017-06-13 17:53 ` Ævar Arnfjörð Bjarmason
2017-06-18 21:17 ` [PATCH v2 " Sahil Dua
2017-06-13 16:17 ` [PATCH 3/3] branch: add a --copy (-c) option to go with --move (-m) Sahil Dua
2017-06-13 17:05 ` Junio C Hamano
2017-06-13 17:30 ` Junio C Hamano
2017-06-14 8:01 ` Sahil Dua
2017-06-18 21:19 ` [PATCH v2 " Sahil Dua
2017-06-13 17:06 ` [PATCH 1/3] config: create a function to format section headers Junio C Hamano
2017-06-13 17:09 ` Ævar Arnfjörð Bjarmason
2017-06-18 21:16 ` [PATCH v2 " Sahil Dua
2017-06-19 12:08 ` Ramsay Jones
2017-06-19 14:51 ` Sahil Dua
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqfufk8oik.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=sahildua2305@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox