| Message ID | c0d7bafd43823ef9df5a73bc80b90cf003988bc9.1709907271.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | checkout: cleanup --conflict= | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/merge-ll.c b/merge-ll.c > index 6570707297d..bf1077ae092 100644 > --- a/merge-ll.c > +++ b/merge-ll.c > .. > -#define LL_MERGE_OPTIONS_INIT {0} > +#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 } Makes sense, and this obviously makes the previous step worth doing. It looks quite wrong that low-level merge options definition is hosted in a file whose name is merge low-level. Is it too late to rename the file to fix this, by the way? Thanks.
Hi Junio On 08/03/2024 15:46, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> diff --git a/merge-ll.c b/merge-ll.c >> index 6570707297d..bf1077ae092 100644 >> --- a/merge-ll.c >> +++ b/merge-ll.c >> .. >> -#define LL_MERGE_OPTIONS_INIT {0} >> +#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 } > > Makes sense, and this obviously makes the previous step worth doing. > > It looks quite wrong that low-level merge options definition is > hosted in a file whose name is merge low-level. Is it too late to > rename the file to fix this, by the way? I agree it is confusing, Elijah renamed it from ll-merge.c relatively recently 6723899932e (merge-ll: rename from ll-merge, 2023-05-16). It looks like the idea was to group it with the other merge* files: merge-ll: rename from ll-merge A long term (but rather minor) pet-peeve of mine was the name ll-merge.[ch]. I thought it made it harder to realize what stuff was related to merging when I was working on the merge machinery and trying to improve it. Further, back in d1cbe1e6d8a ("hash-ll.h: split out of hash.h to remove dependency on repository.h", 2023-04-22), we have split the portions of hash.h that do not depend upon repository.h into a "hash-ll.h" (due to the recommendation to use "ll" for "low-level" in its name[1], but which I used as a suffix precisely because of my distaste for "ll-merge"). When we discussed adding additional "*-ll.h" files, a request was made that we use "ll" consistently as either a prefix or a suffix. Since it is already in use as both a prefix and a suffix, the only way to do so is to rename some files. Besides my distaste for the ll-merge.[ch] name, let me also note that the files ll-fsmonitor.h, ll-hash.h, ll-merge.h, ll-object-store.h, ll-read-cache.h would have essentially nothing to do with each other and make no sense to group. But giving them the common "ll-" prefix would group them. Using "-ll" as a suffix thus seems just much more logical to me. Rename ll-merge.[ch] to merge-ll.[ch] to achieve this consistency, and to ensure we get a more logical grouping of files. [1] https://lore.kernel.org/git/kl6lsfcu1g8w.fsf@chooglen-macbookpro.roam.corp.google.com/ Best Wishes Phillip
phillip.wood123@gmail.com writes: > I agree it is confusing, Elijah renamed it from ll-merge.c relatively > recently 6723899932e (merge-ll: rename from ll-merge, 2023-05-16). It > looks like the idea was to group it with the other merge* files: That reasoning cuts both ways. When you are only interested in the upper level API functions, being able to skip ll-anything as "low level details" is a powerful thing. merge-ll & hash-ll separated far apart will make it impossible.
On Fri, Mar 8, 2024 at 8:48 AM Junio C Hamano <gitster@pobox.com> wrote: > > phillip.wood123@gmail.com writes: > > > I agree it is confusing, Elijah renamed it from ll-merge.c relatively > > recently 6723899932e (merge-ll: rename from ll-merge, 2023-05-16). It > > looks like the idea was to group it with the other merge* files: > > That reasoning cuts both ways. When you are only interested in the > upper level API functions, being able to skip ll-anything as "low > level details" is a powerful thing. merge-ll & hash-ll separated > far apart will make it impossible. merge-ll is wildly different than every other *-ll.h file we have in the tree; the latter set of files may be misnamed, for reasons other than what you are suggesting here. hash-ll, hex-ll, etc. exist due to the main include file having some rarely used API that require more #include statements, and most users of e.g. hex functions can get away with just including hex-ll.h rather than the full hex.h. Thus, hex-ll.h is _not_ "low level details that you can skip", but "the _primary_ data structures and functions". It doesn't get the name hex.h, though, because if we did that then the folks that need both primary parts of the API and the occasional additional function would need to have two hex-related includes. Also, every function declared within hex-ll.h is still defined within hex.c; there is no separate hex-ll.c file, and the same is true of all the other *-ll.h files other than merge-ll.h. As such, there is absolutely no relation between hex-ll.h, hash-ll.h, fsmonitor-ll.h, etc. Using an ll- prefix on those filenames thus makes no sense to me. (It's not clear that -ll even makes sense as a suffix for these files either, but it's not clear what else to use instead. If I recall correctly I originally put forward "-basics" as a possible name suffix for these files, but someone else suggested "-ll", and not having any better ideas or strong opinions I just went with it.) merge-ll is different in that it is actually a separate level of the API, and there are both a merge-ll.h file and a merge-ll.c file. I originally had proposed only adding the hex-ll.h, hash-ll.h, fsmonitor-ll.h, etc. files, but you suggested that ll-merge should either be renamed or that these new files should be (https://lore.kernel.org/git/CABPp-BErrVUnuDjL73edDpmkKUvs6Ny6cYwvueXw1toB4JcF-Q@mail.gmail.com/). Now, all that said, and assuming we were to go back to a world where the other *-ll.h files don't exist (or are renamed independently with a different suffix), I'm still not understanding why ll-merge makes more sense to you and Phillip than merge-ll. Could you explain more? If you're only interested in the upper-level API functions, that suggests you are already at the function level and looking within a given file. The low-level functions are already split out into a separate file, so you just don't go looking at that separate file. However, if you're interested in "where in this codebase do I find the stuff related to merging", then you aren't in a file but in a directory listing. The first usecase gains no benefit from renaming these files back to ll-merge.[ch], while the other usecase would have been much improved in my experience from being named merge-ll.[ch]. Granted, it's a really minor point, which was why I never brought it up until you suggested making all the other *-ll.h files and ll-merge.[ch] consistent; since renaming the other *-ll.h files made no sense at all to me, I went with renaming ll-merge.[ch] to merge-ll.[ch]. There's probably some other angle to this that you two are viewing this from that just isn't apparent to me. Sorry for not seeing it (yet). Hopefully the above context is at least helpful, though.
Elijah Newren <newren@gmail.com> writes: > a different suffix), I'm still not understanding why ll-merge makes > more sense to you and Phillip than merge-ll. > Could you explain more? Language and grammar? In other words, "low-level merge routines" is an understandable phrase, while "merge lo-level routines" is not.
diff --git a/merge-ll.c b/merge-ll.c index 6570707297d..bf1077ae092 100644 --- a/merge-ll.c +++ b/merge-ll.c @@ -128,7 +128,9 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse xmp.level = XDL_MERGE_ZEALOUS; xmp.favor = opts->variant; xmp.xpp.flags = opts->xdl_opts; - if (git_xmerge_style >= 0) + if (opts->conflict_style >= 0) + xmp.style = opts->conflict_style; + else if (git_xmerge_style >= 0) xmp.style = git_xmerge_style; if (marker_size > 0) xmp.marker_size = marker_size; diff --git a/merge-ll.h b/merge-ll.h index af1ee36abdb..d038ee0c1e8 100644 --- a/merge-ll.h +++ b/merge-ll.h @@ -78,11 +78,14 @@ struct ll_merge_options { */ unsigned extra_marker_size; + /* Override the global conflict style. */ + int conflict_style; + /* Extra xpparam_t flags as defined in xdiff/xdiff.h. */ long xdl_opts; }; -#define LL_MERGE_OPTIONS_INIT {0} +#define LL_MERGE_OPTIONS_INIT { .conflict_style = -1 } enum ll_merge_result { LL_MERGE_ERROR = -1, diff --git a/merge-ort.c b/merge-ort.c index 4a02c3ecd99..a9ab4031451 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -1966,6 +1966,7 @@ static int merge_3way(struct merge_options *opt, ll_opts.renormalize = opt->renormalize; ll_opts.extra_marker_size = extra_marker_size; ll_opts.xdl_opts = opt->xdl_opts; + ll_opts.conflict_style = opt->conflict_style; if (opt->priv->call_depth) { ll_opts.virtual_ancestor = 1; diff --git a/merge-recursive.c b/merge-recursive.c index 02b7b584f95..33b5f9384e8 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1054,6 +1054,7 @@ static int merge_3way(struct merge_options *opt, ll_opts.renormalize = opt->renormalize; ll_opts.extra_marker_size = extra_marker_size; ll_opts.xdl_opts = opt->xdl_opts; + ll_opts.conflict_style = opt->conflict_style; if (opt->priv->call_depth) { ll_opts.virtual_ancestor = 1; @@ -3899,6 +3900,8 @@ void init_merge_options(struct merge_options *opt, opt->renormalize = 0; + opt->conflict_style = -1; + merge_recursive_config(opt); merge_verbosity = getenv("GIT_MERGE_VERBOSITY"); if (merge_verbosity) diff --git a/merge-recursive.h b/merge-recursive.h index 3d3b3e3c295..e67d38c3030 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -31,6 +31,7 @@ struct merge_options { /* xdiff-related options (patience, ignore whitespace, ours/theirs) */ long xdl_opts; + int conflict_style; enum { MERGE_VARIANT_NORMAL = 0, MERGE_VARIANT_OURS,