diff mbox series

[4/7] merge-ort: capture and print ll-merge warnings in our preferred fashion

Message ID ce95b82fc492d48fa6022df424f9a303a1c70ad4.1630376800.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Add a new --remerge-diff capability to show & log | expand

Commit Message

Elijah Newren Aug. 31, 2021, 2:26 a.m. UTC 
From: Elijah Newren <newren@gmail.com>

Instead of immediately printing ll-merge warnings to stderr, we save
them in our output strbuf.  Besides allowing us to move these warnings
to a special file for --remerge-diff, this has two other benefits for
regular merges done by merge-ort:

  * The deferral of messages ensures we can print all messages about
    any given path together (merge-recursive was known to sometimes
    intersperse messages about other paths, particularly when renames
    were involved).

  * The deferral of messages means we can avoid printing spurious
    conflict messages when we just end up aborting due to local user
    modifications in the way.  (In contrast to merge-recursive.c which
    prematurely checks for local modifications in the way via
    unpack_trees() and gets the check wrong both in terms of false
    positives and false negatives relative to renames, merge-ort does
    not perform the local modifications in the way check until the
    checkout() step after the full merge has been computed.)

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-ort.c                | 11 ++++++++---
 t/t6404-recursive-merge.sh | 10 ++++++++--
 t/t6406-merge-attr.sh      | 10 ++++++++--
 3 files changed, 24 insertions(+), 7 deletions(-)

Comments

Jeff King Sept. 28, 2021, 10:39 p.m. UTC | #1 
On Tue, Aug 31, 2021 at 02:26:37AM +0000, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
> 
> Instead of immediately printing ll-merge warnings to stderr, we save
> them in our output strbuf.  Besides allowing us to move these warnings
> to a special file for --remerge-diff, this has two other benefits for
> regular merges done by merge-ort:
> 
>   * The deferral of messages ensures we can print all messages about
>     any given path together (merge-recursive was known to sometimes
>     intersperse messages about other paths, particularly when renames
>     were involved).
> 
>   * The deferral of messages means we can avoid printing spurious
>     conflict messages when we just end up aborting due to local user
>     modifications in the way.  (In contrast to merge-recursive.c which
>     prematurely checks for local modifications in the way via
>     unpack_trees() and gets the check wrong both in terms of false
>     positives and false negatives relative to renames, merge-ort does
>     not perform the local modifications in the way check until the
>     checkout() step after the full merge has been computed.)

Yeah, this is a good example of why having ll_merge() print warnings in
the first place is probably wrong. If you buy my argument that
ll_merge() should be returning an enum, then this code becomes IMHO even
nicer, as you can generate your own sensible message in the caller.

-Peff
Ævar Arnfjörð Bjarmason Sept. 30, 2021, 4:53 p.m. UTC | #2 
On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:

> @@ -108,8 +108,14 @@ test_expect_success 'refuse to merge binary files' '
>  	printf "\0\0" >binary-file &&
>  	git add binary-file &&
>  	git commit -m binary2 &&
> -	test_must_fail git merge F >merge.out 2>merge.err &&
> -	grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
> +	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
> +	then
> +		test_must_fail git merge F >merge.out &&
> +		grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.out
> +	else
> +		test_must_fail git merge F >merge.out 2>merge.err &&
> +		grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
> +	fi
>  '

To save readers from eyeballing if a single character has changed here,
which I don't think it has, just do:

if ...
then
	cmd >actual
else
	other cmd >actual
fi &&
grep [...]

I.e. no need to duplicate the "grep" here just because of merge.out v.s. merge.err.

[...]

> -	test_must_fail git merge bin-main 2>stderr &&
> -	grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
> +	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
> +	then
> +		test_must_fail git merge bin-main >stdout &&
> +		grep -i "warning.*cannot merge.*HEAD vs. bin-main" stdout
> +	else
> +		test_must_fail git merge bin-main 2>stderr &&
> +		grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
> +	fi


ditto.
Elijah Newren Oct. 1, 2021, 1:54 a.m. UTC | #3 
On Thu, Sep 30, 2021 at 9:53 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:
>
> > @@ -108,8 +108,14 @@ test_expect_success 'refuse to merge binary files' '
> >       printf "\0\0" >binary-file &&
> >       git add binary-file &&
> >       git commit -m binary2 &&
> > -     test_must_fail git merge F >merge.out 2>merge.err &&
> > -     grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
> > +     if test "$GIT_TEST_MERGE_ALGORITHM" = ort
> > +     then
> > +             test_must_fail git merge F >merge.out &&
> > +             grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.out
> > +     else
> > +             test_must_fail git merge F >merge.out 2>merge.err &&
> > +             grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
> > +     fi
> >  '
>
> To save readers from eyeballing if a single character has changed here,
> which I don't think it has, just do:
>
> if ...
> then
>         cmd >actual
> else
>         other cmd >actual

