[PATCH] negotiator/skipping: skip commits during fetch

[PATCH] negotiator/skipping: skip commits during fetch

[Jonathan Tan]

Introduce a new negotiation algorithm used during fetch that skips
commits in an effort to find common ancestors faster. The skips grow
similarly to the Fibonacci sequence as the commit walk proceeds further
away from the tips. The skips may cause unnecessary commits to be
included in the packfile, but the negotiation step typically ends more
quickly.

Usage of this algorithm is guarded behind the configuration flag
fetch.negotiationAlgorithm.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
This is on jt/fetch-pack-negotiator, but also applies cleanly on
jt/fetch-nego-tip.

A previous version of this had rough backtracking after a "match" is
found, but that backtracking implementation was complicated and inexact,
so I've removed it.

At $DAY_JOB we have been using this patch with some success, so I'm
sending it to this mailing list.
---
 Documentation/config.txt             |   9 +
 Makefile                             |   1 +
 fetch-negotiator.c                   |   8 +-
 fetch-negotiator.h                   |   3 +-
 fetch-pack.c                         |   7 +-
 negotiator/skipping.c                | 250 +++++++++++++++++++++++++++
 negotiator/skipping.h                |   8 +
 t/t5552-skipping-fetch-negotiator.sh | 179 +++++++++++++++++++
 8 files changed, 461 insertions(+), 4 deletions(-)
 create mode 100644 negotiator/skipping.c
 create mode 100644 negotiator/skipping.h
 create mode 100755 t/t5552-skipping-fetch-negotiator.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a9..53443fb9db 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1491,6 +1491,15 @@ fetch.output::
 	`full` and `compact`. Default value is `full`. See section
 	OUTPUT in linkgit:git-fetch[1] for detail.
 
+fetch.negotiationAlgorithm::
+	Control how information about the commits in the local repository is
+	sent when negotiating the contents of the packfile to be sent by the
+	server. Set to "skipping" to use an algorithm that skips commits in an
+	effort to converge faster, but may result in a larger-than-necessary
+	packfile; any other value instructs Git to use the default algorithm
+	that never skips commits (unless the server has acknowledged it or one
+	of its descendants).
+
 format.attach::
 	Enable multipart/mixed attachments as the default for
 	'format-patch'.  The value can also be a double quoted string
diff --git a/Makefile b/Makefile
index 96f84d1dcf..617475622b 100644
--- a/Makefile
+++ b/Makefile
@@ -893,6 +893,7 @@ LIB_OBJS += merge-recursive.o
 LIB_OBJS += mergesort.o
 LIB_OBJS += name-hash.o
 LIB_OBJS += negotiator/default.o
+LIB_OBJS += negotiator/skipping.o
 LIB_OBJS += notes.o
 LIB_OBJS += notes-cache.o
 LIB_OBJS += notes-merge.o
diff --git a/fetch-negotiator.c b/fetch-negotiator.c
index 2675d120fe..5d283049f4 100644
--- a/fetch-negotiator.c
+++ b/fetch-negotiator.c
@@ -1,8 +1,14 @@
 #include "git-compat-util.h"
 #include "fetch-negotiator.h"
 #include "negotiator/default.h"
+#include "negotiator/skipping.h"
 
-void fetch_negotiator_init(struct fetch_negotiator *negotiator)
+void fetch_negotiator_init(struct fetch_negotiator *negotiator,
+			   const char *algorithm)
 {
+	if (algorithm && !strcmp(algorithm, "skipping")) {
+		skipping_negotiator_init(negotiator);
+		return;
+	}
 	default_negotiator_init(negotiator);
 }
diff --git a/fetch-negotiator.h b/fetch-negotiator.h
index b1290aa9c6..ddb44a22dc 100644
--- a/fetch-negotiator.h
+++ b/fetch-negotiator.h
@@ -52,6 +52,7 @@ struct fetch_negotiator {
 	void *data;
 };
 
-void fetch_negotiator_init(struct fetch_negotiator *negotiator);
+void fetch_negotiator_init(struct fetch_negotiator *negotiator,
+			   const char *algorithm);
 
 #endif
diff --git a/fetch-pack.c b/fetch-pack.c
index 1e50d90082..50773fdde3 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -33,6 +33,7 @@ static int agent_supported;
 static int server_supports_filtering;
 static struct lock_file shallow_lock;
 static const char *alternate_shallow_file;
+static char *negotiation_algorithm;
 
 /* Remember to update object flag allocation in object.h */
 #define COMPLETE	(1U << 0)
@@ -905,7 +906,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 	const char *agent_feature;
 	int agent_len;
 	struct fetch_negotiator negotiator;
-	fetch_negotiator_init(&negotiator);
+	fetch_negotiator_init(&negotiator, negotiation_algorithm);
 
 	sort_ref_list(&ref, ref_compare_name);
 	QSORT(sought, nr_sought, cmp_ref_by_name);
@@ -1287,7 +1288,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	int in_vain = 0;
 	int haves_to_send = INITIAL_FLUSH;
 	struct fetch_negotiator negotiator;
-	fetch_negotiator_init(&negotiator);
+	fetch_negotiator_init(&negotiator, negotiation_algorithm);
 	packet_reader_init(&reader, fd[0], NULL, 0,
 			   PACKET_READ_CHOMP_NEWLINE);
 
@@ -1366,6 +1367,8 @@ static void fetch_pack_config(void)
 	git_config_get_bool("repack.usedeltabaseoffset", &prefer_ofs_delta);
 	git_config_get_bool("fetch.fsckobjects", &fetch_fsck_objects);
 	git_config_get_bool("transfer.fsckobjects", &transfer_fsck_objects);
+	git_config_get_string("fetch.negotiationalgorithm",
+			      &negotiation_algorithm);
 
 	git_config(git_default_config, NULL);
 }
diff --git a/negotiator/skipping.c b/negotiator/skipping.c
new file mode 100644
index 0000000000..74d25e8825
--- /dev/null
+++ b/negotiator/skipping.c
@@ -0,0 +1,250 @@
+#include "cache.h"
+#include "skipping.h"
+#include "../commit.h"
+#include "../fetch-negotiator.h"
+#include "../prio-queue.h"
+#include "../refs.h"
+#include "../tag.h"
+
+/* Remember to update object flag allocation in object.h */
+/*
+ * Both us and the server know that both parties have this object.
+ */
+#define COMMON		(1U << 2)
+/*
+ * The server has told us that it has this object. We still need to tell the
+ * server that we have this object (or one of its descendants), but since we are
+ * going to do that, we do not need to tell the server about its ancestors.
+ */
+#define ADVERTISED	(1U << 3)
+/*
+ * This commit has entered the priority queue.
+ */
+#define SEEN		(1U << 4)
+/*
+ * This commit has left the priority queue.
+ */
+#define POPPED		(1U << 5)
+
+static int marked;
+
+/*
+ * An entry in the priority queue.
+ */
+struct entry {
+	struct commit *commit;
+
+	/*
+	 * Used only if commit is not COMMON.
+	 */
+	uint16_t original_ttl;
+	uint16_t ttl;
+};
+
+struct data {
+	struct prio_queue rev_list;
+
+	/*
+	 * The number of non-COMMON commits in rev_list.
+	 */
+	int non_common_revs;
+};
+
+static int compare(const void *a_, const void *b_, void *unused)
+{
+	const struct entry *a = a_;
+	const struct entry *b = b_;
+	return compare_commits_by_commit_date(a->commit, b->commit, NULL);
+}
+
+static struct entry *rev_list_push(struct data *data, struct commit *commit, int mark)
+{
+	struct entry *entry;
+	commit->object.flags |= mark | SEEN;
+
+	entry = xcalloc(1, sizeof(*entry));
+	entry->commit = commit;
+	prio_queue_put(&data->rev_list, entry);
+
+	if (!(mark & COMMON))
+		data->non_common_revs++;
+	return entry;
+}
+
+static int clear_marks(const char *refname, const struct object_id *oid,
+		       int flag, void *cb_data)
+{
+	struct object *o = deref_tag(parse_object(oid), refname, 0);
+
+	if (o && o->type == OBJ_COMMIT)
+		clear_commit_marks((struct commit *)o,
+				   COMMON | ADVERTISED | SEEN | POPPED);
+	return 0;
+}
+
+/*
+ * Mark this SEEN commit and all its SEEN ancestors as COMMON.
+ */
+static void mark_common(struct data *data, struct commit *c)
+{
+	struct commit_list *p;
+
+	if (c->object.flags & COMMON)
+		return;
+	c->object.flags |= COMMON;
+	if (!(c->object.flags & POPPED))
+		data->non_common_revs--;
+
+	if (!c->object.parsed)
+		return;
+	for (p = c->parents; p; p = p->next) {
+		if (p->item->object.flags & SEEN)
+			mark_common(data, p->item);
+	}
+}
+
+/*
+ * Ensure that the priority queue has an entry for to_push, and ensure that the
+ * entry has the correct flags and ttl.
+ *
+ * This function returns 1 if an entry was found or created, and 0 otherwise
+ * (because the entry for this commit had already been popped).
+ */
+static int push_parent(struct data *data, struct entry *entry,
+		       struct commit *to_push)
+{
+	struct entry *parent_entry;
+
+	if (to_push->object.flags & SEEN) {
+		int i;
+		if (to_push->object.flags & POPPED)
+			/*
+			 * The entry for this commit has already been popped,
+			 * due to clock skew. Pretend that this parent does not
+			 * exist.
+			 */
+			return 0;
+		/*
+		 * Find the existing entry and use it.
+		 */
+		for (i = 0; i < data->rev_list.nr; i++) {
+			parent_entry = data->rev_list.array[i].data;
+			if (parent_entry->commit == to_push)
+				goto parent_found;
+		}
+		BUG("missing parent in priority queue");
+parent_found:
+		;
+	} else {
+		parent_entry = rev_list_push(data, to_push, 0);
+	}
+
+	if (entry->commit->object.flags & (COMMON | ADVERTISED)) {
+		mark_common(data, to_push);
+	} else {
+		uint16_t new_original_ttl = entry->ttl
+			? entry->original_ttl : entry->original_ttl * 3 / 2 + 1;
+		uint16_t new_ttl = entry->ttl
+			? entry->ttl - 1 : new_original_ttl;
+		if (parent_entry->original_ttl < new_original_ttl) {
+			parent_entry->original_ttl = new_original_ttl;
+			parent_entry->ttl = new_ttl;
+		}
+	}
+
+	return 1;
+}
+
+static const struct object_id *get_rev(struct data *data)
+{
+	struct commit *to_send = NULL;
+
+	while (to_send == NULL) {
+		struct entry *entry;
+		struct commit *commit;
+		struct commit_list *p;
+		int parent_pushed = 0;
+
+		if (data->rev_list.nr == 0 || data->non_common_revs == 0)
+			return NULL;
+
+		entry = prio_queue_get(&data->rev_list);
+		commit = entry->commit;
+		commit->object.flags |= POPPED;
+		if (!(commit->object.flags & COMMON))
+			data->non_common_revs--;
+
+		if (!(commit->object.flags & COMMON) && !entry->ttl)
+			to_send = commit;
+
+		parse_commit(commit);
+		for (p = commit->parents; p; p = p->next)
+			parent_pushed |= push_parent(data, entry, p->item);
+
+		if (!(commit->object.flags & COMMON) && !parent_pushed)
+			/*
+			 * This commit has no parents, or all of its parents
+			 * have already been popped (due to clock skew), so send
+			 * it anyway.
+			 */
+			to_send = commit;
+
+		free(entry);
+	}
+
+	return &to_send->object.oid;
+}
+
+static void known_common(struct fetch_negotiator *n, struct commit *c)
+{
+	if (c->object.flags & SEEN)
+		return;
+	rev_list_push(n->data, c, ADVERTISED);
+}
+
+static void add_tip(struct fetch_negotiator *n, struct commit *c)
+{
+	n->known_common = NULL;
+	if (c->object.flags & SEEN)
+		return;
+	rev_list_push(n->data, c, 0);
+}
+
+static const struct object_id *next(struct fetch_negotiator *n)
+{
+	n->known_common = NULL;
+	n->add_tip = NULL;
+	return get_rev(n->data);
+}
+
+static int ack(struct fetch_negotiator *n, struct commit *c)
+{
+	int known_to_be_common = !!(c->object.flags & COMMON);
+	if (!(c->object.flags & SEEN))
+		die("received ack for commit %s not sent as 'have'\n",
+		    oid_to_hex(&c->object.oid));
+	mark_common(n->data, c);
+	return known_to_be_common;
+}
+
+static void release(struct fetch_negotiator *n)
+{
+	clear_prio_queue(&((struct data *)n->data)->rev_list);
+	FREE_AND_NULL(n->data);
+}
+
+void skipping_negotiator_init(struct fetch_negotiator *negotiator)
+{
+	struct data *data;
+	negotiator->known_common = known_common;
+	negotiator->add_tip = add_tip;
+	negotiator->next = next;
+	negotiator->ack = ack;
+	negotiator->release = release;
+	negotiator->data = data = xcalloc(1, sizeof(*data));
+	data->rev_list.compare = compare;
+
+	if (marked)
+		for_each_ref(clear_marks, NULL);
+	marked = 1;
+}
diff --git a/negotiator/skipping.h b/negotiator/skipping.h
new file mode 100644
index 0000000000..d7dfa6c6e4
--- /dev/null
+++ b/negotiator/skipping.h
@@ -0,0 +1,8 @@
+#ifndef NEGOTIATOR_SKIPPING_H
+#define NEGOTIATOR_SKIPPING_H
+
+struct fetch_negotiator;
+
+void skipping_negotiator_init(struct fetch_negotiator *negotiator);
+
+#endif
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
new file mode 100755
index 0000000000..8c417f435f
--- /dev/null
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -0,0 +1,179 @@
+#!/bin/sh
+
+test_description='test skipping fetch negotiator'
+. ./test-lib.sh
+
+have_sent () {
+	while test "$#" -ne 0
+	do
+		grep "fetch> have $(git -C client rev-parse $1)" trace
+		if test $? -ne 0
+		then
+			echo "No have $(git -C client rev-parse $1) ($1)"
+			return 1
+		fi
+		shift
+	done
+}	
+
+have_not_sent () {
+	while test "$#" -ne 0
+	do
+		grep "fetch> have $(git -C client rev-parse $1)" trace
+		if test $? -eq 0
+		then
+			return 1
+		fi
+		shift
+	done
+}
+
+test_expect_success 'commits with no parents are sent regardless of skip distance' '
+	git init server &&
+	test_commit -C server to_fetch &&
+
+	git init client &&
+	for i in $(seq 7)
+	do
+		test_commit -C client c$i
+	done &&
+
+	# We send: "c7" (skip 1) "c5" (skip 2) "c2" (skip 4). After that, since
+	# "c1" has no parent, it is still sent as "have" even though it would
+	# normally be skipped.
+	test_config -C client fetch.negotiationalgorithm skipping &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	have_sent c7 c5 c2 c1 &&
+	have_not_sent c6 c4 c3
+'
+
+test_expect_success 'when two skips collide, favor the larger one' '
+	rm -rf server client trace &&
+	git init server &&
+	test_commit -C server to_fetch &&
+
+	git init client &&
+	for i in $(seq 11)
+	do
+		test_commit -C client c$i
+	done &&
+	git -C client checkout c5 &&
+	test_commit -C client c5side &&
+
+	# Before reaching c5, we send "c5side" (skip 1) and "c11" (skip 1) "c9"
+	# (skip 2) "c6" (skip 4). The larger skip (skip 4) takes precedence, so
+	# the next "have" sent will be "c1" (from "c6" skip 4) and not "c4"
+	# (from "c5side" skip 1).
+	test_config -C client fetch.negotiationalgorithm skipping &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	have_sent c5side c11 c9 c6 c1 &&
+	have_not_sent c10 c8 c7 c5 c4 c3 c2
+'
+
+test_expect_success 'use ref advertisement to filter out commits' '
+	rm -rf server client trace &&
+	git init server &&
+	test_commit -C server c1 &&
+	test_commit -C server c2 &&
+	test_commit -C server c3 &&
+	git -C server tag -d c1 c2 c3 &&
+
+	git clone server client &&
+	test_commit -C client c4 &&
+	test_commit -C client c5 &&
+	git -C client checkout c4^^ &&
+	test_commit -C client c2side &&
+
+	git -C server checkout --orphan anotherbranch &&
+	test_commit -C server to_fetch &&
+
+	# The server advertising "c3" (as "refs/heads/master") means that we do
+	# not need to send any ancestors of "c3", but we still need to send "c3"
+	# itself.
+	test_config -C client fetch.negotiationalgorithm skipping &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch origin to_fetch &&
+	have_sent c5 c4^ c2side &&
+	have_not_sent c4 c4^^ c4^^^
+'
+
+test_expect_success 'handle clock skew' '
+	rm -rf server client trace &&
+	git init server &&
+	test_commit -C server to_fetch &&
+
+	git init client &&
+
+	# 2 regular commits
+	test_tick=2000000000 &&
+	test_commit -C client c1 &&
+	test_commit -C client c2 &&
+
+	# 4 old commits
+	test_tick=1000000000 &&
+	git -C client checkout c1 &&
+	test_commit -C client old1 &&
+	test_commit -C client old2 &&
+	test_commit -C client old3 &&
+	test_commit -C client old4 &&
+
+	# "c2" and "c1" are popped first, then "old4" to "old1". "old1" would
+	# normally be skipped, but is treated as a commit without a parent here
+	# and sent, because (due to clock skew) its only parent has already been
+	# popped off the priority queue.
+	test_config -C client fetch.negotiationalgorithm skipping &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+	have_sent c2 c1 old4 old2 old1 &&
+	have_not_sent old3
+'
+
+test_expect_success 'do not send "have" with ancestors of commits that server ACKed' '
+	rm -rf server client trace &&
+	git init server &&
+	test_commit -C server to_fetch &&
+
+	git init client &&
+	for i in $(seq 8)
+	do
+		git -C client checkout --orphan b$i &&
+		test_commit -C client b$i.c0
+	done &&
+	for j in $(seq 19)
+	do
+		for i in $(seq 8)
+		do
+			git -C client checkout b$i &&
+			test_commit -C client b$i.c$j
+		done
+	done &&
+
+	# Copy this branch over to the server and add a commit on it so that it
+	# is reachable but not advertised.
+	git -C server fetch --no-tags "$(pwd)/client" b1:refs/heads/b1 &&
+	git -C server checkout b1 &&
+	test_commit -C server commit-on-b1 &&
+
+	test_config -C client fetch.negotiationalgorithm skipping &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" to_fetch &&
+	grep "  fetch" trace &&
+
+	# fetch-pack sends 2 requests each containing 16 "have" lines before
+	# processing the first response. In these 2 requests, 4 commits from
+	# each branch are sent. Just check the first branch.
+	have_sent b1.c19 b1.c17 b1.c14 b1.c9 &&
+	have_not_sent b1.c18 b1.c16 b1.c15 b1.c13 b1.c12 b1.c11 b1.c10 &&
+
+	# While fetch-pack is processing the first response, it should read that
+	# the server ACKs b1.c19 and b1.c17.
+	grep "fetch< ACK $(git -C client rev-parse b1.c19) common" trace &&
+	grep "fetch< ACK $(git -C client rev-parse b1.c17) common" trace &&
+
+	# fetch-pack should thus not send any more commits in the b1 branch, but
+	# should still send the others (in this test, just check b2).
+	for i in $(seq 0 8)
+	do
+		have_not_sent b1.c$i
+	done &&
+	have_sent b2.c1 b2.c0
+'
+
+test_done
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH] negotiator/skipping: skip commits during fetch

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Introduce a new negotiation algorithm used during fetch that skips
> commits in an effort to find common ancestors faster. The skips grow
> similarly to the Fibonacci sequence as the commit walk proceeds further
> away from the tips. The skips may cause unnecessary commits to be
> included in the packfile, but the negotiation step typically ends more
> quickly.
>
> Usage of this algorithm is guarded behind the configuration flag
> fetch.negotiationAlgorithm.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> This is on jt/fetch-pack-negotiator, but also applies cleanly on
> jt/fetch-nego-tip.

