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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 27, 2011, 20:02 UTC
Message-ID: <7vr55bo47e.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1311689582-3116-8-git-send-email-mhagger@alum.mit.edu>

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.

Show 9 quoted lines

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

Show 5 quoted lines

> 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;
	}

Show 7 quoted lines

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