[PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

[PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

[Corentin BOMPARD]

Adding the --set-upstream option to git pull/fetch
which lets the user set the upstream configuration
for the current branch.

For example a typical use-case like
    git clone http://example.com/my-public-fork
    git remote add main http://example.com/project-main-repo
    git pull main master --set-upstream
or, instead of the last line
    git fetch main master --set-upstream
    git merge # or git rebase

This foncionality works like git 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 <matthieu.moy@univ-lyon1.fr>
---
 At the moment it works on overall but we'd like to have the
 community feedback about:
 
 * We decide to add the set-upstream as an argument for both git pull
    and git fetch but we can also implement the set-upstream argument
    in git fetch or in git pull only.

 * We took the ref_map entry which fetches locally to FETCH_HEAD 
    as the new current branch's config because git fetch store the 
    remote-tracking branch into FETCH_HEAD before call git merge.
 
 Or about things we missed and things we could improve.
 

 Documentation/fetch-options.txt |   5 ++
 builtin/fetch.c                 |  60 ++++++++++++++++-
 builtin/pull.c                  |   6 ++
 t/t5553-set-upstream.sh         | 141 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 211 insertions(+), 1 deletion(-)
 create mode 100644 t/t5553-set-upstream.sh

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index fa0a3151b..4d2d55643 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -165,6 +165,11 @@ ifndef::git-pull[]
 	Disable recursive fetching of submodules (this has the same effect as
 	using the `--recurse-submodules=no` option).
 
+--set-upstream::
+	If the new URL remote is correct, pull and add upstream (tracking) 
+	reference, used by argument-less linkgit:git-push[1] and other commands.
+	For more information, see `branch.<name>.merge` 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 b620fd54b..ebe72ea1c 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"
 
 static const char * const builtin_fetch_usage[] = {
 	N_("git fetch [<options>] [<repository> [<refspec>...]]"),
@@ -46,7 +47,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 tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
 static int max_children = 1;
@@ -113,6 +114,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"),
@@ -1317,6 +1320,61 @@ static int do_fetch(struct transport *transport,
 		retcode = 1;
 		goto cleanup;
 	}
+
+	/* TODO: remove debug trace */
+	if (set_upstream) {
+		struct branch *branch = branch_get("HEAD");
+		struct ref *rm;
+		struct ref *target = NULL;
+		/*
+		 * We want to set the current branch config following the 
+		 * ref_map entry which fetches on FETCH_HEAD
+		 * In case of "git pull <remote> --set-upstream" we
+		 * 	don't want to set all branches' config.
+		 * If there is no local ref which points on FETCH_HEAD
+		 * 	we don't set the config for the current branch
+		 * 	and warn the user.
+		 * If there is a fetch of more than one branch for example: 
+		 * 	"git pull <remote> <branch> <branch> --set-upstream"
+		 *	setting the current branch's config makes no sense.
+		 * Where we are in case of "git pull <remote> <branch>:<branch>" we
+		 * 	don't want to set the config for the local branch
+		 * 	can be improved in the future to set local branch's config.
+		 */
+		for (rm = ref_map; rm; rm = rm->next) {
+			fprintf(stderr, "\n -%s", rm->name);
+			if (rm->peer_ref) {
+				fprintf(stderr, " -> %s", rm->peer_ref->name);
+			} else {
+				if (target) {
+					fprintf(stderr, " -> FETCH_HEAD\n");
+					warning(_("Multiple FETCH_HEAD"));
+					target = NULL;
+					break;
+				} else {
+					target = rm;
+					fprintf(stderr, " -> FETCH_HEAD");
+				}
+			}
+		}
+		fprintf(stderr, "\n\n");
+		if (target) {
+			if (!strcmp(ref_map->name, "HEAD") ||
+					starts_with(ref_map->name, "refs/heads/")) {
+				install_branch_config(0, branch->name,
+							 transport->remote->name,
+							 target->name);
+			} else if (starts_with(ref_map->name, "refs/remotes/")) {
+				warning(_("Not setting upstream for a remote remote-tracking branch"));
+			} else if (starts_with(ref_map->name, "refs/tags/")) {
+				warning(_("Tag upstream not set"));
+			} else {
+				warning(_("Unknown branch type"));
+			}
+		} else {
+			warning(_("Fetching more than one branch. Current branch's upstream not set"));
+		}
+	}
 	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 701d1473d..06d7cddce 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -122,6 +122,7 @@ static char *opt_update_shallow;
 static char *opt_refmap;
 static char *opt_ipv4;
 static char *opt_ipv6;
+static char *set_upstream;
 
 static struct option pull_options[] = {
 	/* Shared options */
@@ -233,6 +234,9 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU('6',  "ipv6", &opt_ipv6, NULL,
 		N_("use IPv6 addresses only"),
 		PARSE_OPT_NOARG),
+	OPT_PASSTHRU(0, "set-upstream", &set_upstream, NULL,
+		N_("set upstream for git pull/fetch"),
+		PARSE_OPT_NOARG),
 
 	OPT_END()
 };
@@ -541,6 +545,8 @@ static int run_fetch(const char *repo, const char **refspecs)
 		argv_array_push(&args, opt_ipv4);
 	if (opt_ipv6)
 		argv_array_push(&args, opt_ipv6);
+	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 100644
index 000000000..92350dfb0
--- /dev/null
+++ b/t/t5553-set-upstream.sh
@@ -0,0 +1,141 @@
+#!/bin/sh
+
+test_description='"git fetch/pull --set-upstream" basic tests.
+
+'
+. ./test-lib.sh
+
+
+
+check_config() {
+	(echo $2; echo $3) >expect.$1
+	(git config branch.$1.remote
+	 git config branch.$1.merge) >actual.$1
+	test_cmp expect.$1 actual.$1
+}
+
+check_config_empty() {
+	git config branch.$1.remote >remote.$1
+	test_must_be_empty remote.$1
+	git config branch.$1.merge >merge.$1
+	test_must_be_empty merge.$1
+}
+
+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 &&
+	git remote add up 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 branch other' '
+	git checkout master &&
+	git fetch --set-upstream upstream &&
+	check_config_empty master &&
+	check_config_empty other
+'
+
+#test_expect_success 'fetch --set-upstream does not set branch other' '
+#	git checkout master &&
+#	git fetch --set-upstream upstream &&
+#	check_config master upstream refs/heads/master &&
+#	check_config_empty other
+#'
+
+test_expect_success 'fetch --set-upstream upstream master sets branch master but not other' '
+	git fetch --set-upstream upstream master &&
+	check_config master upstream refs/heads/master &&
+	check_config_empty other
+'
+
+
+test_expect_success 'fetch --set-upstream upstream other sets branch other' '
+	git fetch --set-upstream upstream other &&
+	check_config master upstream refs/heads/other &&
+	check_config_empty other
+'
+
+
+test_expect_success 'fetch --set-upstream master:other does not set the branch other2' '
+	git fetch --set-upstream upstream master:other2 &&
+	check_config_empty other2
+'
+
+
+test_expect_success 'fetch --set-upstream http://nosuchdomain.example.com fails with the bad url' '
+	test_must_fail git fetch --set-upstream http://nosuchdomain.example.com
+'
+
+
+#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' '
+	git pull --set-upstream upstream master &&
+	check_config master upstream refs/heads/master &&
+	check_config_empty other
+'
+
+test_expect_success 'pull --set-upstream master:other2 does not set the branch other2' '
+	git pull --set-upstream upstream master:other2 &&
+	check_config_empty other2
+'
+
+test_expect_success 'pull --set-upstream upstream other sets branch other' '
+	git pull --set-upstream upstream other &&
+	check_config master upstream refs/heads/other &&
+	check_config_empty other
+'
+
+test_expect_success 'pull --set-upstream upstream tag does not set the tag' '
+	git pull --tags --set-upstream upstream three &&
+	check_config_empty three
+'
+
+test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails with the bad url' '
+	test_must_fail git pull --set-upstream http://nosuchdomain.example.com
+'
+
+test_expect_success 'pull --set-upstream upstream HEAD sets branch HEAD' '
+	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' '
+	git pull --set-upstream upstream master three &&
+	check_config master upstream HEAD &&
+	check_config_empty three
+'
+
+test_done
-- 
2.11.0

Re: [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

[Matthieu Moy]

BOMPARD CORENTIN p1603631 <corentin.bompard@etu.univ-lyon1.fr> writes:

> Adding the --set-upstream option to git pull/fetch

We usually write commit messages with imperative tone, hence "add", not
"adding".

> +		/*
> +		 * We want to set the current branch config following the 
> +		 * ref_map entry which fetches on FETCH_HEAD

fetches _to_? And period at end of sentence.

> +		 * In case of "git pull <remote> --set-upstream" we
> +		 * 	don't want to set all branches' config.
> +		 * If there is no local ref which points on FETCH_HEAD

Indentation is weird. If you're just writting sentences, just wrap the
text 1 column away from the "*", and to make paragraphs, add blank lines
(containing just "*") between paragraphs.

> +		 * 	we don't set the config for the current branch
> +		 * 	and warn the user.
> +		 * If there is a fetch of more than one branch for example: 
> +		 * 	"git pull <remote> <branch> <branch> --set-upstream"
> +		 *	setting the current branch's config makes no sense.
> +		 * Where we are in case of "git pull <remote> <branch>:<branch>" we
> +		 * 	don't want to set the config for the local branch
> +		 * 	can be improved in the future to set local branch's config.
> +		 */

I'm biaised because we talked about this in real-life, but I find the
explanation unclear. I'd write stg like

/*
 * We're setting the upstream configuration for the current branch. The
 * relevant 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.
 */

I think the discussion about the various use-case that may lead to
different cases (0, 1 or >1 branches fetched to FETCH_HEAD) is not
needed here, but can be relevant comments in the tests.

> +		for (rm = ref_map; rm; rm = rm->next) {
> +			fprintf(stderr, "\n -%s", rm->name);
> +			if (rm->peer_ref) {
> +				fprintf(stderr, " -> %s", rm->peer_ref->name);
> +			} else {
> +				if (target) {
> +					fprintf(stderr, " -> FETCH_HEAD\n");
> +					warning(_("Multiple FETCH_HEAD"));

Is this a debug statement or a real warning? In the later case, it
should be made clearer to the user.

> +					target = NULL;
> +					break;
> +				} else {
> +					target = rm;

This is the branch you're fetching from, right? If so, "target" is a
misleading name. Perhaps source_ref?

> +					fprintf(stderr, " -> FETCH_HEAD");
> +				}
> +			}
> +		}
> +		fprintf(stderr, "\n\n");
> +		if (target) {
> +			if (!strcmp(ref_map->name, "HEAD") ||
> +					starts_with(ref_map->name, "refs/heads/")) {

Weird indentation. Perhaps you have a tab-width != 8?

More importantly, shouldn't ref_map->name be target->name here?

> +				install_branch_config(0, branch->name,
> +							 transport->remote->name,
> +							 target->name);
> +			} else if (starts_with(ref_map->name, "refs/remotes/")) {
> +				warning(_("Not setting upstream for a remote remote-tracking branch"));
> +			} else if (starts_with(ref_map->name, "refs/tags/")) {
> +				warning(_("Tag upstream not set"));
> +			} else {
> +				warning(_("Unknown branch type"));
> +			}
> +		} else {
> +			warning(_("Fetching more than one branch. Current branch's upstream not set"));

The warning seems misleading to me: this else branch is executed in many
cases (described in the comment above), not only when there's more than
one branch, right?

> --- /dev/null
> +++ b/t/t5553-set-upstream.sh
> @@ -0,0 +1,141 @@
> +#!/bin/sh
> +
> +test_description='"git fetch/pull --set-upstream" basic tests.
> +
> +'
> +. ./test-lib.sh
> +
> +
> +
> +check_config() {
> +	(echo $2; echo $3) >expect.$1
> +	(git config branch.$1.remote
> +	 git config branch.$1.merge) >actual.$1
> +	test_cmp expect.$1 actual.$1
> +}
> +
> +check_config_empty() {
> +	git config branch.$1.remote >remote.$1
> +	test_must_be_empty remote.$1
> +	git config branch.$1.merge >merge.$1
> +	test_must_be_empty merge.$1
> +}

Broken &&-chain (in both functions, but most importantly in the second,
where the first test_must_be_empty is useless without &&.

> +test_expect_success 'fetch --set-upstream does not set branch other' '

Misleading test name: "set branch" -> "set upstream"? And here it's not
just about "other" but about all branches.

'fetch --set-upstream does not set upstream w/o branch'

?

> +	git checkout master &&
> +	git fetch --set-upstream upstream &&
> +	check_config_empty master &&
> +	check_config_empty other
> +'

> +#test_expect_success 'fetch --set-upstream does not set branch other' '
> +#	git checkout master &&
> +#	git fetch --set-upstream upstream &&
> +#	check_config master upstream refs/heads/master &&
> +#	check_config_empty other
> +#'

Avoid leaving leftovers like this, even in WIP patches, they distract
the reader.

> +test_expect_success 'fetch --set-upstream upstream master sets branch master but not other' '
> +	git fetch --set-upstream upstream master &&
> +	check_config master upstream refs/heads/master &&
> +	check_config_empty other
> +'
> +
> +

Style: you sometimes leave 2 blank lines, sometimes 1 between tests. Try
to be consistent.

> +test_expect_success 'pull --set-upstream upstream other sets branch other' '

Test title and content say the opposite of each other.

> +	git pull --set-upstream upstream other &&
> +	check_config master upstream refs/heads/other &&
> +	check_config_empty other
> +'

> +test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails with the bad url' '
> +	test_must_fail git pull --set-upstream http://nosuchdomain.example.com
> +'

You should check that it doesn't touch the config. That it fails is not
a surprise regardless of the correctness of your code, but the thing to
check is that it does not touch the config before failing.

> +test_expect_success 'pull --set-upstream upstream with more than one branch does nothing' '

Here also, test title and content say different things. Probably you
need to reset the config and use check_config_empty.

> +	git pull --set-upstream upstream master three &&
> +	check_config master upstream HEAD &&
> +	check_config_empty three
> +'

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

[Corentin BOMPARD]

> BOMPARD CORENTIN p1603631 <corentin.bompard@etu.univ-lyon1.fr> writes:
>
>> Adding the --set-upstream option to git pull/fetch
>
> We usually write commit messages with imperative tone, hence "add", not
> "adding".

Fixed.

>> +		/*
>> +		 * We want to set the current branch config following the 
>> +		 * ref_map entry which fetches on FETCH_HEAD
>
> fetches _to_? And period at end of sentence.

Fixed.

>> +		 * In case of "git pull <remote> --set-upstream" we
>> +		 * 	don't want to set all branches' config.
>> +		 * If there is no local ref which points on FETCH_HEAD
>
> Indentation is weird. If you're just writting sentences, just wrap the
> text 1 column away from the "*", and to make paragraphs, add blank lines
> (containing just "*") between paragraphs.

We fixed indentation.

>> +		 * 	we don't set the config for the current branch
>> +		 * 	and warn the user.
>> +		 * If there is a fetch of more than one branch for example: 
>> +		 * 	"git pull <remote> <branch> <branch> --set-upstream"
>> +		 *	setting the current branch's config makes no sense.
>> +		 * Where we are in case of "git pull <remote> <branch>:<branch>" we
>> +		 * 	don't want to set the config for the local branch
>> +		 * 	can be improved in the future to set local branch's config.
>> +		 */
>
> I'm biaised because we talked about this in real-life, but I find the
> explanation unclear. I'd write stg like
> /*
> * We're setting the upstream configuration for the current branch. The
> * relevant 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.
> */
>
> I think the discussion about the various use-case that may lead to
> different cases (0, 1 or >1 branches fetched to FETCH_HEAD) is not
> needed here, but can be relevant comments in the tests.

We took your message and we will add the use-case in test file.

>> +		for (rm = ref_map; rm; rm = rm->next) {
>> +			fprintf(stderr, "\n -%s", rm->name);
>> +			if (rm->peer_ref) {
>> +				fprintf(stderr, " -> %s", rm->peer_ref->name);
>> +			} else {
>> +				if (target) {
>> +					fprintf(stderr, " -> FETCH_HEAD\n");
>> +					warning(_("Multiple FETCH_HEAD"));
>
> Is this a debug statement or a real warning? In the later case, it
> should be made clearer to the user.

This statement is called when the user call set-upstream with more
than one branch like "git pull <remote> <branch> <branch> --set-upstream"
We replaced the warning message by the following message
"Multiple branch detected, incompatible with --set-upstream".

>> +					target = NULL;
>> +					break;
>> +				} else {
>> +					target = rm;
>
> This is the branch you're fetching from, right? If so, "target" is a
> misleading name. Perhaps source_ref?

We replaced target with source_ref because it's clearer.

>> +					fprintf(stderr, " -> FETCH_HEAD");
>> +				}
>> +			}
>> +		}
>> +		fprintf(stderr, "\n\n");
>> +		if (target) {
>> +			if (!strcmp(ref_map->name, "HEAD") ||
>> +					starts_with(ref_map->name, "refs/heads/")) {
>
> Weird indentation. Perhaps you have a tab-width != 8?

Taken in consideration.

> More importantly, shouldn't ref_map->name be target->name here?

Fixed.

>> +				install_branch_config(0, branch->name,
>> +							 transport->remote->name,
>> +							 target->name);
>> +			} else if (starts_with(ref_map->name, "refs/remotes/")) {
>> +				warning(_("Not setting upstream for a remote remote-tracking branch"));
>> +			} else if (starts_with(ref_map->name, "refs/tags/")) {
>> +				warning(_("Tag upstream not set"));
>> +			} else {
>> +				warning(_("Unknown branch type"));
>> +			}
>> +		} else {
>> +			warning(_("Fetching more than one branch. Current branch's upstream not set"));
>
> The warning seems misleading to me: this else branch is executed in many
> cases (described in the comment above), not only when there's more than
> one branch, right?

This else clause is executed if there is more than one branch which fetches to FETCH_HEAD
or if the user use the syntax git pull --set-upstream <remote> <branch>:<branch> or if there is no
branch which fetches to FETCH_HEAD.

>> --- /dev/null
>> +++ b/t/t5553-set-upstream.sh
>> @@ -0,0 +1,141 @@
>> +#!/bin/sh
>> +
>> +test_description='"git fetch/pull --set-upstream" basic tests.
>> +
>> +'
>> +. ./test-lib.sh
>> +
>> +
>> +
>> +check_config() {
>> +	(echo $2; echo $3) >expect.$1
>> +	(git config branch.$1.remote
>> +	 git config branch.$1.merge) >actual.$1
>> +	test_cmp expect.$1 actual.$1
>> +}
>> +
>> +check_config_empty() {
>> +	git config branch.$1.remote >remote.$1
>> +	test_must_be_empty remote.$1
>> +	git config branch.$1.merge >merge.$1
>> +	test_must_be_empty merge.$1
>> +}
>
> Broken &&-chain (in both functions, but most importantly in the second,
> where the first test_must_be_empty is useless without &&.

We restored the &&-chain in the functions. 

>> +test_expect_success 'fetch --set-upstream does not set branch other' '
>
> Misleading test name: "set branch" -> "set upstream"? And here it's not
> just about "other" but about all branches.
>
> 'fetch --set-upstream does not set upstream w/o branch'
> ?

We edited the test's title

>> +	git checkout master &&
>> +	git fetch --set-upstream upstream &&
>> +	check_config_empty master &&
>> +	check_config_empty other
>> +'
>
>> +#test_expect_success 'fetch --set-upstream does not set branch other' '
>> +#	git checkout master &&
>> +#	git fetch --set-upstream upstream &&
>> +#	check_config master upstream refs/heads/master &&
>> +#	check_config_empty other
>> +#'
>
> Avoid leaving leftovers like this, even in WIP patches, they distract
> the reader.

We removed the test in comment because it no longer makes sense.

>> +test_expect_success 'fetch --set-upstream upstream master sets branch master but not other' '
>> +	git fetch --set-upstream upstream master &&
>> +	check_config master upstream refs/heads/master &&
>> +	check_config_empty other
>> +'
>> +
>> +
>
> Style: you sometimes leave 2 blank lines, sometimes 1 between tests. Try
> to be consistent.

We removed to have only 1 blank line between tests.

>> +test_expect_success 'pull --set-upstream upstream other sets branch other' '
>
> Test title and content say the opposite of each other.
>
>> +	git pull --set-upstream upstream other &&
>> +	check_config master upstream refs/heads/other &&
>> +	check_config_empty other
>> +'

We changed the title of this test.

>> +test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails with the bad url' '
>> +	test_must_fail git pull --set-upstream http://nosuchdomain.example.com
>> +'
>
> You should check that it doesn't touch the config. That it fails is not
> a surprise regardless of the correctness of your code, but the thing to
> check is that it does not touch the config before failing.

We added some config check and improved 
the test 'fetch ---set-upstream http://nosuchdomain.example.com fails with the bad url'.

>> +test_expect_success 'pull --set-upstream upstream with more than one branch does nothing' '
>
> Here also, test title and content say different things. Probably you
> need to reset the config and use check_config_empty.

We created a new function clear_config which clears the branches config and use check_config_empty to 
check if the config is empty for all branches.

The fixed patch will follow.

[PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

[Corentin BOMPARD]

Add the --set-upstream option to git pull/fetch
which lets the user set the upstream configuration
for the current branch.

For example a typical use-case like
    git clone http://example.com/my-public-fork
    git remote add main http://example.com/project-main-repo
    git pull main master --set-upstream
or, instead of the last line
    git fetch main master --set-upstream
    git merge # or git rebase

This foncionality works like git 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 <matthieu.moy@univ-lyon1.fr>
---
 Sorry for being so long.

 Documentation/fetch-options.txt |   5 ++
 builtin/fetch.c                 |  55 ++++++++++++-
 builtin/pull.c                  |   6 ++
 t/t5553-set-upstream.sh         | 142 ++++++++++++++++++++++++++++++++
 4 files changed, 207 insertions(+), 1 deletion(-)
 create mode 100644 t/t5553-set-upstream.sh

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index fa0a3151b..4d2d55643 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -165,6 +165,11 @@ ifndef::git-pull[]
 	Disable recursive fetching of submodules (this has the same effect as
 	using the `--recurse-submodules=no` option).
 
+--set-upstream::
+	If the new URL remote is correct, pull and add upstream (tracking) 
+	reference, used by argument-less linkgit:git-push[1] and other commands.
+	For more information, see `branch.<name>.merge` 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 b620fd54b..b43a4e0a2 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"
 
 static const char * const builtin_fetch_usage[] = {
 	N_("git fetch [<options>] [<repository> [<refspec>...]]"),
@@ -46,7 +47,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 tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
 static int max_children = 1;
@@ -113,6 +114,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"),
@@ -1317,6 +1320,56 @@ static int do_fetch(struct transport *transport,
 		retcode = 1;
 		goto cleanup;
 	}
+
+	/* TODO: remove debug trace */
+	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 waring.
+		 */
+		for (rm = ref_map; rm; rm = rm->next) {
+			fprintf(stderr, "\n -%s", rm->name);
+			if (rm->peer_ref) {
+				fprintf(stderr, " -> %s", rm->peer_ref->name);
+			} else {
+				if (source_ref) {
+					fprintf(stderr, " -> FETCH_HEAD\n");
+					warning(_("Multiple branch detected, incompatible with set-upstream"));
+					source_ref = NULL;
+					goto skip;
+				} else {
+					source_ref = rm;
+					fprintf(stderr, " -> FETCH_HEAD");
+				}
+			}
+		}
+		fprintf(stderr, "\n\n");
+		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(_("Tag upstream not set"));
+			} else {
+				warning(_("Unknown branch type"));
+			}
+		} else {
+			warning(_("No source branch found. \n You need to specify excatly "
+						"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 701d1473d..06d7cddce 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -122,6 +122,7 @@ static char *opt_update_shallow;
 static char *opt_refmap;
 static char *opt_ipv4;
 static char *opt_ipv6;
+static char *set_upstream;
 
 static struct option pull_options[] = {
 	/* Shared options */
@@ -233,6 +234,9 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU('6',  "ipv6", &opt_ipv6, NULL,
 		N_("use IPv6 addresses only"),
 		PARSE_OPT_NOARG),
+	OPT_PASSTHRU(0, "set-upstream", &set_upstream, NULL,
+		N_("set upstream for git pull/fetch"),
+		PARSE_OPT_NOARG),
 
 	OPT_END()
 };
@@ -541,6 +545,8 @@ static int run_fetch(const char *repo, const char **refspecs)
 		argv_array_push(&args, opt_ipv4);
 	if (opt_ipv6)
 		argv_array_push(&args, opt_ipv6);
+	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 100644
index 000000000..6126bb188
--- /dev/null
+++ b/t/t5553-set-upstream.sh
@@ -0,0 +1,142 @@
+#!/bin/sh
+
+test_description='"git fetch/pull --set-upstream" basic tests.
+
+'
+. ./test-lib.sh
+
+check_config() {
+	(echo $2; echo $3) >expect.$1 &&
+	(git config branch.$1.remote
+	 git config branch.$1.merge) >actual.$1 &&
+	test_cmp expect.$1 actual.$1
+}
+
+check_config_empty() {
+	test_must_fail git config branch.$1.remote &&
+	test_must_fail git config branch.$1.merge
+}
+check_config_empty1() {
+	git config branch.$1.remote >remote.$1
+	test_must_be_empty remote.$1 &&
+	git config branch.$1.merge >merge.$1
+	test_must_be_empty merge.$1
+}
+
+clear_config() {
+	git config --unset branch.$1.remote
+	git config --unset branch.$1.merge
+}
+
+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 &&
+	git remote add up 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' '
+	git checkout master &&
+	git fetch --set-upstream upstream &&
+	check_config_empty master &&
+	check_config_empty other
+'
+
+test_expect_success 'fetch --set-upstream upstream master sets branch master but not other' '
+	git fetch --set-upstream upstream master &&
+	check_config master upstream refs/heads/master &&
+	check_config_empty other
+'
+
+test_expect_success 'fetch --set-upstream upstream other sets branch other' '
+	git fetch --set-upstream upstream other &&
+	check_config master upstream refs/heads/other &&
+	check_config_empty other
+'
+
+test_expect_success 'fetch --set-upstream master:other does not set the branch other2' '
+	git fetch --set-upstream upstream master:other2 &&
+	check_config_empty other2
+'
+
+test_expect_success 'fetch --set-upstream http://nosuchdomain.example.com fails with the bad url' '
+	test_must_fail git fetch --set-upstream http://nosuchdomain.example.com &&
+	check_config master upstream refs/heads/other &&
+	check_config_empty other &&
+	check_config_empty 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' '
+	git pull --set-upstream upstream master &&
+	check_config master upstream refs/heads/master &&
+	check_config_empty other
+'
+
+test_expect_success 'pull --set-upstream master:other2 does not set the branch other2' '
+	git pull --set-upstream upstream master:other2 &&
+	check_config_empty other2
+'
+
+test_expect_success 'pull --set-upstream upstream other sets branch master' '
+	git pull --set-upstream upstream other &&
+	check_config master upstream refs/heads/other &&
+	check_config_empty other
+'
+
+test_expect_success 'pull --set-upstream upstream tag does not set the tag' '
+	git pull --tags --set-upstream upstream three &&
+	check_config_empty three
+'
+
+test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails with the bad url' '
+	test_must_fail git pull --set-upstream http://nosuchdomain.example.com &&
+	check_config master upstream refs/heads/other &&
+	check_config_empty other &&
+	check_config_empty other2 &&
+	check_config_empty three
+'
+
+test_expect_success 'pull --set-upstream upstream HEAD sets branch HEAD' '
+	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 &&
+	git pull --set-upstream upstream master three &&
+	check_config_empty master &&
+	check_config_empty three
+'
+
+test_done
-- 
2.21.0-rc0

Re: [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

[Junio C Hamano]

Corentin BOMPARD <corentin.bompard@etu.univ-lyon1.fr> writes:

> Add the --set-upstream option to git pull/fetch
> which lets the user set the upstream configuration
> for the current branch.

I think it is a good idea to mention what you exactly mean by "the
upstream configuration" here.  

Do you mean the "branch.<current-branch-name>.merge" configuration
variable?

> For example a typical use-case like
>     git clone http://example.com/my-public-fork
>     git remote add main http://example.com/project-main-repo
>     git pull main master --set-upstream
> or, instead of the last line
>     git fetch main master --set-upstream
>     git merge # or git rebase

A bit more blank lines around the block of sample commands would
make the result easier to read.

> This foncionality works like git push --set-upstream.

functionality?

>
> 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 <matthieu.moy@univ-lyon1.fr>
> ---
>  Sorry for being so long.
>
>  Documentation/fetch-options.txt |   5 ++
>  builtin/fetch.c                 |  55 ++++++++++++-
>  builtin/pull.c                  |   6 ++
>  t/t5553-set-upstream.sh         | 142 ++++++++++++++++++++++++++++++++
>  4 files changed, 207 insertions(+), 1 deletion(-)
>  create mode 100644 t/t5553-set-upstream.sh
>
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index fa0a3151b..4d2d55643 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -165,6 +165,11 @@ ifndef::git-pull[]
>  	Disable recursive fetching of submodules (this has the same effect as
>  	using the `--recurse-submodules=no` option).
>  
> +--set-upstream::
> +	If the new URL remote is correct, pull and add upstream (tracking) 
> +	reference, used by argument-less linkgit:git-push[1] and other commands.

git-push and other commands?  The way I read the motivating use case
example we saw in the proposed commit log message, i.e.

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

was that your initial cloning made "fetch/pull" by default interact
with your public fork by mistake, and you are correcting it with the
new "--set-upstream" option so that future "fetch/pull" will instead
go to the true upstream, while directing your "push" traffic to still
go to your public fork.  If that is the case, then shouldn't this
paragraph in the doc talking about affecting future "git-fetch and
other commands", or "git fetch and pull" (which may be better)?

	Side note *1*: by the way, don't write --dashed-options
	after positional arguments; the parse-options parser may
	allow such a sloppy command line but it makes the examples
	inconsistent when done in the documentation and log
	messages.

> @@ -1317,6 +1320,56 @@ static int do_fetch(struct transport *transport,
>  		retcode = 1;
>  		goto cleanup;
>  	}
> +
> +	/* TODO: remove debug trace */

Perhaps do so before sending it out for the review?

> +	if (set_upstream) {
> +		struct branch *branch = branch_get("HEAD");
> +		struct ref *rm;
> +		struct ref *source_ref = NULL;

Have a blank line here, after the decls that appear before the first
statement in a block.

> +		/*
> +		 * 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 waring.
> +		 */
> +		for (rm = ref_map; rm; rm = rm->next) {
> +			fprintf(stderr, "\n -%s", rm->name);
> +			if (rm->peer_ref) {
> +				fprintf(stderr, " -> %s", rm->peer_ref->name);
> +			} else {
> +				if (source_ref) {
> +					fprintf(stderr, " -> FETCH_HEAD\n");
> +					warning(_("Multiple branch detected, incompatible with set-upstream"));

downcase "M" for consistency.  I won't repeat for other new messages
in the patch.

Shouldn't this be diagnosed as an error and stop the "fetch" or
"pull", though?

> diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
> new file mode 100644

Make your test scripts executable.

> index 000000000..6126bb188
> --- /dev/null
> +++ b/t/t5553-set-upstream.sh
> @@ -0,0 +1,142 @@
> +#!/bin/sh
> +
> +test_description='"git fetch/pull --set-upstream" basic tests.
> +
> +'
> +. ./test-lib.sh
> +
> +check_config() {

SP before () is missing here (I won't repeat).

> +	(echo $2; echo $3) >expect.$1 &&

Make sure to dq quote $variable_references UNLESS you mean you
intend to pass a string with $IFS in it and want the shell to split
the interpolation into individual tokens (I won't repeat).

Especially, quote the filename that is a target for redirection to
work-around a (mis)feature in bash (I won't repeat).

You do not need subshell for the above.  Perhaps

	printf "%s\n" "$2" "$3" >"expect.$1" &&

> +	(git config branch.$1.remote
> +	 git config branch.$1.merge) >actual.$1 &&

You do not need a subshell for this, either

	{
		git config "branch.$1.remote" && git config "branch.$1.merge"
	} >"actual.$1"

> +	test_cmp expect.$1 actual.$1

> +check_config_empty() {

s/empty/missing/ would make the distinction even clear.

> +	test_must_fail git config branch.$1.remote &&
> +	test_must_fail git config branch.$1.merge

Do we document that "git config" errors out with a more specific
signal to say "the reason why the command has failed is because the
key was not found", by the way?  I think we do, and in that case the
test should expect that specific exit code.

> +}
> +check_config_empty1() {

A blank line before a new shell function.

This one is about an empty string, so it can be named check_config_empty
once the misnamed one above that checked for a missing definition gets
renamed away.

> +	git config branch.$1.remote >remote.$1

Here is a break &&-chain; intended?

> +	test_must_be_empty remote.$1 &&
> +	git config branch.$1.merge >merge.$1

Likewise.

> +	test_must_be_empty merge.$1
> +}

If this wanted to say "It is OK for the variable to be missing, and
it also is OK for the variable to have an empty string as its value;
all other cases are unacceptable", then have another layer of helper
perhaps like

        variable_missing_or_empty () (
                value=$(git config "$1")
                case $? in
                0)	# exists
                        test -z "$value" ;;
                1)	# missing
                        true ;;
                *)	false ;;
                esac
        )

and then you can say

	check_config_missing_or_empty () {
		variable_missing_or_empty "remote.$1" &&
		variable_missing_or_empty "merge.$1"
	}

In any case, you do not seem to use empty1 variant in the rest of
the patch.  Has this been proofread before getting sent?

	... Ahh, this is WIP/RFC.  So a later iteration may start
	using it.  OK then.

Re: [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

>> --- a/Documentation/fetch-options.txt
>> +++ b/Documentation/fetch-options.txt
>> @@ -165,6 +165,11 @@ ifndef::git-pull[]
>>  	Disable recursive fetching of submodules (this has the same effect as
>>  	using the `--recurse-submodules=no` option).
>>  
>> +--set-upstream::
>> +	If the new URL remote is correct, pull and add upstream (tracking) 
>> +	reference, used by argument-less linkgit:git-push[1] and other commands.
>
> git-push and other commands?

I think this is taken from the documentation of --set-upstream for push,
which says:

-u::
--set-upstream::
	For every branch that is up to date or successfully pushed, add
	upstream (tracking) reference, used by argument-less
	linkgit:git-pull[1] and other commands. For more information,
	see `branch.<name>.merge` in linkgit:git-config[1].

Probably the reasoning was to make a symmetry between "git push
--set-upstream", which mentions "pull" in the doc, and the new "git pull
--set-upstream". However, I do not think there should be such symmetry:

Actually, the way I see it, the notion of uptream (i.e.
branch.<branch>.remote and branch.<branch>.merge) is primarily about
"pull" and friends, and "push" happens to use it also by default. But
when branch.<branch>.pushRemote is set, upstream is really about
pulling, and pushing goes to the pushRemote.

>> +	/* TODO: remove debug trace */
>
> Perhaps do so before sending it out for the review?

Yes. This is WIP for now, but it's time to get closer to a real patch,
and these debug statements are counter-productive for that.

>> +	test_must_be_empty merge.$1
>> +}
>
> If this wanted to say "It is OK for the variable to be missing, and
> it also is OK for the variable to have an empty string as its value;
> all other cases are unacceptable",

Actually, I don't think the "present but empty" case makes sense here,
so just test_must_fail git config "$1" should do the trick.

I agree with all other remarks.

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

[Matthieu Moy]

BOMPARD CORENTIN p1603631 <corentin.bompard@etu.univ-lyon1.fr> writes:

> +			warning(_("No source branch found. \n You need to specify excatly "
> +						"one branch with the set-upstream option."));

s/excatly/exactly/

Also, this " \n " is weird, the trailing whitespace is useless, and the
leading one on the next line is weird. You can use the \n to split the
string in the source without risking space issues:

			warning(_("no source branch found.\n"
                                  "You need to specify excatly one branch with the set-upstream option."));

?

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@univ-lyon1.fr> writes:

> -u::
> --set-upstream::
> 	For every branch that is up to date or successfully pushed, add
> 	upstream (tracking) reference, used by argument-less
> 	linkgit:git-pull[1] and other commands. For more information,
> 	see `branch.<name>.merge` in linkgit:git-config[1].
>
> Probably the reasoning was to make a symmetry between "git push
> --set-upstream", which mentions "pull" in the doc, and the new "git pull
> --set-upstream". However, I do not think there should be such symmetry:

Yeah, if "git push --set-upstream" affects the settings that is used
by "git pull", then the above description is good.  Does this new
"git pull --set-upstream" affect the settings used by "git push"?  I
somehow did not think so.  It records the remote and branch used by
this particular "git pull" invocation in branch.<name>.{remote,merge}
for use by future uses of "git pull", right?

Re: [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@univ-lyon1.fr> writes:
>
>> -u::
>> --set-upstream::
>> 	For every branch that is up to date or successfully pushed, add
>> 	upstream (tracking) reference, used by argument-less
>> 	linkgit:git-pull[1] and other commands. For more information,
>> 	see `branch.<name>.merge` in linkgit:git-config[1].
>>
>> Probably the reasoning was to make a symmetry between "git push
>> --set-upstream", which mentions "pull" in the doc, and the new "git pull
>> --set-upstream". However, I do not think there should be such symmetry:
>
> Yeah, if "git push --set-upstream" affects the settings that is used
> by "git pull", then the above description is good.  Does this new
> "git pull --set-upstream" affect the settings used by "git push"?  I
> somehow did not think so.  It records the remote and branch used by
> this particular "git pull" invocation in branch.<name>.{remote,merge}
> for use by future uses of "git pull", right?

It also affects push, in the absence of a branch.<name>.pushRemote
setting (branch.<name>.remote will be used).

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

[Corentin BOMPARD]

>Corentin BOMPARD <corentin.bompard@etu.univ-lyon1.fr> writes:
>
>> Add the --set-upstream option to git pull/fetch
>> which lets the user set the upstream configuration
>> for the current branch.
>
> I think it is a good idea to mention what you exactly mean by "the
> upstream configuration" here.  
>
> Do you mean the "branch.<current-branch-name>.merge" configuration
> variable?

The upstream configuration means the branch.<current-branch-name>.merge 
and branch.<current-branch-name>.remote

>> +             /*
>> +              * 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 waring.
>> +              */
>> +             for (rm = ref_map; rm; rm = rm->next) {
>> +                     fprintf(stderr, "\n -%s", rm->name);
>> +                     if (rm->peer_ref) {
>> +                             fprintf(stderr, " -> %s", rm->peer_ref->name);
>> +                     } else {
>> +                             if (source_ref) {
>> +                                     fprintf(stderr, " -> FETCH_HEAD\n");
>> +                                     warning(_("Multiple branch detected, incompatible with set-upstream"));
>
> Shouldn't this be diagnosed as an error and stop the "fetch" or
> "pull", though?

We can actually replace the warning with a die, but we think it's too harsh on the user, 
and if the warning is showing the upstream stays the same.

We fixed the spotted bugs/mistakes.

The fixed patch will follow.

[PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

[Corentin BOMPARD]

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.

For example a typical use-case like

    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 fonctionality works like git 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 <matthieu.moy@univ-lyon1.fr>
---
 Documentation/fetch-options.txt |   6 ++
 builtin/fetch.c                 |  49 +++++++++++-
 builtin/pull.c                  |   6 ++
 t/t5553-set-upstream.sh         | 137 ++++++++++++++++++++++++++++++++
 4 files changed, 197 insertions(+), 1 deletion(-)
 create mode 100755 t/t5553-set-upstream.sh

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index fa0a3151b..a00705ea4 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -165,6 +165,12 @@ ifndef::git-pull[]
 	Disable recursive fetching of submodules (this has the same effect as
 	using the `--recurse-submodules=no` option).
 
+--set-upstream::
+	If the new URL remote is correct, pull and add upstream (tracking) 
+	reference, used by argument-less linkgit:git-push[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 b620fd54b..760630f11 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"
 
 static const char * const builtin_fetch_usage[] = {
 	N_("git fetch [<options>] [<repository> [<refspec>...]]"),
@@ -46,7 +47,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 tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
 static int max_children = 1;
@@ -113,6 +114,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"),
@@ -1317,6 +1320,50 @@ 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 waring.
+		 */
+		for (rm = ref_map; rm; rm = rm->next) {
+			if (!rm->peer_ref) {
+				if (source_ref) {
+					warning(_("multiple branch detected, incompatible with set-upstream"));
+					source_ref = NULL;
+					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(_("tag upstream not set"));
+			} 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 701d1473d..06d7cddce 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -122,6 +122,7 @@ static char *opt_update_shallow;
 static char *opt_refmap;
 static char *opt_ipv4;
 static char *opt_ipv6;
+static char *set_upstream;
 
 static struct option pull_options[] = {
 	/* Shared options */
@@ -233,6 +234,9 @@ static struct option pull_options[] = {
 	OPT_PASSTHRU('6',  "ipv6", &opt_ipv6, NULL,
 		N_("use IPv6 addresses only"),
 		PARSE_OPT_NOARG),
+	OPT_PASSTHRU(0, "set-upstream", &set_upstream, NULL,
+		N_("set upstream for git pull/fetch"),
+		PARSE_OPT_NOARG),
 
 	OPT_END()
 };
@@ -541,6 +545,8 @@ static int run_fetch(const char *repo, const char **refspecs)
 		argv_array_push(&args, opt_ipv4);
 	if (opt_ipv6)
 		argv_array_push(&args, opt_ipv6);
+	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 000000000..62ff77342
--- /dev/null
+++ b/t/t5553-set-upstream.sh
@@ -0,0 +1,137 @@
+#!/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_empty () {
+	test_expect_code 1 git config "branch.$1.remote" &&
+	test_expect_code 1 git config "branch.$1.merge"
+}
+
+clear_config () {
+	git config --unset "branch.$1.remote" &&
+	git config --unset "branch.$1.merge"
+}
+
+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 &&
+	git remote add up 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' '
+	git checkout master &&
+	git fetch --set-upstream upstream &&
+	check_config_empty master &&
+	check_config_empty other
+'
+
+test_expect_success 'fetch --set-upstream upstream master sets branch master but not other' '
+	git fetch --set-upstream upstream master &&
+	check_config master upstream refs/heads/master &&
+	check_config_empty other
+'
+
+test_expect_success 'fetch --set-upstream upstream other sets branch other' '
+	git fetch --set-upstream upstream other &&
+	check_config master upstream refs/heads/other &&
+	check_config_empty other
+'
+
+test_expect_success 'fetch --set-upstream master:other does not set the branch other2' '
+	git fetch --set-upstream upstream master:other2 &&
+	check_config_empty other2
+'
+
+test_expect_success 'fetch --set-upstream http://nosuchdomain.example.com fails with the bad url' '
+	test_must_fail git fetch --set-upstream http://nosuchdomain.example.com &&
+	check_config master upstream refs/heads/other &&
+	check_config_empty other &&
+	check_config_empty 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' '
+	git pull --set-upstream upstream master &&
+	check_config master upstream refs/heads/master &&
+	check_config_empty other
+'
+
+test_expect_success 'pull --set-upstream master:other2 does not set the branch other2' '
+	git pull --set-upstream upstream master:other2 &&
+	check_config_empty other2
+'
+
+test_expect_success 'pull --set-upstream upstream other sets branch master' '
+	git pull --set-upstream upstream other &&
+	check_config master upstream refs/heads/other &&
+	check_config_empty other
+'
+
+test_expect_success 'pull --set-upstream upstream tag does not set the tag' '
+	git pull --tags --set-upstream upstream three &&
+	check_config_empty three
+'
+
+test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails with the bad url' '
+	test_must_fail git pull --set-upstream http://nosuchdomain.example.com &&
+	check_config master upstream refs/heads/other &&
+	check_config_empty other &&
+	check_config_empty other2 &&
+	check_config_empty three
+'
+
+test_expect_success 'pull --set-upstream upstream HEAD sets branch HEAD' '
+	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 &&
+	git pull --set-upstream upstream master three &&
+	check_config_empty master &&
+	check_config_empty three
+'
+
+test_done
-- 
2.21.0-rc0

Re: [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream

[Matthieu Moy]

BOMPARD CORENTIN p1603631 <corentin.bompard@etu.univ-lyon1.fr> writes:

> Add the --set-upstream option to git pull/fetch

Add _a_?

> which lets the user set the upstream configuration
> (branch.<current-branch-name>.merge and
> branch.<current-branch-name>.remote) for the current branch.
>
> For example a typical use-case like

I don't understand this sentence. Perhaps

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

I'd keep the newline before and after the block of commands, but not
between commands.

> +--set-upstream::
> +	If the new URL remote is correct, pull and add upstream (tracking) 

"URL remote" seems translated literally from french. You probably meant
"remote URL". I'd write "If the remote is fetched successfully, ...".

> +	reference, used by argument-less linkgit:git-push[1] and other commands.

What's your conclusion on the discussion following your previous
submission here? Mine is that git-push is not the best command to
mention here. The setting impacts pull, fetch, push, merge and rebase
(I may have missed others), and to me the main motivation is to impact
pull, so if only one command should be cited, it should be pull.

> +		 * When there are several such branches, consider the request ambiguous and
> +		 * err on the safe side by doing nothing and just emit a waring.

s/waring/warning/

> +			if (!rm->peer_ref) {
> +				if (source_ref) {
> +					warning(_("multiple branch detected, incompatible with set-upstream"));
> +					source_ref = NULL;
> +					goto skip;

"source_ref = NULL" is dead code due to the "goto skip" right below. I'd
remove it.

> +		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(_("tag upstream not set"));

The second warning seems a bit cryptic to me. Why not take the same as
the first, with s/remote-tracking branch/tag/?

> +			warning(_("no source branch found. \n" 

Already noted in previous round: useless trailing whitespace.

> --- /dev/null
> +++ b/t/t5553-set-upstream.sh
> @@ -0,0 +1,137 @@
> +#!/bin/sh
> +
> +test_description='"git fetch/pull --set-upstream" basic tests.
> +
> +'

Don't make $test_description a multi-line string, just close the ' on
the first line.

> +check_config_empty () {

Perhaps name the function check_config_missing instead of ..._empty.

> +test_expect_success 'setup bare parent fetch' '
> +	ensure_fresh_upstream &&
> +	git remote add upstream parent &&
> +	git remote add up parent

I don't think you ever use this "up". Either add a comment explaining
why it's needed, or remove it.

> +test_expect_success 'fetch --set-upstream upstream master sets branch master but not other' '
> +	git fetch --set-upstream upstream master &&
> +	check_config master upstream refs/heads/master &&
> +	check_config_empty other
> +'
> +
> +test_expect_success 'fetch --set-upstream upstream other sets branch other' '
> +	git fetch --set-upstream upstream other &&
> +	check_config master upstream refs/heads/other &&
> +	check_config_empty other
> +'

The first test sets the config for master, and the config is not reset
between tests, so the second may read the config set by "git fetch"
right above, or just a leftover config of the previous test.

You need a "clear_config" in between. Best is to put it at the beginning
of tests so that it's clear that the test does not depend on what has
been executed previously. There are several places where you really need
it. It probably makes sense to use it at the start of every tests for
consistency and future-proof-ness.

> +test_expect_success 'fetch --set-upstream http://nosuchdomain.example.com fails with the bad url' '
> +	test_must_fail git fetch --set-upstream http://nosuchdomain.example.com &&
> +	check_config master upstream refs/heads/other &&
> +	check_config_empty other &&
> +	check_config_empty other2
> +'

It would probably make sense to check what happens when running

  git fetch --set-upstream <some-valid-url>

i.e. use a URL instead of a named remote.

-- 
Matthieu Moy
https://matthieu-moy.fr/