Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New options to support "simple" centralized workflow #1161

Closed
wants to merge 3 commits into from
Closed

New options to support "simple" centralized workflow #1161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5b08edc
branch: new autosetupmerge option 'simple' for matching branches
TaoK Feb 21, 2022
31184c3
push: default to single remote even when not named origin
TaoK Apr 29, 2022
41c88e5
push: new config option "push.autoSetupRemote" supports "simple" push
TaoK Apr 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions Documentation/config/branch.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ branch.autoSetupMerge::
automatic setup is done when the starting point is either a
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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'
Expand Down Expand Up @@ -38,8 +40,9 @@ branch.<name>.remote::
may be overridden with `remote.pushDefault` (for all branches).
The remote to push to, for the current branch, may be further
overridden by `branch.<name>.pushRemote`. If no remote is
configured, or if you are not on any branch, it defaults to
`origin` for fetching and `remote.pushDefault` for pushing.
configured, or if you are not on any branch and there is more than
one remote defined in the repository, it defaults to `origin` for
fetching and `remote.pushDefault` for pushing.
Additionally, `.` (a period) is the current local repository
(a dot-repository), see `branch.<name>.merge`'s final note below.

Expand Down
11 changes: 11 additions & 0 deletions Documentation/config/push.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
push.autoSetupRemote::
If set to "true" assume `--set-upstream` on default push when no
upstream tracking exists for the current branch; this option
takes effect with push.default options 'simple', 'upstream',
and 'current'. It is useful if by default you want new branches
to be pushed to the default remote (like the behavior of
'push.default=current') and you also want the upstream tracking
to be set. Workflows most likely to benefit from this option are
'simple' central workflows where all branches are expected to
have the same name on the remote.

push.default::
Defines the action `git push` should take if no refspec is
given (whether from the command-line, config, or elsewhere).
Expand Down
18 changes: 11 additions & 7 deletions Documentation/git-branch.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
27 changes: 26 additions & 1 deletion branch.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ static int find_tracked_branch(struct remote *remote, void *priv)
string_list_clear(tracking->srcs, 0);
break;
}
/* remote_find_tracking() searches by src if present */
tracking->spec.src = NULL;
}

return 0;
}

Expand Down Expand Up @@ -264,15 +264,23 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,

if (!tracking.matches)
switch (track) {
/* If ref is not remote, still use local */
case BRANCH_TRACK_ALWAYS:
case BRANCH_TRACK_EXPLICIT:
case BRANCH_TRACK_OVERRIDE:
/* Remote matches not evaluated */
case BRANCH_TRACK_INHERIT:
break;
/* Otherwise, if no remote don't track */
default:
goto cleanup;
}

/*
* This check does not apply to BRANCH_TRACK_INHERIT;
* that supports multiple entries in tracking_srcs but
* leaves tracking.matches at 0.
*/
if (tracking.matches > 1) {
int status = die_message(_("not tracking: ambiguous information for ref '%s'"),
orig_ref);
Expand Down Expand Up @@ -307,6 +315,21 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
exit(status);
}

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 only BRANCH_TRACK_INHERIT can
* produce more than one entry).
*/
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,
Expand Down Expand Up @@ -603,6 +626,8 @@ static int submodule_create_branch(struct repository *r,
/* Default for "git checkout". Do not pass --track. */
case BRANCH_TRACK_REMOTE:
/* Default for "git branch". Do not pass --track. */
case BRANCH_TRACK_SIMPLE:
/* Config-driven only. Do not pass --track. */
break;
}

