threads / patch / 48694

patchbuiltin/send-pack: populate the default configs

Subject: [PATCH] builtin/send-pack: populate the default configs

## tl;dr

8 messages between Jun 12, 2018 and Jun 12, 2018. Diffs are folded; open one to read it.

replies: 7people: 5as markdown or json

Masaya Suzuki· Jun 12, 2018, 17:26 UTC · lore

builtin/send-pack didn't call git_default_config, and because of this git push --signed didn't respect the username and email in gitconfig in the HTTP transport.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 builtin/send-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/send-pack.c +1 −1
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 4923b1058..42fd8d1a3 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -121,7 +121,7 @@ static int send_pack_config(const char *k, const char *v, void *cb)
 			}
 		}
 	}
-	return 0;
+	return git_default_config(k, v, cb);
 }
 
 int cmd_send_pack(int argc, const char **argv, const char *prefix)

-- 
2.18.0.rc1.242.g61856ae69a-goog
Junio C Hamano· Jun 12, 2018, 17:38 UTC · re: Masaya Suzuki · lore

Re: [PATCH] builtin/send-pack: populate the default configs

Masaya Suzuki <masayasuzuki@google.com> writes:
Show 8 quoted lines
> builtin/send-pack didn't call git_default_config, and because of this
> git push --signed didn't respect the username and email in gitconfig in
> the HTTP transport.
>
> Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
> ---
>  builtin/send-pack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Makes sense. I wonder if this was deliberate (i.e. we had some reason not wanting to be affected by some _other_ configuration items and refraining from doing the default-config was the way to implement it), but I do not think of a reason offhand.

Will queue.  Thanks.
Show 14 quoted lines
>
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 4923b1058..42fd8d1a3 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -121,7 +121,7 @@ static int send_pack_config(const char *k, const char *v, void *cb)
>  			}
>  		}
>  	}
> -	return 0;
> +	return git_default_config(k, v, cb);
>  }
>  
>  int cmd_send_pack(int argc, const char **argv, const char *prefix)
Stefan Beller· Jun 12, 2018, 17:53 UTC · re: Junio C Hamano · lore

[PATCH] builtin/send-pack: report gpg config errors

Any caller except of git_gpg_config() except the one in send_pack_config() handles the return value of git_gpg_config(). Also handle the return value there.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
> Makes sense.  I wonder if this was deliberate

There was no need for other configuration to be loaded and signing pushes is the first that needs repository configuration.

Looking at 68c757f2199 (push: add a config option push.gpgSign for default signed pushes, 2015-08-19) further, maybe we want to also have this?

