Re: [PATCH v2 1/2] merge: new autosetupmerge option 'simple' for matching branches

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 25, 2022, 20:15 UTC
Message-ID: <xmqqczjaaeiv.fsf@gitster.g>
In-Reply-To: <890e016bfc0809d25a4ae8ae924b23895f520810.1645815142.git.gitgitgadget@gmail.com>

"Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 8 quoted lines

> +	/*
> +	 * 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);

Show 7 quoted lines

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

Show 6 quoted lines

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