On Thu, Aug 2, 2018 at 8:22 PM Junio C Hamano <gits...@pobox.com> wrote:
> >
> > Helped-by: Duy Nguyen <pclo...@gmail.com>
> > Signed-off-by: Han-Wen Nienhuys <han...@google.com>
> > ---
> > Documentation/config.txt | 9 +++
> > help.c | 1 +
> > help.h | 1 +
> > sideband.c | 119 +++++++++++++++++++++++++---
> > t/t5409-colorize-remote-messages.sh | 47 +++++++++++
> > 5 files changed, 168 insertions(+), 9 deletions(-)
> > create mode 100644 t/t5409-colorize-remote-messages.sh
>
> I'll "chmod +x" while queuing.
>
Done.
> If your "make test" did not catch this as an error, then we may need
> to fix t/Makefile, as it is supposed to run test-lint.
I've been running tests individually as
sh t5409-colorize-remote-messages.sh -v -d
> > +color.remote::
> > + A boolean to enable/disable colored remote output. If unset,
> > + then the value of `color.ui` is used (`auto` by default).
>
> Nobody tells the end-users what "colored remote output" does;
> arguably they can find it out themselves by enabling the feature and
> observing remote messages, but that is not user friendly.
expanded doc.
> > +color.remote.<slot>::
> > + Use customized color for each remote keywords. `<slot>` may be
>
> Isn't 'each' a singular, i.e. "for each remote keyword"? If so I do
> not mind dropping 's' myself while queuing.
Done.
>
> > + `hint`, `warning`, `success` or `error` which match the
> > + corresponding keyword.
>
> We need to say that keywords are painted case insensitively
> somewhere in the doc. Either do that here, or in the updated
> description of `color.remote`---I am not sure which one results in
> more readable text offhand.
Done.
> > +void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
> > +{
> > + int i;
>
> In a block with a dozen more more lines, it is easier to have a
> blank line between decls and the first statement, i.e. here.
Done.
> > + if (!want_color_stderr(use_sideband_colors())) {
>
> The above line is indented with SP followed by HT; don't.
Fixed. It would be great if there were a pre-commit hook that I could
install to prevent this from ever getting committed.
> > + struct kwtable* p = keywords + i;
>
> struct kwtable *p = keywords + i;
Done.
> > + int len = strlen(p->keyword);
> > + /*
> > + * Match case insensitively, so we colorize output from
> > existing
> > + * servers regardless of the case that they use for their
> > + * messages. We only highlight the word precisely, so
> > + * "successful" stays uncolored.
> > + */
>
> Indent with tabs, not a run of spaces, i.e.
Done.
> Use write_script, i.e. instead of all the above, say
Done.
> Our tests are not written to demonstrate that our code works as
> written. It is to protect our code from getting broken by others
> who may not share vision of the original author. Make sure that you
> cast what you care about in stone, e.g. include "echo ERROR: bad" or
> something in the above to ensure that future updates to the code
> will not turn the match into a case sensitive one without breaking
> the test suite.
Add some more cases.
> > + echo 1 >file &&
> > + git add file &&
> > + git commit -m 1 &&
> > + git clone . child &&
> > + cd child &&
> > + echo 2 > file &&
> > + git commit -a -m 2
>
> Don't chdir the whole testing environment like this. Depending on
> the success and failure in the middle of the above &&-chain, the
> next test will start at an unexpected place, which is bad.
>
> Instead, do something like
>
> git clone . child &&
> echo 2 >child/file &&
> git -C child commit -a -m 2
>
> or
>
Done.
> > +test_expect_success 'push' '
> > + git -c color.remote=always push -f origin HEAD:refs/heads/newbranch
> > 2>output &&
> > + test_decode_color <output >decoded &&
> > + grep "<BOLD;RED>error<RESET>:" decoded &&
> > + grep "<YELLOW>hint<RESET>:" decoded &&
> > + grep "<BOLD;GREEN>success<RESET>:" decoded &&
> > + grep "<BOLD;YELLOW>warning<RESET>:" decoded &&
> > + grep "prefixerror: error" decoded
>
> A comment before this test (which covers both of these two) that
> explains why many "grep" invocations are necessary, instead of a
> comparison with a single multi-line expected result file. I am
> guessing that it is *not* because you cannot rely on the order of
> these lines coming out from the update hook, but because the remote
> output have lines other than what is given by the update hook and
> we cannot afford to write them in the expected result file.
Comparing exact outputs is IMO an antipattern in general. It makes the
test more fragile than they need to be. (what if we change the
"remote: " prefix to something else for example?).
If using a golden output, the second test would either require a
repetitive, too long golden file, or it would use loose greps anyway.
Added a comment.
--
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