Sounds sensible.

Unfortunately, this one is among many others that get hurt by
needless semantic conflicts caused by reusing the old function name
and changing the function signature to pass the_repository thru some
codepaths, without adding transition macros.  I am running out of
time today as I need some post-office-move clean-ups before getting
organized enough, so I expect I won't be able to clean it up and
push it out on 'pu' by the end of day today.

Thanks.

Re: [PATCH] negotiator/skipping: skip commits during fetch

[Johannes Schindelin]

Hi Jonathan,

On Mon, 16 Jul 2018, Jonathan Tan wrote:

>  t/t5552-skipping-fetch-negotiator.sh | 179 +++++++++++++++++++

This test seems to be failing consistently in the recent `pu` builds:

https://git-for-windows.visualstudio.com/git/_build/results?buildId=14337&view=logs

Could you have a look, please?

Ciao,
Dscho

P.S.: For your convenience, I will paste the last part of the output with
`-i -v -x` here:

-- snipsnap --
2018-07-26T08:18:39.7864833Z expecting success: 
2018-07-26T08:18:39.7868553Z 	rm -rf server client trace &&
2018-07-26T08:18:39.7869403Z 	git init server &&
2018-07-26T08:18:39.7869606Z 	test_commit -C server to_fetch &&
2018-07-26T08:18:39.7870066Z 
2018-07-26T08:18:39.7870281Z 	git init client &&
2018-07-26T08:18:39.7870403Z 
2018-07-26T08:18:39.7870579Z 	# 2 regular commits
2018-07-26T08:18:39.7870779Z 	test_tick=2000000000 &&
2018-07-26T08:18:39.7870943Z 	test_commit -C client c1 &&
2018-07-26T08:18:39.7871103Z 	test_commit -C client c2 &&
2018-07-26T08:18:39.7871228Z 
2018-07-26T08:18:39.7871419Z 	# 4 old commits
2018-07-26T08:18:39.7871575Z 	test_tick=1000000000 &&
2018-07-26T08:18:39.7871734Z 	git -C client checkout c1 &&
2018-07-26T08:18:39.7871916Z 	test_commit -C client old1 &&
2018-07-26T08:18:39.7872081Z 	test_commit -C client old2 &&
2018-07-26T08:18:39.7872396Z 	test_commit -C client old3 &&
2018-07-26T08:18:39.7872598Z 	test_commit -C client old4 &&
2018-07-26T08:18:39.7872743Z 
2018-07-26T08:18:39.7872918Z 	# "c2" and "c1" are popped first, then "old4" to "old1". "old1" would
2018-07-26T08:18:39.7873114Z 	# normally be skipped, but is treated as a commit without a parent here
2018-07-26T08:18:39.7873329Z 	# and sent, because (due to clock skew) its only parent has already been
2018-07-26T08:18:39.7873524Z 	# popped off the priority queue.
2018-07-26T08:18:39.7873700Z 	test_config -C client fetch.negotiationalgorithm skipping &&
2018-07-26T08:18:39.7873908Z 	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
2018-07-26T08:18:39.7874091Z 	have_sent c2 c1 old4 old2 old1 &&
2018-07-26T08:18:39.7874262Z 	have_not_sent old3
2018-07-26T08:18:39.7874383Z 
2018-07-26T08:18:39.8353323Z ++ rm -rf server client trace
2018-07-26T08:18:40.3404166Z ++ git init server
2018-07-26T08:18:40.3756394Z Initialized empty Git repository in D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/server/.git/
2018-07-26T08:18:40.3769512Z ++ test_commit -C server to_fetch
2018-07-26T08:18:40.3776271Z ++ notick=
2018-07-26T08:18:40.3777103Z ++ signoff=
2018-07-26T08:18:40.3777282Z ++ indir=
2018-07-26T08:18:40.3777465Z ++ test 3 '!=' 0
2018-07-26T08:18:40.3777648Z ++ case "$1" in
2018-07-26T08:18:40.3777801Z ++ indir=server
2018-07-26T08:18:40.3777948Z ++ shift
2018-07-26T08:18:40.3778093Z ++ shift
2018-07-26T08:18:40.3778493Z ++ test 1 '!=' 0
2018-07-26T08:18:40.3778921Z ++ case "$1" in
2018-07-26T08:18:40.3779072Z ++ break
2018-07-26T08:18:40.3779241Z ++ indir=server/
2018-07-26T08:18:40.3779431Z ++ file=to_fetch.t
2018-07-26T08:18:40.3779603Z ++ echo to_fetch
2018-07-26T08:18:40.3779923Z ++ git -C server/ add to_fetch.t
2018-07-26T08:18:40.4072248Z ++ test -z ''
2018-07-26T08:18:40.4072727Z ++ test_tick
2018-07-26T08:18:40.4072948Z ++ test -z set
2018-07-26T08:18:40.4073113Z ++ test_tick=1112913673
2018-07-26T08:18:40.4073758Z ++ GIT_COMMITTER_DATE='1112913673 -0700'
2018-07-26T08:18:40.4074001Z ++ GIT_AUTHOR_DATE='1112913673 -0700'
2018-07-26T08:18:40.4074178Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
2018-07-26T08:18:40.4074357Z ++ git -C server/ commit -m to_fetch
2018-07-26T08:18:40.4485364Z [master (root-commit) ff85695] to_fetch
2018-07-26T08:18:40.4485997Z  Author: A U Thor <author@example.com>
2018-07-26T08:18:40.4486201Z  1 file changed, 1 insertion(+)
2018-07-26T08:18:40.4486414Z  create mode 100644 to_fetch.t
2018-07-26T08:18:40.4499970Z ++ git -C server/ tag to_fetch
2018-07-26T08:18:40.4809208Z ++ git init client
2018-07-26T08:18:40.5139949Z Initialized empty Git repository in D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/client/.git/
2018-07-26T08:18:40.5158270Z ++ test_tick=2000000000
2018-07-26T08:18:40.5158466Z ++ test_commit -C client c1
2018-07-26T08:18:40.5159077Z ++ notick=
2018-07-26T08:18:40.5159492Z ++ signoff=
2018-07-26T08:18:40.5159697Z ++ indir=
2018-07-26T08:18:40.5159855Z ++ test 3 '!=' 0
2018-07-26T08:18:40.5160010Z ++ case "$1" in
2018-07-26T08:18:40.5160209Z ++ indir=client
2018-07-26T08:18:40.5160362Z ++ shift
2018-07-26T08:18:40.5160507Z ++ shift
2018-07-26T08:18:40.5160657Z ++ test 1 '!=' 0
2018-07-26T08:18:40.5160831Z ++ case "$1" in
2018-07-26T08:18:40.5161289Z ++ break
2018-07-26T08:18:40.5161582Z ++ indir=client/
2018-07-26T08:18:40.5161764Z ++ file=c1.t
2018-07-26T08:18:40.5161916Z ++ echo c1
2018-07-26T08:18:40.5162231Z ++ git -C client/ add c1.t
2018-07-26T08:18:40.5456318Z ++ test -z ''
2018-07-26T08:18:40.5460548Z ++ test_tick
2018-07-26T08:18:40.5461417Z ++ test -z set
2018-07-26T08:18:40.5463657Z ++ test_tick=2000000060
2018-07-26T08:18:40.5464369Z ++ GIT_COMMITTER_DATE='2000000060 -0700'
2018-07-26T08:18:40.5464617Z ++ GIT_AUTHOR_DATE='2000000060 -0700'
2018-07-26T08:18:40.5464805Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
2018-07-26T08:18:40.5464988Z ++ git -C client/ commit -m c1
2018-07-26T08:18:40.5857440Z [master (root-commit) dc824fa] c1
2018-07-26T08:18:40.5858031Z  Author: A U Thor <author@example.com>
2018-07-26T08:18:40.5858251Z  1 file changed, 1 insertion(+)
2018-07-26T08:18:40.5858451Z  create mode 100644 c1.t
2018-07-26T08:18:40.5872839Z ++ git -C client/ tag c1
2018-07-26T08:18:40.6174770Z ++ test_commit -C client c2
2018-07-26T08:18:40.6175120Z ++ notick=
2018-07-26T08:18:40.6175398Z ++ signoff=
2018-07-26T08:18:40.6175583Z ++ indir=
2018-07-26T08:18:40.6175737Z ++ test 3 '!=' 0
2018-07-26T08:18:40.6175925Z ++ case "$1" in
2018-07-26T08:18:40.6176079Z ++ indir=client
2018-07-26T08:18:40.6176246Z ++ shift
2018-07-26T08:18:40.6176415Z ++ shift
2018-07-26T08:18:40.6176569Z ++ test 1 '!=' 0
2018-07-26T08:18:40.6176738Z ++ case "$1" in
2018-07-26T08:18:40.6176905Z ++ break
2018-07-26T08:18:40.6177052Z ++ indir=client/
2018-07-26T08:18:40.6177200Z ++ file=c2.t
2018-07-26T08:18:40.6177369Z ++ echo c2
2018-07-26T08:18:40.6177525Z ++ git -C client/ add c2.t
2018-07-26T08:18:40.6474943Z ++ test -z ''
2018-07-26T08:18:40.6479175Z ++ test_tick
2018-07-26T08:18:40.6479861Z ++ test -z set
2018-07-26T08:18:40.6482344Z ++ test_tick=2000000120
2018-07-26T08:18:40.6483064Z ++ GIT_COMMITTER_DATE='2000000120 -0700'
2018-07-26T08:18:40.6483243Z ++ GIT_AUTHOR_DATE='2000000120 -0700'
2018-07-26T08:18:40.6483412Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
2018-07-26T08:18:40.6483597Z ++ git -C client/ commit -m c2
2018-07-26T08:18:40.6883597Z [master 9ab4692] c2
2018-07-26T08:18:40.6884552Z  Author: A U Thor <author@example.com>
2018-07-26T08:18:40.6884902Z  1 file changed, 1 insertion(+)
2018-07-26T08:18:40.6885078Z  create mode 100644 c2.t
2018-07-26T08:18:40.6898418Z ++ git -C client/ tag c2
2018-07-26T08:18:40.7214970Z ++ test_tick=1000000000
2018-07-26T08:18:40.7215737Z ++ git -C client checkout c1
2018-07-26T08:18:40.7537971Z Note: checking out 'c1'.
2018-07-26T08:18:40.7538294Z 
2018-07-26T08:18:40.7538485Z You are in 'detached HEAD' state. You can look around, make experimental
2018-07-26T08:18:40.7538901Z changes and commit them, and you can discard any commits you make in this
2018-07-26T08:18:40.7539153Z state without impacting any branches by performing another checkout.
2018-07-26T08:18:40.7539288Z 
2018-07-26T08:18:40.7539455Z If you want to create a new branch to retain commits you create, you may
2018-07-26T08:18:40.7539646Z do so (now or later) by using -b with the checkout command again. Example:
2018-07-26T08:18:40.7539799Z 
2018-07-26T08:18:40.7539979Z   git checkout -b <new-branch-name>
2018-07-26T08:18:40.7540099Z 
2018-07-26T08:18:40.7540264Z HEAD is now at dc824fa c1
2018-07-26T08:18:40.7552832Z ++ test_commit -C client old1
2018-07-26T08:18:40.7559118Z ++ notick=
2018-07-26T08:18:40.7559789Z ++ signoff=
2018-07-26T08:18:40.7559966Z ++ indir=
2018-07-26T08:18:40.7560066Z ++ test 3 '!=' 0
2018-07-26T08:18:40.7565193Z ++ case "$1" in
2018-07-26T08:18:40.7565286Z ++ indir=client
2018-07-26T08:18:40.7565373Z ++ shift
2018-07-26T08:18:40.7565456Z ++ shift
2018-07-26T08:18:40.7566662Z ++ test 1 '!=' 0
2018-07-26T08:18:40.7566796Z ++ case "$1" in
2018-07-26T08:18:40.7566879Z ++ break
2018-07-26T08:18:40.7566961Z ++ indir=client/
2018-07-26T08:18:40.7567066Z ++ file=old1.t
2018-07-26T08:18:40.7567150Z ++ echo old1
2018-07-26T08:18:40.7567238Z ++ git -C client/ add old1.t
2018-07-26T08:18:40.7962371Z ++ test -z ''
2018-07-26T08:18:40.7962668Z ++ test_tick
2018-07-26T08:18:40.7963247Z ++ test -z set
2018-07-26T08:18:40.7963453Z ++ test_tick=1000000060
2018-07-26T08:18:40.7963649Z ++ GIT_COMMITTER_DATE='1000000060 -0700'
2018-07-26T08:18:40.7963832Z ++ GIT_AUTHOR_DATE='1000000060 -0700'
2018-07-26T08:18:40.7964000Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
2018-07-26T08:18:40.7964164Z ++ git -C client/ commit -m old1
2018-07-26T08:18:40.8356576Z [detached HEAD e9a2c09] old1
2018-07-26T08:18:40.8357378Z  Author: A U Thor <author@example.com>
2018-07-26T08:18:40.8357554Z  1 file changed, 1 insertion(+)
2018-07-26T08:18:40.8357652Z  create mode 100644 old1.t
2018-07-26T08:18:40.8372002Z ++ git -C client/ tag old1
2018-07-26T08:18:40.8671359Z ++ test_commit -C client old2
2018-07-26T08:18:40.8676749Z ++ notick=
2018-07-26T08:18:40.8677398Z ++ signoff=
2018-07-26T08:18:40.8679262Z ++ indir=
2018-07-26T08:18:40.8679957Z ++ test 3 '!=' 0
2018-07-26T08:18:40.8680150Z ++ case "$1" in
2018-07-26T08:18:40.8680305Z ++ indir=client
2018-07-26T08:18:40.8680447Z ++ shift
2018-07-26T08:18:40.8680587Z ++ shift
2018-07-26T08:18:40.8680798Z ++ test 1 '!=' 0
2018-07-26T08:18:40.8680949Z ++ case "$1" in
2018-07-26T08:18:40.8681143Z ++ break
2018-07-26T08:18:40.8681311Z ++ indir=client/
2018-07-26T08:18:40.8681458Z ++ file=old2.t
2018-07-26T08:18:40.8681604Z ++ echo old2
2018-07-26T08:18:40.8681789Z ++ git -C client/ add old2.t
2018-07-26T08:18:40.8990053Z ++ test -z ''
2018-07-26T08:18:40.8990392Z ++ test_tick
2018-07-26T08:18:40.8990953Z ++ test -z set
2018-07-26T08:18:40.8991246Z ++ test_tick=1000000120
2018-07-26T08:18:40.8991421Z ++ GIT_COMMITTER_DATE='1000000120 -0700'
2018-07-26T08:18:40.8991585Z ++ GIT_AUTHOR_DATE='1000000120 -0700'
2018-07-26T08:18:40.8991771Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
2018-07-26T08:18:40.8991936Z ++ git -C client/ commit -m old2
2018-07-26T08:18:40.9402744Z [detached HEAD 41bd8dc] old2
2018-07-26T08:18:40.9403286Z  Author: A U Thor <author@example.com>
2018-07-26T08:18:40.9403510Z  1 file changed, 1 insertion(+)
2018-07-26T08:18:40.9403678Z  create mode 100644 old2.t
2018-07-26T08:18:40.9418122Z ++ git -C client/ tag old2
2018-07-26T08:18:40.9736807Z ++ test_commit -C client old3
2018-07-26T08:18:40.9737058Z ++ notick=
2018-07-26T08:18:40.9737152Z ++ signoff=
2018-07-26T08:18:40.9737238Z ++ indir=
2018-07-26T08:18:40.9737327Z ++ test 3 '!=' 0
2018-07-26T08:18:40.9737600Z ++ case "$1" in
2018-07-26T08:18:40.9737690Z ++ indir=client
2018-07-26T08:18:40.9737776Z ++ shift
2018-07-26T08:18:40.9737860Z ++ shift
2018-07-26T08:18:40.9737964Z ++ test 1 '!=' 0
2018-07-26T08:18:40.9738054Z ++ case "$1" in
2018-07-26T08:18:40.9738140Z ++ break
2018-07-26T08:18:40.9738248Z ++ indir=client/
2018-07-26T08:18:40.9738338Z ++ file=old3.t
2018-07-26T08:18:40.9738445Z ++ echo old3
2018-07-26T08:18:40.9738540Z ++ git -C client/ add old3.t
2018-07-26T08:18:41.0035565Z ++ test -z ''
2018-07-26T08:18:41.0036056Z ++ test_tick
2018-07-26T08:18:41.0036299Z ++ test -z set
2018-07-26T08:18:41.0036467Z ++ test_tick=1000000180
2018-07-26T08:18:41.0036638Z ++ GIT_COMMITTER_DATE='1000000180 -0700'
2018-07-26T08:18:41.0037189Z ++ GIT_AUTHOR_DATE='1000000180 -0700'
2018-07-26T08:18:41.0037403Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
2018-07-26T08:18:41.0037574Z ++ git -C client/ commit -m old3
2018-07-26T08:18:41.0429238Z [detached HEAD 4ff0db5] old3
2018-07-26T08:18:41.0429619Z  Author: A U Thor <author@example.com>
2018-07-26T08:18:41.0429799Z  1 file changed, 1 insertion(+)
2018-07-26T08:18:41.0429965Z  create mode 100644 old3.t
2018-07-26T08:18:41.0443795Z ++ git -C client/ tag old3
2018-07-26T08:18:41.0752553Z ++ test_commit -C client old4
2018-07-26T08:18:41.0752824Z ++ notick=
2018-07-26T08:18:41.0752936Z ++ signoff=
2018-07-26T08:18:41.0753905Z ++ indir=
2018-07-26T08:18:41.0754193Z ++ test 3 '!=' 0
2018-07-26T08:18:41.0754374Z ++ case "$1" in
2018-07-26T08:18:41.0754531Z ++ indir=client
2018-07-26T08:18:41.0754682Z ++ shift
2018-07-26T08:18:41.0754828Z ++ shift
2018-07-26T08:18:41.0755007Z ++ test 1 '!=' 0
2018-07-26T08:18:41.0755162Z ++ case "$1" in
2018-07-26T08:18:41.0755340Z ++ break
2018-07-26T08:18:41.0755491Z ++ indir=client/
2018-07-26T08:18:41.0755664Z ++ file=old4.t
2018-07-26T08:18:41.0755814Z ++ echo old4
2018-07-26T08:18:41.0755971Z ++ git -C client/ add old4.t
2018-07-26T08:18:41.1064316Z ++ test -z ''
2018-07-26T08:18:41.1064885Z ++ test_tick
2018-07-26T08:18:41.1065169Z ++ test -z set
2018-07-26T08:18:41.1065432Z ++ test_tick=1000000240
2018-07-26T08:18:41.1065637Z ++ GIT_COMMITTER_DATE='1000000240 -0700'
2018-07-26T08:18:41.1065820Z ++ GIT_AUTHOR_DATE='1000000240 -0700'
2018-07-26T08:18:41.1066008Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
2018-07-26T08:18:41.1066212Z ++ git -C client/ commit -m old4
2018-07-26T08:18:41.1488203Z [detached HEAD caef059] old4
2018-07-26T08:18:41.1488670Z  Author: A U Thor <author@example.com>
2018-07-26T08:18:41.1489153Z  1 file changed, 1 insertion(+)
2018-07-26T08:18:41.1489370Z  create mode 100644 old4.t
2018-07-26T08:18:41.1502887Z ++ git -C client/ tag old4
2018-07-26T08:18:41.1824546Z ++ test_config -C client fetch.negotiationalgorithm skipping
2018-07-26T08:18:41.1825670Z ++ config_dir=
2018-07-26T08:18:41.1826072Z ++ test -C = -C
2018-07-26T08:18:41.1826299Z ++ shift
2018-07-26T08:18:41.1826528Z ++ config_dir=client
2018-07-26T08:18:41.1826809Z ++ shift
2018-07-26T08:18:41.1827079Z ++ test_when_finished 'test_unconfig -C '\''client'\'' '\''fetch.negotiationalgorithm'\'''
2018-07-26T08:18:41.1827289Z ++ test 0 = 0
2018-07-26T08:18:41.1827561Z ++ test_cleanup='{ test_unconfig -C '\''client'\'' '\''fetch.negotiationalgorithm'\''
2018-07-26T08:18:41.1827751Z 		} && (exit "$eval_ret"); eval_ret=$?; :'
2018-07-26T08:18:41.1827930Z ++ git -C client config fetch.negotiationalgorithm skipping
2018-07-26T08:18:41.2196451Z +++ pwd
2018-07-26T08:18:41.2196831Z +++ builtin pwd -W
2018-07-26T08:18:41.2274040Z +++ pwd
2018-07-26T08:18:41.2274458Z +++ builtin pwd -W
2018-07-26T08:18:41.2285081Z ++ GIT_TRACE_PACKET='D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/trace'
2018-07-26T08:18:41.2285515Z ++ git -C client fetch 'D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/server'
2018-07-26T08:18:41.3054360Z warning: no common commits
2018-07-26T08:18:41.3264762Z From D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/server
2018-07-26T08:18:41.3265204Z  * branch            HEAD       -> FETCH_HEAD
2018-07-26T08:18:41.3362819Z ++ have_sent c2 c1 old4 old2 old1
2018-07-26T08:18:41.3370525Z ++ test 5 -ne 0
2018-07-26T08:18:41.3423124Z +++ git -C client rev-parse c2
2018-07-26T08:18:41.3756643Z ++ grep 'fetch> have 9ab46928dc282aa09f4dbf96893a252e058e7e8e' trace
2018-07-26T08:18:41.3878403Z packet:        fetch> have 9ab46928dc282aa09f4dbf96893a252e058e7e8e
2018-07-26T08:18:41.3883984Z ++ test 0 -ne 0
2018-07-26T08:18:41.3884900Z ++ shift
2018-07-26T08:18:41.3885199Z ++ test 4 -ne 0
2018-07-26T08:18:41.3938298Z +++ git -C client rev-parse c1
2018-07-26T08:18:41.4243782Z ++ grep 'fetch> have dc824fafb05f3229aedf1f320bbe572e35364dfe' trace
2018-07-26T08:18:41.4375402Z packet:        fetch> have dc824fafb05f3229aedf1f320bbe572e35364dfe
2018-07-26T08:18:41.4383803Z ++ test 0 -ne 0
2018-07-26T08:18:41.4384733Z ++ shift
2018-07-26T08:18:41.4385018Z ++ test 3 -ne 0
2018-07-26T08:18:41.4436622Z +++ git -C client rev-parse old4
2018-07-26T08:18:41.4749084Z ++ grep 'fetch> have caef059de69917b9119176a11b88afcef769331d' trace
2018-07-26T08:18:41.4888266Z ++ test 1 -ne 0
2018-07-26T08:18:41.4941092Z +++ git -C client rev-parse old4
2018-07-26T08:18:41.5253206Z ++ echo 'No have caef059de69917b9119176a11b88afcef769331d (old4)'
2018-07-26T08:18:41.5253602Z ++ return 1
2018-07-26T08:18:41.5254746Z error: last command exited with $?=1
2018-07-26T08:18:41.5254865Z No have caef059de69917b9119176a11b88afcef769331d (old4)
2018-07-26T08:18:41.5260970Z not ok 4 - handle clock skew
2018-07-26T08:18:41.5441990Z #	
2018-07-26T08:18:41.5442184Z #		rm -rf server client trace &&
2018-07-26T08:18:41.5442422Z #		git init server &&
2018-07-26T08:18:41.5448007Z #		test_commit -C server to_fetch &&
2018-07-26T08:18:41.5448223Z #	
2018-07-26T08:18:41.5448357Z #		git init client &&
2018-07-26T08:18:41.5448466Z #	
2018-07-26T08:18:41.5448661Z #		# 2 regular commits
2018-07-26T08:18:41.5448810Z #		test_tick=2000000000 &&
2018-07-26T08:18:41.5449231Z #		test_commit -C client c1 &&
2018-07-26T08:18:41.5449393Z #		test_commit -C client c2 &&
2018-07-26T08:18:41.5449509Z #	
2018-07-26T08:18:41.5449679Z #		# 4 old commits
2018-07-26T08:18:41.5449859Z #		test_tick=1000000000 &&
2018-07-26T08:18:41.5450017Z #		git -C client checkout c1 &&
2018-07-26T08:18:41.5450220Z #		test_commit -C client old1 &&
2018-07-26T08:18:41.5450343Z #		test_commit -C client old2 &&
2018-07-26T08:18:41.5450449Z #		test_commit -C client old3 &&
2018-07-26T08:18:41.5450667Z #		test_commit -C client old4 &&
2018-07-26T08:18:41.5450821Z #	
2018-07-26T08:18:41.5450954Z #		# "c2" and "c1" are popped first, then "old4" to "old1". "old1" would
2018-07-26T08:18:41.5451133Z #		# normally be skipped, but is treated as a commit without a parent here
2018-07-26T08:18:41.5451392Z #		# and sent, because (due to clock skew) its only parent has already been
2018-07-26T08:18:41.5451547Z #		# popped off the priority queue.
2018-07-26T08:18:41.5451675Z #		test_config -C client fetch.negotiationalgorithm skipping &&
2018-07-26T08:18:41.5451829Z #		GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
2018-07-26T08:18:41.5451961Z #		have_sent c2 c1 old4 old2 old1 &&
2018-07-26T08:18:41.5452091Z #		have_not_sent old3
2018-07-26T08:18:41.5452186Z #	

