Re: [PATCH] Per-path attribute based hunk header selection.

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 6, 2007, 12:38 UTC
Message-ID: <Pine.LNX.4.64.0707061320510.4093@racer.site>
In-Reply-To: <7v8x9uexji.fsf_-_@assigned-by-dhcp.cox.net>

Hi,

On Fri, 6 Jul 2007, Junio C Hamano wrote:

Show 25 quoted lines

> diff --git a/diff.c b/diff.c
> @@ -1143,6 +1154,82 @@ int diff_filespec_is_binary(struct diff_filespec *one)
>  	return one->is_binary;
>  }
>  
> +static struct hunk_header_regexp {
> +	char *name;
> +	char *regexp;
> +	struct hunk_header_regexp *next;
> +} *hunk_header_regexp_list, **hunk_header_regexp_tail;
> +
> +static int hunk_header_config(const char *var, const char *value)
> +{
> +	static const char funcname[] = "funcname.";
> +	struct hunk_header_regexp *hh;
> +
> +	if (prefixcmp(var, funcname))
> +		return 0;
> +	var += strlen(funcname);
> +	for (hh = hunk_header_regexp_list; hh; hh = hh->next)
> +		if (!strcmp(var, hh->name)) {
> +			free(hh->regexp);
> +			hh->regexp = xstrdup(value);
> +			return 0;
> +		}

Heh. By reordering your code, you could say

	if ((hh = hunk_header_regexp(var))) {
		free(hh->regexp);
		hh->regexp = xstrdup(value);
		return 0;
	}

Show 7 quoted lines

> +	hh = xcalloc(1, sizeof(*hh));
> +	hh->name = xstrdup(var);
> +	hh->regexp = xstrdup(value);
> +	hh->next = NULL;
> +	*hunk_header_regexp_tail = hh;
> +	return 0;
> +}

Is that tail expansion not overly complex? Why not just set "hh->next = hunk_header_regexp_list; hunk_header_regexp_list = hh";

Yes, your code seems correct, but I took some extra cycles to get at that impression. A "static int parsed_config_for_hunk_headers" would have helped, instead of reusing _tail for two purposes. And this variable could be set at the beginning of hunk_header_config(), so that hunk_header_regexp() is usable from inside hunk_header_config().

Show 13 quoted lines

> +static const char *hunk_header_regexp(const char *ident)
> +{
> +	struct hunk_header_regexp *hh;
> +
> +	if (!hunk_header_regexp_tail) {
> +		hunk_header_regexp_tail = &hunk_header_regexp_list;
> +		git_config(hunk_header_config);
> +	}
> +	for (hh = hunk_header_regexp_list; hh; hh = hh->next)
> +		if (!strcmp(ident, hh->name))
> +			return hh->regexp;
> +	return NULL;
> +}

Another thing. These long names are a bit inconsistent. In the config, you name it "funcname". In xdiff, we name them "FUNCNAMES". Yes, here they are hunk_headers.

Also, since the expressions are not strictly regular expressions, but lists of them, and with your idea they are even more different, why not just go for "funcname_list"? It's easier to read, and static anyway.

Rest looks fine to me...

Ciao, Dscho