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

[Junio C Hamano <gitster@pobox.com>]

Joanna Wang <jojwang@google.com> writes:

> +/*
> + * This implements native file mode attr support.
> + * We always return the current mode of the path, not the mode stored
> + * in the index, unless the path is a directory where we check the index
> + * to see if it is a GITLINK. It is ok to rely on the index for GITLINK
> + * modes because a path cannot become a GITLINK (which is a git concept only)
> + * without having it indexed with a GITLINK mode in git.
> + */
> +static unsigned int native_mode_attr(struct index_state *istate, const char *path)
> +{
> +	struct stat st;
> +	unsigned int mode;

Please have a blank line here between decls and the first statement.

> +	if (lstat(path, &st))
> +		die_errno(_("unable to stat '%s'"), path);

For checking in, this is probably OK, but don't we need to switch
between lstat() on the filesystem entity and inspecting ce_mode()
for the in-index cache_entry, depending on the git_attr_direction?

> +	mode = canon_mode(st.st_mode);
> +	if (S_ISDIR(mode)) {
> +		int pos = index_name_pos(istate, path, strlen(path));
> +		if (pos >= 0)
> +			if (S_ISGITLINK(istate->cache[pos]->ce_mode))
> +				return istate->cache[pos]->ce_mode;

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?  You are missing "if (pos < 0)"
case where you'd probably want to see if the directory at the path
looks like the top of a working tree with ".git" subdirectory that
is a valid repository, or something like that to detect such a case.

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.

> +	}
> +	return mode;
> +}
> +
> +
>  void git_check_attr(struct index_state *istate,
>  		    const char *path,
>  		    struct attr_check *check)
>  {
>  	int i;
>  	const struct object_id *tree_oid = default_attr_source();
> +	struct strbuf sb = STRBUF_INIT;
>  
>  	collect_some_attrs(istate, tree_oid, path, check);
>  
>  	for (i = 0; i < check->nr; i++) {
>  		unsigned int n = check->items[i].attr->attr_nr;
>  		const char *value = check->all_attrs[n].value;
> -		if (value == ATTR__UNKNOWN)
> -			value = ATTR__UNSET;
> +		if (value == ATTR__UNKNOWN){

Style.  Missing SP between "){".

> +			if (strcmp(check->all_attrs[n].attr->name, "mode") == 0) {

Style.  We avoid comparison with 0; so say

			if (!strcmp(check->all_attrs[n].attr->name, "mode") == 0) {

instead.

It is disturbing that we always need to perform a string comparison
in this loop (and the function is called repeatedly while traversing
the tree looking for paths that match pathspec).  The attribute objects
are designed to be interned, so at least you should be able to do

	mode_attr = git_attr("mode");

before the loop and then compare check->all_attrs[n].attr and
mode_attr as two addresses.

> +				/*
> +				 * If value is ATTR_UNKNOWN then it has not been set
> +				 * anywhere with -mode (ATTR_FALSE), !mode (ATTR_UNSET),
> +				 * mode (ATTR_TRUE), or an explicit value. We fill
> +				 * value with the native mode value.
> +				 */
> +				strbuf_addf(&sb, "%06o", native_mode_attr(istate, path));
> +				value = sb.buf;

Or even better yet, as this is not a place to write too many lines
for a single oddball attribute, it might make even more sense to
introduce a helper function and make a call to it like so:

		if (value == ATTR__UNKNOWN)
-			value = ATTR__UNSET;
+			value = compute_attr_from_path(istate, path, check_items[i].attr);

with the new helper function do all the heavy lifting, e.g.,

	static const char *compute_attr_from_path(struct index_state *istate,
						  const char *path,
						  const struct git_attr *attr) {
		static struct git_attr mode_attr;

		if (!mode_attr)
			mode_attr = git_attr("mode");

		if (attr == mode_attr) {
			call native_mode_attr(), stringify, and
			return it;
		}
		return ATTR__UNSET;
	}

which will allow us to easily add in the future special attribute
similar to "mode" whose fallback value is computed.

Otherwise, looking pretty good.  Thanks for working on this.