Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

On Mon, Sep 24, 2018 at 08:27:47PM -0400, Noam Postavsky wrote:

Yeah, I really like your explanations/diagrams in the comments. It makes the logic very clear.

It's OK to start a new script if you have some tricky or complicated setup. Probably it ought to be t4214-log-graph-octopus to keep it near the other log tests. And it should be added in the actual patch, but I assume you just kept it out here since you weren't sure where to put it yet.

I'll try to comment on the test script itself.

This should actually describe what's going on in the test. Usually a one-sentence is OK, but I think it might be good to specifically mention that we're handling this special octopus case.

Please use:

  for i in "$@"

which is a bit less subtle (there's also only one caller of this function, so it could be inlined; note that it's OK to use "return" in a test_expect block).

Why do we need the tag name to be different?

This is assuming we just made a branch called "1", but that's one of the arguments. Probably this should be "$1" (or the whole thing should just be inlined so it is clear that what the set of parameters we expect is).

Good use of test_tick so that we have predictable traversal ordering.

It might actually be worth setting up the uncolored expect file as part of this, since it so neatly diagrams the graph you're trying to produce.

I.e., something like (completely untested; note that the leading indentation is all tabs, which will be removed by the "<<-" operator):

test_expect_success 'set up merge history' '
	# This is the graph we expect to generate here.
	cat >expect.uncolored <<-\EOF &&
	* left
	| *---.   octopus-merge
	| |\ \ \
	|/ / / /
	| | | * 4
	| | * | 3
	| | |/
	| * | 2
	| |/
	* | 1
	|/
	* initial
	EOF
	for i in 1 2 3 4; do
		git checkout -b $i $master || return $?
		# Make tag name different from branch name.
		test_commit $i $i $i tag$i || return $?
	done &&
	git checkout -b merge 1 &&
	test_tick &&
	git merge -m octopus-merge 1 2 3 4
'

A few style bits: we prefer to keep even setup steps like this inside a test_expect block (though you may see some very old tests which have not been fixed yet). Also, we omit the space after ">".

Yikes. :) This one is pretty hard to read. I'm not sure if there's a good alternative. If you pipe the output of test_decode through this:

  sed '
	s/<RED>.<RESET>/R/g;
	s/<BLUE>.<RESET>/B/g;
	s/<MAGENTA>.<RESET>/M/g;
	s/<YELLOW>.<RESET>/Y/g;
  '

you get this:

  * left
  R *BBMM   octopus-merge
  R RY B M
  RR Y B M
  R Y B * 4
  R Y * M 3
  R Y MM
  R * M 2
  R MM
  * M 1
  MM
  * initial

which is admittedly pretty horrible, too, but at least resembles a graph. I dunno.

I'm also not thrilled that we depend on the exact sequence of default colors, but I suspect it's not the first time. And it wouldn't be too hard to update it if that default changes.

Try not to put "git" on the left-hand side of a pipe, since it means we'll miss its exit code (and especially we'd miss its death due to ASan or Valgrind problems, which I think was one of the major ways of detecting the original problem). So:

  git log ... >actual.raw &&
  test_decode_color <actual.raw | sed ... >actual &&
  test_cmp expect.colors actual

This is the expect output that I suggested showing earlier. :)

Leftover "debug" cruft?

The same pipe comment applies as above.

Two dones; we exit after the first one (so everything after this is ignored).

I think it's OK to have a dedicated script for even these two tests, if it makes things easier to read. However, would we also want to test the octopus without the problematic graph here? I think if we just omit "left" we get that, don't we?

-Peff