Re: [PATCH 07/19] Provide access to the name attribute of git_attr

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

Michael Haggerty <mhagger@alum.mit.edu> writes:

> It will be present in any likely future reimplementation, and its
> availability simplifies other code.

Looks good to me except for minor nits.

> struct git_attr is an opaque structure containing, among other things,
> the name of the attribute that it represents.  This patch adds a
> function git_attr_name(struct git_attr *) that allows the name to be
> read from the structure.  This functionality will be convenient for
> later patches.
>
> This seems harmless to me.  It is hardly conceivable that the
> implementation will change so dramatically that it becomes impossible
> to derive the attribute name from the git_attr.

The original design of attribute API wanted to make sure that the callers
do not have to care what _other_ attributes are on the path, and defined
only "if you have a specific name, you can ask if that is set". Of course
once you start wanting to enumerate the attributes without knowing what
attributes may exist in the world, you would need an API to grab all the
attributes and then find out the name of each of them. So I think this is
not just harmless but is necessary.

> diff --git a/attr.c b/attr.c
> ...
> +char *git_attr_name(struct git_attr *attr) {
> +	return attr->name;
> +}

(Style)

	char *git_attr_name(struct git_attr *attr)
	{
		return attr->name;
	}

> diff --git a/attr.h b/attr.h
> ...
> +/*
> + * Return the name of the attribute represented by the argument.  The
> + * return value is a pointer to a null-delimited string that is part
> + * of the internal data structure; it should not be modified or freed.
> + */

should not be modified NOR freed?