[PATCH 0/4] upload-pack: support a missing-action

[PATCH 0/4] upload-pack: support a missing-action

[Christian Couder]

`git pack-objects` already supports a `--missing=<missing-action>`
option, so that it can avoid erroring out if some objects aren't
available.

It is interesting to have `git upload-pack` support a similar way to
avoid sending some objects in case they aren't available on the
server.

For example, in case both the server and the client are using a
separate promisor remote that contain some objects, it can be better
if the server doesn't try to send such objects back to the client, but
instead let the client get those objects separately from the promisor
remote. (The client needs to have the separate promisor remote
configured, for that to work.)

Another example could be a server where some objects have been
corrupted or deleted. It could still be useful for clients who could
get those objects from another source, like perhaps a different
client, to be able to fetch or clone from the server.

As `git rev-list` also supports a `--missing=<missing-action>` option,
the first 3 patches in this series are about refactoring related code
from both `git rev-list` and `git pack-objects` into new
"missing.{c,h}" files. Patch 4/4 then adds a new
`uploadpack.missingAction` configuration variable.

Christian Couder (4):
  rev-list: refactor --missing=<missing-action>
  missing: support rejecting --missing=print
  pack-objects: use the missing action API
  upload-pack: allow configuring a missing-action

 Documentation/config/uploadpack.txt |   9 ++
 Makefile                            |   1 +
 builtin/pack-objects.c              |  46 +++++-----
 builtin/rev-list.c                  |  41 ++-------
 missing.c                           |  42 ++++++++++
 missing.h                           |  20 +++++
 t/t5706-upload-pack-missing.sh      | 125 ++++++++++++++++++++++++++++
 upload-pack.c                       |  19 +++++
 8 files changed, 244 insertions(+), 59 deletions(-)
 create mode 100644 missing.c
 create mode 100644 missing.h
 create mode 100755 t/t5706-upload-pack-missing.sh

-- 
2.44.0.655.g111bceeb19

[PATCH 1/4] rev-list: refactor --missing=<missing-action>

[Christian Couder]

Both `git rev-list` and `git pack-objects` support a
`--missing=<missing-action>` feature, but they currently don't share
any code for that.

In a following commit, we are going to add support for a similar
'missing-action' feature to another command. To avoid duplicating
similar code, let's start refactoring the rev-list code for this
feature into new "missing.{c,h}" files.

For now, this refactoring is about moving code from
"builtin/rev-list.c" into new "missing.{c,h}" files. But in a
following commit, that refactored code will be used in
`git pack-objects` too.

As `enum missing_action` and parse_missing_action_value() are moved to
"missing.{c,h}", we need to modify the later a bit, so that it stops
updating a global variable, but instead returns either -1 on error or
the parsed value otherwise.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Makefile           |  1 +
 builtin/rev-list.c | 41 ++++++-----------------------------------
 missing.c          | 26 ++++++++++++++++++++++++++
 missing.h          | 18 ++++++++++++++++++
 4 files changed, 51 insertions(+), 35 deletions(-)
 create mode 100644 missing.c
 create mode 100644 missing.h

diff --git a/Makefile b/Makefile
index 1e31acc72e..75583a71a0 100644
--- a/Makefile
+++ b/Makefile
@@ -1076,6 +1076,7 @@ LIB_OBJS += merge-recursive.o
 LIB_OBJS += merge.o
 LIB_OBJS += midx.o
 LIB_OBJS += midx-write.o
+LIB_OBJS += missing.o
 LIB_OBJS += name-hash.o
 LIB_OBJS += negotiator/default.o
 LIB_OBJS += negotiator/noop.o
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 77803727e0..f71cc5ebe1 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -20,6 +20,7 @@
 #include "reflog-walk.h"
 #include "oidset.h"
 #include "packfile.h"
+#include "missing.h"
 
 static const char rev_list_usage[] =
 "git rev-list [<options>] <commit>... [--] [<path>...]\n"
