{"thread":{"id":"48809","subject":"[PATCH] fast-import: Don't count delta attempts against an empty buffer","startedAt":"2018-06-30T22:18:45Z","lastAt":"2018-07-06T16:45:31Z","messageCount":4,"participants":["Mike Hommey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"351454","messageId":"20180630214106.19848-1-mh@glandium.org","threadId":"48809","inReplyTo":null,"subject":"[PATCH] fast-import: Don't count delta attempts against an empty buffer","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2018-06-30T21:41:06Z","receivedAt":"2018-06-30T22:18:45Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"When the reference buffer is empty, diff_delta returns NULL without\nreally attempting anything, yet fast-import counts that as a delta\nattempt.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n fast-import.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 4d55910ab9..12195d54d7 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1076,7 +1076,7 @@ static int store_object(\n \t\treturn 1;\n \t}\n \n-\tif (last && last->data.buf && last->depth < max_depth\n+\tif (last && last->data.len && last->data.buf && last->depth < max_depth\n \t\t&& dat->len > the_hash_algo->rawsz) {\n \n \t\tdelta_count_attempts_by_type[type]++;\n-- \n2.18.0\n\n"},{"id":"351640","messageId":"xmqqmuv8qhzt.fsf@gitster-ct.c.googlers.com","threadId":"48809","inReplyTo":"20180630214106.19848-1-mh@glandium.org","subject":"Re: [PATCH] fast-import: Don't count delta attempts against an empty buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-03T18:41:42Z","receivedAt":"2018-07-03T18:41:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> When the reference buffer is empty, diff_delta returns NULL without\n> really attempting anything, yet fast-import counts that as a delta\n> attempt.\n\nBut that is an attempt nevertheless, no?  Attempted and failed to\nfind anything useful, that is.  What problem are you trying to solve\nand what issue are you trying to address, exactly?\n\n... goes and reads the patch ...\n\n> Signed-off-by: Mike Hommey <mh@glandium.org>\n> ---\n>  fast-import.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/fast-import.c b/fast-import.c\n> index 4d55910ab9..12195d54d7 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1076,7 +1076,7 @@ static int store_object(\n>  \t\treturn 1;\n>  \t}\n>  \n> -\tif (last && last->data.buf && last->depth < max_depth\n> +\tif (last && last->data.len && last->data.buf && last->depth < max_depth\n>  \t\t&& dat->len > the_hash_algo->rawsz) {\n>  \n>  \t\tdelta_count_attempts_by_type[type]++;\n\nThis is a misleading patch as the title and the proposed log message\nboth talk about \"attempts are counted but we didn't actually do\nanything\", implying that the only problem is that the counter is not\naligned with reality.  The fact that the post-context we see here\nonly shows the \"counting\" part does not help us, either.\n\nBut the next line in the post-context is actually code that does\nsomething important, which is ...\n\n\t\tdelta = diff_delta(last->data.buf, last->data.len,\n\t\t\tdat->buf, dat->len,\n\t\t\t&deltalen, dat->len - the_hash_algo->rawsz);\n\t} else\n\t\tdelta = NULL;\n\n\n... and makes the reader realize that the change itself is much\nbetter than the patch with 3-line context, the title, and the\nproposed log message advertises it as ;-)\n\nHow about selling it this way instead?\n\n\tfast-import: do not call diff_delta() with empty buffer\n\n\tWe know diff_delta() returns NULL, saying \"no good delta\n\texists for it\", when fed an empty data.  Check the length of\n\tthe data in the caller to avoid such a call.  \n\n\tThis incidentally reduces the number of attempted deltification\n\twe see in the final statistics.\n\nor something like that?\n"},{"id":"351665","messageId":"20180703215920.wjzhhp5yn3mt6rqw@glandium.org","threadId":"48809","inReplyTo":"xmqqmuv8qhzt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fast-import: Don't count delta attempts against an empty buffer","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2018-07-03T21:59:20Z","receivedAt":"2018-07-03T21:59:28Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, Jul 03, 2018 at 11:41:42AM -0700, Junio C Hamano wrote:\n> Mike Hommey <mh@glandium.org> writes:\n> \n> > When the reference buffer is empty, diff_delta returns NULL without\n> > really attempting anything, yet fast-import counts that as a delta\n> > attempt.\n> \n> But that is an attempt nevertheless, no?  Attempted and failed to\n> find anything useful, that is.  What problem are you trying to solve\n> and what issue are you trying to address, exactly?\n> \n> ... goes and reads the patch ...\n> \n> > Signed-off-by: Mike Hommey <mh@glandium.org>\n> > ---\n> >  fast-import.c | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/fast-import.c b/fast-import.c\n> > index 4d55910ab9..12195d54d7 100644\n> > --- a/fast-import.c\n> > +++ b/fast-import.c\n> > @@ -1076,7 +1076,7 @@ static int store_object(\n> >  \t\treturn 1;\n> >  \t}\n> >  \n> > -\tif (last && last->data.buf && last->depth < max_depth\n> > +\tif (last && last->data.len && last->data.buf && last->depth < max_depth\n> >  \t\t&& dat->len > the_hash_algo->rawsz) {\n> >  \n> >  \t\tdelta_count_attempts_by_type[type]++;\n> \n> This is a misleading patch as the title and the proposed log message\n> both talk about \"attempts are counted but we didn't actually do\n> anything\", implying that the only problem is that the counter is not\n> aligned with reality.  The fact that the post-context we see here\n> only shows the \"counting\" part does not help us, either.\n> \n> But the next line in the post-context is actually code that does\n> something important, which is ...\n> \n> \t\tdelta = diff_delta(last->data.buf, last->data.len,\n> \t\t\tdat->buf, dat->len,\n> \t\t\t&deltalen, dat->len - the_hash_algo->rawsz);\n> \t} else\n> \t\tdelta = NULL;\n> \n> \n> ... and makes the reader realize that the change itself is much\n> better than the patch with 3-line context, the title, and the\n> proposed log message advertises it as ;-)\n> \n> How about selling it this way instead?\n> \n> \tfast-import: do not call diff_delta() with empty buffer\n> \n> \tWe know diff_delta() returns NULL, saying \"no good delta\n> \texists for it\", when fed an empty data.  Check the length of\n> \tthe data in the caller to avoid such a call.  \n> \n> \tThis incidentally reduces the number of attempted deltification\n> \twe see in the final statistics.\n> \n> or something like that?\n\nFair enough. Do you want me to send a v2 with this description?\n\nMike\n"},{"id":"351782","messageId":"xmqqlgaomhy2.fsf@gitster-ct.c.googlers.com","threadId":"48809","inReplyTo":"20180703215920.wjzhhp5yn3mt6rqw@glandium.org","subject":"Re: [PATCH] fast-import: Don't count delta attempts against an empty buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T16:45:25Z","receivedAt":"2018-07-06T16:45:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> On Tue, Jul 03, 2018 at 11:41:42AM -0700, Junio C Hamano wrote:\n>> Mike Hommey <mh@glandium.org> writes:\n>> \n>> > When the reference buffer is empty, diff_delta returns NULL without\n>> > really attempting anything, yet fast-import counts that as a delta\n>> > attempt.\n>> \n>> But that is an attempt nevertheless, no?  Attempted and failed to\n>> find anything useful, that is.  What problem are you trying to solve\n>> and what issue are you trying to address, exactly?\n>> \n>> ... goes and reads the patch ...\n>> \n>> > Signed-off-by: Mike Hommey <mh@glandium.org>\n>> > ---\n>> >  fast-import.c | 2 +-\n>> >  1 file changed, 1 insertion(+), 1 deletion(-)\n>> >\n>> > diff --git a/fast-import.c b/fast-import.c\n>> > index 4d55910ab9..12195d54d7 100644\n>> > --- a/fast-import.c\n>> > +++ b/fast-import.c\n>> > @@ -1076,7 +1076,7 @@ static int store_object(\n>> >  \t\treturn 1;\n>> >  \t}\n>> >  \n>> > -\tif (last && last->data.buf && last->depth < max_depth\n>> > +\tif (last && last->data.len && last->data.buf && last->depth < max_depth\n>> >  \t\t&& dat->len > the_hash_algo->rawsz) {\n>> >  \n>> >  \t\tdelta_count_attempts_by_type[type]++;\n>> \n>> This is a misleading patch as the title and the proposed log message\n>> both talk about \"attempts are counted but we didn't actually do\n>> anything\", implying that the only problem is that the counter is not\n>> aligned with reality.  The fact that the post-context we see here\n>> only shows the \"counting\" part does not help us, either.\n>> \n>> But the next line in the post-context is actually code that does\n>> something important, which is ...\n>> \n>> \t\tdelta = diff_delta(last->data.buf, last->data.len,\n>> \t\t\tdat->buf, dat->len,\n>> \t\t\t&deltalen, dat->len - the_hash_algo->rawsz);\n>> \t} else\n>> \t\tdelta = NULL;\n>> \n>> \n>> ... and makes the reader realize that the change itself is much\n>> better than the patch with 3-line context, the title, and the\n>> proposed log message advertises it as ;-)\n>> \n>> How about selling it this way instead?\n>> \n>> \tfast-import: do not call diff_delta() with empty buffer\n>> \n>> \tWe know diff_delta() returns NULL, saying \"no good delta\n>> \texists for it\", when fed an empty data.  Check the length of\n>> \tthe data in the caller to avoid such a call.  \n>> \n>> \tThis incidentally reduces the number of attempted deltification\n>> \twe see in the final statistics.\n>> \n>> or something like that?\n>\n> Fair enough. Do you want me to send a v2 with this description?\n\nFor a single-patch topic like this one, if you like what was in the\ne-mail verbatim, saying so is sufficient, as I can just use the\nmaterial to run \"git commit --amend\".  For anything more involved\n(e.g. \"oh, then insert a code to do this before that function\"\nand/or a fix to an earlier patch in a multi-patch series), I'd\nprefer a re-submission, which can be processed just the same way as\nany other new topic.\n\nThanks.\n\n\n"}]}