[v5,0/8] mv: fix out-of-cone file/directory move logic

Message

Shaoxuan Yuan June 30, 2022, 2:37 a.m. UTC

## Changes since PATCH v4 ##

1. Fix style-nits.

2. Add t1092 tests (2/8) for "mv: add check_dir_in_index() and solve 
   general dir check issue" (8/8).

## Changes since WIP v3 ##

1. Fix style-nits.

2. Move "mv: update sparsity after moving from out-of-cone to in-cone" to the 
   (2/7) position (was (7/7) position in WIP v3). Make this move so that we
   can drop "git sparse-checkout reapply" from the tests, as suggested by
   Victoria [1]. The reason is we need to check out the moved cache_entry
   for all out-of-cone to in-cone moves, so this commit works better if being the
   first one.

3. Fix the commit message of "mv: use flags mode for update_mode", as suggested
   here [2].

4. Add "Helped-by" and "Suggested-by" trailers.

5. In "mv: add check_dir_in_index() and solve general dir check issue", change
   the 'check_dir_in_index()' to no more accept 'namelen' as argument. The
   original "namelen + 1" logic can be erroneous, for example when 'name' already
   has a trailing slash. So just use strlen() to save the trouble.

[1] https://lore.kernel.org/git/[e-mail]/
[2] https://lore.kernel.org/git/[e-mail]/

## Changes since WIP v2 ##

1. Write helper functions for t7002 to reuse some code.

2. Refactor/decouple the if/else-if checking chain.

3. Separate out the 'update_mode' refactor into a single commit.

4. Stop using update_sparsity() and instead update the SKIP_WORKTREE
   bit for each cache_entry and check it out to the working tree.

## Changes since WIP v1 ##

1. Move t7002 tests to the front and turn corresponding tests to 
   test_expect_success along with corresponding commits.

2. Add two tests to t7002.

3. Update check_dir_in_index() and added corresponding documentation.

4. Turn update_mode into enum flags.

5. Use update_sparsity() to replace advise*() function after touching
   sparse contents (this change is INCOMPLETE, NEED FIX).

6. Fix some format issues.

## Starting WIP v1 ##

Before integrating 'mv' with sparse-index, I still find some possibly buggy
UX when 'mv' is interacting with 'sparse-checkout'. 

So I kept sparse-index off in order to sort things out without a sparse index.
We can proceed to integrate with sparse-index once these changes are solid.

Note that this patch is tentative, and still have known glitches, but it 
illustrates a general approach that I intended to harmonize 'mv' 
with 'sparse-checkout'.

## Pre WIP v1 discussion ##

RFC patch about integarting sparse index with 'mv' [3]

[3] https://lore.kernel.org/git/[e-mail]/

Shaoxuan Yuan (8):
  t7002: add tests for moving out-of-cone file/directory
  t1092: mv directory from out-of-cone to in-cone
  mv: update sparsity after moving from out-of-cone to in-cone
  mv: decouple if/else-if checks using goto
  mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit
  mv: check if <destination> exists in index to handle overwriting
  mv: use flags mode for update_mode
  mv: add check_dir_in_index() and solve general dir check issue

 builtin/mv.c                             | 239 +++++++++++++++++------
 t/t1092-sparse-checkout-compatibility.sh |  25 +++
 t/t7002-mv-sparse-checkout.sh            |  84 ++++++++
 3 files changed, 284 insertions(+), 64 deletions(-)

Range-diff against v4:
1:  6ec0fee96c = 1:  c2715ffd84 t7002: add tests for moving out-of-cone file/directory
-:  ---------- > 2:  7069755abd t1092: mv directory from out-of-cone to in-cone
2:  0887fbbc82 ! 3:  33188b9f1c mv: update sparsity after moving from out-of-cone to in-cone
    @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
     +			active_cache[dst_pos]->ce_flags &= ~CE_SKIP_WORKTREE;
     +
     +			if (checkout_entry(active_cache[dst_pos], &state, NULL, NULL))
    -+				die(_("cannot checkout %s"), ce->name);
    ++				die(_("cannot checkout %s"), active_cache[dst_pos]->name);
     +		}
      	}
      
3:  e79785487e = 4:  6100d6ca40 mv: decouple if/else-if checks using goto
4:  bdbfd90843 = 5:  c34e2871d1 mv: check if out-of-cone file exists in index with SKIP_WORKTREE bit
5:  807de09f19 = 6:  e4a0ba40df mv: check if <destination> exists in index to handle overwriting
6:  af84df7dd0 ! 7:  7bc5cddcf4 mv: use flags mode for update_mode
    @@ Metadata
      ## Commit message ##
         mv: use flags mode for update_mode
     
    -    As suggested by Derrick [1],
    -    move the in-line definition of "enum update_mode" to the top
    -    of the file and make it use "flags" mode (each state is a different
    -    bit in the word).
    +    As suggested by Derrick [1], move the in-line definition of
    +    "enum update_mode" to the top of the file and make it use "flags"
    +    mode (each state is a different bit in the word).
     
         Change the flag assignments from '=' (single assignment) to '|='
         (additive). Also change flag evaluation from '==' to '&', etc.
7:  d9fd1c452c ! 8:  9f89faa557 mv: add check_dir_in_index() and solve general dir check issue
    @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
      
      		pos = cache_name_pos(src, strlen(src));
     
    + ## t/t1092-sparse-checkout-compatibility.sh ##
    +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'checkout behaves oddly with df-conflict-2' '
    + 	test_cmp full-checkout-err sparse-index-err
    + '
    + 
    +-test_expect_failure 'mv directory from out-of-cone to in-cone' '
    ++test_expect_success 'mv directory from out-of-cone to in-cone' '
    + 	init_repos &&
    + 
    + 	# <source> as a sparse directory (or SKIP_WORKTREE_DIR without enabling
    +
      ## t/t7002-mv-sparse-checkout.sh ##
     @@ t/t7002-mv-sparse-checkout.sh: test_expect_success 'refuse to move file to non-skip-worktree sparse path' '
      	test_cmp expect stderr

base-commit: e4a4b31577c7419497ac30cebe30d755b97752c5

Comments

Derrick Stolee July 1, 2022, 7:43 p.m. UTC | #1

On 6/29/2022 10:37 PM, Shaoxuan Yuan wrote:
> ## Changes since PATCH v4 ##
> 
> 1. Fix style-nits.
> 
> 2. Add t1092 tests (2/8) for "mv: add check_dir_in_index() and solve 
>    general dir check issue" (8/8).

Thank you for these updates. I just took a quick
re-read and I'm happy with this version.

Thanks,
-Stolee

Junio C Hamano July 1, 2022, 9:50 p.m. UTC | #2

Derrick Stolee <[e-mail]> writes:

> On 6/29/2022 10:37 PM, Shaoxuan Yuan wrote:
>> ## Changes since PATCH v4 ##
>> 
>> 1. Fix style-nits.
>> 
>> 2. Add t1092 tests (2/8) for "mv: add check_dir_in_index() and solve 
>>    general dir check issue" (8/8).
>
> Thank you for these updates. I just took a quick
> re-read and I'm happy with this version.
>
> Thanks,
> -Stolee

Thanks, all.  Will queue.