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

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

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.

Looking at the history of create_ce_mode(), this "||"
condition was created in this commit:

commit 9eec4795d44439cd170fb52c73827c728252648d
Author: Linus Torvalds [off-list ref]
Date:   Mon Apr 9 21:14:58 2007 -0700

    Add "S_IFDIRLNK" file mode infrastructure for git links
    
    This just adds the basic helper functions to recognize and work with git
    tree entries that are links to other git repositories ("subprojects").
    They still aren't actually connected up to any of the code-paths, but
    now all the infrastructure is in place.
    
    The next commit will start actually adding actual subproject support.
    
    Signed-off-by: Linus Torvalds [off-list ref]
    Signed-off-by: Junio C Hamano [off-list ref]

There isn't any justification of why S_ISDIR() is there. Perhaps
it was defensive programming? If that is the case, then this simpler
logic will work.

Thanks,
-Stolee