git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Matthieu Moy <Matthieu.Moy@univ-lyon1.fr>
To: BOMPARD CORENTIN p1603631 <corentin.bompard@etu.univ-lyon1.fr>
Cc: "git\@vger.kernel.org" <git@vger.kernel.org>,
	"BERBEZIER NATHAN p1601409" <nathan.berbezier@etu.univ-lyon1.fr>,
	CHABANNE PABLO p1602176 <pablo.chabanne@etu.univ-lyon1.fr>
Subject: Re: [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream
Date: Thu, 04 Apr 2019 17:43:34 +0200	[thread overview]
Message-ID: <86sguxvkrd.fsf@univ-lyon1.fr> (raw)
In-Reply-To: <d21d42228425408298da9e99b5877ac9@BPMBX2013-01.univ-lyon1.fr> (BOMPARD CORENTIN's message of "Thu, 4 Apr 2019 12:22:26 +0000")

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/

       reply	other threads:[~2019-04-04 15:43 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <d21d42228425408298da9e99b5877ac9@BPMBX2013-01.univ-lyon1.fr>
2019-04-04 15:43 ` Matthieu Moy [this message]
2019-04-09 12:52   ` [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream Corentin BOMPARD
2019-04-17 16:01     ` Corentin BOMPARD
2019-04-18  1:35       ` Junio C Hamano
2019-04-19 16:00         ` Corentin BOMPARD
2019-04-19 18:42           ` Corentin BOMPARD
     [not found]           ` <f601baa2c2a04ddea4ba32ab25d0dd21@BPMBX2013-01.univ-lyon1.fr>
2019-04-22 10:38             ` Matthieu Moy
2019-08-14 13:46               ` [PATCH] pull, fetch: add --set-upstream option Matthieu Moy
2019-08-14 17:14                 ` Pratyush Yadav
2019-08-19  9:08                   ` Matthieu Moy
2019-08-19  9:11                     ` [PATCH v2] " Matthieu Moy
2019-08-14 17:38                 ` [PATCH] " Junio C Hamano
2019-08-19  9:07                   ` Matthieu Moy
2019-08-19 20:04                     ` Junio C Hamano
2019-08-20  8:09                       ` Matthieu Moy
     [not found]       ` <36559daca9d84f7a91933add734020cd@BPMBX2013-01.univ-lyon1.fr>
2019-04-18  9:51         ` [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream Matthieu Moy
2019-04-19  4:46           ` Junio C Hamano
     [not found]           ` <04f23ebf83bd4aff90ee9ca88cec984e@BPMBX2013-01.univ-lyon1.fr>
2019-04-19  9:44             ` Matthieu Moy
     [not found]     ` <3d2ba75520b74c2e9e8251c41d6632ba@BPMBX2013-01.univ-lyon1.fr>
2019-04-18  9:56       ` Matthieu Moy
2019-04-04 12:22 Corentin BOMPARD

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=86sguxvkrd.fsf@univ-lyon1.fr \
    --to=matthieu.moy@univ-lyon1.fr \
    --cc=corentin.bompard@etu.univ-lyon1.fr \
    --cc=git@vger.kernel.org \
    --cc=nathan.berbezier@etu.univ-lyon1.fr \
    --cc=pablo.chabanne@etu.univ-lyon1.fr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).