Re: [PATCH v7 28/31] merge-recursive: fix remaining directory rename + dirty overwrite cases

[Elijah Newren]

On Mon, Feb 5, 2018 at 1:52 PM, Stefan Beller  wrote:
> On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:

>> +   /*
>> +* Stupid double negatives in remove_file; it somehow manages
>> +* to repeatedly mess me up.  So, just for myself:
>> +*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
>> +*/
>
> Not sure iff this comment is at the right place and is a good addition to
> the code base.

Fair enough, and I should apologize for letting my frustration come
through there.  However, what if I replaced the first two lines of the
comment with:

"Because the double negatives somehow keep confusing me..."

so that it reads:
/*
 * 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
 */

Even if my wording was suboptimal, the rest of the comment did seem
pretty important because I messed up the line after the comment
multiple times.  (You'd think that the odds of getting it right should
be 50/50 and that a simple inversion would fix it, so one could only
mess the line up once, but I'm apparently special).  And then I came
back to look at it later and was still confused.  For some reason, I
seem to need the longer explanation.

> However it hints at the underlying issue of a bad API that is provided
> by remove_file ?

I'd definitely agree with that.

Re: [PATCH v7 28/31] merge-recursive: fix remaining directory rename + dirty overwrite cases

[Stefan Beller]

On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:
> Signed-off-by: Elijah Newren 
> ---
>  merge-recursive.c   | 26 +++---
>  t/t6043-merge-rename-directories.sh |  8 
>  2 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index fba1a0d207..62e4266d21 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1320,11 +1320,23 @@ static int handle_file(struct merge_options *o,
>
> 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;
>
> -   remove_file(o, 0, rename->path, 0);
> +   if (ren_src_was_dirty) {
> +   output(o, 1, _("Refusing to lose dirty file at %s"),
> +  rename->path);
> +   }
> +   /*
> +* Stupid double negatives in remove_file; it somehow manages
> +* to repeatedly mess me up.  So, just for myself:
> +*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
> +*/

Not sure iff this comment is at the right place and is a good addition to
the code base.

However it hints at the underlying issue of a bad API that is provided
by remove_file ?

[PATCH v7 28/31] merge-recursive: fix remaining directory rename + dirty overwrite cases

[Elijah Newren]

Signed-off-by: Elijah Newren 
---
 merge-recursive.c   | 26 +++---
 t/t6043-merge-rename-directories.sh |  8 
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index fba1a0d207..62e4266d21 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1320,11 +1320,23 @@ static int handle_file(struct merge_options *o,
 
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;
 
-   remove_file(o, 0, rename->path, 0);
+   if (ren_src_was_dirty) {
+   output(o, 1, _("Refusing to lose dirty file at %s"),
+  rename->path);
+   }
+   /*
+* Stupid double negatives in remove_file; it somehow manages
+* to repeatedly mess me up.  So, just for myself:
+*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)) {
@@ -1462,7 +1474,10 @@ static int conflict_rename_rename_2to1(struct 
merge_options *o,
char *new_path2 = unique_path(o, path, ci->branch2);
output(o, 1, _("Renaming %s to %s and %s to %s instead"),
   a->path, new_path1, b->path, new_path2);
-   if (would_lose_untracked(path))
+   if (was_dirty(o, path))
+   output(o, 1, _("Refusing to lose dirty file at %s"),
+  path);
+   else if (would_lose_untracked(path))
/*
 * Only way we get here is if both renames were from
 * a directory rename AND user had an untracked file
@@ -2042,6 +2057,7 @@ static void apply_directory_rename_modifications(struct 
merge_options *o,
 {
struct string_list_item *item;
int stage = (tree == a_tree ? 2 : 3);
+   int update_wd;
 
/*
 * In all cases where we can do directory rename detection,
@@ -2052,7 +2068,11 @@ static void apply_directory_rename_modifications(struct 
merge_options *o,
 * saying the file would have been overwritten), but it might
 * be dirty, though.
 */
-   remove_file(o, 1, pair->two->path, 0 /* no_wd */);
+   update_wd = !was_dirty(o, pair->two->path);
+   if (!update_wd)
+   output(o, 1, _("Refusing to lose dirty file at %s"),
+  pair->two->path);
+   remove_file(o, 1, pair->two->path, !update_wd);
 
/* Find or create a new re->dst_entry */
item = string_list_lookup(entries, new_path);
diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 89b2eacf38..a34c57d986 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3362,7 +3362,7 @@ test_expect_success '11b-setup: Avoid losing dirty file 
involved in directory re
)
 '
 
-test_expect_failure '11b-check: Avoid losing dirty file involved in directory 
rename' '
+test_expect_success '11b-check: Avoid losing dirty file involved in directory 
rename' '
(
cd 11b &&
 
@@ -3504,7 +3504,7 @@ test_expect_success '11d-setup: Avoid losing not-uptodate 
with rename + D/F conf
)
 '
 
-test_expect_failure '11d-check: Avoid losing not-uptodate with rename + D/F 
conflict' '
+test_expect_success '11d-check: Avoid losing not-uptodate with rename + D/F 
conflict' '
(
cd 11d &&
 
@@ -3583,7 +3583,7 @@ test_expect_success '11e-setup: Avoid deleting 
not-uptodate with dir rename/rena
)
 '
 
-test_expect_failure '11e-check: Avoid deleting not-uptodate with dir 
rename/rename(1to2)/add' '
+test_expect_success '11e-check: Avoid deleting not-uptodate with dir 
rename/rename(1to2)/add' '
(
cd 11e &&
 
@@ -3659,7 +3659,7 @@ test_expect_success '11f-setup: Avoid deleting 
not-uptodate with dir rename/rena
)
 '
 
-test_expect_failure '11f-check: Avoid deleting not-uptodate with dir 
rename/rename(2to1)' '
+test_expect_success '11f-check: Avoid deleting not-uptodate with dir 
rename/rename(2to1)' '
(
cd 11f &&
 
-- 
2.16.1.106.gf69932adfe