Re: [PATCH v3 8/8] merge-recursive: improve rename/rename(1to2)/add[/add] handling
On 11/2/2018 1:27 PM, Elijah Newren wrote:
On Thu, Nov 1, 2018 at 12:01 AM Elijah Newren wrote:
On Wed, Oct 31, 2018 at 8:08 AM Derrick Stolee wrote:
On 10/19/2018 3:31 PM, Elijah Newren wrote:
[snip]
+ char *new_path = NULL;
+ if (dir_in_way(b->path, !o->call_depth, 0)) {
+ new_path = unique_path(o, b->path, ci->branch2);
+ output(o, 1, _("%s is a directory in %s adding "
+"as %s instead"),
+b->path, ci->branch1, new_path);
I tried really hard, but failed to get a test to cover the block below.
I was able to
find that the "check handling of differently renamed file with D/F
conflicts" test
in t6022-merge-rename.sh covers the block above. Trying to tweak the
example using
untracked files seems to hit an error message from unpack-trees.c instead.
+ } else if (would_lose_untracked(b->path)) {
+ new_path = unique_path(o, b->path, ci->branch2);
+ output(o, 1, _("Refusing to lose untracked file"
+" at %s; adding as %s instead"),
+b->path, new_path);
So now I'm confused. This block was not listed in your coverage
report[1]. And, in fact, I think this block IS covered by testcase
10c of t6043. However, there is a very similar looking block about 30
lines up that is uncovered (and which was mentioned in your report):
} else if (would_lose_untracked(a->path)) {
new_path = unique_path(o, a->path, ci->branch1);
output(o, 1, _("Refusing to lose untracked file"
" at %s; adding as %s instead"),
a->path, new_path);
covering it, I think, is just a matter of repeating the 10c test with
the merge repeated in the other direction (checkout B and merge A
instead of checking out A and merging B) -- and touching up the checks
accordingly.
However, now I'm wondering if I'm crazy. Was it really the block you
had highlighted that you were seeing uncovered?
Trust the report (generated by computer) over me (generated by squinting
at an email, trying to match line numbers).
Thanks,
-Stolee
Re: [PATCH v3 8/8] merge-recursive: improve rename/rename(1to2)/add[/add] handling
On Thu, Nov 1, 2018 at 12:01 AM Elijah Newren wrote:
>
> On Wed, Oct 31, 2018 at 8:08 AM Derrick Stolee wrote:
> >
> > On 10/19/2018 3:31 PM, Elijah Newren wrote:
> > > [snip]
> > >
> > > + char *new_path = NULL;
> > > + if (dir_in_way(b->path, !o->call_depth, 0)) {
> > > + new_path = unique_path(o, b->path,
> > > ci->branch2);
> > > + output(o, 1, _("%s is a directory in %s
> > > adding "
> > > +"as %s instead"),
> > > +b->path, ci->branch1, new_path);
> >
> > I tried really hard, but failed to get a test to cover the block below.
> > I was able to
> > find that the "check handling of differently renamed file with D/F
> > conflicts" test
> > in t6022-merge-rename.sh covers the block above. Trying to tweak the
> > example using
> > untracked files seems to hit an error message from unpack-trees.c instead.
> >
> > > + } else if (would_lose_untracked(b->path)) {
> > > + new_path = unique_path(o, b->path,
> > > ci->branch2);
> > > + output(o, 1, _("Refusing to lose untracked
> > > file"
> > > +" at %s; adding as %s
> > > instead"),
> > > +b->path, new_path);
> >
So now I'm confused. This block was not listed in your coverage
report[1]. And, in fact, I think this block IS covered by testcase
10c of t6043. However, there is a very similar looking block about 30
lines up that is uncovered (and which was mentioned in your report):
} else if (would_lose_untracked(a->path)) {
new_path = unique_path(o, a->path, ci->branch1);
output(o, 1, _("Refusing to lose untracked file"
" at %s; adding as %s instead"),
a->path, new_path);
covering it, I think, is just a matter of repeating the 10c test with
the merge repeated in the other direction (checkout B and merge A
instead of checking out A and merging B) -- and touching up the checks
accordingly.
However, now I'm wondering if I'm crazy. Was it really the block you
had highlighted that you were seeing uncovered?
Thanks,
Elijah
[1] https://public-inbox.org/git/62f0bcf6-aa73-c192-d804-e6d69cac1...@gmail.com/
Re: [PATCH v3 8/8] merge-recursive: improve rename/rename(1to2)/add[/add] handling
On Wed, Oct 31, 2018 at 8:08 AM Derrick Stolee wrote:
>
> On 10/19/2018 3:31 PM, Elijah Newren wrote:
> > [snip]
> >
> > + char *new_path = NULL;
> > + if (dir_in_way(b->path, !o->call_depth, 0)) {
> > + new_path = unique_path(o, b->path,
> > ci->branch2);
> > + output(o, 1, _("%s is a directory in %s
> > adding "
> > +"as %s instead"),
> > +b->path, ci->branch1, new_path);
>
> I tried really hard, but failed to get a test to cover the block below.
> I was able to
> find that the "check handling of differently renamed file with D/F
> conflicts" test
> in t6022-merge-rename.sh covers the block above. Trying to tweak the
> example using
> untracked files seems to hit an error message from unpack-trees.c instead.
>
> > + } else if (would_lose_untracked(b->path)) {
> > + new_path = unique_path(o, b->path,
> > ci->branch2);
> > + output(o, 1, _("Refusing to lose untracked
> > file"
> > +" at %s; adding as %s
> > instead"),
> > +b->path, new_path);
>
> It could also be that I failed because I'm less familiar with this part
> of the
> codebase. Elijah, do you think it is possible to hit this block?
Yeah, this one's going to be a little harder; the upper block would be
done with a D/F, but I think for this block you'd need a directory
rename so that unpack-trees.c can't tell that the untracked file in
the way is actually in the way of anything. But since this is in the
rename/rename(1to2) area, I think I had rules around avoiding doing
directory renames if the other side renamed the file to avoid getting
into rename/rename(1to3) situations and other weirdness. So, it might
require a transitive rename (i.e. file renamed on both sides, and on
one side it's renamed into a directory that the other side renamed
away).
I'll try to take a look at it tomorrow, with everything else. We'll
see how much I can get done.
Thanks for digging in to all these and bringing them up.
Re: [PATCH v3 8/8] merge-recursive: improve rename/rename(1to2)/add[/add] handling
On 10/19/2018 3:31 PM, Elijah Newren wrote:
[snip]
+ char *new_path = NULL;
+ if (dir_in_way(b->path, !o->call_depth, 0)) {
+ new_path = unique_path(o, b->path, ci->branch2);
+ output(o, 1, _("%s is a directory in %s adding "
+ "as %s instead"),
+ b->path, ci->branch1, new_path);
I tried really hard, but failed to get a test to cover the block below.
I was able to
find that the "check handling of differently renamed file with D/F
conflicts" test
in t6022-merge-rename.sh covers the block above. Trying to tweak the
example using
untracked files seems to hit an error message from unpack-trees.c instead.
+ } else if (would_lose_untracked(b->path)) {
+ new_path = unique_path(o, b->path, ci->branch2);
+ output(o, 1, _("Refusing to lose untracked file"
+ " at %s; adding as %s instead"),
+ b->path, new_path);
It could also be that I failed because I'm less familiar with this part
of the
codebase. Elijah, do you think it is possible to hit this block?
Thanks,
-Stolee
[PATCH v3 8/8] merge-recursive: improve rename/rename(1to2)/add[/add] handling
When we have a rename/rename(1to2) conflict, each of the renames can
collide with a file addition. Each of these rename/add conflicts suffered
from the same kinds of problems that normal rename/add suffered from.
Make the code use handle_file_conflicts() as well so that we get all the
same fixes and consistent behavior between the different conflict types.
Signed-off-by: Elijah Newren
---
merge-recursive.c| 154 +--
t/t6042-merge-rename-corner-cases.sh | 29 +++--
t/t6043-merge-rename-directories.sh | 24 +++--
3 files changed, 113 insertions(+), 94 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index c78b347112..5986b6 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1709,80 +1709,17 @@ static int handle_rename_add(struct merge_options *o,
ci->dst_entry1->stages[other_stage].mode);
}
-static int handle_file(struct merge_options *o,
- struct diff_filespec *rename,
- int stage,
- struct rename_conflict_info *ci)
-{
- char *dst_name = rename->path;
- struct stage_data *dst_entry;
- const char *cur_branch, *other_branch;
- struct diff_filespec other;
- struct diff_filespec *add;
- int ret;
-
- if (stage == 2) {
- dst_entry = ci->dst_entry1;
- cur_branch = ci->branch1;
- other_branch = ci->branch2;
- } else {
- dst_entry = ci->dst_entry2;
- cur_branch = ci->branch2;
- other_branch = ci->branch1;
- }
-
- add = filespec_from_entry(, dst_entry, stage ^ 1);
- if (add) {
- int ren_src_was_dirty = was_dirty(o, rename->path);
- char *add_name = unique_path(o, rename->path, other_branch);
- if (update_file(o, 0, >oid, add->mode, add_name))
- return -1;
-
- if (ren_src_was_dirty) {
- output(o, 1, _("Refusing to lose dirty file at %s"),
- rename->path);
- }
- /*
-* Because the double negatives somehow keep confusing me...
-*1) update_wd iff !ren_src_was_dirty.
-*2) no_wd iff !update_wd
-*3) so, no_wd == !!ren_src_was_dirty == ren_src_was_dirty
-*/
- remove_file(o, 0, rename->path, ren_src_was_dirty);
- dst_name = unique_path(o, rename->path, cur_branch);
- } else {
- if (dir_in_way(rename->path, !o->call_depth, 0)) {
- dst_name = unique_path(o, rename->path, cur_branch);
- output(o, 1, _("%s is a directory in %s adding as %s
instead"),
- rename->path, other_branch, dst_name);
- } else if (!o->call_depth &&
- would_lose_untracked(rename->path)) {
- dst_name = unique_path(o, rename->path, cur_branch);
- output(o, 1, _("Refusing to lose untracked file at %s; "
- "adding as %s instead"),
- rename->path, dst_name);
- }
- }
- if ((ret = update_file(o, 0, >oid, rename->mode, dst_name)))
- ; /* fall through, do allow dst_name to be released */
- else if (stage == 2)
- ret = update_stages(o, rename->path, NULL, rename, add);
- else
- ret = update_stages(o, rename->path, NULL, add, rename);
-
- if (dst_name != rename->path)
- free(dst_name);
-
- return ret;
-}
-
static int handle_rename_rename_1to2(struct merge_options *o,
struct rename_conflict_info *ci)
{
/* One file was renamed in both branches, but to different names. */
+ struct merge_file_info mfi;
+ struct diff_filespec other;
+ struct diff_filespec *add;
struct diff_filespec *one = ci->pair1->one;
struct diff_filespec *a = ci->pair1->two;
struct diff_filespec *b = ci->pair2->two;
+ char *path_desc;
output(o, 1, _("CONFLICT (rename/rename): "
"Rename \"%s\"->\"%s\" in branch \"%s\" "
@@ -1790,15 +1727,16 @@ static int handle_rename_rename_1to2(struct
merge_options *o,
one->path, a->path, ci->branch1,
one->path, b->path, ci->branch2,
o->call_depth ? _(" (left unresolved)") : "");
- if (o->call_depth) {
- struct merge_file_info mfi;
- struct diff_filespec other;
- struct diff_filespec *add;
- if (merge_mode_and_contents(o, one, a, b, one->path,
- ci->branch1, ci->branch2,
- o->call_depth * 2, ))
-
