Re: [PATCH 1/2] Have a filter_start/filter_end API.

From: Alex Riesen <raa.lkml@gmail.com>
Date: Oct 7, 2007, 21:50 UTC
Message-ID: <20071007215041.GB2765@steel.home>
In-Reply-To: <20071007165218.GE10024@artemis.corp>

Pierre Habouzit, Sun, Oct 07, 2007 18:52:18 +0200:

Show 7 quoted lines

>   So, maybe there is a way to rename strbuf_start_filter so that it's
> more straightforward. The way to use the API is:
> 
>  @  char *to_free = NULL;
>  @  if ((src is inside dst && need_realloc) || operation is not in-place)
>  @      to_free = strbuf_detach(dst, NULL);
>  @  strbuf_make_room(dst, needed_size);

Why do you have to play these games with src being inside/outside? Don't you know where src already is? AFAICS in convert.c crlf_to_git - you do. And everywhere else you know this fact too.

Show 9 quoted lines

>     // do whatever you want with src and dst
> 
>     free(to_free);
> 
> strbuf_start_filter tries to implement the block marked with `@'.  Of
> course:
>   * need_realloc == (needed_size >= dst->alloc)
>   * src is inside dst means:
>     src > dst->buf && src < dst->buf + dst->alloc

in convert.c it is never "inside". It is _exactly_ the ->buf. No need for generic "inside" check. Just test if src == ->buf.

And AFAICS, there is no other users of the function.

> Though, those are both things that I find ugly to "know" in convert.c.
> How things are allocated in strbufs is one of the few things we don't
> want to assume anywhere outside of the strbuf module.

src is outside of strbuf scope. It is not internal to struct strbuf. The caller must already know if it is inside of the given strbuf instance.

need_realloc is covered by make_room, isn't it?

I'd suggest just fix the caller, it is simple in convert.c: just use ret, which contains exactly this information. If you insist on editing in-place, which makes your routines really need the in-placeability informaion. Just give it to them, better explicitely. All of this makes the routines very convert.c specific, which is the reason why I argument to have them just there and nowhere else.

Alternatively, one can memdup ->buf (as it is the input for next filter) every time a filter modifies it (which is safe, but simple, slow, requires memory, and may fragment heap):

/* simplified */
int convert_to_git(const char *path, const char *src, size_t len, struct strbuf *dst)
{
	int crlf = CRLF_GUESS;
	int ident = 0, ret = 0;
	char *filter = NULL;
	const char *tmp = src;

	ret |= apply_filter(path, tmp, len, dst, filter);
	if (ret) {
		tmp = xmemdupz(dst->buf, dst->len);
		len = dst->len;
	}

	if (crlf_to_git(path, tmp, len, dst, crlf)) {
		ret |= 1;
		if (tmp != src)
			free((void*)tmp);
		tmp = xmemdupz(dst->buf, dst->len);
		len = dst->len;
	}

	ret |= ident_to_git(path, tmp, len, dst, ident);
	if (tmp != src)
		free((void*)tmp);
	return ret;
}

It is just to show that nothing like strbuf_start_filter is _needed_ at all.