Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr

From: Rubén Justo <rjusto@gmail.com>
Date: May 1, 2024, 21:13 UTC
Message-ID: <0dc1f9f9-02ec-4394-9c25-7a7fee6147ee@gmail.com>
In-Reply-To: <xmqq5xvx5x1u.fsf@gitster.g>

On Wed, May 01, 2024 at 09:14:37AM -0700, Junio C Hamano wrote:

Show 29 quoted lines

> phillip.wood123@gmail.com writes:
> 
> >>>> -		fprintf(stderr, _("No changes.\n"));
> >>>> +		err(&s, _("No changes."));
> >>>>    	else if (binary_count == s.file_diff_nr)
> >>>> -		fprintf(stderr, _("Only binary files changed.\n"));
> >>>> +		err(&s, _("Only binary files changed."));
> >>>
> >>> These two mean we'll now color these messages which we didn't do before. I
> > ...
> > I think so
> > ...
> >> -		err(&s, _("Only binary files changed."));
> >> +		error(_("only binary files changed"));
> >>     	add_p_state_clear(&s);
> >>   	return 0;
> >> Or, simply leave them untouched in this series.
> >
> > Either option sounds good to me
> 
> We are returning "success" to the caller, so using error() here is a
> bit strong.  Judging from how other messages emitted with err() in
> this program is meant to help the end user, they are all to tell the
> user why their input caused no actual change, and showing these two
> messages the same way as these other messages would be the best for
> consistency.
> 
> So I'm inclined to say that what was posted is good.  If it paints
> these two messages in the same color as others, that is even getter.

Having:

    $ printf "\0" >binary
    $ git add -N binary

Displaying the message in RED does not seem as a bad change:

    $ git add -i
               staged     unstaged path
      1:        +0/-0       binary binary
    
    *** Commands ***
      1: status       2: update       3: revert       4: add untracked
      5: patch        6: diff         7: quit         8: help
    What now> p
    <RED>Only binary files changed.<RED>

However, here could be disturbing:

    $ git add -p
    <RED>Only binary files changed.<RED>

And the "No changes." message is going to have even more audience, I think.

Perhaps this seems sensible:

Range-diff against v5:
2:  b0f042f4ff ! 1:  94c3f80041 add-patch: do not show UI messages on stderr
    @@ add-patch.c: int run_add_p(struct repository *r, enum add_p_mode mode,

        if (s.file_diff_nr == 0)
     -          fprintf(stderr, _("No changes.\n"));
    -+          err(&s, _("No changes."));
    ++          fprintf(stdout, _("No changes.\n"));
        else if (binary_count == s.file_diff_nr)
     -          fprintf(stderr, _("Only binary files changed.\n"));
    -+          err(&s, _("Only binary files changed."));
    ++          fprintf(stdout, _("Only binary files changed.\n"));

        add_p_state_clear(&s);
        return 0;
1:  4a0eb1337f = 2:  abaf904e8c add-patch: response to unknown command

But I'm not sure and I do not want to send a new iteration unless it is necessary. I was already happy with previous versions ;-).