[v2,1/7] negotiator/null: add null fetch negotiator

Commit Message

Jonathan Tan Aug. 11, 2020, 10:52 p.m. UTC

Add a null fetch negotiator. This is introduced to allow partial clones
to skip the unneeded negotiation step when fetching missing objects
using a "git fetch" subprocess. (The implementation of spawning a "git
fetch" subprocess will be done in a subsequent patch.) But this can also
be useful for end users, e.g. as a blunt fix for object corruption.

Signed-off-by: Jonathan Tan <[e-mail]>
---
 Documentation/config/fetch.txt   |  5 +++-
 Makefile                         |  1 +
 fetch-negotiator.c               |  5 ++++
 negotiator/null.c                | 44 ++++++++++++++++++++++++++++++++
 negotiator/null.h                |  8 ++++++
 repo-settings.c                  |  2 ++
 repository.h                     |  1 +
 t/t5554-null-fetch-negotiator.sh | 22 ++++++++++++++++
 8 files changed, 87 insertions(+), 1 deletion(-)
 create mode 100644 negotiator/null.c
 create mode 100644 negotiator/null.h
 create mode 100755 t/t5554-null-fetch-negotiator.sh

Comments

Derrick Stolee Aug. 12, 2020, 12:55 p.m. UTC | #1

On 8/11/2020 6:52 PM, Jonathan Tan wrote:
> Add a null fetch negotiator. 

I understand the value of this negotiator. I'm concerned about using
"null" as the name, since it has a clear relationship to zero-valued
pointers and that's not what is happening. (My gut feeling starting the
patch was that some function pointers would be NULL or something.)

Instead, might I recommend "noop" or "no_op" in place of "null" here?

Thanks,
-Stolee

Junio C Hamano Aug. 12, 2020, 4:44 p.m. UTC | #2

Derrick Stolee <[e-mail]> writes:

> On 8/11/2020 6:52 PM, Jonathan Tan wrote:
>> Add a null fetch negotiator. 
>
> I understand the value of this negotiator. I'm concerned about using
> "null" as the name, since it has a clear relationship to zero-valued
> pointers and that's not what is happening. (My gut feeling starting the
> patch was that some function pointers would be NULL or something.)
>
> Instead, might I recommend "noop" or "no_op" in place of "null" here?

Personally I am OK with null [*], but noop is also fine.

	Side note.  I actually would find it good to establish the
	pattern that something that does not use NULL pointer as its
	implementation detail can be called null if "null-ness" of
	its behaviour is its defining characteristics.

Thanks.

Jonathan Tan Aug. 12, 2020, 5:29 p.m. UTC | #3

> Derrick Stolee <[e-mail]> writes:
> 
> > On 8/11/2020 6:52 PM, Jonathan Tan wrote:
> >> Add a null fetch negotiator. 
> >
> > I understand the value of this negotiator. I'm concerned about using
> > "null" as the name, since it has a clear relationship to zero-valued
> > pointers and that's not what is happening. (My gut feeling starting the
> > patch was that some function pointers would be NULL or something.)
> >
> > Instead, might I recommend "noop" or "no_op" in place of "null" here?
> 
> Personally I am OK with null [*], but noop is also fine.
> 
> 	Side note.  I actually would find it good to establish the
> 	pattern that something that does not use NULL pointer as its
> 	implementation detail can be called null if "null-ness" of
> 	its behaviour is its defining characteristics.
> 
> Thanks.

OK, in a future version I'll go with "noop".

Patch

diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index 0aaa05e8c0..09aff404be 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -60,7 +60,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; The default is "default" which instructs Git to use the default algorithm
+	packfile; or set to "null" to not send any information at all, which
+	will almost certainly result in a larger-than-necessary packfile, but
+	will skip the negotiation step.
+	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). If `feature.experimental` is enabled, then this
 	setting defaults to "skipping".
diff --git a/Makefile b/Makefile
index 372139f1f2..297ea5e517 100644
--- a/Makefile
+++ b/Makefile
@@ -917,6 +917,7 @@  LIB_OBJS += mergesort.o
 LIB_OBJS += midx.o
 LIB_OBJS += name-hash.o
 LIB_OBJS += negotiator/default.o
+LIB_OBJS += negotiator/null.o
 LIB_OBJS += negotiator/skipping.o
 LIB_OBJS += notes-cache.o
 LIB_OBJS += notes-merge.o
