>> Even if we assume that this code is currently meant to work only >> with GIT_ATTR_CHECKIN, I do not think this is what you want. When >> asked to perform "git add . ':(exclude,mode=160000)'", you not only >> want to exclude the submodules that are already known to this >> superproject, but also a repository that _can_ become a submodule of >> this superproject when added, no? Sorry, I was totally ignorant of two key concepts that you mention here. I somehow missed the concept of git_attr_direction altogether and I also did not know submodules could be added with git-add (I was only familiar with how they are currently added in chromium via git update-index). This makes sense now. I'll fix in the next patch. >> On the other hand, the GIT_ATTR_CHECKOUT direction is hopefully much >> simpler. You'd see what the path in the index is, among a gitlink, >> a regular non-executable file, an executable file, or a symlink. I noticed for both the GIT_ATTR_CHECKOUT and GIT_ATTR_CHECKIN directions, in read_attr(), the indexed .gitattributes file is checked with the actual file as fallback or vice versa. I would think that we'd only want to use attributes from one state (e.g. what's actually in the file) or the other (e.g. what's indexed). So I guess I'm still not sure what the "direction" concept is. For GIT_ATTR_CHECKOUT, would we want to fallback to lstat? >> "ls-tree" documentation seems to call it %(objectmode). I can change it to 'objectmode' in a followup patch, if there are no objections to this. >> I think the idea is that "mode" being a too generic word can be used >> for totally different purposes My thinking was, no matter how generic or rare a name we choose, there is always a chance no matter how tiny, that the name will be in use in someone's .gitattributes. But if people are ok with choosing something less generic and have that become a 'reserved' attribute name and not have any existing values in .gitattributes take precedence I can do that. I just don't know how git has historically balanced breaking existing workflows vs easier/more reasonable implementation/behavior. >> But stepping back a bit, >> such an application is likely marking selected few paths with the >> attribute, and paths for which "mode" was "unset" are now given >> these natural "mode"; it is inevitable to crash with such uses. I'm confused. This does not match what I think is the current behavior of my patch. If "mode" was unset or removed for a path (meaning '<path> !mode' was added to .gitattributes), the code in my patch would respect that and not return the native mode. It would return 'unspecified' or 'unset'. I have tests for these in the patch, but if I've missed some test cases please let me know. >> Again, this has one hole, I think. Paths that are not mentioned >> (not even with "!mode") would come to the function as ATTR__UNKNOWN >> and trigger the fallback behaviour, even when other paths are given >> end-user specified "mode" attribute values. What you are describing sounds correct/what I intended. So are you saying that the expected behavior is actually: If the user sets 'mode' for 1+ paths in the repo, then the native mode fallback should NOT be used for all paths in the repo?
