From: Junio C Hamano <gitster@pobox•com>
To: "Øystein Walle" <oystwa@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] fetch: add configuration for making --all default
Date: Fri, 17 Jul 2015 09:08:40 -0700 [thread overview]
Message-ID: <xmqqk2ty3gxj.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1437139899-6210-1-git-send-email-oystwa@gmail.com> ("Øystein Walle"'s message of "Fri, 17 Jul 2015 15:31:39 +0200")
Øystein Walle <oystwa@gmail•com> writes:
> Fetching from all remotes by default is useful if you're working on a
> repo with few and/or fast remotes.
That part is sensible.
> It also lets you fetch from origin
> even if the current branch's upstream is elsewhere without specifying it
> explicitly.
I do not think this description is necessary or even beneficial.
The users can already do that with 'fetch --all', so there is
no value added by having a new configuration.
The only thing a new configuration gives the users is a convenience
of not having to say `--all` from the command line.
> diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
> index e62d9a0..584f3fb 100644
> --- a/Documentation/git-fetch.txt
> +++ b/Documentation/git-fetch.txt
> @@ -36,7 +36,8 @@ there is a remotes.<group> entry in the configuration file.
> (See linkgit:git-config[1]).
>
> When no remote is specified, by default the `origin` remote will be used,
> -unless there's an upstream branch configured for the current branch.
> +unless there's an upstream branch configured for the current branch, or the
> +`fetch.all` configuration variable is set to true.
This sounds a bit strange. Other places talk about fetch.all being
the same as giving `--all`, but neither the original description nor
the resulting text with this patch says anything about what would
happen 'when no remote is given but the --all option is given', which
makes it sound as if fetch.all and `--all` behave differently here.
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> ...
> @@ -1168,7 +1174,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> git_config(submodule_config, NULL);
> }
>
> - if (all) {
> + if (all == 1) {
> if (argc == 1)
> die(_("fetch --all does not take a repository argument"));
> else if (argc > 1)
> @@ -1176,9 +1182,14 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> (void) for_each_remote(get_one_remote_for_fetch, &list);
> result = fetch_multiple(&list);
> } else if (argc == 0) {
> - /* No arguments -- use default remote */
> - remote = remote_get(NULL);
> - result = fetch_one(remote, argc, argv);
> + if (fetch_all_config && all != 0) {
> + (void) for_each_remote(get_one_remote_for_fetch, &list);
> + result = fetch_multiple(&list);
Can't you rearrange if/else cascade around here, so that these two
lines do not have to be duplicated?
> + } else {
> + /* No arguments and no --all -- use default remote */
> + remote = remote_get(NULL);
> + result = fetch_one(remote, argc, argv);
> + }
> } else if (multiple) {
> /* All arguments are assumed to be remotes or groups */
> for (i = 0; i < argc; i++)
> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
> index 4b4b667..4e773ee 100755
> --- a/t/t5514-fetch-multiple.sh
> +++ b/t/t5514-fetch-multiple.sh
> @@ -55,6 +55,18 @@ test_expect_success 'git fetch --all' '
> test_cmp expect output)
> '
>
> +test_expect_success 'git fetch (fetch.all = true)' '
> + (git clone one test9 &&
> + cd test9 &&
> + git config fetch.all true &&
> + git remote add one ../one &&
> + git remote add two ../two &&
> + git remote add three ../three &&
> + git fetch &&
> + git branch -r > output &&
> + test_cmp ../test/expect output)
> +'
> +
> test_expect_success 'git fetch --all should continue if a remote has errors' '
> (git clone one test2 &&
> cd test2 &&
> @@ -91,6 +103,17 @@ test_expect_success 'git fetch --multiple (but only one remote)' '
> test_cmp ../expect output)
> '
>
> +test_expect_success 'git fetch one (fetch.all = true)' '
> + (cd test3 &&
This means that this test piece will break if the previous one did
not succeed, which is not a very good idea.
> + git config fetch.all true &&
> + git fetch three &&
> + git branch -r > output &&
> + test_cmp ../expect output &&
What future errors will this catch?
You have only "three" and "origin" and the expected output wants to
see tracking from both. Does the test break when in some future
somebody broke the resulting code after applying this patch in such
a way that "fetch from only 'three'" from the command line gets
ignored and fetch.all went ahead and fetched from everybody?
> + git fetch --no-all &&
> + git branch -r > output &&
> + test_cmp ../expect output)
Same question.
> +'
> +
> cat > expect << EOF
> one/master
> one/side
prev parent reply other threads:[~2015-07-17 16:08 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-17 13:31 [PATCH] fetch: add configuration for making --all default Øystein Walle
2015-07-17 15:02 ` Remi Galan Alfonso
2015-07-17 16:14 ` [PATCH v2] " Øystein Walle
2015-07-17 16:08 ` Junio C Hamano [this message]
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqk2ty3gxj.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=oystwa@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox