Re: [PATCH v10 25/36] merge-recursive: fix overwriting dirty files involved in renames

[Elijah Newren]

On Thu, Apr 19, 2018 at 1:48 PM, Martin Ågren  wrote:
> On 19 April 2018 at 19:58, 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 dirty files that are involved in directory rename
>> detection will be added in a subsequent commit.
>>
>> Reviewed-by: Stefan Beller 
>> Signed-off-by: Elijah Newren 
>> Signed-off-by: Junio C Hamano 
>> ---
>>  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 c1c4faf61e..7fdcba4f22 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -337,32 +337,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;
>>  }
>
> As mentioned in a reply to patch 33/36 [1], I've got a patch to add
> `clear_unpack_trees_porcelain()` which frees the resources allocated by
> `setup_unpack_trees_porcelain()`. Before this patch, I could easily call
> it at the end of this function. After this, the ownership is less
> obvious to me.

I wouldn't put the call to clear_unpack_trees_porcelain() at the end
of this function, but rather at the end of merge_trees().
merge_trees() is the only caller of git_merge_trees() and it continues
using o->unpack_opts until the end of that function.  At the end of
that function, there is no further need for o->unpack_opts.
Basically, put it right where I put the "FIXME: Need to also free data
allocated by setup_unpack_trees_porcelain()" comment.

Re: [PATCH v10 25/36] merge-recursive: fix overwriting dirty files involved in renames

[Martin Ågren]

On 19 April 2018 at 22:48, Martin Ågren  wrote:
> On 19 April 2018 at 19:58, Elijah Newren  wrote:
>> -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)
>>  {
[...]
>> +   memset(>unpack_opts, 0, sizeof(o->unpack_opts));
[...]
>> +   setup_unpack_trees_porcelain(>unpack_opts, "merge");
[...]
>>  }
>
> As mentioned in a reply to patch 33/36 [1], I've got a patch to add
> `clear_unpack_trees_porcelain()` which frees the resources allocated by
> `setup_unpack_trees_porcelain()`. Before this patch, I could easily call
> it at the end of this function. After this, the ownership is less
> obvious to me.
>
> It turns out that the only user of `unpack_opts` outside this function
> can indeed end up wanting to use the error messages that `clear_...()`
> would set out to free. So yes, the call to `clear_...()` will need to go
> elsewhere.
>
> It does sort of make me wonder if we should memset `unpack_opts` to zero
> somewhere early, so that we can then `clear_...()` it early here before
> zeroizing it. So yes, we'd be constantly allocating and freeing those
> strings. Am I right to assume that the code after your series would do
> (roughly) the same number of calls to `setup_unpack_trees_porcelain()`,
> i.e., `git_merge_trees()` as it did before?

Or, of course, both `setup_...` and `clear_...` would go outside this
function to churn less memory... Anyway, this still holds:

> All of this is arguably irrelevant for this series. It might be better
> if I clarify this memory ownership and do any adjustments as part of my
> patch (series), rather than you shuffling things around at this time.

Re: [PATCH v10 25/36] merge-recursive: fix overwriting dirty files involved in renames

[Martin Ågren]

On 19 April 2018 at 19:58, 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 dirty files that are involved in directory rename
> detection will be added in a subsequent commit.
>
> Reviewed-by: Stefan Beller 
> Signed-off-by: Elijah Newren 
> Signed-off-by: Junio C Hamano 
> ---
>  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 c1c4faf61e..7fdcba4f22 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -337,32 +337,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;
>  }

As mentioned in a reply to patch 33/36 [1], I've got a patch to add
`clear_unpack_trees_porcelain()` which frees the resources allocated by
`setup_unpack_trees_porcelain()`. Before this patch, I could easily call
it at the end of this function. After this, the ownership is less
obvious to me.

It turns out that the only user of `unpack_opts` outside this function
can indeed end up wanting to use the error messages that `clear_...()`
would set out to free. So yes, the call to `clear_...()` will need to go
elsewhere.

It does sort of make me wonder if we should memset `unpack_opts` to zero
somewhere early, so that we can then `clear_...()` it early here before
zeroizing it. So yes, we'd be constantly allocating and freeing those
strings. Am I right to assume that the code after your series would do
(roughly) the same number of calls to `setup_unpack_trees_porcelain()`,
i.e., `git_merge_trees()` as it did before?

All of this is arguably irrelevant for this series. It might be better
if I clarify this memory ownership and do any adjustments as part of my
patch (series), rather than you shuffling things around at this time.

Mostly thinking out loud. If you have any thoughts, feel free to share.

Martin

[1] 
https://public-inbox.org/git/can0hesqujbommgay+5xomqxcgohtxxf1mjbmy_l7y+aa4eg...@mail.gmail.com/

[PATCH v10 25/36] 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 dirty files that are involved in directory rename
detection will be added in a subsequent commit.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
Signed-off-by: Junio C Hamano 
---
 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 c1c4faf61e..7fdcba4f22 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -337,32 +337,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;
 }
@@ -795,6 +800,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;
@@ -2687,6 +2706,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,
@@ -2761,7 +2781,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);
@@ -2795,6 +2815,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 */
+