pull, fetch: add --set-upstream option
diff mbox series

Message ID 20190814134629.21096-1-git@matthieu-moy.fr
State New
Headers show
Series
  • pull, fetch: add --set-upstream option
Related show

Commit Message

Matthieu Moy Aug. 14, 2019, 1:46 p.m. UTC
From: Corentin BOMPARD <corentin.bompard@etu.univ-lyon1.fr>

Add the --set-upstream option to git pull/fetch
which lets the user set the upstream configuration
(branch.<current-branch-name>.merge and
branch.<current-branch-name>.remote) for the current branch.

A typical use-case is:

    git clone http://example.com/my-public-fork
    git remote add main http://example.com/project-main-repo
    git pull --set-upstream main master

or, instead of the last line:

    git fetch --set-upstream main master
    git merge # or git rebase

This functionality is analog to push --set-upstream.

Signed-off-by: Corentin BOMPARD <corentin.bompard@etu.univ-lyon1.fr>
Signed-off-by: Nathan BERBEZIER <nathan.berbezier@etu.univ-lyon1.fr>
Signed-off-by: Pablo CHABANNE <pablo.chabanne@etu.univ-lyon1.fr>
Signed-off-by: Matthieu Moy <git@matthieu-moy.fr>
Patch-edited-by: Matthieu Moy <git@matthieu-moy.fr>
---
This is a followup on
https://public-inbox.org/git/86zhoil3yw.fsf@univ-lyon1.fr/. It's
initially a student project, but students didn't get time to complete
it. Still, I think the feature is interesting, and I finally get time
to fix the remarks made up to now. This now looks good to me, but
obviously needs other pairs of eyes.

Thanks,

 Documentation/fetch-options.txt |   7 ++
 builtin/fetch.c                 |  48 ++++++++-
 builtin/pull.c                  |   6 ++
 t/t5553-set-upstream.sh         | 178 ++++++++++++++++++++++++++++++++
 4 files changed, 238 insertions(+), 1 deletion(-)
 create mode 100755 t/t5553-set-upstream.sh

Comments

Pratyush Yadav Aug. 14, 2019, 5:14 p.m. UTC | #1
Hi Matthieu,

This is not really a review. Just some minor nitpicks I spotted while 
reading through.

On 14/08/19 03:46PM, Matthieu Moy wrote:
> From: Corentin BOMPARD <corentin.bompard@etu.univ-lyon1.fr>
> 
> Add the --set-upstream option to git pull/fetch
> which lets the user set the upstream configuration
> (branch.<current-branch-name>.merge and
> branch.<current-branch-name>.remote) for the current branch.
> 
> A typical use-case is:
> 
>     git clone http://example.com/my-public-fork
>     git remote add main http://example.com/project-main-repo
>     git pull --set-upstream main master
> 
> or, instead of the last line:
> 
>     git fetch --set-upstream main master
>     git merge # or git rebase
> 
> This functionality is analog to push --set-upstream.
> 
> Signed-off-by: Corentin BOMPARD <corentin.bompard@etu.univ-lyon1.fr>
> Signed-off-by: Nathan BERBEZIER <nathan.berbezier@etu.univ-lyon1.fr>
> Signed-off-by: Pablo CHABANNE <pablo.chabanne@etu.univ-lyon1.fr>
> Signed-off-by: Matthieu Moy <git@matthieu-moy.fr>
> Patch-edited-by: Matthieu Moy <git@matthieu-moy.fr>
> ---
> This is a followup on
> https://public-inbox.org/git/86zhoil3yw.fsf@univ-lyon1.fr/. It's
> initially a student project, but students didn't get time to complete
> it. Still, I think the feature is interesting, and I finally get time
> to fix the remarks made up to now. This now looks good to me, but
> obviously needs other pairs of eyes.
> 
> Thanks,
> 
>  Documentation/fetch-options.txt |   7 ++
>  builtin/fetch.c                 |  48 ++++++++-
>  builtin/pull.c                  |   6 ++
>  t/t5553-set-upstream.sh         | 178 ++++++++++++++++++++++++++++++++
>  4 files changed, 238 insertions(+), 1 deletion(-)
>  create mode 100755 t/t5553-set-upstream.sh
> 
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 3c9b4f9e09..99df1f3d4e 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -169,6 +169,13 @@ ifndef::git-pull[]
>  	Disable recursive fetching of submodules (this has the same effect as
>  	using the `--recurse-submodules=no` option).
>  
> +--set-upstream::
> +	If the remote is fetched successfully, pull and add upstream
> +	(tracking) reference, used by argument-less
> +	linkgit:git-pull[1] and other commands. For more information,
> +	see `branch.<name>.merge` and `branch.<name>.remote` in
> +	linkgit:git-config[1].
> +
>  --submodule-prefix=<path>::
>  	Prepend <path> to paths printed in informative messages
>  	such as "Fetching submodule foo".  This option is used
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 717dd14e89..5557ae1c04 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -23,6 +23,7 @@
>  #include "packfile.h"
>  #include "list-objects-filter-options.h"
>  #include "commit-reach.h"
> +#include "branch.h"
>  
>  #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
>  
> @@ -50,7 +51,7 @@ static int fetch_prune_tags_config = -1; /* unspecified */
>  static int prune_tags = -1; /* unspecified */
>  #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */
>  
> -static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity, deepen_relative;
> +static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity, deepen_relative, set_upstream;

