{"thread":{"id":"18581","subject":"[PATCH] log-tree: fix patch filename computation in \"git format-patch\"","startedAt":"2009-03-27T00:13:01Z","lastAt":"2009-03-27T07:15:11Z","messageCount":4,"participants":["Christian Couder","Junio C Hamano","Stephen Boyd"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"109585","messageId":"20090327011301.a5185805.chriscool@tuxfamily.org","threadId":"18581","inReplyTo":null,"subject":"[PATCH] log-tree: fix patch filename computation in \"git format-patch\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-03-27T00:13:01Z","receivedAt":"2009-03-27T00:13:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When using \"git format-patch\", \"get_patch_filename\" in\n\"log-tree.c\" calls \"strbuf_splice\" that could die with\nthe following message:\n\n\"`pos + len' is too far after the end of the buffer\"\n\nif you have:\n\n\tbuf->len < start_len + FORMAT_PATCH_NAME_MAX\n\nbut:\n\n\tbuf->len + suffix_len > start_len + FORMAT_PATCH_NAME_MAX\n\nThis patch tries to get rid of that bug.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n log-tree.c |   11 ++++++-----\n 1 files changed, 6 insertions(+), 5 deletions(-)\n\nThis bug happens on \"pu\".\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 56a3488..ade79ab 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -187,16 +187,17 @@ void get_patch_filename(struct commit *commit, int nr, const char *suffix,\n \n \tstrbuf_addf(buf, commit ? \"%04d-\" : \"%d\", nr);\n \tif (commit) {\n+\t\tint max_len = start_len + FORMAT_PATCH_NAME_MAX;\n \t\tformat_commit_message(commit, \"%f\", buf, DATE_NORMAL);\n \t\t/*\n \t\t * Replace characters at the end with the suffix if the\n \t\t * filename is too long\n \t\t */\n-\t\tif (buf->len + suffix_len > FORMAT_PATCH_NAME_MAX + start_len)\n-\t\t\tstrbuf_splice(buf,\n-\t\t\t\tstart_len + FORMAT_PATCH_NAME_MAX - suffix_len,\n-\t\t\t\tsuffix_len, suffix, suffix_len);\n-\t\telse\n+\t\tif (buf->len + suffix_len > max_len) {\n+\t\t\tint base = (max_len > buf->len) ? buf->len : max_len;\n+\t\t\tstrbuf_splice(buf, base - suffix_len, suffix_len,\n+\t\t\t\t      suffix, suffix_len);\n+\t\t} else\n \t\t\tstrbuf_addstr(buf, suffix);\n \t}\n }\n-- \n1.6.2.1.506.g7aa09\n"},{"id":"109598","messageId":"7v3acziot0.fsf@gitster.siamese.dyndns.org","threadId":"18581","inReplyTo":"20090327011301.a5185805.chriscool@tuxfamily.org","subject":"Re: [PATCH] log-tree: fix patch filename computation in \"git format-patch\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-27T04:15:39Z","receivedAt":"2009-03-27T04:15:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> When using \"git format-patch\", \"get_patch_filename\" in\n> \"log-tree.c\" calls \"strbuf_splice\" that could die with\n> the following message:\n>\n> \"`pos + len' is too far after the end of the buffer\"\n>\n> if you have:\n>\n> \tbuf->len < start_len + FORMAT_PATCH_NAME_MAX\n>\n> but:\n>\n> \tbuf->len + suffix_len > start_len + FORMAT_PATCH_NAME_MAX\n>\n> This patch tries to get rid of that bug.\n\nhmm, tries to?\n\n> diff --git a/log-tree.c b/log-tree.c\n> index 56a3488..ade79ab 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -187,16 +187,17 @@ void get_patch_filename(struct commit *commit, int nr, const char *suffix,\n>  \n>  \tstrbuf_addf(buf, commit ? \"%04d-\" : \"%d\", nr);\n>  \tif (commit) {\n> +\t\tint max_len = start_len + FORMAT_PATCH_NAME_MAX;\n>  \t\tformat_commit_message(commit, \"%f\", buf, DATE_NORMAL);\n>  \t\t/*\n>  \t\t * Replace characters at the end with the suffix if the\n>  \t\t * filename is too long\n>  \t\t */\n> +\t\tif (buf->len + suffix_len > max_len) {\n> +\t\t\tint base = (max_len > buf->len) ? buf->len : max_len;\n> +\t\t\tstrbuf_splice(buf, base - suffix_len, suffix_len,\n> +\t\t\t\t      suffix, suffix_len);\n> +\t\t} else\n>  \t\t\tstrbuf_addstr(buf, suffix);\n\nYour third argument to splice does not look right; if the existing length\nis very very long, you would need to remove a lot, and if the existing\nlength is slightly long, you would need to remove just a little bit, but\nyou always seem to remove the fixed amount, to splice the suffix in.\n\nIn any case, why does this have to be so complex?\n\nIn your buffer, you originally have start_len, and would want to end up\nwith \"%f\" expansion, plus the suffix, but you are not allowed to exceed\nFORMAT_PATCH_NAME_MAX to store what you add, and are only allowed to chop\nthe \"%f\" expansion if you are short of room.\n\nShouldn't it be just:\n\n\tsize_t max_len = start_len + FORMAT_PATCH_NAME_MAX - suffix_len;\n        if (max_len < buf->len)\n                strbuf_setlen(buf, max_len);\n\tstrbuf_addstr(buf, suffix);\n\nThe caller must make sure that suffix_len is sufficiently shorter than\nFORMAT_PATCH_NAME_MAX; I do not know if the current code does that,\nthough.\n"},{"id":"109599","messageId":"780e0a6b0903262131l26e9ea6ua3289925fd322d93@mail.gmail.com","threadId":"18581","inReplyTo":"7v3acziot0.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] log-tree: fix patch filename computation in \"git format-patch\"","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2009-03-27T04:31:05Z","receivedAt":"2009-03-27T04:31:05Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"On Thu, Mar 26, 2009 at 9:15 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Shouldn't it be just:\n>\n>        size_t max_len = start_len + FORMAT_PATCH_NAME_MAX - suffix_len;\n>        if (max_len < buf->len)\n>                strbuf_setlen(buf, max_len);\n>        strbuf_addstr(buf, suffix);\n\nYes, this is good.\n\n>\n> The caller must make sure that suffix_len is sufficiently shorter than\n> FORMAT_PATCH_NAME_MAX; I do not know if the current code does that,\n> though.\n>\n\nThe original code never did this. What should happen in this case?\n\nI am away on travel this week, so I won't be able to update this until Monday.\n\nThanks,\nStephen\n"},{"id":"109616","messageId":"200903270815.11888.chriscool@tuxfamily.org","threadId":"18581","inReplyTo":"7v3acziot0.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] log-tree: fix patch filename computation in \"git format-patch\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-03-27T07:15:11Z","receivedAt":"2009-03-27T07:15:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le vendredi 27 mars 2009, Junio C Hamano a écrit :\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> > When using \"git format-patch\", \"get_patch_filename\" in\n> > \"log-tree.c\" calls \"strbuf_splice\" that could die with\n> > the following message:\n> >\n> > \"`pos + len' is too far after the end of the buffer\"\n> >\n> > if you have:\n> >\n> > \tbuf->len < start_len + FORMAT_PATCH_NAME_MAX\n> >\n> > but:\n> >\n> > \tbuf->len + suffix_len > start_len + FORMAT_PATCH_NAME_MAX\n> >\n> > This patch tries to get rid of that bug.\n>\n> hmm, tries to?\n\nYeah, I was tired last night, when I created and sent this patch so I knew \nthat it could be wrong.\n\n>\n> > diff --git a/log-tree.c b/log-tree.c\n> > index 56a3488..ade79ab 100644\n> > --- a/log-tree.c\n> > +++ b/log-tree.c\n> > @@ -187,16 +187,17 @@ void get_patch_filename(struct commit *commit,\n> > int nr, const char *suffix,\n> >\n> >  \tstrbuf_addf(buf, commit ? \"%04d-\" : \"%d\", nr);\n> >  \tif (commit) {\n> > +\t\tint max_len = start_len + FORMAT_PATCH_NAME_MAX;\n> >  \t\tformat_commit_message(commit, \"%f\", buf, DATE_NORMAL);\n> >  \t\t/*\n> >  \t\t * Replace characters at the end with the suffix if the\n> >  \t\t * filename is too long\n> >  \t\t */\n> > +\t\tif (buf->len + suffix_len > max_len) {\n> > +\t\t\tint base = (max_len > buf->len) ? buf->len : max_len;\n> > +\t\t\tstrbuf_splice(buf, base - suffix_len, suffix_len,\n> > +\t\t\t\t      suffix, suffix_len);\n> > +\t\t} else\n> >  \t\t\tstrbuf_addstr(buf, suffix);\n>\n> Your third argument to splice does not look right; if the existing length\n> is very very long, you would need to remove a lot, and if the existing\n> length is slightly long, you would need to remove just a little bit, but\n> you always seem to remove the fixed amount, to splice the suffix in.\n>\n> In any case, why does this have to be so complex?\n>\n> In your buffer, you originally have start_len, and would want to end up\n> with \"%f\" expansion, plus the suffix, but you are not allowed to exceed\n> FORMAT_PATCH_NAME_MAX to store what you add, and are only allowed to chop\n> the \"%f\" expansion if you are short of room.\n>\n> Shouldn't it be just:\n>\n> \tsize_t max_len = start_len + FORMAT_PATCH_NAME_MAX - suffix_len;\n>         if (max_len < buf->len)\n>                 strbuf_setlen(buf, max_len);\n> \tstrbuf_addstr(buf, suffix);\n>\n> The caller must make sure that suffix_len is sufficiently shorter than\n> FORMAT_PATCH_NAME_MAX; I do not know if the current code does that,\n> though.\n\nYes, this looks better.\n\nThanks,\nChristian.\n"}]}