Re: [PATCH] negotiator/skipping: skip commits during fetch

[Johannes Schindelin]

Hi Jonathan,

On Thu, 26 Jul 2018, Johannes Schindelin wrote:

> On Mon, 16 Jul 2018, Jonathan Tan wrote:
> 
> >  t/t5552-skipping-fetch-negotiator.sh | 179 +++++++++++++++++++
> 
> This test seems to be failing consistently in the recent `pu` builds:
> 
> https://git-for-windows.visualstudio.com/git/_build/results?buildId=14337&view=logs

It now also causes `next` builds to fail:

https://git-for-windows.visualstudio.com/git/_build/results?buildId=14345&view=logs

Please have a look,
Dscho

> Could you have a look, please?
> 
> Ciao,
> Dscho
> 
> P.S.: For your convenience, I will paste the last part of the output with
> `-i -v -x` here:
> 
> -- snipsnap --
> 2018-07-26T08:18:39.7864833Z expecting success: 
> 2018-07-26T08:18:39.7868553Z 	rm -rf server client trace &&
> 2018-07-26T08:18:39.7869403Z 	git init server &&
> 2018-07-26T08:18:39.7869606Z 	test_commit -C server to_fetch &&
> 2018-07-26T08:18:39.7870066Z 
> 2018-07-26T08:18:39.7870281Z 	git init client &&
> 2018-07-26T08:18:39.7870403Z 
> 2018-07-26T08:18:39.7870579Z 	# 2 regular commits
> 2018-07-26T08:18:39.7870779Z 	test_tick=2000000000 &&
> 2018-07-26T08:18:39.7870943Z 	test_commit -C client c1 &&
> 2018-07-26T08:18:39.7871103Z 	test_commit -C client c2 &&
> 2018-07-26T08:18:39.7871228Z 
> 2018-07-26T08:18:39.7871419Z 	# 4 old commits
> 2018-07-26T08:18:39.7871575Z 	test_tick=1000000000 &&
> 2018-07-26T08:18:39.7871734Z 	git -C client checkout c1 &&
> 2018-07-26T08:18:39.7871916Z 	test_commit -C client old1 &&
> 2018-07-26T08:18:39.7872081Z 	test_commit -C client old2 &&
> 2018-07-26T08:18:39.7872396Z 	test_commit -C client old3 &&
> 2018-07-26T08:18:39.7872598Z 	test_commit -C client old4 &&
> 2018-07-26T08:18:39.7872743Z 
> 2018-07-26T08:18:39.7872918Z 	# "c2" and "c1" are popped first, then "old4" to "old1". "old1" would
> 2018-07-26T08:18:39.7873114Z 	# normally be skipped, but is treated as a commit without a parent here
> 2018-07-26T08:18:39.7873329Z 	# and sent, because (due to clock skew) its only parent has already been
> 2018-07-26T08:18:39.7873524Z 	# popped off the priority queue.
> 2018-07-26T08:18:39.7873700Z 	test_config -C client fetch.negotiationalgorithm skipping &&
> 2018-07-26T08:18:39.7873908Z 	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
> 2018-07-26T08:18:39.7874091Z 	have_sent c2 c1 old4 old2 old1 &&
> 2018-07-26T08:18:39.7874262Z 	have_not_sent old3
> 2018-07-26T08:18:39.7874383Z 
> 2018-07-26T08:18:39.8353323Z ++ rm -rf server client trace
> 2018-07-26T08:18:40.3404166Z ++ git init server
> 2018-07-26T08:18:40.3756394Z Initialized empty Git repository in D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/server/.git/
> 2018-07-26T08:18:40.3769512Z ++ test_commit -C server to_fetch
> 2018-07-26T08:18:40.3776271Z ++ notick=
> 2018-07-26T08:18:40.3777103Z ++ signoff=
> 2018-07-26T08:18:40.3777282Z ++ indir=
> 2018-07-26T08:18:40.3777465Z ++ test 3 '!=' 0
> 2018-07-26T08:18:40.3777648Z ++ case "$1" in
> 2018-07-26T08:18:40.3777801Z ++ indir=server
> 2018-07-26T08:18:40.3777948Z ++ shift
> 2018-07-26T08:18:40.3778093Z ++ shift
> 2018-07-26T08:18:40.3778493Z ++ test 1 '!=' 0
> 2018-07-26T08:18:40.3778921Z ++ case "$1" in
> 2018-07-26T08:18:40.3779072Z ++ break
> 2018-07-26T08:18:40.3779241Z ++ indir=server/
> 2018-07-26T08:18:40.3779431Z ++ file=to_fetch.t
> 2018-07-26T08:18:40.3779603Z ++ echo to_fetch
> 2018-07-26T08:18:40.3779923Z ++ git -C server/ add to_fetch.t
> 2018-07-26T08:18:40.4072248Z ++ test -z ''
> 2018-07-26T08:18:40.4072727Z ++ test_tick
> 2018-07-26T08:18:40.4072948Z ++ test -z set
> 2018-07-26T08:18:40.4073113Z ++ test_tick=1112913673
> 2018-07-26T08:18:40.4073758Z ++ GIT_COMMITTER_DATE='1112913673 -0700'
> 2018-07-26T08:18:40.4074001Z ++ GIT_AUTHOR_DATE='1112913673 -0700'
> 2018-07-26T08:18:40.4074178Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
> 2018-07-26T08:18:40.4074357Z ++ git -C server/ commit -m to_fetch
> 2018-07-26T08:18:40.4485364Z [master (root-commit) ff85695] to_fetch
> 2018-07-26T08:18:40.4485997Z  Author: A U Thor <author@example.com>
> 2018-07-26T08:18:40.4486201Z  1 file changed, 1 insertion(+)
> 2018-07-26T08:18:40.4486414Z  create mode 100644 to_fetch.t
> 2018-07-26T08:18:40.4499970Z ++ git -C server/ tag to_fetch
> 2018-07-26T08:18:40.4809208Z ++ git init client
> 2018-07-26T08:18:40.5139949Z Initialized empty Git repository in D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/client/.git/
> 2018-07-26T08:18:40.5158270Z ++ test_tick=2000000000
> 2018-07-26T08:18:40.5158466Z ++ test_commit -C client c1
> 2018-07-26T08:18:40.5159077Z ++ notick=
> 2018-07-26T08:18:40.5159492Z ++ signoff=
> 2018-07-26T08:18:40.5159697Z ++ indir=
> 2018-07-26T08:18:40.5159855Z ++ test 3 '!=' 0
> 2018-07-26T08:18:40.5160010Z ++ case "$1" in
> 2018-07-26T08:18:40.5160209Z ++ indir=client
> 2018-07-26T08:18:40.5160362Z ++ shift
> 2018-07-26T08:18:40.5160507Z ++ shift
> 2018-07-26T08:18:40.5160657Z ++ test 1 '!=' 0
> 2018-07-26T08:18:40.5160831Z ++ case "$1" in
> 2018-07-26T08:18:40.5161289Z ++ break
> 2018-07-26T08:18:40.5161582Z ++ indir=client/
> 2018-07-26T08:18:40.5161764Z ++ file=c1.t
> 2018-07-26T08:18:40.5161916Z ++ echo c1
> 2018-07-26T08:18:40.5162231Z ++ git -C client/ add c1.t
> 2018-07-26T08:18:40.5456318Z ++ test -z ''
> 2018-07-26T08:18:40.5460548Z ++ test_tick
> 2018-07-26T08:18:40.5461417Z ++ test -z set
> 2018-07-26T08:18:40.5463657Z ++ test_tick=2000000060
> 2018-07-26T08:18:40.5464369Z ++ GIT_COMMITTER_DATE='2000000060 -0700'
> 2018-07-26T08:18:40.5464617Z ++ GIT_AUTHOR_DATE='2000000060 -0700'
> 2018-07-26T08:18:40.5464805Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
> 2018-07-26T08:18:40.5464988Z ++ git -C client/ commit -m c1
> 2018-07-26T08:18:40.5857440Z [master (root-commit) dc824fa] c1
> 2018-07-26T08:18:40.5858031Z  Author: A U Thor <author@example.com>
> 2018-07-26T08:18:40.5858251Z  1 file changed, 1 insertion(+)
> 2018-07-26T08:18:40.5858451Z  create mode 100644 c1.t
> 2018-07-26T08:18:40.5872839Z ++ git -C client/ tag c1
> 2018-07-26T08:18:40.6174770Z ++ test_commit -C client c2
> 2018-07-26T08:18:40.6175120Z ++ notick=
> 2018-07-26T08:18:40.6175398Z ++ signoff=
> 2018-07-26T08:18:40.6175583Z ++ indir=
> 2018-07-26T08:18:40.6175737Z ++ test 3 '!=' 0
> 2018-07-26T08:18:40.6175925Z ++ case "$1" in
> 2018-07-26T08:18:40.6176079Z ++ indir=client
> 2018-07-26T08:18:40.6176246Z ++ shift
> 2018-07-26T08:18:40.6176415Z ++ shift
> 2018-07-26T08:18:40.6176569Z ++ test 1 '!=' 0
> 2018-07-26T08:18:40.6176738Z ++ case "$1" in
> 2018-07-26T08:18:40.6176905Z ++ break
> 2018-07-26T08:18:40.6177052Z ++ indir=client/
> 2018-07-26T08:18:40.6177200Z ++ file=c2.t
> 2018-07-26T08:18:40.6177369Z ++ echo c2
> 2018-07-26T08:18:40.6177525Z ++ git -C client/ add c2.t
> 2018-07-26T08:18:40.6474943Z ++ test -z ''
> 2018-07-26T08:18:40.6479175Z ++ test_tick
> 2018-07-26T08:18:40.6479861Z ++ test -z set
> 2018-07-26T08:18:40.6482344Z ++ test_tick=2000000120
> 2018-07-26T08:18:40.6483064Z ++ GIT_COMMITTER_DATE='2000000120 -0700'
> 2018-07-26T08:18:40.6483243Z ++ GIT_AUTHOR_DATE='2000000120 -0700'
> 2018-07-26T08:18:40.6483412Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
> 2018-07-26T08:18:40.6483597Z ++ git -C client/ commit -m c2
> 2018-07-26T08:18:40.6883597Z [master 9ab4692] c2
> 2018-07-26T08:18:40.6884552Z  Author: A U Thor <author@example.com>
> 2018-07-26T08:18:40.6884902Z  1 file changed, 1 insertion(+)
> 2018-07-26T08:18:40.6885078Z  create mode 100644 c2.t
> 2018-07-26T08:18:40.6898418Z ++ git -C client/ tag c2
> 2018-07-26T08:18:40.7214970Z ++ test_tick=1000000000
> 2018-07-26T08:18:40.7215737Z ++ git -C client checkout c1
> 2018-07-26T08:18:40.7537971Z Note: checking out 'c1'.
> 2018-07-26T08:18:40.7538294Z 
> 2018-07-26T08:18:40.7538485Z You are in 'detached HEAD' state. You can look around, make experimental
> 2018-07-26T08:18:40.7538901Z changes and commit them, and you can discard any commits you make in this
> 2018-07-26T08:18:40.7539153Z state without impacting any branches by performing another checkout.
> 2018-07-26T08:18:40.7539288Z 
> 2018-07-26T08:18:40.7539455Z If you want to create a new branch to retain commits you create, you may
> 2018-07-26T08:18:40.7539646Z do so (now or later) by using -b with the checkout command again. Example:
> 2018-07-26T08:18:40.7539799Z 
> 2018-07-26T08:18:40.7539979Z   git checkout -b <new-branch-name>
> 2018-07-26T08:18:40.7540099Z 
> 2018-07-26T08:18:40.7540264Z HEAD is now at dc824fa c1
> 2018-07-26T08:18:40.7552832Z ++ test_commit -C client old1
> 2018-07-26T08:18:40.7559118Z ++ notick=
> 2018-07-26T08:18:40.7559789Z ++ signoff=
> 2018-07-26T08:18:40.7559966Z ++ indir=
> 2018-07-26T08:18:40.7560066Z ++ test 3 '!=' 0
> 2018-07-26T08:18:40.7565193Z ++ case "$1" in
> 2018-07-26T08:18:40.7565286Z ++ indir=client
> 2018-07-26T08:18:40.7565373Z ++ shift
> 2018-07-26T08:18:40.7565456Z ++ shift
> 2018-07-26T08:18:40.7566662Z ++ test 1 '!=' 0
> 2018-07-26T08:18:40.7566796Z ++ case "$1" in
> 2018-07-26T08:18:40.7566879Z ++ break
> 2018-07-26T08:18:40.7566961Z ++ indir=client/
> 2018-07-26T08:18:40.7567066Z ++ file=old1.t
> 2018-07-26T08:18:40.7567150Z ++ echo old1
> 2018-07-26T08:18:40.7567238Z ++ git -C client/ add old1.t
> 2018-07-26T08:18:40.7962371Z ++ test -z ''
> 2018-07-26T08:18:40.7962668Z ++ test_tick
> 2018-07-26T08:18:40.7963247Z ++ test -z set
> 2018-07-26T08:18:40.7963453Z ++ test_tick=1000000060
> 2018-07-26T08:18:40.7963649Z ++ GIT_COMMITTER_DATE='1000000060 -0700'
> 2018-07-26T08:18:40.7963832Z ++ GIT_AUTHOR_DATE='1000000060 -0700'
> 2018-07-26T08:18:40.7964000Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
> 2018-07-26T08:18:40.7964164Z ++ git -C client/ commit -m old1
> 2018-07-26T08:18:40.8356576Z [detached HEAD e9a2c09] old1
> 2018-07-26T08:18:40.8357378Z  Author: A U Thor <author@example.com>
> 2018-07-26T08:18:40.8357554Z  1 file changed, 1 insertion(+)
> 2018-07-26T08:18:40.8357652Z  create mode 100644 old1.t
> 2018-07-26T08:18:40.8372002Z ++ git -C client/ tag old1
> 2018-07-26T08:18:40.8671359Z ++ test_commit -C client old2
> 2018-07-26T08:18:40.8676749Z ++ notick=
> 2018-07-26T08:18:40.8677398Z ++ signoff=
> 2018-07-26T08:18:40.8679262Z ++ indir=
> 2018-07-26T08:18:40.8679957Z ++ test 3 '!=' 0
> 2018-07-26T08:18:40.8680150Z ++ case "$1" in
> 2018-07-26T08:18:40.8680305Z ++ indir=client
> 2018-07-26T08:18:40.8680447Z ++ shift
> 2018-07-26T08:18:40.8680587Z ++ shift
> 2018-07-26T08:18:40.8680798Z ++ test 1 '!=' 0
> 2018-07-26T08:18:40.8680949Z ++ case "$1" in
> 2018-07-26T08:18:40.8681143Z ++ break
> 2018-07-26T08:18:40.8681311Z ++ indir=client/
> 2018-07-26T08:18:40.8681458Z ++ file=old2.t
> 2018-07-26T08:18:40.8681604Z ++ echo old2
> 2018-07-26T08:18:40.8681789Z ++ git -C client/ add old2.t
> 2018-07-26T08:18:40.8990053Z ++ test -z ''
> 2018-07-26T08:18:40.8990392Z ++ test_tick
> 2018-07-26T08:18:40.8990953Z ++ test -z set
> 2018-07-26T08:18:40.8991246Z ++ test_tick=1000000120
> 2018-07-26T08:18:40.8991421Z ++ GIT_COMMITTER_DATE='1000000120 -0700'
> 2018-07-26T08:18:40.8991585Z ++ GIT_AUTHOR_DATE='1000000120 -0700'
> 2018-07-26T08:18:40.8991771Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
> 2018-07-26T08:18:40.8991936Z ++ git -C client/ commit -m old2
> 2018-07-26T08:18:40.9402744Z [detached HEAD 41bd8dc] old2
> 2018-07-26T08:18:40.9403286Z  Author: A U Thor <author@example.com>
> 2018-07-26T08:18:40.9403510Z  1 file changed, 1 insertion(+)
> 2018-07-26T08:18:40.9403678Z  create mode 100644 old2.t
> 2018-07-26T08:18:40.9418122Z ++ git -C client/ tag old2
> 2018-07-26T08:18:40.9736807Z ++ test_commit -C client old3
> 2018-07-26T08:18:40.9737058Z ++ notick=
> 2018-07-26T08:18:40.9737152Z ++ signoff=
> 2018-07-26T08:18:40.9737238Z ++ indir=
> 2018-07-26T08:18:40.9737327Z ++ test 3 '!=' 0
> 2018-07-26T08:18:40.9737600Z ++ case "$1" in
> 2018-07-26T08:18:40.9737690Z ++ indir=client
> 2018-07-26T08:18:40.9737776Z ++ shift
> 2018-07-26T08:18:40.9737860Z ++ shift
> 2018-07-26T08:18:40.9737964Z ++ test 1 '!=' 0
> 2018-07-26T08:18:40.9738054Z ++ case "$1" in
> 2018-07-26T08:18:40.9738140Z ++ break
> 2018-07-26T08:18:40.9738248Z ++ indir=client/
> 2018-07-26T08:18:40.9738338Z ++ file=old3.t
> 2018-07-26T08:18:40.9738445Z ++ echo old3
> 2018-07-26T08:18:40.9738540Z ++ git -C client/ add old3.t
> 2018-07-26T08:18:41.0035565Z ++ test -z ''
> 2018-07-26T08:18:41.0036056Z ++ test_tick
> 2018-07-26T08:18:41.0036299Z ++ test -z set
> 2018-07-26T08:18:41.0036467Z ++ test_tick=1000000180
> 2018-07-26T08:18:41.0036638Z ++ GIT_COMMITTER_DATE='1000000180 -0700'
> 2018-07-26T08:18:41.0037189Z ++ GIT_AUTHOR_DATE='1000000180 -0700'
> 2018-07-26T08:18:41.0037403Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
> 2018-07-26T08:18:41.0037574Z ++ git -C client/ commit -m old3
> 2018-07-26T08:18:41.0429238Z [detached HEAD 4ff0db5] old3
> 2018-07-26T08:18:41.0429619Z  Author: A U Thor <author@example.com>
> 2018-07-26T08:18:41.0429799Z  1 file changed, 1 insertion(+)
> 2018-07-26T08:18:41.0429965Z  create mode 100644 old3.t
> 2018-07-26T08:18:41.0443795Z ++ git -C client/ tag old3
> 2018-07-26T08:18:41.0752553Z ++ test_commit -C client old4
> 2018-07-26T08:18:41.0752824Z ++ notick=
> 2018-07-26T08:18:41.0752936Z ++ signoff=
> 2018-07-26T08:18:41.0753905Z ++ indir=
> 2018-07-26T08:18:41.0754193Z ++ test 3 '!=' 0
> 2018-07-26T08:18:41.0754374Z ++ case "$1" in
> 2018-07-26T08:18:41.0754531Z ++ indir=client
> 2018-07-26T08:18:41.0754682Z ++ shift
> 2018-07-26T08:18:41.0754828Z ++ shift
> 2018-07-26T08:18:41.0755007Z ++ test 1 '!=' 0
> 2018-07-26T08:18:41.0755162Z ++ case "$1" in
> 2018-07-26T08:18:41.0755340Z ++ break
> 2018-07-26T08:18:41.0755491Z ++ indir=client/
> 2018-07-26T08:18:41.0755664Z ++ file=old4.t
> 2018-07-26T08:18:41.0755814Z ++ echo old4
> 2018-07-26T08:18:41.0755971Z ++ git -C client/ add old4.t
> 2018-07-26T08:18:41.1064316Z ++ test -z ''
> 2018-07-26T08:18:41.1064885Z ++ test_tick
> 2018-07-26T08:18:41.1065169Z ++ test -z set
> 2018-07-26T08:18:41.1065432Z ++ test_tick=1000000240
> 2018-07-26T08:18:41.1065637Z ++ GIT_COMMITTER_DATE='1000000240 -0700'
> 2018-07-26T08:18:41.1065820Z ++ GIT_AUTHOR_DATE='1000000240 -0700'
> 2018-07-26T08:18:41.1066008Z ++ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
> 2018-07-26T08:18:41.1066212Z ++ git -C client/ commit -m old4
> 2018-07-26T08:18:41.1488203Z [detached HEAD caef059] old4
> 2018-07-26T08:18:41.1488670Z  Author: A U Thor <author@example.com>
> 2018-07-26T08:18:41.1489153Z  1 file changed, 1 insertion(+)
> 2018-07-26T08:18:41.1489370Z  create mode 100644 old4.t
> 2018-07-26T08:18:41.1502887Z ++ git -C client/ tag old4
> 2018-07-26T08:18:41.1824546Z ++ test_config -C client fetch.negotiationalgorithm skipping
> 2018-07-26T08:18:41.1825670Z ++ config_dir=
> 2018-07-26T08:18:41.1826072Z ++ test -C = -C
> 2018-07-26T08:18:41.1826299Z ++ shift
> 2018-07-26T08:18:41.1826528Z ++ config_dir=client
> 2018-07-26T08:18:41.1826809Z ++ shift
> 2018-07-26T08:18:41.1827079Z ++ test_when_finished 'test_unconfig -C '\''client'\'' '\''fetch.negotiationalgorithm'\'''
> 2018-07-26T08:18:41.1827289Z ++ test 0 = 0
> 2018-07-26T08:18:41.1827561Z ++ test_cleanup='{ test_unconfig -C '\''client'\'' '\''fetch.negotiationalgorithm'\''
> 2018-07-26T08:18:41.1827751Z 		} && (exit "$eval_ret"); eval_ret=$?; :'
> 2018-07-26T08:18:41.1827930Z ++ git -C client config fetch.negotiationalgorithm skipping
> 2018-07-26T08:18:41.2196451Z +++ pwd
> 2018-07-26T08:18:41.2196831Z +++ builtin pwd -W
> 2018-07-26T08:18:41.2274040Z +++ pwd
> 2018-07-26T08:18:41.2274458Z +++ builtin pwd -W
> 2018-07-26T08:18:41.2285081Z ++ GIT_TRACE_PACKET='D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/trace'
> 2018-07-26T08:18:41.2285515Z ++ git -C client fetch 'D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/server'
> 2018-07-26T08:18:41.3054360Z warning: no common commits
> 2018-07-26T08:18:41.3264762Z From D:/a/1/s/t/trash directory.t5552-skipping-fetch-negotiator/server
> 2018-07-26T08:18:41.3265204Z  * branch            HEAD       -> FETCH_HEAD
> 2018-07-26T08:18:41.3362819Z ++ have_sent c2 c1 old4 old2 old1
> 2018-07-26T08:18:41.3370525Z ++ test 5 -ne 0
> 2018-07-26T08:18:41.3423124Z +++ git -C client rev-parse c2
> 2018-07-26T08:18:41.3756643Z ++ grep 'fetch> have 9ab46928dc282aa09f4dbf96893a252e058e7e8e' trace
> 2018-07-26T08:18:41.3878403Z packet:        fetch> have 9ab46928dc282aa09f4dbf96893a252e058e7e8e
> 2018-07-26T08:18:41.3883984Z ++ test 0 -ne 0
> 2018-07-26T08:18:41.3884900Z ++ shift
> 2018-07-26T08:18:41.3885199Z ++ test 4 -ne 0
> 2018-07-26T08:18:41.3938298Z +++ git -C client rev-parse c1
> 2018-07-26T08:18:41.4243782Z ++ grep 'fetch> have dc824fafb05f3229aedf1f320bbe572e35364dfe' trace
> 2018-07-26T08:18:41.4375402Z packet:        fetch> have dc824fafb05f3229aedf1f320bbe572e35364dfe
> 2018-07-26T08:18:41.4383803Z ++ test 0 -ne 0
> 2018-07-26T08:18:41.4384733Z ++ shift
> 2018-07-26T08:18:41.4385018Z ++ test 3 -ne 0
> 2018-07-26T08:18:41.4436622Z +++ git -C client rev-parse old4
> 2018-07-26T08:18:41.4749084Z ++ grep 'fetch> have caef059de69917b9119176a11b88afcef769331d' trace
> 2018-07-26T08:18:41.4888266Z ++ test 1 -ne 0
> 2018-07-26T08:18:41.4941092Z +++ git -C client rev-parse old4
> 2018-07-26T08:18:41.5253206Z ++ echo 'No have caef059de69917b9119176a11b88afcef769331d (old4)'
> 2018-07-26T08:18:41.5253602Z ++ return 1
> 2018-07-26T08:18:41.5254746Z error: last command exited with $?=1
> 2018-07-26T08:18:41.5254865Z No have caef059de69917b9119176a11b88afcef769331d (old4)
> 2018-07-26T08:18:41.5260970Z not ok 4 - handle clock skew
> 2018-07-26T08:18:41.5441990Z #	
> 2018-07-26T08:18:41.5442184Z #		rm -rf server client trace &&
> 2018-07-26T08:18:41.5442422Z #		git init server &&
> 2018-07-26T08:18:41.5448007Z #		test_commit -C server to_fetch &&
> 2018-07-26T08:18:41.5448223Z #	
> 2018-07-26T08:18:41.5448357Z #		git init client &&
> 2018-07-26T08:18:41.5448466Z #	
> 2018-07-26T08:18:41.5448661Z #		# 2 regular commits
> 2018-07-26T08:18:41.5448810Z #		test_tick=2000000000 &&
> 2018-07-26T08:18:41.5449231Z #		test_commit -C client c1 &&
> 2018-07-26T08:18:41.5449393Z #		test_commit -C client c2 &&
> 2018-07-26T08:18:41.5449509Z #	
> 2018-07-26T08:18:41.5449679Z #		# 4 old commits
> 2018-07-26T08:18:41.5449859Z #		test_tick=1000000000 &&
> 2018-07-26T08:18:41.5450017Z #		git -C client checkout c1 &&
> 2018-07-26T08:18:41.5450220Z #		test_commit -C client old1 &&
> 2018-07-26T08:18:41.5450343Z #		test_commit -C client old2 &&
> 2018-07-26T08:18:41.5450449Z #		test_commit -C client old3 &&
> 2018-07-26T08:18:41.5450667Z #		test_commit -C client old4 &&
> 2018-07-26T08:18:41.5450821Z #	
> 2018-07-26T08:18:41.5450954Z #		# "c2" and "c1" are popped first, then "old4" to "old1". "old1" would
> 2018-07-26T08:18:41.5451133Z #		# normally be skipped, but is treated as a commit without a parent here
> 2018-07-26T08:18:41.5451392Z #		# and sent, because (due to clock skew) its only parent has already been
> 2018-07-26T08:18:41.5451547Z #		# popped off the priority queue.
> 2018-07-26T08:18:41.5451675Z #		test_config -C client fetch.negotiationalgorithm skipping &&
> 2018-07-26T08:18:41.5451829Z #		GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
> 2018-07-26T08:18:41.5451961Z #		have_sent c2 c1 old4 old2 old1 &&
> 2018-07-26T08:18:41.5452091Z #		have_not_sent old3
> 2018-07-26T08:18:41.5452186Z #	
> 