This line is getting pretty long. I think it is a good idea to split it 
into two.

>  static int progress = -1;
>  static int enable_auto_gc = 1;
>  static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
> @@ -123,6 +124,8 @@ static struct option builtin_fetch_options[] = {
>  	OPT__VERBOSITY(&verbosity),
>  	OPT_BOOL(0, "all", &all,
>  		 N_("fetch from all remotes")),
> +	OPT_BOOL(0, "set-upstream", &set_upstream,
> +		 N_("set upstream for git pull/fetch")),
>  	OPT_BOOL('a', "append", &append,
>  		 N_("append to .git/FETCH_HEAD instead of overwriting")),
>  	OPT_STRING(0, "upload-pack", &upload_pack, N_("path"),
> @@ -1367,6 +1370,49 @@ static int do_fetch(struct transport *transport,
>  		retcode = 1;
>  		goto cleanup;
>  	}
> +
> +	if (set_upstream) {
> +		struct branch *branch = branch_get("HEAD");
> +		struct ref *rm;
> +		struct ref *source_ref = NULL;
> +
> +		/*
> +		 * We're setting the upstream configuration for the current branch. The
> +		 * relevent upstream is the fetched branch that is meant to be merged with
> +		 * the current one, i.e. the one fetched to FETCH_HEAD.
> +		 *
> +		 * When there are several such branches, consider the request ambiguous and
> +		 * err on the safe side by doing nothing and just emit a warning.
> +		 */

The comment lines cross the 80 column boundary. The usual convention in 
this project is to try to keep lines below 80 columns. For strings IMO 
an exception can be allowed because breaking them up makes it harder to 
grep for them. But comments are the easiest to format.

Are you using a tab size of 4? That might explain why your line breaks 
are just after the 80 col boundary. The coding guidelines say you should 
make your tab characters 8 columns wide.

> +		for (rm = ref_map; rm; rm = rm->next) {
> +			if (!rm->peer_ref) {
> +				if (source_ref) {
> +					warning(_("multiple branch detected, incompatible with --set-upstream"));
> +					goto skip;
> +				} else {
> +					source_ref = rm;
> +				}
> +			}
> +		}
> +		if (source_ref) {
> +			if (!strcmp(source_ref->name, "HEAD") || 

This line has a trailing space.

> +				starts_with(source_ref->name, "refs/heads/")) {
> +				install_branch_config(0, branch->name,
> +							 transport->remote->name,
> +							 source_ref->name);

In other places around this code, multi line function calls are aligned 
with the opening parenthesis. It is a good idea to follow that 
convention.

So this should change to something like:

				install_branch_config(0, branch->name,
						      transport->remote->name,
						      source_ref->name);
 
Maybe this discrepancy is because you are using the wrong tab size?

> +			} else if (starts_with(source_ref->name, "refs/remotes/")) {
> +				warning(_("not setting upstream for a remote remote-tracking branch"));
> +			} else if (starts_with(source_ref->name, "refs/tags/")) {
> +				warning(_("not setting upstream for a remote tag"));
> +			} else {
> +				warning(_("unknown branch type"));
> +			}

No need to wrap single line if statements in braces.

> +		} else {
> +			warning(_("no source branch found.\n"
> +				"you need to specify exactly one branch with the --set-upstream option."));
> +		}
> +	}
> + skip:
>  	free_refs(ref_map);
>  
>  	/* if neither --no-tags nor --tags was specified, do automated tag
> diff --git a/builtin/pull.c b/builtin/pull.c
> index f1eaf6e6ed..d25ff13a60 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -129,6 +129,7 @@ static char *opt_refmap;
>  static char *opt_ipv4;
>  static char *opt_ipv6;
>  static int opt_show_forced_updates = -1;
> +static char *set_upstream;
>  
>  static struct option pull_options[] = {
>  	/* Shared options */
> @@ -243,6 +244,9 @@ static struct option pull_options[] = {
>  		PARSE_OPT_NOARG),
>  	OPT_BOOL(0, "show-forced-updates", &opt_show_forced_updates,
>  		 N_("check for forced-updates on all updated branches")),
> +	OPT_PASSTHRU(0, "set-upstream", &set_upstream, NULL,
> +		N_("set upstream for git pull/fetch"),
> +		PARSE_OPT_NOARG),
>  
>  	OPT_END()
>  };
> @@ -556,6 +560,8 @@ static int run_fetch(const char *repo, const char **refspecs)
>  		argv_array_push(&args, "--show-forced-updates");
>  	else if (opt_show_forced_updates == 0)
>  		argv_array_push(&args, "--no-show-forced-updates");
> +	if (set_upstream)
> +		argv_array_push(&args, set_upstream);
>  
>  	if (repo) {
>  		argv_array_push(&args, repo);
> diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
> new file mode 100755
> index 0000000000..bd1a94f494
> --- /dev/null
> +++ b/t/t5553-set-upstream.sh
> @@ -0,0 +1,178 @@
> +#!/bin/sh
> +
> +test_description='"git fetch/pull --set-upstream" basic tests.'
> +. ./test-lib.sh
> +
> +check_config () {
> +	printf "%s\n" "$2" "$3" >"expect.$1" &&
> +	{
> +		git config "branch.$1.remote" && git config "branch.$1.merge"
> +	} >"actual.$1" &&
> +	test_cmp "expect.$1" "actual.$1"
> +}
> +
> +check_config_missing () {
> +	test_expect_code 1 git config "branch.$1.remote" &&
> +	test_expect_code 1 git config "branch.$1.merge"
> +}
> +
> +clear_config () {
> +	for branch in "$@"; do
> +		test_might_fail git config --unset-all "branch.$branch.remote"
> +		test_might_fail git config --unset-all "branch.$branch.merge"
> +	done
> +}
> +
> +ensure_fresh_upstream () {
> +	rm -rf parent && git init --bare parent
> +}
> +
> +test_expect_success 'setup bare parent fetch' '
> +	ensure_fresh_upstream &&
> +	git remote add upstream parent
> +'
> +
> +test_expect_success 'setup commit on master and other fetch' '
> +	test_commit one &&
> +	git push upstream master &&
> +	git checkout -b other &&
> +	test_commit two &&
> +	git push upstream other
> +'
> +
> +#tests for fetch --set-upstream

Add a space after the '#'. Same in other comments below.

> +
> +test_expect_success 'fetch --set-upstream does not set upstream w/o branch' '
> +	clear_config master other &&
> +	git checkout master &&
> +	git fetch --set-upstream upstream &&
> +	check_config_missing master &&
> +	check_config_missing other
> +'
> +
> +test_expect_success 'fetch --set-upstream upstream master sets branch master but not other' '
> +	clear_config master other &&
> +	git fetch --set-upstream upstream master &&
> +	check_config master upstream refs/heads/master &&
> +	check_config_missing other
> +'
> +
> +test_expect_success 'fetch --set-upstream upstream other sets branch other' '
> +	clear_config master other &&
> +	git fetch --set-upstream upstream other &&
> +	check_config master upstream refs/heads/other &&
> +	check_config_missing other
> +'
> +
> +test_expect_success 'fetch --set-upstream master:other does not set the branch other2' '
> +	clear_config other2 &&
> +	git fetch --set-upstream upstream master:other2 &&
> +	check_config_missing other2
> +'
> +
> +test_expect_success 'fetch --set-upstream http://nosuchdomain.example.com fails with invalid url' '
> +	# master explicitly not cleared, we check that it is not touched from previous value
> +	clear_config other other2 &&
> +	test_must_fail git fetch --set-upstream http://nosuchdomain.example.com &&
> +	check_config master upstream refs/heads/other &&
> +	check_config_missing other &&
> +	check_config_missing other2
> +'
> +
> +test_expect_success 'fetch --set-upstream with valid URL sets upstream to URL' '
> +	clear_config other other2 &&
> +	url="file://'"$PWD"'" &&
> +	git fetch --set-upstream "$url" &&
> +	check_config master "$url" HEAD &&
> +	check_config_missing other &&
> +	check_config_missing other2
> +'
> +
> +#tests for pull --set-upstream
> +
> +test_expect_success 'setup bare parent pull' '
> +	git remote rm upstream &&
> +	ensure_fresh_upstream &&
> +	git remote add upstream parent
> +'
> +
> +test_expect_success 'setup commit on master and other pull' '
> +	test_commit three &&
> +	git push --tags upstream master &&
> +	test_commit four &&
> +	git push upstream other
> +'
> +
> +test_expect_success 'pull --set-upstream upstream master sets branch master but not other' '
> +	clear_config master other &&
> +	git pull --set-upstream upstream master &&
> +	check_config master upstream refs/heads/master &&
> +	check_config_missing other
> +'
> +
> +test_expect_success 'pull --set-upstream master:other2 does not set the branch other2' '
> +	clear_config other2 &&
> +	git pull --set-upstream upstream master:other2 &&
> +	check_config_missing other2
> +'
> +
> +test_expect_success 'pull --set-upstream upstream other sets branch master' '
> +	clear_config master other &&
> +	git pull --set-upstream upstream other &&
> +	check_config master upstream refs/heads/other &&
> +	check_config_missing other
> +'
> +
> +test_expect_success 'pull --set-upstream upstream tag does not set the tag' '
> +	clear_config three &&
> +	git pull --tags --set-upstream upstream three &&
> +	check_config_missing three
> +'
> +
> +test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails with invalid url' '
> +	# master explicitly not cleared, we check that it is not touched from previous value
> +	clear_config other other2 three &&
> +	test_must_fail git pull --set-upstream http://nosuchdomain.example.com &&
> +	check_config master upstream refs/heads/other &&
> +	check_config_missing other &&
> +	check_config_missing other2 &&
> +	check_config_missing three
> +'
> +
> +test_expect_success 'pull --set-upstream upstream HEAD sets branch HEAD' '
> +	clear_config master other &&
> +	git pull --set-upstream upstream HEAD &&
> +	check_config master upstream HEAD &&
> +	git checkout other &&
> +	git pull --set-upstream upstream HEAD &&
> +	check_config other upstream HEAD
> +'
> +
> +test_expect_success 'pull --set-upstream upstream with more than one branch does nothing' '
> +	clear_config master three &&
> +	git pull --set-upstream upstream master three &&
> +	check_config_missing master &&
> +	check_config_missing three
> +'
> +
> +test_expect_success 'pull --set-upstream with valid URL sets upstream to URL' '
> +	clear_config master other other2 &&
> +	git checkout master &&
> +	url="file://'"$PWD"'" &&
> +	git pull --set-upstream "$url" &&
> +	check_config master "$url" HEAD &&
> +	check_config_missing other &&
> +	check_config_missing other2
> +'
> +
> +test_expect_success 'pull --set-upstream with valid URL and branch sets branch' '
> +	clear_config master other other2 &&
> +	git checkout master &&
> +	url="file://'"$PWD"'" &&
> +	git pull --set-upstream "$url" master &&
> +	check_config master "$url" refs/heads/master &&
> +	check_config_missing other &&
> +	check_config_missing other2
> +'
> +
> +test_done
> -- 
> 2.20.1.98.gecbdaf0
>
Junio C Hamano Aug. 14, 2019, 5:38 p.m. UTC | #2
Matthieu Moy <git@matthieu-moy.fr> writes:

> From: Corentin BOMPARD <corentin.bompard@etu.univ-lyon1.fr>
>
> Add the --set-upstream option to git pull/fetch
> which lets the user set the upstream configuration
> (branch.<current-branch-name>.merge and
> branch.<current-branch-name>.remote) for the current branch.
>
> A typical use-case is:
>
>     git clone http://example.com/my-public-fork
>     git remote add main http://example.com/project-main-repo
>     git pull --set-upstream main master
>
> or, instead of the last line:
>
>     git fetch --set-upstream main master
>     git merge # or git rebase
>
> This functionality is analog to push --set-upstream.

I was writing a one-paragraph summary for this topic, for the
"What's cooking" report, and here is what I have:

 "git fetch" learned "--set-upstream" option to help those who first
 clone from a forked repository they intend to push to, add the true
 upstream via "git remote add" and then "git fetch" from it.

After describing it like so, I cannot shake the feeling that the
workflow this intends to support feels somewhat backwards and
suboptimal.

 - Unless you rely on server-side "fork" like GitHub does, you would
   first clone from the upstream, and then push to your "fork".  The
   flow whose first step is to clone from your "fork", not from the
   true upstream, feels backwards (cloning from upstream then adding
   your fork as a secondary may be more natural, without need for
   the complexity of --set-upstream to pull/fetch/push, no?).

 - The second step adds the true upstream using "git remote", and at
   that point, in your mind you are quite clear that you want to
   pull from there (and push to your own fork).  Not having the "I
   am adding this new remote; from now on, it is my upstream"
   feature at this step, and instead having to say that with your
   first "git pull", feels backwards.  If this feature were instead
   added to "git remote", then the last step in your example does
   not even have to say "main" (and no need for this new option),
   does it?
Matthieu Moy Aug. 19, 2019, 9:07 a.m. UTC | #3
Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <git@matthieu-moy.fr> writes:
>
>> From: Corentin BOMPARD <corentin.bompard@etu.univ-lyon1.fr>
>>
>> Add the --set-upstream option to git pull/fetch
>> which lets the user set the upstream configuration
>> (branch.<current-branch-name>.merge and
>> branch.<current-branch-name>.remote) for the current branch.
>>
>> A typical use-case is:
>>
>>     git clone http://example.com/my-public-fork
>>     git remote add main http://example.com/project-main-repo
>>     git pull --set-upstream main master
>>
>> or, instead of the last line:
>>
>>     git fetch --set-upstream main master
>>     git merge # or git rebase
>>
>> This functionality is analog to push --set-upstream.
>
> I was writing a one-paragraph summary for this topic, for the
> "What's cooking" report, and here is what I have:
>
>  "git fetch" learned "--set-upstream" option to help those who first
>  clone from a forked repository they intend to push to, add the true
>  upstream via "git remote add" and then "git fetch" from it.
>
> After describing it like so, I cannot shake the feeling that the
> workflow this intends to support feels somewhat backwards and
> suboptimal.
>
>  - Unless you rely on server-side "fork" like GitHub does,

Note that these days, this is not a very restrictive statement ;-).

And when you make a fork on GitHub or GitLab from the web UI, the next
thing you see is the page of your fork with a button "clone or download"
pointing to your local copy's URL. So even though there are arguments to
clone upstream first, it's also quite natural from the UI point of view
to clone the local copy, and add upstream when needed.

>    you would first clone from the upstream, and then push to your
>    "fork". The flow whose first step is to clone from your "fork", not
>    from the true upstream, feels backwards (cloning from upstream then
>    adding your fork as a secondary may be more natural, without need for
>    the complexity of --set-upstream to pull/fetch/push, no?).

To me, it depends on the involvement in the project. If I plan to send
several contributions to a project, I'd usually clone the upstream and
add my fork. But I also often do:

- Find a project on GitHub/GitLab/...
- Think about a minor contribution I can make
- Fork from the web UI
- clone my fork
- code, commit, push
- make a PR

Only if my PR takes time to get accepted, I'll add upstream as a remote
and pull from there to rebase my PR.

>  - The second step adds the true upstream using "git remote", and at
>    that point, in your mind you are quite clear that you want to
>    pull from there (and push to your own fork).  Not having the "I
>    am adding this new remote; from now on, it is my upstream"

Note that you can also group "remote add" and "pull" by saying just

  git pull --set-upstream http://example.com/project-main-repo master

(I still tend to prefer the "remote add" + "pull" flow to name the
remote, though).

>    feature at this step, and instead having to say that with your
>    first "git pull", feels backwards.  If this feature were instead
>    added to "git remote", then the last step in your example does
>    not even have to say "main" (and no need for this new option),
>    does it?

There's already "git remote add --track <branch> <remote> <url>", but it
does something different: it does not set the upstream information but
only sets the glob refspec to fetch only one branch from the remote.

We could add a new option like

  git remote --set-upstream <branch> <remote> <url>

That would do

  git remote add <remote> <url>
  git branch --set-upstream-to=<branch>

That wouldn't make the commands really easier to type IMHO, as you would
still have to pull at some point, so it's:

  git remote add main http://example.com/project-main-repo
  git pull --set-upstream main master
  
Vs

  git remote add --set-upstream master main http://example.com/project-main-repo
  git pull

The second is a bit shorter (saves the second instance of "master"), but
I tend to prefer the first to avoid the overly long "git remote add"
command.

Also, if one has several local branches, one may run just one "git
remote add" and several "git pull --set-upstream".

Note that there are other possible use-cases, like "upstream was using a
flow where 'master' was the main branch, but now commits to 'develop'
branch and only merges to 'master' for releases", where you can just

  git pull --set-upstream origin develop

Actually, since "--set-upstream" means "next time, *pull* from this
branch", it felt weird to have it in "git *push*" and not in "git pull".
Certainly, not having "git pull --set-upstream" it "git pull" wasn't
that much bothering (otherwise, someone would have implemented it long
ago), but I still find it a nice-to-have shortcut.

Cheers,
Matthieu Moy Aug. 19, 2019, 9:08 a.m. UTC | #4
Pratyush Yadav <me@yadavpratyush.com> writes:

> This is not really a review. Just some minor nitpicks I spotted while 
> reading through.

Thanks for the comments.

>> -static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity, deepen_relative;
>> +static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity, deepen_relative, set_upstream;
>
> This line is getting pretty long. I think it is a good idea to split it 
> into two.

Indeed, and it was already >80 characters, I've split it.

>> +	if (set_upstream) {
>> +		struct branch *branch = branch_get("HEAD");
>> +		struct ref *rm;
>> +		struct ref *source_ref = NULL;
>> +
>> +		/*
>> +		 * We're setting the upstream configuration for the current branch. The
>> +		 * relevent upstream is the fetched branch that is meant to be merged with
>> +		 * the current one, i.e. the one fetched to FETCH_HEAD.
>> +		 *
>> +		 * When there are several such branches, consider the request ambiguous and
>> +		 * err on the safe side by doing nothing and just emit a warning.
>> +		 */
>
> The comment lines cross the 80 column boundary. The usual convention in 
> this project is to try to keep lines below 80 columns. For strings IMO 
> an exception can be allowed because breaking them up makes it harder to 
> grep for them. But comments are the easiest to format.
>
> Are you using a tab size of 4?

I'm not, but it's possible that the original authors had. Anyway, I've
wrapped it.

>> +		for (rm = ref_map; rm; rm = rm->next) {
>> +			if (!rm->peer_ref) {
>> +				if (source_ref) {
>> +					warning(_("multiple branch detected, incompatible with --set-upstream"));
>> +					goto skip;
>> +				} else {
>> +					source_ref = rm;
>> +				}
>> +			}
>> +		}
>> +		if (source_ref) {
>> +			if (!strcmp(source_ref->name, "HEAD") || 
>
> This line has a trailing space.

Fixed.

> So this should change to something like:
>
> 				install_branch_config(0, branch->name,
> 						      transport->remote->name,
> 						      source_ref->name);

I've added a newline after the comma, I don't like mixing "several
arguments on the same line" and "one argument per line".

>  
> Maybe this discrepancy is because you are using the wrong tab size?
>
>> +			} else if (starts_with(source_ref->name, "refs/remotes/")) {
>> +				warning(_("not setting upstream for a remote remote-tracking branch"));
>> +			} else if (starts_with(source_ref->name, "refs/tags/")) {
>> +				warning(_("not setting upstream for a remote tag"));
>> +			} else {
>> +				warning(_("unknown branch type"));
>> +			}
>
> No need to wrap single line if statements in braces.

Fixed.

>> +#tests for fetch --set-upstream
>
> Add a space after the '#'. Same in other comments below.

Fixed.

Thanks. Version 2 fixing all these follows.
Junio C Hamano Aug. 19, 2019, 8:04 p.m. UTC | #5
Matthieu Moy <Matthieu.Moy@matthieu-moy.fr> writes:

> To me, it depends on the involvement in the project. If I plan to send
> several contributions to a project, I'd usually clone the upstream and
> add my fork. But I also often do:
>
> - Find a project on GitHub/GitLab/...
> - Think about a minor contribution I can make
> - Fork from the web UI
> - clone my fork
> - code, commit, push
> - make a PR
>
> Only if my PR takes time to get accepted, I'll add upstream as a remote
> and pull from there to rebase my PR.

OK.

>>  - The second step adds the true upstream using "git remote", and at
>>    that point, in your mind you are quite clear that you want to
>>    pull from there (and push to your own fork).  Not having the "I
>>    am adding this new remote; from now on, it is my upstream"
>
> Note that you can also group "remote add" and "pull" by saying just
>
>   git pull --set-upstream http://example.com/project-main-repo master
>
> (I still tend to prefer the "remote add" + "pull" flow to name the
> remote, though).

I do too, and that's where my "shouldn't this feature be part of
'remote add' comes from.

> That wouldn't make the commands really easier to type IMHO, as you would
> still have to pull at some point, so it's:
>
>   git remote add main http://example.com/project-main-repo
>   git pull --set-upstream main master
>   
> Vs
>
>   git remote add --set-upstream master main http://example.com/project-main-repo
>   git pull
>
> The second is a bit shorter (saves the second instance of "master"), but
> I tend to prefer the first to avoid the overly long "git remote add"
> command.

I do not particularly care about five extra keystrokes.  The reason
I prefer the latter more is conceptual clarity of it saying "I use
'remote' to set things up, and then use 'pull' to get updated" (as
opposed to "I use 'remote' to set things half-way up, and then use
the first 'pull' to finish setting things up and getting updated.
I should remember that I do not need to give --set-upstream to later
'pull' I used to get further updates").

> Actually, since "--set-upstream" means "next time, *pull* from this
> branch", it felt weird to have it in "git *push*" and not in "git pull".
> Certainly, not having "git pull --set-upstream" it "git pull" wasn't
> that much bothering (otherwise, someone would have implemented it long
> ago), but I still find it a nice-to-have shortcut.

Yeah, I do not think 'push --set-upstream' is a great feature,
either, but since we have it already, I do not mind too much to have
another on the 'pull' side.  It just feels that we are piling band
aid for the lack of the right feature in the right command by adding
it to wrong command(s).
Matthieu Moy Aug. 20, 2019, 8:09 a.m. UTC | #6
Junio C Hamano <gitster@pobox.com> writes:

>> That wouldn't make the commands really easier to type IMHO, as you would
>> still have to pull at some point, so it's:
>>
>>   git remote add main http://example.com/project-main-repo
>>   git pull --set-upstream main master
>>   
>> Vs
>>
>>   git remote add --set-upstream master main http://example.com/project-main-repo
>>   git pull
>>
>> The second is a bit shorter (saves the second instance of "master"), but
>> I tend to prefer the first to avoid the overly long "git remote add"
>> command.
>
> I do not particularly care about five extra keystrokes.  The reason
> I prefer the latter more is conceptual clarity of it saying "I use
> 'remote' to set things up, and then use 'pull' to get updated" (as
> opposed to "I use 'remote' to set things half-way up, and then use
> the first 'pull' to finish setting things up and getting updated.
> I should remember that I do not need to give --set-upstream to later
> 'pull' I used to get further updates").

That's a good argument to add a similar feature to "git remote", and
it's a good idea for a microproject in the future actually. I admit I
didn't consider this possibility before this discussion, thanks.

I think I'll still appreciate having the possibility to "pull
--set-upstream" too:

* "git remote add" is ran once for a remote, "git pull --set-upstream"
  can be run several times for several branches.

* In practice, even when "remote add" supports "--set-upstream", I'll
  very likely forget it, and by the time I run "git pull", it'll be too
  late to add --set-upstream to my "remote add" command.

Patch
diff mbox series

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 3c9b4f9e09..99df1f3d4e 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -169,6 +169,13 @@  ifndef::git-pull[]
 	Disable recursive fetching of submodules (this has the same effect as
 	using the `--recurse-submodules=no` option).
 
+--set-upstream::
+	If the remote is fetched successfully, pull and add upstream
+	(tracking) reference, used by argument-less
+	linkgit:git-pull[1] and other commands. For more information,
+	see `branch.<name>.merge` and `branch.<name>.remote` in
+	linkgit:git-config[1].
+
 --submodule-prefix=<path>::
 	Prepend <path> to paths printed in informative messages
 	such as "Fetching submodule foo".  This option is used
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 717dd14e89..5557ae1c04 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -23,6 +23,7 @@ 
 #include "packfile.h"
 #include "list-objects-filter-options.h"
 #include "commit-reach.h"
+#include "branch.h"
 
 #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
 
@@ -50,7 +51,7 @@  static int fetch_prune_tags_config = -1; /* unspecified */
 static int prune_tags = -1; /* unspecified */
 #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */
 
-static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity, deepen_relative;
+static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity, deepen_relative, set_upstream;
 static int progress = -1;
 static int enable_auto_gc = 1;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
@@ -123,6 +124,8 @@  static struct option builtin_fetch_options[] = {
 	OPT__VERBOSITY(&verbosity),
 	OPT_BOOL(0, "all", &all,
 		 N_("fetch from all remotes")),
+	OPT_BOOL(0, "set-upstream", &set_upstream,
+		 N_("set upstream for git pull/fetch")),
 	OPT_BOOL('a', "append", &append,
 		 N_("append to .git/FETCH_HEAD instead of overwriting")),
 	OPT_STRING(0, "upload-pack", &upload_pack, N_("path"),
@@ -1367,6 +1370,49 @@  static int do_fetch(struct transport *transport,
 		retcode = 1;
 		goto cleanup;
 	}
+
+	if (set_upstream) {
+		struct branch *branch = branch_get("HEAD");
+		struct ref *rm;
+		struct ref *source_ref = NULL;
+
+		/*
+		 * We're setting the upstream configuration for the current branch. The
+		 * relevent upstream is the fetched branch that is meant to be merged with
+		 * the current one, i.e. the one fetched to FETCH_HEAD.
+		 *
+		 * When there are several such branches, consider the request ambiguous and
+		 * err on the safe side by doing nothing and just emit a warning.
+		 */
+		for (rm = ref_map; rm; rm = rm->next) {
+			if (!rm->peer_ref) {
+				if (source_ref) {
+					warning(_("multiple branch detected, incompatible with --set-upstream"));
+					goto skip;
+				} else {
+					source_ref = rm;
+				}
+			}
+		}
+		if (source_ref) {
+			if (!strcmp(source_ref->name, "HEAD") || 
+				starts_with(source_ref->name, "refs/heads/")) {
+				install_branch_config(0, branch->name,
+							 transport->remote->name,
+							 source_ref->name);
+			} else if (starts_with(source_ref->name, "refs/remotes/")) {
+				warning(_("not setting upstream for a remote remote-tracking branch"));
+			} else if (starts_with(source_ref->name, "refs/tags/")) {
+				warning(_("not setting upstream for a remote tag"));
+			} else {
+				warning(_("unknown branch type"));
+			}
+		} else {
+			warning(_("no source branch found.\n"
+				"you need to specify exactly one branch with the --set-upstream option."));
+		}
+	}
+ skip:
 	free_refs(ref_map);
 
 	/* if neither --no-tags nor --tags was specified, do automated tag
diff --git a/builtin/pull.c b/builtin/pull.c
index f1eaf6e6ed..d25ff13a60 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -129,6 +129,7 @@  static char *opt_refmap;
 static char *opt_ipv4;
 static char *opt_ipv6;
 static int opt_show_forced_updates = -1;
+static char *set_upstream;
 
 static struct option pull_options[] = {
 	/* Shared options */
@@ -243,6 +244,9 @@  static struct option pull_options[] = {
 		PARSE_OPT_NOARG),
 	OPT_BOOL(0, "show-forced-updates", &opt_show_forced_updates,
 		 N_("check for forced-updates on all updated branches")),
+	OPT_PASSTHRU(0, "set-upstream", &set_upstream, NULL,
+		N_("set upstream for git pull/fetch"),
+		PARSE_OPT_NOARG),
 
 	OPT_END()
 };
@@ -556,6 +560,8 @@  static int run_fetch(const char *repo, const char **refspecs)
 		argv_array_push(&args, "--show-forced-updates");
 	else if (opt_show_forced_updates == 0)
 		argv_array_push(&args, "--no-show-forced-updates");
+	if (set_upstream)
+		argv_array_push(&args, set_upstream);
 
 	if (repo) {
 		argv_array_push(&args, repo);
diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
new file mode 100755
index 0000000000..bd1a94f494
--- /dev/null
+++ b/t/t5553-set-upstream.sh
@@ -0,0 +1,178 @@ 
+#!/bin/sh
+
+test_description='"git fetch/pull --set-upstream" basic tests.'
+. ./test-lib.sh
+
+check_config () {
+	printf "%s\n" "$2" "$3" >"expect.$1" &&
+	{
+		git config "branch.$1.remote" && git config "branch.$1.merge"
+	} >"actual.$1" &&
+	test_cmp "expect.$1" "actual.$1"
+}
+
+check_config_missing () {
+	test_expect_code 1 git config "branch.$1.remote" &&
+	test_expect_code 1 git config "branch.$1.merge"
+}
+
+clear_config () {
+	for branch in "$@"; do
+		test_might_fail git config --unset-all "branch.$branch.remote"
+		test_might_fail git config --unset-all "branch.$branch.merge"
+	done
+}
+
+ensure_fresh_upstream () {
+	rm -rf parent && git init --bare parent
+}
+
+test_expect_success 'setup bare parent fetch' '
+	ensure_fresh_upstream &&
+	git remote add upstream parent
+'
+
+test_expect_success 'setup commit on master and other fetch' '
+	test_commit one &&
+	git push upstream master &&
+	git checkout -b other &&
+	test_commit two &&
+	git push upstream other
+'
+
+#tests for fetch --set-upstream
+
+test_expect_success 'fetch --set-upstream does not set upstream w/o branch' '
+	clear_config master other &&
+	git checkout master &&
+	git fetch --set-upstream upstream &&
+	check_config_missing master &&
+	check_config_missing other
+'
+
+test_expect_success 'fetch --set-upstream upstream master sets branch master but not other' '
+	clear_config master other &&
+	git fetch --set-upstream upstream master &&
+	check_config master upstream refs/heads/master &&
+	check_config_missing other
+'
+
+test_expect_success 'fetch --set-upstream upstream other sets branch other' '
+	clear_config master other &&
+	git fetch --set-upstream upstream other &&
+	check_config master upstream refs/heads/other &&
+	check_config_missing other
+'
+
+test_expect_success 'fetch --set-upstream master:other does not set the branch other2' '
+	clear_config other2 &&
+	git fetch --set-upstream upstream master:other2 &&
+	check_config_missing other2
+'
+
+test_expect_success 'fetch --set-upstream http://nosuchdomain.example.com fails with invalid url' '
+	# master explicitly not cleared, we check that it is not touched from previous value
+	clear_config other other2 &&
+	test_must_fail git fetch --set-upstream http://nosuchdomain.example.com &&
+	check_config master upstream refs/heads/other &&
+	check_config_missing other &&
+	check_config_missing other2
+'
+
+test_expect_success 'fetch --set-upstream with valid URL sets upstream to URL' '
+	clear_config other other2 &&
+	url="file://'"$PWD"'" &&
+	git fetch --set-upstream "$url" &&
+	check_config master "$url" HEAD &&
+	check_config_missing other &&
+	check_config_missing other2
+'
+
+#tests for pull --set-upstream
+
+test_expect_success 'setup bare parent pull' '
+	git remote rm upstream &&
+	ensure_fresh_upstream &&
+	git remote add upstream parent
+'
+
+test_expect_success 'setup commit on master and other pull' '
+	test_commit three &&
+	git push --tags upstream master &&
+	test_commit four &&
+	git push upstream other
+'
+
+test_expect_success 'pull --set-upstream upstream master sets branch master but not other' '
+	clear_config master other &&
+	git pull --set-upstream upstream master &&
+	check_config master upstream refs/heads/master &&
+	check_config_missing other
+'
+
+test_expect_success 'pull --set-upstream master:other2 does not set the branch other2' '
+	clear_config other2 &&
+	git pull --set-upstream upstream master:other2 &&
+	check_config_missing other2
+'
+
+test_expect_success 'pull --set-upstream upstream other sets branch master' '
+	clear_config master other &&
+	git pull --set-upstream upstream other &&
+	check_config master upstream refs/heads/other &&
+	check_config_missing other
+'
+
+test_expect_success 'pull --set-upstream upstream tag does not set the tag' '
+	clear_config three &&
+	git pull --tags --set-upstream upstream three &&
+	check_config_missing three
+'
+
+test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails with invalid url' '
+	# master explicitly not cleared, we check that it is not touched from previous value
+	clear_config other other2 three &&
+	test_must_fail git pull --set-upstream http://nosuchdomain.example.com &&
+	check_config master upstream refs/heads/other &&
+	check_config_missing other &&
+	check_config_missing other2 &&
+	check_config_missing three
+'
+
+test_expect_success 'pull --set-upstream upstream HEAD sets branch HEAD' '
+	clear_config master other &&
+	git pull --set-upstream upstream HEAD &&
+	check_config master upstream HEAD &&
+	git checkout other &&
+	git pull --set-upstream upstream HEAD &&
+	check_config other upstream HEAD
+'
+
+test_expect_success 'pull --set-upstream upstream with more than one branch does nothing' '
+	clear_config master three &&
+	git pull --set-upstream upstream master three &&
+	check_config_missing master &&
+	check_config_missing three
+'
+
+test_expect_success 'pull --set-upstream with valid URL sets upstream to URL' '
+	clear_config master other other2 &&
+	git checkout master &&
+	url="file://'"$PWD"'" &&
+	git pull --set-upstream "$url" &&
+	check_config master "$url" HEAD &&
+	check_config_missing other &&
+	check_config_missing other2
+'
+
+test_expect_success 'pull --set-upstream with valid URL and branch sets branch' '
+	clear_config master other other2 &&
+	git checkout master &&
+	url="file://'"$PWD"'" &&
+	git pull --set-upstream "$url" master &&
+	check_config master "$url" refs/heads/master &&
+	check_config_missing other &&
+	check_config_missing other2
+'
+
+test_done