On Mon, Sep 24, 2018 at 08:27:47PM -0400, Noam Postavsky wrote:
> > So I think this is doing the right thing. I'm not sure if there's a
> > better way to explain "dashless" or not.
>
> I've updated the comments and renamed a few variables, see if that helps.
Yeah, I really like your explanations/diagrams in the comments. It makes
the logic very clear.
> > Do you feel comfortable trying to add something to the test suite for
> > this?
>
> Um, sort of. I managed to recast my script into the framework of the
> other tests (see attached t4299-octopus.sh); it seems like it should
> go into t4202-log.sh, but it's not clear to me how I can do this
> without breaking all the other tests which expect a certain sequence
> of commits.
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.
> #!/bin/sh
>
> test_description='git log'
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.
> . ./test-lib.sh
>
> make_octopus_merge () {
> for i ; do
> git checkout master -b $i || return $?
> # Make tag name different from branch name.
> test_commit $i $i $i tag$i || return $?
> done
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?
> 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).
> test_tick &&
> git merge -m octopus-merge "$@"
Good use of test_tick so that we have predictable traversal ordering.
> test_expect_success 'set up merge history' '
> test_commit initial &&
> make_octopus_merge 1 2 3 4 &&
> git checkout 1 -b L &&
> test_commit left
> '
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
'
> cat > expect.colors <<\EOF
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 ">".
> * left
> <RED>|<RESET> *<BLUE>-<RESET><BLUE>-<RESET><MAGENTA>-<RESET><MAGENTA>.<RESET> octopus-merge
> <RED>|<RESET> <RED>|<RESET><YELLOW>\<RESET> <BLUE>\<RESET> <MAGENTA>\<RESET>
> <RED>|<RESET><RED>/<RESET> <YELLOW>/<RESET> <BLUE>/<RESET> <MAGENTA>/<RESET>
> <RED>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> * 4
> <RED>|<RESET> <YELLOW>|<RESET> * <MAGENTA>|<RESET> 3
> <RED>|<RESET> <YELLOW>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET>
> <RED>|<RESET> * <MAGENTA>|<RESET> 2
> <RED>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET>
> * <MAGENTA>|<RESET> 1
> <MAGENTA>|<RESET><MAGENTA>/<RESET>
> * initial
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.
> test_expect_success 'log --graph with tricky octopus merge' '
> git log --color=always --graph --date-order --pretty=tformat:%s --all |
> test_decode_color | sed "s/ *\$//" >actual &&
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
> cat > expect <<\EOF
> * left
> | *---. octopus-merge
> | |\ \ \
> |/ / / /
> | | | * 4
> | | * | 3
> | | |/
> | * | 2
> | |/
> * | 1
> |/
> * initial
> EOF
This is the expect output that I suggested showing earlier. :)
> test_expect_success 'log --graph with tricky octopus merge' '
> debug git log --color=never --graph --date-order --pretty=tformat:%s --all |
> sed "s/ *\$//" >actual &&
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).
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
