Re: [PATCH 1/1] attr: add native file mode values support

From: Joanna Wang <jojwang@google.com>
Date: Nov 14, 2023, 02:28 UTC
Message-ID: <CAMmZTi-7=L6XOB9-MwZCpD5QaziTi0x6J7m-UTtwQ0S00wnVgQ@mail.gmail.com>

Show 6 quoted lines

>> 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?