Re: [PATCH v2 12/18] branch-diff: use color for the commit pairs
Hi Todd,
On Mon, 7 May 2018, Todd Zullinger wrote:
> Johannes Schindelin wrote:
> >
> > On Sat, 5 May 2018, Todd Zullinger wrote:
> >
> >>> @@ -430,6 +451,8 @@ int cmd_branch_diff(int argc, const char **argv,
> >>> const char *prefix)
> >>> struct string_list branch1 = STRING_LIST_INIT_DUP;
> >>> struct string_list branch2 = STRING_LIST_INIT_DUP;
> >>>
> >>> + git_diff_basic_config("diff.color.frag", "magenta", NULL);
> >>> +
> >>> diff_setup();
> >>> diffopt.output_format = DIFF_FORMAT_PATCH;
> >>> diffopt.flags.suppress_diff_headers = 1;
> >>
> >> Should this also (or only) check color.diff.frag?
> >
> > This code is not querying diff.color.frag, it is setting it. Without
> > any way to override it.
> >
> > Having thought about it longer, and triggered by Peff's suggestion to
> > decouple the "reverse" part from the actual color, I fixed this by
> >
> > - *not* setting .frag to magenta,
> >
> > - using the reverse method also to mark outer *hunk headers* (not only
> > the outer -/+ markers).
> >
> > - actually calling git_diff_ui_config()...
>
> Excellent. That seems to work nicely now, respecting the
> color.diff. config.
>
> > The current work in progress can be pulled as `branch-diff` from
> > https://github.com/dscho/git, if I could ask you to test?
>
> While the colors and 'branch --diff' usage seem to work
> nicely, I found that with 4ac3413cc8 ("branch-diff: left-pad
> patch numbers", 2018-05-05), 'git branch' itself is broken.
>
> Running 'git branch' creates a branch named 'branch'.
> Calling 'git branch --list' shows only 'branch' as the only
> branch.
>
> I didn't look too closely, but I'm guessing that the argv
> handling is leaving the 'branch' argument in place where it
> should be stripped?
>
> This unsurprisingly breaks a large number of tests. :)
You will be delighted to learn that all of this is now moot, as I renamed
the command to `range-diff`, as this is what the wisdom of the crowd
chose.
Ciao,
Johannes
Re: [PATCH v2 12/18] branch-diff: use color for the commit pairs
Hi Johannes,
Johannes Schindelin wrote:
> Hi Todd,
>
> On Sat, 5 May 2018, Todd Zullinger wrote:
>
>>> @@ -430,6 +451,8 @@ int cmd_branch_diff(int argc, const char **argv, const
>>> char *prefix)
>>> struct string_list branch1 = STRING_LIST_INIT_DUP;
>>> struct string_list branch2 = STRING_LIST_INIT_DUP;
>>>
>>> + git_diff_basic_config("diff.color.frag", "magenta", NULL);
>>> +
>>> diff_setup();
>>> diffopt.output_format = DIFF_FORMAT_PATCH;
>>> diffopt.flags.suppress_diff_headers = 1;
>>
>> Should this also (or only) check color.diff.frag?
>
> This code is not querying diff.color.frag, it is setting it. Without
> any way to override it.
>
> Having thought about it longer, and triggered by Peff's suggestion to
> decouple the "reverse" part from the actual color, I fixed this by
>
> - *not* setting .frag to magenta,
>
> - using the reverse method also to mark outer *hunk headers* (not only the
> outer -/+ markers).
>
> - actually calling git_diff_ui_config()...
Excellent. That seems to work nicely now, respecting the
color.diff. config.
> The current work in progress can be pulled as `branch-diff` from
> https://github.com/dscho/git, if I could ask you to test?
While the colors and 'branch --diff' usage seem to work
nicely, I found that with 4ac3413cc8 ("branch-diff: left-pad
patch numbers", 2018-05-05), 'git branch' itself is broken.
Running 'git branch' creates a branch named 'branch'.
Calling 'git branch --list' shows only 'branch' as the only
branch.
I didn't look too closely, but I'm guessing that the argv
handling is leaving the 'branch' argument in place where it
should be stripped?
This unsurprisingly breaks a large number of tests. :)
Thanks,
--
Todd
~~
A common mistake people make when trying to design something
completely foolproof is to underestimate the ingenuity of complete
fools.
-- Douglas Adams
Re: [PATCH v2 12/18] branch-diff: use color for the commit pairs
Hi Todd,
On Sat, 5 May 2018, Todd Zullinger wrote:
> > @@ -430,6 +451,8 @@ int cmd_branch_diff(int argc, const char **argv, const
> > char *prefix)
> > struct string_list branch1 = STRING_LIST_INIT_DUP;
> > struct string_list branch2 = STRING_LIST_INIT_DUP;
> >
> > + git_diff_basic_config("diff.color.frag", "magenta", NULL);
> > +
> > diff_setup();
> > diffopt.output_format = DIFF_FORMAT_PATCH;
> > diffopt.flags.suppress_diff_headers = 1;
>
> Should this also (or only) check color.diff.frag?
This code is not querying diff.color.frag, it is setting it. Without
any way to override it.
Having thought about it longer, and triggered by Peff's suggestion to
decouple the "reverse" part from the actual color, I fixed this by
- *not* setting .frag to magenta,
- using the reverse method also to mark outer *hunk headers* (not only the
outer -/+ markers).
- actually calling git_diff_ui_config()...
> I thought that color.diff.* was preferred over diff.color.*, though
> that doesn't seem to be entirely true in all parts of the current
> codebase.
>
> In testing this series it seems that setting color.diff
> options to change the various colors read earlier in this
> patch via diff_get_color_opt, as well as the 'frag' slot,
> are ignored. Setting them via diff.color. does work.
In my tests, it did not even work via diff.color.. But I think I
fixed this (at least my local testing confirms this) by calling
git_diff_ui_config().
> The later patch adding a man page documents branch-diff as
> using `diff.color.*` and points to git-config(1), but the
> config docs only list color.diff.
In the current form (`git branch --diff`), I refrained from going into
*so* much detail ;-) But the gist still holds, and now the code should
support it, too.
The current work in progress can be pulled as `branch-diff` from
https://github.com/dscho/git, if I could ask you to test?
Ciao,
Dscho
Re: [PATCH v2 12/18] branch-diff: use color for the commit pairs
Hi Johannes,
As many others have already said, thanks for this series!
I've used tbdiff a bit over the years, but having a builtin
will make it much more convenient (and the speed boost from
a C implementation will be a very nice bonus).
Johannes Schindelin wrote:
> @@ -430,6 +451,8 @@ int cmd_branch_diff(int argc, const char **argv, const
> char *prefix)
> struct string_list branch1 = STRING_LIST_INIT_DUP;
> struct string_list branch2 = STRING_LIST_INIT_DUP;
>
> + git_diff_basic_config("diff.color.frag", "magenta", NULL);
> +
> diff_setup();
> diffopt.output_format = DIFF_FORMAT_PATCH;
> diffopt.flags.suppress_diff_headers = 1;
Should this also (or only) check color.diff.frag? I thought
that color.diff.* was preferred over diff.color.*, though
that doesn't seem to be entirely true in all parts of the
current codebase.
In testing this series it seems that setting color.diff
options to change the various colors read earlier in this
patch via diff_get_color_opt, as well as the 'frag' slot,
are ignored. Setting them via diff.color. does work.
The later patch adding a man page documents branch-diff as
using `diff.color.*` and points to git-config(1), but the
config docs only list color.diff.
Is this a bug in the diff_get_color{,_opt}() tooling?
It's certainly not anything you've introduced here, of
course. I just noticed that some custom color.diff settings
I've used weren't picked up by branch-diff, despite your
clear intention to respect colors from the config.
--
Todd
~~
Abandon the search for Truth; settle for a good fantasy.
