[PATCH 0/4] merge-ort unused parameter cleanups

[PATCH 0/4] merge-ort unused parameter cleanups

[Jeff King]

A few small cleanups for merge-ort collected from playing with
-Wunused-parameter. The first one actually fixes a user-visible bug, and
the rest are just cleanups.

  [1/4]: merge-ort: drop custom err() function
  [2/4]: merge-ort: stop passing "opt" to read_oid_strbuf()
  [3/4]: merge-ort: drop unused parameters from detect_and_process_renames()
  [4/4]: merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()

 merge-ort.c           | 48 +++++++++++--------------------------------
 t/t6406-merge-attr.sh |  3 ++-
 2 files changed, 14 insertions(+), 37 deletions(-)

-Peff

[PATCH 1/4] merge-ort: drop custom err() function

[Jeff King]

The merge-ort code has an err() function, but it's really just error()
in disguise. It differs in two ways:

  1. It takes a "struct merge_options" argument. But the function
     completely ignores it! We can simply remove it.

  2. It formats the error string into a strbuf, prepending "error: ",
     and then feeds the result into error(). But this is wrong! The
     error() function already adds the prefix, so we end up with:

        error: error: Failed to execute internal merge

So let's just drop this function entirely and call error() directly, as
the functions are otherwise identical (note that they both always return
-1).

Presumably nobody noticed the bogus messages because they are quite hard
to trigger (they are mostly internal errors reading and writing
objects). However, one easy trigger is a custom merge driver which dies
by signal; we have a test already here, but we were not checking the
contents of stderr.

Signed-off-by: Jeff King <peff@peff.net>
---
A few of these messages starts with capital letters, which is unlike our
usual error message style. I didn't clean that up here. We could do so
on top, but I actually wonder if some of these ought to be using
path_msg() and continuing instead, to give output closer to other
conflict or error cases (e.g., conflicts caused by missing submodule
objects). But I dunno. I guess these are all more clearly "woah,
something is totally wrong" that we do not expect to happen, so it
probably isn't a big deal to just abort.

 merge-ort.c           | 28 +++++-----------------------
 t/t6406-merge-attr.sh |  3 ++-
 2 files changed, 7 insertions(+), 24 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 8631c99700..027ecc7f78 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -721,23 +721,6 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
 	renames->callback_data_nr = renames->callback_data_alloc = 0;
 }
 
-__attribute__((format (printf, 2, 3)))
-static int err(struct merge_options *opt, const char *err, ...)
-{
-	va_list params;
-	struct strbuf sb = STRBUF_INIT;
-
-	strbuf_addstr(&sb, "error: ");
-	va_start(params, err);
-	strbuf_vaddf(&sb, err, params);
-	va_end(params);
-
-	error("%s", sb.buf);
-	strbuf_release(&sb);
-
-	return -1;
-}
-
 static void format_commit(struct strbuf *sb,
 			  int indent,
 			  struct repository *repo,
@@ -2122,13 +2105,12 @@ static int handle_content_merge(struct merge_options *opt,
 					  &result_buf);
 
 		if ((merge_status < 0) || !result_buf.ptr)
-			ret = err(opt, _("Failed to execute internal merge"));
+			ret = error(_("Failed to execute internal merge"));
 
 		if (!ret &&
 		    write_object_file(result_buf.ptr, result_buf.size,
 				      OBJ_BLOB, &result->oid))
-			ret = err(opt, _("Unable to add %s to database"),
-				  path);
+			ret = error(_("Unable to add %s to database"), path);
 
 		free(result_buf.ptr);
 		if (ret)
@@ -3518,10 +3500,10 @@ static int read_oid_strbuf(struct merge_options *opt,
 	unsigned long size;
 	buf = repo_read_object_file(the_repository, oid, &type, &size);
 	if (!buf)
-		return err(opt, _("cannot read object %s"), oid_to_hex(oid));
+		return error(_("cannot read object %s"), oid_to_hex(oid));
 	if (type != OBJ_BLOB) {
 		free(buf);
-		return err(opt, _("object %s is not a blob"), oid_to_hex(oid));
+		return error(_("object %s is not a blob"), oid_to_hex(oid));
 	}
 	strbuf_attach(dst, buf, size, size + 1);
 	return 0;
@@ -4973,7 +4955,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
 		 * TRANSLATORS: The %s arguments are: 1) tree hash of a merge
 		 * base, and 2-3) the trees for the two trees we're merging.
 		 */
