Re: [PATCH v1 2/7] mv: add documentation for check_dir_in_index()

[Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>]

On 7/20/2022 2:01 AM, Victoria Dye wrote:
 > Shaoxuan Yuan wrote:
 >> Using check_dir_in_index without checking if the directory is on-disk
 >> could get a false positive for partially sparsified directory.
 >>
 >> Add a note in the documentation to warn potential user.
 >>
 >> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
 >> ---
 >>  builtin/mv.c | 5 +++++
 >>  1 file changed, 5 insertions(+)
 >>
 >> diff --git a/builtin/mv.c b/builtin/mv.c
 >> index 4729bb1a1a..c8b9069db8 100644
 >> --- a/builtin/mv.c
 >> +++ b/builtin/mv.c
 >> @@ -132,6 +132,11 @@ static int index_range_of_same_dir(const char 
*src, int length,
 >>   * Return 0 if such directory exist (i.e. with any of its contained 
files not
 >>   * marked with CE_SKIP_WORKTREE, the directory would be present in 
working tree).
 >>   * Return 1 otherwise.
 >> + *
 >> + * Note: *always* check the directory is not on-disk before this 
function
 >> + * (i.e. using lstat());
 >> + * otherwise it may return a false positive for a partially sparsified
 >> + * directory.
 >
 > To me, a comment like this indicates that either the function is not 
doing
 > what it should be doing, or its name doesn't properly describe the
 > function's behavior.
 >
 > Per the function description:
 >
 >     Check if an out-of-cone directory should be in the index. Imagine
 >     this case that all the files under a directory are marked with
 >     'CE_SKIP_WORKTREE' bit and thus the directory is sparsified.
 >
 > But neither the name of the function ('check_dir_in_index') nor the
 > function's behavior (hence the comment you're adding here) match that
 > description.
 >
 > Since this function is intended to verify that a directory 1) exists 
in the
 > index, and 2) is *entirely* sparse, I have two suggestions:
 >
 > * Change the description to specify that the non-existence of the 
directory
 >   on disk is an *assumption*, not an opportunity for a "false positive."
 >   It's a subtle distinction, but it clarifies that the function should be
 >   used only when the caller already knows the directory is empty. For
 >   example:
 >
 >     /*
 >      * Given the path of a directory that does not exist on-disk, 
check whether the
 >      * directory contains any entries in the index with the 
SKIP_WORKTREE flag
 >      * enabled.
 >      *
 >      * Return 1 if such index entries exist.
 >      * Return 0 otherwise.
 >      */
 >
 > * 'check_dir_in_index' doesn't reflect the "is not on disk" 
prerequisite, so
 >   the function name can be updated to clarify that (e.g.,
 >   'empty_dir_has_sparse_contents')
 >

I really breathed a sigh of relief after seeing these two points :-)
They word things out much better than the original ones.

--
Thanks,
Shaoxuan