@@ -71,12 +72,6 @@ static struct oidset omitted_objects;
 static int arg_print_omitted; /* print objects omitted by filter */
 
 static struct oidset missing_objects;
-enum missing_action {
-	MA_ERROR = 0,    /* fail if any missing objects are encountered */
-	MA_ALLOW_ANY,    /* silently allow ALL missing objects */
-	MA_PRINT,        /* print ALL missing objects in special section */
-	MA_ALLOW_PROMISOR, /* silently allow all missing PROMISOR objects */
-};
 static enum missing_action arg_missing_action;
 
 /* display only the oid of each object encountered */
@@ -392,34 +387,6 @@ static void print_disk_usage(off_t size)
 	strbuf_release(&sb);
 }
 
-static inline int parse_missing_action_value(const char *value)
-{
-	if (!strcmp(value, "error")) {
-		arg_missing_action = MA_ERROR;
-		return 1;
-	}
-
-	if (!strcmp(value, "allow-any")) {
-		arg_missing_action = MA_ALLOW_ANY;
-		fetch_if_missing = 0;
-		return 1;
-	}
-
-	if (!strcmp(value, "print")) {
-		arg_missing_action = MA_PRINT;
-		fetch_if_missing = 0;
-		return 1;
-	}
-
-	if (!strcmp(value, "allow-promisor")) {
-		arg_missing_action = MA_ALLOW_PROMISOR;
-		fetch_if_missing = 0;
-		return 1;
-	}
-
-	return 0;
-}
-
 static int try_bitmap_count(struct rev_info *revs,
 			    int filter_provided_objects)
 {
@@ -569,10 +536,14 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		if (skip_prefix(arg, "--missing=", &arg)) {
+			int res;
 			if (revs.exclude_promisor_objects)
 				die(_("options '%s' and '%s' cannot be used together"), "--exclude-promisor-objects", "--missing");
-			if (parse_missing_action_value(arg))
+			res = parse_missing_action_value(arg);
+			if (res >= 0) {
+				arg_missing_action = res;
 				break;
+			}
 		}
 	}
 
diff --git a/missing.c b/missing.c
new file mode 100644
index 0000000000..83e0c5e584
--- /dev/null
+++ b/missing.c
@@ -0,0 +1,26 @@
+#include "git-compat-util.h"
+#include "missing.h"
+#include "object-file.h"
+
+int parse_missing_action_value(const char *value)
+{
+	if (!strcmp(value, "error"))
+		return MA_ERROR;
+
+	if (!strcmp(value, "allow-any")) {
+		fetch_if_missing = 0;
+		return MA_ALLOW_ANY;
+	}
+
+	if (!strcmp(value, "print")) {
+		fetch_if_missing = 0;
+		return MA_PRINT;
+	}
+
+	if (!strcmp(value, "allow-promisor")) {
+		fetch_if_missing = 0;
+		return MA_ALLOW_PROMISOR;
+	}
+
+	return -1;
+}
diff --git a/missing.h b/missing.h
new file mode 100644
index 0000000000..c8261dfe55
--- /dev/null
+++ b/missing.h
@@ -0,0 +1,18 @@
+#ifndef MISSING_H
+#define MISSING_H
+
+enum missing_action {
+	MA_ERROR = 0,    /* fail if any missing objects are encountered */
+	MA_ALLOW_ANY,    /* silently allow ALL missing objects */
+	MA_PRINT,        /* print ALL missing objects in special section */
+	MA_ALLOW_PROMISOR, /* silently allow all missing PROMISOR objects */
+};
+
+/*
+  Return an `enum missing_action` in case parsing is successful or -1
+  if parsing failed. Also sets the fetch_if_missing global variable
+  from "object-file.h".
+ */
+int parse_missing_action_value(const char *value);
+
+#endif /* MISSING_H */
-- 
2.44.0.655.g111bceeb19

[PATCH 2/4] missing: support rejecting --missing=print

[Christian Couder]