Do you mean
   other cmd >/dev/null 2>actual
?


> fi &&
> grep [...]
>
> I.e. no need to duplicate the "grep" here just because of merge.out v.s. merge.err.
>
> [...]
>
> > -     test_must_fail git merge bin-main 2>stderr &&
> > -     grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
> > +     if test "$GIT_TEST_MERGE_ALGORITHM" = ort
> > +     then
> > +             test_must_fail git merge bin-main >stdout &&
> > +             grep -i "warning.*cannot merge.*HEAD vs. bin-main" stdout
> > +     else
> > +             test_must_fail git merge bin-main 2>stderr &&
> > +             grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
> > +     fi
>
>
> ditto.
Ævar Arnfjörð Bjarmason Oct. 1, 2021, 7:23 a.m. UTC | #4 
On Thu, Sep 30 2021, Elijah Newren wrote:

> On Thu, Sep 30, 2021 at 9:53 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:
>>
>> > @@ -108,8 +108,14 @@ test_expect_success 'refuse to merge binary files' '
>> >       printf "\0\0" >binary-file &&
>> >       git add binary-file &&
>> >       git commit -m binary2 &&
>> > -     test_must_fail git merge F >merge.out 2>merge.err &&
>> > -     grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
>> > +     if test "$GIT_TEST_MERGE_ALGORITHM" = ort
>> > +     then
>> > +             test_must_fail git merge F >merge.out &&
>> > +             grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.out
>> > +     else
>> > +             test_must_fail git merge F >merge.out 2>merge.err &&
>> > +             grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
>> > +     fi
>> >  '
>>
>> To save readers from eyeballing if a single character has changed here,
>> which I don't think it has, just do:
>>
>> if ...
>> then
>>         cmd >actual
>> else
>>         other cmd >actual
>
> Do you mean
>    other cmd >/dev/null 2>actual
> ?

Yes, sorry. I was going for lazy pseudocode. I meant maybe the "grep"
can be shared between the two.
diff mbox series

Patch

diff --git a/merge-ort.c b/merge-ort.c
index a9e69d9cbb0..831f8f3e049 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1741,6 +1741,7 @@  static int merge_3way(struct merge_options *opt,
 	struct ll_merge_options ll_opts = {0};
 	char *base, *name1, *name2;
 	int merge_status;
+	struct strbuf warnings = STRBUF_INIT;
 
 	if (!opt->priv->attr_index.initialized)
 		initialize_attr_index(opt);
@@ -1781,10 +1782,14 @@  static int merge_3way(struct merge_options *opt,
 	read_mmblob(&src1, a);
 	read_mmblob(&src2, b);
 
-	merge_status = ll_merge(result_buf, path, &orig, base,
-				&src1, name1, &src2, name2,
-				&opt->priv->attr_index, &ll_opts);
+	merge_status = ll_merge_with_warnings(result_buf, &warnings, path,
+					      &orig, base,
+					      &src1, name1, &src2, name2,
+					      &opt->priv->attr_index, &ll_opts);
+	if (warnings.len > 0)
+		path_msg(opt, path, 0, "%s", warnings.buf);
 
+	strbuf_release(&warnings);
 	free(base);
 	free(name1);
 	free(name2);
diff --git a/t/t6404-recursive-merge.sh b/t/t6404-recursive-merge.sh
index eaf48e941e2..a986eec0420 100755
--- a/t/t6404-recursive-merge.sh
+++ b/t/t6404-recursive-merge.sh
@@ -108,8 +108,14 @@  test_expect_success 'refuse to merge binary files' '
 	printf "\0\0" >binary-file &&
 	git add binary-file &&
 	git commit -m binary2 &&
-	test_must_fail git merge F >merge.out 2>merge.err &&
-	grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
+	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
+	then
+		test_must_fail git merge F >merge.out &&
+		grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.out
+	else
+		test_must_fail git merge F >merge.out 2>merge.err &&
+		grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
+	fi
 '
 
 test_expect_success 'mark rename/delete as unmerged' '
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 84946458371..75a7994988a 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -221,8 +221,14 @@  test_expect_success 'binary files with union attribute' '
 	printf "two\0" >bin.txt &&
 	git commit -am two &&
 
-	test_must_fail git merge bin-main 2>stderr &&
-	grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
+	if test "$GIT_TEST_MERGE_ALGORITHM" = ort
+	then
+		test_must_fail git merge bin-main >stdout &&
+		grep -i "warning.*cannot merge.*HEAD vs. bin-main" stdout
+	else
+		test_must_fail git merge bin-main 2>stderr &&
+		grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
+	fi
 '
 
 test_done