Re: [PATCH v6 2/9] ssh signing: add ssh signature format and signing using ssh keys

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

>> +/*
>> + * Strip CR from the line endings, in case we are on Windows.
>> + * NEEDSWORK: make it trim only CRs before LFs and rename
>> + */
>> +static void remove_cr_after(struct strbuf *buffer, size_t offset)
>> +{
>> +	size_t i, j;
>> +
>> +	for (i = j = offset; i < buffer->len; i++) {
>> +		if (buffer->buf[i] != '\r') {
>> +			if (i != j)
>> +				buffer->buf[j] = buffer->buf[i];
>> +			j++;
>> +		}
>> +	}
>> +	strbuf_setlen(buffer, j);
>> +}
>
> In the future, I would prefer refactoring like this to be in its own
> patch. For the moment, this should probably be called "remove_cr" (no
> "after" as CRs are removed wherever they are in the string).

You have me to blame for that "after".  It was meant to signal that
CR's before the given "offset" are retained.

> A config that has 2 modes of operation is quite error-prone, I think.
> For example, a user could put a path starting with "ssh-" (admittedly
> unlikely since it would usually be an absolute path, but not
> impossible). And also from an implementation point of view, here the
> "ssh-" is case-sensitive, but in a future patch, there is a "ssh-" that
> is case-insensitive.
>
> Can this just always take a path?

Sensible simplification, I guess.

Thanks for a careful review.

>> +	if (ret) {
>> +		if (strstr(signer_stderr.buf, "usage:"))
>> +			error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)"));
>> +
>> +		error("%s", signer_stderr.buf);
>> +		goto out;
>> +	}
>
> Checking for "usage:" seems fragile -  a binary running in a different
> locale might emit a different string, and legitimate output may somehow
> contain the string "usage:". Is there a different way to detect a
> version mismatch?