`git pack-objects` supports the `--missing=<missing-action>` option in
the same way as `git rev-list` except when '<missing-action>' is
"print", which `git pack-objects` doesn't support.

As we want to refactor `git pack-objects` to use the same code from
"missing.{c,h}" as `git rev-list` for the `--missing=...` feature, let's
make it possible for that code to reject `--missing=print`.

`git pack-objects` will then use that code in a following commit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/rev-list.c | 2 +-
 missing.c          | 4 ++--
 missing.h          | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index f71cc5ebe1..a712a6fd62 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -539,7 +539,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			int res;
 			if (revs.exclude_promisor_objects)
 				die(_("options '%s' and '%s' cannot be used together"), "--exclude-promisor-objects", "--missing");
-			res = parse_missing_action_value(arg);
+			res = parse_missing_action_value(arg, 1);
 			if (res >= 0) {
 				arg_missing_action = res;
 				break;
diff --git a/missing.c b/missing.c
index 83e0c5e584..d306dea2d9 100644
--- a/missing.c
+++ b/missing.c
@@ -2,7 +2,7 @@
 #include "missing.h"
 #include "object-file.h"
 
-int parse_missing_action_value(const char *value)
+int parse_missing_action_value(const char *value, int print_ok)
 {
 	if (!strcmp(value, "error"))
 		return MA_ERROR;
@@ -12,7 +12,7 @@ int parse_missing_action_value(const char *value)
 		return MA_ALLOW_ANY;
 	}
 
-	if (!strcmp(value, "print")) {
+	if (!strcmp(value, "print") && print_ok) {
 		fetch_if_missing = 0;
 		return MA_PRINT;
 	}
diff --git a/missing.h b/missing.h
index c8261dfe55..77906691e7 100644
--- a/missing.h
+++ b/missing.h
@@ -13,6 +13,6 @@ enum missing_action {
   if parsing failed. Also sets the fetch_if_missing global variable
   from "object-file.h".
  */
-int parse_missing_action_value(const char *value);
+int parse_missing_action_value(const char *value, int print_ok);
 
 #endif /* MISSING_H */
-- 
2.44.0.655.g111bceeb19

[PATCH 3/4] pack-objects: use the missing action API

[Christian Couder]

Both `git rev-list` and `git pack-objects` support a
`--missing=<missing-action>` option. Previous commits created an API
in "missing.{c,h}" to help supporting that option, but only
`git rev-list` has been using that API so far.

Let's make `git pack-objects` use it too.

This involves creating a new show_object_fn_from_action() function to
set the `fn_show_object` variable independently from parsing the
missing action, which is now performed by the
parse_missing_action_value() API function.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/pack-objects.c | 46 ++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index baf0090fc8..bce586b9a6 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -39,6 +39,7 @@
 #include "promisor-remote.h"
 #include "pack-mtimes.h"
 #include "parse-options.h"
+#include "missing.h"
 
 /*
  * Objects we are going to pack are collected in the `to_pack` structure.
@@ -250,11 +251,6 @@ static unsigned long window_memory_limit = 0;
 
 static struct string_list uri_protocols = STRING_LIST_INIT_NODUP;
 
-enum missing_action {
-	MA_ERROR = 0,      /* fail if any missing objects are encountered */
-	MA_ALLOW_ANY,      /* silently allow ALL missing objects */
-	MA_ALLOW_PROMISOR, /* silently allow all missing PROMISOR objects */
-};
 static enum missing_action arg_missing_action;
 static show_object_fn fn_show_object;
 
@@ -3826,33 +3822,35 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,
 	show_object(obj, name, data);
 }
 
+static show_object_fn show_object_fn_from_action(enum missing_action action)
+{
+	switch (action) {
+	case MA_ERROR:
+		return show_object;
+	case MA_ALLOW_ANY:
+		return show_object__ma_allow_any;
+	case MA_ALLOW_PROMISOR:
+		return show_object__ma_allow_promisor;
+	default:
+		BUG("invalid missing action %d", action);
+	}
+}
+
 static int option_parse_missing_action(const struct option *opt UNUSED,
 				       const char *arg, int unset)
 {
+	int res;
+
 	assert(arg);
 	assert(!unset);
 
-	if (!strcmp(arg, "error")) {
-		arg_missing_action = MA_ERROR;
-		fn_show_object = show_object;
-		return 0;
-	}
-
-	if (!strcmp(arg, "allow-any")) {
-		arg_missing_action = MA_ALLOW_ANY;
-		fetch_if_missing = 0;
-		fn_show_object = show_object__ma_allow_any;
-		return 0;
-	}
+	res = parse_missing_action_value(arg, 0);
+	if (res < 0)
+		die(_("invalid value for '%s': '%s'"), "--missing", arg);
 
-	if (!strcmp(arg, "allow-promisor")) {
-		arg_missing_action = MA_ALLOW_PROMISOR;
-		fetch_if_missing = 0;
-		fn_show_object = show_object__ma_allow_promisor;
-		return 0;
-	}
+	arg_missing_action = res;
+	fn_show_object = show_object_fn_from_action(arg_missing_action);
 
-	die(_("invalid value for '%s': '%s'"), "--missing", arg);
 	return 0;
 }
 
-- 
2.44.0.655.g111bceeb19

[PATCH 4/4] upload-pack: allow configuring a missing-action

[Christian Couder]

From: Christian Couder <chriscool@tuxfamily.org>

In case some objects are missing from a server, it might still be
useful to be able to fetch or clone from it if the client already has
the missing objects or can get them in some way.

For example, in case both the server and the client are using a
separate promisor remote that contain some objects, it can be better
if the server doesn't try to send such objects back to the client, but
instead let the client get those objects separately from the promisor
remote. (The client needs to have the separate promisor remote
configured, for that to work.)

Another example could be a server where some objects have been
corrupted or deleted. It could still be useful for clients who could
get those objects from another source, like perhaps a different
client, to be able to fetch or clone from the server.

To configure what a server should do if there are such missing
objects, let's teach `git upload-pack` a new
`uploadpack.missingAction` configuration variable.

This missing-action works in a similar way as what `git rev-list` and
`git pack-objects` already support with their
`--missing=<missing-action>` command line options. In fact when
`uploadpack.missingAction` is set to something different than "error",
`git upload-pack` will just pass the corresponding
`--missing=<missing-action>` to `git pack-objects`.

So the `uploadpack.missingAction` has the same limitations as
`git pack-objects --missing=<missing-action>`. Especially when not
using promisor remotes, 'allow-any' works only for blobs.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/uploadpack.txt |   9 ++
 missing.c                           |  16 ++++
 missing.h                           |   2 +
 t/t5706-upload-pack-missing.sh      | 125 ++++++++++++++++++++++++++++
 upload-pack.c                       |  19 +++++
 5 files changed, 171 insertions(+)
 create mode 100755 t/t5706-upload-pack-missing.sh

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index 16264d82a7..cd70e853b3 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -82,3 +82,12 @@ uploadpack.allowRefInWant::
 	is intended for the benefit of load-balanced servers which may
 	not have the same view of what OIDs their refs point to due to
 	replication delay.
+
+uploadpack.missingAction::
+	If this option is set, `upload-pack` will call
+	linkgit:git-pack-objects[1] passing it the corresponding
+	`--missing=<missing-action>` command line option. See the
+	documentation for that option, to see the valid values and
+	their meaning. This could be useful if some objects are
+	missing on a server, but a client already has them or could
+	still get them from somewhere else.
diff --git a/missing.c b/missing.c
index d306dea2d9..022ebbe4be 100644
--- a/missing.c
+++ b/missing.c
@@ -24,3 +24,19 @@ int parse_missing_action_value(const char *value, int print_ok)
 
 	return -1;
 }
