Re: [PATCH v3 11/20] sparse-index: convert from full to sparse

On 3/17/2021 3:55 PM, Derrick Stolee wrote:

On 3/17/2021 9:43 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

On Tue, Mar 16 2021, Derrick Stolee via GitGitGadget wrote:

quoted

@@ -251,6 +251,8 @@ static inline unsigned int create_ce_mode(unsigned int mode)
 {
 	if (S_ISLNK(mode))
 		return S_IFLNK;
+	if (mode == S_IFDIR)
+		return S_IFDIR;

Does this actually need to be mode == S_IFDIR v.s. S_ISDIR(mode)? Those
aren't the same thing...

quoted

 	if (S_ISDIR(mode) || S_ISGITLINK(mode))
 		return S_IFGITLINK;

...and if it can be S_ISDIR(mode) then this becomes just
S_ISGITLINK(mode), but losing the "if" there makes me suspect that some
dir == submodule heuristic is being broken somewhere..

 
I have a vague recollection that I did that at one point, and
it didn't work. However, using the simpler

	if (S_ISDIR(mode))
		return S_IFDIR;
	if (S_ISGITLINK(mode))
		return S_IFGITLINK;

passes all of my tests.

I'm not sure why it was passing yesterday (maybe I was in the
wrong worktree) but I _do_ get failures, such as this one in t2105:

expecting success of 2105.4 'add gitlink to relative .git file': 
        git update-index --add -- sub2

+ git update-index --add -- sub2
warning: index entry is a directory, but not sparse (00000000)
error: Could not read 50e526bb426771f6036ad3a8b0c81d511d91fc2a
BUG: read-cache.c:324: unsupported ce_mode: 40000
Aborted (core dumped)
error: last command exited with $?=134
not ok 4 - add gitlink to relative .git file
#
#               git update-index --add -- sub2
#

In this case, the mode that is specified is equal to 040775,
so we need to use the permission bits outside of __S_IFMT
(0170000) to determine if this is a sparse directory or a
submodule entry. Submodules will never be sparse, so
permissions matter. Sparse directories never actually exist,
so permissions don't matter.

Playing around with it, I still only see the exact equality
as working for me.

I can, however, use this format for these if statements:

	if (S_ISSPARSEDIR(mode))
		return S_IFDIR;
	if (S_ISDIR(mode) || S_ISGITLINK(mode))
		return S_IFGITLINK;

The S_ISSPARSEDIR macro expands to the exact equality.

Now, if we intended to make this work differently, then a
change would be required to construct_sparse_dir_entry()
in sparse-index.c:

static struct cache_entry *construct_sparse_dir_entry(
				struct index_state *istate,
				const char *sparse_dir,
				struct cache_tree *tree)
{
	struct cache_entry *de;

	de = make_cache_entry(istate, S_IFDIR, &tree->oid, sparse_dir, 0, 0);

	de->ce_flags |= CE_SKIP_WORKTREE;
	return de;
}

For instance, we could at this point assign de->ce_mode to
be S_IFDIR directly. It seems like the wrong place to do that
to me, but I'm open to suggestions.

Thanks,
-Stolee