[v3,5/5] upload-pack: make uploadpack.packObjectsHook protected

Commit Message

Glen Choo May 27, 2022, 9:09 p.m. UTC

From: Glen Choo <[e-mail]>

Now that protected config includes "-c", "uploadpack.packObjectsHook"
behaves identically to a 'Protected config only' variable. Refactor it
to use git_protected_config() and mark it 'Protected config only'.

Signed-off-by: Glen Choo <[e-mail]>
---
 Documentation/config/uploadpack.txt | 22 +++++++++-------------
 upload-pack.c                       | 17 +++++++++++------
 2 files changed, 20 insertions(+), 19 deletions(-)

Comments

Derrick Stolee June 2, 2022, 1:18 p.m. UTC | #1

On 5/27/2022 5:09 PM, Glen Choo via GitGitGadget wrote:
> From: Glen Choo <[e-mail]>
> 
> Now that protected config includes "-c", "uploadpack.packObjectsHook"
> behaves identically to a 'Protected config only' variable. Refactor it
> to use git_protected_config() and mark it 'Protected config only'.

I'm really glad to see this simplification at the end of your series.

> @@ -1321,18 +1321,21 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)
>  		data->advertise_sid = git_config_bool(var, value);
>  	}
>  
> -	if (current_config_scope() != CONFIG_SCOPE_LOCAL &&
> -	    current_config_scope() != CONFIG_SCOPE_WORKTREE) {
> -		if (!strcmp("uploadpack.packobjectshook", var))
> -			return git_config_string(&data->pack_objects_hook, var, value);
> -	}
> -

...

> +static int upload_pack_protected_config(const char *var, const char *value, void *cb_data)
> +{
> +	struct upload_pack_data *data = cb_data;
> +
> +	if (!strcmp("uploadpack.packobjectshook", var))
> +		return git_config_string(&data->pack_objects_hook, var, value);
> +	return 0;
> +}
> +

This is much cleaner.

> @@ -1342,6 +1345,7 @@ void upload_pack(const int advertise_refs, const int stateless_rpc,
>  	upload_pack_data_init(&data);
>  
>  	git_config(upload_pack_config, &data);
> +	git_protected_config(upload_pack_protected_config, &data);
>  
>  	data.stateless_rpc = stateless_rpc;
>  	data.timeout = timeout;
> @@ -1697,6 +1701,7 @@ int upload_pack_v2(struct repository *r, struct packet_reader *request)
>  	data.use_sideband = LARGE_PACKET_MAX;
>  
>  	git_config(upload_pack_config, &data);
> +	git_protected_config(upload_pack_protected_config, &data);

It's unfortunate that there are two places that need this change.
Is it worth adding a static helper that executes these?

static void get_upload_pack_config(void *data)
{
	git_config(upload_pack_config, data);
	git_protected_config(upload_pack_protected_config, data);
}

Thanks,
-Stolee

Patch

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index 32fad5bbe81..57e5e021323 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -39,19 +39,15 @@  uploadpack.keepAlive::
 	disables keepalive packets entirely. The default is 5 seconds.
 
 uploadpack.packObjectsHook::
-	If this option is set, when `upload-pack` would run
-	`git pack-objects` to create a packfile for a client, it will
-	run this shell command instead.  The `pack-objects` command and
-	arguments it _would_ have run (including the `git pack-objects`
-	at the beginning) are appended to the shell command. The stdin
-	and stdout of the hook are treated as if `pack-objects` itself
-	was run. I.e., `upload-pack` will feed input intended for
-	`pack-objects` to the hook, and expects a completed packfile on
-	stdout.
-+
-Note that this configuration variable is ignored if it is seen in the
-repository-level config (this is a safety measure against fetching from
-untrusted repositories).
+	'(Protected config only)' If this option is set, when
+	`upload-pack` would run `git pack-objects` to create a packfile
+	for a client, it will run this shell command instead. The
+	`pack-objects` command and arguments it _would_ have run
+	(including the `git pack-objects` at the beginning) are appended
+	to the shell command. The stdin and stdout of the hook are
+	treated as if `pack-objects` itself was run. I.e., `upload-pack`
+	will feed input intended for `pack-objects` to the hook, and
+	expects a completed packfile on stdout.
 
 uploadpack.allowFilter::
 	If this option is set, `upload-pack` will support partial
diff --git a/upload-pack.c b/upload-pack.c
index 3a851b36066..2a39391369d 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1321,18 +1321,21 @@  static int upload_pack_config(const char *var, const char *value, void *cb_data)
 		data->advertise_sid = git_config_bool(var, value);
 	}
 
-	if (current_config_scope() != CONFIG_SCOPE_LOCAL &&
-	    current_config_scope() != CONFIG_SCOPE_WORKTREE) {
-		if (!strcmp("uploadpack.packobjectshook", var))
-			return git_config_string(&data->pack_objects_hook, var, value);
-	}
-
 	if (parse_object_filter_config(var, value, data) < 0)
 		return -1;
 
 	return parse_hide_refs_config(var, value, "uploadpack");
 }
 
+static int upload_pack_protected_config(const char *var, const char *value, void *cb_data)
+{
+	struct upload_pack_data *data = cb_data;
+
+	if (!strcmp("uploadpack.packobjectshook", var))
+		return git_config_string(&data->pack_objects_hook, var, value);
+	return 0;
+}
+
 void upload_pack(const int advertise_refs, const int stateless_rpc,
 		 const int timeout)
 {
@@ -1342,6 +1345,7 @@  void upload_pack(const int advertise_refs, const int stateless_rpc,
 	upload_pack_data_init(&data);
 
 	git_config(upload_pack_config, &data);
+	git_protected_config(upload_pack_protected_config, &data);
 
 	data.stateless_rpc = stateless_rpc;
 	data.timeout = timeout;
@@ -1697,6 +1701,7 @@  int upload_pack_v2(struct repository *r, struct packet_reader *request)
 	data.use_sideband = LARGE_PACKET_MAX;
 
 	git_config(upload_pack_config, &data);
+	git_protected_config(upload_pack_protected_config, &data);
 
 	while (state != FETCH_DONE) {
 		switch (state) {