From: Glen Choo <chooglen@google.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, jonathantanmy@google.com,
steadmon@google.com, emilyshaffer@google.com, avarab@gmail.com,
levraiphilippeblain@gmail.com, gitster@pobox.com
Subject: Re: [PATCH v3 1/5] branch: move --set-upstream-to behavior to setup_tracking()
Date: Thu, 09 Dec 2021 14:16:42 -0800 [thread overview]
Message-ID: <kl6lh7bha0fp.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <20211209211951.498358-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
>> This refactor is motivated by a desire to add a "dry_run" parameter to
>> create_branch() that will validate whether or not a branch can be
>> created without actually creating it - this behavior be used in a
>> subsequent commit that adds `git branch --recurse-submodules topic`.
>
> When I said that the patch was doing too much [1], I meant that for a
> patch that is supposed to be about refactoring, there were many lines
> that didn't seem to be a straightforward move. I've attached a version
> of the patch that I expected - notice that a reviewer can see that the
> lines are straightforwardly moved.
>
> [1] https://lore.kernel.org/git/20211123231035.3607109-1-jonathantanmy@google.com/
Thanks, the patch gave me a lot of think about.
Comparing yours and mine side-by-side makes it obvious that what I
thought was a 'simple' refactor is not so obvious to readers. I think I
can incorporate most of your patch.
>> +void setup_tracking(const char *new_ref, const char *orig_ref,
>> + enum branch_track track, int quiet, int expand_orig)
>> {
>> struct tracking tracking;
>> struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
>> int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
>> + char *full_orig_ref;
>> + struct object_id unused_oid;
>>
>> memset(&tracking, 0, sizeof(tracking));
>> - tracking.spec.dst = (char *)orig_ref;
>> + if (expand_orig)
>> + validate_branch_start(the_repository, orig_ref, track, &unused_oid, &full_orig_ref);
>> + else
>> + full_orig_ref = xstrdup(orig_ref);
>
> Having two meanings for a parameter (and which meaning used depending on
> another parameter) is quite confusing - I think it's better to call
> another function to expand the ref if necessary. See my patch below for
> an example.
Since you and Junio have both commented on this inconsistency, I'll drop
the extra parameter.
I wonder if you've considered another alternative [1], which is to
_always_ expand the ref, instead of _never_ expanding the ref. Always
expanding the ref wastes cycles, but it avoids creating an implicit
contract between setup_tracking() and
validate_branch_start_or_TODO_name().
>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> index 267a624671..18e285a876 100755
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -42,6 +42,23 @@ test_expect_success 'git branch abc should create a branch' '
>> git branch abc && test_path_is_file .git/refs/heads/abc
>> '
>>
>> +test_expect_success 'git branch abc should fail when abc exists' '
>> + test_must_fail git branch abc
>> +'
>> +
>> +test_expect_success 'git branch --force abc should fail when abc is checked out' '
>> + test_when_finished git switch main &&
>> + git switch abc &&
>> + test_must_fail git branch --force abc HEAD~1
>> +'
>> +
>> +test_expect_success 'git branch --force abc should succeed when abc exists' '
>> + git rev-parse HEAD~1 >expect &&
>> + git branch --force abc HEAD~1 &&
>> + git rev-parse abc >actual &&
>> + test_cmp expect actual
>> +'
>
> This seems like an unrelated test for a refactoring that doesn't seem to
> touch "force" (in builtin/branch.c, the line removed hardcodes force to
> 0).
I added this test because I was trying to simplify create_branch() by
moving around/deleting forcing and clobber_head_ok. It turns out that I
had actually broken the behavior of --force, but the test suite didn't
catch it.
I think this test is a step in the right direction, but it probably
falls into the category of 'incidental fix'. I am not sure where it
belongs if not here.
> Below is my diff, as mentioned.
>
> ---
> diff --git a/branch.c b/branch.c
> index 07a46430b3..a6803e9900 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -131,8 +131,8 @@ int install_branch_config(int flag, const char *local, const char *origin, const
> * to infer the settings for branch.<new_ref>.{remote,merge} from the
> * config.
> */
> -static void setup_tracking(const char *new_ref, const char *orig_ref,
> - enum branch_track track, int quiet)
> +void setup_tracking(const char *new_ref, const char *orig_ref,
> + enum branch_track track, int quiet)
> {
> struct tracking tracking;
> int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
> @@ -243,30 +243,12 @@ N_("\n"
> "will track its remote counterpart, you may want to use\n"
> "\"git push -u\" to set the upstream config as you push.");
>
> -void create_branch(struct repository *r,
> - const char *name, const char *start_name,
> - int force, int clobber_head_ok, int reflog,
> - int quiet, enum branch_track track)
> +void TODO_name(struct repository *r, const char *start_name, int explicit_tracking,
> + char **out_real_ref, struct commit **out_commit)
> {
> struct commit *commit;
> struct object_id oid;
> char *real_ref;
> - struct strbuf ref = STRBUF_INIT;
> - int forcing = 0;
> - int dont_change_ref = 0;
> - int explicit_tracking = 0;
> -
> - if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
> - explicit_tracking = 1;
> -
> - if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
> - ? validate_branchname(name, &ref)
> - : validate_new_branchname(name, &ref, force)) {
> - if (!force)
> - dont_change_ref = 1;
> - else
> - forcing = 1;
> - }
>
> real_ref = NULL;
> if (get_oid_mb(start_name, &oid)) {
> @@ -304,6 +286,38 @@ void create_branch(struct repository *r,
>
> if ((commit = lookup_commit_reference(r, &oid)) == NULL)
> die(_("Not a valid branch point: '%s'."), start_name);
> + if (out_real_ref)
> + *out_real_ref = real_ref;
> + if (out_commit)
> + *out_commit = commit;
> +}
Using a separate out_real_ref makes a lot more sense than reusing
real_ref. I'll borrow this :)
> +
> +void create_branch(struct repository *r,
> + const char *name, const char *start_name,
> + int force, int clobber_head_ok, int reflog,
> + int quiet, enum branch_track track)
> +{
> + struct commit *commit;
> + struct object_id oid;
> + char *real_ref = NULL;
> + struct strbuf ref = STRBUF_INIT;
> + int forcing = 0;
> + int dont_change_ref = 0;
> + int explicit_tracking = 0;
> +
> + if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
> + explicit_tracking = 1;
> +
> + if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
> + ? validate_branchname(name, &ref)
> + : validate_new_branchname(name, &ref, force)) {
> + if (!force)
> + dont_change_ref = 1;
> + else
> + forcing = 1;
> + }
This patch just moves things around without performing internal
simplification (like removing dont_change_ref). The simplification would
probably be done in a later patch.
Makes sense, I see how this is easier to review.
> @@ -821,12 +822,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> if (!ref_exists(branch->refname))
> die(_("branch '%s' does not exist"), branch->name);
>
> - /*
> - * create_branch takes care of setting up the tracking
> - * info and making sure new_upstream is correct
> - */
> - create_branch(the_repository, branch->name, new_upstream,
> - 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
> + TODO_name(the_repository, new_upstream, 1, &real_ref, NULL);
> + setup_tracking(branch->name, real_ref, BRANCH_TRACK_OVERRIDE, quiet);
> + free(real_ref);
> } else if (unset_upstream) {
> struct branch *branch = branch_get(argv[0]);
> struct strbuf buf = STRBUF_INIT;
See my previous comment and [1] about avoiding the implicit contract of
TODO_name() + setup_tracking(), but of course, this is still more
consistent than what what I proposed.
[1] https://lore.kernel.org/git/kl6l5yrzaq5z.fsf@chooglen-macbookpro.roam.corp.google.com
Thanks!
next prev parent reply other threads:[~2021-12-09 22:16 UTC|newest]
Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-22 22:32 [PATCH 0/4] implement branch --recurse-submodules Glen Choo
2021-11-22 22:32 ` [PATCH 1/4] submodule-config: add submodules_of_tree() helper Glen Choo
2021-11-23 2:12 ` Jonathan Tan
2021-11-23 19:48 ` Glen Choo
2021-11-23 10:53 ` Ævar Arnfjörð Bjarmason
2021-11-23 18:35 ` Glen Choo
2021-11-23 22:46 ` Junio C Hamano
2021-11-22 22:32 ` [PATCH 2/4] branch: refactor out branch validation from create_branch() Glen Choo
2021-11-22 22:32 ` [PATCH 3/4] branch: add --dry-run option to branch Glen Choo
2021-11-23 10:42 ` Ævar Arnfjörð Bjarmason
2021-11-23 18:42 ` Glen Choo
2021-11-23 23:10 ` Jonathan Tan
2021-11-24 0:52 ` Glen Choo
2021-11-22 22:32 ` [PATCH 4/4] branch: add --recurse-submodules option for branch creation Glen Choo
2021-11-23 10:45 ` Ævar Arnfjörð Bjarmason
2021-11-23 18:56 ` Glen Choo
2021-11-23 19:41 ` Philippe Blain
2021-11-23 23:43 ` Glen Choo
2021-11-24 1:31 ` Jonathan Tan
2021-11-24 18:18 ` Glen Choo
2021-11-29 21:01 ` Jonathan Tan
2021-12-06 21:55 ` [PATCH v2 0/3] implement branch --recurse-submodules Glen Choo
2021-12-06 21:55 ` [PATCH v2 1/3] branch: move --set-upstream-to behavior to setup_tracking() Glen Choo
2021-12-06 22:48 ` Junio C Hamano
2021-12-08 18:48 ` Glen Choo
2021-12-06 23:28 ` Junio C Hamano
2021-12-08 17:09 ` Glen Choo
2021-12-06 21:55 ` [PATCH v2 2/3] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-06 21:55 ` [PATCH v2 3/3] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-09 18:49 ` [PATCH v3 0/5] implement branch --recurse-submodules Glen Choo
2021-12-09 18:49 ` [PATCH v3 1/5] branch: move --set-upstream-to behavior to setup_tracking() Glen Choo
2021-12-09 21:19 ` Jonathan Tan
2021-12-09 22:16 ` Glen Choo [this message]
2021-12-09 18:49 ` [PATCH v3 2/5] branch: remove forward declaration of validate_branch_start() Glen Choo
2021-12-09 18:49 ` [PATCH v3 3/5] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-09 21:23 ` Jonathan Tan
2021-12-09 21:57 ` Glen Choo
2021-12-09 18:49 ` [PATCH v3 4/5] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-11 18:08 ` Philippe Blain
2021-12-14 20:08 ` Glen Choo
2021-12-09 18:49 ` [PATCH v3 5/5] branch.c: replace questionable exit() codes Glen Choo
2021-12-10 2:21 ` Ævar Arnfjörð Bjarmason
2021-12-10 17:43 ` Glen Choo
2021-12-13 9:02 ` Junio C Hamano
2021-12-13 9:19 ` Ævar Arnfjörð Bjarmason
2021-12-13 19:26 ` Junio C Hamano
2021-12-09 21:59 ` [PATCH v3 0/5] implement branch --recurse-submodules Jonathan Tan
2021-12-09 22:21 ` Glen Choo
2021-12-13 23:20 ` Jonathan Tan
2021-12-14 18:47 ` Glen Choo
2021-12-16 0:32 ` [PATCH v4 " Glen Choo
2021-12-16 0:32 ` [PATCH v4 1/5] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2021-12-16 0:32 ` [PATCH v4 2/5] branch: make create_branch() always create a branch Glen Choo
2021-12-16 0:32 ` [PATCH v4 3/5] branch: add a dry_run parameter to create_branch() Glen Choo
2021-12-16 0:32 ` [PATCH v4 4/5] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-16 0:32 ` [PATCH v4 5/5] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-16 23:33 ` [PATCH v5 0/5] implement branch --recurse-submodules Glen Choo
2021-12-16 23:33 ` [PATCH v5 1/5] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2021-12-16 23:33 ` [PATCH v5 2/5] branch: make create_branch() always create a branch Glen Choo
2021-12-16 23:33 ` [PATCH v5 3/5] branch: add a dry_run parameter to create_branch() Glen Choo
2021-12-16 23:33 ` [PATCH v5 4/5] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-16 23:33 ` [PATCH v5 5/5] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-17 0:34 ` [PATCH v5 0/5] implement branch --recurse-submodules Junio C Hamano
2021-12-17 0:45 ` Junio C Hamano
2021-12-20 19:09 ` Glen Choo
2021-12-20 19:50 ` Junio C Hamano
2021-12-20 20:25 ` Glen Choo
2021-12-20 23:34 ` [PATCH v6 " Glen Choo
2021-12-20 23:34 ` [PATCH v6 1/5] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2022-01-11 2:09 ` Jonathan Tan
2022-01-11 17:29 ` Glen Choo
2022-01-11 20:03 ` Jonathan Tan
2021-12-20 23:34 ` [PATCH v6 2/5] branch: make create_branch() always create a branch Glen Choo
2022-01-11 2:19 ` Jonathan Tan
2022-01-11 17:51 ` Glen Choo
2021-12-20 23:34 ` [PATCH v6 3/5] branch: add a dry_run parameter to create_branch() Glen Choo
2021-12-20 23:34 ` [PATCH v6 4/5] builtin/branch: clean up action-picking logic in cmd_branch() Glen Choo
2021-12-20 23:34 ` [PATCH v6 5/5] branch: add --recurse-submodules option for branch creation Glen Choo
2021-12-26 4:09 ` Junio C Hamano
2022-01-11 3:28 ` Jonathan Tan
2022-01-11 18:11 ` Glen Choo
2022-01-11 20:15 ` Jonathan Tan
2022-01-11 23:22 ` Glen Choo
2021-12-20 23:36 ` [PATCH v6 0/5] implement branch --recurse-submodules Glen Choo
2021-12-21 1:07 ` Junio C Hamano
2021-12-21 17:51 ` Glen Choo
2022-01-24 20:44 ` [PATCH v7 0/6] " Glen Choo
2022-01-24 20:44 ` [PATCH v7 1/6] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2022-01-24 20:44 ` [PATCH v7 2/6] branch: make create_branch() always create a branch Glen Choo
2022-01-24 20:44 ` [PATCH v7 3/6] branch: add a dry_run parameter to create_branch() Glen Choo
2022-01-24 20:44 ` [PATCH v7 4/6] builtin/branch: consolidate action-picking logic in cmd_branch() Glen Choo
2022-01-24 20:44 ` [PATCH v7 5/6] branch: add --recurse-submodules option for branch creation Glen Choo
2022-01-27 20:29 ` Jonathan Tan
2022-01-27 21:32 ` Glen Choo
2022-01-27 22:42 ` Glen Choo
2022-01-24 20:44 ` [PATCH v7 6/6] branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaks Glen Choo
2022-01-27 22:15 ` Junio C Hamano
2022-01-28 19:44 ` Glen Choo
2022-01-29 0:04 ` [PATCH v8 0/6] implement branch --recurse-submodules Glen Choo
2022-01-29 0:04 ` [PATCH v8 1/6] branch: move --set-upstream-to behavior to dwim_and_setup_tracking() Glen Choo
2022-01-29 0:04 ` [PATCH v8 2/6] branch: make create_branch() always create a branch Glen Choo
2022-02-01 22:20 ` Junio C Hamano
2022-01-29 0:04 ` [PATCH v8 3/6] branch: add a dry_run parameter to create_branch() Glen Choo
2022-01-29 0:04 ` [PATCH v8 4/6] builtin/branch: consolidate action-picking logic in cmd_branch() Glen Choo
2022-01-29 0:04 ` [PATCH v8 5/6] branch: add --recurse-submodules option for branch creation Glen Choo
2022-02-04 1:10 ` Glen Choo
2022-02-04 16:15 ` Junio C Hamano
2022-02-04 18:10 ` Glen Choo
2022-01-29 0:04 ` [PATCH v8 6/6] branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaks Glen Choo
2022-02-01 17:43 ` [PATCH v8 0/6] implement branch --recurse-submodules Jonathan Tan
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=kl6lh7bha0fp.fsf@chooglen-macbookpro.roam.corp.google.com \
--to=chooglen@google.com \
--cc=avarab@gmail.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=levraiphilippeblain@gmail.com \
--cc=steadmon@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).