{"thread":{"id":"48694","subject":"[PATCH] builtin/send-pack: populate the default configs","startedAt":"2018-06-12T17:26:41Z","lastAt":"2018-06-12T18:22:19Z","messageCount":8,"participants":["Masaya Suzuki","Junio C Hamano","Stefan Beller","Eric Sunshine","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"349992","messageId":"20180612172623.165420-1-masayasuzuki@google.com","threadId":"48694","inReplyTo":null,"subject":"[PATCH] builtin/send-pack: populate the default configs","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-06-12T17:26:23Z","receivedAt":"2018-06-12T17:26:41Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"builtin/send-pack didn't call git_default_config, and because of this\ngit push --signed didn't respect the username and email in gitconfig in\nthe HTTP transport.\n\nSigned-off-by: Masaya Suzuki \n---\n builtin/send-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 4923b1058..42fd8d1a3 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -121,7 +121,7 @@ static int send_pack_config(const char *k, const char *v, void *cb)\n \t\t\t}\n \t\t}\n \t}\n-\treturn 0;\n+\treturn git_default_config(k, v, cb);\n }\n \n int cmd_send_pack(int argc, const char **argv, const char *prefix)\n-- \n2.18.0.rc1.242.g61856ae69a-goog\n\n"},{"id":"349994","messageId":"xmqqk1r3aorh.fsf@gitster-ct.c.googlers.com","threadId":"48694","inReplyTo":"20180612172623.165420-1-masayasuzuki@google.com","subject":"Re: [PATCH] builtin/send-pack: populate the default configs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-12T17:38:42Z","receivedAt":"2018-06-12T17:38:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Masaya Suzuki writes:\n\n> builtin/send-pack didn't call git_default_config, and because of this\n> git push --signed didn't respect the username and email in gitconfig in\n> the HTTP transport.\n>\n> Signed-off-by: Masaya Suzuki \n> ---\n> builtin/send-pack.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nMakes sense. I wonder if this was deliberate (i.e. we had some\nreason not wanting to be affected by some _other_ configuration\nitems and refraining from doing the default-config was the way to\nimplement it), but I do not think of a reason offhand.\n\nWill queue. Thanks.\n\n>\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index 4923b1058..42fd8d1a3 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -121,7 +121,7 @@ static int send_pack_config(const char *k, const char *v, void *cb)\n> \t\t\t}\n> \t\t}\n> \t}\n> -\treturn 0;\n> +\treturn git_default_config(k, v, cb);\n> }\n> \n> int cmd_send_pack(int argc, const char **argv, const char *prefix)\n"},{"id":"349996","messageId":"20180612175344.65466-1-sbeller@google.com","threadId":"48694","inReplyTo":"xmqqk1r3aorh.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] builtin/send-pack: report gpg config errors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-12T17:53:44Z","receivedAt":"2018-06-12T17:53:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Any caller except of git_gpg_config() except the one in send_pack_config()\nhandles the return value of git_gpg_config(). Also handle the return value\nthere.\n\nSigned-off-by: Stefan Beller \n---\n\n> Makes sense. I wonder if this was deliberate\n\nThere was no need for other configuration to be loaded and signing\npushes is the first that needs repository configuration.\n\nLooking at 68c757f2199 (push: add a config option push.gpgSign for default\nsigned pushes, 2015-08-19) further, maybe we want to also have this?\n\nThanks,\nStefan\n\n builtin/send-pack.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 4923b1058c6..b54a0cae878 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -101,7 +101,8 @@ static void print_helper_status(struct ref *ref)\n \n static int send_pack_config(const char *k, const char *v, void *cb)\n {\n-\tgit_gpg_config(k, v, NULL);\n+\tif (git_gpg_config(k, v, NULL) < 0)\n+\t\treturn -1;\n \n \tif (!strcmp(k, \"push.gpgsign\")) {\n \t\tconst char *value;\n-- \n2.18.0.rc1.244.gcf134e6275-goog\n\n"},{"id":"349997","messageId":"CAPig+cTfWqd+kZUFdS4jT4yKnttvsB3ENwVNd5zCo8ew-HrqaQ@mail.gmail.com","threadId":"48694","inReplyTo":"20180612175344.65466-1-sbeller@google.com","subject":"Re: [PATCH] builtin/send-pack: report gpg config errors","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-12T17:55:47Z","receivedAt":"2018-06-12T17:55:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 12, 2018 at 1:53 PM, Stefan Beller wrote:\n> Any caller except of git_gpg_config() except the one in send_pack_config()\n\nDrop the first \"except\"?\n\nAlso: s/Any caller/All callers/\n\n> handles the return value of git_gpg_config(). Also handle the return value\n> there.\n>\n> Signed-off-by: Stefan Beller \n"},{"id":"349998","messageId":"20180612180705.GA28554@aiede.svl.corp.google.com","threadId":"48694","inReplyTo":"20180612175344.65466-1-sbeller@google.com","subject":"Re: [PATCH] builtin/send-pack: report gpg config errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-06-12T18:07:05Z","receivedAt":"2018-06-12T18:09:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> Any caller except of git_gpg_config() except the one in send_pack_config()\n> handles the return value of git_gpg_config(). Also handle the return value\n> there.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/send-pack.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n\nThe other callers do\n\n\tint status;\n\n\tif (!strcmp(k, \"key.i.handle\")) {\n\t\t...\n\t\treturn ...;\n\t}\n\treturn git_gpg_config(k, v, NULL);\n\nor\n\n\tint status;\n\n\tif (!strcmp(k, \"key.i.handle\")) {\n\t\t...\n\t\treturn ...;\n\t}\n\n\tstatus = git_gpg_config(k, v, NULL);\n\tif (status)\n\t\treturn status;\n\t...\n\nShould this do the same? That way, if config_fn_t learns to return\nmore fine-grained errors (e.g. a new return value of -2 or 1) then it\nwould be propagated.\n\nThanks,\nJonathan\n"},{"id":"350000","messageId":"20180612181624.GB28554@aiede.svl.corp.google.com","threadId":"48694","inReplyTo":"20180612172623.165420-1-masayasuzuki@google.com","subject":"Re: [PATCH] builtin/send-pack: populate the default configs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-06-12T18:16:24Z","receivedAt":"2018-06-12T18:16:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMasaya Suzuki wrote:\n\n> builtin/send-pack didn't call git_default_config, and because of this\n> git push --signed didn't respect the username and email in gitconfig in\n> the HTTP transport.\n>\n> Signed-off-by: Masaya Suzuki \n> ---\n> builtin/send-pack.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nDo you have a set of commands I can run to reproduce this? Bonus\npoints if they're in the form of a patch to t/, but commands I can\nmanually run would be fine, too.\n\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -121,7 +121,7 @@ static int send_pack_config(const char *k, const char *v, void *cb)\n> \t\t\t}\n> \t\t}\n> \t}\n> -\treturn 0;\n> +\treturn git_default_config(k, v, cb);\n\nsend-pack is not a command served by a daemon so this is less\npotentially scary than the corresponding potential change to\nupload-pack or receive-pack. Some configuration this brings in:\n\n- core.askpass: allows specifying an arbitrary command to use to\n ask for a password. Respecting this setting should be very useful,\n even if it would be scary in a daemon.\n\n- core.pager: allows specifying an arbitrary command to use as a\n pager, if pagination is on (but it shouldn't be on).\n- core.logallrefupdates: whether to create reflogs for new refs\n (including new remote-tracking refs). Good.\n- core.abbrev: what length of abbreviations to use when printing\n abbreviated object ids (good).\n- core.compression, core.packedgitwindowsize, etc: pack generation\n tunables (good).\n- advice.*: would allow us to make \"git push\" produce configurable\n advice (good!)\n- etc\n\nSo it looks like a very good change. Thanks for writing it.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"350001","messageId":"CAGZ79kadthZi1oj-2FFpiUiwDtK744Cr05Hj1MkMFPs8uto7ZA@mail.gmail.com","threadId":"48694","inReplyTo":"20180612180705.GA28554@aiede.svl.corp.google.com","subject":"Re: [PATCH] builtin/send-pack: report gpg config errors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-12T18:19:12Z","receivedAt":"2018-06-12T18:19:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jun 12, 2018 at 11:08 AM Jonathan Nieder wrote:\n>\n> Hi,\n>\n> Stefan Beller wrote:\n>\n> > Any caller except of git_gpg_config() except the one in send_pack_config()\n> > handles the return value of git_gpg_config(). Also handle the return value\n> > there.\n> >\n> > Signed-off-by: Stefan Beller \n> > ---\n> > builtin/send-pack.c | 3 ++-\n> > 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> The other callers do\n\nexcept builtin/log, which I followed. :/\n\n>\n> int status;\n>\n> if (!strcmp(k, \"key.i.handle\")) {\n> ...\n> return ...;\n> }\n> return git_gpg_config(k, v, NULL);\n\nThis conflicts with Masayas patch, that has a return of the\nstandard options.\n\n\n>\n> or\n>\n> int status;\n>\n> if (!strcmp(k, \"key.i.handle\")) {\n> ...\n> return ...;\n> }\n>\n> status = git_gpg_config(k, v, NULL);\n> if (status)\n> return status;\n> ...\n\nThis looks like a good idea.\n\nWill send a follow up patch.\n"},{"id":"350002","messageId":"CAPig+cQZe2TH01eWOYE2Gzo9vTjMoEAQFxZZhu7vcE2jwV9Yuw@mail.gmail.com","threadId":"48694","inReplyTo":"20180612181624.GB28554@aiede.svl.corp.google.com","subject":"Re: [PATCH] builtin/send-pack: populate the default configs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-12T18:22:15Z","receivedAt":"2018-06-12T18:22:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 12, 2018 at 2:16 PM, Jonathan Nieder wrote:\n> Masaya Suzuki wrote:\n>> builtin/send-pack didn't call git_default_config, and because of this\n>> git push --signed didn't respect the username and email in gitconfig in\n>> the HTTP transport.\n>>\n>> Signed-off-by: Masaya Suzuki \n>> ---\n> send-pack is not a command served by a daemon so this is less\n> potentially scary than the corresponding potential change to\n> upload-pack or receive-pack. Some configuration this brings in:\n>\n> - core.askpass: allows specifying an arbitrary command to use to\n> ask for a password. Respecting this setting should be very useful,\n> even if it would be scary in a daemon.\n>\n> - core.pager: allows specifying an arbitrary command to use as a\n> pager, if pagination is on (but it shouldn't be on).\n> - core.logallrefupdates: whether to create reflogs for new refs\n> (including new remote-tracking refs). Good.\n> - core.abbrev: what length of abbreviations to use when printing\n> abbreviated object ids (good).\n> - core.compression, core.packedgitwindowsize, etc: pack generation\n> tunables (good).\n> - advice.*: would allow us to make \"git push\" produce configurable\n> advice (good!)\n> - etc\n\nThis summary probably ought to be in the commit message itself (plus\nadditional commentary implied by the ending \"etc.\"). It's exactly the\nsort of information someone looking at this patch in the future will\nwant to know to feel assured that such behavior changes were taken\ninto account by the patch author.\n"}]}