* [PATCH] builtin/send-pack: populate the default configs
@ 2018-06-12 17:26 Masaya Suzuki
2018-06-12 17:38 ` Junio C Hamano
2018-06-12 18:16 ` [PATCH] builtin/send-pack: populate the default configs Jonathan Nieder
0 siblings, 2 replies; 8+ messages in thread
From: Masaya Suzuki @ 2018-06-12 17:26 UTC (permalink / raw)
To: git; +Cc: Masaya Suzuki
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(-)
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] builtin/send-pack: populate the default configs
2018-06-12 17:26 [PATCH] builtin/send-pack: populate the default configs Masaya Suzuki
@ 2018-06-12 17:38 ` Junio C Hamano
2018-06-12 17:53 ` [PATCH] builtin/send-pack: report gpg config errors Stefan Beller
2018-06-12 18:16 ` [PATCH] builtin/send-pack: populate the default configs Jonathan Nieder
1 sibling, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2018-06-12 17:38 UTC (permalink / raw)
To: Masaya Suzuki; +Cc: git
Masaya Suzuki <masayasuzuki@google.com> writes:
> 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.
>
> 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)
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] builtin/send-pack: report gpg config errors
2018-06-12 17:38 ` Junio C Hamano
@ 2018-06-12 17:53 ` Stefan Beller
2018-06-12 17:55 ` Eric Sunshine
2018-06-12 18:07 ` Jonathan Nieder
0 siblings, 2 replies; 8+ messages in thread
From: Stefan Beller @ 2018-06-12 17:53 UTC (permalink / raw)
To: gitster; +Cc: git, masayasuzuki, Stefan Beller
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(-)
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] builtin/send-pack: report gpg config errors
2018-06-12 17:53 ` [PATCH] builtin/send-pack: report gpg config errors Stefan Beller
@ 2018-06-12 17:55 ` Eric Sunshine
2018-06-12 18:07 ` Jonathan Nieder
1 sibling, 0 replies; 8+ messages in thread
From: Eric Sunshine @ 2018-06-12 17:55 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, Git List, masayasuzuki
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>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] builtin/send-pack: report gpg config errors
2018-06-12 17:53 ` [PATCH] builtin/send-pack: report gpg config errors Stefan Beller
2018-06-12 17:55 ` Eric Sunshine
@ 2018-06-12 18:07 ` Jonathan Nieder
2018-06-12 18:19 ` Stefan Beller
1 sibling, 1 reply; 8+ messages in thread
From: Jonathan Nieder @ 2018-06-12 18:07 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, masayasuzuki
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
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] builtin/send-pack: populate the default configs
2018-06-12 17:26 [PATCH] builtin/send-pack: populate the default configs Masaya Suzuki
2018-06-12 17:38 ` Junio C Hamano
@ 2018-06-12 18:16 ` Jonathan Nieder
2018-06-12 18:22 ` Eric Sunshine
1 sibling, 1 reply; 8+ messages in thread
From: Jonathan Nieder @ 2018-06-12 18:16 UTC (permalink / raw)
To: Masaya Suzuki; +Cc: git, Jeff King
Hi,
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>
> ---
> 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.
> --- 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>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] builtin/send-pack: report gpg config errors
2018-06-12 18:07 ` Jonathan Nieder
@ 2018-06-12 18:19 ` Stefan Beller
0 siblings, 0 replies; 8+ messages in thread
From: Stefan Beller @ 2018-06-12 18:19 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git, Masaya Suzuki
On Tue, Jun 12, 2018 at 11:08 AM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> 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. :/
>
> 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.
>
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] builtin/send-pack: populate the default configs
2018-06-12 18:16 ` [PATCH] builtin/send-pack: populate the default configs Jonathan Nieder
@ 2018-06-12 18:22 ` Eric Sunshine
0 siblings, 0 replies; 8+ messages in thread
From: Eric Sunshine @ 2018-06-12 18:22 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Masaya Suzuki, Git List, Jeff King
On Tue, Jun 12, 2018 at 2:16 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-06-12 18:22 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-12 17:26 [PATCH] builtin/send-pack: populate the default configs Masaya Suzuki
2018-06-12 17:38 ` Junio C Hamano
2018-06-12 17:53 ` [PATCH] builtin/send-pack: report gpg config errors Stefan Beller
2018-06-12 17:55 ` Eric Sunshine
2018-06-12 18:07 ` Jonathan Nieder
2018-06-12 18:19 ` Stefan Beller
2018-06-12 18:16 ` [PATCH] builtin/send-pack: populate the default configs Jonathan Nieder
2018-06-12 18:22 ` Eric Sunshine
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).