From: Christian Couder <chriscool@tuxfamily.org>
Date: Fri, 27 Mar 2009 07:15:11 GMT
Subject: Re: [PATCH] log-tree: fix patch filename computation in "git format-patch"
Message-ID: <200903270815.11888.chriscool@tuxfamily.org>
In-Reply-To: <7v3acziot0.fsf@gitster.siamese.dyndns.org>

Le vendredi 27 mars 2009, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > When using "git format-patch", "get_patch_filename" in
> > "log-tree.c" calls "strbuf_splice" that could die with
> > the following message:
> >
> > "`pos + len' is too far after the end of the buffer"
> >
> > if you have:
> >
> > 	buf->len < start_len + FORMAT_PATCH_NAME_MAX
> >
> > but:
> >
> > 	buf->len + suffix_len > start_len + FORMAT_PATCH_NAME_MAX
> >
> > This patch tries to get rid of that bug.
>
> hmm, tries to?

Yeah, I was tired last night, when I created and sent this patch so I knew 
that it could be wrong.

>
> > diff --git a/log-tree.c b/log-tree.c
> > index 56a3488..ade79ab 100644
> > --- a/log-tree.c
> > +++ b/log-tree.c
> > @@ -187,16 +187,17 @@ void get_patch_filename(struct commit *commit,
> > int nr, const char *suffix,
> >
> >  	strbuf_addf(buf, commit ? "%04d-" : "%d", nr);
> >  	if (commit) {
> > +		int max_len = start_len + FORMAT_PATCH_NAME_MAX;
> >  		format_commit_message(commit, "%f", buf, DATE_NORMAL);
> >  		/*
> >  		 * Replace characters at the end with the suffix if the
> >  		 * filename is too long
> >  		 */
> > +		if (buf->len + suffix_len > max_len) {
> > +			int base = (max_len > buf->len) ? buf->len : max_len;
> > +			strbuf_splice(buf, base - suffix_len, suffix_len,
> > +				      suffix, suffix_len);
> > +		} else
> >  			strbuf_addstr(buf, suffix);
>
> Your third argument to splice does not look right; if the existing length
> is very very long, you would need to remove a lot, and if the existing
> length is slightly long, you would need to remove just a little bit, but
> you always seem to remove the fixed amount, to splice the suffix in.
>
> In any case, why does this have to be so complex?
>
> In your buffer, you originally have start_len, and would want to end up
> with "%f" expansion, plus the suffix, but you are not allowed to exceed
> FORMAT_PATCH_NAME_MAX to store what you add, and are only allowed to chop
> the "%f" expansion if you are short of room.
>
> Shouldn't it be just:
>
> 	size_t max_len = start_len + FORMAT_PATCH_NAME_MAX - suffix_len;
>         if (max_len < buf->len)
>                 strbuf_setlen(buf, max_len);
> 	strbuf_addstr(buf, suffix);
>
> The caller must make sure that suffix_len is sufficiently shorter than
> FORMAT_PATCH_NAME_MAX; I do not know if the current code does that,
> though.

Yes, this looks better.

Thanks,
Christian.