Hi Eric,
I've updated my patch per your request.
On Sun, Jan 31, 2021 at 5:09 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> [cc:+junio +peff +duy +dscho +rene]
>
> On Sun, Jan 31, 2021 at 1:49 PM Lance Ward <ljward10@xxxxxxxxx> wrote:
> > On Sun, Jan 31, 2021 at 1:31 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> > > On Sat, Jan 30, 2021 at 10:51 AM Lance Ward via GitGitGadget
> > > <gitgitgadget@xxxxxxxxx> wrote:
> > > > diff --git a/diff.c b/diff.c
> > > > +void set_diff_color(int use_color)
> > > > +{
> > > > + diff_use_color_default = use_color;
> > > > +}
> > >
> > > This new API for setting `diff_use_color_default` feels a bit too
> > > quick-and-dirty and assumes that the caller has intimate knowledge
> > > about when it is safe/correct to call the new function. Did you
> > > consider the alternate approach of having wt-status functionality set
> > > the appropriate diff_options.use_color value at the time it drives the
> > > diff machinery? For instance, as a test, I added:
> > >
> > > rev.diffopt.use_color = s->use_color;
> > >
> > > to the functions wt-status.c:wt_status_collect_changes_worktree(),
> > > wt_status_collect_changes_index(), and wt_longstatus_print_verbose(),
> > > so that the `use_color` value from the `struct wt_status *` supplied
> > > by commit.c:cmd_status() is automatically applied to the diff options.
> >
> > Originally I tried to use what I thought would be a much more appropriate
> > method which is to simply let the --color flag set things the same way
> > as the config option status.color=always does, but I noticed it does
> > not work the same when piped.
>
> I'm not quite sure what you mean. How exactly did you originally
> implement --config to accomplish this?
>
> > For example the following produces full color output:
> > git -c status.color=always status -v
> >
> > However, running this colors only the status, not the diff:
> > git -c status.color=always status -v | cat
> >
> > Right now I can only get what I expect by running:
> > git -c status.color=always -c diff.color=always status -v | cat
> >
> > This appeared to me to be inconsistent behaviour [...]
>
> At an implementation level, this behavior makes sense, but I can see
> how it might be confusing and unexpected from a user's viewpoint. 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?
Yes, it did.
>
> > [...] and lead me down
> > a path trying to figure out where the color was being disabled which
> > made me realize that the status command shares code paths with
> > the commit message and porcelain output. I wanted to be very
> > careful not to do anything which might break either of these in some
> > unforeseen way which is why I created the function.
>
> I see where you are coming from and understand the desire to isolate
> this behavior change, however, I can't shake the feeling that this
> sledge-hammer approach is going in the wrong direction and that the
> fine-grained approach I suggested in my review is more desirable.
> Having said that, I'm not particularly familiar with this area of the
> code -- and had to spend some time digging through it to find those
> functions in wt-status.c to make the fine-grained approach work -- so
> it would be nice to hear from people who have spent a lot more time in
> that area of the code (I Cc:'d them).
>
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.
> > If you feel existing unit tests would mitigate any issues with commit
> > messages and porcelain output I can try going the route you
> > suggested instead?
>
> I doubt that anyone on this project feels that the unit tests have
> sufficient coverage to make any guarantees. However, for changes such
> as the one I proposed which might have unforeseen side-effects, Junio
> tends to let the changes "cook" for a while in his 'next' branch
> before promoting them to the 'master' branch so as to give time for
> unexpected fallout to be discovered.
>
I did not have to patch all the functions you mentioned and think
the new change is cleaner and will not break anything else.
> > Also if you agree the behavior of status.color should be the same for
> > both piped and not piped output I could add that to this patch as well?
>
> I'm not quite sure I understand your question. Are you asking if
> `color.status` should imply `color.diff`? If so, I haven't thought a
> lot about it, but I can see how the present behavior may have a high
> surprise-factor for users, so it might be worthwhile.
Yes, the inconsistency I mentioned was basically that:
color.status implies color.diff when outputting to stdout,
but this is not true when outputting to a file. My latest
change resolves this inconsistency. Now color.status
implies color.diff when running git status regardless
of where the output is going.
>
> 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).
>
> (By the way, `status.color` is a deprecated synonym for `color.status`.)
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.
