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

[Alex Riesen <raa.lkml@gmail.com>]

Pierre Habouzit, Fri, Oct 05, 2007 22:19:30 +0200:
> Those are helpers to build functions that transform a buffer into a
> strbuf, allowing the "buffer" to be taken from the destination buffer.

They are horrible. And very specialized for these "filter" routines.
To the point where I would move them into the file where they are used
(convert.c only, isn't it?)

If you continue to insist the code is generic enough to justify its
residence in strbuf.c, continue reading.

First off, what was wrong with dumb

    void strbuf_make_room(struct strbuf *, size_t newsize);

again?

> +void *strbuf_start_filter(struct strbuf *sb, const char *src, ssize_t hint)
> +{
> +	if (src < sb->buf || src >= sb->buf + sb->alloc) {
> +		if (hint > 0 && (size_t)hint >= sb->alloc)
> +			strbuf_grow(sb, hint - sb->len);
> +		return NULL;
> +	}
> +
> +	if (hint < 0)
> +		return strbuf_detach(sb, NULL);

what is that for? Why can't the caller just use strbuf_detach? (He
already has to pass negative hint somehow, which should be a concious
action).

> +		

trailing space (two HTs)

> +	if ((size_t)hint >= sb->alloc) {
> +		void *tmp = strbuf_detach(sb, NULL);
> +		strbuf_grow(sb, hint);
> +		return tmp;
> +	}
> +
> +	return NULL;
> +}

How can one know when it sb is safe to use after strbuf_end_filter?
It is for the first "if", for example. free() wont free the buf in sb.
Oh, right, one can check if returned pointer !NULL. Which just adds
more code to handle your API.

What actually happens to sb? Is it detached? Is it reallocated?
When it is detached and when it is reallocated?

Why is the returned pointer useful only for giving it to
strbuf_end_filter?

Take for example your change in crlf_to_git:
@@ -85,6 +85,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 {
 	struct text_stat stats;
 	char *dst;
+	void *tmp;
 
 	if ((action == CRLF_BINARY) || !auto_crlf || !len)
 		return 0;
@@ -110,9 +111,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 			return 0;
 	}
 
-	/* only grow if not in place */
-	if (strbuf_avail(buf) + buf->len < len)
-		strbuf_grow(buf, len - buf->len);
+	tmp = strbuf_start_filter(buf, src, len);
 	dst = buf->buf;
 	if (action == CRLF_GUESS) {
 		/*
@@ -133,13 +132,14 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 		} while (--len);
 	}
 	strbuf_setlen(buf, dst - buf->buf);
+	strbuf_end_filter(tmp);
 	return 1;
 }

And try to rewrite it with the strbuf_make_room:

@@ -110,9 +111,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 			return 0;
 	}
 
-	/* only grow if not in place */
-	if (strbuf_avail(buf) + buf->len < len)
-		strbuf_grow(buf, len - buf->len);
+	strbuf_make_room(buf, len);
 	dst = buf->buf;
 	if (action == CRLF_GUESS) {
 		/*

The change looks rather simple

> +/*----- filter API -----*/
> +extern void *strbuf_start_filter(struct strbuf *, const char *, ssize_t);
> +extern void strbuf_end_filter(void *p);

I find the naming very confusing: what filtering takes place where?
If strbuf_end_filter is just free, why is it needed at all? For the
sake of wrapping free()?