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

From: Victoria Dye <vdye@github.com>
Date: Jul 19, 2022, 18:01 UTC
Message-ID: <6188fab7-df5e-b3fb-92a2-47039254ac45@github.com>
In-Reply-To: <20220719132809.409247-3-shaoxuan.yuan02@gmail.com>

Shaoxuan Yuan wrote:

Show 23 quoted lines

> 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')

>   */
>  static int check_dir_in_index(const char *name)
>  {