Skip to content

fetch: add --[no-]show-forced-updates #273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Documentation/config/advice.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ advice.*::
can tell Git that you do not need help by setting these to 'false':
+
--
fetchShowForcedUpdates::
Advice shown when linkgit:git-fetch[1] takes a long time
to calculate forced updates after ref updates, or to warn
that the check is disabled.
pushUpdateRejected::
Set this variable to 'false' if you want to disable
'pushNonFFCurrent',
Expand Down
5 changes: 5 additions & 0 deletions Documentation/config/fetch.txt
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,8 @@ fetch.negotiationAlgorithm::
Unknown values will cause 'git fetch' to error out.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, SZEDER Gábor wrote (reply to this):

The test '--no-show-forced-updates' in 't5510-fetch.sh' added in
cdbd70c437 (fetch: add --[no-]show-forced-updates argument,
2019-06-18) runs '! test_i18ngrep ...'.  This is wrong, because when
running the test with GIT_TEST_GETTEXT_POISON=true, then
'test_i18ngrep' is basically a noop and always returns with success,
the leading ! turns that into a failure, which then fails the test.

Use 'test_i18ngrep ! ...' instead.

This went unnoticed by our GETTEXT_POISON CI builds, because those
builds don't run this test case: in those builds we don't install
Apache, and this test comes after 't5510' sources 'lib-httpd.sh',
which, consequently, skips all the remaining tests, including this
one.

Signed-off-by: SZEDER Gábor <[email protected]>
---

This is a minor issue in v2.23.0-rc0.

'git grep "! test_i18n"' shows no other similar cases.

 t/t5510-fetch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 139f7106f7..f2481de577 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -997,7 +997,7 @@ test_expect_success '--no-show-forced-updates' '
 	(
 		cd no-forced-update-clone &&
 		git fetch --no-show-forced-updates origin 2>output &&
-		! test_i18ngrep "(forced update)" output
+		test_i18ngrep ! "(forced update)" output
 	)
 '
 
-- 
2.22.0.926.g602b9a0287

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Tue, Jul 30, 2019 at 11:29:15PM +0200, SZEDER Gábor wrote:
> The test '--no-show-forced-updates' in 't5510-fetch.sh' added in
> cdbd70c437 (fetch: add --[no-]show-forced-updates argument,
> 2019-06-18) runs '! test_i18ngrep ...'.  This is wrong, because when
> running the test with GIT_TEST_GETTEXT_POISON=true, then
> 'test_i18ngrep' is basically a noop and always returns with success,
> the leading ! turns that into a failure, which then fails the test.
> 
> Use 'test_i18ngrep ! ...' instead.
> 
> This went unnoticed by our GETTEXT_POISON CI builds, because those
> builds don't run this test case: in those builds we don't install
> Apache, and this test comes after 't5510' sources 'lib-httpd.sh',
> which, consequently, skips all the remaining tests, including this
> one.

Hrm...  It looks like there is nothing httpd-specific in this test
case, at all, so we could run it even if a webserver is not available.
Moving this test case earlier in the script seems to confirm it, as it
still succeeds.

However, I'm not really familiar with this
'--[no-]show-forced-updates' option, and this is not the time to get
up to speed, so I would let Derrick to decide and follow up...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 7/30/2019 5:40 PM, SZEDER Gábor wrote:
> On Tue, Jul 30, 2019 at 11:29:15PM +0200, SZEDER Gábor wrote:
>> The test '--no-show-forced-updates' in 't5510-fetch.sh' added in
>> cdbd70c437 (fetch: add --[no-]show-forced-updates argument,
>> 2019-06-18) runs '! test_i18ngrep ...'.  This is wrong, because when
>> running the test with GIT_TEST_GETTEXT_POISON=true, then
>> 'test_i18ngrep' is basically a noop and always returns with success,
>> the leading ! turns that into a failure, which then fails the test.
>>
>> Use 'test_i18ngrep ! ...' instead.
>>
>> This went unnoticed by our GETTEXT_POISON CI builds, because those
>> builds don't run this test case: in those builds we don't install
>> Apache, and this test comes after 't5510' sources 'lib-httpd.sh',
>> which, consequently, skips all the remaining tests, including this
>> one.
> 
> Hrm...  It looks like there is nothing httpd-specific in this test
> case, at all, so we could run it even if a webserver is not available.
> Moving this test case earlier in the script seems to confirm it, as it
> still succeeds.
> 
> However, I'm not really familiar with this
> '--[no-]show-forced-updates' option, and this is not the time to get
> up to speed, so I would let Derrick to decide and follow up...

