[v3,1/7] merge-ort-wrappers: make printed message match the one from recursive

Commit Message

Elijah Newren July 21, 2022, 8:16 a.m. UTC

From: Elijah Newren <[e-mail]>

When the index does not match HEAD, the merge strategies are responsible
to detect that condition and abort.  The merge-ort-wrappers had code to
implement this and meant to copy the error message from merge-recursive
but deviated in two ways, both due to the message in merge-recursive
being processed by another function that made additional changes:
  * It added an implicit "error: " prefix
  * It added an implicit trailing newline

Add these things, but do so in a couple extra steps to avoid having
translators need to translate another not-quite-identical string.

Signed-off-by: Elijah Newren <[e-mail]>
---
 merge-ort-wrappers.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Junio C Hamano July 21, 2022, 3:47 p.m. UTC | #1

"Elijah Newren via GitGitGadget" <[e-mail]> writes:

>  	if (head && repo_index_has_changes(opt->repo, head, &sb)) {
> -		fprintf(stderr, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
> +		struct strbuf err = STRBUF_INIT;
> +		strbuf_addstr(&err, "error: ");
> +		strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
>  		    sb.buf);
> +		strbuf_addch(&err, '\n');
> +		fputs(err.buf, stderr);
> +		strbuf_release(&err);

Makes me wonder why this is not a mere

	error(_("Your local chagnes ... by merge:\n  %s"), sb.buf);

that reuses the exact string.  The err() function in merge-recursive.c 
is strangely complex (and probably buggy---if it is not buffering
output, it adds "error: " prefix to opt->obuf before calling vaddf
to add the message, and then sends that to error() to give it
another "error: " prefix), but all the above does is to send a
message to standard error stream.

>  		strbuf_release(&sb);
>  		return -1;
>  	}

Elijah Newren July 21, 2022, 7:51 p.m. UTC | #2

On Thu, Jul 21, 2022 at 8:47 AM Junio C Hamano <[e-mail]> wrote:
>
> "Elijah Newren via GitGitGadget" <[e-mail]> writes:
>
> >       if (head && repo_index_has_changes(opt->repo, head, &sb)) {
> > -             fprintf(stderr, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
> > +             struct strbuf err = STRBUF_INIT;
> > +             strbuf_addstr(&err, "error: ");
> > +             strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
> >                   sb.buf);
> > +             strbuf_addch(&err, '\n');
> > +             fputs(err.buf, stderr);
> > +             strbuf_release(&err);
>
> Makes me wonder why this is not a mere
>
>         error(_("Your local chagnes ... by merge:\n  %s"), sb.buf);
>
> that reuses the exact string.  The err() function in merge-recursive.c
> is strangely complex (and probably buggy---if it is not buffering
> output, it adds "error: " prefix to opt->obuf before calling vaddf
> to add the message, and then sends that to error() to give it
> another "error: " prefix), but all the above does is to send a
> message to standard error stream.

Ah, that would be nicer; thanks for the pointer.  I would still need
to prefix it with an
    strbuf_addch(&sb, '\n');
but two lines certainly beats six.

>
> >               strbuf_release(&sb);
> >               return -1;
> >       }

Junio C Hamano July 21, 2022, 8:05 p.m. UTC | #3

Elijah Newren <[e-mail]> writes:

> On Thu, Jul 21, 2022 at 8:47 AM Junio C Hamano <[e-mail]> wrote:
>>
>> "Elijah Newren via GitGitGadget" <[e-mail]> writes:
>>
>> >       if (head && repo_index_has_changes(opt->repo, head, &sb)) {
>> > -             fprintf(stderr, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
>> > +             struct strbuf err = STRBUF_INIT;
>> > +             strbuf_addstr(&err, "error: ");
>> > +             strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
>> >                   sb.buf);
>> > +             strbuf_addch(&err, '\n');
>> > +             fputs(err.buf, stderr);
>> > +             strbuf_release(&err);
>>
>> Makes me wonder why this is not a mere
>>
>>         error(_("Your local chagnes ... by merge:\n  %s"), sb.buf);
>>
>> that reuses the exact string.  The err() function in merge-recursive.c
>> is strangely complex (and probably buggy---if it is not buffering
>> output, it adds "error: " prefix to opt->obuf before calling vaddf
>> to add the message, and then sends that to error() to give it
>> another "error: " prefix), but all the above does is to send a
>> message to standard error stream.
>
> Ah, that would be nicer; thanks for the pointer.  I would still need
> to prefix it with an
>     strbuf_addch(&sb, '\n');
> but two lines certainly beats six.

Your "strbuf" version uses the same format string as my error()
thing and then manually add one LF at the end, before sending it to
fputs(), which, unlike puts() does not add any extra LF at the end.

error() gives a terminating newline at the end.

Do you still need to add one more?

Elijah Newren July 21, 2022, 9:14 p.m. UTC | #4

On Thu, Jul 21, 2022 at 1:05 PM Junio C Hamano <[e-mail]> wrote:
>
> Elijah Newren <[e-mail]> writes:
>
> > On Thu, Jul 21, 2022 at 8:47 AM Junio C Hamano <[e-mail]> wrote:
> >>
> >> "Elijah Newren via GitGitGadget" <[e-mail]> writes:
> >>
> >> >       if (head && repo_index_has_changes(opt->repo, head, &sb)) {
> >> > -             fprintf(stderr, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
> >> > +             struct strbuf err = STRBUF_INIT;
> >> > +             strbuf_addstr(&err, "error: ");
> >> > +             strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
> >> >                   sb.buf);
> >> > +             strbuf_addch(&err, '\n');
> >> > +             fputs(err.buf, stderr);
> >> > +             strbuf_release(&err);
> >>
> >> Makes me wonder why this is not a mere
> >>
> >>         error(_("Your local chagnes ... by merge:\n  %s"), sb.buf);
> >>
> >> that reuses the exact string.  The err() function in merge-recursive.c
> >> is strangely complex (and probably buggy---if it is not buffering
> >> output, it adds "error: " prefix to opt->obuf before calling vaddf
> >> to add the message, and then sends that to error() to give it
> >> another "error: " prefix), but all the above does is to send a
> >> message to standard error stream.
> >
> > Ah, that would be nicer; thanks for the pointer.  I would still need
> > to prefix it with an
> >     strbuf_addch(&sb, '\n');
> > but two lines certainly beats six.
>
> Your "strbuf" version uses the same format string as my error()
> thing and then manually add one LF at the end, before sending it to
> fputs(), which, unlike puts() does not add any extra LF at the end.
>
> error() gives a terminating newline at the end.
>
> Do you still need to add one more?

Ah, sorry, my mistake.  I somehow thought error() just added the
"error: " prefix.  So, indeed, this is just a change from
fprintf(stderr, ...) to error(...).  No second newline needed.

Thanks!

Patch

diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c
index ad041061695..d2c416bb5c0 100644
--- a/merge-ort-wrappers.c
+++ b/merge-ort-wrappers.c
@@ -10,8 +10,13 @@  static int unclean(struct merge_options *opt, struct tree *head)
 	struct strbuf sb = STRBUF_INIT;
 
 	if (head && repo_index_has_changes(opt->repo, head, &sb)) {
-		fprintf(stderr, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
+		struct strbuf err = STRBUF_INIT;
+		strbuf_addstr(&err, "error: ");
+		strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
 		    sb.buf);
+		strbuf_addch(&err, '\n');
+		fputs(err.buf, stderr);
+		strbuf_release(&err);
 		strbuf_release(&sb);
 		return -1;
 	}