Expand Down
1 change: 1 addition & 0 deletions branch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
64 changes: 51 additions & 13 deletions builtin/push.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* "git push"
*/
#include "cache.h"
#include "branch.h"
#include "config.h"
#include "refs.h"
#include "refspec.h"
Expand Down Expand Up @@ -151,7 +152,8 @@ static NORETURN void die_push_simple(struct branch *branch,
* upstream to a non-branch, we should probably be showing
* them the big ugly fully qualified ref.
*/
const char *advice_maybe = "";
const char *advice_pushdefault_maybe = "";
const char *advice_automergesimple_maybe = "";
const char *short_upstream = branch->merge[0]->src;

skip_prefix(short_upstream, "refs/heads/", &short_upstream);
Expand All @@ -161,9 +163,16 @@ static NORETURN void die_push_simple(struct branch *branch,
* push.default.
*/
if (push_default == PUSH_DEFAULT_UNSPECIFIED)
advice_maybe = _("\n"
advice_pushdefault_maybe = _("\n"
"To choose either option permanently, "
"see push.default in 'git help config'.");
"see push.default in 'git help config'.\n");
if (git_branch_track != BRANCH_TRACK_SIMPLE)
advice_automergesimple_maybe = _("\n"
"To avoid automatically configuring "
"upstream branches when their name\n"
"doesn't match the local branch, see option "
"'simple' of branch.autosetupmerge\n"
"in 'git help config'.\n");
die(_("The upstream branch of your current branch does not match\n"
"the name of your current branch. To push to the upstream branch\n"
"on the remote, use\n"
Expand All @@ -173,9 +182,10 @@ static NORETURN void die_push_simple(struct branch *branch,
"To push to the branch of the same name on the remote, use\n"
"\n"
" git push %s HEAD\n"
"%s"),
"%s%s"),
remote->name, short_upstream,
remote->name, advice_maybe);
remote->name, advice_pushdefault_maybe,
advice_automergesimple_maybe);
}

static const char message_detached_head_die[] =
Expand All @@ -185,24 +195,40 @@ static const char message_detached_head_die[] =
"\n"
" git push %s HEAD:<name-of-remote-branch>\n");

static const char *get_upstream_ref(struct branch *branch, const char *remote_name)
static const char *get_upstream_ref(int flags, struct branch *branch, const char *remote_name)
{
if (!branch->merge_nr || !branch->merge || !branch->remote_name)
if (branch->merge_nr == 0 && (flags & TRANSPORT_PUSH_AUTO_UPSTREAM)) {
/* if missing, assume same; set_upstream will be defined later */
return branch->refname;
}

if (!branch->merge_nr || !branch->merge || !branch->remote_name) {
const char *advice_autosetup_maybe = "";
if (!(flags & TRANSPORT_PUSH_AUTO_UPSTREAM)) {
advice_autosetup_maybe = _("\n"
"To have this happen automatically for "
"branches without a tracking\n"
"upstream, see 'push.autoSetupRemote' "
"in 'git help config'.\n");
}
die(_("The current branch %s has no upstream branch.\n"
"To push the current branch and set the remote as upstream, use\n"
"\n"
" git push --set-upstream %s %s\n"),
" git push --set-upstream %s %s\n"
"%s"),
branch->name,
remote_name,
branch->name);
branch->name,
advice_autosetup_maybe);
}
if (branch->merge_nr != 1)
die(_("The current branch %s has multiple upstream branches, "
"refusing to push."), branch->name);

return branch->merge[0]->src;
}

static void setup_default_push_refspecs(struct remote *remote)
static void setup_default_push_refspecs(int *flags, struct remote *remote)
{
struct branch *branch;
const char *dst;
Expand Down Expand Up @@ -234,7 +260,7 @@ static void setup_default_push_refspecs(struct remote *remote)
case PUSH_DEFAULT_SIMPLE:
if (!same_remote)
break;
if (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
if (strcmp(branch->refname, get_upstream_ref(*flags, branch, remote->name)))
die_push_simple(branch, remote);
break;

Expand All @@ -244,13 +270,21 @@ static void setup_default_push_refspecs(struct remote *remote)
"your current branch '%s', without telling me what to push\n"
"to update which remote branch."),
remote->name, branch->name);
dst = get_upstream_ref(branch, remote->name);
dst = get_upstream_ref(*flags, branch, remote->name);
break;

case PUSH_DEFAULT_CURRENT:
break;
}

/*
* this is a default push - if auto-upstream is enabled and there is
* no upstream defined, then set it (with options 'simple', 'upstream',
* and 'current').
*/
if ((*flags & TRANSPORT_PUSH_AUTO_UPSTREAM) && branch->merge_nr == 0)
*flags |= TRANSPORT_PUSH_SET_UPSTREAM;

refspec_appendf(&rs, "%s:%s", branch->refname, dst);
}