Re: [PATCH] negotiator/skipping: skip commits during fetch

[Jonathan Tan]

> Hi Jonathan,
> 
> On Mon, 16 Jul 2018, Jonathan Tan wrote:
> 
> >  t/t5552-skipping-fetch-negotiator.sh | 179 +++++++++++++++++++
> 
> This test seems to be failing consistently in the recent `pu` builds:
> 
> https://git-for-windows.visualstudio.com/git/_build/results?buildId=14337&view=logs
> 
> Could you have a look, please?

Hmm...on my Linux computer, this test passes on both pu (as of the time
of writing) and 838143aa5c ("Merge branch 'ab/newhash-is-sha256' into
pu", 2018-07-25) (pu at the time of that build, according to the website
you linked above). If you could rerun that test with additional code,
could you add a "cat trace" and show me what the client sends? When I do
that, the relevant parts are:

  packet:        fetch> have 9ab46928dc282aa09f4dbf96893a252e058e7e8e
  packet:        fetch> have dc824fafb05f3229aedf1f320bbe572e35364dfe
  packet:        fetch> have caef059de69917b9119176a11b88afcef769331d
  packet:        fetch> have 41bd8dc092ee110ba80e350a346ec507ab2e42a0
  packet:        fetch> have e9a2c092a8e911567a377c881a7f6031e7f892ea
  packet:        fetch> done

