[PATCH 0/3] fetch: add --[no-]show-forced-updates

[PATCH 0/3] fetch: add --[no-]show-forced-updates

[Derrick Stolee via GitGitGadget]

The git fetch builtin includes a calculation to see if the ref values were
forced updates (i.e. if the old value is not in the history of the new
value). In a repo with many refs and a fast-moving history, this calculation
can be very slow.

This series adds a new --[no-]show-forced-updates option to 'git fetch' to
avoid this calculation when requested. Further:

 1. Add a new fetch.showForcedUpdates config setting that provides a default
    value for --[no-]show-forced-updates.
    
    
 2. Add a new advice.fetchShowForcedUpdates config setting associated with a
    warning that suggests fetch.showForcedUpdates when the calculation takes
    a long time, or warns about the lack of "(forced update)" messages.
    
    
 3. Add the command-line options to 'git pull'.
    
    

We have been running with these commits in microsoft/git for a while now and
no user has complained that the messages were removed (VFS for Git sets 
fetch.showForcedUpdates=false by default). Users have complained about the
warning messages always appearing, so this series includes the advice config
setting. Further, I added a test to demonstrate the behavior change between
the two options.

The fetch.showForcedUpdates config setting is a candidate to be added to the
proposed "large repo" meta-config setting previously discussed [1]. I'm
putting this out for independent review as it is much smaller compared to
the potential of that series.

Thanks, -Stolee

[1] https://public-inbox.org/git/pull.254.git.gitgitgadget@gmail.com/

Derrick Stolee (3):
  fetch: add --[no-]show-forced-updates argument
  fetch: warn about forced updates in branch listing
  pull: add --[no-]show-forced-updates passthrough

 Documentation/config/advice.txt |  4 ++++
 Documentation/config/fetch.txt  |  5 +++++
 Documentation/fetch-options.txt | 13 +++++++++++++
 advice.c                        |  2 ++
 advice.h                        |  1 +
 builtin/fetch.c                 | 34 ++++++++++++++++++++++++++++++++-
 builtin/pull.c                  |  7 +++++++
 t/t5510-fetch.sh                | 23 ++++++++++++++++++++++
 8 files changed, 88 insertions(+), 1 deletion(-)


base-commit: b697d92f56511e804b8ba20ccbe7bdc85dc66810
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-273%2Fderrickstolee%2Fshow-forced-updates-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-273/derrickstolee/show-forced-updates-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/273
-- 
gitgitgadget

[PATCH 1/3] fetch: add --[no-]show-forced-updates argument

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

After updating a set of remove refs during a 'git fetch', we walk the
commits in the new ref value and not in the old ref value to discover
if the update was a forced update. This results in two things happening
during the command:

 1. The line including the ref update has an additional "(forced-update)"
    marker at the end.

 2. The ref log for that remote branch includes a bit saying that update
    is a forced update.

For many situations, this forced-update message happens infrequently, or
is a small bit of information among many ref updates. Many users ignore
these messages, but the calculation required here slows down their fetches
significantly. Keep in mind that they do not have the opportunity to
calculate a commit-graph file containing the newly-fetched commits, so
these comparisons can be very slow.

Add a '--[no-]show-forced-updates' option that allows a user to skip this
calculation. The only permanent result is dropping the forced-update bit
in the reflog.

Include a new fetch.showForcedUpdates config setting that allows this
behavior without including the argument in every command. The config
setting is overridden by the command-line arguments.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/config/fetch.txt  |  5 +++++
 Documentation/fetch-options.txt | 13 +++++++++++++
 builtin/fetch.c                 | 11 ++++++++++-
 t/t5510-fetch.sh                | 23 +++++++++++++++++++++++
 4 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index cbfad6cdbb..ba890b5884 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -63,3 +63,8 @@ fetch.negotiationAlgorithm::
 	Unknown values will cause 'git fetch' to error out.
 +
 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.
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 91c47752ec..5801d23ae4 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -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.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4ba63d5ac6..571c255218 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -39,6 +39,7 @@ enum {
 };
 
 static int fetch_prune_config = -1; /* unspecified */
