Re: [PATCH v2] ssh signing: better error message when key not in agent

[Adam Szkoda <adaszko@gmail.com>]

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