Re: [PATCH 2/2] Highlight keywords in remote sideband output.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 1, 2018, 18:16 UTC
- Message-ID
- <xmqqsh3y6jfp.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <CAFQ2z_PXfp60C-aiizUURjcqr-A+VJQDjMJ+fU_5DOo10x+rcQ@mail.gmail.com>
Han-Wen Nienhuys <hanwen@google.com> writes:
Show 7 quoted lines
> On Wed, Aug 1, 2018 at 5:41 PM Junio C Hamano <gitster@pobox.com> wrote: > >> Hmm, do we actually say things like "Error: blah"? I am not sure if >> I like this strncasecmp all that much. > > this is for the remote end, so what we (git-core) says isn't all that > relevant.
It is very relevant, I would think. Because the coloring is controlled at the client end with this implementation, third-party remote implementations have strong incentive to follow what our remote end says and not to deviate. Preventing them from being different just to be different does help the users, no?
Show 14 quoted lines
>> > So, despite the explanation in the commit message, this function isn't >> > _generally_ highlighting keywords in the sideband. Instead, it is >> > highlighting a keyword only if it finds it at the start of string >> > (ignoring whitespace). Perhaps the commit message could be more clear >> > about that. >> >> Sounds good. >> >> I didn't comment on other parts of your review posed as questions >> (that require some digging and thinking), but I think they are all >> worthwhile thing to think about. > > Sorry for being dense, but do you want me to send an updated patch or > not based on your and Eric's comments or not?
It would help to see the comments responded with either "such a change is not needed for such and such reasons", "it may make sense but let's leave it to a follow-up patch later," etc., or with a "here is an updated patch, taking all the comments to the previous version into account---note that I rejected that particular comment because of such and such reasons".