Message ID | pull.1270.v2.git.git.1674573972087.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v2] ssh signing: better error message when key not in agent | expand |
"Adam Szkoda via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Adam Szkoda <adaszko@gmail.com> > > When signing a commit with a SSH key, with the private key missing from > ssh-agent, a confusing error message is produced: > > error: Load key > "/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7": > invalid format? fatal: failed to write commit object > > The temporary file .git_signing_key_tmpkArSj7 created by git contains a > valid *public* key. The error message comes from `ssh-keygen -Y sign' and > is caused by a fallback mechanism in ssh-keygen whereby it tries to > interpret .git_signing_key_tmpkArSj7 as a *private* key if it can't find in > the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All that > needs to be done is to pass an additional backward-compatible option -U to > 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets the file > as public key and expects to find the private key in the agent. > > As a result, when the private key is missing from the agent, a more accurate > error message gets produced: > > error: Couldn't find key in agent > > [1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429 > > Signed-off-by: Adam Szkoda <adaszko@gmail.com> > --- Well explained. > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v2 > Pull-Request: https://github.com/git/git/pull/1270 > > Range-diff vs v1: > > 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent > -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent This is a fairly useless range-diff. Even when a range-diff shows the differences in the patches, mechanically generated range-diff can only show _what_ changed. It is helpful to explain the changes in your own words to highlight _why_ such changes are done, and this place after the "---" line and the diffstat we see below is the place to do so. Does GitGitGadget allow its users to describe the differences since the previous iteration yourself? > gpg-interface.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/gpg-interface.c b/gpg-interface.c > index f877a1ea564..33899a450eb 100644 > --- a/gpg-interface.c > +++ b/gpg-interface.c > @@ -998,6 +998,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, > char *ssh_signing_key_file = NULL; > struct strbuf ssh_signature_filename = STRBUF_INIT; > const char *literal_key = NULL; > + int literal_ssh_key = 0; > > if (!signing_key || signing_key[0] == '\0') > return error( > @@ -1005,6 +1006,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, > > if (is_literal_ssh_key(signing_key, &literal_key)) { > /* A literal ssh key */ > + literal_ssh_key = 1; > key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX"); > if (!key_file) > return error_errno( > @@ -1036,11 +1038,14 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, > } > > strvec_pushl(&signer.args, use_format->program, > - "-Y", "sign", > - "-n", "git", > - "-f", ssh_signing_key_file, > - buffer_file->filename.buf, > - NULL); > + "-Y", "sign", > + "-n", "git", > + "-f", ssh_signing_key_file, > + NULL); Please avoid making a pointless indentation change like this. We do not pass filename yet with this pushl(), because ... > + if (literal_ssh_key) { > + strvec_push(&signer.args, "-U"); > + } ... when we give a literal key, we want to insert "-U" in front, and then > + strvec_push(&signer.args, buffer_file->filename.buf); ... the filename. Which makes sense. The insertion of "-U" is a single statement as the body of a if() statement. We do not want {} around it, by the way. Other than that, nicely done. Thanks. > sigchain_push(SIGPIPE, SIG_IGN); > ret = pipe_command(&signer, NULL, 0, NULL, 0, &signer_stderr, 0); > > base-commit: 844ede312b4e988881b6e27e352f469d8ab80b2a
On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano <gitster@pobox.com> wrote: > > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v2 > > Pull-Request: https://github.com/git/git/pull/1270 > > > > Range-diff vs v1: > > > > 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent > > -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent > > This is a fairly useless range-diff. > > Even when a range-diff shows the differences in the patches, > mechanically generated range-diff can only show _what_ changed. It > is helpful to explain the changes in your own words to highlight > _why_ such changes are done, and this place after the "---" line > and the diffstat we see below is the place to do so. > > Does GitGitGadget allow its users to describe the differences since > the previous iteration yourself? No, I don't think it does. It got generated automatically without giving me an opportunity to edit. > > gpg-interface.c | 15 ++++++++++----- > > 1 file changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/gpg-interface.c b/gpg-interface.c > > index f877a1ea564..33899a450eb 100644 > > --- a/gpg-interface.c > > +++ b/gpg-interface.c > > @@ -998,6 +998,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, > > char *ssh_signing_key_file = NULL; > > struct strbuf ssh_signature_filename = STRBUF_INIT; > > const char *literal_key = NULL; > > + int literal_ssh_key = 0; > > > > if (!signing_key || signing_key[0] == '\0') > > return error( > > @@ -1005,6 +1006,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, > > > > if (is_literal_ssh_key(signing_key, &literal_key)) { > > /* A literal ssh key */ > > + literal_ssh_key = 1; > > key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX"); > > if (!key_file) > > return error_errno( > > @@ -1036,11 +1038,14 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, > > } > > > > strvec_pushl(&signer.args, use_format->program, > > - "-Y", "sign", > > - "-n", "git", > > - "-f", ssh_signing_key_file, > > - buffer_file->filename.buf, > > - NULL); > > + "-Y", "sign", > > + "-n", "git", > > + "-f", ssh_signing_key_file, > > + NULL); > > Please avoid making a pointless indentation change like this. Yep, removed. It was largely accidental. > We do > not pass filename yet with this pushl(), because ... > > > + if (literal_ssh_key) { > > + strvec_push(&signer.args, "-U"); > > + } > > ... when we give a literal key, we want to insert "-U" in front, and then > > > + strvec_push(&signer.args, buffer_file->filename.buf); > > ... the filename. Which makes sense. I'm not sure what you mean in this paragraph. If there's something more that needs to be done, I'd appreciate it if you could rephrase it. > The insertion of "-U" is a single statement as the body of a if() > statement. We do not want {} around it, by the way. Removed the superfluous {}. Thanks — Adam
Adam Szkoda <adaszko@gmail.com> writes: > On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano <gitster@pobox.com> wrote: >> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v2 >> > Pull-Request: https://github.com/git/git/pull/1270 >> > >> > Range-diff vs v1: >> > >> > 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent >> > -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent >> >> This is a fairly useless range-diff. >> >> Even when a range-diff shows the differences in the patches, >> mechanically generated range-diff can only show _what_ changed. It >> is helpful to explain the changes in your own words to highlight >> _why_ such changes are done, and this place after the "---" line >> and the diffstat we see below is the place to do so. >> >> Does GitGitGadget allow its users to describe the differences since >> the previous iteration yourself? > > No, I don't think it does. It got generated automatically without > giving me an opportunity to edit. Hmph. The text after "---" and before "Fetch-it-via:" does look like something a human wrote. The part often is byte-for-byte identical duplicate of the proposed log message, but I think I have seen patches via GitGitGadget that have different text there, and was hoping perhaps the authors can use it to describe commentary for the range-diff. > Yep, removed. It was largely accidental. > ... > Removed the superfluous {}. > > Thanks Thanks. Looking good. Will queue and merge down to 'next'.
Adam Szkoda <adaszko@gmail.com> writes: >> We do >> not pass filename yet with this pushl(), because ... >> >> > + if (literal_ssh_key) { >> > + strvec_push(&signer.args, "-U"); >> > + } >> >> ... when we give a literal key, we want to insert "-U" in front, and then >> >> > + strvec_push(&signer.args, buffer_file->filename.buf); >> >> ... the filename. Which makes sense. > > I'm not sure what you mean in this paragraph. If there's something > more that needs to be done, I'd appreciate it if you could rephrase > it. "Which makes sense." is the key conclusion. Instead of saying "This part of the patch looks good" without explaining why I say so (it could be that I am saying so without really reading or understanding the changes or thinking the ramifications of the change through), the earlier parts that lead to the conclusion is a way to give weight to the conclusion. In other words, it is meant to show that the reviewer did read the patch well enough to understand the reasoning behind it. Thanks.
On Wed, Jan 25, 2023 at 8:05 AM Adam Szkoda <adaszko@gmail.com> wrote: > On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano <gitster@pobox.com> wrote: > > > Range-diff vs v1: > > > > > > 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent > > > -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent > > > > This is a fairly useless range-diff. > > > > Even when a range-diff shows the differences in the patches, > > mechanically generated range-diff can only show _what_ changed. It > > is helpful to explain the changes in your own words to highlight > > _why_ such changes are done, and this place after the "---" line > > and the diffstat we see below is the place to do so. > > > > Does GitGitGadget allow its users to describe the differences since > > the previous iteration yourself? > > No, I don't think it does. It got generated automatically without > giving me an opportunity to edit. Yes, the user can describe the differences since the previous iteration by editing the pull-request's description. Specifically, when ready to send a new iteration: (1) force push the new iteration to the same branch on GitHub (2) edit the pull-request description; this is the very first "comment" on the pull-request page; press the "..." button on that comment and choose the "Edit" menu item; revise the text to describe the changes since the previous revision, then press the "Update comment" button to save (3) post a "/submit" comment to the pull-request to tell GitGitGadget to send the new revision to the Git mailing list
Eric Sunshine <sunshine@sunshineco.com> writes: > On Wed, Jan 25, 2023 at 8:05 AM Adam Szkoda <adaszko@gmail.com> wrote: >> On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano <gitster@pobox.com> wrote: >> > > Range-diff vs v1: >> > > >> > > 1: 0ce06076242 < -: ----------- ssh signing: better error message when key not in agent >> > > -: ----------- > 1: 03dfca79387 ssh signing: better error message when key not in agent >> > >> > This is a fairly useless range-diff. >> > >> > Even when a range-diff shows the differences in the patches, >> > mechanically generated range-diff can only show _what_ changed. It >> > is helpful to explain the changes in your own words to highlight >> > _why_ such changes are done, and this place after the "---" line >> > and the diffstat we see below is the place to do so. >> > >> > Does GitGitGadget allow its users to describe the differences since >> > the previous iteration yourself? >> >> No, I don't think it does. It got generated automatically without >> giving me an opportunity to edit. > > Yes, the user can describe the differences since the previous > iteration by editing the pull-request's description. Specifically, > when ready to send a new iteration: > > (1) force push the new iteration to the same branch on GitHub > > (2) edit the pull-request description; this is the very first > "comment" on the pull-request page; press the "..." button on that > comment and choose the "Edit" menu item; revise the text to describe > the changes since the previous revision, then press the "Update > comment" button to save > > (3) post a "/submit" comment to the pull-request to tell GitGitGadget > to send the new revision to the Git mailing list Thanks. I thought the above would make a good addition to our documentation set. Documentation/MyFirstContribution.txt does have this to say: Once you have your branch again in the shape you want following all review comments, you can submit again: ---- $ git push -f remotename psuh ---- Next, go look at your pull request against GitGitGadget; you should see the CI has been kicked off again. Now while the CI is running is a good time for you to modify your description at the top of the pull request thread; it will be used again as the cover letter. You should use this space to describe what has changed since your previous version, so that your reviewers have some idea of what they're looking at. When the CI is done running, you can comment once more with `/submit` - GitGitGadget will automatically add a v2 mark to your changes. before it talks about doing the "/submit" again. Expanding the above into a bulletted list form like you did might make it easier to follow through, perhaps? I dunno.
On Wed, Jan 25, 2023 at 5:23 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > On Wed, Jan 25, 2023 at 8:05 AM Adam Szkoda <adaszko@gmail.com> wrote: > >> On Tue, Jan 24, 2023 at 6:52 PM Junio C Hamano <gitster@pobox.com> wrote: > >> > Does GitGitGadget allow its users to describe the differences since > >> > the previous iteration yourself? > >> > >> No, I don't think it does. It got generated automatically without > >> giving me an opportunity to edit. > > > > Yes, the user can describe the differences since the previous > > iteration by editing the pull-request's description. Specifically, > > when ready to send a new iteration: > > > > (1) force push the new iteration to the same branch on GitHub > > > > (2) edit the pull-request description; this is the very first > > "comment" on the pull-request page; press the "..." button on that > > comment and choose the "Edit" menu item; revise the text to describe > > the changes since the previous revision, then press the "Update > > comment" button to save > > > > (3) post a "/submit" comment to the pull-request to tell GitGitGadget > > to send the new revision to the Git mailing list > > Thanks. I thought the above would make a good addition to our > documentation set. Documentation/MyFirstContribution.txt does have > this to say: > > Next, go look at your pull request against GitGitGadget; you should see the CI > has been kicked off again. Now while the CI is running is a good time for you > to modify your description at the top of the pull request thread; it will be > used again as the cover letter. You should use this space to describe what > has changed since your previous version, so that your reviewers have some idea > of what they're looking at. When the CI is done running, you can comment once > more with `/submit` - GitGitGadget will automatically add a v2 mark to your > changes. > > before it talks about doing the "/submit" again. Expanding the > above into a bulletted list form like you did might make it easier > to follow through, perhaps? I dunno. Perhaps, though I wonder how many people consult MyFirstConstribution. Maybe SubmittingPatches would be a better location for such instructions, although (I notice now that) that would be an even bigger change since SubmittingPatches doesn't mention GitGitGadget at all. Another appropriate place might be the "welcome" message that GitGitGadget posts the very first time someone submits a patch series via that tool (assuming that the welcome message doesn't already explain it).
diff --git a/gpg-interface.c b/gpg-interface.c index f877a1ea564..33899a450eb 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -998,6 +998,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, char *ssh_signing_key_file = NULL; struct strbuf ssh_signature_filename = STRBUF_INIT; const char *literal_key = NULL; + int literal_ssh_key = 0; if (!signing_key || signing_key[0] == '\0') return error( @@ -1005,6 +1006,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, if (is_literal_ssh_key(signing_key, &literal_key)) { /* A literal ssh key */ + literal_ssh_key = 1; key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX"); if (!key_file) return error_errno( @@ -1036,11 +1038,14 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, } strvec_pushl(&signer.args, use_format->program, - "-Y", "sign", - "-n", "git", - "-f", ssh_signing_key_file, - buffer_file->filename.buf, - NULL); + "-Y", "sign", + "-n", "git", + "-f", ssh_signing_key_file, + NULL); + if (literal_ssh_key) { + strvec_push(&signer.args, "-U"); + } + strvec_push(&signer.args, buffer_file->filename.buf); sigchain_push(SIGPIPE, SIG_IGN); ret = pipe_command(&signer, NULL, 0, NULL, 0, &signer_stderr, 0);