Re: [PATCH v3 0/6] use the pager in 'add -p'

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 10, 2024, 14:09 UTC
Message-ID: <9f1884ae-0f9f-4d9f-a262-b6929b81d7d8@gmail.com>
In-Reply-To: <219a195c-74d0-4c21-bf54-0752bb5b01df@gmail.com>

Hi Rubén

On 09/06/2024 08:44, Rubén Justo wrote:

Show 26 quoted lines

> On Sat, Jun 08, 2024 at 07:54:34AM +0200, Dragan Simic wrote:
> 
>> When "|xyz" is used instead, the version of the hunk with no coloring
>> escape sequences should be piped to xyz.
> 
> That is a sane and conservative approach, and I'm not opposed.  However,
> giving the colorful version though a custom pager is a good thing to
> have, I think, i.e: allowing a simple "head" without losing the
> coloring.
> 
> Let's recap a bit.
> 
> Initially, this series aimed to enable sending chunks to the pager
> during "add -p" sessions.
> 
> To reduce the blast radius of spawning a pager for each chunk, we
> introduced a new command "P".
> 
> Junio suggested opening up the command to allow specifying a custom
> pager, in the form of "P<program>".
> 
> The "P" command started to resemble a lot to the common pipe operator.
> Thus, we shifted to "|<program>".
> 
> Some concerns were raised about controlling when to send coloring escape
> sequences.

I'm still not really convinced that the escape sequences are a problem. As Peff has pointed out [1] this new command exists primarily to display the current hunk. I've asked for concrete examples of programs that it would be useful to run from "git add -p" where the escape codes are a problem [2,3]. Sadly the replies talked in generic terms about an imaginary program without any reference to displaying or processing a hunk from "git add -p". Without a clear use case for stripping the escape codes I think we should add a single command that pipes the colored output to a user specified program. We can make it clear in the documentation that the input to the user's command will contain escape sequences unless they pass "-c color.diff=false" when starting "git add -p". If it becomes clear that there is a use for the plain output we can add that at a later stage.

Best Wishes

Phillip

[1] https://lore.kernel.org/git/20240605090935.GF2345232@coredump.intra.peff.net [2] https://lore.kernel.org/git/a2a59f5e-fd55-41d3-8472-b99256e1f428@gmail.com> [3] https://lore.kernel.org/git/1ae0715d-df76-4019-995e-f00f3506f2ac@gmail.com

Show 13 quoted lines

> Several ideas were discussed to address this, including
> introducing a new command ">", a modifier for "|": "||", and others.
> Alternatively, we could leave it up to the user to filter as needed.
> Or, simply, do not send escape codes at all.
> 
> So, looking back at the ideas discussed in the thread, perhaps a
> reasonable next step might be to reintroduce the 'P<program>' command
> and let '|<program>' be the way to send raw, uncolored, chunks.
> 
> This approach makes sense to me.  I'll wait a bit before sending a
> reroll to gather feedback, though.
> 
> Thanks.