which is exactly as I (and the test) expect.

Two possible reasons for the discrepancy that I can think of offhand are
that (1) my computer generates different commits from your test system,
and (2) the priority queue pops commits in a different order. For (1),
that's not possible because the SHA-1s are the same (as can be seen by
comparing your link and the "have" lines I quoted above), and for (2),
the code seems OK:

  static int compare(const void *a_, const void *b_, void *unused)
  {
  	const struct entry *a = a_;
  	const struct entry *b = b_;
  	return compare_commits_by_commit_date(a->commit, b->commit, NULL);
  }

Let me know if you can observe the output of "cat trace" or if you have
any other ideas.

Re: [PATCH] negotiator/skipping: skip commits during fetch

[Johannes Schindelin]

Hi Jonathan,

On Thu, 26 Jul 2018, Jonathan Tan wrote:

> > On Mon, 16 Jul 2018, Jonathan Tan wrote:
> > 
> > >  t/t5552-skipping-fetch-negotiator.sh | 179 +++++++++++++++++++
> > 
> > This test seems to be failing consistently in the recent `pu` builds:
> > 
> > https://git-for-windows.visualstudio.com/git/_build/results?buildId=14337&view=logs
> > 
> > Could you have a look, please?
> 
> Hmm...on my Linux computer, this test passes on both pu (as of the time
> of writing) and 838143aa5c ("Merge branch 'ab/newhash-is-sha256' into
> pu", 2018-07-25) (pu at the time of that build, according to the website
> you linked above). If you could rerun that test with additional code,
> could you add a "cat trace" and show me what the client sends?

