New options to support "simple" centralized workflow #1161
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There are issues in commit f18acad: |
|
There are issues in commit 7c74ceb: |
TaoK
force-pushed
the
feature-branch-autosetupmerge-simple
branch
2 times, most recently
from
86c75b7
to
39c1490
Compare
|
/submit |
|
Submitted as pull.1161.git.1645695940.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
branch.c
Outdated
| @@ -256,6 +256,15 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, | |||
| die(_("not tracking: ambiguous information for ref %s"), | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Tao Klerks <tao@klerks.biz>
>
> The push.defaut option "simple" helps produce
The cover letter wrappeed around 70 columns, which was much easier
to read.
Please re-read Documentation/SubmittingPatches[[describe-changes]]
section before going forward.
> 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.
>
> However, such users can often find themselves
> confused by the behavior of git after they first
> branch, and before they push. At that stage,
> their upstream tracking branch is the original
> remote branch, and pull (for example) behaves
> very differently to how it later does when they
> create their own same-name remote branch.
Instead of saying "very differently", explain what happens before
and after the behaviour-change-triggering-event.
> 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.
>
> It will set up automatic tracking for a new
> branch only if the remote ref is a branch and
> that remote branch name matches the new local
> branch name. It is a reduction in scope of
> the existing default option, "true".
>
> Signed-off-by: Tao Klerks <tao@klerks.biz>
> ---
> branch.c | 9 +++++++++
> branch.h | 1 +
> config.c | 3 +++
> 3 files changed, 13 insertions(+)
>
> diff --git a/branch.c b/branch.c
> index 6b31df539a5..246bc82ce3c 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -256,6 +256,15 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
> die(_("not tracking: ambiguous information for ref %s"),
> orig_ref);
>
> + if (track == BRANCH_TRACK_SIMPLE) {
> + // only track if remote branch name matches
> + // (tracking.srcs must contain only one entry from find_tracked_branch with this config)
/*
* Our multi-line comments look exactly
* like this. They are not overly long,
* have their opening and closing slash-aster
* and aster-slash on their own line.
*/
> + if (strncmp(tracking.srcs->items[0].string, "refs/heads/", 11))
> + return;
> + if (strcmp(tracking.srcs->items[0].string + 11, new_ref))
> + return;
Don't count hardcoded string length.
char *tracked_branch;
if (!skip_prefix(tracking.srcs->items[0].string,
"refs/heads/", &tracked_branch) ||
strcmp(tracked_branch, new_ref))
return;
or something along the line, perhaps?
But the post-context in this hunk makes the refernece to items[0] in
the above look very wrong. It says tracking.srcs may not have even
a single item at this point in the original code flow. If that is
true, the above reference to ->items[0] may not be safely done at
all.
Also, what happens when there are more than one in the items[]
array? What makes it sensible to use the first one, ignoring the
others?
> + }
> +
> 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;
These two hunks look perfect.
| @@ -9,7 +9,9 @@ branch.autoSetupMerge:: | |||
| automatic setup is done when the starting point is either a | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Tao Klerks <tao@klerks.biz>
>
> Updating the branch and config documentation to reflect
> the new "simple" option to branch.autosetupmerge.
Documentation/Submittingpatches[[describe-changes]].
But it would be moot; these changes are better done as part of [1/3]
and in that case, updating the documentation (or testing the desired
behaviour, for that matter) is not something we need to justify
separately. It is something we must done as part of the change.
> diff --git a/Documentation/config/branch.txt b/Documentation/config/branch.txt
> index 1e0c7af014b..7b4e5ca5b74 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 when the starting point is
It may be clearer to say "done only when". I dunno.
> + a remote-tracking branch and the new branch has the same name as the
> + remote branch. This option defaults to true.
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index c8b4f9ce3c7..f99d6a6b008 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -227,7 +227,9 @@ want `git switch`, `git checkout` and `git branch` to always behave as if `--no-
> 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.
> +branch point. Set it to `simple` if you want this behavior only when
> +the start-point is a remote branch and the new branch has the same name
> +as the remote branch.
The existing "if you want this behaviour when" is already awkward.
What it means is that only those who want to use the "start-point"
itself as the upstream whether the start-point is local or
remote-tracking,can use "always" and does not get hurt.
But using the phrase for "simple" makes it even worse, as the
condition that the tracking behaviour kicks in is even narrower. If
you know that start-point is not a remote-tracking branch (by the
way, do not say "remote branch" when you mean "remote-tracking
brnach"), or its name is not the same as the local branch, you just
do not pass --track=simple from the command line. Strike everything
after "Set it to `simple`" and replace with something like
`--track=simple` sets up the upstream information only when the
start-point is a remote-tracking branch and ...
perhaps?
Thanks.
b3ca520
to
c16a8fe
Compare
|
/submit |
|
Submitted as pull.1161.v2.git.1645815142.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| @@ -9,7 +9,9 @@ branch.autoSetupMerge:: | |||
| automatic setup is done when the starting point is either a | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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.
There was a problem hiding this comment.
On the Git mailing list, Tao Klerks wrote (reply to this):
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?
|
This branch is now known as |
|
This patch series was integrated into seen via git@cf908f9. |
|
There was a status update in the "New Topics" section about the branch "git -c branch.autosetupmerge=simple branch $A $B" will set the $B as $A's upstream only when $A and $B shares the same name, and "git -c push.default=simple" on branch $A would push to update the branch $A at the remote $B came from. Needs review. source: <pull.1161.v2.git.1645815142.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@52b3240. |
|
This patch series was integrated into seen via git@b349ec4. |
TaoK
force-pushed
the
feature-branch-autosetupmerge-simple
branch
from
c16a8fe
to
d5b18c7
Compare
|
User |
|
/submit |
|
Submitted as pull.1161.v3.git.1646032466.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| @@ -886,6 +886,41 @@ test_expect_success 'branch from tag w/--track causes failure' ' | |||
| test_must_fail git branch --track my11 foobar | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Feb 28 2022, Tao Klerks via GitGitGadget wrote:
> From: Tao Klerks <tao@klerks.biz>
>
> In the previous commit a new autosetupmerge option was introduced.
>
> Extend the existing branch tests with three new cases testing this
> option - the obvious matching-name and non-matching-name cases, and
> also a non-matching-ref-type case.
>
> The matching-name case needs to temporarily create an independent
> repo to fetch from, as the general strategy of using the local repo as
> the remote in these tests precludes locally branching with the same
> name as in the "remote".
>
> Signed-off-by: Tao Klerks <tao@klerks.biz>
> ---
> t/t3200-branch.sh | 35 +++++++++++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 7a0ff75ba86..15cc58f1e64 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -886,6 +886,41 @@ test_expect_success 'branch from tag w/--track causes failure' '
> test_must_fail git branch --track my11 foobar
> '
>
> +test_expect_success 'simple tracking works when remote branch name matches' '
> + test_create_repo otherserver &&
> + test_commit -C otherserver my_commit 1 &&
> + git -C otherserver branch feature &&
> + git config branch.autosetupmerge simple &&
> + git config remote.otherserver.url otherserver &&
> + git config remote.otherserver.fetch refs/heads/*:refs/remotes/otherserver/* &&
Shouldn't these use test_config, or if the tests below need them do that
via a helper, so later added tests don't need to reset this state?
> + git fetch otherserver &&
> + git branch feature otherserver/feature &&
> + rm -fr otherserver &&
Instead of "rm -rf" after, do above:
test_when_finished "rm -rf otherserver" &&
git init otherserver
(you don't need "test_create_repo" either, just use "git init")
> + test $(git config branch.feature.remote) = otherserver &&
> + test $(git config branch.feature.merge) = refs/heads/feature
Use:
echo otherserver >expect &&
git config ... >actual &&
test_cmp expect actual
etc., the pattern you're using here will hide git's exit code on
segfaults, abort() etc., and also makes for less useful debug info on
failure than test_cmp.
> +'
> +
> +test_expect_success 'simple tracking skips when remote branch name does not match' '
> + git config branch.autosetupmerge simple &&
> + git config remote.local.url . &&
> + git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
ditto config setup above, this is quite hard to follow in sequence since
yo uneed to reason about all existing config. Let's start with a clean
slate for each test_expect_success and setup the specific config we want
instead.fallow since
> + (git show-ref -q refs/remotes/local/main || git fetch local) &&
This likewise hides segfaults etc. Use:
test_might_fail git show-ref ...
But maybe this whole thing should use "git rev-parse --verify" or
something?
> + git branch my-other local/main &&
> + test -z "$(git config branch.my-other.remote)" &&
> + test -z "$(git config branch.my-other.merge)"
ditto test_cmp comments, but here:
git ... >out &&
test_must_be_empty out
> +'
> +
> +test_expect_success 'simple tracking skips when remote ref is not a branch' '
> + git config branch.autosetupmerge simple &&
> + git tag mytag12 main &&
> + git config remote.localtags.url . &&
> + git config remote.localtags.fetch refs/tags/*:refs/remotes/localtags/* &&
> + (git show-ref -q refs/remotes/localtags/mytag12 || git fetch localtags) &&
> + git branch mytag12 localtags/mytag12 &&
> + test -z "$(git config branch.mytag12.remote)" &&
> + test -z "$(git config branch.mytag12.merge)"
ditto above.
> +'
> +
> test_expect_success '--set-upstream-to fails on multiple branches' '
> echo "fatal: too many arguments to set new upstream" >expect &&
> test_must_fail git branch --set-upstream-to main a b c 2>err &&
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Mon, Feb 28, 2022 at 5:54 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Feb 28 2022, Tao Klerks via GitGitGadget wrote:
> > + test $(git config branch.feature.remote) = otherserver &&
> > + test $(git config branch.feature.merge) = refs/heads/feature
>
> Use:
>
> echo otherserver >expect &&
> git config ... >actual &&
> test_cmp expect actual
>
> etc., the pattern you're using here will hide git's exit code on
> segfaults, abort() etc., and also makes for less useful debug info on
> failure than test_cmp.
Better yet, use test_cmp_config():
test_cmp_config otherserver branch.feature.remote &&
There was a problem hiding this comment.
On the Git mailing list, Tao Klerks wrote (reply to this):
On Mon, Feb 28, 2022 at 10:39 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Mon, Feb 28 2022, Tao Klerks via GitGitGadget wrote:
>
> > +test_expect_success 'simple tracking works when remote branch name matches' '
> > + test_create_repo otherserver &&
> > + test_commit -C otherserver my_commit 1 &&
> > + git -C otherserver branch feature &&
> > + git config branch.autosetupmerge simple &&
> > + git config remote.otherserver.url otherserver &&
> > + git config remote.otherserver.fetch refs/heads/*:refs/remotes/otherserver/* &&
>
> Shouldn't these use test_config, or if the tests below need them do that
> via a helper, so later added tests don't need to reset this state?
Yes, I will look at this; I was naively (and clearly incorrectly)
following a pattern I saw in this same test file.
>
> > + git fetch otherserver &&
> > + git branch feature otherserver/feature &&
> > + rm -fr otherserver &&
>
> Instead of "rm -rf" after, do above:
>
> test_when_finished "rm -rf otherserver" &&
> git init otherserver
>
> (you don't need "test_create_repo" either, just use "git init")
Will do, thx!
>
> > + test $(git config branch.feature.remote) = otherserver &&
> > + test $(git config branch.feature.merge) = refs/heads/feature
>
> Use:
>
> echo otherserver >expect &&
> git config ... >actual &&
> test_cmp expect actual
>
> etc., the pattern you're using here will hide git's exit code on
> segfaults, abort() etc., and also makes for less useful debug info on
> failure than test_cmp.
Again, thank you! (I will look at test_cmp_config() as Eric suggested)
>
>
> > +'
> > +
> > +test_expect_success 'simple tracking skips when remote branch name does not match' '
> > + git config branch.autosetupmerge simple &&
> > + git config remote.local.url . &&
> > + git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
>
> ditto config setup above, this is quite hard to follow in sequence since
> yo uneed to reason about all existing config. Let's start with a clean
> slate for each test_expect_success and setup the specific config we want
> instead.fallow since
>
> > + (git show-ref -q refs/remotes/local/main || git fetch local) &&
>
> This likewise hides segfaults etc. Use:
>
> test_might_fail git show-ref ...
>
> But maybe this whole thing should use "git rev-parse --verify" or
> something?
Honestly, I think this bad pattern is just a premature optimization against
a pretty-fast local fetch. Will simplify, and do the same for existing
patterns in this file.
>
> > + git branch my-other local/main &&
> > + test -z "$(git config branch.my-other.remote)" &&
> > + test -z "$(git config branch.my-other.merge)"
>
> ditto test_cmp comments, but here:
>
> git ... >out &&
> test_must_be_empty out
>
OK, will look, thx.
There was a problem hiding this comment.
On the Git mailing list, Tao Klerks wrote (reply to this):
On Tue, Mar 1, 2022 at 3:59 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Mon, Feb 28, 2022 at 5:54 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > On Mon, Feb 28 2022, Tao Klerks via GitGitGadget wrote:
> > > + test $(git config branch.feature.remote) = otherserver &&
> > > + test $(git config branch.feature.merge) = refs/heads/feature
> >
> > Use:
> >
> > echo otherserver >expect &&
> > git config ... >actual &&
> > test_cmp expect actual
> >
> > etc., the pattern you're using here will hide git's exit code on
> > segfaults, abort() etc., and also makes for less useful debug info on
> > failure than test_cmp.
>
> Better yet, use test_cmp_config():
>
> test_cmp_config otherserver branch.feature.remote &&
Noted, thx.
|
User |
| @@ -9,7 +9,9 @@ branch.autoSetupMerge:: | |||
| automatic setup is done when the starting point is either a | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Mon, Feb 28 2022, Tao Klerks via GitGitGadget wrote:
I think squashing 2/2 inot this would make this much easier to follow,
i.e. to have tests along with the new feature.
> + /*
> + * 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);
This function is the only user of find_tracked_branch(). For e.g. "git
checkout we emit";
fatal: builtin/checkout.c:1246: 'foo' matched multiple (4) remote tracking branches
Perhaps we can do something similar here, and even with some advise()
emit information about what other branches conflicted.
> + 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;
> + }
> +
I wondered when reading this if there isn't a way to merge this and the
"branch_get" call made in "inherit_tracking" earlier in this function in
the "track != BRANCH_TRACK_INHERIT" case.
But maybe not, and that whole API entry point is a bit messy in needing
to cover both the use-case of an existing branch & nonexisting
(i.e. initial creation).
There was a problem hiding this comment.
On the Git mailing list, Tao Klerks wrote (reply to this):
On Mon, Feb 28, 2022 at 11:56 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Mon, Feb 28 2022, Tao Klerks via GitGitGadget wrote:
>
> I think squashing 2/2 inot this would make this much easier to follow,
> i.e. to have tests along with the new feature.
>
OK! Doing.
> > + /*
> > + * 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);
>
> This function is the only user of find_tracked_branch(). For e.g. "git
> checkout we emit";
>
> fatal: builtin/checkout.c:1246: 'foo' matched multiple (4) remote tracking branches
>
> Perhaps we can do something similar here
I'm not sure what you're pointing to specifically - the fact that the
checkout message provides a count? If so I guess I understand/agree,
find_tracked_branch() could be enhanced to keep counting rather than
exiting at the first sign of trouble, to support such a
slightly-more-explicit message here.
I'm not convinced that this situation is common enough to warrant
change: mapping multiple remotes to the same remote-tracking path
seems like a strange setup - is this something we recommend or
document anywhere? maybe to have 2 "remotes" that correspond to the
same server over different protocols appear as one set of tracking
branches?
On the other hand I am of course happy to make things better if we
think this will do that!
> even with some advise()
> emit information about what other branches conflicted.
I believe the conflict is not about different "branches" exactly, but
about *refspecs* that map to the tracking branch.
If I understand correctly this change would entail creating a new
advice type (and documenting it), and figuring out what the advice
should look like - something like "find and disambiguate your fetch
refspecs to enable auto tracking setup! If you want to keep your
ambiguous refspecs, set auto tracking setup to false!" - but nicer :)
>
> > + 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;
> > + }
> > +
>
> I wondered when reading this if there isn't a way to merge this and the
> "branch_get" call made in "inherit_tracking" earlier in this function in
> the "track != BRANCH_TRACK_INHERIT" case.
>
> But maybe not, and that whole API entry point is a bit messy in needing
> to cover both the use-case of an existing branch & nonexisting
> (i.e. initial creation).
Hmm, I had a hard time understanding this comment. I *think* you were
saying "why don't you use an existing API to get the full ref name of the
new local branch, and compare that to the full name of the remote
branch you already have, rather than messing with a "refs/heads/"
prefix explicitly/redundantly"... Is that right?
There was a problem hiding this comment.
On the Git mailing list, Tao Klerks wrote (reply to this):
On Wed, Mar 2, 2022 at 10:35 AM Tao Klerks <tao@klerks.biz> wrote:
>
> On Mon, Feb 28, 2022 at 11:56 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> >
> >
> > This function is the only user of find_tracked_branch(). For e.g. "git
> > checkout we emit";
> >
> > fatal: builtin/checkout.c:1246: 'foo' matched multiple (4) remote tracking branches
> >
> > Perhaps we can do something similar here
>
> I'm not sure what you're pointing to specifically - the fact that the
> checkout message provides a count? If so I guess I understand/agree,
> find_tracked_branch() could be enhanced to keep counting rather than
> exiting at the first sign of trouble, to support such a
> slightly-more-explicit message here.
>
> I'm not convinced that this situation is common enough to warrant
> change: mapping multiple remotes to the same remote-tracking path
> seems like a strange setup - is this something we recommend or
> document anywhere? maybe to have 2 "remotes" that correspond to the
> same server over different protocols appear as one set of tracking
> branches?
>
> On the other hand I am of course happy to make things better if we
> think this will do that!
Having finally understood the logic in play here, I now see that
find_tracked_branch() does not "exit at the first sign of trouble" as
I thought, so there isn't much change required to produce a marginally
richer error message here, but I've decided to work on this proposed
enhancement in a separate patch. The more I look at this, the less
confident I am about exactly the right thing to do - and I'd rather
not hold up the (in my opinion) net-good branch.autosetupmerge=simple
work.
The specific concern I have is about changing the "fatal: Not
tracking: ambiguous information for ref refs/remotes/origin/master"
message. Having understood when it can occur, I've realized it is
probably quite common - I at least have certainly seen it a few times,
as the situation it describes is what happens if you copy/paste a
"remote" section in your git config file, to create a new remote with
the same setup as an existing one, without remembering to adjust the
refspec for the new remote name.
> > even with some advise()
> > emit information about what other branches conflicted.
>
> I believe the conflict is not about different "branches" exactly, but
> about *refspecs* that map to the tracking branch.
>
> If I understand correctly this change would entail creating a new
> advice type (and documenting it), and figuring out what the advice
> should look like - something like "find and disambiguate your fetch
> refspecs to enable auto tracking setup! If you want to keep your
> ambiguous refspecs, set auto tracking setup to false!" - but nicer :)
In addition to the mechanics of creating a new advice type, I
eventually realized that the right message would list the *remotes*
that have refspecs mapping to the same tracking ref - which would mean
newly tracking those in the per-remote find_tracked_branch() looping.
I initially thought this situation was too rare to warrant this kind
of change, but now, understanding how I myself have reached this
situation a few times *and it took me a while to understand what I did
wrong* (at least the first time), I think it's worthwhile work in and
of itself.
Expect a new separate patchset sometime.
|
This patch series was integrated into seen via git@e904f8c. |
|
This patch series was integrated into seen via git@7d304ba. |
|
User |
|
This patch series was integrated into seen via git@e6076a0. |
|
There was a status update in the "Cooking" section about the branch "git -c branch.autosetupmerge=simple branch $A $B" will set the $B as $A's upstream only when $A and $B shares the same name, and "git -c push.default=simple" on branch $A would push to update the branch $A at the remote $B came from. Also more places use the sole remote, if exists, before defaulting to 'origin'. source: <pull.1161.v5.git.1651226206.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@7db8268. |
|
This patch series was integrated into seen via git@e8f8d4f. |
|
There was a status update in the "Cooking" section about the branch "git -c branch.autosetupmerge=simple branch $A $B" will set the $B as $A's upstream only when $A and $B shares the same name, and "git -c push.default=simple" on branch $A would push to update the branch $A at the remote $B came from. Also more places use the sole remote, if exists, before defaulting to 'origin'. source: <pull.1161.v5.git.1651226206.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@72c2b20. |
|
This patch series was integrated into seen via git@8c08c81. |
|
This patch series was integrated into seen via git@ebca13a. |
|
There was a status update in the "Cooking" section about the branch "git -c branch.autosetupmerge=simple branch $A $B" will set the $B as $A's upstream only when $A and $B shares the same name, and "git -c push.default=simple" on branch $A would push to update the branch $A at the remote $B came from. Also more places use the sole remote, if exists, before defaulting to 'origin'. Thoughts? source: <pull.1161.v5.git.1651226206.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@ad6d1a3. |
|
This patch series was integrated into seen via git@16172fb. |
|
There was a status update in the "Cooking" section about the branch "git -c branch.autosetupmerge=simple branch $A $B" will set the $B as $A's upstream only when $A and $B shares the same name, and "git -c push.default=simple" on branch $A would push to update the branch $A at the remote $B came from. Also more places use the sole remote, if exists, before defaulting to 'origin'. Will merge to 'next'? source: <pull.1161.v5.git.1651226206.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@ccd2353. |
|
This patch series was integrated into seen via git@ad7b596. |
|
This patch series was integrated into seen via git@14f75fb. |
|
This patch series was integrated into seen via git@d19c015. |
|
This patch series was integrated into next via git@9666852. |
|
This patch series was integrated into seen via git@4e8197a. |
|
There was a status update in the "Cooking" section about the branch "git -c branch.autosetupmerge=simple branch $A $B" will set the $B as $A's upstream only when $A and $B shares the same name, and "git -c push.default=simple" on branch $A would push to update the branch $A at the remote $B came from. Also more places use the sole remote, if exists, before defaulting to 'origin'. Will merge to 'master'. source: <pull.1161.v5.git.1651226206.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@0391a84. |
|
This patch series was integrated into seen via git@97c7e35. |
|
This patch series was integrated into seen via git@b79c0d6. |
|
There was a status update in the "Cooking" section about the branch "git -c branch.autosetupmerge=simple branch $A $B" will set the $B as $A's upstream only when $A and $B shares the same name, and "git -c push.default=simple" on branch $A would push to update the branch $A at the remote $B came from. Also more places use the sole remote, if exists, before defaulting to 'origin'. Will merge to 'master'. source: <pull.1161.v5.git.1651226206.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@f49c478. |
|
This patch series was integrated into master via git@f49c478. |
|
This patch series was integrated into next via git@f49c478. |
|
Closed via f49c478. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patchset introduces two new configuration options, intended to be consistent with and complementary to the push.default "simple" option. It also improves remote-defaulting in "default push" scenarios.
In some "simple" centralized workflows, users expect remote tracking branch names to match local branch names. "git push" pushes to the remote version/instance of the branch, and "git pull" pulls any changes to the remote branch (changes made by the same user in another place, or by other users). The default push.default option, "simple", supports this kind of workflow by "raising eyebrows" if a user looks like they are trying to push to the "wrong" remote tracking branch.
None of the existing branch.autosetupmerge settings support this workflow/expectation well, so the new "branch.autosetupmerge=simple" option addresses this - acting like the default "remote" option only when the remote branch and (new) local branch have the same name. The new option is referred to in new advice in the push.default=simple mismatching remote branch error text.
At a later stage in the new-branch workflow, when a user first goes to push a new branch to the remote, the default "git push" will complain that there is no remote tracking branch (unless push.default=current). For users that always expect remote branch names to match local branch names, on a single remote, this is inconvenient. New config setting "push.autoSetupRemote" addresses this by automatically specifying "--set-upstream" (and allowing the push) when there is no configured remote for push.default options "simple", "upstream", and "current". In the case of "current", this helps make "pull" work correctly (under these workflow assumptions). For the other two options the primary benefit is being able to simply say "git push" and not be interrupted with an unnecessary "but this is a new branch!" error.
Along the way, we also enhance the remote-defaulting behavior for "git push" (and ls-remote) to not only support "origin" as the default remote, but rather any single configured remote. Default push should only fail for lack of a remote if there are none, or if there is more than one and none are called "origin".
Changes since v4:
Open questions:
cc: Tao Klerks tao@klerks.biz
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Eric Sunshine sunshine@sunshineco.com
cc: Josh Steadmon steadmon@google.com