Thanks, Stefan

 builtin/send-pack.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to builtin/send-pack.c +2 −1
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 4923b1058c6..b54a0cae878 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -101,7 +101,8 @@ static void print_helper_status(struct ref *ref)
 
 static int send_pack_config(const char *k, const char *v, void *cb)
 {
-	git_gpg_config(k, v, NULL);
+	if (git_gpg_config(k, v, NULL) < 0)
+		return -1;
 
 	if (!strcmp(k, "push.gpgsign")) {
 		const char *value;

-- 
2.18.0.rc1.244.gcf134e6275-goog
Eric Sunshine· Jun 12, 2018, 17:55 UTC · re: Stefan Beller · lore

Re: [PATCH] builtin/send-pack: report gpg config errors

On Tue, Jun 12, 2018 at 1:53 PM, Stefan Beller <sbeller@google.com> wrote:
> Any caller except of git_gpg_config() except the one in send_pack_config()
Drop the first "except"?
Also: s/Any caller/All callers/
> handles the return value of git_gpg_config(). Also handle the return value
> there.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
Jonathan Nieder· Jun 12, 2018, 18:07 UTC · re: Stefan Beller · lore

Re: [PATCH] builtin/send-pack: report gpg config errors

Hi,
Stefan Beller wrote:
Show 8 quoted lines
> Any caller except of git_gpg_config() except the one in send_pack_config()
> handles the return value of git_gpg_config(). Also handle the return value
> there.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/send-pack.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
The other callers do
	int status;
	if (!strcmp(k, "key.i.handle")) {
		...
		return ...;
	}
	return git_gpg_config(k, v, NULL);
or
	int status;
	if (!strcmp(k, "key.i.handle")) {
		...
		return ...;
	}
	status = git_gpg_config(k, v, NULL);
	if (status)
		return status;
	...

Should this do the same? That way, if config_fn_t learns to return more fine-grained errors (e.g. a new return value of -2 or 1) then it would be propagated.

Thanks, Jonathan

Stefan Beller· Jun 12, 2018, 18:19 UTC · re: Jonathan Nieder · lore

Re: [PATCH] builtin/send-pack: report gpg config errors

On Tue, Jun 12, 2018 at 11:08 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 15 quoted lines
>
> Hi,
>
> Stefan Beller wrote:
>
> > Any caller except of git_gpg_config() except the one in send_pack_config()
> > handles the return value of git_gpg_config(). Also handle the return value
> > there.
> >
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
> >  builtin/send-pack.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
>
> The other callers do
except builtin/log, which I followed. :/
Show 8 quoted lines
>
>         int status;
>
>         if (!strcmp(k, "key.i.handle")) {
>                 ...
>                 return ...;
>         }
>         return git_gpg_config(k, v, NULL);

This conflicts with Masayas patch, that has a return of the standard options.

Show 14 quoted lines
>
> or
>
>         int status;
>
>         if (!strcmp(k, "key.i.handle")) {
>                 ...
>                 return ...;
>         }
>
>         status = git_gpg_config(k, v, NULL);
>         if (status)
>                 return status;
>         ...
This looks like a good idea.
Will send a follow up patch.
Jonathan Nieder· Jun 12, 2018, 18:16 UTC · re: Masaya Suzuki · lore

Re: [PATCH] builtin/send-pack: populate the default configs

Hi,
Masaya Suzuki wrote:
Show 8 quoted lines
> builtin/send-pack didn't call git_default_config, and because of this
> git push --signed didn't respect the username and email in gitconfig in
> the HTTP transport.
>
> Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
> ---
>  builtin/send-pack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Do you have a set of commands I can run to reproduce this? Bonus points if they're in the form of a patch to t/, but commands I can manually run would be fine, too.

Show 8 quoted lines
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -121,7 +121,7 @@ static int send_pack_config(const char *k, const char *v, void *cb)
>  			}
>  		}
>  	}
> -	return 0;
> +	return git_default_config(k, v, cb);

send-pack is not a command served by a daemon so this is less potentially scary than the corresponding potential change to upload-pack or receive-pack. Some configuration this brings in:

- core.askpass: allows specifying an arbitrary command to use to
  ask for a password.  Respecting this setting should be very useful,
  even if it would be scary in a daemon.
- core.pager: allows specifying an arbitrary command to use as a
  pager, if pagination is on (but it shouldn't be on).
- core.logallrefupdates: whether to create reflogs for new refs
  (including new remote-tracking refs). Good.
- core.abbrev: what length of abbreviations to use when printing
  abbreviated object ids (good).
- core.compression, core.packedgitwindowsize, etc: pack generation
  tunables (good).
- advice.*: would allow us to make "git push" produce configurable
  advice (good!)
- etc
So it looks like a very good change.  Thanks for writing it.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Eric Sunshine· Jun 12, 2018, 18:22 UTC · re: Jonathan Nieder · lore

Re: [PATCH] builtin/send-pack: populate the default configs

On Tue, Jun 12, 2018 at 2:16 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 26 quoted lines
> Masaya Suzuki wrote:
>> builtin/send-pack didn't call git_default_config, and because of this
>> git push --signed didn't respect the username and email in gitconfig in
>> the HTTP transport.
>>
>> Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
>> ---
> send-pack is not a command served by a daemon so this is less
> potentially scary than the corresponding potential change to
> upload-pack or receive-pack.  Some configuration this brings in:
>
> - core.askpass: allows specifying an arbitrary command to use to
>   ask for a password.  Respecting this setting should be very useful,
>   even if it would be scary in a daemon.
>
> - core.pager: allows specifying an arbitrary command to use as a
>   pager, if pagination is on (but it shouldn't be on).
> - core.logallrefupdates: whether to create reflogs for new refs
>   (including new remote-tracking refs). Good.
> - core.abbrev: what length of abbreviations to use when printing
>   abbreviated object ids (good).
> - core.compression, core.packedgitwindowsize, etc: pack generation
>   tunables (good).
> - advice.*: would allow us to make "git push" produce configurable
>   advice (good!)
> - etc

This summary probably ought to be in the commit message itself (plus additional commentary implied by the ending "etc."). It's exactly the sort of information someone looking at this patch in the future will want to know to feel assured that such behavior changes were taken into account by the patch author.

← back to recent threads