Re: [PATCH v2 19/19] Rename struct git_attr_check to git_attr_value
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Aug 4, 2011, 03:20 UTC
- Message-ID
- <4E3A0FFD.7010604@alum.mit.edu>
- In-Reply-To
- <7vsjpjg6k7.fsf@alter.siamese.dyndns.org>
Thanks for the good feedback.
On 08/02/2011 05:46 PM, Junio C Hamano wrote:
Show 10 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes: > >> This described its purpose better, especially when used with >> git_allattrs(). > > You probably meant s/described/describes/ but more importantly does it > really? It is a structure used to probe into the attributes system for the > state of various attributes on a path, and the set of possible states > includes "there is no value" (aka unset), so it feels actively wrong to > call it attr_value and that is why I didn't call it in the first place.
I don't think it is so unusual for a "value" object to be able to reflect the fact that the value is unset, but I can understand your point of view too. I will omit this renaming in the re-roll.
Show 10 quoted lines
> I also think git_all_attrs() (i.e. word-break underscore after "all") is > more in line with the naming throughout the codebase, after looking at > output from > > $ git grep -e _all'[a-z]' --and --not -e alloc -e _all_ -- '*.c' > > Other than these, and the earlier comment about the copy&paste done from > git_checkattr (which by the way should probably be "git_check_attr"), it > seems that the series mostly consist of good clean-ups and an addition of > a new and (probably) useful feature that is straightforward. Nice.
I thought the name was awkward, too, but I chose it to be consistent with git_checkattr(). So in the re-roll I will happily rename both functions.
Michael
-- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/