+static int fetch_show_forced_updates = 1;
 static int prune = -1; /* unspecified */
 #define PRUNE_BY_DEFAULT 0 /* do we prune by default? */
 
@@ -79,6 +80,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;
@@ -169,6 +175,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()
 };
 
@@ -773,9 +781,10 @@ static int update_local_ref(struct ref *ref,
 		return r;
 	}
 
-	if (in_merge_bases(current, updated)) {
+	if (!fetch_show_forced_updates || in_merge_bases(current, updated)) {
 		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);
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index e98d90dd9b..139f7106f7 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -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
-- 
gitgitgadget

[PATCH 2/3] fetch: warn about forced updates in branch listing

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The --[no-]show-forced-updates option in 'git fetch' can be confusing
for some users, especially if it is enabled via config setting and not
by argument. Add advice to warn the user that the (forced update)
messages were not listed.

Additionally, warn users when the forced update check takes longer
than ten seconds, and recommend that they disable the check. These
messages can be disabled by the advice.fetchShowForcedUpdates config
setting.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/config/advice.txt |  4 ++++
 advice.c                        |  2 ++
 advice.h                        |  1 +
 builtin/fetch.c                 | 25 ++++++++++++++++++++++++-
 4 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index ec4f6ae658..1f1e847fb4 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -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',
diff --git a/advice.c b/advice.c
index ce5f374ecd..4b283be51a 100644
--- a/advice.c
+++ b/advice.c
@@ -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;
@@ -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 },
diff --git a/advice.h b/advice.h
index e50f02cdfe..495e53255c 100644
--- a/advice.h
+++ b/advice.h
@@ -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;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 571c255218..cf7eb0dd8d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -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>"),
@@ -40,6 +42,7 @@ 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? */
 
@@ -707,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)
@@ -781,7 +785,15 @@ static int update_local_ref(struct ref *ref,
 		return r;
 	}
 
-	if (!fetch_show_forced_updates || 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;
 
@@ -980,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);
-- 
gitgitgadget

[PATCH 3/3] pull: add --[no-]show-forced-updates passthrough

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The 'git fetch' command can avoid calculating forced updates, so
allow users of 'git pull' to provide that option. This is particularly
necessary when the advice to use '--no-show-forced-updates' is given
at the end of the command.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/pull.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 9dd32a115b..f1eaf6e6ed 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -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 */
@@ -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()
 };
@@ -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);
-- 
gitgitgadget

[PATCH] t5510-fetch: fix negated 'test_i18ngrep' invocation

[SZEDER Gábor]

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 <szeder.dev@gmail.com>
---

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

Re: [PATCH] t5510-fetch: fix negated 'test_i18ngrep' invocation

[SZEDER Gábor]

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...

Re: [PATCH] t5510-fetch: fix negated 'test_i18ngrep' invocation

[Derrick Stolee]

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

[PATCH 0/3] tests: run non-httpd-specific tests before sourcing 'lib-httpd.sh'

[SZEDER Gábor]

> 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

[PATCH 1/3] t5510-fetch: run non-httpd-specific test before sourcing 'lib-httpd.sh'

[SZEDER Gábor]

'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 <szeder.dev@gmail.com>
---
 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

[PATCH 2/3] t5703: run all non-httpd-specific tests before sourcing 'lib-httpd.sh'

[SZEDER Gábor]

't5703-upload-pack-ref-in-want.sh' sources 'lib-httpd.sh' near the end
to run a couple of httpd-specific tests, but 'lib-httpd.sh' skips all
the rest of the test script if the dependencies for running httpd
tests are not fulfilled.  However, the last six tests in 't5703' are
not httpd-specific, but they are skipped as well when httpd tests
can't be run.