I was about to recommend you move the test to before the check for
the changes. I only put the test near the end as I normally don't want
to insert into the middle of a test script.  It makes sense to do so
here.

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, SZEDER Gábor wrote (reply to this):

> Hrm...  It looks like there is nothing httpd-specific in this test
> case, at all, so we could run it even if a webserver is not available.
> Moving this test case earlier in the script seems to confirm it, as it
> still succeeds.

It turns out 't5510' is not the only test script that contains
non-httpd-specific tests after sourcing 'lib-httpd.sh', but 't5703'
did so as well.

The first patch is a follow-up to 'sg/t5510-test-i18ngrep-fix'.
The second patch fixes a similar issue from the v2.19 era.
The last one only adds comments to all the other test scripts sourcing
'lib-httpd.sh' to warn against adding non-httpd-specific tests at the
end, in the hope that it won't happen again.

SZEDER Gábor (3):
  t5510-fetch: run non-httpd-specific test before sourcing
    'lib-httpd.sh'
  t5703: run all non-httpd-specific tests before sourcing 'lib-httpd.sh'
  tests: warn against appending non-httpd-specific tests at the end

 t/t0410-partial-clone.sh           |   3 +
 t/t5500-fetch-pack.sh              |   3 +
 t/t5510-fetch.sh                   |  47 +++----
 t/t5537-fetch-shallow.sh           |   3 +
 t/t5545-push-options.sh            |   3 +
 t/t5601-clone.sh                   |   3 +
 t/t5616-partial-clone.sh           |   3 +
 t/t5700-protocol-v1.sh             |   3 +
 t/t5702-protocol-v2.sh             |   3 +
 t/t5703-upload-pack-ref-in-want.sh | 204 +++++++++++++++--------------
 10 files changed, 153 insertions(+), 122 deletions(-)

-- 
2.22.0.926.g602b9a0287

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, SZEDER Gábor wrote (reply to this):

't5510-fetch.sh' sources 'lib-httpd.sh' near the end to run a
httpd-specific test, but 'lib-httpd.sh' skips all the rest of the test
script if the dependencies for running httpd tests are not fulfilled.
Alas, recently cdbd70c437 (fetch: add --[no-]show-forced-updates
argument, 2019-06-18) appended a non-httpd-specific test at the end,
and this test is then skipped as well when httpd tests can't be run.

Move this new test earlier in the test script, before 'lib-httpd.sh'
is sourced, so it will be run even when httpd tests aren't.

Also add a comment at the end of this test script to warn against
adding non-httpd-specific tests at the end, in the hope that it will
help prevent similar issues in the future.

Signed-off-by: SZEDER Gábor <[email protected]>
---
 t/t5510-fetch.sh | 47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index f2481de577..34b486f1a4 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -902,6 +902,29 @@ test_expect_success C_LOCALE_OUTPUT 'fetch compact output' '
 	test_cmp expect actual
 '
 
