Re: [PATCH v2 06/20] merge-ort: implement a very basic collect_merge_info()

From: Elijah Newren <newren@gmail.com>
Date: Nov 6, 2020, 23:10 UTC
Message-ID: <CABPp-BFkAu6oO5V0jRh=ExuPxBXma1F17zaq-zEaqMagc72Ccw@mail.gmail.com>
In-Reply-To: <20201106221934.764672-1-jonathantanmy@google.com>

On Fri, Nov 6, 2020 at 2:19 PM Jonathan Tan <jonathantanmy@google.com> wrote:

Show 23 quoted lines

>
> > diff --git a/merge-ort.c b/merge-ort.c
> > index 537da9f6df..626eb9713e 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -77,13 +77,130 @@ static int err(struct merge_options *opt, const char *err, ...)
> >       return -1;
> >  }
> >
> > +static int collect_merge_info_callback(int n,
> > +                                    unsigned long mask,
> > +                                    unsigned long dirmask,
> > +                                    struct name_entry *names,
> > +                                    struct traverse_info *info)
> > +{
>
> [snip]
>
> > +     unsigned mbase_null = !(mask & 1);
> > +     unsigned side1_null = !(mask & 2);
> > +     unsigned side2_null = !(mask & 4);
>
> Should these be "int"?

Does the type matter, particularly since "boolean" isn't available?

Show 34 quoted lines

> > +     /*
> > +      * A bunch of sanity checks verifying that traverse_trees() calls
> > +      * us the way I expect.  Could just remove these at some point,
> > +      * though maybe they are helpful to future code readers.
> > +      */
> > +     assert(mbase_null == is_null_oid(&names[0].oid));
> > +     assert(side1_null == is_null_oid(&names[1].oid));
> > +     assert(side2_null == is_null_oid(&names[2].oid));
> > +     assert(!mbase_null || !side1_null || !side2_null);
> > +     assert(mask > 0 && mask < 8);
>
> These were helpful to me.
>
> > +     /* Other invariant checks, mostly for documentation purposes. */
> > +     assert(mask == (dirmask | filemask));
>
> But not this - filemask was computed in this function, so I need not
> look elsewhere to see that this is correct.
>
> > +     /*
> > +      * TODO: record information about the path other than all zeros,
> > +      * so we can resolve later in process_entries.
> > +      */
> > +     ci = xcalloc(1, sizeof(struct conflict_info));
> > +     strmap_put(&opti->paths, fullpath, ci);
>
> OK - so each entry is a full-size conflict_info to store all relevant
> information. Presumably some of these will be converted later into what
> is effectively a struct merged_info (so, the extra struct conflict_info
> fields are unused but memory is still occupied).
>
> I do see that in patch 10, there is an optimization that directly
> allocates the smaller struct merged_info when it is known at this point
> that there is no conflict.

Yep.  :-)

Show 22 quoted lines

> [snip rest of function]
>
> >  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.");
> > +     int ret;
> > +     struct tree_desc t[3];
> > +     struct traverse_info info;
> > +     char *toplevel_dir_placeholder = "";
> > +
> > +     opt->priv->current_dir_name = toplevel_dir_placeholder;
> > +     setup_traverse_info(&info, toplevel_dir_placeholder);
>
> I thought that this was written like this (instead of inlining the 2
> double-quotes) to ensure that the string-equality-is-pointer-equality
> characteristic holds, but I see that that characteristic is for
> directory_name in struct merged_info, not current_dir_name in struct
> merge_options_internal. Any reason for not inlining ""?

You're really digging in; I love it.  From setup_path_info(), the
directory_name is set from the current_dir_name:
        path_info->merged.directory_name = current_dir_name;
(and if you follow where the current_dir_name parameter gets its value
from, you find that it came indirectly from
opt->priv->current_dir_name), so current_dir_name must meet all the
requirements on merge_info's directory_name field.

Perhaps there's still some kind of additional simplification possible here, but directory rename detection is an area that has to take some special care around this requirement. I simplified the code a little bit in this area as I was trying to break off a good first 20 patches to submit, but even if we can simplify it more, the structure is just going to come back later.