| Message ID | 890e016bfc0809d25a4ae8ae924b23895f520810.1645815142.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | adding new branch.autosetupmerge option "simple" | expand |
"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Tao Klerks <tao@klerks.biz> > > This commit introduces a new option to the branch.autosetupmerge > setting, "simple", which is intended to be consistent with and > complementary to the push.default "simple" option. Documentation/SubmittingPatches. We do not say "This commit does this". Instead, we say "Add a new option that does X". Usually that is done after the explanation of the status quo is finished to make readers understand what the problem the change is trying to solve is. So... > The push.defaut option "simple" helps produce > predictable/understandable behavior for beginners, where they don't > accidentally push to the "wrong" branch in centralized workflows. If > they create a local branch with a different name and then try to do a > plain push, it will helpfully fail and explain why. ... this would be a better first paragraph to start the proposed log message with. With push.default set to "simple", the users fork from a local branch from a remote-tracking branch of the same name, and are protected from a mistake to push to a wrong branch. If they create a ... and explain why. > However, such users can often find themselves confused by the behavior > of git after they first branch, and before they push. At that stage, Depending on how they "branch", they may or may not be confused. Be more specific to illustrate what problem you are solving, e.g. ... after they create a new local branch from a remote-tracking branch with a different name. > their upstream tracking branch is the original remote branch, and pull > will be bringing in "upstream changes" - eg all changes to "main", in > a typical project where that's where they branched from. OK. So "pull" tries to grab from the upstream (which is most likely an integration branch with bland name like 'master', 'main' or 'trunk'), while "push" does not allow the work on a branch (which is named after the theme of the work and not a bland name suitable for integration branches) to be pushed to the upstream. It may probably not be so clear why it is a problem to many readers, I suspect. Isn't that what happens in a typical triangular workflow to work with a project with a centralized repository? You fork from the integration branch shared among project participants, you work on your own branch, occasionally rebasing on top of the updated upstream, and when you are done, try to push it out to the integration branch, and that final leg needs to be explicit to make sure you won't push out to a wrong branch (in this case, a new branch at the remote with the same name as your local topic branch) by mistake? > On the other hand, once they push their new branch (dealing with the > initial error, following instructions to push to the right name), > subsequent "pull" calls will behave as expected, only bring in any > changes to that new branch they pushed. Is that because the upstream for this local branch is updated? The "following instructions..." part may want to clarify. It somehow feels that a better solution might be to suggest updating the push.default to 'upstream' when it happens? I dunno. In any case, now we have explained what happens with today's code, here is a good place to propose a solution. Do so in imperative, e.g. Allow branch.autosetupmerge to take a new value, 'simple', which sets the upstream of the new branch only when the local branch being created has the same name as the remote-tracking branch it was created out of. Otherwise the new local branch will not get any tracking information and or something, perhaps? > + /* > + * This check does not apply to the BRANCH_TRACK_INHERIT > + * option; you can inherit one or more tracking entries > + * and the tracking.matches counter is not incremented. > + */ > if (tracking.matches > 1) > die(_("not tracking: ambiguous information for ref %s"), > orig_ref); > + if (track == BRANCH_TRACK_SIMPLE) { > + /* > + * Only track if remote branch name matches. > + * Reaching into items[0].string is safe because > + * we know there is at least one and not more than > + * one entry (because not BRANCH_TRACK_INHERIT). > + */ OK, because in the pre-context of this hunk, we would have jumped to cleanup: if there were no .matches; so we know there should at least be one, and we rejected ambiguous matches already, so we know there is only one. > + const char *tracked_branch; > + if (!skip_prefix(tracking.srcs->items[0].string, > + "refs/heads/", &tracked_branch) || > + strcmp(tracked_branch, new_ref)) > + return; > + } That looks sensible. Sometimes we do not set tracking information and just return.
On Fri, Feb 25, 2022 at 9:15 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > This commit introduces a new option to the branch.autosetupmerge > > setting, "simple", which is intended to be consistent with and > > complementary to the push.default "simple" option. > > Documentation/SubmittingPatches. > > We do not say "This commit does this". Instead, we say "Add a new > option that does X". Usually that is done after the explanation of > the status quo is finished to make readers understand what the > problem the change is trying to solve is. So... Yep, sorry, thx! (fixed, reroll coming!) > > > The push.defaut option "simple" helps produce > > predictable/understandable behavior for beginners, where they don't > > accidentally push to the "wrong" branch in centralized workflows. If > > they create a local branch with a different name and then try to do a > > plain push, it will helpfully fail and explain why. > > ... this would be a better first paragraph to start the proposed log > message with. > > With push.default set to "simple", the users fork from a > local branch from a remote-tracking branch of the same name, > and are protected from a mistake to push to a wrong branch. > If they create a ... and explain why. > > > However, such users can often find themselves confused by the behavior > > of git after they first branch, and before they push. At that stage, > > Depending on how they "branch", they may or may not be confused. Be > more specific to illustrate what problem you are solving, e.g. > > ... after they create a new local branch from a > remote-tracking branch with a different name. > > > their upstream tracking branch is the original remote branch, and pull > > will be bringing in "upstream changes" - eg all changes to "main", in > > a typical project where that's where they branched from. > > OK. So "pull" tries to grab from the upstream (which is most likely > an integration branch with bland name like 'master', 'main' or > 'trunk'), while "push" does not allow the work on a branch (which is > named after the theme of the work and not a bland name suitable for > integration branches) to be pushed to the upstream. > > It may probably not be so clear why it is a problem to many readers, > I suspect. Isn't that what happens in a typical triangular workflow > to work with a project with a centralized repository? You fork from > the integration branch shared among project participants, you work on > your own branch, occasionally rebasing on top of the updated upstream, > and when you are done, try to push it out to the integration branch, > and that final leg needs to be explicit to make sure you won't push > out to a wrong branch (in this case, a new branch at the remote with > the same name as your local topic branch) by mistake? > > > On the other hand, once they push their new branch (dealing with the > > initial error, following instructions to push to the right name), > > subsequent "pull" calls will behave as expected, only bring in any > > changes to that new branch they pushed. > > Is that because the upstream for this local branch is updated? > The "following instructions..." part may want to clarify. > > It somehow feels that a better solution might be to suggest > updating the push.default to 'upstream' when it happens? I dunno. > > In any case, now we have explained what happens with today's code, > here is a good place to propose a solution. Do so in imperative, > e.g. > > Allow branch.autosetupmerge to take a new value, 'simple', which > sets the upstream of the new branch only when the local branch > being created has the same name as the remote-tracking branch it > was created out of. Otherwise the new local branch will not get > any tracking information and > > or something, perhaps? Thank you for taking the time to make sense of the rambling / largely incoherent message and helping me identify some context other reviewers will expect. I've rewritten the whole thing to try to address these concerns, but of course I may well have introduced a whole new set. If nothing else, it's become even more rambling. Is there a recommended limit to the length of a commit message?
diff --git a/Documentation/config/branch.txt b/Documentation/config/branch.txt index 1e0c7af014b..8df10d07129 100644 --- a/Documentation/config/branch.txt +++ b/Documentation/config/branch.txt @@ -9,7 +9,9 @@ branch.autoSetupMerge:: automatic setup is done when the starting point is either a local branch or remote-tracking branch; `inherit` -- if the starting point has a tracking configuration, it is copied to the new - branch. This option defaults to true. + branch; `simple` -- automatic setup is done only when the starting point + is a remote-tracking branch and the new branch has the same name as the + remote branch. This option defaults to true. branch.autoSetupRebase:: When a new branch is created with 'git branch', 'git switch' or 'git checkout' diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt index c8b4f9ce3c7..ae82378349d 100644 --- a/Documentation/git-branch.txt +++ b/Documentation/git-branch.txt @@ -221,13 +221,17 @@ The exact upstream branch is chosen depending on the optional argument: itself as the upstream; `--track=inherit` means to copy the upstream configuration of the start-point branch. + -`--track=direct` is the default when the start point is a remote-tracking branch. -Set the branch.autoSetupMerge configuration variable to `false` if you -want `git switch`, `git checkout` and `git branch` to always behave as if `--no-track` -were given. Set it to `always` if you want this behavior when the -start-point is either a local or remote-tracking branch. Set it to -`inherit` if you want to copy the tracking configuration from the -branch point. +The branch.autoSetupMerge configuration variable specifies how `git switch`, +`git checkout` and `git branch` should behave when neither `--track` nor +`--no-track` are specified: ++ +The default option, `true`, behaves as though `--track=direct` +were given whenever the start-point is a remote-tracking branch. +`false` behaves as if `--no-track` were given. `always` behaves as though +`--track=direct` were given. `inherit` behaves as though `--track=inherit` +were given. `simple` behaves as though `--track=direct` were given only when +the start-point is a remote-tracking branch and the new branch has the same +name as the remote branch. + See linkgit:git-pull[1] and linkgit:git-config[1] for additional discussion on how the `branch.<name>.remote` and `branch.<name>.merge` options are used. diff --git a/branch.c b/branch.c index 6b31df539a5..81613ade8bf 100644 --- a/branch.c +++ b/branch.c @@ -252,10 +252,29 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, goto cleanup; } + /* + * This check does not apply to the BRANCH_TRACK_INHERIT + * option; you can inherit one or more tracking entries + * and the tracking.matches counter is not incremented. + */ if (tracking.matches > 1) die(_("not tracking: ambiguous information for ref %s"), orig_ref); + if (track == BRANCH_TRACK_SIMPLE) { + /* + * Only track if remote branch name matches. + * Reaching into items[0].string is safe because + * we know there is at least one and not more than + * one entry (because not BRANCH_TRACK_INHERIT). + */ + const char *tracked_branch; + if (!skip_prefix(tracking.srcs->items[0].string, + "refs/heads/", &tracked_branch) || + strcmp(tracked_branch, new_ref)) + return; + } + if (tracking.srcs->nr < 1) string_list_append(tracking.srcs, orig_ref); if (install_branch_config_multiple_remotes(config_flags, new_ref, diff --git a/branch.h b/branch.h index 04df2aa5b51..560b6b96a8f 100644 --- a/branch.h +++ b/branch.h @@ -12,6 +12,7 @@ enum branch_track { BRANCH_TRACK_EXPLICIT, BRANCH_TRACK_OVERRIDE, BRANCH_TRACK_INHERIT, + BRANCH_TRACK_SIMPLE, }; extern enum branch_track git_branch_track; diff --git a/config.c b/config.c index e0c03d154c9..cc586ac816c 100644 --- a/config.c +++ b/config.c @@ -1673,6 +1673,9 @@ static int git_default_branch_config(const char *var, const char *value) } else if (value && !strcmp(value, "inherit")) { git_branch_track = BRANCH_TRACK_INHERIT; return 0; + } else if (value && !strcmp(value, "simple")) { + git_branch_track = BRANCH_TRACK_SIMPLE; + return 0; } git_branch_track = git_config_bool(var, value); return 0;