Re: [PATCH RFC 2/2] diff: teach diff to expand tabs in output

[Junio C Hamano]

Jeff King  writes:

> That also means an option to something like "expand" is tough, because
> "first character" really means "first non-ANSI-code character".

That is true, but once you commit to the mindset that you are
extending "expand", then that becomes a mere part of what must be
done, i.e. if you want your "expand" to be able to handle coloured
input, you'd need to know how wide each segment of the input is.
For that matter, you would also want your "expand" to pay attention
to the differences between display- vs byte-widths of a string,
perhaps reusing utf8_strwidth() or something.

Also "the first character is special" does not have to be a "diff
specific hack".  Your extended "expand" can take a list of
tab-widths and the special case for highlighting diff output happens
to take 9,8 as the "list of tab-widths" parameter (whose semantics
is that each number in the list tells how wide the tab is, and the
last number repeats forever, so 9,8 really means 9,8,8,8,8,...).

Re: [PATCH RFC 2/2] diff: teach diff to expand tabs in output

[Jeff King]

On Tue, Mar 28, 2017 at 01:05:40PM -0700, Jacob Keller wrote:

> On Tue, Mar 28, 2017 at 12:03 PM, Junio C Hamano  wrote:
> > Jacob Keller  writes:
> >
> >> I'm really not a fan of how the ws code ended up. It seems pretty ugly
> >> and weird to hack in the expand_tabs stuff here. However, I'm really not
> >> sure how else I could handle this. Additionally, I'm not 100% sure
> >> whether this interacts with format-patch or other machinery which may
> >> well want some way to be excluded. Thoughts?
> >
> > As long as you do the same as "do we color the output?  no, no, we
> > are format-patch and must not color" logic to refrain from expanding
> > the tabs, you should be OK.
> >
> >> I think there also may be some wonky bits when performing the tab
> >> expansion during whitespace checks, due to the way we expand, because I
> >> don't think that the tabexpand function takes into account the "current"
> >> location when adding a string, so it very well may not be correct I
> >> am unsure if there is a good way to fix this.
> >
> > This "feature" is limited to the diff output, so one way may be to
> > leave the code as-is and pipe the output to a filter that is similar
> > to /usr/bin/expand but knows that the first column is special (this
> > is the part that "this is limited to diff" kicks in).  You may even
> > be able to implement it as a new option to "expand(1)" and then
> > people who aren't Git users would also benefit.
> >
> 
> That makes more sense. I'll take a look at that. It might even be
> possible to modify the pager setup so that it does that as part of its
> process.

This is similar to how I use diff-highlight, which is basically:

  [pager]
  log = diff-highlight | less
  show = diff-highlight | less
  diff = diff-highlight | less

I've wanted to move diff-highlight inside git, but ran into ugly
conflicts with the whitespace-marking code as well. Something like:

  git show b16a991c1be5681b4b673d4343dfcc0c2f5ad498 |
  perl -pe 's/^(.)(\t+)/$1 . (" " x (length($2) * 8))/e'

But sticking it in your pager pipeline is tough, because you actually
need to skip over the color bytes when they are present. You can see in
diff-highlight how this is handled.

That also means an option to something like "expand" is tough, because
"first character" really means "first non-ANSI-code character".

-Peff

Re: [PATCH RFC 2/2] diff: teach diff to expand tabs in output

[Jacob Keller]

On Tue, Mar 28, 2017 at 12:03 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> I'm really not a fan of how the ws code ended up. It seems pretty ugly
>> and weird to hack in the expand_tabs stuff here. However, I'm really not
>> sure how else I could handle this. Additionally, I'm not 100% sure
>> whether this interacts with format-patch or other machinery which may
>> well want some way to be excluded. Thoughts?
>
> As long as you do the same as "do we color the output?  no, no, we
> are format-patch and must not color" logic to refrain from expanding
> the tabs, you should be OK.
>
>> I think there also may be some wonky bits when performing the tab
>> expansion during whitespace checks, due to the way we expand, because I
>> don't think that the tabexpand function takes into account the "current"
>> location when adding a string, so it very well may not be correct I
>> am unsure if there is a good way to fix this.
>
> This "feature" is limited to the diff output, so one way may be to
> leave the code as-is and pipe the output to a filter that is similar
> to /usr/bin/expand but knows that the first column is special (this
> is the part that "this is limited to diff" kicks in).  You may even
> be able to implement it as a new option to "expand(1)" and then
> people who aren't Git users would also benefit.
>

That makes more sense. I'll take a look at that. It might even be
possible to modify the pager setup so that it does that as part of its
process.

Thanks,
Jake

Re: [PATCH RFC 2/2] diff: teach diff to expand tabs in output

[Junio C Hamano]

Jacob Keller  writes:

> I'm really not a fan of how the ws code ended up. It seems pretty ugly
> and weird to hack in the expand_tabs stuff here. However, I'm really not
> sure how else I could handle this. Additionally, I'm not 100% sure
> whether this interacts with format-patch or other machinery which may
> well want some way to be excluded. Thoughts?

As long as you do the same as "do we color the output?  no, no, we
are format-patch and must not color" logic to refrain from expanding
the tabs, you should be OK.

> I think there also may be some wonky bits when performing the tab
> expansion during whitespace checks, due to the way we expand, because I
> don't think that the tabexpand function takes into account the "current"
> location when adding a string, so it very well may not be correct I
> am unsure if there is a good way to fix this.

This "feature" is limited to the diff output, so one way may be to
leave the code as-is and pipe the output to a filter that is similar
to /usr/bin/expand but knows that the first column is special (this
is the part that "this is limited to diff" kicks in).  You may even
be able to implement it as a new option to "expand(1)" and then
people who aren't Git users would also benefit.