Move these six tests earlier in the test script, before 'lib-httpd.sh'
is sourced, so they will be run even when httpd tests aren't.  Note
that this is not merely a pure code movement, because the setup test
case for the httpd tests needed an additional 'rm -rf
"$LOCAL_PRISTINE"' to clean up a directory left behind by the moved
non-httpd-specific tests.

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 <szeder.dev@gmail.com>
---
 t/t5703-upload-pack-ref-in-want.sh | 204 +++++++++++++++--------------
 1 file changed, 104 insertions(+), 100 deletions(-)

diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index de4b6106ef..3a2c143c6d 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -157,106 +157,6 @@ test_expect_success 'want-ref with ref we already have commit for' '
 	check_output
 '
 
-. "$TEST_DIRECTORY"/lib-httpd.sh
-start_httpd
-
-REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
-LOCAL_PRISTINE="$(pwd)/local_pristine"
-
-test_expect_success 'setup repos for change-while-negotiating test' '
-	(
-		git init "$REPO" &&
-		cd "$REPO" &&
-		>.git/git-daemon-export-ok &&
-		test_commit m1 &&
-		git tag -d m1 &&
-
-		# Local repo with many commits (so that negotiation will take
-		# more than 1 request/response pair)
-		git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo" "$LOCAL_PRISTINE" &&
-		cd "$LOCAL_PRISTINE" &&
-		git checkout -b side &&
-		test_commit_bulk --id=s 33 &&
-
-		# Add novel commits to upstream
-		git checkout master &&
-		cd "$REPO" &&
-		test_commit m2 &&
-		test_commit m3 &&
-		git tag -d m2 m3
-	) &&
-	git -C "$LOCAL_PRISTINE" remote set-url origin "http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo" &&
-	git -C "$LOCAL_PRISTINE" config protocol.version 2
-'
-
-inconsistency () {
-	# Simulate that the server initially reports $2 as the ref
-	# corresponding to $1, and after that, $1 as the ref corresponding to
-	# $1. This corresponds to the real-life situation where the server's
-	# repository appears to change during negotiation, for example, when
-	# different servers in a load-balancing arrangement serve (stateless)
-	# RPCs during a single negotiation.
-	printf "s/%s/%s/" \
-	       $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
-	       $(git -C "$REPO" rev-parse $2 | tr -d "\n") \
-	       >"$HTTPD_ROOT_PATH/one-time-sed"
-}
-
-test_expect_success 'server is initially ahead - no ref in want' '
-	git -C "$REPO" config uploadpack.allowRefInWant false &&
-	rm -rf local &&
-	cp -r "$LOCAL_PRISTINE" local &&
-	inconsistency master 1234567890123456789012345678901234567890 &&
-	test_must_fail git -C local fetch 2>err &&
-	test_i18ngrep "fatal: remote error: upload-pack: not our ref" err
-'
-
-test_expect_success 'server is initially ahead - ref in want' '
-	git -C "$REPO" config uploadpack.allowRefInWant true &&
-	rm -rf local &&
-	cp -r "$LOCAL_PRISTINE" local &&
-	inconsistency master 1234567890123456789012345678901234567890 &&
-	git -C local fetch &&
-
-	git -C "$REPO" rev-parse --verify master >expected &&
-	git -C local rev-parse --verify refs/remotes/origin/master >actual &&
-	test_cmp expected actual
-'
-
-test_expect_success 'server is initially behind - no ref in want' '
-	git -C "$REPO" config uploadpack.allowRefInWant false &&
-	rm -rf local &&
-	cp -r "$LOCAL_PRISTINE" local &&
-	inconsistency master "master^" &&
-	git -C local fetch &&
-
-	git -C "$REPO" rev-parse --verify "master^" >expected &&
-	git -C local rev-parse --verify refs/remotes/origin/master >actual &&
-	test_cmp expected actual
-'
-
-test_expect_success 'server is initially behind - ref in want' '
-	git -C "$REPO" config uploadpack.allowRefInWant true &&
-	rm -rf local &&
-	cp -r "$LOCAL_PRISTINE" local &&
-	inconsistency master "master^" &&
-	git -C local fetch &&
-
-	git -C "$REPO" rev-parse --verify "master" >expected &&
-	git -C local rev-parse --verify refs/remotes/origin/master >actual &&
-	test_cmp expected actual
-'
-
-test_expect_success 'server loses a ref - ref in want' '
-	git -C "$REPO" config uploadpack.allowRefInWant true &&
-	rm -rf local &&
-	cp -r "$LOCAL_PRISTINE" local &&
-	echo "s/master/raster/" >"$HTTPD_ROOT_PATH/one-time-sed" &&
-	test_must_fail git -C local fetch 2>err &&
-
-	test_i18ngrep "fatal: remote error: unknown ref refs/heads/raster" err
-'
-
 REPO="$(pwd)/repo"
 LOCAL_PRISTINE="$(pwd)/local_pristine"
 
