git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Lance Ward <ljward10@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: "Lance Ward via GitGitGadget" <gitgitgadget@gmail.com>,
	"Git List" <git@vger.kernel.org>,
	"Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"René Scharfe" <l.s.r@web.de>,
	"Eric Sunshine" <ericsunshine@gmail.com>
Subject: Re: [PATCH] status: learn --color for piping colored output
Date: Mon, 1 Feb 2021 22:09:00 -0600	[thread overview]
Message-ID: <CACPHW2VBEu=02HFhyrDes=6KceLtHzGDqBJVf2qAnD2s1f8VAg@mail.gmail.com> (raw)
In-Reply-To: <CAPig+cS-hnwp2HjkkFPeJ4aibFHnJ0VZq0DSVgdWB0H_q5=oXw@mail.gmail.com>

Hi Eric,

I've updated my patch per your request.

On Sun, Jan 31, 2021 at 5:09 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> [cc:+junio +peff +duy +dscho +rene]
>
> On Sun, Jan 31, 2021 at 1:49 PM Lance Ward <ljward10@gmail.com> wrote:
> > On Sun, Jan 31, 2021 at 1:31 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > On Sat, Jan 30, 2021 at 10:51 AM Lance Ward via GitGitGadget
> > > <gitgitgadget@gmail.com> 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.

  reply	other threads:[~2021-02-02  4:09 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-30 15:45 [PATCH] status: learn --color for piping colored output Lance Ward via GitGitGadget
2021-01-31  7:31 ` Eric Sunshine
2021-01-31 18:49   ` Lance Ward
2021-01-31 23:09     ` Eric Sunshine
2021-02-02  4:09       ` Lance Ward [this message]
2021-02-03  4:46         ` Eric Sunshine
2021-02-03 21:08           ` Lance Ward
2021-02-05  6:23             ` Eric Sunshine
2021-02-03 21:40 Lance Ward via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CACPHW2VBEu=02HFhyrDes=6KceLtHzGDqBJVf2qAnD2s1f8VAg@mail.gmail.com' \
    --to=ljward10@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=ericsunshine@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).