I can give you something even better: a playground. Just open a PR at
https://github.com/gitgitgadget/git (all of the branches on gitster/git ar
mirrored, including yours, I am sure, so you can target that branch
specifically).

Once you open a Pull Request, it will automatically build and run the test
suite on Windows, macOS and Linux. You will see it in the "checks" section
on the bottom. Example for my range-diff series:

https://git-for-windows.visualstudio.com/git/_build/results?buildId=14279

For a quicker turnaround, you could add a commit that forces the `all`
target in `t/Makefile` to run only your test.

> When I do that, the relevant parts are:
> 
>   packet:        fetch> have 9ab46928dc282aa09f4dbf96893a252e058e7e8e
>   packet:        fetch> have dc824fafb05f3229aedf1f320bbe572e35364dfe
>   packet:        fetch> have caef059de69917b9119176a11b88afcef769331d
>   packet:        fetch> have 41bd8dc092ee110ba80e350a346ec507ab2e42a0
>   packet:        fetch> have e9a2c092a8e911567a377c881a7f6031e7f892ea
>   packet:        fetch> done
> 
> which is exactly as I (and the test) expect.
> 
> Two possible reasons for the discrepancy that I can think of offhand are
> that (1) my computer generates different commits from your test system,
> and (2) the priority queue pops commits in a different order. For (1),
> that's not possible because the SHA-1s are the same (as can be seen by
> comparing your link and the "have" lines I quoted above), and for (2),
> the code seems OK:
> 
>   static int compare(const void *a_, const void *b_, void *unused)
>   {
>   	const struct entry *a = a_;
>   	const struct entry *b = b_;
>   	return compare_commits_by_commit_date(a->commit, b->commit, NULL);
>   }
> 
> Let me know if you can observe the output of "cat trace" or if you have
> any other ideas.

Like I said, you can use those "CI" builds, I think that would be more
effective than if you waited for me to react, I am quite overwhelmed these
days.

Ciao,
Dscho

Re: [PATCH] negotiator/skipping: skip commits during fetch

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 16 2018, Jonathan Tan wrote:

Didn't catch this until this was in next, sorry.

Re-arranged the diff a bit:

> -void fetch_negotiator_init(struct fetch_negotiator *negotiator)
> +void fetch_negotiator_init(struct fetch_negotiator *negotiator,
> +			   const char *algorithm)
>  {
> +	if (algorithm && !strcmp(algorithm, "skipping")) {
> +		skipping_negotiator_init(negotiator);
> +		return;
> +	}
>  	default_negotiator_init(negotiator);
>  }

Okey, I understand that's how it works now, but....

> +fetch.negotiationAlgorithm::
> +	Control how information about the commits in the local repository is
> +	sent when negotiating the contents of the packfile to be sent by the
> +	server. Set to "skipping" to use an algorithm that skips commits in an
> +	effort to converge faster, but may result in a larger-than-necessary
> +	packfile; any other value instructs Git to use the default algorithm
> +	that never skips commits (unless the server has acknowledged it or one
> +	of its descendants).
> +

...let's instead document that there's just the values "skipping" and
"default", and say "default" is provided by default, and perhaps change
the code to warn about anything that isn't those two.

Then we're not painting ourselves into a corner by needing to break a
promise in the docs ("any other value instructs Git to use the default")
if we add a new one of these, and aren't silently falling back on the
default if we add new-fancy-algo the user's version doesn't know about.

Also, switching gears entirely, I'm very excited about this whole thing
because it allows me to address something I've been meaning to get to
for a while.

At work I sometimes want to see what commits I've made to all our git
repos, for remembering what I was doing last February or whatever (this
is for filling in quarterly reports).

So I have this script that basically does this:

    for repo in $(get-list-of-all-the-things)
    do
        git config "remote.$repo.url" git@git-server.example.com:$repo.git
        git config "remote.$repo.fetch" "+HEAD:$repo/HEAD"
        git config "remote.$repo.tagOpt" "--no-tags"
    done &&
    git fetch --all

I.e. for every repo like git/git I'll fetch its upstream HEAD as the
branch git/git/HEAD. Then I can do stuff like:

    git shortlog --author=Ævar --since=2018-02-01 --until=2018-03-01

Now, running that "git fetch --all" takes ages, and I know why. It's
because the in the negotiation for "git fetch some/small-repo" I'm
emitting hundreds of thousands of "have" lines for SHA1s found in other
unrelated repos, only to get a NAK for all of them.

