From: Junio C Hamano <gitster@pobox.com>
To: Corentin BOMPARD <corentin.bompard@etu.univ-lyon1.fr>
Cc: <git@vger.kernel.org>, <matthieu.moy@univ-lyon1.fr>,
<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: Thu, 18 Apr 2019 10:35:48 +0900 [thread overview]
Message-ID: <xmqq7ebshz7v.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190417160138.6114-1-corentin.bompard@etu.univ-lyon1.fr> (Corentin BOMPARD's message of "Wed, 17 Apr 2019 18:01:38 +0200")
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.
next prev parent reply other threads:[~2019-04-18 1:35 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
2019-04-17 16:01 ` Corentin BOMPARD
2019-04-18 1:35 ` Junio C Hamano [this message]
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=xmqq7ebshz7v.fsf@gitster-ct.c.googlers.com \
--to=gitster@pobox.com \
--cc=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).