From: Corentin BOMPARD <corentin.bompard@etu.univ-lyon1.fr>
To: <matthieu.moy@univ-lyon1.fr>
Cc: <corentin.bompard@etu.univ-lyon1.fr>, <git@vger.kernel.org>,
<nathan.berbezier@etu.univ-lyon1.fr>,
<pablo.chabanne@etu.univ-lyon1.fr>
Subject: Re: [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream
Date: Tue, 9 Apr 2019 14:52:05 +0200 [thread overview]
Message-ID: <20190409125205.13754-1-corentin.bompard@etu.univ-lyon1.fr> (raw)
In-Reply-To: <86sguxvkrd.fsf@univ-lyon1.fr>
> 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.
next prev parent reply other threads:[~2019-04-09 12:53 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 ` [PATCH] [WIP/RFC] add git pull and git fetch --set-upstream Matthieu Moy
2019-04-09 12:52 ` Corentin BOMPARD [this message]
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=20190409125205.13754-1-corentin.bompard@etu.univ-lyon1.fr \
--to=corentin.bompard@etu.univ-lyon1.fr \
--cc=git@vger.kernel.org \
--cc=matthieu.moy@univ-lyon1.fr \
--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).