Re: [PATCH v2 05/20] merge-ort: add an err() function similar to one from merge-recursive

[Elijah Newren <newren@gmail.com>]

On Wed, Nov 11, 2020 at 5:58 AM Derrick Stolee <stolee@gmail.com> wrote:

> > +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;
> > +}
> > +
>
> This seems simple enough to have a duplicate copy lying
> around. Do you anticipate that all common code will live
> in the same file eventually? Or will these two static err()
> method always be duplicated?

I anticipate that merge-recursive.[ch] will be deleted.

merge-ort was what I wanted to modify merge-recursive.[ch] to be, but
Junio suggested doing it as a different merge backend since it had
invasive changes, so that we could have an easy way to try it out and
fallback to the known good algorithm until we had sufficient comfort
with the new algorithm to switch over to it.

> Aside: I wonder if these errors could be logged using trace2
> primitives, to assist diagnosing problems with merges. CC'ing
> Jeff Hostetler if he has an opinion.
>
> >  static int collect_merge_info(struct merge_options *opt,
> >                             struct tree *merge_base,
> >                             struct tree *side1,
> >                             struct tree *side2)
> >  {
> > +     /* TODO: Implement this using traverse_trees() */
> >       die("Not yet implemented.");
> >  }
>
> This comment looks to be applied to the wrong patch.

Oops!  You are correct; will fix.