diff mbox series

[v2,07/11] add -p (built-in): do not color the progress indicator separately

Message ID 73b6d60a801766605cde9403d7a9e8451ef99e11.1605097704.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 6681e3603216f0e279fea467a0e6d829fffb82a5
Headers show
Series Fix color handling in git add -i | expand

Commit Message

Johannes Schindelin Nov. 11, 2020, 12:28 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

The Perl version of this command colors the progress indicator and the
prompt message in one go, let's do the same in the built-in version.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Phillip Wood Nov. 13, 2020, 12:07 p.m. UTC | #1 
Hi Dscho

On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> The Perl version of this command colors the progress indicator and the
> prompt message in one go, let's do the same in the built-in version.

Why? the C version has access to an api that means we don't have to 
remember to print the reset string each time so why move away from that? 
I don't think it matters to the user that there are some extra escape 
codes in the prompt of the C version. The answer is probably "so we can 
use the same test as the perl version" which might be a good reason - if 
it is I think it would be helpful to say so in the commit message.

Best Wishes

Phillip

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   add-patch.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index bf89c43145..2fad92ca37 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1461,15 +1461,15 @@ static int patch_update_file(struct add_p_state *s,
>   		else
>   			prompt_mode_type = PROMPT_HUNK;
>   
> -		color_fprintf(stdout, s->s.prompt_color,
> -			      "(%"PRIuMAX"/%"PRIuMAX") ",
> +		printf("%s(%"PRIuMAX"/%"PRIuMAX") ", s->s.prompt_color,
>   			      (uintmax_t)hunk_index + 1,
>   			      (uintmax_t)(file_diff->hunk_nr
>   						? file_diff->hunk_nr
>   						: 1));
> -		color_fprintf(stdout, s->s.prompt_color,
> -			      _(s->mode->prompt_mode[prompt_mode_type]),
> -			      s->buf.buf);
> +		printf(_(s->mode->prompt_mode[prompt_mode_type]),
> +		       s->buf.buf);
> +		if (*s->s.reset_color)
> +			fputs(s->s.reset_color, stdout);
>   		fflush(stdout);
>   		if (read_single_character(s) == EOF)
>   			break;
>
Johannes Schindelin Nov. 13, 2020, 1:57 p.m. UTC | #2 
Hi Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

> On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The Perl version of this command colors the progress indicator and the
> > prompt message in one go, let's do the same in the built-in version.
>
> Why? the C version has access to an api that means we don't have to remember
> to print the reset string each time so why move away from that? I don't think
> it matters to the user that there are some extra escape codes in the prompt of
> the C version. The answer is probably "so we can use the same test as the perl
> version" which might be a good reason - if it is I think it would be helpful
> to say so in the commit message.

Honestly, the number one reason is so that the _same_ test passes using
the Perl version as well as with the built-in version, something that is
required by the `linux-clang` job in our CI build.

I am not really willing to change this, unless I hear a goooood reason to
complicate the test.

Ciao,
Dscho
Phillip Wood Nov. 13, 2020, 4:06 p.m. UTC | #3 
Hi Dscho

On 13/11/2020 13:57, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Fri, 13 Nov 2020, Phillip Wood wrote:
> 
>> On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:
>>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>>
>>> The Perl version of this command colors the progress indicator and the
>>> prompt message in one go, let's do the same in the built-in version.
>>
>> Why? the C version has access to an api that means we don't have to remember
>> to print the reset string each time so why move away from that? I don't think
>> it matters to the user that there are some extra escape codes in the prompt of
>> the C version. The answer is probably "so we can use the same test as the perl
>> version" which might be a good reason - if it is I think it would be helpful
>> to say so in the commit message.
> 
> Honestly, the number one reason is so that the _same_ test passes using
> the Perl version as well as with the built-in version, something that is
> required by the `linux-clang` job in our CI build.

That's what I assumed - it would be good to document that in the commit 
message.

> I am not really willing to change this, unless I hear a goooood reason to
> complicate the test.

I guess we could change the perl version to match the C version as it is 
the perl version that will be retired but I'm not that fussed

Best Wishes

Phillip


> Ciao,
> Dscho
>
Johannes Schindelin Nov. 15, 2020, 11:55 p.m. UTC | #4 
Hi Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

> On 13/11/2020 13:57, Johannes Schindelin wrote:
> >
> > On Fri, 13 Nov 2020, Phillip Wood wrote:
> >
> > > On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:
> > > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > >
> > > > The Perl version of this command colors the progress indicator and the
> > > > prompt message in one go, let's do the same in the built-in version.
> > >
> > > Why? the C version has access to an api that means we don't have to
> > > remember to print the reset string each time so why move away from
> > > that? I don't think it matters to the user that there are some extra
> > > escape codes in the prompt of the C version. The answer is probably
> > > "so we can use the same test as the perl version" which might be a
> > > good reason - if it is I think it would be helpful to say so in the
> > > commit message.
> >
> > Honestly, the number one reason is so that the _same_ test passes using
> > the Perl version as well as with the built-in version, something that is
> > required by the `linux-clang` job in our CI build.
>
> That's what I assumed - it would be good to document that in the commit
> message.

I did that:

    add -p (built-in): do not color the progress indicator separately

    The Perl version of this command colors the progress indicator and the
    prompt message in one go.

    Let's do the same in the built-in version so that the same upcoming test
    (which will compare the output of `git add -p` against a known-good
    version) will pass both for the Perl version as well as for the built-in
    version.

> > I am not really willing to change this, unless I hear a goooood reason to
> > complicate the test.
>
> I guess we could change the perl version to match the C version as it is the
> perl version that will be retired but I'm not that fussed

Well, since we want to supersede the Perl version by the built-in version,
the latter needs to imitate the former, no?

Ciao,
Dscho
diff mbox series

Patch

diff --git a/add-patch.c b/add-patch.c
index bf89c43145..2fad92ca37 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1461,15 +1461,15 @@  static int patch_update_file(struct add_p_state *s,
 		else
 			prompt_mode_type = PROMPT_HUNK;
 
-		color_fprintf(stdout, s->s.prompt_color,
-			      "(%"PRIuMAX"/%"PRIuMAX") ",
+		printf("%s(%"PRIuMAX"/%"PRIuMAX") ", s->s.prompt_color,
 			      (uintmax_t)hunk_index + 1,
 			      (uintmax_t)(file_diff->hunk_nr
 						? file_diff->hunk_nr
 						: 1));
-		color_fprintf(stdout, s->s.prompt_color,
-			      _(s->mode->prompt_mode[prompt_mode_type]),
-			      s->buf.buf);
+		printf(_(s->mode->prompt_mode[prompt_mode_type]),
+		       s->buf.buf);
+		if (*s->s.reset_color)
+			fputs(s->s.reset_color, stdout);
 		fflush(stdout);
 		if (read_single_character(s) == EOF)
 			break;