One way to fix that with this facility would be to have some way to pass
in arguments, similar to what we have for merge drivers, so I can say
"just emit 'have' lines for stuff found in this branch". The most
pathological cases are when I'm fetching a remote that has one commit,
and I'm desperately trying to find something in common by asking if the
remote has hundreds of K of commits it has no chance of having.

Or there may be some smarter way to do this, what do you think?

Re: [PATCH] negotiator/skipping: skip commits during fetch

[Jonathan Tan]

> > +fetch.negotiationAlgorithm::
> > +	Control how information about the commits in the local repository is
> > +	sent when negotiating the contents of the packfile to be sent by the
> > +	server. Set to "skipping" to use an algorithm that skips commits in an
> > +	effort to converge faster, but may result in a larger-than-necessary
> > +	packfile; any other value instructs Git to use the default algorithm
> > +	that never skips commits (unless the server has acknowledged it or one
> > +	of its descendants).
> > +
> 
> ...let's instead document that there's just the values "skipping" and
> "default", and say "default" is provided by default, and perhaps change
> the code to warn about anything that isn't those two.
> 
> Then we're not painting ourselves into a corner by needing to break a
> promise in the docs ("any other value instructs Git to use the default")
> if we add a new one of these, and aren't silently falling back on the
> default if we add new-fancy-algo the user's version doesn't know about.

My intention was to allow future versions of Git to introduce more
algorithms, but have older versions of Git still work even if a
repository is configured to use a newer algorithm. But your suggestion
is reasonable too.

> Now, running that "git fetch --all" takes ages, and I know why. It's
> because the in the negotiation for "git fetch some/small-repo" I'm
> emitting hundreds of thousands of "have" lines for SHA1s found in other
> unrelated repos, only to get a NAK for all of them.
> 
> One way to fix that with this facility would be to have some way to pass
> in arguments, similar to what we have for merge drivers, so I can say
> "just emit 'have' lines for stuff found in this branch". The most
> pathological cases are when I'm fetching a remote that has one commit,
> and I'm desperately trying to find something in common by asking if the
> remote has hundreds of K of commits it has no chance of having.
> 
> Or there may be some smarter way to do this, what do you think?

Well, there is already a commit in "next" that does this :-)

3390e42adb ("fetch-pack: support negotiation tip whitelist", 2018-07-03)

[PATCH 0/2] negotiator: improve recent behavior + docs

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 31 2018, Jonathan Tan wrote:

>> > +fetch.negotiationAlgorithm::
>> > +  Control how information about the commits in the local repository is
>> > +  sent when negotiating the contents of the packfile to be sent by the
>> > +  server. Set to "skipping" to use an algorithm that skips commits in an
>> > +  effort to converge faster, but may result in a larger-than-necessary
>> > +  packfile; any other value instructs Git to use the default algorithm
>> > +  that never skips commits (unless the server has acknowledged it or one
>> > +  of its descendants).
>> > +
>> 
>> ...let's instead document that there's just the values "skipping" and
>> "default", and say "default" is provided by default, and perhaps change
>> the code to warn about anything that isn't those two.
>> 
>> Then we're not painting ourselves into a corner by needing to break a
>> promise in the docs ("any other value instructs Git to use the default")
>> if we add a new one of these, and aren't silently falling back on the
>> default if we add new-fancy-algo the user's version doesn't know about.
>
> My intention was to allow future versions of Git to introduce more
> algorithms, but have older versions of Git still work even if a
> repository is configured to use a newer algorithm. But your suggestion
> is reasonable too.

I think 01/02 in this patch series implements something that's better
& more future-proof.

>> Now, running that "git fetch --all" takes ages, and I know why. It's
>> because the in the negotiation for "git fetch some/small-repo" I'm
>> emitting hundreds of thousands of "have" lines for SHA1s found in other
>> unrelated repos, only to get a NAK for all of them.
>> 
>> One way to fix that with this facility would be to have some way to pass
>> in arguments, similar to what we have for merge drivers, so I can say
>> "just emit 'have' lines for stuff found in this branch". The most
>> pathological cases are when I'm fetching a remote that has one commit,
>> and I'm desperately trying to find something in common by asking if the
>> remote has hundreds of K of commits it has no chance of having.
>> 
>> Or there may be some smarter way to do this, what do you think?
>
> Well, there is already a commit in "next" that does this :-)
>
> 3390e42adb ("fetch-pack: support negotiation tip whitelist", 2018-07-03)

That's awesome. This is exactly what I wanted, this patch series also
fixes another small issue in 02/02; which is that the docs for the two
really should cross-link to make these discoverable from one another.

Another thing I noticed, which I have not fixed, is that there's no
way to say "I don't want to you to say that anything is in
common". Currently I'm hacking around in my script by doing:

    parallel --eta --verbose --progress '
        if git rev-parse {}/HEAD 2>/dev/null
        then
            git fetch --negotiation-tip={}/HEAD {}
        else
            git fetch --negotiation-tip=ref-i-have-with-one-commit {}
        fi
    ' ::: $(git remote)

I.e. the way I'm doing this is I add all the remotes first, then I
fetch them all in parallel, but because the first time around I don't
have anything for that remote (and they don't share any commits) I
need to fake it up and pretend to be fetching from a repo that has
just one commit.

It would be better if I could somehow say that I don't mind that the
ref doesn't exist, but currently you either error out with this, or
ignore the glob, depending on the mode.

So I want this, but can't think of a less shitty UI than:

    git fetch --negotiation-tip=$REF --negotiation-tip-error-handling=missing-ref-means-no-want

Or something equally atrocious, do you have any better ideas?

Ævar Arnfjörð Bjarmason (2):
  negotiator: unknown fetch.negotiationAlgorithm should error out
  fetch doc: cross-link two new negotiation options

 Documentation/config.txt             |  5 ++++-
 Documentation/fetch-options.txt      |  3 +++
 fetch-negotiator.c                   | 12 +++++++++---
 t/t5552-skipping-fetch-negotiator.sh | 23 +++++++++++++++++++++++
 4 files changed, 39 insertions(+), 4 deletions(-)

-- 
2.18.0.345.g5c9ce644c3

[PATCH 1/2] negotiator: unknown fetch.negotiationAlgorithm should error out

[Ævar Arnfjörð Bjarmason]

Change the handling of fetch.negotiationAlgorithm=<str> to error out
on unknown strings, i.e. everything except "default" or "skipping".

This changes the behavior added in 42cc7485a2 ("negotiator/skipping:
skip commits during fetch", 2018-07-16) which would ignore all unknown
values and silently fall back to the "default" value.

For a feature like this it's much better to produce an error than
proceed. We don't want users to debug some amazingly slow fetch that
should benefit from "skipping", only to find that they'd forgotten to
deploy the new git version on that particular machine.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt             |  3 ++-
 fetch-negotiator.c                   | 12 +++++++++---
 t/t5552-skipping-fetch-negotiator.sh | 23 +++++++++++++++++++++++
 3 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 63365dcf3d..84f73d7458 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1536,9 +1536,10 @@ fetch.negotiationAlgorithm::
 	sent when negotiating the contents of the packfile to be sent by the
 	server. Set to "skipping" to use an algorithm that skips commits in an
 	effort to converge faster, but may result in a larger-than-necessary
-	packfile; any other value instructs Git to use the default algorithm
+	packfile; The default is "default" which instructs Git to use the default algorithm
 	that never skips commits (unless the server has acknowledged it or one
 	of its descendants).
+	Unknown values will cause 'git fetch' to error out.
 
 format.attach::
 	Enable multipart/mixed attachments as the default for
diff --git a/fetch-negotiator.c b/fetch-negotiator.c
index 5d283049f4..d6d685cba0 100644
--- a/fetch-negotiator.c
+++ b/fetch-negotiator.c
@@ -6,9 +6,15 @@
 void fetch_negotiator_init(struct fetch_negotiator *negotiator,
 			   const char *algorithm)
 {
-	if (algorithm && !strcmp(algorithm, "skipping")) {
-		skipping_negotiator_init(negotiator);
-		return;
+	if (algorithm) {
+		if (!strcmp(algorithm, "skipping")) {
+			skipping_negotiator_init(negotiator);
+			return;
+		} else if (!strcmp(algorithm, "default")) {
+			/* Fall through to default initialization */
+		} else {
+			die("unknown fetch negotiation algorithm '%s'", algorithm);
+		}
 	}
 	default_negotiator_init(negotiator);
 }
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index 0a8e0e42ed..3b60bd44e0 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -47,6 +47,29 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc
 	have_not_sent c6 c4 c3
 '
 
+test_expect_success 'unknown fetch.negotiationAlgorithm values error out' '
+	rm -rf server client trace &&
+	git init server &&
+	test_commit -C server to_fetch &&
+
+	git init client &&
+	test_commit -C client on_client &&
+	git -C client checkout on_client &&
+
+	test_config -C client fetch.negotiationAlgorithm invalid &&
+	test_must_fail git -C client fetch "$(pwd)/server" 2>err &&
+	test_i18ngrep "unknown fetch negotiation algorithm" err &&
+
+	# Explicit "default" value
+	test_config -C client fetch.negotiationAlgorithm default &&
+	git -C client -c fetch.negotiationAlgorithm=default fetch "$(pwd)/server" &&
+
+	# Implementation detail: If there is nothing to fetch, we will not error out
+	test_config -C client fetch.negotiationAlgorithm invalid &&
+	git -C client fetch "$(pwd)/server" 2>err &&
+	test_i18ngrep ! "unknown fetch negotiation algorithm" err
+'
+
 test_expect_success 'when two skips collide, favor the larger one' '
 	rm -rf server client trace &&
 	git init server &&
-- 
2.18.0.345.g5c9ce644c3

[PATCH 2/2] fetch doc: cross-link two new negotiation options

[Ævar Arnfjörð Bjarmason]

Users interested in the fetch.negotiationAlgorithm variable added in
42cc7485a2 ("negotiator/skipping: skip commits during fetch",
2018-07-16) are probably interested in the related --negotiation-tip
option added in 3390e42adb ("fetch-pack: support negotiation tip
whitelist", 2018-07-02).

Change the documentation for those two to reference one another to
point readers in the right direction.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt        | 2 ++
 Documentation/fetch-options.txt | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 84f73d7458..dc55ff17e0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1540,6 +1540,8 @@ fetch.negotiationAlgorithm::
 	that never skips commits (unless the server has acknowledged it or one
 	of its descendants).
 	Unknown values will cause 'git fetch' to error out.
++
+See also the `--negotiation-tip` option for linkgit:git-fetch[1].
 
 format.attach::
 	Enable multipart/mixed attachments as the default for
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 2d09f87b4b..8bc36af4b1 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -57,6 +57,9 @@ commits reachable from any of the given commits.
 The argument to this option may be a glob on ref names, a ref, or the (possibly
 abbreviated) SHA-1 of a commit. Specifying a glob is equivalent to specifying
 this option multiple times, one for each matching ref name.
++
+See also the `fetch.negotiationAlgorithm` configuration variable
+documented in linkgit:git-config[1].
 
 ifndef::git-pull[]
 --dry-run::
-- 
2.18.0.345.g5c9ce644c3

Re: [PATCH 0/2] negotiator: improve recent behavior + docs

[Jonathan Tan]

> I think 01/02 in this patch series implements something that's better
> & more future-proof.

Thanks. Both patches are:
Reviewed-by: Jonathan Tan <jonathantanmy@google.com>

A small note:

> -	packfile; any other value instructs Git to use the default algorithm
> +	packfile; The default is "default" which instructs Git to use the default algorithm

I think we generally don't capitalize words after semicolons.

Thanks for noticing that the check of fetch.negotiationAlgorithm only
happens when a negotiation actually occurs - before your patches, it
didn't really matter because we tolerated anything, but now we do. I
think this is fine - as far as I know, Git commands generally only read
the configs relevant to them, and if fetch.negotiationAlgorithm is not
relevant in a certain situation, we don't need to read it.

> That's awesome. This is exactly what I wanted, this patch series also
> fixes another small issue in 02/02; which is that the docs for the two
> really should cross-link to make these discoverable from one another.

That's a good idea; thanks for doing it.

> I.e. the way I'm doing this is I add all the remotes first, then I
> fetch them all in parallel, but because the first time around I don't
> have anything for that remote (and they don't share any commits) I
> need to fake it up and pretend to be fetching from a repo that has
> just one commit.
> 
> It would be better if I could somehow say that I don't mind that the
> ref doesn't exist, but currently you either error out with this, or
> ignore the glob, depending on the mode.
> 
> So I want this, but can't think of a less shitty UI than:
> 
>     git fetch --negotiation-tip=$REF --negotiation-tip-error-handling=missing-ref-means-no-want
> 
> Or something equally atrocious, do you have any better ideas?

If you wanted to do this, it seems better to me to just declare a "null"
negotiation algorithm that does not perform any negotiation at all.

Re: [PATCH 0/2] negotiator: improve recent behavior + docs

[Ævar Arnfjörð Bjarmason]

On Wed, Aug 01 2018, Jonathan Tan wrote:

>> I think 01/02 in this patch series implements something that's better
>> & more future-proof.
>
> Thanks. Both patches are:
> Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
>
> A small note:
>
>> -	packfile; any other value instructs Git to use the default algorithm
>> +	packfile; The default is "default" which instructs Git to use the default algorithm
>
> I think we generally don't capitalize words after semicolons.

Yeah I think that's right. Will fix (or if there's no other comments
perhaps Junio will munge it...) :)

> Thanks for noticing that the check of fetch.negotiationAlgorithm only
> happens when a negotiation actually occurs - before your patches, it
> didn't really matter because we tolerated anything, but now we do. I
> think this is fine - as far as I know, Git commands generally only read
> the configs relevant to them, and if fetch.negotiationAlgorithm is not
> relevant in a certain situation, we don't need to read it.

Yeah I think that's OK.

>> That's awesome. This is exactly what I wanted, this patch series also
>> fixes another small issue in 02/02; which is that the docs for the two
>> really should cross-link to make these discoverable from one another.
>
> That's a good idea; thanks for doing it.
>
>> I.e. the way I'm doing this is I add all the remotes first, then I
>> fetch them all in parallel, but because the first time around I don't
>> have anything for that remote (and they don't share any commits) I
>> need to fake it up and pretend to be fetching from a repo that has
>> just one commit.
>>
>> It would be better if I could somehow say that I don't mind that the
>> ref doesn't exist, but currently you either error out with this, or
>> ignore the glob, depending on the mode.
>>
>> So I want this, but can't think of a less shitty UI than:
>>
>>     git fetch --negotiation-tip=$REF --negotiation-tip-error-handling=missing-ref-means-no-want
>>
>> Or something equally atrocious, do you have any better ideas?
>
> If you wanted to do this, it seems better to me to just declare a "null"
> negotiation algorithm that does not perform any negotiation at all.

I think such an algorithm is a good idea in general, especially for
testing, and yeah, maybe that's the best way out of this, i.e. to do:

    if git rev-parse {}/HEAD 2>/dev/null
    then
        git fetch --negotiation-tip={}/HEAD {}
    else
        git -c fetch.negotiationAlgorithm=null fetch {}
    fi

Would such an algorithm be added by overriding default.c's add_tip
function to never add anything by calling default_negotiator_init()
followed by null_negotiator_init(), which would only override add_tip?
(yay C OO)

If so from fetch-pack.c it looks like there may be the limitation on the
interface that the negotiator can't exit early (in
fetch-pack.c:mark_tips). But I've just skimmed this, so maybe I've
missed something.

Also, looks like because of the current interface =null and
--negotiation-tip=* would (somewhat confusingly) do a "real" negotiation
if done that way, since it'll bypass the API and insert tips for it to
negotiate, but it looks like overriding next() will get around that.

Re: [PATCH] negotiator/skipping: skip commits during fetch

[Johannes Schindelin]

Hi Jonathan,

On Fri, 27 Jul 2018, Johannes Schindelin wrote:

> On Thu, 26 Jul 2018, Jonathan Tan wrote:
> 
> > > On Mon, 16 Jul 2018, Jonathan Tan wrote:
> > > 
> > > >  t/t5552-skipping-fetch-negotiator.sh | 179 +++++++++++++++++++
> > > 
> > > This test seems to be failing consistently in the recent `pu` builds:
> > > 
> > > https://git-for-windows.visualstudio.com/git/_build/results?buildId=14337&view=logs
> > > 
> > > Could you have a look, please?
> > 
> > Hmm...on my Linux computer, this test passes on both pu (as of the time
> > of writing) and 838143aa5c ("Merge branch 'ab/newhash-is-sha256' into
> > pu", 2018-07-25) (pu at the time of that build, according to the website
> > you linked above). If you could rerun that test with additional code,
> > could you add a "cat trace" and show me what the client sends?
> 
> I can give you something even better: a playground. Just open a PR at
> https://github.com/gitgitgadget/git (all of the branches on gitster/git ar
> mirrored, including yours, I am sure, so you can target that branch
> specifically).
> 
> Once you open a Pull Request, it will automatically build and run the test
> suite on Windows, macOS and Linux. You will see it in the "checks" section
> on the bottom. Example for my range-diff series:
> 
> https://git-for-windows.visualstudio.com/git/_build/results?buildId=14279
> 
> For a quicker turnaround, you could add a commit that forces the `all`
> target in `t/Makefile` to run only your test.
> 
> > When I do that, the relevant parts are:
> > 
> >   packet:        fetch> have 9ab46928dc282aa09f4dbf96893a252e058e7e8e
> >   packet:        fetch> have dc824fafb05f3229aedf1f320bbe572e35364dfe
> >   packet:        fetch> have caef059de69917b9119176a11b88afcef769331d
> >   packet:        fetch> have 41bd8dc092ee110ba80e350a346ec507ab2e42a0
> >   packet:        fetch> have e9a2c092a8e911567a377c881a7f6031e7f892ea
> >   packet:        fetch> done
> > 
> > which is exactly as I (and the test) expect.
> > 
> > Two possible reasons for the discrepancy that I can think of offhand are
> > that (1) my computer generates different commits from your test system,
> > and (2) the priority queue pops commits in a different order. For (1),
> > that's not possible because the SHA-1s are the same (as can be seen by
> > comparing your link and the "have" lines I quoted above), and for (2),
> > the code seems OK:
> > 
> >   static int compare(const void *a_, const void *b_, void *unused)
> >   {
> >   	const struct entry *a = a_;
> >   	const struct entry *b = b_;
> >   	return compare_commits_by_commit_date(a->commit, b->commit, NULL);
> >   }
> > 
> > Let me know if you can observe the output of "cat trace" or if you have
> > any other ideas.
> 
> Like I said, you can use those "CI" builds, I think that would be more
> effective than if you waited for me to react, I am quite overwhelmed these
> days.

Hopefully you have a chance to do so. I got the impression that it is
actually more of a flakey test than a consistent test failure:

https://git-for-windows.visualstudio.com/git/_build/results?buildId=15015&view=logs

Ciao,
Dscho

Re: [PATCH 0/2] negotiator: improve recent behavior + docs

[Jonathan Tan]

> > If you wanted to do this, it seems better to me to just declare a "null"
> > negotiation algorithm that does not perform any negotiation at all.
> 
> I think such an algorithm is a good idea in general, especially for
> testing, and yeah, maybe that's the best way out of this, i.e. to do:
> 
>     if git rev-parse {}/HEAD 2>/dev/null
>     then
>         git fetch --negotiation-tip={}/HEAD {}
>     else
>         git -c fetch.negotiationAlgorithm=null fetch {}
>     fi
> 
> Would such an algorithm be added by overriding default.c's add_tip
> function to never add anything by calling default_negotiator_init()
> followed by null_negotiator_init(), which would only override add_tip?
> (yay C OO)
> 
> If so from fetch-pack.c it looks like there may be the limitation on the
> interface that the negotiator can't exit early (in
> fetch-pack.c:mark_tips). But I've just skimmed this, so maybe I've
> missed something.

