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). Diffing the output of "git diff --color --graph --oneline" on git.git both before and after your patch, the changes all look generally positive to me. The graph above is pretty dense, but that's not really what real-world graphs look like; it was designed to show off the changes. That plus the fact that you're fixing real bugs (like the octopus coloring) makes me inclined to just move to your suggested output. -Peff
