Re: [PATCH v7 27/31] merge-recursive: fix overwriting dirty files involved in renames

[Elijah Newren]

On Mon, Feb 5, 2018 at 12:52 PM, Stefan Beller  wrote:
> On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:
>> This fixes an issue that existed before my directory rename detection
>> patches that affects both normal renames and renames implied by
>> directory rename detection.  Additional codepaths that only affect
>> overwriting of directy files that are involved in directory rename

Ugh, "dirty" not "directy".  I must have gotten my fingers trained to
type "directory" too much.  I'll fix that up.

>> detection will be added in a subsequent commit.
>>
>> Signed-off-by: Elijah Newren 
>
> So this fixes bugs in the current rename detection with
> overwriting untracked files? Then this is an additional
> selling point of this series, maybe worth covering in the
> cover letter!

Yes, with a nitpick: the existing issue it fixes is with dirty files
(by which I mean uncommitted changes to tracked files) involved in
renames rather than being an issue with untracked files.

I did mention this fix in my original cover letter[1], but it would
have been really easy to miss because it was a really long cover
letter, and the mention came at the very end.  Quoting from it:

"""
These last three deal with untracked and dirty file overwriting
headaches.  The middle patch in particular, isn't just a fix for
directory rename detection but fixes a bug in current versions of git
in overwriting dirty files that are involved in a rename.  That patch
could be backported and submitted independent of this series, but the
final patch depends heavily on it.
"""

[1] https://public-inbox.org/git/20171110190550.27059-1-new...@gmail.com/

Re: [PATCH v7 27/31] merge-recursive: fix overwriting dirty files involved in renames

[Stefan Beller]

On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:
> This fixes an issue that existed before my directory rename detection
> patches that affects both normal renames and renames implied by
> directory rename detection.  Additional codepaths that only affect
> overwriting of directy files that are involved in directory rename
> detection will be added in a subsequent commit.
>
> Signed-off-by: Elijah Newren 

So this fixes bugs in the current rename detection with
overwriting untracked files? Then this is an additional
selling point of this series, maybe worth covering in the
cover letter!

[PATCH v7 27/31] merge-recursive: fix overwriting dirty files involved in renames

[Elijah Newren]

This fixes an issue that existed before my directory rename detection
patches that affects both normal renames and renames implied by
directory rename detection.  Additional codepaths that only affect
overwriting of directy files that are involved in directory rename
detection will be added in a subsequent commit.

Signed-off-by: Elijah Newren 
---
 merge-recursive.c   | 85 -
 merge-recursive.h   |  2 +
 t/t3501-revert-cherry-pick.sh   |  2 +-
 t/t6043-merge-rename-directories.sh |  2 +-
 t/t7607-merge-overwrite.sh  |  2 +-
 unpack-trees.c  |  4 +-
 unpack-trees.h  |  4 ++
 7 files changed, 77 insertions(+), 24 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 39e161e094..fba1a0d207 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -334,32 +334,37 @@ static void init_tree_desc_from_tree(struct tree_desc 
*desc, struct tree *tree)
init_tree_desc(desc, tree->buffer, tree->size);
 }
 
-static int git_merge_trees(int index_only,
+static int git_merge_trees(struct merge_options *o,
   struct tree *common,
   struct tree *head,
   struct tree *merge)
 {
int rc;
struct tree_desc t[3];
-   struct unpack_trees_options opts;
 
-   memset(, 0, sizeof(opts));
-   if (index_only)
-   opts.index_only = 1;
+   memset(>unpack_opts, 0, sizeof(o->unpack_opts));
+   if (o->call_depth)
+   o->unpack_opts.index_only = 1;
else
-   opts.update = 1;
-   opts.merge = 1;
-   opts.head_idx = 2;
-   opts.fn = threeway_merge;
-   opts.src_index = _index;
-   opts.dst_index = _index;
-   setup_unpack_trees_porcelain(, "merge");
+   o->unpack_opts.update = 1;
+   o->unpack_opts.merge = 1;
+   o->unpack_opts.head_idx = 2;
+   o->unpack_opts.fn = threeway_merge;
+   o->unpack_opts.src_index = _index;
+   o->unpack_opts.dst_index = _index;
+   setup_unpack_trees_porcelain(>unpack_opts, "merge");
 
init_tree_desc_from_tree(t+0, common);
init_tree_desc_from_tree(t+1, head);
init_tree_desc_from_tree(t+2, merge);
 
-   rc = unpack_trees(3, t, );
+   rc = unpack_trees(3, t, >unpack_opts);
+   /*
+* unpack_trees NULLifies src_index, but it's used in verify_uptodate,
+* so set to the new index which will usually have modification
+* timestamp info copied over.
+*/
+   o->unpack_opts.src_index = _index;
cache_tree_free(_cache_tree);
return rc;
 }
@@ -792,6 +797,20 @@ static int would_lose_untracked(const char *path)
return !was_tracked(path) && file_exists(path);
 }
 
+static int was_dirty(struct merge_options *o, const char *path)
+{
+   struct cache_entry *ce;
+   int dirty = 1;
+
+   if (o->call_depth || !was_tracked(path))
+   return !dirty;
+
+   ce = cache_file_exists(path, strlen(path), ignore_case);
+   dirty = (ce->ce_stat_data.sd_mtime.sec > 0 &&
+verify_uptodate(ce, >unpack_opts) != 0);
+   return dirty;
+}
+
 static int make_room_for_path(struct merge_options *o, const char *path)
 {
int status, i;
@@ -2654,6 +2673,7 @@ static int handle_modify_delete(struct merge_options *o,
 
 static int merge_content(struct merge_options *o,
 const char *path,
+int file_in_way,
 struct object_id *o_oid, int o_mode,
 struct object_id *a_oid, int a_mode,
 struct object_id *b_oid, int b_mode,
@@ -2728,7 +2748,7 @@ static int merge_content(struct merge_options *o,
return -1;
}
 
-   if (df_conflict_remains) {
+   if (df_conflict_remains || file_in_way) {
char *new_path;
if (o->call_depth) {
remove_file_from_cache(path);
@@ -2762,6 +2782,30 @@ static int merge_content(struct merge_options *o,
return mfi.clean;
 }
 
+static int conflict_rename_normal(struct merge_options *o,
+ const char *path,
+ struct object_id *o_oid, unsigned int o_mode,
+ struct object_id *a_oid, unsigned int a_mode,
+ struct object_id *b_oid, unsigned int b_mode,
+ struct rename_conflict_info *ci)
+{
+   int clean_merge;
+   int file_in_the_way = 0;
+
+   if (was_dirty(o, path)) {
+   file_in_the_way = 1;
+   output(o, 1, _("Refusing to lose dirty file at %s"), path);
+   }
+
+   /* Merge the content and write it out */
+   clean_merge = merge_content(o, path, file_in_the_way,
+