@@ -372,4 +272,108 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '
 	grep "want-ref refs/heads/o/bar" log
 '
 
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for change-while-negotiating test' '
+	(
+		git init "$REPO" &&
+		cd "$REPO" &&
+		>.git/git-daemon-export-ok &&
+		test_commit m1 &&
+		git tag -d m1 &&
+
+		# Local repo with many commits (so that negotiation will take
+		# more than 1 request/response pair)
+		rm -rf "$LOCAL_PRISTINE" &&
+		git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo" "$LOCAL_PRISTINE" &&
+		cd "$LOCAL_PRISTINE" &&
+		git checkout -b side &&
+		test_commit_bulk --id=s 33 &&
+
+		# Add novel commits to upstream
+		git checkout master &&
+		cd "$REPO" &&
+		test_commit m2 &&
+		test_commit m3 &&
+		git tag -d m2 m3
+	) &&
+	git -C "$LOCAL_PRISTINE" remote set-url origin "http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo" &&
+	git -C "$LOCAL_PRISTINE" config protocol.version 2
+'
+
+inconsistency () {
+	# Simulate that the server initially reports $2 as the ref
+	# corresponding to $1, and after that, $1 as the ref corresponding to
+	# $1. This corresponds to the real-life situation where the server's
+	# repository appears to change during negotiation, for example, when
+	# different servers in a load-balancing arrangement serve (stateless)
+	# RPCs during a single negotiation.
+	printf "s/%s/%s/" \
+	       $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
+	       $(git -C "$REPO" rev-parse $2 | tr -d "\n") \
+	       >"$HTTPD_ROOT_PATH/one-time-sed"
+}
+
+test_expect_success 'server is initially ahead - no ref in want' '
+	git -C "$REPO" config uploadpack.allowRefInWant false &&
+	rm -rf local &&
+	cp -r "$LOCAL_PRISTINE" local &&
+	inconsistency master 1234567890123456789012345678901234567890 &&
+	test_must_fail git -C local fetch 2>err &&
+	test_i18ngrep "fatal: remote error: upload-pack: not our ref" err
+'
+
+test_expect_success 'server is initially ahead - ref in want' '
+	git -C "$REPO" config uploadpack.allowRefInWant true &&
+	rm -rf local &&
+	cp -r "$LOCAL_PRISTINE" local &&
+	inconsistency master 1234567890123456789012345678901234567890 &&
+	git -C local fetch &&
+
+	git -C "$REPO" rev-parse --verify master >expected &&
+	git -C local rev-parse --verify refs/remotes/origin/master >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'server is initially behind - no ref in want' '
+	git -C "$REPO" config uploadpack.allowRefInWant false &&
+	rm -rf local &&
+	cp -r "$LOCAL_PRISTINE" local &&
+	inconsistency master "master^" &&
+	git -C local fetch &&
+
+	git -C "$REPO" rev-parse --verify "master^" >expected &&
+	git -C local rev-parse --verify refs/remotes/origin/master >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'server is initially behind - ref in want' '
+	git -C "$REPO" config uploadpack.allowRefInWant true &&
+	rm -rf local &&
+	cp -r "$LOCAL_PRISTINE" local &&
+	inconsistency master "master^" &&
+	git -C local fetch &&
+
+	git -C "$REPO" rev-parse --verify "master" >expected &&
+	git -C local rev-parse --verify refs/remotes/origin/master >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'server loses a ref - ref in want' '
+	git -C "$REPO" config uploadpack.allowRefInWant true &&
+	rm -rf local &&
+	cp -r "$LOCAL_PRISTINE" local &&
+	echo "s/master/raster/" >"$HTTPD_ROOT_PATH/one-time-sed" &&
+	test_must_fail git -C local fetch 2>err &&
+
+	test_i18ngrep "fatal: remote error: unknown ref refs/heads/raster" err
+'
+
+# 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

