Hi Eric, I made all the changes you suggested and split it into two independent changes: This patch handles the --color option: https://github.com/git/git/pull/955 This patch fixes the coloring inconsistency: https://github.com/git/git/pull/954 Both have failed a CI test, but the reason has nothing to do with my code. CI/PR / static-analysis VM is trying to install the package coccinelle, but is failing to install it with the following: E: Unable to locate package coccinelle Error: Process completed with exit code 100. On Tue, Feb 2, 2021 at 10:46 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > On Mon, Feb 1, 2021 at 11:09 PM Lance Ward <ljward10@xxxxxxxxx> wrote: > > On Sun, Jan 31, 2021 at 5:09 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > > I think the approach I suggested of patching those wt-status.c functions > > > to use: > > > rev.diffopt.use_color = s->use_color; > > > would fix this inconsistency, wouldn't it? > > > > I've made the change you requested and it resolves the issue. > > It also fixed the inconsistency I mentioned. I only needed > > to modify wt_longstatus_print_verbose to resolve the issue > > since it is the only status path that had an issue with the > > git status command. > > Okay, makes sense. As long as you insert the assignment somewhere > above the special case: > > if (s->fp != stdout) { > rev.diffopt.use_color = 0; > wt_status_add_cut_line(s->fp); > } > > then it should work correctly, I would think. However, it might be > easier for people to grok the logic overall if you incorporate it into > that conditional, perhaps like this: > > if (s->fp != stdout) { > rev.diffopt.use_color = 0; > wt_status_add_cut_line(s->fp); > } else { > rev.diffopt.use_color = s->use_color; > } > > Or this: > > if (s->fp == stdout) { > rev.diffopt.use_color = s->use_color; > else { > rev.diffopt.use_color = 0; > wt_status_add_cut_line(s->fp); > } > > It's subjective, of course, so use your best judgment. > I've made the change you suggested. > > > In fact, I can envision this patch being re-rolled as a two-patch > > > series which (1) patches the wt-status.c functions to do > > > `rev.diffopt.use_color = s->use_color` which should make > > > `color.status` imply `color.diff`, and (2) adds a --color option to > > > `git status` which sets `wt_status.use_color` (which would then be > > > automatically inherited by the diff machinery due to the first patch). > > > > Right now as it stands my patch resolves both of these, but > > if you'd like to make it two separate patches that would be fine. > > The reason I was thinking of splitting these changes into two patches > is that they have different purposes. You'd sell the first patch as a > straight up bug fix, and it's easier to formulate a proper "sales > spiel" for that if it's not blurred with other changes. (This may be > important because it could be slightly controversial if other people > don't consider the behavior as being buggy.) The second patch would be > an easy sell as a straightforward and simple enhancement. Another > reason for splitting them into two patches is that doing so would make > it easier to revert the bug-fix patch separately if it turns out that > there are unforeseen negative side-effects. > > Having said that, a well-crafted commit message may very well make it > easy to sell the change(s) as a single patch. Again, use your best > judgment. I've split it into two patches.
