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

[Matthieu Moy <Matthieu.Moy@univ-lyon1.fr>]

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/