-		err(opt, _("collecting merge info failed for trees %s, %s, %s"),
+		error(_("collecting merge info failed for trees %s, %s, %s"),
 		    oid_to_hex(&merge_base->object.oid),
 		    oid_to_hex(&side1->object.oid),
 		    oid_to_hex(&side2->object.oid));
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 9677180a5b..05ad13b23e 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -179,7 +179,8 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
 
 	>./please-abort &&
 	echo "* merge=custom" >.gitattributes &&
-	test_must_fail git merge main &&
+	test_must_fail git merge main 2>err &&
+	grep "^error: Failed to execute internal merge" err &&
 	git ls-files -u >output &&
 	git diff --name-only HEAD >>output &&
 	test_must_be_empty output
-- 
2.42.0.628.g8a27295885

Re: [PATCH 1/4] merge-ort: drop custom err() function

[Elijah Newren]

On Thu, Sep 14, 2023 at 2:39 AM Jeff King <peff@peff.net> wrote:
>
> The merge-ort code has an err() function, but it's really just error()
> in disguise. It differs in two ways:
>
>   1. It takes a "struct merge_options" argument. But the function
>      completely ignores it! We can simply remove it.

Oops, when I simplified the err() function copied from
merge-recursive.c in one way, I failed to notice that it enabled
further simplifications.

>   2. It formats the error string into a strbuf, prepending "error: ",
>      and then feeds the result into error(). But this is wrong! The
>      error() function already adds the prefix, so we end up with:
>
>         error: error: Failed to execute internal merge

...and the same problem can be found in merge-recursive.c's err() function.

Not sure what current opinions on whether we should bother fixing
those up.  I do intend on nuking merge-recursive.c, but I obviously
haven't had much Git time this year.

> So let's just drop this function entirely and call error() directly, as
> the functions are otherwise identical (note that they both always return
> -1).
>
> Presumably nobody noticed the bogus messages because they are quite hard
> to trigger (they are mostly internal errors reading and writing
> objects). However, one easy trigger is a custom merge driver which dies
> by signal; we have a test already here, but we were not checking the
> contents of stderr.

Thanks for catching this.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> A few of these messages starts with capital letters, which is unlike our
> usual error message style. I didn't clean that up here. We could do so
> on top,

There are two of these.  In my defense, they were copied verbatim from
merge-recursive.c.  And I, um, never noticed the problem over there
before copying.  Or after.

> but I actually wonder if some of these ought to be using
> path_msg() and continuing instead, to give output closer to other
> conflict or error cases (e.g., conflicts caused by missing submodule
> objects). But I dunno. I guess these are all more clearly "woah,
> something is totally wrong" that we do not expect to happen, so it
> probably isn't a big deal to just abort.

Yeah, all callers of err()/error() are for things that should never
happen regardless of repository contents and should result in an
instant abort, whereas anything calling path_msg() is a conflict or
informational message that is expected for various kinds of repository
data -- these messages are accumulated and later shown.

Another distinction is that any call to path_msg() is associated to a
very specific path (or a few specific paths in special cases like
renames or add/add with conflict modes), whereas none of the calls to
err()/error() have a specific path they are about.  This serves a few
purposes:
  * We've had reports before that users get confused when there are
multiple conflict messages about a path and they do not occur
together.  The structure of the merge machinery is such that it often
has to process conflicts by type and then by path, rather than by path
and then by type.  If a merge has many conflicts, processing by type
and then by path, combined with printing as you go, naturally results
in cases where there are multiple conflict type messages for a single
path, but the messages are separated by dozens or hundreds of lines of
conflict messages about other paths.  By accumulating and printing
later, at print time we can sort based on path and provide nicer
output (though renames and such might still cause some separation of
related messages).
  * Accumulating and printing conflict & informational messages later
is also more friendly for use by other tools such as merge-tree or
rebase that may want to only conditionally print the messages or even
operate on the structured data (the specific paths and conflict types
recorded with them) in some special way.  Dscho and I talked about
that for his webby-merge-ui-for-github tool he was working on.

Anyway, long story short is that I think continuing to use error()
instead of path_msg() or something else makes sense here.  The capital
to lowercase cleanups make sense; we could even #leftoverbits for that
piece.


>  merge-ort.c           | 28 +++++-----------------------
>  t/t6406-merge-attr.sh |  3 ++-
>  2 files changed, 7 insertions(+), 24 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 8631c99700..027ecc7f78 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -721,23 +721,6 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
>         renames->callback_data_nr = renames->callback_data_alloc = 0;
>  }
>
> -__attribute__((format (printf, 2, 3)))
> -static int err(struct merge_options *opt, const char *err, ...)
> -{
> -       va_list params;
> -       struct strbuf sb = STRBUF_INIT;
> -
> -       strbuf_addstr(&sb, "error: ");
> -       va_start(params, err);
> -       strbuf_vaddf(&sb, err, params);
> -       va_end(params);
> -
> -       error("%s", sb.buf);
> -       strbuf_release(&sb);
> -
> -       return -1;
> -}
> -
>  static void format_commit(struct strbuf *sb,
>                           int indent,
>                           struct repository *repo,
> @@ -2122,13 +2105,12 @@ static int handle_content_merge(struct merge_options *opt,
>                                           &result_buf);
>
>                 if ((merge_status < 0) || !result_buf.ptr)
> -                       ret = err(opt, _("Failed to execute internal merge"));
> +                       ret = error(_("Failed to execute internal merge"));
>
>                 if (!ret &&
>                     write_object_file(result_buf.ptr, result_buf.size,
>                                       OBJ_BLOB, &result->oid))
> -                       ret = err(opt, _("Unable to add %s to database"),
> -                                 path);
> +                       ret = error(_("Unable to add %s to database"), path);
>
>                 free(result_buf.ptr);
>                 if (ret)
> @@ -3518,10 +3500,10 @@ static int read_oid_strbuf(struct merge_options *opt,
>         unsigned long size;
>         buf = repo_read_object_file(the_repository, oid, &type, &size);
>         if (!buf)
> -               return err(opt, _("cannot read object %s"), oid_to_hex(oid));
> +               return error(_("cannot read object %s"), oid_to_hex(oid));
>         if (type != OBJ_BLOB) {
>                 free(buf);
> -               return err(opt, _("object %s is not a blob"), oid_to_hex(oid));
> +               return error(_("object %s is not a blob"), oid_to_hex(oid));
>         }
>         strbuf_attach(dst, buf, size, size + 1);
>         return 0;
> @@ -4973,7 +4955,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
>                  * TRANSLATORS: The %s arguments are: 1) tree hash of a merge
>                  * base, and 2-3) the trees for the two trees we're merging.
>                  */
> -               err(opt, _("collecting merge info failed for trees %s, %s, %s"),
> +               error(_("collecting merge info failed for trees %s, %s, %s"),
>                     oid_to_hex(&merge_base->object.oid),
>                     oid_to_hex(&side1->object.oid),
>                     oid_to_hex(&side2->object.oid));
> diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
> index 9677180a5b..05ad13b23e 100755
> --- a/t/t6406-merge-attr.sh
> +++ b/t/t6406-merge-attr.sh
> @@ -179,7 +179,8 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
>
>         >./please-abort &&
>         echo "* merge=custom" >.gitattributes &&
> -       test_must_fail git merge main &&
> +       test_must_fail git merge main 2>err &&
> +       grep "^error: Failed to execute internal merge" err &&
>         git ls-files -u >output &&
>         git diff --name-only HEAD >>output &&
>         test_must_be_empty output
> --
> 2.42.0.628.g8a27295885

Thanks for fixing this up.

Re: [PATCH 1/4] merge-ort: drop custom err() function

[Jeff King]

On Fri, Sep 15, 2023 at 07:54:28PM -0700, Elijah Newren wrote:

> Oops, when I simplified the err() function copied from
> merge-recursive.c in one way, I failed to notice that it enabled
> further simplifications.

Ah, I didn't even realize that it had been copied from there. That makes
a lot more sense now.

> >   2. It formats the error string into a strbuf, prepending "error: ",
> >      and then feeds the result into error(). But this is wrong! The
> >      error() function already adds the prefix, so we end up with:
> >
> >         error: error: Failed to execute internal merge
> 
> ...and the same problem can be found in merge-recursive.c's err() function.
> 
> Not sure what current opinions on whether we should bother fixing
> those up.  I do intend on nuking merge-recursive.c, but I obviously
> haven't had much Git time this year.

Hmm, I'm not sure that it does. It has this code:

          if (opt->buffer_output < 2)
                  flush_output(opt);
          else {
                  strbuf_complete(&opt->obuf, '\n');
                  strbuf_addstr(&opt->obuf, "error: ");
          }

so we only add the extra "error:" tag when we are in a buffering mode
that writes into the strbuf (indicated by a high buffer_output field).

And then later, after formatting the new string into opt->obuf, we do
this:

          if (opt->buffer_output > 1)
                  strbuf_addch(&opt->obuf, '\n');
          else {
                  error("%s", opt->obuf.buf);
                  strbuf_reset(&opt->obuf);
          }

So we call error() iff buffer is low, in which case we would not have
added the prefix earlier. And that makes sense. If we are collecting
messages in obuf, we cannot use error(), and we have to handle the
prefix ourselves.

So I think it's correct? If you started with merge-recursive's err() and
then stripped it down to remove the extra buffering stuff, I can see
that it would be a natural error to accidentally leave in the extra
prefix while doing so.

I certainly find the code confusing (the split "< 2" and "> 1"
conditions to trigger related cases is a nice obfuscation bonus), but I
_think_ it's right. And if the end goal is to ditch merge-recursive.c, I
don't think it's worth spending any more brain power on it.

> > A few of these messages starts with capital letters, which is unlike our
> > usual error message style. I didn't clean that up here. We could do so
> > on top,
> 
> There are two of these.  In my defense, they were copied verbatim from
> merge-recursive.c.  And I, um, never noticed the problem over there
> before copying.  Or after.

That makes sense. We have a ton of these lurking in the code base. I
usually try to clean them up when I touch relevant lines, but I wasn't
sure about the path_msg thing here.

> Yeah, all callers of err()/error() are for things that should never
> happen regardless of repository contents and should result in an
> instant abort, whereas anything calling path_msg() is a conflict or
> informational message that is expected for various kinds of repository
> data -- these messages are accumulated and later shown.

That makes sense in general. And I think most of these err() calls are
of that form (in fact, I'd expect most corruption to just trigger a
die() at a low level anyway, but we've been slowly lib-ifying that
away).

The one that gave me pause was my "the external merge driver gave us a
failure" case that I used for testing. In theory we could say "oops,
your merge driver is broken" and keep going. But I think it's not just
"broken", but "oops, your merge driver died with a signal", since a
non-zero exit just means conflicts. So at that point probably something
really has gone terribly wrong (not necessarily with Git, but with your
merge driver!).

> Another distinction is that any call to path_msg() is associated to a
> very specific path (or a few specific paths in special cases like
> renames or add/add with conflict modes), whereas none of the calls to
> err()/error() have a specific path they are about.  This serves a few
> purposes:

I think some of them are about specific paths. We hit err() if
merge_3way() returns -1, but that call can of course also result in a
regular conflict.

But if they're catastrophic errors anyway (as above), this part is kind
of moot.

> Anyway, long story short is that I think continuing to use error()
> instead of path_msg() or something else makes sense here.  The capital
> to lowercase cleanups make sense; we could even #leftoverbits for that
> piece.

Sounds good. I'll post a patch in a second, just to take care of it
while we're thinking about it.

-Peff

[PATCH 2/4] merge-ort: stop passing "opt" to read_oid_strbuf()

[Jeff King]

This function doesn't look at its merge_options parameter. It used to
pass it down to err(), but that function no longer exists (and didn't
look at "opt" anyway). We can drop it here.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ort.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 027ecc7f78..31c663b297 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -3491,8 +3491,7 @@ static int sort_dirs_next_to_their_children(const char *one, const char *two)
 		return c1 - c2;
 }
 
