From: Glen Choo <chooglen@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Jonathan Tan" <jonathantanmy@google.com>,
"Josh Steadmon" <steadmon@google.com>,
"Emily Shaffer" <emilyshaffer@google.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Philippe Blain" <levraiphilippeblain@gmail.com>
Subject: Re: [PATCH v2 1/3] branch: move --set-upstream-to behavior to setup_tracking()
Date: Wed, 08 Dec 2021 10:48:40 -0800 [thread overview]
Message-ID: <kl6l5yrzaq5z.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <xmqqbl1tcptq.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
>> As Jonathan noted in v1, the diff is quite large. I could shrink this
>> by forward-declaring setup_tracking() so that the function definitions
>> are in the same order; let me know if that would be preferred.
>
> If you are not making any changes to setup_tracking() and just
> moving, then the patch size inflated by the move is OK.
>
> If you are moving and changing at the same time, well, that would
> make it harder to read what is going on in the patch, so you want to
> find a way to avoid it. Splitting it the pure move into a separate
> patch or use of forward-declaration may be good ways to do so.
Thanks for the advice! I'll use one of these approaches.
> The post context continues with:
>
> advise(_(upstream_advice));
> exit(1);
> }
> die(_(upstream_missing), start_name);
>
> This is not a problem with this patch, and it should not be fixed as
> part of this series, but since I noticed it, I'll mention it as a
> leftover low-hanging fruit to be fixed after the dust settles. The
> exit(1) looks wrong. We should exit with 128 just like die() does.
> Issuing of an advice message should not affect the exit code.
I'll include an optional cleanup patch to address this and the exit(-1).
>> +void setup_tracking(const char *new_ref, const char *orig_ref,
>> + enum branch_track track, int quiet, int expand_orig)
>
> It is unclear what expand_orig option is supposed to do and how it
> would help the caller. Perhaps a comment before the function is in
> order (the comment in branch.h before the declaration of this
> function does not make it clear, either).
Ah, I will add extra clarification.
This boolean parameter controls whether or not we should validate + DWIM
orig_ref. This is a performance optimization to avoid expanding an
orig_ref that has already been expanded (e.g. because create_branch()
has already expanded it).
But, as you point out later on, this may not be a good idea.
>> +{
>> + struct tracking tracking;
>> + int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
>> + char *full_orig_ref;
>> + struct object_id unused_oid;
>> +
>> + memset(&tracking, 0, sizeof(tracking));
>> + if (expand_orig)
>> + validate_branch_start(the_repository, orig_ref, track, &unused_oid, &full_orig_ref);
>
> So, the idea is, because we are setting up a new_ref to "track"
> orig_ref, we may be better off pretending that we are "creating a
> new branch from the orig_ref and tracking it", so that orig_ref that
> is not something we can track will be caught with the same logic?
>
> This will cause full_orig_ref to start with "refs/heads/" or
> "refs/remotes/" if 'track' is something that requires tracking.
Yes.
>> + else
>> + full_orig_ref = xstrdup(orig_ref);
>
> Even though the variable claims to be FULL orig_ref, when this side
> of if/else is taken, nobody guarantees that full_orig_ref is in fact
> a full ref, or merely the name of the branch, no? Would that cause
> problems later?
Yes, the assumption is that the caller has already done the work of
making sure that orig_ref has been validated and expanded into its full
form. As mentioned earlier, this is purely a performance optimzation,
but it is not a very safe one because it requires the caller to pick the
correct value for expand_orig.
I chose this approach because I anticipate that the only callers of
setup_tracking() will be create_branch() (which always wants expand_orig
= 0) and cmd_branch() (which always wants expand_orig = 1), so the right
course of action is clear for now. I don't think setup_tracking() will
be useful to anyone else, but expand_orig is a potential sharp edge for
new callers.
My next-preferred option would be to remove "expand_orig" and always
call validate_branch_start(). We might waste a few cycles sometimes, but
the function becomes impossible to misuse.
>> + tracking.spec.dst = full_orig_ref;
>> + if (for_each_remote(find_tracked_branch, &tracking))
>> + goto cleanup;
>> +
>> + if (!tracking.matches)
>> + switch (track) {
>> + case BRANCH_TRACK_ALWAYS:
>> + case BRANCH_TRACK_EXPLICIT:
>> + case BRANCH_TRACK_OVERRIDE:
>> + break;
>> + default:
>> + goto cleanup;
>> + }
>> +
>> + if (tracking.matches > 1)
>> + die(_("Not tracking: ambiguous information for ref %s"),
>> + full_orig_ref);
>
> What's the next step for the user to take, after seeing this message?
> Do we have the necessary info readily available to help them at this
> point in tracking.* structure (e.g. "it could be following X or Y and
> we cannot decide between the two for you"), or have we discarded the
> information already?
This information is discarded in expand_ref(). From the function
signature:
int expand_ref(struct repository *repo, const char *str, int len,
struct object_id *oid, char **ref)
The return value is the number of matched refs and "ref" is an out
parameter containing the first matched ref.
> If tracking.matches == 0, because we broke out of the switch() for
> some values of track, we will make this install_branch_config()
> using members of the tracking structure, which is a bit unnerving.
>
>> + if (install_branch_config(config_flags, new_ref, tracking.remote,
>> + tracking.src ? tracking.src : full_orig_ref) < 0)
>
> But tracking.src==NULL is substituted with full_orig_ref, so as long
> as the value in that variable is sensible, we would probably be ok
> on the 4th parameter.
When tracking.matches is zero, the assumption is that
install_branch_config() should set up tracking based off a local branch.
By default, we assume that users only want tracking for remote-tracking
branches, so only track local branches if we are confident that the user
wants this, aka track = BRANCH_TRACK_{EXPLICIT,ALWAYS,OVERRIDE} (and
soon, BRANCH_TRACK_INHERIT).
> I am not sure who set tracking.remote or if it is always set to a
> sensible value. Especially when tracking.matches is zero.
tracking.matches == 0 should imply that tracking.remote == NULL, which
gives us the expected behavior of tracking a local branch in
install_branch_config().
This is also a bit too implicit for my tastes, but I don't think this is
the time for such a refactor :)
next prev parent reply other threads:[~2021-12-08 18:48 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 [this message]
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
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=kl6l5yrzaq5z.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).