(I was reminded to reply to this offlist - sorry for the late reply.)

I think too many things need to be replaced (known_common, add_tip, and
ack all need to do nothing), so it's best to start from scratch. That
way, we also don't need to deal with the subtleties of C OO :-)

> Also, looks like because of the current interface =null and
> --negotiation-tip=* would (somewhat confusingly) do a "real" negotiation
> if done that way, since it'll bypass the API and insert tips for it to
> negotiate, but it looks like overriding next() will get around that.

If you do it as I suggest (in particular, add_tip doing nothing) then
there is the opposite problem that it won't be easy to inform the user
that --negotiation-tip does nothing in this case. Maybe there needs to
be an "accepts_tips" field in struct fetch_negotiator that, if false,
means that custom tips (or any tips) are not accepted, allowing the
caller of the negotiator to print a warning message in this case.

Re: [PATCH 0/2] negotiator: improve recent behavior + docs

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 27 2018, Jonathan Tan wrote:

>> > If you wanted to do this, it seems better to me to just declare a "null"
>> > negotiation algorithm that does not perform any negotiation at all.
>>
>> I think such an algorithm is a good idea in general, especially for
>> testing, and yeah, maybe that's the best way out of this, i.e. to do:
>>
>>     if git rev-parse {}/HEAD 2>/dev/null
>>     then
>>         git fetch --negotiation-tip={}/HEAD {}
>>     else
>>         git -c fetch.negotiationAlgorithm=null fetch {}
>>     fi
>>
>> Would such an algorithm be added by overriding default.c's add_tip
>> function to never add anything by calling default_negotiator_init()
>> followed by null_negotiator_init(), which would only override add_tip?
>> (yay C OO)
>>
>> If so from fetch-pack.c it looks like there may be the limitation on the
>> interface that the negotiator can't exit early (in
>> fetch-pack.c:mark_tips). But I've just skimmed this, so maybe I've
>> missed something.
>
> (I was reminded to reply to this offlist - sorry for the late reply.)
>
> I think too many things need to be replaced (known_common, add_tip, and
> ack all need to do nothing), so it's best to start from scratch. That
> way, we also don't need to deal with the subtleties of C OO :-)
>
>> Also, looks like because of the current interface =null and
>> --negotiation-tip=* would (somewhat confusingly) do a "real" negotiation
>> if done that way, since it'll bypass the API and insert tips for it to
>> negotiate, but it looks like overriding next() will get around that.
>
> If you do it as I suggest (in particular, add_tip doing nothing) then
> there is the opposite problem that it won't be easy to inform the user
> that --negotiation-tip does nothing in this case. Maybe there needs to
> be an "accepts_tips" field in struct fetch_negotiator that, if false,
> means that custom tips (or any tips) are not accepted, allowing the
> caller of the negotiator to print a warning message in this case.

Thanks, yeah it seems the interface would need to be tweaked for such a
"null" negotiator.

Some more general questions (which I can turn into docs once I
understand this). If I run this, as a testcase for two random repos
where I "fetch" an unrelated one and use the first ever commit to
git.git as an alias for this "null" negotiatior, i.e. "just present this
one commit":

    (
        rm -rf /tmp/git &&
        git clone https://github.com/git/git.git /tmp/git &&
        cd /tmp/git &&
        git remote add gitlab-shell https://github.com/cr-marcstevens/sha1collisiondetection &&
        GIT_TRACE_PACKET=/tmp/git/packet.trace git fetch --negotiation-tip=$(git log --reverse|head -n 1|cut -d ' ' -f2) gitlab-shell &&
        grep -c "fetch-pack> have" /tmp/git/packet.trace
    )

I get:

    warning: Ignoring --negotiation-tip because the protocol does not support it.

And the grep -c shows we tried to present 55170 commits in "have" lines
to the server. Now, change that to SSH and all is well:

    (
        rm -rf /tmp/git &&
        git clone git@github.com:git/git.git /tmp/git &&
        cd /tmp/git &&
        git remote add gitlab-shell git@github.com:cr-marcstevens/sha1collisiondetection &&
        GIT_TRACE_PACKET=/tmp/git/packet.trace git fetch --negotiation-tip=$(git log --reverse|head -n 1|cut -d ' ' -f2) gitlab-shell &&
        grep -c "fetch-pack> have" /tmp/git/packet.trace
    )

I don't understand this limitation. With the SSH version we skip
straight to saying we "want" with just the 1 "have" line of
"e83c5163316f89bfbde7d9ab23ca2e25604af290".

Why aren't we doing the same over http? I don't get how protocol support
is needed, it's us who decide to send over the "have" lines. Some
variant of this does work over "skipping":

    (
        rm -rf /tmp/git &&
        git clone https://github.com/git/git.git /tmp/git &&
        cd /tmp/git &&
        git remote add gitlab-shell https://github.com/cr-marcstevens/sha1collisiondetection &&
        GIT_TRACE_PACKET=/tmp/git/packet.trace git -c fetch.negotiationAlgorithm=skipping fetch gitlab-shell &&
        grep -c "fetch-pack> have" /tmp/git/packet.trace
    )

There we send 14002 "have" lines, which seems expected, but then with
the same thing over SSH we don't send any:

    (
        rm -rf /tmp/git &&
        git clone git@github.com:git/git.git /tmp/git &&
        cd /tmp/git &&
        git remote add gitlab-shell git@github.com:cr-marcstevens/sha1collisiondetection &&
        GIT_TRACE_PACKET=/tmp/git/packet.trace git -c fetch.negotiationAlgorithm=skipping fetch gitlab-shell &&
        grep -c "fetch-pack> have" /tmp/git/packet.trace
    )

So that seems like another bug, and as an aside, a "skipping"
implementation that sends ~1/4 of the commits in the repo seems way less
aggressive than it should be. I was expecting something that would
gradually "ramp up" from the tips. Where say starting at master/next/pu
we present every 100th commit as a "have" until the 1000th commit, then
every 1000 commits until 10k and quickly after that step up the size
rapidly.

Re: [PATCH 0/2] negotiator: improve recent behavior + docs

[Jonathan Tan]

> I get:
> 
>     warning: Ignoring --negotiation-tip because the protocol does not support it.

When I implemented --negotiation-tip, I only implemented it for
protocols that support connect or stateless-connect, because
implementing it fully would have required expanding the protocol helper
functionality. For reference, the commit is 3390e42adb ("fetch-pack:
support negotiation tip whitelist", 2018-07-03).

So HTTPS wouldn't work unless you were using protocol v2.

> So that seems like another bug, and as an aside, a "skipping"
> implementation that sends ~1/4 of the commits in the repo seems way less
> aggressive than it should be. I was expecting something that would
> gradually "ramp up" from the tips. Where say starting at master/next/pu
> we present every 100th commit as a "have" until the 1000th commit, then
> every 1000 commits until 10k and quickly after that step up the size
> rapidly.

I reproduced using your commands, and yes, there is a bug - I'll take a
look.