On 13/10/2019 08:15, Jeff King wrote: > On Thu, Oct 10, 2019 at 09:13:42AM -0700, James Coglan via GitGitGadget wrote: > >> A final addition to that set of changes fixes the coloring of dashes that >> are drawn next to octopus merges, in a manner compatible with all these >> changes. The early commits in this set are refactorings that make the >> functional changes easier to introduce. > > As somebody who has pondered the octopus coloring code (for an > embarrassingly long time considering that it still has some bugs!), let > me just say thank you for taking this on. :) > > Moreover, I'll echo Dscho's comments elsewhere on the quality of this > series. It's a tricky topic to explain, and the way you've broken it up, > along with the commit messages, comments, and diagrams made it much > easier to follow. > > Others have already commented on things I saw while reading it, so I'll > just add a few more thoughts. > >> This series of patches are designed to improve the output of the log --graph >> command; their effect can be summed up in the following diagram: >> >> Before After >> ------ ----- >> >> * >> |\ >> | * * >> | |\ |\ >> | | * | * >> | | | | |\ >> | | \ | | * >> | *-. \ | * | >> | |\ \ \ |/|\| >> |/ / / / | | * >> | | | / | * | >> | | |/ | |/ >> | | * * / >> | * | |/ >> | |/ * >> * | >> |/ >> * > > I wondered if anybody would prefer the sparseness of the "before" > diagram, and if that would merit having two modes that could selected at > runtime. I'm not sure I'd want to carry the code for both types, though; > it seems like a recipe for the non-default output format to accrue a > bunch of bugs (since the graph code has proven itself to be a magnet for > off-by-ones and other weirdness). You're probably right about the non-default mode hiding bugs, but if you did want to do this, I believe the original rendering can be preserved by the following small switches: - always set `merge_layout = 1`; this will prevent skewing to the left - do not modify `edges_added` if the last parent fuses with its neighbor I believe all the layout changes are driven by these values, so you should be able to set in such a way as to preserve the original rendering by ignoring the special cases that lead to them having different values.