diff --git a/fetch-negotiator.c b/fetch-negotiator.c
index 0a1357dc9d..e2ecbe4367 100644
--- a/fetch-negotiator.c
+++ b/fetch-negotiator.c
@@ -2,6 +2,7 @@ 
 #include "fetch-negotiator.h"
 #include "negotiator/default.h"
 #include "negotiator/skipping.h"
+#include "negotiator/null.h"
 #include "repository.h"
 
 void fetch_negotiator_init(struct repository *r,
@@ -13,6 +14,10 @@  void fetch_negotiator_init(struct repository *r,
 		skipping_negotiator_init(negotiator);
 		return;
 
+	case FETCH_NEGOTIATION_NULL:
+		null_negotiator_init(negotiator);
+		return;
+
 	case FETCH_NEGOTIATION_DEFAULT:
 	default:
 		default_negotiator_init(negotiator);
diff --git a/negotiator/null.c b/negotiator/null.c
new file mode 100644
index 0000000000..1bd834f121
--- /dev/null
+++ b/negotiator/null.c
@@ -0,0 +1,44 @@ 
+#include "cache.h"
+#include "null.h"
+#include "../commit.h"
+#include "../fetch-negotiator.h"
+
+static void known_common(struct fetch_negotiator *n, struct commit *c)
+{
+	/* do nothing */
+}
+
+static void add_tip(struct fetch_negotiator *n, struct commit *c)
+{
+	/* do nothing */
+}
+
+static const struct object_id *next(struct fetch_negotiator *n)
+{
+	return NULL;
+}
+
+static int ack(struct fetch_negotiator *n, struct commit *c)
+{
+	/*
+	 * This negotiator does not emit any commits, so there is no commit to
+	 * be acknowledged. If there is any ack, there is a bug.
+	 */
+	BUG("ack with null negotiator, which does not emit any commits");
+	return 0;
+}
+
+static void release(struct fetch_negotiator *n)
+{
+	/* nothing to release */
+}
+
+void null_negotiator_init(struct fetch_negotiator *negotiator)
+{
+	negotiator->known_common = known_common;
+	negotiator->add_tip = add_tip;
+	negotiator->next = next;
+	negotiator->ack = ack;
+	negotiator->release = release;
+	negotiator->data = NULL;
+}
diff --git a/negotiator/null.h b/negotiator/null.h
new file mode 100644
index 0000000000..96713f280e
--- /dev/null
+++ b/negotiator/null.h
@@ -0,0 +1,8 @@ 
+#ifndef NEGOTIATOR_NULL_H
+#define NEGOTIATOR_NULL_H
+
+struct fetch_negotiator;
+
+void null_negotiator_init(struct fetch_negotiator *negotiator);
+
+#endif
diff --git a/repo-settings.c b/repo-settings.c
index 0918408b34..a8c7e1edd7 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -39,6 +39,8 @@  void prepare_repo_settings(struct repository *r)
 	if (!repo_config_get_string(r, "fetch.negotiationalgorithm", &strval)) {
 		if (!strcasecmp(strval, "skipping"))
 			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;
+		else if (!strcasecmp(strval, "null"))
+			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NULL;
 		else
 			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
 	}
diff --git a/repository.h b/repository.h
index 3c1f7d54bd..f72911a185 100644
--- a/repository.h
+++ b/repository.h
@@ -23,6 +23,7 @@  enum fetch_negotiation_setting {
 	FETCH_NEGOTIATION_NONE = 0,
 	FETCH_NEGOTIATION_DEFAULT = 1,
 	FETCH_NEGOTIATION_SKIPPING = 2,
+	FETCH_NEGOTIATION_NULL = 3,
 };
 
 struct repo_settings {
diff --git a/t/t5554-null-fetch-negotiator.sh b/t/t5554-null-fetch-negotiator.sh
new file mode 100755
index 0000000000..09a8f0d608
--- /dev/null
+++ b/t/t5554-null-fetch-negotiator.sh
@@ -0,0 +1,22 @@ 
+#!/bin/sh
+
+test_description='test null fetch negotiator'
+. ./test-lib.sh
+
+test_expect_success 'null negotiator does not emit any "have"' '
+	rm -f trace &&
+
+	test_create_repo server &&
+	test_commit -C server to_fetch &&
+
+	test_create_repo client &&
+	test_commit -C client we_have &&
+
+	test_config -C client fetch.negotiationalgorithm null &&
+	GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch "$(pwd)/server" &&
+
+	! grep "fetch> have" trace &&
+	grep "fetch> done" trace
+'
+
+test_done