| Message ID | 504f1f7c892c8bfc4774ac5fec912855e74e38a5.1572343246.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Move doc to header files | expand |
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... 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? > + * 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. > + * 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. > + * - 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. > + * - 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. > + * > + * 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? > 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. */ > long xdl_opts; Perhaps document this one with: /* Extra xpparam_t flags as defined in xdiff/xdiff.h. */ > }; > > +/** > + * 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,
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,
Elijah Newren <newren@gmail.com> writes: > 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... > Thanks, both. Especially thanks Elijah for all good comments.
diff --git a/Documentation/technical/api-merge.txt b/Documentation/technical/api-merge.txt deleted file mode 100644 index 9dc1bed768..0000000000 --- a/Documentation/technical/api-merge.txt +++ /dev/null @@ -1,104 +0,0 @@ -merge API -========= - -The merge API helps a program to reconcile two competing sets of -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. The library called through this API is -responsible for a few things. - - * determining which trees to merge (recursive ancestor consolidation); - - * 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; - - * performing a three-way merge of corresponding files, taking - path-specific merge drivers (specified in `.gitattributes`) - into account. - -Data structures ---------------- - -* `mmbuffer_t`, `mmfile_t` - -These store data usable for use by the xdiff backend, for writing and -for reading, respectively. See `xdiff/xdiff.h` for the definitions -and `diff.c` for examples. - -* `struct ll_merge_options` - -This describes the set of options the calling program wants to affect -the operation of a low-level (single file) merge. Some options: - -`virtual_ancestor`:: - 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 (see linkgit:gitattributes[5]). - -`variant`:: - 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`. - -`renormalize`:: - 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. - -Low-level (single file) merge ------------------------------ - -`ll_merge`:: - - 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. - -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. - -Everything else ---------------- - -Talk about <merge-recursive.h> and merge_file(): - - - merge_trees() to merge with rename detection - - merge_recursive() for ancestor consolidation - - try_merge_command() for other strategies - - conflict format - - merge options - -(Daniel, Miklos, Stephan, JC) 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 + * 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. The library called through this API is + * responsible for a few things. + * + * - determining which trees to merge (recursive ancestor consolidation); + * + * - 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; + * + * - performing a three-way merge of corresponding files, taking + * path-specific merge drivers (specified in `.gitattributes`) + * into account. + * + * 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. + */ + + struct index_state; +/** + * 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. + */ 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; + unsigned extra_marker_size; long xdl_opts; }; +/** + * 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,