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

From: Derrick Stolee <stolee@gmail.com>
Date: Mar 18, 2021, 13:38 UTC
Message-ID: <70fe8d2e-d648-a064-bd8a-9e0b813980a0@gmail.com>
In-Reply-To: <1de9bc8a-b0fd-596a-3ae2-2e055b37534b@gmail.com>

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

Show 29 quoted lines

> On 3/17/2021 9:43 AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Tue, Mar 16 2021, Derrick Stolee via GitGitGadget wrote:
>>> @@ -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...
>>
>>>  	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