Re: [PATCH v3 4/8] merge-recursive: new function for better colliding conflict resolutions
On Wed, Oct 31, 2018 at 6:57 AM Derrick Stolee wrote:
>
> On 10/31/2018 9:53 AM, Derrick Stolee wrote:
> > On 10/19/2018 3:31 PM, Elijah Newren wrote:
> >> +#if 0 // #if-0-ing avoids unused function warning; will make live in
> >> next commit
> >> +static int handle_file_collision(struct merge_options *o,
> >> + const char *collide_path,
> >> + const char *prev_path1,
> >> + const char *prev_path2,
> >> + const char *branch1, const char *branch2,
> >> + const struct object_id *a_oid,
> >> + unsigned int a_mode,
> >> + const struct object_id *b_oid,
> >> + unsigned int b_mode)
> >> +{
> >> +struct merge_file_info mfi;
> >> +struct diff_filespec null, a, b;
> >> +char *alt_path = NULL;
> >> +const char *update_path = collide_path;
> >> +
> >> +/*
> >> + * In the recursive case, we just opt to undo renames
> >> + */
> >> +if (o->call_depth && (prev_path1 || prev_path2)) {
> >> +/* Put first file (a_oid, a_mode) in its original spot */
> >> +if (prev_path1) {
> >> +if (update_file(o, 1, a_oid, a_mode, prev_path1))
> >> +return -1;
> >> +} else {
> >> +if (update_file(o, 1, a_oid, a_mode, collide_path))
> >
> > The latest test coverage report [1] shows this if statement is never
> > run, so
> > it appears that every call to this method in the test suite has either
> > o->call_depth positive, prev_path1 non-NULL, or both prev_path1 and
> > prev_path2
> > NULL.
> >
> > Is there a way we can add a test case that calls this method with
> > o->call_depth
> > positive, prev_path1 NULL, and prev_path2 non-NULL?
> >
> >> +return -1;
> >> +}
> >> +
> >> +/* Put second file (b_oid, b_mode) in its original spot */
> >> +if (prev_path2) {
> >> +if (update_file(o, 1, b_oid, b_mode, prev_path2))
> >
> > Since this line is covered, we _do_ call the method with prev_path2
> > non-NULL, but
> > prev_path1 must be non-NULL in all cases.
> >
> > I may have found a reason why this doesn't happen in one of the
> > callers you introduced.
> > I'm going to comment on PATCH 8/8 to see if that is the case.
>
> Nevermind on the PATCH 8/8 situation. I thought I saw you pass (a->path,
> NULL) and
> (b->path, NULL) into the (prev_path1, prev_path2) pairs, but in each
> case the non-NULL
> parameter is actually for 'collide_path'.
>
> It is still interesting if we can hit this case. Perhaps we need a
> different kind of
> conflict, like (rename, delete) [but I struggle to make sense of how to
> do that].
rename/delete conflicts are sent through handle_rename_delete() which
do not call into handle_file_collision(). What you'd instead need is
a rename/add conflict, in the virtual merge base, on the appropriate
side. The fact that the prev_path2 non-NULL case is covered means
there's already a regression test that's probably nearly good enough,
you'd just need to edit the committer timestamps of the two merge
bases so that a different one was older. I'm pretty sure we can come
up with one without too much effort. I'll take a look tomorrow; too
late tonight.
Re: [PATCH v3 4/8] merge-recursive: new function for better colliding conflict resolutions
On 10/31/2018 9:53 AM, Derrick Stolee wrote:
On 10/19/2018 3:31 PM, Elijah Newren wrote:
+#if 0 // #if-0-ing avoids unused function warning; will make live in
next commit
+static int handle_file_collision(struct merge_options *o,
+ const char *collide_path,
+ const char *prev_path1,
+ const char *prev_path2,
+ const char *branch1, const char *branch2,
+ const struct object_id *a_oid,
+ unsigned int a_mode,
+ const struct object_id *b_oid,
+ unsigned int b_mode)
+{
+ struct merge_file_info mfi;
+ struct diff_filespec null, a, b;
+ char *alt_path = NULL;
+ const char *update_path = collide_path;
+
+ /*
+ * In the recursive case, we just opt to undo renames
+ */
+ if (o->call_depth && (prev_path1 || prev_path2)) {
+ /* Put first file (a_oid, a_mode) in its original spot */
+ if (prev_path1) {
+ if (update_file(o, 1, a_oid, a_mode, prev_path1))
+ return -1;
+ } else {
+ if (update_file(o, 1, a_oid, a_mode, collide_path))
The latest test coverage report [1] shows this if statement is never
run, so
it appears that every call to this method in the test suite has either
o->call_depth positive, prev_path1 non-NULL, or both prev_path1 and
prev_path2
NULL.
Is there a way we can add a test case that calls this method with
o->call_depth
positive, prev_path1 NULL, and prev_path2 non-NULL?
+ return -1;
+ }
+
+ /* Put second file (b_oid, b_mode) in its original spot */
+ if (prev_path2) {
+ if (update_file(o, 1, b_oid, b_mode, prev_path2))
Since this line is covered, we _do_ call the method with prev_path2
non-NULL, but
prev_path1 must be non-NULL in all cases.
I may have found a reason why this doesn't happen in one of the
callers you introduced.
I'm going to comment on PATCH 8/8 to see if that is the case.
Nevermind on the PATCH 8/8 situation. I thought I saw you pass (a->path,
NULL) and
(b->path, NULL) into the (prev_path1, prev_path2) pairs, but in each
case the non-NULL
parameter is actually for 'collide_path'.
It is still interesting if we can hit this case. Perhaps we need a
different kind of
conflict, like (rename, delete) [but I struggle to make sense of how to
do that].
Thanks,
-Stolee
Re: [PATCH v3 4/8] merge-recursive: new function for better colliding conflict resolutions
On 10/19/2018 3:31 PM, Elijah Newren wrote:
+#if 0 // #if-0-ing avoids unused function warning; will make live in next
commit
+static int handle_file_collision(struct merge_options *o,
+const char *collide_path,
+const char *prev_path1,
+const char *prev_path2,
+const char *branch1, const char *branch2,
+const struct object_id *a_oid,
+unsigned int a_mode,
+const struct object_id *b_oid,
+unsigned int b_mode)
+{
+ struct merge_file_info mfi;
+ struct diff_filespec null, a, b;
+ char *alt_path = NULL;
+ const char *update_path = collide_path;
+
+ /*
+* In the recursive case, we just opt to undo renames
+*/
+ if (o->call_depth && (prev_path1 || prev_path2)) {
+ /* Put first file (a_oid, a_mode) in its original spot */
+ if (prev_path1) {
+ if (update_file(o, 1, a_oid, a_mode, prev_path1))
+ return -1;
+ } else {
+ if (update_file(o, 1, a_oid, a_mode, collide_path))
The latest test coverage report [1] shows this if statement is never run, so
it appears that every call to this method in the test suite has either
o->call_depth positive, prev_path1 non-NULL, or both prev_path1 and
prev_path2
NULL.
Is there a way we can add a test case that calls this method with
o->call_depth
positive, prev_path1 NULL, and prev_path2 non-NULL?
+ return -1;
+ }
+
+ /* Put second file (b_oid, b_mode) in its original spot */
+ if (prev_path2) {
+ if (update_file(o, 1, b_oid, b_mode, prev_path2))
Since this line is covered, we _do_ call the method with prev_path2
non-NULL, but
prev_path1 must be non-NULL in all cases.
I may have found a reason why this doesn't happen in one of the callers
you introduced.
I'm going to comment on PATCH 8/8 to see if that is the case.
Thanks,
-Stolee
[1]
https://public-inbox.org/git/62f0bcf6-aa73-c192-d804-e6d69cac1...@gmail.com/
[PATCH v3 4/8] merge-recursive: new function for better colliding conflict resolutions
There are three conflict types that represent two (possibly entirely unrelated) files colliding at the same location: * add/add * rename/add * rename/rename(2to1) These three conflict types already share more similarity than might be immediately apparent from their description: (1) the handling of the rename variants already involves removing any entries from the index corresponding to the original file names[*], thus only leaving entries in the index for the colliding path; (2) likewise, any trace of the original file name in the working tree is also removed. So, in all three cases we're left with how to represent two colliding files in both the index and the working copy. [*] Technically, this isn't quite true because rename/rename(2to1) conflicts in the recursive (o->call_depth > 0) case do an "unrename" since about seven years ago. But even in that case, Junio felt compelled to explain that my decision to "unrename" wasn't necessarily the only or right answer -- search for "Comment from Junio" in t6036 for details. My initial motivation for looking at these three conflict types was that if the handling of these three conflict types is the same, at least in the limited set of cases where a renamed file is unmodified on the side of history where the file is not renamed, then a significant performance improvement for rename detection during merges is possible. However, while that served as motivation to look at these three types of conflicts, the actual goal of this new function is to try to improve the handling for all three cases, not to merely make them the same as each other in that special circumstance. === Handling the working tree === The previous behavior for these conflict types in regards to the working tree (assuming the file collision occurs at 'foo') was: * add/add does a two-way merge of the two files and records it as 'foo'. * rename/rename(2to1) records the two different files into two new uniquely named files (foo~HEAD and foo~$MERGE), while removing 'foo' from the working tree. * rename/add records the two different files into two different locations, recording the add at foo~$SIDE and, oddly, recording the rename at foo (why is the rename more important than the add?) So, the question for what to write to the working tree boils down to whether the two colliding files should be two-way merged and recorded in place, or recorded into separate files. As per discussion on the git mailing lit, two-way merging was deemed to always be preferred, as that makes these cases all more like content conflicts that users can handle from within their favorite editor, IDE, or merge tool. Note that since renames already involve a content merge, rename/add and rename/rename(2to1) conflicts could result in nested conflict markers. === Handling of the index === For a typical rename, unpack_trees() would set up the index in the following fashion: old_path new_path stage1: 5ca1ab1e stage2: f005ba11 stage3: b0a710ad And merge-recursive would rewrite this to new_path stage1: 5ca1ab1e stage2: f005ba11 stage3: b0a710ad Removing old_path from the index means the user won't have to `git rm old_path` manually every time a renamed path has a content conflict. It also means they can use `git checkout [--ours|--theirs|--conflict|-m] new_path`, `git diff [--ours|--theirs]` and various other commands that would be difficult otherwise. This strategy becomes a problem when we have a rename/add or rename/rename(2to1) conflict, however, because then we have only three slots to store blob sha1s and we need either four or six. Previously, this was handled by continuing to delete old_path from the index, and just outright ignoring any blob shas from old_path. That had the downside of deleting any trace of changes made to old_path on the other side of history. This function instead does a three-way content merge of the renamed file, and stores the blob sha1 for that at either stage2 or stage3 for new_path (depending on which side the rename came from). That has the advantage of bringing information about changes on both sides and still allows for easy resolution (no need to git rm old_path, etc.), but does have the downside that if the content merge had conflict markers, then what we store in the index is the sha1 of a blob with conflict markers. While that is a downside, it seems less problematic than the downsides of any obvious alternatives, and certainly makes more sense than the previous handling. Further, it has a precedent in that when we do recursive merges, we may accept a file with conflict markers as the resolution for the merge of the merge-bases, which will then show up in the index of the outer merge at stage 1 if a conflict exists at the outer level. Signed-off-by: Elijah Newren --- merge-recursive.c | 121 ++ 1 file changed, 121 insertions(+) diff
