Message ID | ff09ddb9caf73632c9792c07f1f7499a75a09606.1607962900.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | merge-ort: add basic rename detection | expand |
On 12/14/2020 11:21 AM, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > > Implement cases where renames are involved in type changes (i.e. the > side of history that didn't rename the file changed its type from a > regular file to a symlink or submodule). There was some code to handle > this in merge-recursive but only in the special case when the renamed > file had no content changes. The code here works differently -- it > knows process_entry() can handle mode conflicts, so it does a few > minimal tweaks to ensure process_entry() can just finish the job as > needed. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > merge-ort.c | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/merge-ort.c b/merge-ort.c > index 9aac33c8e31..11e33f56edf 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -778,7 +778,32 @@ static int process_renames(struct merge_options *opt, > S_ISREG(newinfo->stages[target_index].mode)); > if (type_changed && collision) { > /* special handling so later blocks can handle this */ Perhaps drop this comment, or incorporate it into the lower one? > - die("Not yet implemented"); > + /* > + * if type_changed && collision are both true, then this > + * was really a double rename, but one side wasn't > + * detected due to lack of break detection. I.e. > + * something like > + * orig: has normal file 'foo' > + * side1: renames 'foo' to 'bar', adds 'foo' symlink > + * side2: renames 'foo' to 'bar' > + * In this case, the foo->bar rename on side1 won't be > + * detected because the new symlink named 'foo' is > + * there and we don't do break detection. But we detect > + * this here because we don't want to merge the content > + * of the foo symlink with the foo->bar file, so we > + * have some logic to handle this special case. The > + * easiest way to do that is make 'bar' on side1 not > + * be considered a colliding file but the other part > + * of a normal rename. If the file is very different, > + * well we're going to get content merge conflicts > + * anyway so it doesn't hurt. And if the colliding > + * file also has a different type, that'll be handled > + * by the content merge logic in process_entry() too. > + * > + * See also t6430, 'rename vs. rename/symlink' I appreciate the callout to a test that exercises this behavior. > + */ > + collision = 0; > + } Here, we regain that closing curly brace, fixing the compiler errors from earlier. > if (source_deleted) { > if (target_index == 1) { > rename_branch = opt->branch1; > @@ -858,7 +883,11 @@ static int process_renames(struct merge_options *opt, > newinfo->pathnames[0] = oldpath; > if (type_changed) { > /* rename vs. typechange */ > - die("Not yet implemented"); > + /* Mark the original as resolved by removal */ > + memcpy(&oldinfo->stages[0].oid, &null_oid, > + sizeof(oldinfo->stages[0].oid)); > + oldinfo->stages[0].mode = 0; > + oldinfo->filemask &= 0x06; This matches your explanation in the comment above. I wonder if 0x06 could be less magical, but we are really deep in the weeds here already. Thanks, -Stolee
On Tue, Dec 15, 2020 at 6:31 AM Derrick Stolee <stolee@gmail.com> wrote: > > On 12/14/2020 11:21 AM, Elijah Newren via GitGitGadget wrote: > > From: Elijah Newren <newren@gmail.com> > > > > Implement cases where renames are involved in type changes (i.e. the > > side of history that didn't rename the file changed its type from a > > regular file to a symlink or submodule). There was some code to handle > > this in merge-recursive but only in the special case when the renamed > > file had no content changes. The code here works differently -- it > > knows process_entry() can handle mode conflicts, so it does a few > > minimal tweaks to ensure process_entry() can just finish the job as > > needed. > > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > --- > > merge-ort.c | 33 +++++++++++++++++++++++++++++++-- > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > diff --git a/merge-ort.c b/merge-ort.c > > index 9aac33c8e31..11e33f56edf 100644 > > --- a/merge-ort.c > > +++ b/merge-ort.c > > @@ -778,7 +778,32 @@ static int process_renames(struct merge_options *opt, > > S_ISREG(newinfo->stages[target_index].mode)); > > if (type_changed && collision) { > > /* special handling so later blocks can handle this */ > > Perhaps drop this comment, or incorporate it into the lower one? Will do. > > - die("Not yet implemented"); > > + /* > > + * if type_changed && collision are both true, then this > > + * was really a double rename, but one side wasn't > > + * detected due to lack of break detection. I.e. > > + * something like > > + * orig: has normal file 'foo' > > + * side1: renames 'foo' to 'bar', adds 'foo' symlink > > + * side2: renames 'foo' to 'bar' > > + * In this case, the foo->bar rename on side1 won't be > > + * detected because the new symlink named 'foo' is > > + * there and we don't do break detection. But we detect > > + * this here because we don't want to merge the content > > + * of the foo symlink with the foo->bar file, so we > > + * have some logic to handle this special case. The > > + * easiest way to do that is make 'bar' on side1 not > > + * be considered a colliding file but the other part > > + * of a normal rename. If the file is very different, > > + * well we're going to get content merge conflicts > > + * anyway so it doesn't hurt. And if the colliding > > + * file also has a different type, that'll be handled > > + * by the content merge logic in process_entry() too. > > + * > > + * See also t6430, 'rename vs. rename/symlink' > > I appreciate the callout to a test that exercises this behavior. > > > + */ > > + collision = 0; > > + } > > Here, we regain that closing curly brace, fixing the compiler errors from > earlier. So embarrassing. I was pretty sure I tested the individual patches, but maybe I somehow missed this series?? Anyway, yeah, I'll fix it up. > > > if (source_deleted) { > > if (target_index == 1) { > > rename_branch = opt->branch1; > > @@ -858,7 +883,11 @@ static int process_renames(struct merge_options *opt, > > newinfo->pathnames[0] = oldpath; > > if (type_changed) { > > /* rename vs. typechange */ > > - die("Not yet implemented"); > > + /* Mark the original as resolved by removal */ > > + memcpy(&oldinfo->stages[0].oid, &null_oid, > > + sizeof(oldinfo->stages[0].oid)); > > + oldinfo->stages[0].mode = 0; > > + oldinfo->filemask &= 0x06; > > This matches your explanation in the comment above. I wonder if 0x06 > could be less magical, but we are really deep in the weeds here already. > > Thanks, > -Stolee >
diff --git a/merge-ort.c b/merge-ort.c index 9aac33c8e31..11e33f56edf 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -778,7 +778,32 @@ static int process_renames(struct merge_options *opt, S_ISREG(newinfo->stages[target_index].mode)); if (type_changed && collision) { /* special handling so later blocks can handle this */ - die("Not yet implemented"); + /* + * if type_changed && collision are both true, then this + * was really a double rename, but one side wasn't + * detected due to lack of break detection. I.e. + * something like + * orig: has normal file 'foo' + * side1: renames 'foo' to 'bar', adds 'foo' symlink + * side2: renames 'foo' to 'bar' + * In this case, the foo->bar rename on side1 won't be + * detected because the new symlink named 'foo' is + * there and we don't do break detection. But we detect + * this here because we don't want to merge the content + * of the foo symlink with the foo->bar file, so we + * have some logic to handle this special case. The + * easiest way to do that is make 'bar' on side1 not + * be considered a colliding file but the other part + * of a normal rename. If the file is very different, + * well we're going to get content merge conflicts + * anyway so it doesn't hurt. And if the colliding + * file also has a different type, that'll be handled + * by the content merge logic in process_entry() too. + * + * See also t6430, 'rename vs. rename/symlink' + */ + collision = 0; + } if (source_deleted) { if (target_index == 1) { rename_branch = opt->branch1; @@ -858,7 +883,11 @@ static int process_renames(struct merge_options *opt, newinfo->pathnames[0] = oldpath; if (type_changed) { /* rename vs. typechange */ - die("Not yet implemented"); + /* Mark the original as resolved by removal */ + memcpy(&oldinfo->stages[0].oid, &null_oid, + sizeof(oldinfo->stages[0].oid)); + oldinfo->stages[0].mode = 0; + oldinfo->filemask &= 0x06; } else if (source_deleted) { /* rename/delete */ newinfo->path_conflict = 1;