[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH 04/10] merge: move doc to ll-merge.h
From:       Heba Waly <heba.waly () gmail ! com>
Date:       2019-10-31 19:35:02
Message-ID: CACg5j25WCHf8_pMVPeakYx-sdSG+-naPPchqUCesfaNQHVCCNQ () mail ! gmail ! com
[Download RAW message or body]

On Thu, Oct 31, 2019 at 11:09 AM Elijah Newren <newren@gmail.com> wrote:
>
> Hi Heba,
> Thanks for the contribution.  I know you weren't the original author
> of most this stuff, but I was curious if it really all belonged in
> ll-merge.c and then noticed other issues...

Hi Elijah, thanks a lot for the feedback.
This is my first interaction with the merge API, so I wasn't
completely sure where the intro of the doc needed to go, looks like
ll-merge.h is not the perfect place, is there a top level merge file
where this generic intro would be more suitable and helpful?

> On Tue, Oct 29, 2019 at 11:49 AM Heba Waly via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> [...]
> > diff --git a/ll-merge.h b/ll-merge.h
> > index e78973dd55..ec3617c627 100644
> > --- a/ll-merge.h
> > +++ b/ll-merge.h
> > @@ -7,16 +7,94 @@
> >
> >  #include "xdiff/xdiff.h"
> >
> > +/**
> > + * The merge API helps a program to reconcile two competing sets of
>
> Is this talking about xdiff/xmerge.c, ll_merge.c, merge-recursive.c,
> or builtin/merge.c?  Those are all different level of "merge API" and
> it's not clear.  Perhaps "The Low Level Merge API" or something like
> that since you are moving it into ll-merge.h?

Yea, that's why I'm thinking maybe move this paragraph to another
top-level file?

> > + * improvements to some files (e.g., unregistered changes from the work
> > + * tree versus changes involved in switching to a new branch), reporting
> > + * conflicts if found.
>
> Seems weird to bring up checkout -m without mentioning in by name
> given that it isn't the default checkout behavior.  Would seem more
> natural to mention a merge or rebase case.

I agree with you, can change the example if we agreed on keeping this paragraph.

> > + *   The library called through this API is
> > + * responsible for a few things.
> > + *
> > + *  - determining which trees to merge (recursive ancestor consolidation);
>
> Um, that's done at the merge-recursive.c level, not at the ll-merge.c
> level.  I'm confused why it'd be mentioned here.

you're right.

> > + *  - lining up corresponding files in the trees to be merged (rename
> > + *    detection, subtree shifting), reporting edge cases like add/add
> > + *    and rename/rename conflicts to the user;
>
> All of that is also clearly stuff for merge-recursive.c; I'm not sure
> why it'd be mentioned in the Low-Level merge file.

got it.

> > + *  - performing a three-way merge of corresponding files, taking
> > + *    path-specific merge drivers (specified in `.gitattributes`)
> > + *    into account.
>
> This, however, is ll-merge.c stuff.

So, move the whole paragraph to another file?
because, I think the paragraph is helpful as a whole, and I don't see
the value in dividing it between merge-recursive and ll-merge.
What do you think?

> > + *
> > + * Calling sequence:
> > + * ----------------
> > + *
> > + * - Prepare a `struct ll_merge_options` to record options.
> > + *   If you have no special requests, skip this and pass `NULL`
> > + *   as the `opts` parameter to use the default options.
> > + *
> > + * - Allocate an mmbuffer_t variable for the result.
> > + *
> > + * - Allocate and fill variables with the file's original content
> > + *   and two modified versions (using `read_mmfile`, for example).
> > + *
> > + * - Call `ll_merge()`.
> > + *
> > + * - Read the merged content from `result_buf.ptr` and `result_buf.size`.
> > + *
> > + * - Release buffers when finished.  A simple
> > + *   `free(ancestor.ptr); free(ours.ptr); free(theirs.ptr);
> > + *   free(result_buf.ptr);` will do.
> > + *
> > + * If the modifications do not merge cleanly, `ll_merge` will return a
> > + * nonzero value and `result_buf` will generally include a description of
> > + * the conflict bracketed by markers such as the traditional `<<<<<<<`
> > + * and `>>>>>>>`.
> > + *
> > + * The `ancestor_label`, `our_label`, and `their_label` parameters are
> > + * used to label the different sides of a conflict if the merge driver
> > + * supports this.
> > + */
>
> This part looks good.
>
> > +/**
> > + * This describes the set of options the calling program wants to affect
> > + * the operation of a low-level (single file) merge.
> > + */
> >  struct ll_merge_options {
> > +
> > +    /**
> > +     * Behave as though this were part of a merge between common ancestors in
> > +     * a recursive merge. If a helper program is specified by the
> > +        * `[merge "<driver>"] recursive` configuration, it will be used.
> > +     */
>
> This kind of leaves out the why.  Maybe add "(merges of binary files
> may need to be handled differently in such cases, for example)" to the
> end of the first sentence?

Yes, ok.

> >         unsigned virtual_ancestor : 1;
> > -       unsigned variant : 2;   /* favor ours, favor theirs, or union merge */
> > +
> > +       /**
> > +        * Resolve local conflicts automatically in favor of one side or the other
> > +        * (as in 'git merge-file' `--ours`/`--theirs`/`--union`).  Can be `0`,
> > +        * `XDL_MERGE_FAVOR_OURS`, `XDL_MERGE_FAVOR_THEIRS`,
> > +        * or `XDL_MERGE_FAVOR_UNION`.
> > +        */
> > +       unsigned variant : 2;
> > +
> > +       /**
> > +        * Resmudge and clean the "base", "theirs" and "ours" files before merging.
> > +        * Use this when the merge is likely to have overlapped with a change in
> > +        * smudge/clean or end-of-line normalization rules.
> > +        */
> >         unsigned renormalize : 1;
>
> All looks good.
>
> > +
> >         unsigned extra_marker_size;
>
> No documentation for this one?  Perhaps:
>
> /*
>  * Increase the length of conflict markers so that nested conflicts
>  * can be differentiated.
>  */

sure.

> >         long xdl_opts;
>
> Perhaps document this one with:
>
> /* Extra xpparam_t flags as defined in xdiff/xdiff.h. */

great. thanks!

>
>
> >  };
> >
> > +/**
> > + * Perform a three-way single-file merge in core.  This is a thin wrapper
> > + * around `xdl_merge` that takes the path and any merge backend specified in
> > + * `.gitattributes` or `.git/info/attributes` into account.
> > + * Returns 0 for a clean merge.
> > + */
> >  int ll_merge(mmbuffer_t *result_buf,
> >              const char *path,
> >              mmfile_t *ancestor, const char *ancestor_label,
[prev in list] [next in list] [prev in thread] [next in thread]