On Thu, Jul 26, 2018 at 8:50 PM Junio C Hamano [off-list ref] wrote:

    Hold your objection a bit.  I'll come back to it soon ;-)

It theoretically may make more sense to color on the sender side,
but that is true only if done at a higher layer that prepares a
string and calls into the sideband code to send.  That code must
know what the bytes _mean_ a lot better than the code at the
sideband layer, so we do not have to guess.

Having written all the above, I think you are doing this at the
receiving end, so this actually makes quite a lot of sense.  I was
fooled greatly by "EMIT_sideband", which in reality does NOT emit at
all.  That function is badly misnamed.

fixed.

The function is more like "color sideband payload"; actual
"emitting" is still done at the places the code originally "emitted"
them to the receiving user.

quoted

Signed-off-by: Han-Wen Nienhuys <redacted>
Change-Id: I090412a1288bc2caef0916447e28c2d0199da47d

That's an unusual trailer we do not use in this project.

Yes, I know. I forgot to strip it from v2 again, though :-(

quoted

+void emit_sideband(struct strbuf *dest, const char *src, int n) {

Open brace on its own line.

Done.

quoted

+        // NOSUBMIT - maybe use transport.color property?

Avoid // comment.

Done

In our codebase in C, asterisk sticks to the variable not the type.

Done.

quoted

+        } keywords[] = {
+                {"hint", GIT_COLOR_YELLOW},
+                {"warning", GIT_COLOR_BOLD_YELLOW},
+                {"success", GIT_COLOR_BOLD_GREEN},
+                {"error", GIT_COLOR_BOLD_RED},
+                {},

Drop the last sentinel element, and instead stop iteration over the
array using (i < ARRAY_SIZE(keywords)).

Done.

quoted

+        for (struct kwtable* p = keywords; p->keyword; p++) {

Does anybody know if we already use the variable decl inside the
"for (...)" construct like this?  I know we discussed the idea of
using it somewhere as a weather-balloon to see if people with exotic
environment would mind, and I certainly do not mind making this
patch serve as such a weather-baloon, but if that is what we are
doing, I want the commit log message clearly marked as such, so that
we can later "git log --grep=C99" to see how long ago such an
experiment started.

I elided this. (I had expected for the compile to enforce restrictions
like these using --std=c99.)

quoted

  * Receive multiplexed output stream over git native protocol.

@@ -48,8 +95,10 @@ int recv_sideband(const char *me, int in_stream, int out)
              len--;
              switch (band) {
              case 3:
-                     strbuf_addf(&outbuf, "%s%s%s", outbuf.len ? "\n" : "",
-                                 DISPLAY_PREFIX, buf + 1);
+                     strbuf_addf(&outbuf, "%s%s", outbuf.len ? "\n" : "",
+                                 DISPLAY_PREFIX);
+                        emit_sideband(&outbuf, buf+1, len);
+

Let's not lose SP around "+" on both sides.

Also you seem to be indenting some lines with all SP and some with
mixture of HT and SP.  We prefer to use as many 8-column HT and then
fill the remainder with SP if needed to align with the opening
parenthesis on line above it (imitate the way strbuf_addf() is split
into two lines in the original in this hunk).

Fixed these, I think.

Thanks.  While there are need for mostly minor fix-ups, the logic
seems quite sane.  I think we can start without configuration and
then "fix" it later.

I need the configuration to be able to test this, though.

While I am OK with calling that variable "transport.<something>", we
should not define/explain it as "color output coming from the other
end over the wire transport".  Those who want to see messages
emitted remotely during "git fetch" in color would want to see the
messages generated by "git fetch" locally painted in the same color
scheme, so it makes sense to let "git fetch" pay attention and honor
that variable even for its own locally generated messages.  The
variable instead means "color any message, either generated locally
or remotely, during an operation that has something to do with
object transport", or something like that.

I used color.remote for the property,  but I'm happy to colorize the
bikeshed with another color.

--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado