Re: [PATCH] branch: optionally setup branch.*.merge from upstream local branches

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Jay Soffian <jaysoffian@xxxxxxxxx> writes:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index fb6dae0..f15fe0a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -378,11 +378,14 @@ apply.whitespace::
>  	as the '--whitespace' option. See linkgit:git-apply[1].
>  
>  branch.autosetupmerge::
> ...
> +	Tells `git-branch` and `git-checkout` to setup `branch.*.remote`
> +	and `branch.*.merge` for new branches so that linkgit:git-pull[1]
> +	will appropriately merge from that upstream branch. Note that even
> +	if this option is not set, this behavior can be chosen per-branch
> +	using the `--track`	and `--no-track` options. This option defaults

You have an unintended tab there.

> +	to true, which will setup tracking for remote branches. Set it to
> +	`always` to automatically setup the aforementioned options for local
> +	upstream branches as well.

I somehow find it easier to read if the default is always
described at the end of the paragraph, i.e. "Setting this to
value X does foo, Y does bar, Z does baz. Defaults to X.", but
that may be just me.

> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 7e8874a..ce2fc64 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -105,20 +105,20 @@ OPTIONS
>  	Display the full sha1s in output listing rather than abbreviating them.
>  
>  --track::
> ...
> +	Set up configuration so that git-pull will
> +	automatically retrieve data from the upstream branch.  Use this if
> +	you always pull from the same upstream branch into the new branch,
> +	or if you don't want to use "git pull <repository> <refspec>"
> +	explicitly. This behavior is the default for remote branches.
> +	Set the branch.autosetupmerge configuration variable to false if you
> +	want git-checkout and git-branch to always behave as if '--no-track'
> +	were given. Set it to 'always' if you want git-checkout and git-branch
> +	to always behave as if '--track' were given.

Re-wrapping the
paragraph at an unusual place.  It is Ok to do so if it is done
to keep the unchanged part intact to minimize the diff text, but
otherwise, please don't.

You reworded "remote branch" to "upstream branch" here and other
places.  I think that makes the intention easier to understand.
It is "the branch this new branch intends to build on top of",
so the earlier text should have said "remote tracking branch"
but the intention has always been to mean "on top of whom will I
be building".

This is not a flaw in your patch, but I suspect "or if you
don't want to" should have been "and if you don't want to".

"This behaviour is the default for remote branches" feels a bit
odd.  It confuses the first time reader as if you are saying "if
you are creating a remote branch with "git-branch" command, this
behaviour is the default", which is not what you are saying.  I
think "... the default when forking off of a remote tracking
branch" is what you meant.

>  --no-track::
> -	When a branch is created off a remote branch,
> -	set up configuration so that git-pull will not retrieve data
> -	from the remote branch, ignoring the branch.autosetupmerge
> -	configuration variable.
> +	Ignore the branch.autosetupmerge configuration variable. When
> +	using git pull with the new branch, a <refspec> will have to be
> +	given explicitly.

This rewrite probably has made the description easier to understand.

> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> index b4cfa04..ad2ab51 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -48,21 +48,20 @@ OPTIONS
>  	may restrict the characters allowed in a branch name.
>  
>  --track::
> ...
> +	When -b is given set up configuration so that git-pull will

s/given/&,/;

> +	automatically retrieve data from the upstream branch.  Use this if
> +	you always pull from the same upstream branch into the new branch,
> +	or if you don't want to use "git pull <repository> <refspec>"
> +	explicitly. This behavior is the default for remote branches.

The same comment on "or if you don't want to" applies.

> @@ -49,8 +49,12 @@ static int setup_tracking(const char *new_ref, const char *orig_ref)
>  	memset(&tracking, 0, sizeof(tracking));
>  	tracking.spec.dst = (char *)orig_ref;
>  	if (for_each_remote(find_tracked_branch, &tracking) ||
> -			!tracking.matches)
> -		return 1;
> +			!tracking.matches) {
> +		if (!always)
> +			return 1;
> +		tracking.matches = 1;
> +		tracking.src = xstrdup(orig_ref);
> +	}

I suspect this is a clever fallback to pretend as if you have
[remote "."] fetch=refs/*:refs/* when nothing else matches, but
I think it needs an explanation why this is a good idea and
would not break others.  Also I suspect this should be moved
after you see for-each-remote returns without error and did not
find any match.

> @@ -71,7 +76,7 @@ static int setup_tracking(const char *new_ref, const char *orig_ref)
>  
>  void create_branch(const char *head,
>  		   const char *name, const char *start_name,
> -		   int force, int reflog, int track)
> +		   int force, int reflog, enum branch_track track)
>  {
>  	struct ref_lock *lock;
>  	struct commit *commit;
> @@ -130,7 +135,7 @@ void create_branch(const char *head,
>  	   automatically merges from there.  So far, this is only done for
>  	   remotes registered via .git/config.  */
>  	if (real_ref && track)
> -		setup_tracking(name, real_ref);
> +		setup_tracking(name, real_ref, (track == BRANCH_TRACK_ALWAYS));

It probably is better to pass 'track' itself and have the
special case logic inside setup_tracking() instead.  Maybe we
would want to add new tracking mode that needs different
matching logic there later.

The move to default_config() is probably a good idea.

> diff --git a/cache.h b/cache.h
> index 2b59c44..90b3a9e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -393,6 +393,14 @@ enum safe_crlf {
>  
>  extern enum safe_crlf safe_crlf;
>  
> +enum branch_track {
> +	BRANCH_TRACK_REMOTES_FALSE = 0,
> +	BRANCH_TRACK_REMOTES_TRUE,
> +	BRANCH_TRACK_ALWAYS,
> +};

Perhaps BRANCH_TRACK_{NEVER,REMOTE,ALWAYS} would be easier
(although I cannot decide if s/NEVER/FALSE/ would be better)?
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux