Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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?
