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

[Junio C Hamano <gitster@pobox.com>]

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