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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

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

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

> +	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().

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