+
+const char *missing_action_to_string(enum missing_action action)
+{
+	switch (action) {
+	case MA_ERROR:
+		return "error";
+	case MA_ALLOW_ANY:
+		return "allow-any";
+	case MA_PRINT:
+		return "print";
+	case MA_ALLOW_PROMISOR:
+		return "allow-promisor";
+	default:
+		BUG("invalid missing action %d", action);
+	}
+}
diff --git a/missing.h b/missing.h
index 77906691e7..ca574f83a0 100644
--- a/missing.h
+++ b/missing.h
@@ -15,4 +15,6 @@ enum missing_action {
  */
 int parse_missing_action_value(const char *value, int print_ok);
 
+const char *missing_action_to_string(enum missing_action action);
+
 #endif /* MISSING_H */
diff --git a/t/t5706-upload-pack-missing.sh b/t/t5706-upload-pack-missing.sh
new file mode 100755
index 0000000000..21bb6aed2b
--- /dev/null
+++ b/t/t5706-upload-pack-missing.sh
@@ -0,0 +1,125 @@
+#!/bin/sh
+
+test_description='handling of missing objects in upload-pack'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+# Setup the repository with three commits, this way HEAD is always
+# available and we can hide commit 1 or 2.
+test_expect_success 'setup: create "template" repository' '
+	git init template &&
+	test_commit -C template 1 &&
+	test_commit -C template 2 &&
+	test_commit -C template 3 &&
+	test-tool genrandom foo 10240 >template/foo &&
+	git -C template add foo &&
+	git -C template commit -m foo
+'
+
+# A bare repo will act as a server repo with unpacked objects.
+test_expect_success 'setup: create bare "server" repository' '
+	git clone --bare --no-local template server &&
+	mv server/objects/pack/pack-* . &&
+	packfile=$(ls pack-*.pack) &&
+	git -C server unpack-objects --strict <"$packfile"
+'
+
+# Fetching with 'uploadpack.missingAction=allow-any' only works with
+# blobs, as `git pack-objects --missing=allow-any` fails if a missing
+# object is not a blob.
+test_expect_success "fetch with uploadpack.missingAction=allow-any" '
+	oid="$(git -C server rev-parse HEAD:1.t)" &&
+	oid_path="$(test_oid_to_path $oid)" &&
+	path="server/objects/$oid_path" &&
+
+	mv "$path" "$path.hidden" &&
+	test_when_finished "mv $path.hidden $path" &&
+
+	git init client &&
+	test_when_finished "rm -rf client" &&
+
+	# Client needs the missing objects to be available somehow
+	client_path="client/.git/objects/$oid_path" &&
+	mkdir -p $(dirname "$client_path") &&
+	cp "$path.hidden" "$client_path" &&
+
+	test_must_fail git -C client fetch ../server &&
+	git -C server config uploadpack.missingAction error &&
+	test_must_fail git -C client fetch ../server &&
+	git -C server config uploadpack.missingAction allow-any &&
+	git -C client fetch ../server &&
+	git -C server config --unset uploadpack.missingAction
+'
+
+check_missing_objects () {
+	git -C "$1" rev-list --objects --all --missing=print > all.txt &&
+	sed -n "s/^\?\(.*\)/\1/p" <all.txt >missing.txt &&
+	test_line_count = "$2" missing.txt &&
+	test "$3" = "$(cat missing.txt)"
+}
+
+test_expect_success "setup for testing uploadpack.missingAction=allow-promisor" '
+	# Create another bare repo called "server2"
+	git init --bare server2 &&
+
+	# Copy the largest object from server to server2
+	obj="HEAD:foo" &&
+	oid="$(git -C server rev-parse $obj)" &&
+	oid_path="$(test_oid_to_path $oid)" &&
+	path="server/objects/$oid_path" &&
+	path2="server2/objects/$oid_path" &&
+	mkdir -p $(dirname "$path2") &&
+	cp "$path" "$path2" &&
+
+	# Repack everything first
+	git -C server -c repack.writebitmaps=false repack -a -d &&
+
+	# Repack without the largest object and create a promisor pack on server
+	git -C server -c repack.writebitmaps=false repack -a -d \
+	    --filter=blob:limit=5k --filter-to="$(pwd)" &&
+	promisor_file=$(ls server/objects/pack/*.pack | sed "s/\.pack/.promisor/") &&
+	> "$promisor_file" &&
+
+	# Check that only one object is missing on the server
+	check_missing_objects server 1 "$oid" &&
+
+	# Configure server2 as promisor remote for server
+	git -C server remote add server2 "file://$(pwd)/server2" &&
+	git -C server config remote.server2.promisor true &&
+
+	git -C server2 config uploadpack.allowFilter true &&
+	git -C server2 config uploadpack.allowAnySHA1InWant true &&
+	git -C server config uploadpack.allowFilter true &&
+	git -C server config uploadpack.allowAnySHA1InWant true
+'
+
+test_expect_success "fetch with uploadpack.missingAction=allow-promisor" '
+	git -C server config uploadpack.missingAction allow-promisor &&
+
+	# Clone from server to create a client
+	git clone -c remote.server2.promisor=true \
+		-c remote.server2.fetch="+refs/heads/*:refs/remotes/server2/*" \
+		-c remote.server2.url="file://$(pwd)/server2" \
+		--no-local --filter="blob:limit=5k" server client &&
+	test_when_finished "rm -rf client" &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
+test_expect_success "fetch without uploadpack.missingAction=allow-promisor" '
+	git -C server config --unset uploadpack.missingAction &&
+
+	# Clone from server to create a client
+	git clone -c remote.server2.promisor=true \
+		-c remote.server2.fetch="+refs/heads/*:refs/remotes/server2/*" \
+		-c remote.server2.url="file://$(pwd)/server2" \
+		--no-local --filter="blob:limit=5k" server client &&
+	test_when_finished "rm -rf client" &&
+
+	# Check that the largest object is not missing on the server anymore
+	check_missing_objects server 0 ""
+'
+
+test_done
diff --git a/upload-pack.c b/upload-pack.c
index 902144b9d3..c355ebe1e5 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -29,6 +29,8 @@
 #include "write-or-die.h"
 #include "json-writer.h"
 #include "strmap.h"
+#include "missing.h"
+#include "object-file.h"
 
 /* Remember to update object flag allocation in object.h */
 #define THEY_HAVE	(1u << 11)
@@ -96,6 +98,8 @@ struct upload_pack_data {
 
 	const char *pack_objects_hook;
 
+	enum missing_action missing_action;
+
 	unsigned stateless_rpc : 1;				/* v0 only */
 	unsigned no_done : 1;					/* v0 only */
 	unsigned daemon_mode : 1;				/* v0 only */
@@ -315,6 +319,9 @@ static void create_pack_file(struct upload_pack_data *pack_data,
 		strvec_push(&pack_objects.args, "--delta-base-offset");
 	if (pack_data->use_include_tag)
 		strvec_push(&pack_objects.args, "--include-tag");
+	if (pack_data->missing_action)
+		strvec_pushf(&pack_objects.args, "--missing=%s",
+			     missing_action_to_string(pack_data->missing_action));
 	if (pack_data->filter_options.choice) {
 		const char *spec =
 			expand_list_objects_filter_spec(&pack_data->filter_options);
@@ -1371,6 +1378,18 @@ static int upload_pack_config(const char *var, const char *value,
 		precomposed_unicode = git_config_bool(var, value);
 	} else if (!strcmp("transfer.advertisesid", var)) {
 		data->advertise_sid = git_config_bool(var, value);
+	} else if (!strcmp("uploadpack.missingaction", var)) {
+		int res = parse_missing_action_value(value, 0);
+		if (res < 0)
+			die(_("invalid value for '%s': '%s'"), var, value);
+		/*
+		 * parse_missing_action_value() unsets fetch_if_missing
+		 * but if we allow promisor we want to still fetch from
+		 * the promisor remote
+		 */
+		if (res == MA_ALLOW_PROMISOR)
+			fetch_if_missing =1;
+		data->missing_action = res;
 	}
 
 	if (parse_object_filter_config(var, value, ctx->kvi, data) < 0)
-- 
2.44.0.655.g111bceeb19

Re: [PATCH 0/4] upload-pack: support a missing-action

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> `git pack-objects` already supports a `--missing=<missing-action>`
> option, so that it can avoid erroring out if some objects aren't
> available.
>
> It is interesting to have `git upload-pack` support a similar way to
> avoid sending some objects in case they aren't available on the
> server.

Is it interesting?  In what way?

> For example, in case both the server and the client are using a
> separate promisor remote that contain some objects, it can be better
> if the server doesn't try to send such objects back to the client, but
> instead let the client get those objects separately from the promisor
> remote. (The client needs to have the separate promisor remote
> configured, for that to work.)

It is unclear what the precondition for such an arrangement to work
reliably, and a lot more importantly, how we can validate that the
precondition holds when "fetch" talks to "upload-pack".  If you get
it wrong, you'd have a server that would corrupt repositories that
fetch from it.

That is where my "Is it really interesting?  I do not find your
explanation convincing yet." above primarily comes from.


Presumably "fetch" could tell "upload-pack" something like:

	I know how to fetch missing objects from this and that
	promisor remotes, so if you choose to, you may omit objects
	that you know are available from these promisor remotes when
	sending objects to me.

using a new capability, and we can allow upload-pack to omit objects
only when such a new capability tells it to?

Re: [PATCH 1/4] rev-list: refactor --missing=<missing-action>

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Both `git rev-list` and `git pack-objects` support a
> `--missing=<missing-action>` feature, but they currently don't share
> any code for that.

So "git pack-objects" will still stay unaware of <missing.h> after
this step, which was a bit unexpected.

> For now, this refactoring is about moving code from
> "builtin/rev-list.c" into new "missing.{c,h}" files. But in a
> following commit, that refactored code will be used in
> `git pack-objects` too.

I think it is easier to grok if you said this in the second
paragraph, before mentioning "another command".  The first paragraph
talks about rev-list and pack-objects logically but not phisically
sharing the feature, so with "For now, this refactoring is about
moving ... into" -> "Refactor the support for --missing in rev-list
into", it is enough to explain how this change is a first step to
make things better, without bringing the third thing into picture.

IOW ...

> In a following commit, we are going to add support for a similar
> 'missing-action' feature to another command. To avoid duplicating
> similar code, let's start refactoring the rev-list code for this
> feature into new "missing.{c,h}" files.

... this paragraph should have much lower weight in explaining this
commit.

> As `enum missing_action` and parse_missing_action_value() are moved to
> "missing.{c,h}", we need to modify the later a bit, so that it stops

"later" -> "latter"?

> updating a global variable, but instead returns either -1 on error or
> the parsed value otherwise.

OK.  As a shared helper function, I agree that assignment to a global
should be outside of its responsibility.  Which means that the caller
is now responsible for making that assignment.

>  static int try_bitmap_count(struct rev_info *revs,
>  			    int filter_provided_objects)
>  {
> @@ -569,10 +536,14 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  	for (i = 1; i < argc; i++) {
>  		const char *arg = argv[i];
>  		if (skip_prefix(arg, "--missing=", &arg)) {
> +			int res;
>  			if (revs.exclude_promisor_objects)
>  				die(_("options '%s' and '%s' cannot be used together"), "--exclude-promisor-objects", "--missing");
> -			if (parse_missing_action_value(arg))
> +			res = parse_missing_action_value(arg);
> +			if (res >= 0) {
> +				arg_missing_action = res;
>  				break;
> +			}

... which seems to be missing from here.

>  		}
>  	}
>  
> diff --git a/missing.c b/missing.c
> new file mode 100644
> index 0000000000..83e0c5e584
> --- /dev/null
> +++ b/missing.c
> @@ -0,0 +1,26 @@
> +#include "git-compat-util.h"
> +#include "missing.h"
> +#include "object-file.h"
> +
> +int parse_missing_action_value(const char *value)
> +{
> +	if (!strcmp(value, "error"))
> +		return MA_ERROR;
> +
> +	if (!strcmp(value, "allow-any")) {
> +		fetch_if_missing = 0;
> +		return MA_ALLOW_ANY;
> +	}
> +
> +	if (!strcmp(value, "print")) {
> +		fetch_if_missing = 0;
> +		return MA_PRINT;
> +	}
> +
> +	if (!strcmp(value, "allow-promisor")) {
> +		fetch_if_missing = 0;
> +		return MA_ALLOW_PROMISOR;
> +	}

... and this one still touches the global.

> +	return -1;
> +}
> diff --git a/missing.h b/missing.h
> new file mode 100644
> index 0000000000..c8261dfe55
> --- /dev/null
> +++ b/missing.h
> @@ -0,0 +1,18 @@
> +#ifndef MISSING_H
> +#define MISSING_H
> +
> +enum missing_action {
> +	MA_ERROR = 0,    /* fail if any missing objects are encountered */
> +	MA_ALLOW_ANY,    /* silently allow ALL missing objects */
> +	MA_PRINT,        /* print ALL missing objects in special section */
> +	MA_ALLOW_PROMISOR, /* silently allow all missing PROMISOR objects */
> +};
> +
> +/*
> +  Return an `enum missing_action` in case parsing is successful or -1
> +  if parsing failed. Also sets the fetch_if_missing global variable
> +  from "object-file.h".
> + */

... and this also mentions the global.

> +int parse_missing_action_value(const char *value);
> +
> +#endif /* MISSING_H */

Re: [PATCH 2/4] missing: support rejecting --missing=print

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> `git pack-objects` supports the `--missing=<missing-action>` option in
> the same way as `git rev-list` except when '<missing-action>' is
> "print", which `git pack-objects` doesn't support.
>
> As we want to refactor `git pack-objects` to use the same code from
> "missing.{c,h}" as `git rev-list` for the `--missing=...` feature, let's
> make it possible for that code to reject `--missing=print`.
>
> `git pack-objects` will then use that code in a following commit.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/rev-list.c | 2 +-
>  missing.c          | 4 ++--
>  missing.h          | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index f71cc5ebe1..a712a6fd62 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -539,7 +539,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  			int res;
>  			if (revs.exclude_promisor_objects)
>  				die(_("options '%s' and '%s' cannot be used together"), "--exclude-promisor-objects", "--missing");
> -			res = parse_missing_action_value(arg);
> +			res = parse_missing_action_value(arg, 1);

Hmph, this smells like a horribly unscalable design, as we make the
vocabulary of missing-action richer, you'd end up piling on "this
choice is allowed in this call" parameters, wouldn't you?  The first
person who adds such an ad-hoc parameter would say "hey, what's just
one extra parameter print_ok between friends", but the next person
would say the same thing for their new choice and adds frotz_ok, and
we'd be in chaos.

Rather, shouldn't the _caller_ decide if the parsed value is
something it does not like and barf?

Alternatively, add a _single_ "reject" bitset and do something like

	int parse_missing_action_value(const char *value, unsigned reject)
	{
		...
		if (!(reject & (1<<MA_ERROR) && !strcmp(value, "error")))
			return MA_ERROR;
		if (!(reject & (1<<MA_PRINT) && !strcmp(value, "print")))
			return MA_PRINT;
		...

which would scale better (but still my preference is to have the
caller deal with only the values it recognises---do not make the
caller say "if (res >= 0 && res != MA_PRINT)" as that will not scale
when new choices that are accepted elsewhere are added.