+test_expect_success '--no-show-forced-updates' '
+	mkdir forced-updates &&
+	(
+		cd forced-updates &&
+		git init &&
+		test_commit 1 &&
+		test_commit 2
+	) &&
+	git clone forced-updates forced-update-clone &&
+	git clone forced-updates no-forced-update-clone &&
+	git -C forced-updates reset --hard HEAD~1 &&
+	(
+		cd forced-update-clone &&
+		git fetch --show-forced-updates origin 2>output &&
+		test_i18ngrep "(forced update)" output
+	) &&
+	(
+		cd no-forced-update-clone &&
+		git fetch --no-show-forced-updates origin 2>output &&
+		test_i18ngrep ! "(forced update)" output
+	)
+'
+
 setup_negotiation_tip () {
 	SERVER="$1"
 	URL="$2"
@@ -978,27 +1001,7 @@ test_expect_success '--negotiation-tip limits "have" lines sent with HTTP protoc
 	check_negotiation_tip
 '
 
-test_expect_success '--no-show-forced-updates' '
-	mkdir forced-updates &&
-	(
-		cd forced-updates &&
-		git init &&
-		test_commit 1 &&
-		test_commit 2
-	) &&
-	git clone forced-updates forced-update-clone &&
-	git clone forced-updates no-forced-update-clone &&
-	git -C forced-updates reset --hard HEAD~1 &&
-	(
-		cd forced-update-clone &&
-		git fetch --show-forced-updates origin 2>output &&
-		test_i18ngrep "(forced update)" output
-	) &&
-	(
-		cd no-forced-update-clone &&
-		git fetch --no-show-forced-updates origin 2>output &&
-		test_i18ngrep ! "(forced update)" output
-	)
-'
+# DO NOT add non-httpd-specific tests here, because the last part of this
+# test script is only executed when httpd is available and enabled.
 
 test_done
-- 
2.22.0.926.g602b9a0287

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Thu, Aug 01, 2019 at 05:53:09PM +0200, SZEDER Gábor wrote:
> Subject: Re: [PATCH 3/3] tests: warn against appending non-httpd-specific
>  tests at the end

This subject line kind of sucks, doesn't it?!

Alas I had a bit of a hard time coming up with something better.  So
far the best (well, least bad) I could think of is:

  t: warn against adding non-httpd-specific tests after sourcing 'lib-httpd'


> We have a couple of test scripts that are not completely
> httpd-specific, but do run a few httpd-specific tests at the end.
> These test scripts source 'lib-httpd.sh' somewhere mid-script, which
> then skips all the rest of the test script if the dependencies for
> running httpd tests are not fulfilled.
> 
> As the previous two patches in this series show, already on two
> occasions non-httpd-specific tests were appended at the end of such
> test scripts, and, consequently, they were skipped as well when httpd
> tests couldn't be run.
> 
> Add a comment at the end of these test scripts to warn against adding
> non-httpd-specific tests at the end, in the hope that they will help
> prevent similar issues in the future.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 8/1/2019 11:53 AM, SZEDER Gábor wrote:
> 't5510-fetch.sh' sources 'lib-httpd.sh' near the end to run a
> httpd-specific test, but 'lib-httpd.sh' skips all the rest of the test
> script if the dependencies for running httpd tests are not fulfilled.
> Alas, recently cdbd70c437 (fetch: add --[no-]show-forced-updates
> argument, 2019-06-18) appended a non-httpd-specific test at the end,
> and this test is then skipped as well when httpd tests can't be run.
> 
> Move this new test earlier in the test script, before 'lib-httpd.sh'
> is sourced, so it will be run even when httpd tests aren't.
> 
> Also add a comment at the end of this test script to warn against
> adding non-httpd-specific tests at the end, in the hope that it will
> help prevent similar issues in the future.

I appreciate this comment! Not only will it help authors, it will help
reviewers who only see a slice of the test file during review.

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

SZEDER G=C3=A1bor <[email protected]> writes:

> On Thu, Aug 01, 2019 at 05:53:09PM +0200, SZEDER G=C3=A1bor wrote:
>> Subject: Re: [PATCH 3/3] tests: warn against appending non-httpd-speci=
fic
>>  tests at the end
>
> This subject line kind of sucks, doesn't it?!
>
> Alas I had a bit of a hard time coming up with something better.  So
> far the best (well, least bad) I could think of is:
>
>   t: warn against adding non-httpd-specific tests after sourcing 'lib-h=
ttpd'

That reads well.

Thanks.  It must have been tedious to make sure moving tests around
won't break them due to some (hidden) inter-dependency among them.
Very much appreciated.


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Thu, Aug 01, 2019 at 11:18:42AM -0700, Junio C Hamano wrote:
> SZEDER Gábor <[email protected]> writes:
> 
> > On Thu, Aug 01, 2019 at 05:53:09PM +0200, SZEDER Gábor wrote:
> >> Subject: Re: [PATCH 3/3] tests: warn against appending non-httpd-specific
> >>  tests at the end
> >
> > This subject line kind of sucks, doesn't it?!
> >
> > Alas I had a bit of a hard time coming up with something better.  So
> > far the best (well, least bad) I could think of is:
> >
> >   t: warn against adding non-httpd-specific tests after sourcing 'lib-httpd'
> 
> That reads well.

Ok.  Should I resend, or will you amend? (I see that 'pu' contains the
old subject line).

> Thanks.  It must have been tedious to make sure moving tests around
> won't break them due to some (hidden) inter-dependency among them.
> Very much appreciated.

Luckily, all the moved non-httpd tests and all the httpd tests in
't5510' and 't5703' work in their own directories, meaning that they
neither influence other tests nor are influenced by other tests, with
the exception of that $LOCAL_PRISTINE directory that I noted in the
commit message of patch 2.

I did actually diff-ed the output of the involved tests before and
after these patches, and they were essentially identical (the only
differences were that extra 'rm -rf', a couple of different
timestamps, different commit oids shown by 'git commit' or 'git
fetch', and the occasional races between the trace output and actual
command output).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

SZEDER G=C3=A1bor <[email protected]> writes:

>> >   t: warn against adding non-httpd-specific tests after sourcing 'li=
b-httpd'
>>=20
>> That reads well.
>
> Ok.  Should I resend, or will you amend? (I see that 'pu' contains the
> old subject line).

I do not think the original was too bad, either, but I'll amend;
thanks for being extra careful.

+
See also the `--negotiation-tip` option for linkgit:git-fetch[1].

fetch.showForcedUpdates::
Set to false to enable `--no-show-forced-updates` in
linkgit:git-fetch[1] and linkgit:git-pull[1] commands.
Defaults to true.
13 changes: 13 additions & 0 deletions Documentation/fetch-options.txt
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,19 @@ endif::git-pull[]
When multiple `--server-option=<option>` are given, they are all
sent to the other side in the order listed on the command line.

--show-forced-updates::
By default, git checks if a branch is force-updated during
fetch. This can be disabled through fetch.showForcedUpdates, but
the --show-forced-updates option guarantees this check occurs.
See linkgit:git-config[1].

--no-show-forced-updates::
By default, git checks if a branch is force-updated during
fetch. Pass --no-show-forced-updates or set fetch.showForcedUpdates
to false to skip this check for performance reasons. If used during
'git-pull' the --ff-only option will still check for forced updates
before attempting a fast-forward update. See linkgit:git-config[1].

-4::
--ipv4::
Use IPv4 addresses only, ignoring IPv6 addresses.
Expand Down
2 changes: 2 additions & 0 deletions advice.c
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "color.h"
#include "help.h"

int advice_fetch_show_forced_updates = 1;
int advice_push_update_rejected = 1;
int advice_push_non_ff_current = 1;
int advice_push_non_ff_matching = 1;
Expand Down Expand Up @@ -59,6 +60,7 @@ static struct {
const char *name;
int *preference;
} advice_config[] = {
{ "fetchShowForcedUpdates", &advice_fetch_show_forced_updates },
{ "pushUpdateRejected", &advice_push_update_rejected },
{ "pushNonFFCurrent", &advice_push_non_ff_current },
{ "pushNonFFMatching", &advice_push_non_ff_matching },
Expand Down
1 change: 1 addition & 0 deletions advice.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include "git-compat-util.h"

extern int advice_fetch_show_forced_updates;
extern int advice_push_update_rejected;
extern int advice_push_non_ff_current;
extern int advice_push_non_ff_matching;
Expand Down
34 changes: 33 additions & 1 deletion builtin/fetch.c
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
#include "list-objects-filter-options.h"
#include "commit-reach.h"

#define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)

static const char * const builtin_fetch_usage[] = {
N_("git fetch [<options>] [<repository> [<refspec>...]]"),
N_("git fetch [<options>] <group>"),
Expand All @@ -39,6 +41,8 @@ enum {
};

static int fetch_prune_config = -1; /* unspecified */
static int fetch_show_forced_updates = 1;
static uint64_t forced_updates_ms = 0;
static int prune = -1; /* unspecified */
#define PRUNE_BY_DEFAULT 0 /* do we prune by default? */

Expand Down Expand Up @@ -79,6 +83,11 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
return 0;
}

if (!strcmp(k, "fetch.showforcedupdates")) {
fetch_show_forced_updates = git_config_bool(k, v);
return 0;
}

if (!strcmp(k, "submodule.recurse")) {
int r = git_config_bool(k, v) ?
RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
Expand Down Expand Up @@ -169,6 +178,8 @@ static struct option builtin_fetch_options[] = {
OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
N_("report that we have only objects reachable from this object")),
OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
OPT_BOOL(0, "show-forced-updates", &fetch_show_forced_updates,
N_("check for forced-updates on all updated branches")),
OPT_END()
};

Expand Down Expand Up @@ -699,6 +710,7 @@ static int update_local_ref(struct ref *ref,
enum object_type type;
struct branch *current_branch = branch_get(NULL);
const char *pretty_ref = prettify_refname(ref->name);
int fast_forward = 0;

type = oid_object_info(the_repository, &ref->new_oid, NULL);
if (type < 0)
Expand Down Expand Up @@ -773,9 +785,18 @@ static int update_local_ref(struct ref *ref,
return r;
}

if (in_merge_bases(current, updated)) {
if (fetch_show_forced_updates) {
uint64_t t_before = getnanotime();
fast_forward = in_merge_bases(current, updated);
forced_updates_ms += (getnanotime() - t_before) / 1000000;
} else {
fast_forward = 1;
}

if (fast_forward) {
struct strbuf quickref = STRBUF_INIT;
int r;

strbuf_add_unique_abbrev(&quickref, &current->object.oid, DEFAULT_ABBREV);
strbuf_addstr(&quickref, "..");
strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
Expand Down Expand Up @@ -971,6 +992,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
" 'git remote prune %s' to remove any old, conflicting "
"branches"), remote_name);

if (advice_fetch_show_forced_updates) {
if (!fetch_show_forced_updates) {
warning(_("Fetch normally indicates which branches had a forced update, but that check has been disabled."));
warning(_("To re-enable, use '--show-forced-updates' flag or run 'git config fetch.showForcedUpdates true'."));
} else if (forced_updates_ms > FORCED_UPDATES_DELAY_WARNING_IN_MS) {
warning(_("It took %.2f seconds to check forced updates. You can use '--no-show-forced-updates'\n"),
forced_updates_ms / 1000.0);
warning(_("or run 'git config fetch.showForcedUpdates false' to avoid this check.\n"));
}
}

abort:
strbuf_release(&note);
free(url);
Expand Down
7 changes: 7 additions & 0 deletions builtin/pull.c
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ static char *opt_update_shallow;
static char *opt_refmap;
static char *opt_ipv4;
static char *opt_ipv6;
static int opt_show_forced_updates = -1;

static struct option pull_options[] = {
/* Shared options */
Expand Down Expand Up @@ -240,6 +241,8 @@ static struct option pull_options[] = {
OPT_PASSTHRU('6', "ipv6", &opt_ipv6, NULL,
N_("use IPv6 addresses only"),
PARSE_OPT_NOARG),
OPT_BOOL(0, "show-forced-updates", &opt_show_forced_updates,
N_("check for forced-updates on all updated branches")),

OPT_END()
};
Expand Down Expand Up @@ -549,6 +552,10 @@ static int run_fetch(const char *repo, const char **refspecs)
argv_array_push(&args, opt_ipv4);
if (opt_ipv6)
argv_array_push(&args, opt_ipv6);
if (opt_show_forced_updates > 0)
argv_array_push(&args, "--show-forced-updates");
else if (opt_show_forced_updates == 0)
argv_array_push(&args, "--no-show-forced-updates");

if (repo) {
argv_array_push(&args, repo);
Expand Down
23 changes: 23 additions & 0 deletions t/t5510-fetch.sh
Original file line number Diff line number Diff line change
Expand Up @@ -978,4 +978,27 @@ test_expect_success '--negotiation-tip limits "have" lines sent with HTTP protoc
check_negotiation_tip
'

test_expect_success '--no-show-forced-updates' '
mkdir forced-updates &&
(
cd forced-updates &&
git init &&
test_commit 1 &&
test_commit 2
) &&
git clone forced-updates forced-update-clone &&
git clone forced-updates no-forced-update-clone &&
git -C forced-updates reset --hard HEAD~1 &&
(
cd forced-update-clone &&
git fetch --show-forced-updates origin 2>output &&
test_i18ngrep "(forced update)" output
) &&
(
cd no-forced-update-clone &&
git fetch --no-show-forced-updates origin 2>output &&
! test_i18ngrep "(forced update)" output
)
'

test_done