-static int read_oid_strbuf(struct merge_options *opt,
-			   const struct object_id *oid,
+static int read_oid_strbuf(const struct object_id *oid,
 			   struct strbuf *dst)
 {
 	void *buf;
@@ -3527,8 +3526,8 @@ static int blob_unchanged(struct merge_options *opt,
 	if (oideq(&base->oid, &side->oid))
 		return 1;
 
-	if (read_oid_strbuf(opt, &base->oid, &basebuf) ||
-	    read_oid_strbuf(opt, &side->oid, &sidebuf))
+	if (read_oid_strbuf(&base->oid, &basebuf) ||
+	    read_oid_strbuf(&side->oid, &sidebuf))
 		goto error_return;
 	/*
 	 * Note: binary | is used so that both renormalizations are
-- 
2.42.0.628.g8a27295885

[PATCH 3/4] merge-ort: drop unused parameters from detect_and_process_renames()

[Jeff King]

This function takes three trees representing the merge base and both
sides of the merge, but never looks at any of them. This is due to
f78cf97617 (merge-ort: call diffcore_rename() directly, 2021-02-14).
Prior to that commit, we passed pairs of trees to diff_tree_oid(). But
after that commit, we collect a custom diff_queue for each pair in the
merge_options struct, and just run diffcore_rename() on the result. So
the function does not need to know about the original trees at all
anymore.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ort.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 31c663b297..20eefd9b5e 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -3324,10 +3324,7 @@ static int collect_renames(struct merge_options *opt,
 	return clean;
 }
 
-static int detect_and_process_renames(struct merge_options *opt,
-				      struct tree *merge_base,
-				      struct tree *side1,
-				      struct tree *side2)
+static int detect_and_process_renames(struct merge_options *opt)
 {
 	struct diff_queue_struct combined = { 0 };
 	struct rename_info *renames = &opt->priv->renames;
@@ -4964,8 +4961,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
 	trace2_region_leave("merge", "collect_merge_info", opt->repo);
 
 	trace2_region_enter("merge", "renames", opt->repo);
-	result->clean = detect_and_process_renames(opt, merge_base,
-						   side1, side2);
+	result->clean = detect_and_process_renames(opt);
 	trace2_region_leave("merge", "renames", opt->repo);
 	if (opt->priv->renames.redo_after_renames == 2) {
 		trace2_region_enter("merge", "reset_maps", opt->repo);
-- 
2.42.0.628.g8a27295885

Re: [PATCH 3/4] merge-ort: drop unused parameters from detect_and_process_renames()

[Elijah Newren]

On Thu, Sep 14, 2023 at 2:39 AM Jeff King <peff@peff.net> wrote:
>
> This function takes three trees representing the merge base and both
> sides of the merge, but never looks at any of them. This is due to
> f78cf97617 (merge-ort: call diffcore_rename() directly, 2021-02-14).
> Prior to that commit, we passed pairs of trees to diff_tree_oid(). But
> after that commit, we collect a custom diff_queue for each pair in the
> merge_options struct, and just run diffcore_rename() on the result. So
> the function does not need to know about the original trees at all
> anymore.

Thanks for including the history.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  merge-ort.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 31c663b297..20eefd9b5e 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -3324,10 +3324,7 @@ static int collect_renames(struct merge_options *opt,
>         return clean;
>  }
>
> -static int detect_and_process_renames(struct merge_options *opt,
> -                                     struct tree *merge_base,
> -                                     struct tree *side1,
> -                                     struct tree *side2)
> +static int detect_and_process_renames(struct merge_options *opt)
>  {
>         struct diff_queue_struct combined = { 0 };
>         struct rename_info *renames = &opt->priv->renames;
> @@ -4964,8 +4961,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
>         trace2_region_leave("merge", "collect_merge_info", opt->repo);
>
>         trace2_region_enter("merge", "renames", opt->repo);
> -       result->clean = detect_and_process_renames(opt, merge_base,
> -                                                  side1, side2);
> +       result->clean = detect_and_process_renames(opt);
>         trace2_region_leave("merge", "renames", opt->repo);
>         if (opt->priv->renames.redo_after_renames == 2) {
>                 trace2_region_enter("merge", "reset_maps", opt->repo);
> --
> 2.42.0.628.g8a27295885

Looks good.

[PATCH 4/4] merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()

[Jeff King]

The merge_options parameter has never been used since the function was
introduced in 64aceb6d73 (merge-ort: add code to check for whether
cached renames can be reused, 2021-05-20). In theory some merge options
might impact our decisions here, but that has never been the case so
far.

Let's drop it to appease -Wunused-parameter; it would be easy to add
back later if we need to (there is only one caller).

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ort.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 20eefd9b5e..3953c9f745 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4880,8 +4880,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
 	trace2_region_leave("merge", "allocate/init", opt->repo);
 }
 
-static void merge_check_renames_reusable(struct merge_options *opt,
-					 struct merge_result *result,
+static void merge_check_renames_reusable(struct merge_result *result,
 					 struct tree *merge_base,
 					 struct tree *side1,
 					 struct tree *side2)
@@ -5083,7 +5082,7 @@ void merge_incore_nonrecursive(struct merge_options *opt,
 
 	trace2_region_enter("merge", "merge_start", opt->repo);
 	assert(opt->ancestor != NULL);
-	merge_check_renames_reusable(opt, result, merge_base, side1, side2);
+	merge_check_renames_reusable(result, merge_base, side1, side2);
 	merge_start(opt, result);
 	/*
 	 * Record the trees used in this merge, so if there's a next merge in
-- 
2.42.0.628.g8a27295885

Re: [PATCH 4/4] merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()

[Elijah Newren]

On Thu, Sep 14, 2023 at 2:40 AM Jeff King <peff@peff.net> wrote:
>
> The merge_options parameter has never been used since the function was
> introduced in 64aceb6d73 (merge-ort: add code to check for whether
> cached renames can be reused, 2021-05-20). In theory some merge options
> might impact our decisions here, but that has never been the case so
> far.

Yeah, it was used in some preliminary versions of the code while I was
developing the new algorithm, but there were lots of changes between
when I started working on merge-ort and when it was finally ready to
submit for review.  I must have just overlooked that this parameter
was no longer needed.  Thanks for catching and cleaning up.

> Let's drop it to appease -Wunused-parameter; it would be easy to add
> back later if we need to (there is only one caller).

Yep, makes sense.


> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  merge-ort.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 20eefd9b5e..3953c9f745 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4880,8 +4880,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
>         trace2_region_leave("merge", "allocate/init", opt->repo);
>  }
>
> -static void merge_check_renames_reusable(struct merge_options *opt,
> -                                        struct merge_result *result,
> +static void merge_check_renames_reusable(struct merge_result *result,
>                                          struct tree *merge_base,
>                                          struct tree *side1,
>                                          struct tree *side2)
> @@ -5083,7 +5082,7 @@ void merge_incore_nonrecursive(struct merge_options *opt,
>
>         trace2_region_enter("merge", "merge_start", opt->repo);
>         assert(opt->ancestor != NULL);
> -       merge_check_renames_reusable(opt, result, merge_base, side1, side2);
> +       merge_check_renames_reusable(result, merge_base, side1, side2);
>         merge_start(opt, result);
>         /*
>          * Record the trees used in this merge, so if there's a next merge in
> --
> 2.42.0.628.g8a27295885

Re: [PATCH 4/4] merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()

[Jeff King]

On Fri, Sep 15, 2023 at 08:09:00PM -0700, Elijah Newren wrote:

> On Thu, Sep 14, 2023 at 2:40 AM Jeff King <peff@peff.net> wrote:
> >
> > The merge_options parameter has never been used since the function was
> > introduced in 64aceb6d73 (merge-ort: add code to check for whether
> > cached renames can be reused, 2021-05-20). In theory some merge options
> > might impact our decisions here, but that has never been the case so
> > far.
> 
> Yeah, it was used in some preliminary versions of the code while I was
> developing the new algorithm, but there were lots of changes between
> when I started working on merge-ort and when it was finally ready to
> submit for review.  I must have just overlooked that this parameter
> was no longer needed.  Thanks for catching and cleaning up.

Yeah, that's what I figured. I actually queued quite a few of these
-Wunused-parameter fixups, because the initial iterations of merge-ort
had a lot of stub functions or unimplemented bits. I sat on them for a
year or so because I figured you'd eventually use those parameters. And
indeed, most of them fell out naturally, and what was left for this
series was all pretty easy to understand.

-Peff

Re: [PATCH 0/4] merge-ort unused parameter cleanups

[Elijah Newren]

On Thu, Sep 14, 2023 at 2:34 AM Jeff King <peff@peff.net> wrote:
>
> A few small cleanups for merge-ort collected from playing with
> -Wunused-parameter. The first one actually fixes a user-visible bug, and
> the rest are just cleanups.
>
>   [1/4]: merge-ort: drop custom err() function
>   [2/4]: merge-ort: stop passing "opt" to read_oid_strbuf()
>   [3/4]: merge-ort: drop unused parameters from detect_and_process_renames()
>   [4/4]: merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()
>
>  merge-ort.c           | 48 +++++++++++--------------------------------
>  t/t6406-merge-attr.sh |  3 ++-
>  2 files changed, 14 insertions(+), 37 deletions(-)
>
> -Peff

All look good; thanks.

[PATCH 5/4] merge-ort: lowercase a few error messages

[Jeff King]

On Thu, Sep 14, 2023 at 05:34:09AM -0400, Jeff King wrote:

> A few small cleanups for merge-ort collected from playing with
> -Wunused-parameter. The first one actually fixes a user-visible bug, and
> the rest are just cleanups.
> 
>   [1/4]: merge-ort: drop custom err() function
>   [2/4]: merge-ort: stop passing "opt" to read_oid_strbuf()
>   [3/4]: merge-ort: drop unused parameters from detect_and_process_renames()
>   [4/4]: merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()

Here's one more clean-up on top. I hesitated on this for the initial
send just because I didn't know if we might want to switch these error
messages to path_msg(), which does capitalize sometimes. But Elijah's
response convinced me that we should just leave them in place, in which
case it makes sense to do a minimal style fixup.

Junio, this is on top of what you've queued in
jk/ort-unused-parameter-cleanups.

-- >8 --
Subject: [PATCH] merge-ort: lowercase a few error messages

As noted in CodingGuidelines, error messages should not be capitalized.
Fix up a few of these that were copied verbatim from merge-recursive to
match our modern style.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ort.c           | 4 ++--
 t/t6406-merge-attr.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 3953c9f745..7857ce9fbd 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2105,12 +2105,12 @@ static int handle_content_merge(struct merge_options *opt,
 					  &result_buf);
 
 		if ((merge_status < 0) || !result_buf.ptr)
-			ret = error(_("Failed to execute internal merge"));
+			ret = error(_("failed to execute internal merge"));
 
 		if (!ret &&
 		    write_object_file(result_buf.ptr, result_buf.size,
 				      OBJ_BLOB, &result->oid))
-			ret = error(_("Unable to add %s to database"), path);
+			ret = error(_("unable to add %s to database"), path);
 
 		free(result_buf.ptr);
 		if (ret)
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 05ad13b23e..72f8c1722f 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -180,7 +180,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
 	>./please-abort &&
 	echo "* merge=custom" >.gitattributes &&
 	test_must_fail git merge main 2>err &&
-	grep "^error: Failed to execute internal merge" err &&
+	grep "^error: failed to execute internal merge" err &&
 	git ls-files -u >output &&
 	git diff --name-only HEAD >>output &&
 	test_must_be_empty output
-- 
2.42.0.661.g2507eb519e

Re: [PATCH 5/4] merge-ort: lowercase a few error messages

[Jeff King]

On Sat, Sep 16, 2023 at 02:01:00AM -0400, Jeff King wrote:

> Here's one more clean-up on top. I hesitated on this for the initial
> send just because I didn't know if we might want to switch these error
> messages to path_msg(), which does capitalize sometimes. But Elijah's
> response convinced me that we should just leave them in place, in which
> case it makes sense to do a minimal style fixup.
> 
> Junio, this is on top of what you've queued in
> jk/ort-unused-parameter-cleanups.
> 
> -- >8 --
> Subject: [PATCH] merge-ort: lowercase a few error messages
> 
> As noted in CodingGuidelines, error messages should not be capitalized.
> Fix up a few of these that were copied verbatim from merge-recursive to
> match our modern style.

<sigh> This fails CI because with GIT_TEST_MERGE_ALGORITHM=recursive, we
run the old merge-recursive code, which uses the capitalized version.

I'm inclined to just drop this minor cleanup for now, and we can worry
about it later once merge-recursive goes the way of the dodo.

-Peff

Re: [PATCH 5/4] merge-ort: lowercase a few error messages

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Sep 16, 2023 at 02:01:00AM -0400, Jeff King wrote:
>
>> Here's one more clean-up on top. I hesitated on this for the initial
>> send just because I didn't know if we might want to switch these error
>> messages to path_msg(), which does capitalize sometimes. But Elijah's
>> response convinced me that we should just leave them in place, in which
>> case it makes sense to do a minimal style fixup.
>> 
>> Junio, this is on top of what you've queued in
>> jk/ort-unused-parameter-cleanups.
>> 
>> -- >8 --
>> Subject: [PATCH] merge-ort: lowercase a few error messages
>> 
>> As noted in CodingGuidelines, error messages should not be capitalized.
>> Fix up a few of these that were copied verbatim from merge-recursive to
>> match our modern style.
>
> <sigh> This fails CI because with GIT_TEST_MERGE_ALGORITHM=recursive, we
> run the old merge-recursive code, which uses the capitalized version.
>
> I'm inclined to just drop this minor cleanup for now, and we can worry
> about it later once merge-recursive goes the way of the dodo.

I wonder if it is just the matter of making matching changes to the
original error messages in merge-recursive that share the
capitalized version?

Re: [PATCH 5/4] merge-ort: lowercase a few error messages

[Jeff King]

On Sat, Sep 16, 2023 at 02:49:12PM -0700, Junio C Hamano wrote:

> > <sigh> This fails CI because with GIT_TEST_MERGE_ALGORITHM=recursive, we
> > run the old merge-recursive code, which uses the capitalized version.
> >
> > I'm inclined to just drop this minor cleanup for now, and we can worry
> > about it later once merge-recursive goes the way of the dodo.
> 
> I wonder if it is just the matter of making matching changes to the
> original error messages in merge-recursive that share the
> capitalized version?

It is, but I didn't want to touch merge-recursive at all if it is
destined for removal. But really, it is not much extra work to do so. So
here's an updated patch.

-- >8 --
Subject: [PATCH] merge-ort: lowercase a few error messages

As noted in CodingGuidelines, error messages should not be capitalized.
Fix up a few of these that were copied verbatim from merge-recursive to
match our modern style.

We'll likewise fix up the matching ones from merge-recursive. We care a
bit less there, since the hope is that it will eventually go away. But
besides being the right thing to do in the meantime, it is necessary for
t6406 to pass both with and without GIT_TEST_MERGE_ALGORITHM set (one of
our CI jobs sets it to "recursive", which will use the merge-recursive.c
code). An alternative would be to use "grep -i" in the test to check
the message, but it's nice for the test suite to be be more exact (we'd
notice if the capitalization fix regressed).

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ort.c           | 4 ++--
 merge-recursive.c     | 4 ++--
 t/t6406-merge-attr.sh | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index 3953c9f745..7857ce9fbd 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2105,12 +2105,12 @@ static int handle_content_merge(struct merge_options *opt,
 					  &result_buf);
 
 		if ((merge_status < 0) || !result_buf.ptr)
-			ret = error(_("Failed to execute internal merge"));
+			ret = error(_("failed to execute internal merge"));
 
 		if (!ret &&
 		    write_object_file(result_buf.ptr, result_buf.size,
 				      OBJ_BLOB, &result->oid))
-			ret = error(_("Unable to add %s to database"), path);
+			ret = error(_("unable to add %s to database"), path);
 
 		free(result_buf.ptr);
 		if (ret)
diff --git a/merge-recursive.c b/merge-recursive.c
index 6a4081bb0f..0d7e57e2df 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1383,12 +1383,12 @@ static int merge_mode_and_contents(struct merge_options *opt,
 						  extra_marker_size);
 
 			if ((merge_status < 0) || !result_buf.ptr)
-				ret = err(opt, _("Failed to execute internal merge"));
+				ret = err(opt, _("failed to execute internal merge"));
 
 			if (!ret &&
 			    write_object_file(result_buf.ptr, result_buf.size,
 					      OBJ_BLOB, &result->blob.oid))
-				ret = err(opt, _("Unable to add %s to database"),
+				ret = err(opt, _("unable to add %s to database"),
 					  a->path);
 
 			free(result_buf.ptr);
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 05ad13b23e..72f8c1722f 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -180,7 +180,7 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
 	>./please-abort &&
 	echo "* merge=custom" >.gitattributes &&
 	test_must_fail git merge main 2>err &&
-	grep "^error: Failed to execute internal merge" err &&
+	grep "^error: failed to execute internal merge" err &&
 	git ls-files -u >output &&
 	git diff --name-only HEAD >>output &&
 	test_must_be_empty output
-- 
2.42.0.662.g15203389d6