Expand Down Expand Up @@ -401,7 +435,7 @@ static int do_push(int flags,
if (remote->push.nr) {
push_refspec = &remote->push;
} else if (!(flags & TRANSPORT_PUSH_MIRROR))
setup_default_push_refspecs(remote);
setup_default_push_refspecs(&flags, remote);
}
errs = 0;
url_nr = push_url_of_remote(remote, &url);
Expand Down Expand Up @@ -472,6 +506,10 @@ static int git_push_config(const char *k, const char *v, void *cb)
else
*flags &= ~TRANSPORT_PUSH_FOLLOW_TAGS;
return 0;
} else if (!strcmp(k, "push.autosetupremote")) {
if (git_config_bool(k, v))
*flags |= TRANSPORT_PUSH_AUTO_UPSTREAM;
return 0;
} else if (!strcmp(k, "push.gpgsign")) {
const char *value;
if (!git_config_get_value("push.gpgsign", &value)) {
Expand Down
3 changes: 3 additions & 0 deletions config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1781,6 +1781,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;
Expand Down
2 changes: 2 additions & 0 deletions remote.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,8 @@ static const char *remotes_remote_for_branch(struct remote_state *remote_state,
}
if (explicit)
*explicit = 0;
if (remote_state->remotes_nr == 1)
return remote_state->remotes[0]->name;
return "origin";
}

Expand Down
35 changes: 35 additions & 0 deletions t/t3200-branch.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -886,6 +886,41 @@ test_expect_success 'branch from tag w/--track causes failure' '
test_must_fail git branch --track my11 foobar
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 &&

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 &&

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'

test_expect_success 'simple tracking works when remote branch name matches' '
test_when_finished "rm -rf otherserver" &&
git init otherserver &&
test_commit -C otherserver my_commit 1 &&
git -C otherserver branch feature &&
test_config branch.autosetupmerge simple &&
test_config remote.otherserver.url otherserver &&
test_config remote.otherserver.fetch refs/heads/*:refs/remotes/otherserver/* &&
git fetch otherserver &&
git branch feature otherserver/feature &&
test_cmp_config otherserver branch.feature.remote &&
test_cmp_config refs/heads/feature branch.feature.merge
'

test_expect_success 'simple tracking skips when remote branch name does not match' '
test_config branch.autosetupmerge simple &&
test_config remote.local.url . &&
test_config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
git fetch local &&
git branch my-other local/main &&
test_cmp_config "" --default "" branch.my-other.remote &&
test_cmp_config "" --default "" branch.my-other.merge
'

test_expect_success 'simple tracking skips when remote ref is not a branch' '
test_config branch.autosetupmerge simple &&
test_config remote.localtags.url . &&
test_config remote.localtags.fetch refs/tags/*:refs/remotes/localtags/* &&
git tag mytag12 main &&
git fetch localtags &&
git branch mytag12 localtags/mytag12 &&
test_cmp_config "" --default "" branch.mytag12.remote &&
test_cmp_config "" --default "" branch.mytag12.merge
'

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 &&
Expand Down
17 changes: 14 additions & 3 deletions t/t5512-ls-remote.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ generate_references () {
done
}

test_expect_success 'dies when no remote found' '
test_must_fail git ls-remote
'

test_expect_success setup '
>file &&
git add file &&
Expand All @@ -30,7 +34,8 @@ test_expect_success setup '
git show-ref -d >refs &&
sed -e "s/ / /" refs >>expected.all &&

git remote add self "$(pwd)/.git"
git remote add self "$(pwd)/.git" &&
git remote add self2 "."
'

test_expect_success 'ls-remote --tags .git' '
Expand Down Expand Up @@ -83,11 +88,17 @@ test_expect_success 'ls-remote --sort="-refname" --tags self' '
test_cmp expect actual
'

test_expect_success 'dies when no remote specified and no default remotes found' '
test_expect_success 'dies when no remote specified, multiple remotes found, and no default specified' '
test_must_fail git ls-remote
'

test_expect_success 'use "origin" when no remote specified' '
test_expect_success 'succeeds when no remote specified but only one found' '
test_when_finished git remote add self2 "." &&
git remote remove self2 &&
git ls-remote
'

test_expect_success 'use "origin" when no remote specified and multiple found' '
URL="$(pwd)/.git" &&
echo "From $URL" >exp_err &&

Expand Down