On Wed, 3 Oct 2018 at 00:24, Jeff King <peff@peff.net> wrote:

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

Ok good, I did have the feeling that the logic became actually clearer
to me after I wrestled with the test code, so I think this means I
didn't just imagine that. :)

> (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).

Oh, I think had run into some trouble with the test runner complaining
about a broken &&-chain, but it seems to work fine now (perhaps I
missing the && somewhere else that I fixed later).

> Why do we need the tag name to be different?

Otherwise the 'git checkout' command complains about an ambiguous ref
(added that to the comment).

> >       git checkout 1 -b merge &&
>
> 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).

Oops, right. I've inlined it.

> 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):

Yup, works (I again had run into some problems with &&-chaining
earlier, but now it works fine)

> > * left
> > <RED>|<RESET> *<BLUE>-<RESET><BLUE>-<RESET><MAGENTA>-<RESET><MAGENTA>.<RESET>   octopus-merge
[...]
>
> 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;
[...]
> you get this:
>
>   * left
>   R *BBMM   octopus-merge
>   R RY B M
[...]
> which is admittedly pretty horrible, too, but at least resembles a
> graph. I dunno.

Yeah, but it's lossy, so it doesn't seem usable for the test. Maybe
doubling up some characters?

**  left
R|  **B-B-M-M.      octopus-merge
R|  R|Y\  B\  M\
R|R/  Y/  B/  M/
R|  Y|  B|  **  4
R|  Y|  **  M|  3
R|  Y|  M|M/
R|  **  M|  2
R|  M|M/
**  M|  1
M|M/
**  initial

> 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.

Well, it's easy enough to set the colors explicitly. After doing this
I noticed that green seems to be skipped. Not sure if that's a bug or
not.

> Try not to put "git" on the left-hand side of a pipe, since it means
> we'll miss its exit code

Ok.

> Leftover "debug" cruft?
>
> The same pipe comment applies as above.
>
> > test_done
> > test_done
>
> Two dones; we exit after the first one (so everything after this is
> ignored).

Oops, yeah, this script was still a bit of a rough draft.

> 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?

t4202-log.sh already does test a "normal" octopus merge (starting
around line 615, search for "octopus-a"). But that is only a 3-parent
merge. And adding another test is easy enough.