[PATCH 3/3] tests: warn against appending non-httpd-specific tests at the end

[SZEDER Gábor]

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.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t0410-partial-clone.sh | 3 +++
 t/t5500-fetch-pack.sh    | 3 +++
 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 +++
 8 files changed, 24 insertions(+)

diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 5bd892f2f7..6415063980 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -518,4 +518,7 @@ test_expect_success 'fetching of missing objects from an HTTP server' '
 	git verify-pack --verbose "$IDX" | grep "$HASH"
 '
 
+# 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
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 1c71c0ec77..8210f63d41 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -920,4 +920,7 @@ test_expect_success 'fetch with --filter=blob:limit=0 and HTTP' '
 	fetch_filter_blob_limit_zero "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server"
 '
 
+# 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
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 66f0b64d39..97a67728ca 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -255,4 +255,7 @@ test_expect_success 'shallow fetches check connectivity before writing shallow f
 	git -C client fsck
 '
 
+# 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
diff --git a/t/t5545-push-options.sh b/t/t5545-push-options.sh
index 6d1d59c9b1..04b34c4de1 100755
--- a/t/t5545-push-options.sh
+++ b/t/t5545-push-options.sh
@@ -278,4 +278,7 @@ test_expect_success 'push options keep quoted characters intact (http)' '
 	test_cmp expect "$HTTPD_DOCUMENT_ROOT_PATH"/upstream.git/hooks/pre-receive.push_options
 '
 
+# 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
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 37d76808d4..4a3b901f06 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -739,4 +739,7 @@ test_expect_success 'partial clone using HTTP' '
 	partial_clone "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server"
 '
 
+# 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
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index b91ef548f8..565254558f 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -417,4 +417,7 @@ test_expect_success 'tolerate server sending REF_DELTA against missing promisor
 	! test -e "$HTTPD_ROOT_PATH/one-time-sed"
 '
 
+# 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
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index 7c9511c593..2571eb90b7 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -292,4 +292,7 @@ test_expect_success 'push with http:// using protocol v1' '
 	grep "git< version 1" log
 '
 
+# 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
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 011b81d4fc..fbddd0aea9 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -723,4 +723,7 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
 	test_i18ngrep "expected no other sections to be sent after no .ready." err
 '
 
+# 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

Re: [PATCH 3/3] tests: warn against appending non-httpd-specific tests at the end

[SZEDER Gábor]

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.

Re: [PATCH 1/3] t5510-fetch: run non-httpd-specific test before sourcing 'lib-httpd.sh'

[Derrick Stolee]

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

Re: [PATCH 3/3] tests: warn against appending non-httpd-specific tests at the end

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> 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.

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.

Re: [PATCH 3/3] tests: warn against appending non-httpd-specific tests at the end

[SZEDER Gábor]

On Thu, Aug 01, 2019 at 11:18:42AM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> 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).

Re: [PATCH 3/3] tests: warn against appending non-httpd-specific tests at the end

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

>> >   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).

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