{"thread":{"id":"49179","subject":"[PATCH] test-tool.h: include git-compat-util.h","startedAt":"2018-08-21T18:41:43Z","lastAt":"2018-09-10T19:23:52Z","messageCount":53,"participants":["Jeff King","Junio C Hamano","Derrick Stolee","Ævar Arnfjörð Bjarmason","Ben Peart","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"356182","messageId":"20180821184140.GA24165@sigill.intra.peff.net","threadId":"49179","inReplyTo":null,"subject":"[PATCH] test-tool.h: include git-compat-util.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-21T18:41:40Z","receivedAt":"2018-08-21T18:41:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The test-tool programs include \"test-tool.h\" as their first\ninclude, which breaks our CodingGuideline of \"the first\ninclude must be git-compat-util.h or an equivalent\".\n\nRather than change them all, let's instead make test-tool.h\none of those equivalents, just like we do for builtin.h\n(which many of the actual git builtins include first).\n\nSigned-off-by: Jeff King \n---\nRepost, as the original was in a larger thread about includes and didn't\nget any comment.\n\n t/helper/test-tool.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex e926c416ea..e954e8c522 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -1,6 +1,8 @@\n #ifndef __TEST_TOOL_H__\n #define __TEST_TOOL_H__\n \n+#include \"git-compat-util.h\"\n+\n int cmd__chmtime(int argc, const char **argv);\n int cmd__config(int argc, const char **argv);\n int cmd__ctype(int argc, const char **argv);\n-- \n2.19.0.rc0.398.g138a08f6f6\n"},{"id":"356184","messageId":"xmqqsh378rud.fsf@gitster-ct.c.googlers.com","threadId":"49179","inReplyTo":"20180821184140.GA24165@sigill.intra.peff.net","subject":"Re: [PATCH] test-tool.h: include git-compat-util.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-21T19:03:38Z","receivedAt":"2018-08-21T19:03:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The test-tool programs include \"test-tool.h\" as their first\n> include, which breaks our CodingGuideline of \"the first\n> include must be git-compat-util.h or an equivalent\".\n>\n> Rather than change them all, let's instead make test-tool.h\n> one of those equivalents, just like we do for builtin.h\n> (which many of the actual git builtins include first).\n>\n> Signed-off-by: Jeff King \n> ---\n> Repost, as the original was in a larger thread about includes and didn't\n> get any comment.\n\nThanks. Will queue.\n\n>\n> t/helper/test-tool.h | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\n> index e926c416ea..e954e8c522 100644\n> --- a/t/helper/test-tool.h\n> +++ b/t/helper/test-tool.h\n> @@ -1,6 +1,8 @@\n> #ifndef __TEST_TOOL_H__\n> #define __TEST_TOOL_H__\n> \n> +#include \"git-compat-util.h\"\n> +\n> int cmd__chmtime(int argc, const char **argv);\n> int cmd__config(int argc, const char **argv);\n> int cmd__ctype(int argc, const char **argv);\n"},{"id":"356186","messageId":"20180821190641.GA30764@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180821184140.GA24165@sigill.intra.peff.net","subject":"[PATCH 1/6] t/perf: factor boilerplate out of test_perf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-21T19:06:41Z","receivedAt":"2018-08-21T19:06:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"About half of test_perf() is boilerplate preparing to run\n_any_ test, and the other half is specifically running a\ntiming test. Let's split it into two functions, so that we\ncan reuse the boilerplate in future commits.\n\nSigned-off-by: Jeff King \n---\n t/perf/perf-lib.sh | 61 ++++++++++++++++++++++++++--------------------\n 1 file changed, 35 insertions(+), 26 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex e4c343a6b7..a54be09516 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -179,8 +179,8 @@ exit $ret' >&3 2>&4\n \treturn \"$eval_ret\"\n }\n \n-\n-test_perf () {\n+test_wrapper_ () {\n+\ttest_wrapper_func_=$1; shift\n \ttest_start_\n \ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n \ttest \"$#\" = 2 ||\n@@ -191,35 +191,44 @@ test_perf () {\n \t\tbase=$(basename \"$0\" .sh)\n \t\techo \"$test_count\" >>\"$perf_results_dir\"/$base.subtests\n \t\techo \"$1\" >\"$perf_results_dir\"/$base.$test_count.descr\n-\t\tif test -z \"$verbose\"; then\n-\t\t\tprintf \"%s\" \"perf $test_count - $1:\"\n-\t\telse\n-\t\t\techo \"perf $test_count - $1:\"\n-\t\tfi\n-\t\tfor i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do\n-\t\t\tsay >&3 \"running: $2\"\n-\t\t\tif test_run_perf_ \"$2\"\n-\t\t\tthen\n-\t\t\t\tif test -z \"$verbose\"; then\n-\t\t\t\t\tprintf \" %s\" \"$i\"\n-\t\t\t\telse\n-\t\t\t\t\techo \"* timing run $i/$GIT_PERF_REPEAT_COUNT:\"\n-\t\t\t\tfi\n+\t\tbase=\"$perf_results_dir\"/\"$perf_results_prefix$(basename \"$0\" .sh)\".\"$test_count\"\n+\t\t\"$test_wrapper_func_\" \"$@\"\n+\tfi\n+\n+\ttest_finish_\n+}\n+\n+test_perf_ () {\n+\tif test -z \"$verbose\"; then\n+\t\tprintf \"%s\" \"perf $test_count - $1:\"\n+\telse\n+\t\techo \"perf $test_count - $1:\"\n+\tfi\n+\tfor i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do\n+\t\tsay >&3 \"running: $2\"\n+\t\tif test_run_perf_ \"$2\"\n+\t\tthen\n+\t\t\tif test -z \"$verbose\"; then\n+\t\t\t\tprintf \" %s\" \"$i\"\n \t\t\telse\n-\t\t\t\ttest -z \"$verbose\" && echo\n-\t\t\t\ttest_failure_ \"$@\"\n-\t\t\t\tbreak\n+\t\t\t\techo \"* timing run $i/$GIT_PERF_REPEAT_COUNT:\"\n \t\t\tfi\n-\t\tdone\n-\t\tif test -z \"$verbose\"; then\n-\t\t\techo \" ok\"\n \t\telse\n-\t\t\ttest_ok_ \"$1\"\n+\t\t\ttest -z \"$verbose\" && echo\n+\t\t\ttest_failure_ \"$@\"\n+\t\t\tbreak\n \t\tfi\n-\t\tbase=\"$perf_results_dir\"/\"$perf_results_prefix$(basename \"$0\" .sh)\".\"$test_count\"\n-\t\t\"$TEST_DIRECTORY\"/perf/min_time.perl test_time.* >\"$base\".times\n+\tdone\n+\tif test -z \"$verbose\"; then\n+\t\techo \" ok\"\n+\telse\n+\t\ttest_ok_ \"$1\"\n \tfi\n-\ttest_finish_\n+\t\"$TEST_DIRECTORY\"/perf/min_time.perl test_time.* >\"$base\".times\n+}\n+\n+test_perf () {\n+\ttest_wrapper_ test_perf_ \"$@\"\n }\n \n # We extend test_done to print timings at the end (./run disables this\n-- \n2.19.0.rc0.398.g138a08f6f6\n\n"},{"id":"356187","messageId":"20180821190645.GB30764@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180821184140.GA24165@sigill.intra.peff.net","subject":"[PATCH 2/6] t/perf: factor out percent calculations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-21T19:06:45Z","receivedAt":"2018-08-21T19:06:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This will let us reuse the code when we add new values to\naggregate besides times.\n\nSigned-off-by: Jeff King \n---\n t/perf/aggregate.perl | 21 ++++++++++++---------\n 1 file changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl\nindex bc865160e7..3181b087ab 100755\n--- a/t/perf/aggregate.perl\n+++ b/t/perf/aggregate.perl\n@@ -19,21 +19,24 @@ sub get_times {\n \treturn ($rt, $4, $5);\n }\n \n+sub relative_change {\n+\tmy ($r, $firstr) = @_;\n+\tif ($firstr > 0) {\n+\t\treturn sprintf \"%+.1f%%\", 100.0*($r-$firstr)/$firstr;\n+\t} elsif ($r == 0) {\n+\t\treturn \"=\";\n+\t} else {\n+\t\treturn \"+inf\";\n+\t}\n+}\n+\n sub format_times {\n \tmy ($r, $u, $s, $firstr) = @_;\n \tif (!defined $r) {\n \t\treturn \"\";\n \t}\n \tmy $out = sprintf \"%.2f(%.2f+%.2f)\", $r, $u, $s;\n-\tif (defined $firstr) {\n-\t\tif ($firstr > 0) {\n-\t\t\t$out .= sprintf \" %+.1f%%\", 100.0*($r-$firstr)/$firstr;\n-\t\t} elsif ($r == 0) {\n-\t\t\t$out .= \" =\";\n-\t\t} else {\n-\t\t\t$out .= \" +inf\";\n-\t\t}\n-\t}\n+\t$out .= ' ' . relative_change($r, $firstr) if defined $firstr;\n \treturn $out;\n }\n \n-- \n2.19.0.rc0.398.g138a08f6f6\n\n"},{"id":"356188","messageId":"20180821190654.GC30764@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180821184140.GA24165@sigill.intra.peff.net","subject":"[PATCH 3/6] t/perf: add infrastructure for measuring sizes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-21T19:06:54Z","receivedAt":"2018-08-21T19:06:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The main objective of scripts in the perf framework is to\nrun \"test_perf\", which measures the time it takes to run\nsome operation. However, it can also be interesting to see\nthe change in the output size of certain operations.\n\nThis patch introduces test_size, which records a single\nnumeric output from the test and shows it in the aggregated\noutput (with pretty printing and relative size comparison).\n\nSigned-off-by: Jeff King \n---\n t/perf/README | 25 ++++++++++++++++++++++\n t/perf/aggregate.perl | 48 ++++++++++++++++++++++++++++++++++++++-----\n t/perf/perf-lib.sh | 13 ++++++++++++\n 3 files changed, 81 insertions(+), 5 deletions(-)\n\ndiff --git a/t/perf/README b/t/perf/README\nindex 21321a0f36..be12090c38 100644\n--- a/t/perf/README\n+++ b/t/perf/README\n@@ -168,3 +168,28 @@ that\n While we have tried to make sure that it can cope with embedded\n whitespace and other special characters, it will not work with\n multi-line data.\n+\n+Rather than tracking the performance by run-time as `test_perf` does, you\n+may also track output size by using `test_size`. The stdout of the\n+function should be a single numeric value, which will be captured and\n+shown in the aggregated output. For example:\n+\n+\ttest_perf 'time foo' '\n+\t\t./foo >foo.out\n+\t'\n+\n+\ttest_size 'output size'\n+\t\twc -c ;\n \treturn undef if not defined $line;\n \tclose $fh or die \"cannot close $name: $!\";\n-\t$line =~ /^(?:(\\d+):)?(\\d+):(\\d+(?:\\.\\d+)?) (\\d+(?:\\.\\d+)?) (\\d+(?:\\.\\d+)?)$/\n-\t\tor die \"bad input line: $line\";\n-\tmy $rt = ((defined $1 ? $1 : 0.0)*60+$2)*60+$3;\n-\treturn ($rt, $4, $5);\n+\t# times\n+\tif ($line =~ /^(?:(\\d+):)?(\\d+):(\\d+(?:\\.\\d+)?) (\\d+(?:\\.\\d+)?) (\\d+(?:\\.\\d+)?)$/) {\n+\t\tmy $rt = ((defined $1 ? $1 : 0.0)*60+$2)*60+$3;\n+\t\treturn ($rt, $4, $5);\n+\t# size\n+\t} elsif ($line =~ /^\\d+$/) {\n+\t\treturn $&;\n+\t} else {\n+\t\tdie \"bad input line: $line\";\n+\t}\n }\n \n sub relative_change {\n@@ -32,9 +38,15 @@ sub relative_change {\n \n sub format_times {\n \tmy ($r, $u, $s, $firstr) = @_;\n+\t# no value means we did not finish the test\n \tif (!defined $r) {\n \t\treturn \"\";\n \t}\n+\t# a single value means we have a size, not times\n+\tif (!defined $u) {\n+\t\treturn format_size($r, $firstr);\n+\t}\n+\t# otherwise, we have real/user/system times\n \tmy $out = sprintf \"%.2f(%.2f+%.2f)\", $r, $u, $s;\n \t$out .= ' ' . relative_change($r, $firstr) if defined $firstr;\n \treturn $out;\n@@ -54,6 +66,25 @@ sub usage {\n \texit(1);\n }\n \n+sub human_size {\n+\tmy $n = shift;\n+\tmy @units = ('', qw(K M G));\n+\twhile ($n > 900 && @units > 1) {\n+\t\t$n /= 1000;\n+\t\tshift @units;\n+\t}\n+\treturn $n unless length $units[0];\n+\treturn sprintf '%.1f%s', $n, $units[0];\n+}\n+\n+sub format_size {\n+\tmy ($size, $first) = @_;\n+\t# match the width of a time: 0.00(0.00+0.00)\n+\tmy $out = sprintf '%15s', human_size($size);\n+\t$out .= ' ' . relative_change($size, $first) if defined $first;\n+\treturn $out;\n+}\n+\n my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,\n $codespeed, $sortby, $subsection, $reponame);\n \n@@ -184,7 +215,14 @@ sub print_default_results {\n \t\tmy $firstr;\n \t\tfor my $i (0..$#dirs) {\n \t\t\tmy $d = $dirs[$i];\n-\t\t\t$times{$prefixes{$d}.$t} = [get_times(\"$resultsdir/$prefixes{$d}$t.times\")];\n+\t\t\tmy $base = \"$resultsdir/$prefixes{$d}$t\";\n+\t\t\t$times{$prefixes{$d}.$t} = [];\n+\t\t\tforeach my $type (qw(times size)) {\n+\t\t\t\tif (-e \"$base.$type\") {\n+\t\t\t\t\t$times{$prefixes{$d}.$t} = [get_times(\"$base.$type\")];\n+\t\t\t\t\tlast;\n+\t\t\t\t}\n+\t\t\t}\n \t\t\tmy ($r,$u,$s) = @{$times{$prefixes{$d}.$t}};\n \t\t\tmy $w = length format_times($r,$u,$s,$firstr);\n \t\t\t$colwidth[$i] = $w if $w > $colwidth[$i];\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex a54be09516..11d1922cf5 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -231,6 +231,19 @@ test_perf () {\n \ttest_wrapper_ test_perf_ \"$@\"\n }\n \n+test_size_ () {\n+\tsay >&3 \"running: $2\"\n+\tif test_eval_ \"$2\" 3>\"$base\".size; then\n+\t\ttest_ok_ \"$1\"\n+\telse\n+\t\ttest_failure_ \"$@\"\n+\tfi\n+}\n+\n+test_size () {\n+\ttest_wrapper_ test_size_ \"$@\"\n+}\n+\n # We extend test_done to print timings at the end (./run disables this\n # and does it after running everything)\n test_at_end_hook_ () {\n-- \n2.19.0.rc0.398.g138a08f6f6\n\n"},{"id":"356189","messageId":"20180821190658.GD30764@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180821184140.GA24165@sigill.intra.peff.net","subject":"[PATCH 4/6] t/perf: add perf tests for fetches from a bitmapped server","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-21T19:06:58Z","receivedAt":"2018-08-21T19:07:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A server with bitmapped packs can serve a clone very\nquickly. However, fetches are not necessarily made any\nfaster, because we spend a lot less time in object traversal\n(which is what bitmaps help with) and more time finding\ndeltas (because we may have to throw out on-disk deltas if\nthe client does not have the base).\n\nAs a first step to making this faster, this patch introduces\na new perf script to measure fetches into a repo of various\nages from a fully-bitmapped server.\n\nWe separately measure the work done by the server (in\npack-objects) and that done by the client (in index-pack).\nFurthermore, we measure the size of the resulting pack.\n\nBreaking it down like this (instead of just doing a regular\n\"git fetch\") lets us see how much each side benefits from\nany changes. And since we know the pack size, if we estimate\nthe network speed, then one could calculate a complete\nwall-clock time for the operation (though the script does\nnot do this automatically).\n\nSigned-off-by: Jeff King \n---\n t/perf/p5311-pack-bitmaps-fetch.sh | 45 ++++++++++++++++++++++++++++++\n 1 file changed, 45 insertions(+)\n create mode 100755 t/perf/p5311-pack-bitmaps-fetch.sh\n\ndiff --git a/t/perf/p5311-pack-bitmaps-fetch.sh b/t/perf/p5311-pack-bitmaps-fetch.sh\nnew file mode 100755\nindex 0000000000..b04575951f\n--- /dev/null\n+++ b/t/perf/p5311-pack-bitmaps-fetch.sh\n@@ -0,0 +1,45 @@\n+#!/bin/sh\n+\n+test_description='performance of fetches from bitmapped packs'\n+. ./perf-lib.sh\n+\n+test_perf_default_repo\n+\n+test_expect_success 'create bitmapped server repo' '\n+\tgit config pack.writebitmaps true &&\n+\tgit config pack.writebitmaphashcache true &&\n+\tgit repack -ad\n+'\n+\n+# simulate a fetch from a repository that last fetched N days ago, for\n+# various values of N. We do so by following the first-parent chain,\n+# and assume the first entry in the chain that is N days older than the current\n+# HEAD is where the HEAD would have been then.\n+for days in 1 2 4 8 16 32 64 128; do\n+\ttitle=$(printf '%10s' \"($days days)\")\n+\ttest_expect_success \"setup revs from $days days ago\" '\n+\t\tnow=$(git log -1 --format=%ct HEAD) &&\n+\t\tthen=$(($now - ($days * 86400))) &&\n+\t\ttip=$(git rev-list -1 --first-parent --until=$then HEAD) &&\n+\t\t{\n+\t\t\techo HEAD &&\n+\t\t\techo ^$tip\n+\t\t} >revs\n+\t'\n+\n+\ttest_perf \"server $title\" '\n+\t\tgit pack-objects --stdout --revs \\\n+\t\t\t\t --thin --delta-base-offset \\\n+\t\t\t\t tmp.pack\n+\t'\n+\n+\ttest_size \"size $title\" '\n+\t\twc -c \n---\n pack-bitmap.c | 25 ++++++++++++++++++++++++-\n pack-bitmap.h | 7 +++++++\n 2 files changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex f0a1937a1c..c3231ef9ef 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -86,6 +86,9 @@ struct bitmap_index {\n \t/* Bitmap result of the last performed walk */\n \tstruct bitmap *result;\n \n+\t/* \"have\" bitmap from the last performed walk */\n+\tstruct bitmap *haves;\n+\n \t/* Version of the bitmap index */\n \tunsigned int version;\n \n@@ -759,8 +762,8 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)\n \t\tbitmap_and_not(wants_bitmap, haves_bitmap);\n \n \tbitmap_git->result = wants_bitmap;\n+\tbitmap_git->haves = haves_bitmap;\n \n-\tbitmap_free(haves_bitmap);\n \treturn bitmap_git;\n \n cleanup:\n@@ -1114,5 +1117,25 @@ void free_bitmap_index(struct bitmap_index *b)\n \tfree(b->ext_index.objects);\n \tfree(b->ext_index.hashes);\n \tbitmap_free(b->result);\n+\tbitmap_free(b->haves);\n \tfree(b);\n }\n+\n+int bitmap_has_sha1_in_uninteresting(struct bitmap_index *bitmap_git,\n+\t\t\t\t const unsigned char *sha1)\n+{\n+\tint pos;\n+\n+\tif (!bitmap_git)\n+\t\treturn 0; /* no bitmap loaded */\n+\tif (!bitmap_git->result)\n+\t\tBUG(\"failed to perform bitmap walk before querying\");\n+\tif (!bitmap_git->haves)\n+\t\treturn 0; /* walk had no \"haves\" */\n+\n+\tpos = bitmap_position_packfile(bitmap_git, sha1);\n+\tif (pos < 0)\n+\t\treturn 0;\n+\n+\treturn bitmap_get(bitmap_git->haves, pos);\n+}\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 8a04741e12..c633bf5238 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -53,6 +53,13 @@ int rebuild_existing_bitmaps(struct bitmap_index *, struct packing_data *mapping\n \t\t\t khash_sha1 *reused_bitmaps, int show_progress);\n void free_bitmap_index(struct bitmap_index *);\n \n+/*\n+ * After a traversal has been performed on the bitmap_index, this can be\n+ * queried to see if a particular object was reachable from any of the\n+ * objects flagged as UNINTERESTING.\n+ */\n+int bitmap_has_sha1_in_uninteresting(struct bitmap_index *, const unsigned char *sha1);\n+\n void bitmap_writer_show_progress(int show);\n void bitmap_writer_set_checksum(unsigned char *sha1);\n void bitmap_writer_build_type_index(struct packing_data *to_pack,\n-- \n2.19.0.rc0.398.g138a08f6f6\n\n"},{"id":"356191","messageId":"20180821190705.GF30764@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180821184140.GA24165@sigill.intra.peff.net","subject":"[PATCH 6/6] pack-objects: reuse on-disk deltas for thin \"have\" objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-21T19:07:05Z","receivedAt":"2018-08-21T19:07:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we serve a fetch, we pass the \"wants\" and \"haves\" from\nthe fetch negotiation to pack-objects. That tells us not\nonly which objects we need to send, but we also use the\nboundary commits as \"preferred bases\": their trees and blobs\nare candidates for delta bases, both for reusing on-disk\ndeltas and for finding new ones.\n\nHowever, this misses some opportunities. Modulo some special\ncases like shallow or partial clones, we know that every\nobject reachable from the \"haves\" could be a preferred base.\nWe don't use all of them for two reasons:\n\n 1. It's expensive to traverse the whole history and\n enumerate all of the objects the other side has.\n\n 2. The delta search is expensive, so we want to keep the\n number of candidate bases sane. The boundary commits\n are the most likely to work.\n\nWhen we have reachability bitmaps, though, reason 1 no\nlonger applies. We can efficiently compute the set of\nreachable objects on the other side (and in fact already did\nso as part of the bitmap set-difference to get the list of\ninteresting objects). And using this set conveniently\ncovers the shallow and partial cases, since we have to\ndisable the use of bitmaps for those anyway.\n\nThe second reason argues against using these bases in the\nsearch for new deltas. But there's one case where we can use\nthis information for free: when we have an existing on-disk\ndelta that we're considering reusing, we can do so if we\nknow the other side has the base object. This in fact saves\ntime during the delta search, because it's one less delta we\nhave to compute.\n\nAnd that's exactly what this patch does: when we're\nconsidering whether to reuse an on-disk delta, if bitmaps\ntell us the other side has the object (and we're making a\nthin-pack), then we reuse it.\n\nHere are the results on p5311 using linux.git, which\nsimulates a client fetching after `N` days since their last\nfetch:\n\nTest origin HEAD\n--------------------------------------------------------------------------\n5311.3: server (1 days) 0.27(0.27+0.04) 0.12(0.09+0.03) -55.6%\n5311.4: size (1 days) 0.9M 237.0K -73.7%\n5311.5: client (1 days) 0.04(0.05+0.00) 0.10(0.10+0.00) +150.0%\n5311.7: server (2 days) 0.34(0.42+0.04) 0.13(0.10+0.03) -61.8%\n5311.8: size (2 days) 1.5M 347.7K -76.5%\n5311.9: client (2 days) 0.07(0.08+0.00) 0.16(0.15+0.01) +128.6%\n5311.11: server (4 days) 0.56(0.77+0.08) 0.13(0.10+0.02) -76.8%\n5311.12: size (4 days) 2.8M 566.6K -79.8%\n5311.13: client (4 days) 0.13(0.15+0.00) 0.34(0.31+0.02) +161.5%\n5311.15: server (8 days) 0.97(1.39+0.11) 0.30(0.25+0.05) -69.1%\n5311.16: size (8 days) 4.3M 1.0M -76.0%\n5311.17: client (8 days) 0.20(0.22+0.01) 0.53(0.52+0.01) +165.0%\n5311.19: server (16 days) 1.52(2.51+0.12) 0.30(0.26+0.03) -80.3%\n5311.20: size (16 days) 8.0M 2.0M -74.5%\n5311.21: client (16 days) 0.40(0.47+0.03) 1.01(0.98+0.04) +152.5%\n5311.23: server (32 days) 2.40(4.44+0.20) 0.31(0.26+0.04) -87.1%\n5311.24: size (32 days) 14.1M 4.1M -70.9%\n5311.25: client (32 days) 0.70(0.90+0.03) 1.81(1.75+0.06) +158.6%\n5311.27: server (64 days) 11.76(26.57+0.29) 0.55(0.50+0.08) -95.3%\n5311.28: size (64 days) 89.4M 47.4M -47.0%\n5311.29: client (64 days) 5.71(9.31+0.27) 15.20(15.20+0.32) +166.2%\n5311.31: server (128 days) 16.15(36.87+0.40) 0.91(0.82+0.14) -94.4%\n5311.32: size (128 days) 134.8M 100.4M -25.5%\n5311.33: client (128 days) 9.42(16.86+0.49) 25.34(25.80+0.46) +169.0%\n\nIn all cases we save CPU time on the server (sometimes\nsignificant) and the resulting pack is smaller. We do spend\nmore CPU time on the client side, because it has to\nreconstruct more deltas. But that's the right tradeoff to\nmake, since clients tend to outnumber servers. It just means\nthe thin pack mechanism is doing its job.\n\nFrom the user's perspective, the end-to-end time of the\noperation will generally be faster. E.g., in the 128-day\ncase, we saved 15s on the server at a cost of 16s on the\nclient. Since the resulting pack is 34MB smaller, this is a\nnet win if the network speed is less than 270Mbit/s. And\nthat's actually the worst case. The 64-day case saves just\nover 11s at a cost of just under 11s. So it's a slight win\nat any network speed, and the 40MB saved is pure bonus. That\ntrend continues for the smaller fetches.\n\nThe implementation itself is mostly straightforward, with\nthe new logic going into check_object(). But there are two\ntricky bits.\n\nThe first is that check_object() needs access to the\nrelevant information (the thin flag and bitmap result). We\ncan do this by pushing these into program-lifetime globals.\n\nThe second is that the rest of the code assumes that any\nreused delta will point to another \"struct object_entry\" as\nits base. But of course the case we are interested in here\nis the one where don't have such an entry!\n\nI looked at a number of options that didn't quite work:\n\n - we could use a flag to signal a reused delta, but it's\n not a single bit. We have to actually store the oid of\n the base, which is normally done by pointing to the\n existing object_entry. And we'd have to modify all the\n code which looks at deltas.\n\n - we could add the reused bases to the end of the existing\n object_entry array. While this does create some extra\n work as later stages consider the extra entries, it's\n actually not too bad (we're not sending them, so they\n don't cost much in the delta search, and at most we'd\n have 2*N of them).\n\n But there's a more subtle problem. Adding to the existing\n array means we might need to grow it with realloc, which\n could move the earlier entries around. While many of the\n references to other entries are done by integer index,\n some (including ones on the stack) use pointers, which\n would become invalidated.\n\n This isn't insurmountable, but it would require quite a\n bit of refactoring (and it's hard to know that you've got\n it all, since it may work _most_ of the time and then\n fail subtly based on memory allocation patterns).\n\n - we could allocate a new one-off entry for the base. In\n fact, this is what an earlier version of this patch did.\n However, since the refactoring brought in by ad635e82d6\n (Merge branch 'nd/pack-objects-pack-struct', 2018-05-23),\n the delta_idx code requires that both entries be in the\n main packing list.\n\nSo taking all of those options into account, what I ended up\nwith is a separate list of \"external bases\" that are not\npart of the main packing list. Each delta entry that points\nto an external base has a single-bit flag to do so; we have a\nlittle breathing room in the bitfield section of\nobject_entry.\n\nThis lets us limit the change primarily to the oe_delta()\nand oe_set_delta_ext() functions. And as a bonus, most of\nthe rest of the code does not consider these dummy entries\nat all, saving both runtime CPU and code complexity.\n\nSigned-off-by: Jeff King \n\nSigned-off-by: Jeff King \n---\n builtin/pack-objects.c | 28 +++++++++++++++++++---------\n pack-objects.c | 19 +++++++++++++++++++\n pack-objects.h | 20 ++++++++++++++++++--\n 3 files changed, 56 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 0d80dee2ba..1bd43432f7 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -40,6 +40,7 @@\n #define DELTA_CHILD(obj) oe_delta_child(&to_pack, obj)\n #define DELTA_SIBLING(obj) oe_delta_sibling(&to_pack, obj)\n #define SET_DELTA(obj, val) oe_set_delta(&to_pack, obj, val)\n+#define SET_DELTA_EXT(obj, oid) oe_set_delta_ext(&to_pack, obj, oid)\n #define SET_DELTA_SIZE(obj, val) oe_set_delta_size(&to_pack, obj, val)\n #define SET_DELTA_CHILD(obj, val) oe_set_delta_child(&to_pack, obj, val)\n #define SET_DELTA_SIBLING(obj, val) oe_set_delta_sibling(&to_pack, obj, val)\n@@ -59,6 +60,7 @@ static struct packing_data to_pack;\n \n static struct pack_idx_entry **written_list;\n static uint32_t nr_result, nr_written, nr_seen;\n+static struct bitmap_index *bitmap_git;\n \n static int non_empty;\n static int reuse_delta = 1, reuse_object = 1;\n@@ -79,6 +81,7 @@ static unsigned long pack_size_limit;\n static int depth = 50;\n static int delta_search_threads;\n static int pack_to_stdout;\n+static int thin;\n static int num_preferred_base;\n static struct progress *progress_state;\n \n@@ -1510,11 +1513,15 @@ static void check_object(struct object_entry *entry)\n \t\t\tbreak;\n \t\t}\n \n-\t\tif (base_ref && (base_entry = packlist_find(&to_pack, base_ref, NULL))) {\n+\t\tif (base_ref && (\n+\t\t (base_entry = packlist_find(&to_pack, base_ref, NULL)) ||\n+\t\t (thin &&\n+\t\t bitmap_has_sha1_in_uninteresting(bitmap_git, base_ref)))) {\n \t\t\t/*\n \t\t\t * If base_ref was set above that means we wish to\n-\t\t\t * reuse delta data, and we even found that base\n-\t\t\t * in the list of objects we want to pack. Goodie!\n+\t\t\t * reuse delta data, and either we found that object in\n+\t\t\t * the list of objects we want to pack, or it's one we\n+\t\t\t * know the receiver has.\n \t\t\t *\n \t\t\t * Depth value does not matter - find_deltas() will\n \t\t\t * never consider reused delta as the base object to\n@@ -1523,10 +1530,16 @@ static void check_object(struct object_entry *entry)\n \t\t\t */\n \t\t\toe_set_type(entry, entry->in_pack_type);\n \t\t\tSET_SIZE(entry, in_pack_size); /* delta size */\n-\t\t\tSET_DELTA(entry, base_entry);\n \t\t\tSET_DELTA_SIZE(entry, in_pack_size);\n-\t\t\tentry->delta_sibling_idx = base_entry->delta_child_idx;\n-\t\t\tSET_DELTA_CHILD(base_entry, entry);\n+\n+\t\t\tif (base_entry) {\n+\t\t\t\tSET_DELTA(entry, base_entry);\n+\t\t\t\tentry->delta_sibling_idx = base_entry->delta_child_idx;\n+\t\t\t\tSET_DELTA_CHILD(base_entry, entry);\n+\t\t\t} else {\n+\t\t\t\tSET_DELTA_EXT(entry, base_ref);\n+\t\t\t}\n+\n \t\t\tunuse_pack(&w_curs);\n \t\t\treturn;\n \t\t}\n@@ -2954,7 +2967,6 @@ static int pack_options_allow_reuse(void)\n \n static int get_object_list_from_bitmap(struct rev_info *revs)\n {\n-\tstruct bitmap_index *bitmap_git;\n \tif (!(bitmap_git = prepare_bitmap_walk(revs)))\n \t\treturn -1;\n \n@@ -2970,7 +2982,6 @@ static int get_object_list_from_bitmap(struct rev_info *revs)\n \t}\n \n \ttraverse_bitmap_commit_list(bitmap_git, &add_object_entry_from_bitmap);\n-\tfree_bitmap_index(bitmap_git);\n \treturn 0;\n }\n \n@@ -3118,7 +3129,6 @@ static int option_parse_unpack_unreachable(const struct option *opt,\n int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n {\n \tint use_internal_rev_list = 0;\n-\tint thin = 0;\n \tint shallow = 0;\n \tint all_progress_implied = 0;\n \tstruct argv_array rp = ARGV_ARRAY_INIT;\ndiff --git a/pack-objects.c b/pack-objects.c\nindex 92708522e7..9ae0cecd81 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -177,3 +177,22 @@ struct object_entry *packlist_alloc(struct packing_data *pdata,\n \n \treturn new_entry;\n }\n+\n+void oe_set_delta_ext(struct packing_data *pdata,\n+\t\t struct object_entry *delta,\n+\t\t const unsigned char *sha1)\n+{\n+\tstruct object_entry *base;\n+\n+\tALLOC_GROW(pdata->ext_bases, pdata->nr_ext + 1, pdata->alloc_ext);\n+\tbase = &pdata->ext_bases[pdata->nr_ext++];\n+\tmemset(base, 0, sizeof(*base));\n+\thashcpy(base->idx.oid.hash, sha1);\n+\n+\t/* These flags mark that we are not part of the actual pack output. */\n+\tbase->preferred_base = 1;\n+\tbase->filled = 1;\n+\n+\tdelta->ext_base = 1;\n+\tdelta->delta_idx = base - pdata->ext_bases + 1;\n+}\ndiff --git a/pack-objects.h b/pack-objects.h\nindex 08c6b57d49..3fd10b15c9 100644\n--- a/pack-objects.h\n+++ b/pack-objects.h\n@@ -111,6 +111,7 @@ struct object_entry {\n \tunsigned dfs_state:OE_DFS_STATE_BITS;\n \tunsigned char in_pack_header_size;\n \tunsigned depth:OE_DEPTH_BITS;\n+\tunsigned ext_base:1; /* delta_idx points outside packlist */\n \n \t/*\n \t * pahole results on 64-bit linux (gcc and clang)\n@@ -141,6 +142,14 @@ struct packing_data {\n \tstruct packed_git **in_pack_by_idx;\n \tstruct packed_git **in_pack;\n \n+\t/*\n+\t * This list contains entries for bases which we know the other side\n+\t * has (e.g., via reachability bitmaps), but which aren't in our\n+\t * \"objects\" list.\n+\t */\n+\tstruct object_entry *ext_bases;\n+\tuint32_t nr_ext, alloc_ext;\n+\n \tuintmax_t oe_size_limit;\n };\n \n@@ -228,9 +237,12 @@ static inline struct object_entry *oe_delta(\n \t\tconst struct packing_data *pack,\n \t\tconst struct object_entry *e)\n {\n-\tif (e->delta_idx)\n+\tif (!e->delta_idx)\n+\t\treturn NULL;\n+\tif (e->ext_base)\n+\t\treturn &pack->ext_bases[e->delta_idx - 1];\n+\telse\n \t\treturn &pack->objects[e->delta_idx - 1];\n-\treturn NULL;\n }\n \n static inline void oe_set_delta(struct packing_data *pack,\n@@ -243,6 +255,10 @@ static inline void oe_set_delta(struct packing_data *pack,\n \t\te->delta_idx = 0;\n }\n \n+void oe_set_delta_ext(struct packing_data *pack,\n+\t\t struct object_entry *e,\n+\t\t const unsigned char *sha1);\n+\n static inline struct object_entry *oe_delta_child(\n \t\tconst struct packing_data *pack,\n \t\tconst struct object_entry *e)\n-- \n2.19.0.rc0.398.g138a08f6f6\n"},{"id":"356203","messageId":"xmqqin438pze.fsf@gitster-ct.c.googlers.com","threadId":"49179","inReplyTo":"20180821190705.GF30764@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] pack-objects: reuse on-disk deltas for thin \"have\" objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-21T19:43:49Z","receivedAt":"2018-08-21T19:43:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When we serve a fetch, we pass the \"wants\" and \"haves\" from\n> ...\n> This lets us limit the change primarily to the oe_delta()\n> and oe_set_delta_ext() functions. And as a bonus, most of\n> the rest of the code does not consider these dummy entries\n> at all, saving both runtime CPU and code complexity.\n>\n> Signed-off-by: Jeff King \n>\n> Signed-off-by: Jeff King \n> ---\n\nSorry for commenting on something completely off-topic, but when\napplied with \"git am -s\", I get a resulting commit with 3 S-o-b (the\nabove two, plus the one added by \"-s\"), with a blank line in between\nthem. I can understand the first blank line (the one between your\ntwo S-o-b), as the first S-o-b does not even appear to be part of\nthe trailer block, but cannot explain why I get an extra one before\nthe one added by \"-s\". Puzzled...\n\n> @@ -79,6 +81,7 @@ static unsigned long pack_size_limit;\n> static int depth = 50;\n> static int delta_search_threads;\n> static int pack_to_stdout;\n> +static int thin;\n\nIt appears that this line is the only change since the previous\nround. The remainder of the patch looks cleanly done and readable.\n\nThanks.\n"},{"id":"356204","messageId":"7dadd117-e7f4-4d15-b907-043718022494@gmail.com","threadId":"49179","inReplyTo":"20180821190701.GE30764@sigill.intra.peff.net","subject":"Re: [PATCH 5/6] pack-bitmap: save \"have\" bitmap from walk","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-21T19:47:36Z","receivedAt":"2018-08-21T19:47:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/21/2018 3:07 PM, Jeff King wrote:\n> When we do a bitmap walk, we save the result, which\n> represents (WANTs & ~HAVEs); i.e., every object we care\n> about visiting in our walk. However, we throw away the\n> haves bitmap, which can sometimes be useful, too. Save it\n> and provide an access function so code which has performed a\n> walk can query it.\n\nThis makes a lot of sense. Based on the amount of time the \"Counting \nObjects\" blog post [1] spent talking about delta bases, I would have \nassumed this \"haves\" bitmap was already part of it. But, I can also see \nhow it could be dropped if you are focusing on performance for 'git clone'.\n\n\n> A few notes on the accessor interface:\n>\n> - the bitmap code calls these \"haves\" because it grew out\n> of the want/have negotiation for fetches. But really,\n> these are simply the objects that would be flagged\n> UNINTERESTING in a regular traversal. Let's use that\n> more universal nomenclature for the external module\n> interface. We may want to change the internal naming\n> inside the bitmap code, but that's outside the scope of\n> this patch.\n\nI saw the uninteresting-vs-haves name confusion in the patch below, but \nI agree with your logic here.\n\nSorry that I'm late to the party, but I was interested in the topic.\n\nThanks,\n\n-Stolee\n\n[1] https://githubengineering.com/counting-objects/\n\n     \"Counting Objects\" by Vincent Martí\n\n"},{"id":"356206","messageId":"20180821195403.GC859@sigill.intra.peff.net","threadId":"49179","inReplyTo":"7dadd117-e7f4-4d15-b907-043718022494@gmail.com","subject":"Re: [PATCH 5/6] pack-bitmap: save \"have\" bitmap from walk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-21T19:54:04Z","receivedAt":"2018-08-21T19:54:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2018 at 03:47:36PM -0400, Derrick Stolee wrote:\n\n> On 8/21/2018 3:07 PM, Jeff King wrote:\n> > When we do a bitmap walk, we save the result, which\n> > represents (WANTs & ~HAVEs); i.e., every object we care\n> > about visiting in our walk. However, we throw away the\n> > haves bitmap, which can sometimes be useful, too. Save it\n> > and provide an access function so code which has performed a\n> > walk can query it.\n> \n> This makes a lot of sense. Based on the amount of time the \"Counting\n> Objects\" blog post [1] spent talking about delta bases, I would have assumed\n> this \"haves\" bitmap was already part of it. But, I can also see how it could\n> be dropped if you are focusing on performance for 'git clone'.\n\nI think that blog post was written after we were already using this\npatch. ;)\n\n> > A few notes on the accessor interface:\n> > \n> > - the bitmap code calls these \"haves\" because it grew out\n> > of the want/have negotiation for fetches. But really,\n> > these are simply the objects that would be flagged\n> > UNINTERESTING in a regular traversal. Let's use that\n> > more universal nomenclature for the external module\n> > interface. We may want to change the internal naming\n> > inside the bitmap code, but that's outside the scope of\n> > this patch.\n> \n> I saw the uninteresting-vs-haves name confusion in the patch below, but I\n> agree with your logic here.\n> \n> Sorry that I'm late to the party, but I was interested in the topic.\n\nThis has already missed the freeze for v2.19, so I think there is plenty\nof time. More review would be welcome.\n\n-Peff\n"},{"id":"356207","messageId":"xmqqbm9v8pou.fsf@gitster-ct.c.googlers.com","threadId":"49179","inReplyTo":"xmqqin438pze.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 6/6] pack-objects: reuse on-disk deltas for thin \"have\" objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-21T19:50:09Z","receivedAt":"2018-08-21T19:57:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> When we serve a fetch, we pass the \"wants\" and \"haves\" from\n>> ...\n>> This lets us limit the change primarily to the oe_delta()\n>> and oe_set_delta_ext() functions. And as a bonus, most of\n>> the rest of the code does not consider these dummy entries\n>> at all, saving both runtime CPU and code complexity.\n>>\n>> Signed-off-by: Jeff King \n>>\n>> Signed-off-by: Jeff King \n>> ---\n>\n> Sorry for commenting on something completely off-topic, but when\n> applied with \"git am -s\", I get a resulting commit with 3 S-o-b (the\n> above two, plus the one added by \"-s\"), with a blank line in between\n> them. I can understand the first blank line (the one between your\n> two S-o-b), as the first S-o-b does not even appear to be part of\n> the trailer block, but cannot explain why I get an extra one before\n> the one added by \"-s\". Puzzled...\n\nI think your original \"two s-o-b with a blank line in between\" was\ncaused by the same problem, and \"git commit --amend -s\" perhaps\nadded an extra one at the end, and added a blank line before the\nlast \"paragraph\" while at it?\n\nMy suspicion is the long horizontal line at the beginning of the\ntable, triggers it. I haven't followed the code closely yet,\nthough.\n"},{"id":"356208","messageId":"20180821200011.GD859@sigill.intra.peff.net","threadId":"49179","inReplyTo":"xmqqin438pze.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 6/6] pack-objects: reuse on-disk deltas for thin \"have\" objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-21T20:00:11Z","receivedAt":"2018-08-21T20:00:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2018 at 12:43:49PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > When we serve a fetch, we pass the \"wants\" and \"haves\" from\n> > ...\n> > This lets us limit the change primarily to the oe_delta()\n> > and oe_set_delta_ext() functions. And as a bonus, most of\n> > the rest of the code does not consider these dummy entries\n> > at all, saving both runtime CPU and code complexity.\n> >\n> > Signed-off-by: Jeff King \n> >\n> > Signed-off-by: Jeff King \n> > ---\n> \n> Sorry for commenting on something completely off-topic, but when\n> applied with \"git am -s\", I get a resulting commit with 3 S-o-b (the\n> above two, plus the one added by \"-s\"), with a blank line in between\n> them. I can understand the first blank line (the one between your\n> two S-o-b), as the first S-o-b does not even appear to be part of\n> the trailer block, but cannot explain why I get an extra one before\n> the one added by \"-s\". Puzzled...\n\nHmm. This case is most curious.\n\nI don't normally have a S-o-b in my commits themselves, but in this case\nI had done quite a bit of editing in my MUA while sending out the\npatches, so I picked up the result for my local branch with a \"git\nreset --hard origin && git am ~/what-i-sent\".\n\nThen for sending v2, I did my usual \"format-patch -s\". For the first 5\npatches, it worked fine, but for this one, it created the funny\nduplicate, which should have been suppressed. So something about this\ncommit message seems to confuse our trailer-parsing code. And indeed,\nrunning:\n\n git show -s --format='%(trailers)'\n\nshows nothing, which explains the other behavior (we don't think there's\na trailer block, so we make a new one). I wonder if it gets confused by\nthe big block of perf results (which all start with \"^%[0-9.]:\").\n\n-Peff\n"},{"id":"356209","messageId":"20180821200747.GA21955@sigill.intra.peff.net","threadId":"49179","inReplyTo":"xmqqbm9v8pou.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 6/6] pack-objects: reuse on-disk deltas for thin \"have\" objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-21T20:07:47Z","receivedAt":"2018-08-21T20:07:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2018 at 12:50:09PM -0700, Junio C Hamano wrote:\n\n> > Sorry for commenting on something completely off-topic, but when\n> > applied with \"git am -s\", I get a resulting commit with 3 S-o-b (the\n> > above two, plus the one added by \"-s\"), with a blank line in between\n> > them. I can understand the first blank line (the one between your\n> > two S-o-b), as the first S-o-b does not even appear to be part of\n> > the trailer block, but cannot explain why I get an extra one before\n> > the one added by \"-s\". Puzzled...\n> \n> I think your original \"two s-o-b with a blank line in between\" was\n> caused by the same problem, and \"git commit --amend -s\" perhaps\n> added an extra one at the end, and added a blank line before the\n> last \"paragraph\" while at it?\n> \n> My suspicion is the long horizontal line at the beginning of the\n> table, triggers it. I haven't followed the code closely yet,\n> though.\n\nAh, yeah, I think you're right. We call find_patch_start(), which thinks\nthe \"---\" line is the end of the commit message. That makes sense when\nparsing trailers out of \"format-patch\" output, but not when we know we\nhave just the commit message.\n\nSo one obvious fix is a new option for the trailer code to tell it we\nhave _just_ a commit message. That would still leave this obvious false\npositive for the format-patch case, but I'm not sure it can be helped.\n\n-Peff\n"},{"id":"356210","messageId":"20180821201431.GA23263@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180821200747.GA21955@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] pack-objects: reuse on-disk deltas for thin \"have\" objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-21T20:14:31Z","receivedAt":"2018-08-21T20:14:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2018 at 04:07:47PM -0400, Jeff King wrote:\n\n> On Tue, Aug 21, 2018 at 12:50:09PM -0700, Junio C Hamano wrote:\n> \n> > > Sorry for commenting on something completely off-topic, but when\n> > > applied with \"git am -s\", I get a resulting commit with 3 S-o-b (the\n> > > above two, plus the one added by \"-s\"), with a blank line in between\n> > > them. I can understand the first blank line (the one between your\n> > > two S-o-b), as the first S-o-b does not even appear to be part of\n> > > the trailer block, but cannot explain why I get an extra one before\n> > > the one added by \"-s\". Puzzled...\n> > \n> > I think your original \"two s-o-b with a blank line in between\" was\n> > caused by the same problem, and \"git commit --amend -s\" perhaps\n> > added an extra one at the end, and added a blank line before the\n> > last \"paragraph\" while at it?\n> > \n> > My suspicion is the long horizontal line at the beginning of the\n> > table, triggers it. I haven't followed the code closely yet,\n> > though.\n> \n> Ah, yeah, I think you're right. We call find_patch_start(), which thinks\n> the \"---\" line is the end of the commit message. That makes sense when\n> parsing trailers out of \"format-patch\" output, but not when we know we\n> have just the commit message.\n> \n> So one obvious fix is a new option for the trailer code to tell it we\n> have _just_ a commit message. That would still leave this obvious false\n> positive for the format-patch case, but I'm not sure it can be helped.\n\nAnother is to tighten the check. Something like this seems more\nsensible:\n\ndiff --git a/trailer.c b/trailer.c\nindex 4e309460d1..92ec5cae82 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -793,7 +793,8 @@ static int find_patch_start(const char *str)\n \tconst char *s;\n \n \tfor (s = str; *s; s = next_line(s)) {\n-\t\tif (starts_with(s, \"---\"))\n+\t\tconst char *v;\n+\t\tif (skip_prefix(s, \"---\", &v) && isspace(*v))\n \t\t\treturn s - str;\n \t}\n \n\nas it would catch \"--- /some/file\", \"--- \", \"---\\n\", and \"---\\r\\n\", but\nnot longer dashed lines. I wondered what \"git am\" does, though, and I\nthink it is mailinfo.c:patchbreak(), which has a few other cases to\nhandle things that git itself would not have generated. I don't know if\nthat's worth supporting or not.\n\nI think there really are two bugs here, though. The find_patch_start()\ncheck is overly lax, but we also should not have to use it at all when\nwe know there is no patch.\n\n-Peff\n"},{"id":"356214","messageId":"xmqq7ekj8msu.fsf@gitster-ct.c.googlers.com","threadId":"49179","inReplyTo":"20180821201431.GA23263@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] pack-objects: reuse on-disk deltas for thin \"have\" objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-21T20:52:33Z","receivedAt":"2018-08-21T20:52:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Another is to tighten the check. Something like this seems more\n> sensible:\n\nGreat minds think alike; is it a space was exactly what I was toying\nwith before I went to lunch. FWIW I saw the full test suite passes\nwhen I came back and then saw you had an identical patch ;-)\n\n> I think there really are two bugs here, though. The find_patch_start()\n> check is overly lax, but we also should not have to use it at all when\n> we know there is no patch.\n\nYes, I was grepping for callchains, and it appeared none of them\nactually expected us to feed \"log plus --- plus patch\" format. The\nobvious candidate to take it is \"am\" but we ask mailinfo to do the\nsplitting before the log message part even hits the rest of the\nsystem. So my inclination right now is to see if that is truly the\ncase and get rid of that bogus \"patch start\" thing, and if not, add\na flag to let the caller say \"I know we only have message\" to the\ncallchain.\n"},{"id":"356216","messageId":"xmqq36v78ml8.fsf@gitster-ct.c.googlers.com","threadId":"49179","inReplyTo":"20180821200747.GA21955@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] pack-objects: reuse on-disk deltas for thin \"have\" objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-21T20:57:07Z","receivedAt":"2018-08-21T20:57:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Ah, yeah, I think you're right. We call find_patch_start(), which thinks\n> the \"---\" line is the end of the commit message. That makes sense when\n> parsing trailers out of \"format-patch\" output, but not when we know we\n> have just the commit message.\n\nYes, but that does not explain what we are seeing. If the code\nmistakenly thinks that the log message ends before that table, then\nit should have inserted the S-o-b: _before_ that table, but that is\nnot happening.\n\nSo there are three issues; (1) find-patch-start uses too weak a\nlogic to find the beginning of a patch section (2) even if it found\nthe right place, its caller does not tell \"commit --amend -s\" where\nthe log message ends correctly and (3) some callchains that get\nthere know they only have a log message but there is no way to take\nadvantage of that information and skip the call to find-patch-start.\n"},{"id":"356220","messageId":"20180821213055.GC24431@sigill.intra.peff.net","threadId":"49179","inReplyTo":"xmqq7ekj8msu.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 6/6] pack-objects: reuse on-disk deltas for thin \"have\" objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-21T21:30:55Z","receivedAt":"2018-08-21T21:30:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2018 at 01:52:33PM -0700, Junio C Hamano wrote:\n\n> > I think there really are two bugs here, though. The find_patch_start()\n> > check is overly lax, but we also should not have to use it at all when\n> > we know there is no patch.\n> \n> Yes, I was grepping for callchains, and it appeared none of them\n> actually expected us to feed \"log plus --- plus patch\" format. The\n> obvious candidate to take it is \"am\" but we ask mailinfo to do the\n> splitting before the log message part even hits the rest of the\n> system. So my inclination right now is to see if that is truly the\n> case and get rid of that bogus \"patch start\" thing, and if not, add\n> a flag to let the caller say \"I know we only have message\" to the\n> callchain.\n\nI suspect this same logic is used by git-interpret-trailers, which is\ntaking an arbitrary message on stdin. That is probably the lone caller\nthat needs to retain this magic.\n\n-Peff\n"},{"id":"356221","messageId":"20180821213200.GD24431@sigill.intra.peff.net","threadId":"49179","inReplyTo":"xmqq36v78ml8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 6/6] pack-objects: reuse on-disk deltas for thin \"have\" objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-21T21:32:00Z","receivedAt":"2018-08-21T21:32:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2018 at 01:57:07PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Ah, yeah, I think you're right. We call find_patch_start(), which thinks\n> > the \"---\" line is the end of the commit message. That makes sense when\n> > parsing trailers out of \"format-patch\" output, but not when we know we\n> > have just the commit message.\n> \n> Yes, but that does not explain what we are seeing. If the code\n> mistakenly thinks that the log message ends before that table, then\n> it should have inserted the S-o-b: _before_ that table, but that is\n> not happening.\n> \n> So there are three issues; (1) find-patch-start uses too weak a\n> logic to find the beginning of a patch section (2) even if it found\n> the right place, its caller does not tell \"commit --amend -s\" where\n> the log message ends correctly and (3) some callchains that get\n> there know they only have a log message but there is no way to take\n> advantage of that information and skip the call to find-patch-start.\n\nYes, I'd agree with all of that.\n\nI'm going offline, so if you are part-way through a patch, please do not\nworry that we might duplicate effort. :)\n\n-Peff\n"},{"id":"356247","messageId":"b071f6a2-2403-0bbb-72a5-695f8c4e724d@gmail.com","threadId":"49179","inReplyTo":"20180821190654.GC30764@sigill.intra.peff.net","subject":"Re: [PATCH 3/6] t/perf: add infrastructure for measuring sizes","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-22T13:40:55Z","receivedAt":"2018-08-22T13:41:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/21/2018 3:06 PM, Jeff King wrote:\n> The main objective of scripts in the perf framework is to\n> run \"test_perf\", which measures the time it takes to run\n> some operation. However, it can also be interesting to see\n> the change in the output size of certain operations.\n>\n> This patch introduces test_size, which records a single\n> numeric output from the test and shows it in the aggregated\n> output (with pretty printing and relative size comparison).\n\nI'm interested in exploring this test_size mechanism. The other area \nthat could benefit from size testing is 'git repack', but I don't have \nany plans to change our compression or delta strategies. If we _did_ \nlook into that, then using test_size would be a natural fit.\n\nOverall, I really like this series!\n\nThanks\n-Stolee\n\n\n"},{"id":"356256","messageId":"20180822153104.GE32630@sigill.intra.peff.net","threadId":"49179","inReplyTo":"b071f6a2-2403-0bbb-72a5-695f8c4e724d@gmail.com","subject":"Re: [PATCH 3/6] t/perf: add infrastructure for measuring sizes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-22T15:31:04Z","receivedAt":"2018-08-22T15:31:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 22, 2018 at 09:40:55AM -0400, Derrick Stolee wrote:\n\n> On 8/21/2018 3:06 PM, Jeff King wrote:\n> > The main objective of scripts in the perf framework is to\n> > run \"test_perf\", which measures the time it takes to run\n> > some operation. However, it can also be interesting to see\n> > the change in the output size of certain operations.\n> > \n> > This patch introduces test_size, which records a single\n> > numeric output from the test and shows it in the aggregated\n> > output (with pretty printing and relative size comparison).\n> \n> I'm interested in exploring this test_size mechanism. The other area that\n> could benefit from size testing is 'git repack', but I don't have any plans\n> to change our compression or delta strategies. If we _did_ look into that,\n> then using test_size would be a natural fit.\n\nYeah, I agree it would be a good tool for showing off improvements\nthere.\n\nIt may also be useful for catching regressions in topics that are trying\nto speed things up, but _don't_ intend to change the size. We could even\ndo that proactively now. I.e., something like:\n\n test_perf 'repack' 'git repack -adf'\n test_size 'pack size' 'wc -c <.git/objects/pack/*.pack'\n\njust to see if it ever changes. But I suspect its usefulness may depend\non how you are packing (e.g., is \"-f\" more likely to catch issues than\nwithout?).\n\nThe new tests I added in this series cover packs created for fetches.\nThere's no guarantee that will overlap with the behavior of an on-disk\nrepack, but at least we have some generic coverage of pack-objects\noutput sizes now. Absent any suspicion of a regression for a particular\ncase, that's probably an acceptable canary in the coal mine.\n\n-Peff\n"},{"id":"356319","messageId":"20180823004300.GA1355@sigill.intra.peff.net","threadId":"49179","inReplyTo":"xmqq36v78ml8.fsf@gitster-ct.c.googlers.com","subject":"[PATCH 0/9] trailer-parsing false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T00:43:01Z","receivedAt":"2018-08-23T00:43:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2018 at 01:57:07PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Ah, yeah, I think you're right. We call find_patch_start(), which thinks\n> > the \"---\" line is the end of the commit message. That makes sense when\n> > parsing trailers out of \"format-patch\" output, but not when we know we\n> > have just the commit message.\n> \n> Yes, but that does not explain what we are seeing. If the code\n> mistakenly thinks that the log message ends before that table, then\n> it should have inserted the S-o-b: _before_ that table, but that is\n> not happening.\n> \n> So there are three issues; (1) find-patch-start uses too weak a\n> logic to find the beginning of a patch section (2) even if it found\n> the right place, its caller does not tell \"commit --amend -s\" where\n> the log message ends correctly and (3) some callchains that get\n> there know they only have a log message but there is no way to take\n> advantage of that information and skip the call to find-patch-start.\n\nSo this turned into a bit of a rabbit hole. Here's what I have so far\n(which I think is not quite ready, but I wanted to start discussing).\n\nIssues (2) and (3) are actually the same issue. The caller that does the\nbogus appending for (2) is always append_signoff(). But it always has\njust a commit message (modulo some complications, which I'll get to in a\nminute), and so fixing it with respect to (3) magically solves (2).\nI.e., the code was simply not prepared for the case of \"end of string is\nnot end of trailers\". But since that case cannot exist, we do not have\nto deal with it. :)\n\nNow here's the tricky part. I think patches 1-8 are mostly sensible. But\nwhile doing 7/8, I noticed some weirdness around the ignore_footer\nparameter. It seems git-commit strips some cruft off the end of the\nbuffer, including \"#\" comments, before looking for the trailers. But\nthat doesn't make any sense. We should just handle the cleaned-up commit\nmessage. Worse, the stripping does not take into account options like\n--cleanup=verbatim, where \"#\" comments are _not_ cruft anymore. Double\nworse, it uses the current notion of core.commentChar, but we might be\noperating on historical data (i.e., something created with a different\ncomment char). Triple-worse, this same cleanup is used in the trailer\nparsing code!\n\nSo:\n\n git interpret-trailers --parse <<-\\EOF\n subject\n\n body\n\n Signed-off-by: me\n\n # is this a comment or not?\n EOF\n\nwill find that s-o-b. Should that \"#\" thing be a comment? I'd say no. We\nare not dealing with a commit message template at all. And even in\ngit-commit, I think we aren't (or at least ought not to be) passing the\ntemplate around to the signoff code.\n\nSo I think there may be further opportunities for cleanup here. I'm not\nsure if we'd need to retain this behavior for git-interpret-trailers.\nAFAICT it is not documented, and I suspect is mostly historical\naccident, and not anything anybody ever wanted.\n\nIf we do keep it by default, then the \"--no-divider\" option I added in\npatch 4 should probably get a more generic name and cover this.\nSomething like \"--verbatim-input\".\n\nI'm going to sleep on it and see how I feel tomorrow.\n\n [1/9]: trailer: use size_t for string offsets\n [2/9]: trailer: use size_t for iterating trailer list\n [3/9]: trailer: pass process_trailer_opts to trailer_info_get()\n [4/9]: interpret-trailers: tighten check for \"---\" patch boundary\n [5/9]: interpret-trailers: allow suppressing \"---\" divider\n [6/9]: pretty, ref-filter: format %(trailers) with no_divider option\n [7/9]: sequencer: ignore \"---\" divider when parsing trailers\n [8/9]: append_signoff: use size_t for string offsets\n [9/9]: sequencer: handle ignore_footer when parsing trailers\n\n Documentation/git-interpret-trailers.txt | 10 +++-\n builtin/interpret-trailers.c | 1 +\n commit.c | 6 +--\n commit.h | 2 +-\n pretty.c | 3 ++\n ref-filter.c | 2 +\n sequencer.c | 20 ++++++--\n sequencer.h | 9 +++-\n t/t4205-log-pretty-formats.sh | 23 +++++++++\n t/t6300-for-each-ref.sh | 23 +++++++++\n t/t7501-commit.sh | 16 ++++++\n t/t7513-interpret-trailers.sh | 42 ++++++++++++++++\n trailer.c | 62 +++++++++++++-----------\n trailer.h | 4 +-\n 14 files changed, 184 insertions(+), 39 deletions(-)\n\n-Peff\n"},{"id":"356320","messageId":"20180823004438.GA3126@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180823004300.GA1355@sigill.intra.peff.net","subject":"[PATCH 1/9] trailer: use size_t for string offsets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T00:44:38Z","receivedAt":"2018-08-23T00:44:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many of the string-parsing functions inside trailer.c return\ninteger offsets into the string (e.g., to point to the end\nof the trailer block). Several of these use an \"int\" to\nreturn or store the offsets. On a system where \"size_t\" is\nmuch larger than \"int\" (e.g., most 64-bit ones), it's easy\nto feed a gigantic commit message that results in a negative\noffset. This can result in us reading memory before the\nstring (if the int is used as an index) or far after (if\nit's implicitly cast to a size_t by passing to a strbuf\nfunction).\n\nLet's fix this by using size_t for all string offsets. Note\nthat several of the functions need ssize_t, since they use\n\"-1\" as a sentinel value. The interactions here can be\npretty subtle. E.g., end_of_title in find_trailer_start()\ndoes not itself need to be signed, but it is compared to the\nresult of last_line(), which is. That promotes the latter to\nunsigned, and the \">=\" does not behave as you might expect.\n\nSigned-off-by: Jeff King \n---\nNoticed while I was in the area. I did pretty easily reproduce a\nsegfault with this.\n\n trailer.c | 39 ++++++++++++++++++++-------------------\n 1 file changed, 20 insertions(+), 19 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 4e309460d1..88b35b8e89 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -585,7 +585,7 @@ static const char *token_from_item(struct arg_item *item, char *tok)\n \treturn item->conf.name;\n }\n \n-static int token_matches_item(const char *tok, struct arg_item *item, int tok_len)\n+static int token_matches_item(const char *tok, struct arg_item *item, size_t tok_len)\n {\n \tif (!strncasecmp(tok, item->conf.name, tok_len))\n \t\treturn 1;\n@@ -603,7 +603,7 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le\n * distinguished from the non-well-formed-line case (in which this function\n * returns -1) because some callers of this function need such a distinction.\n */\n-static int find_separator(const char *line, const char *separators)\n+static ssize_t find_separator(const char *line, const char *separators)\n {\n \tint whitespace_found = 0;\n \tconst char *c;\n@@ -630,10 +630,10 @@ static int find_separator(const char *line, const char *separators)\n */\n static void parse_trailer(struct strbuf *tok, struct strbuf *val,\n \t\t\t const struct conf_info **conf, const char *trailer,\n-\t\t\t int separator_pos)\n+\t\t\t ssize_t separator_pos)\n {\n \tstruct arg_item *item;\n-\tint tok_len;\n+\tsize_t tok_len;\n \tstruct list_head *pos;\n \n \tif (separator_pos != -1) {\n@@ -721,7 +721,7 @@ static void process_command_line_args(struct list_head *arg_head,\n \tlist_for_each(pos, new_trailer_head) {\n \t\tstruct new_trailer_item *tr =\n \t\t\tlist_entry(pos, struct new_trailer_item, list);\n-\t\tint separator_pos = find_separator(tr->text, cl_separators);\n+\t\tssize_t separator_pos = find_separator(tr->text, cl_separators);\n \n \t\tif (separator_pos == 0) {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n@@ -763,9 +763,9 @@ static const char *next_line(const char *str)\n /*\n * Return the position of the start of the last line. If len is 0, return -1.\n */\n-static int last_line(const char *buf, size_t len)\n+static ssize_t last_line(const char *buf, size_t len)\n {\n-\tint i;\n+\tssize_t i;\n \tif (len == 0)\n \t\treturn -1;\n \tif (len == 1)\n@@ -788,7 +788,7 @@ static int last_line(const char *buf, size_t len)\n * Return the position of the start of the patch or the length of str if there\n * is no patch in the message.\n */\n-static int find_patch_start(const char *str)\n+static size_t find_patch_start(const char *str)\n {\n \tconst char *s;\n \n@@ -804,10 +804,11 @@ static int find_patch_start(const char *str)\n * Return the position of the first trailer line or len if there are no\n * trailers.\n */\n-static int find_trailer_start(const char *buf, size_t len)\n+static size_t find_trailer_start(const char *buf, size_t len)\n {\n \tconst char *s;\n-\tint end_of_title, l, only_spaces = 1;\n+\tssize_t end_of_title, l;\n+\tint only_spaces = 1;\n \tint recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;\n \t/*\n \t * Number of possible continuation lines encountered. This will be\n@@ -838,7 +839,7 @@ static int find_trailer_start(const char *buf, size_t len)\n \t l = last_line(buf, l)) {\n \t\tconst char *bol = buf + l;\n \t\tconst char **p;\n-\t\tint separator_pos;\n+\t\tssize_t separator_pos;\n \n \t\tif (bol[0] == comment_line_char) {\n \t\t\tnon_trailer_lines += possible_continuation_lines;\n@@ -899,14 +900,14 @@ static int find_trailer_start(const char *buf, size_t len)\n }\n \n /* Return the position of the end of the trailers. */\n-static int find_trailer_end(const char *buf, size_t len)\n+static size_t find_trailer_end(const char *buf, size_t len)\n {\n \treturn len - ignore_non_trailer(buf, len);\n }\n \n static int ends_with_blank_line(const char *buf, size_t len)\n {\n-\tint ll = last_line(buf, len);\n+\tssize_t ll = last_line(buf, len);\n \tif (ll < 0)\n \t\treturn 0;\n \treturn is_blank_line(buf + ll);\n@@ -939,10 +940,10 @@ static void unfold_value(struct strbuf *val)\n \tstrbuf_release(&out);\n }\n \n-static int process_input_file(FILE *outfile,\n-\t\t\t const char *str,\n-\t\t\t struct list_head *head,\n-\t\t\t const struct process_trailer_options *opts)\n+static size_t process_input_file(FILE *outfile,\n+\t\t\t\t const char *str,\n+\t\t\t\t struct list_head *head,\n+\t\t\t\t const struct process_trailer_options *opts)\n {\n \tstruct trailer_info info;\n \tstruct strbuf tok = STRBUF_INIT;\n@@ -1032,7 +1033,7 @@ void process_trailers(const char *file,\n {\n \tLIST_HEAD(head);\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint trailer_end;\n+\tsize_t trailer_end;\n \tFILE *outfile = stdout;\n \n \tensure_configured();\n@@ -1132,7 +1133,7 @@ static void format_trailer_info(struct strbuf *out,\n \n \tfor (i = 0; i < info->trailer_nr; i++) {\n \t\tchar *trailer = info->trailers[i];\n-\t\tint separator_pos = find_separator(trailer, separators);\n+\t\tssize_t separator_pos = find_separator(trailer, separators);\n \n \t\tif (separator_pos >= 1) {\n \t\t\tstruct strbuf tok = STRBUF_INIT;\n-- \n2.19.0.rc0.412.g7005db4e88\n\n"},{"id":"356321","messageId":"20180823004543.GB3126@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180823004300.GA1355@sigill.intra.peff.net","subject":"[PATCH 2/9] trailer: use size_t for iterating trailer list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T00:45:44Z","receivedAt":"2018-08-23T00:45:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We store the length of the trailers list in a size_t. So on\na 64-bit system with a 32-bit int, in the unlikely case that\nwe manage to actually allocate a list with 2^31 entries,\nwe'd loop forever trying to iterate over it (our \"int\" would\nwrap to negative before exceeding info->trailer_nr).\n\nThis probably doesn't matter in practice. Each entry is at\nleast a pointer plus a non-empty string, so even without\nmalloc overhead or the memory to hold the original string\nwe're parsing from, you'd need to allocate tens of\ngigabytes. But it's easy enough to do it right.\n\nSigned-off-by: Jeff King \n---\n sequencer.c | 2 +-\n trailer.c | 6 +++---\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 65d371c746..c81b276239 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -228,7 +228,7 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tint ignore_footer)\n {\n \tstruct trailer_info info;\n-\tint i;\n+\tsize_t i;\n \tint found_sob = 0, found_sob_last = 0;\n \n \ttrailer_info_get(&info, sb->buf);\ndiff --git a/trailer.c b/trailer.c\nindex 88b35b8e89..40eef8880e 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -948,7 +948,7 @@ static size_t process_input_file(FILE *outfile,\n \tstruct trailer_info info;\n \tstruct strbuf tok = STRBUF_INIT;\n \tstruct strbuf val = STRBUF_INIT;\n-\tint i;\n+\tsize_t i;\n \n \ttrailer_info_get(&info, str);\n \n@@ -1112,7 +1112,7 @@ void trailer_info_get(struct trailer_info *info, const char *str)\n \n void trailer_info_release(struct trailer_info *info)\n {\n-\tint i;\n+\tsize_t i;\n \tfor (i = 0; i < info->trailer_nr; i++)\n \t\tfree(info->trailers[i]);\n \tfree(info->trailers);\n@@ -1122,7 +1122,7 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\t\tconst struct trailer_info *info,\n \t\t\t\tconst struct process_trailer_options *opts)\n {\n-\tint i;\n+\tsize_t i;\n \n \t/* If we want the whole block untouched, we can take the fast path. */\n \tif (!opts->only_trailers && !opts->unfold) {\n-- \n2.19.0.rc0.412.g7005db4e88\n\n"},{"id":"356322","messageId":"20180823004623.GC3126@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180823004300.GA1355@sigill.intra.peff.net","subject":"[PATCH 3/9] trailer: pass process_trailer_opts to trailer_info_get()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T00:46:23Z","receivedAt":"2018-08-23T00:46:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most of the trailer code has an \"opts\" struct which is\nfilled in by the caller. We don't pass it down to\ntrailer_info_get(), which does the initial parsing, because\nthere hasn't yet been a need to do so.\n\nLet's start passing it down in preparation for adding new\noptions. Note that there's a single caller which doesn't\notherwise have such an options struct. Since it's just one\ncaller (that we'd have to modify anyway), let's not bother\nwith any special treatment like accepting a NULL options\nstruct, and just have it allocate one with the defaults.\n\nSigned-off-by: Jeff King \n---\nWe end up needing to pass an option in that caller later anyway...\n\n sequencer.c | 3 ++-\n trailer.c | 7 ++++---\n trailer.h | 3 ++-\n 3 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex c81b276239..3e15faa94e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -227,11 +227,12 @@ static const char *get_todo_path(const struct replay_opts *opts)\n static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tint ignore_footer)\n {\n+\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n \tstruct trailer_info info;\n \tsize_t i;\n \tint found_sob = 0, found_sob_last = 0;\n \n-\ttrailer_info_get(&info, sb->buf);\n+\ttrailer_info_get(&info, sb->buf, &opts);\n \n \tif (info.trailer_start == info.trailer_end)\n \t\treturn 0;\ndiff --git a/trailer.c b/trailer.c\nindex 40eef8880e..e769c5b72c 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -950,7 +950,7 @@ static size_t process_input_file(FILE *outfile,\n \tstruct strbuf val = STRBUF_INIT;\n \tsize_t i;\n \n-\ttrailer_info_get(&info, str);\n+\ttrailer_info_get(&info, str, opts);\n \n \t/* Print lines before the trailers as is */\n \tif (!opts->only_trailers)\n@@ -1067,7 +1067,8 @@ void process_trailers(const char *file,\n \tstrbuf_release(&sb);\n }\n \n-void trailer_info_get(struct trailer_info *info, const char *str)\n+void trailer_info_get(struct trailer_info *info, const char *str,\n+\t\t const struct process_trailer_options *opts)\n {\n \tint patch_start, trailer_end, trailer_start;\n \tstruct strbuf **trailer_lines, **ptr;\n@@ -1159,7 +1160,7 @@ void format_trailers_from_commit(struct strbuf *out, const char *msg,\n {\n \tstruct trailer_info info;\n \n-\ttrailer_info_get(&info, msg);\n+\ttrailer_info_get(&info, msg, opts);\n \tformat_trailer_info(out, &info, opts);\n \ttrailer_info_release(&info);\n }\ndiff --git a/trailer.h b/trailer.h\nindex 9c10026c35..b38c8f0d16 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -79,7 +79,8 @@ void process_trailers(const char *file,\n \t\t const struct process_trailer_options *opts,\n \t\t struct list_head *new_trailer_head);\n \n-void trailer_info_get(struct trailer_info *info, const char *str);\n+void trailer_info_get(struct trailer_info *info, const char *str,\n+\t\t const struct process_trailer_options *opts);\n \n void trailer_info_release(struct trailer_info *info);\n \n-- \n2.19.0.rc0.412.g7005db4e88\n\n"},{"id":"356323","messageId":"20180823004821.GD3126@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180823004300.GA1355@sigill.intra.peff.net","subject":"[PATCH 4/9] interpret-trailers: tighten check for \"---\" patch boundary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T00:48:21Z","receivedAt":"2018-08-23T00:48:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The interpret-trailers command accepts not only raw commit\nmessages, but it also can manipulate trailers in\nformat-patch output. That means it must find the \"---\"\nboundary separating the commit message from the patch.\nHowever, it does so by looking for any line starting with\n\"---\", regardless of whether there is further content.\n\nThis is overly lax compared to the parsing done in\nmailinfo.c's patchbreak(), and may cause false positives\n(e.g., t/perf output tables uses dashes; if you cut and\npaste them into your commit message, it fools the parser).\n\nWe could try to reuse patchbreak() here, but it actually has\nseveral heuristics that are not of interest to us (e.g.,\nmatching \"diff -\" without a three-dash separator or even a\nCVS \"Index:\" line). We're not interested in taking in\nwhatever random cruft people may send, but rather handling\ngit-formatted patches.\n\nNote that the existing documentation was written in a loose\nway, so technically we are changing the behavior from what\nit said. But this should implement the original intent in a\nmore accurate way.\n\nSigned-off-by: Jeff King \n---\nBy itself, this would fix the case that spawned this\ndiscussion, though we still have false positives on \"---\" in\na commit message.\n\n Documentation/git-interpret-trailers.txt | 5 +++--\n t/t7513-interpret-trailers.sh | 22 ++++++++++++++++++++++\n trailer.c | 4 +++-\n 3 files changed, 28 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt\nindex b8fafb1e8b..7385bfdb45 100644\n--- a/Documentation/git-interpret-trailers.txt\n+++ b/Documentation/git-interpret-trailers.txt\n@@ -56,8 +56,9 @@ least one Git-generated or user-configured trailer and consists of at\n least 25% trailers.\n The group must be preceded by one or more empty (or whitespace-only) lines.\n The group must either be at the end of the message or be the last\n-non-whitespace lines before a line that starts with '---'. Such three\n-minus signs start the patch part of the message.\n+non-whitespace lines before a line that starts with '---' (followed by a\n+space or the end of the line). Such three minus signs start the patch\n+part of the message.\n \n When reading trailers, there can be whitespaces after the\n token, the separator and the value. There can also be whitespaces\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 164719d1c9..e13b40b43f 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -1417,4 +1417,26 @@ test_expect_success 'unfold' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'handling of --- lines in input' '\n+\techo \"real-trailer: just right\" >expected &&\n+\n+\tgit interpret-trailers --parse >actual <<-\\EOF &&\n+\tsubject\n+\n+\tbody\n+\n+\tnot-a-trailer: too soon\n+\t------ this is just a line in the commit message with a bunch of\n+\t------ dashes; it does not have any syntactic meaning.\n+\n+\treal-trailer: just right\n+\t---\n+\tbelow the dashed line may be a patch, etc.\n+\n+\tnot-a-trailer: too late\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n test_done\ndiff --git a/trailer.c b/trailer.c\nindex e769c5b72c..8392c6c030 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -793,7 +793,9 @@ static size_t find_patch_start(const char *str)\n \tconst char *s;\n \n \tfor (s = str; *s; s = next_line(s)) {\n-\t\tif (starts_with(s, \"---\"))\n+\t\tconst char *v;\n+\n+\t\tif (skip_prefix(s, \"---\", &v) && isspace(*v))\n \t\t\treturn s - str;\n \t}\n \n-- \n2.19.0.rc0.412.g7005db4e88\n\n"},{"id":"356324","messageId":"20180823004955.GE3126@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180823004300.GA1355@sigill.intra.peff.net","subject":"[PATCH 5/9] interpret-trailers: allow suppressing \"---\" divider","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T00:49:56Z","receivedAt":"2018-08-23T00:49:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Even with the newly-tightened \"---\" parser, it's still\npossible for a commit message to trigger a false positive if\nit contains something like \"--- foo\". If the caller knows\nthat it has only a single commit message, it can now tell us\nwith the \"--no-divider\" option, eliminating any false\npositives.\n\nIf we were designing this from scratch, I'd probably make\nthis the default. But we've advertised the \"---\" behavior in\nthe documentation since interpret-trailers has existed.\nSince it's meant to be scripted, breaking that would be a\nbad idea.\n\nNote that the logic is in the underlying trailer.c code,\nwhich is used elsewhere. The default there will keep the\ncurrent behavior, but many callers will benefit from setting\nthis new option. That's left for future patches.\n\nSigned-off-by: Jeff King \n---\n Documentation/git-interpret-trailers.txt | 7 ++++++-\n builtin/interpret-trailers.c | 1 +\n t/t7513-interpret-trailers.sh | 20 ++++++++++++++++++++\n trailer.c | 6 +++++-\n trailer.h | 1 +\n 5 files changed, 33 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt\nindex 7385bfdb45..a5e8b36f62 100644\n--- a/Documentation/git-interpret-trailers.txt\n+++ b/Documentation/git-interpret-trailers.txt\n@@ -58,7 +58,7 @@ The group must be preceded by one or more empty (or whitespace-only) lines.\n The group must either be at the end of the message or be the last\n non-whitespace lines before a line that starts with '---' (followed by a\n space or the end of the line). Such three minus signs start the patch\n-part of the message.\n+part of the message. See also `--no-divider` below.\n \n When reading trailers, there can be whitespaces after the\n token, the separator and the value. There can also be whitespaces\n@@ -126,6 +126,11 @@ OPTIONS\n \tA convenience alias for `--only-trailers --only-input\n \t--unfold`.\n \n+--no-divider::\n+\tDo not treat `---` as the end of the commit message. Use this\n+\twhen you know your input contains just the commit message itself\n+\t(and not an email or the output of `git format-patch`).\n+\n CONFIGURATION VARIABLES\n -----------------------\n \ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex b742539d4d..4b87e0dd2e 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -104,6 +104,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"unfold\", &opts.unfold, N_(\"join whitespace-continued values\")),\n \t\t{ OPTION_CALLBACK, 0, \"parse\", &opts, NULL, N_(\"set parsing options\"),\n \t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse },\n+\t\tOPT_BOOL(0, \"no-divider\", &opts.no_divider, N_(\"do not treat --- specially\")),\n \t\tOPT_CALLBACK(0, \"trailer\", &trailers, N_(\"trailer\"),\n \t\t\t\tN_(\"trailer(s) to add\"), option_parse_trailer),\n \t\tOPT_END()\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex e13b40b43f..c441861331 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -1439,4 +1439,24 @@ test_expect_success 'handling of --- lines in input' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'suppress --- handling' '\n+\techo \"real-trailer: just right\" >expected &&\n+\n+\tgit interpret-trailers --parse --no-divider >actual <<-\\EOF &&\n+\tsubject\n+\n+\tThis commit message has a \"---\" in it, but because we tell\n+\tinterpret-trailers not to respect that, it has no effect.\n+\n+\tnot-a-trailer: too soon\n+\t---\n+\n+\tThis is still the commit message body.\n+\n+\treal-trailer: just right\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n test_done\ndiff --git a/trailer.c b/trailer.c\nindex 8392c6c030..0796f326b3 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1080,7 +1080,11 @@ void trailer_info_get(struct trailer_info *info, const char *str,\n \n \tensure_configured();\n \n-\tpatch_start = find_patch_start(str);\n+\tif (opts->no_divider)\n+\t\tpatch_start = strlen(str);\n+\telse\n+\t\tpatch_start = find_patch_start(str);\n+\n \ttrailer_end = find_trailer_end(str, patch_start);\n \ttrailer_start = find_trailer_start(str, trailer_end);\n \ndiff --git a/trailer.h b/trailer.h\nindex b38c8f0d16..b997739649 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -71,6 +71,7 @@ struct process_trailer_options {\n \tint only_trailers;\n \tint only_input;\n \tint unfold;\n+\tint no_divider;\n };\n \n #define PROCESS_TRAILER_OPTIONS_INIT {0}\n-- \n2.19.0.rc0.412.g7005db4e88\n\n"},{"id":"356325","messageId":"20180823005017.GF3126@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180823004300.GA1355@sigill.intra.peff.net","subject":"[PATCH 6/9] pretty, ref-filter: format %(trailers) with no_divider option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T00:50:17Z","receivedAt":"2018-08-23T00:57:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In both of these cases we know that we are feeding the\ntrailer-parsing code a pure commit message. We should tell\nit so, which avoids false positives for a commit message\nthat contains a \"---\" line.\n\nSigned-off-by: Jeff King \n---\n pretty.c | 3 +++\n ref-filter.c | 2 ++\n t/t4205-log-pretty-formats.sh | 23 +++++++++++++++++++++++\n t/t6300-for-each-ref.sh | 23 +++++++++++++++++++++++\n 4 files changed, 51 insertions(+)\n\ndiff --git a/pretty.c b/pretty.c\nindex 98cf5228f9..8ca29e9281 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1304,6 +1304,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \n \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n+\n+\t\topts.no_divider = 1;\n+\n \t\tif (*arg == ':') {\n \t\t\targ++;\n \t\t\tfor (;;) {\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 0bccfceff2..3e8ee04d09 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -263,6 +263,8 @@ static int trailers_atom_parser(const struct ref_format *format, struct used_ato\n \tstruct string_list params = STRING_LIST_INIT_DUP;\n \tint i;\n \n+\tatom->u.contents.trailer_opts.no_divider = 1;\n+\n \tif (arg) {\n \t\tstring_list_split(¶ms, arg, ',', -1);\n \t\tfor (i = 0; i < params.nr; i++) {\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 2052cadb11..978a8a66ff 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -598,4 +598,27 @@ test_expect_success ':only and :unfold work together' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'trailer parsing not fooled by --- line' '\n+\tgit commit --allow-empty -F - <<-\\EOF &&\n+\tthis is the subject\n+\n+\tThis is the body. The message has a \"---\" line which would confuse a\n+\tmessage+patch parser. But here we know we have only a commit message,\n+\tso we get it right.\n+\n+\ttrailer: wrong\n+\t---\n+\tThis is more body.\n+\n+\ttrailer: right\n+\tEOF\n+\n+\t{\n+\t\techo \"trailer: right\" &&\n+\t\techo\n+\t} >expect &&\n+\tgit log --no-walk --format=\"%(trailers)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 024f8c06f7..97bfbee6e8 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -715,6 +715,29 @@ test_expect_success 'basic atom: head contents:trailers' '\n \ttest_cmp expect actual.clean\n '\n \n+test_expect_success 'trailer parsing not fooled by --- line' '\n+\tgit commit --allow-empty -F - <<-\\EOF &&\n+\tthis is the subject\n+\n+\tThis is the body. The message has a \"---\" line which would confuse a\n+\tmessage+patch parser. But here we know we have only a commit message,\n+\tso we get it right.\n+\n+\ttrailer: wrong\n+\t---\n+\tThis is more body.\n+\n+\ttrailer: right\n+\tEOF\n+\n+\t{\n+\t\techo \"trailer: right\" &&\n+\t\techo\n+\t} >expect &&\n+\tgit for-each-ref --format=\"%(trailers)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'Add symbolic ref for the following tests' '\n \tgit symbolic-ref refs/heads/sym refs/heads/master\n '\n-- \n2.19.0.rc0.412.g7005db4e88\n\n"},{"id":"356326","messageId":"20180823005037.GG3126@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180823004300.GA1355@sigill.intra.peff.net","subject":"[PATCH 7/9] sequencer: ignore \"---\" divider when parsing trailers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T00:50:37Z","receivedAt":"2018-08-23T00:57:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the sequencer code appends a signoff or cherry-pick\norigin, it uses the default trailer-parsing options, which\ntreat \"---\" as the end of the commit message. As a result,\nit may be fooled by a commit message that contains that\nstring and fail to find the existing trailer block. Even\nmore confusing, the actual append code does not know about\n\"---\", and always appends to the end of the string. This can\nlead to bizarre results. E.g., appending a signoff to a\ncommit message like this:\n\n subject\n\n body\n ---\n these dashes confuse the parser!\n\n Signed-off-by: A\n\nresults in output with a final block like:\n\n Signed-off-by: A\n\n Signed-off-by: A\n\nThe parser thinks the final line of the message is \"body\",\nand ignores everything else, claiming there are no trailers.\nSo we output an extra newline separator (wrong) and add a\nduplicate signoff (also wrong).\n\nSince we know we are feeding a pure commit message, we can\nsimply tell the parser to ignore the \"---\" divider.\n\nSigned-off-by: Jeff King \n---\n sequencer.c | 2 ++\n t/t7501-commit.sh | 16 ++++++++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3e15faa94e..25bdbfcce1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -232,6 +232,8 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tsize_t i;\n \tint found_sob = 0, found_sob_last = 0;\n \n+\topts.no_divider = 1;\n+\n \ttrailer_info_get(&info, sb->buf, &opts);\n \n \tif (info.trailer_start == info.trailer_end)\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 51646d8019..1cabb8b8ed 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -517,6 +517,22 @@ Myfooter: x\" &&\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'signoff not confused by ---' '\n+\tcat >expected <<-EOF &&\n+\t\tsubject\n+\n+\t\tbody\n+\t\t---\n+\t\tthese dashes confuse the parser!\n+\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\t# should be a noop, since we already signed\n+\tgit commit --allow-empty --signoff -F expected &&\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'multiple -m' '\n \n \t>negative &&\n-- \n2.19.0.rc0.412.g7005db4e88\n\n"},{"id":"356327","messageId":"20180823005051.GH3126@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180823004300.GA1355@sigill.intra.peff.net","subject":"[PATCH 8/9] append_signoff: use size_t for string offsets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T00:50:51Z","receivedAt":"2018-08-23T00:57:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The append_signoff() function takes an \"int\" to specify the\nnumber of bytes to ignore. Most callers just pass 0, and the\nremainder use ignore_non_trailer() to skip over cruft.\nThat function also returns an int, and uses them internally.\n\nOn systems where size_t is larger than an int (i.e., most\n64-bit systems), dealing with a ridiculously large commit\nmessage could end up overflowing an int, producing\nsurprising results (e.g., returning a negative offset, which\nwould cause us to look outside the original string).\n\nLet's consistently use size_t for these offsets through this\nwhole stack. As a bonus, this makes the meaning of\n\"ignore_footer\" as an offset (and not a boolean) more clear.\nBut while we're here, let's also document the interface.\n\nSigned-off-by: Jeff King \n---\n commit.c | 6 +++---\n commit.h | 2 +-\n sequencer.c | 4 ++--\n sequencer.h | 9 ++++++++-\n 4 files changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 30d1af2b20..9fb67d785d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1784,10 +1784,10 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n * Returns the number of bytes from the tail to ignore, to be fed as\n * the second parameter to append_signoff().\n */\n-int ignore_non_trailer(const char *buf, size_t len)\n+size_t ignore_non_trailer(const char *buf, size_t len)\n {\n-\tint boc = 0;\n-\tint bol = 0;\n+\tsize_t boc = 0;\n+\tsize_t bol = 0;\n \tint in_old_conflicts_block = 0;\n \tsize_t cutoff = wt_status_locate_end(buf, len);\n \ndiff --git a/commit.h b/commit.h\nindex da0db36eba..43f93b973d 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -322,7 +322,7 @@ extern const char *find_commit_header(const char *msg, const char *key,\n \t\t\t\t size_t *out_len);\n \n /* Find the end of the log message, the right place for a new trailer. */\n-extern int ignore_non_trailer(const char *buf, size_t len);\n+extern size_t ignore_non_trailer(const char *buf, size_t len);\n \n typedef int (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,\n \t\t\t\t void *cb_data);\ndiff --git a/sequencer.c b/sequencer.c\nindex 25bdbfcce1..c01ff79ab0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -225,7 +225,7 @@ static const char *get_todo_path(const struct replay_opts *opts)\n * Returns 3 when sob exists within conforming footer as last entry\n */\n static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n-\tint ignore_footer)\n+\tsize_t ignore_footer)\n {\n \tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n \tstruct trailer_info info;\n@@ -3802,7 +3802,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \treturn res;\n }\n \n-void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n+void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag)\n {\n \tunsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;\n \tstruct strbuf sob = STRBUF_INIT;\ndiff --git a/sequencer.h b/sequencer.h\nindex c751c9d6e4..c986bc8251 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -90,7 +90,14 @@ int rearrange_squash(void);\n \n extern const char sign_off_header[];\n \n-void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);\n+/*\n+ * Append a signoff to the commit message in \"msgbuf\". The ignore_footer\n+ * parameter specifies the number of bytes at the end of msgbuf that should\n+ * not be considered at all. I.e., they are not checked for existing trailers,\n+ * and the new signoff will be spliced into the buffer before those bytes.\n+ */\n+void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag);\n+\n void append_conflicts_hint(struct strbuf *msgbuf);\n int message_is_empty(const struct strbuf *sb,\n \t\t enum commit_msg_cleanup_mode cleanup_mode);\n-- \n2.19.0.rc0.412.g7005db4e88\n\n"},{"id":"356329","messageId":"20180823005157.GI3126@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180823004300.GA1355@sigill.intra.peff.net","subject":"[PATCH 9/9] sequencer: handle ignore_footer when parsing trailers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-23T00:51:57Z","receivedAt":"2018-08-23T00:58:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The append_signoff() function takes an \"ignore_footer\"\nargument, which specifies a number of bytes at the end of\nthe message buffer which should not be considered (they\ncannot contain trailers, and the trailer is spliced in\nbefore them).\n\nBut to find the existing trailers, it calls into\nhas_conforming_trailer(). That function takes an\nignore_footer parameter, but since 967dfd4d56 (sequencer:\nuse trailer's trailer layout, 2016-11-02) the parameter is\ncompletely ignored.\n\nThe trailer interface we're using takes a single string,\nwith no option to tell it to use part of the string.\nHowever, since we have a mutable strbuf, we can work around\nthis by simply overwriting (and later restoring) the\nboundary with a NUL.\n\nI'm not sure if this can actually trigger a bug in practice.\nIt's easy to get a non-zero ignore_footer by doing something\nlike this:\n\n git commit -F - --cleanup=verbatim <<-EOF\n subject\n\n body\n\n Signed-off-by: me\n\n # this looks like a comment, but is actually in the\n # message! That makes the earlier s-o-b fake.\n EOF\n\n git commit --amend -s\n\nThere git-commit calls ignore_non_trailer() to count up the\n\"#\" cruft, which becomes the ignore_footer header. But it\nworks even without this patch! That's because the trailer\ncode _also_ calls ignore_non_trailer() and skips the cruft,\ntoo. So it happens to work because the only callers with a\nnon-zero ignore_footer are using the exact same function\nthat the trailer parser uses internally.\n\nAnd that seems true for all of the current callers, but\nthere's nothing guaranteeing it. We're better off only\nfeeding the correct buffer to the trailer code in the first\nplace.\n\nSigned-off-by: Jeff King \n---\nI think this is probably worth doing, even if it's a noop. But I'm\nreally leaning towards the idea that the trailer code calling\nignore_non_trailer() is probably the wrong thing (in which case this\nwould be very important, since we'd be relying on the caller to remove\nany cruft).\n\n sequencer.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex c01ff79ab0..a1f0f17bcd 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -231,11 +231,20 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tstruct trailer_info info;\n \tsize_t i;\n \tint found_sob = 0, found_sob_last = 0;\n+\tchar saved_char;\n \n \topts.no_divider = 1;\n \n+\tif (ignore_footer) {\n+\t\tsaved_char = sb->buf[sb->len - ignore_footer];\n+\t\tsb->buf[sb->len - ignore_footer] = '\\0';\n+\t}\n+\n \ttrailer_info_get(&info, sb->buf, &opts);\n \n+\tif (ignore_footer)\n+\t\tsb->buf[sb->len - ignore_footer] = saved_char;\n+\n \tif (info.trailer_start == info.trailer_end)\n \t\treturn 0;\n \n-- \n2.19.0.rc0.412.g7005db4e88\n"},{"id":"356392","messageId":"xmqqefephr4x.fsf@gitster-ct.c.googlers.com","threadId":"49179","inReplyTo":"20180823004300.GA1355@sigill.intra.peff.net","subject":"Re: [PATCH 0/9] trailer-parsing false positives","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-23T18:30:54Z","receivedAt":"2018-08-23T18:31:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So this turned into a bit of a rabbit hole. Here's what I have so far\n> (which I think is not quite ready, but I wanted to start discussing).\n>\n> Issues (2) and (3) are actually the same issue. The caller that does the\n> bogus appending for (2) is always append_signoff(). But it always has\n> just a commit message (modulo some complications, which I'll get to in a\n> minute), and so fixing it with respect to (3) magically solves (2).\n> I.e., the code was simply not prepared for the case of \"end of string is\n> not end of trailers\". But since that case cannot exist, we do not have\n> to deal with it. :)\n>\n> Now here's the tricky part. I think patches 1-8 are mostly sensible.\n\nYeah, nothing that made me go \"Huh?\" in these 8 patches. Thanks.\n\n> So I think there may be further opportunities for cleanup here. I'm not\n> sure if we'd need to retain this behavior for git-interpret-trailers.\n> AFAICT it is not documented, and I suspect is mostly historical\n> accident, and not anything anybody ever wanted.\n\nI tend to think the behaviour was not designed but it \"just happens\nto work that way\".\n\n> If we do keep it by default, then the \"--no-divider\" option I added in\n> patch 4 should probably get a more generic name and cover this.\n> Something like \"--verbatim-input\".\n\nPerhaps. Even if this is not covered, --verbatim-input would be a\ngood name for the option ;-)\n\n> I'm going to sleep on it and see how I feel tomorrow.\n>\n> [1/9]: trailer: use size_t for string offsets\n> [2/9]: trailer: use size_t for iterating trailer list\n> [3/9]: trailer: pass process_trailer_opts to trailer_info_get()\n> [4/9]: interpret-trailers: tighten check for \"---\" patch boundary\n> [5/9]: interpret-trailers: allow suppressing \"---\" divider\n> [6/9]: pretty, ref-filter: format %(trailers) with no_divider option\n> [7/9]: sequencer: ignore \"---\" divider when parsing trailers\n> [8/9]: append_signoff: use size_t for string offsets\n> [9/9]: sequencer: handle ignore_footer when parsing trailers\n>\n> Documentation/git-interpret-trailers.txt | 10 +++-\n> builtin/interpret-trailers.c | 1 +\n> commit.c | 6 +--\n> commit.h | 2 +-\n> pretty.c | 3 ++\n> ref-filter.c | 2 +\n> sequencer.c | 20 ++++++--\n> sequencer.h | 9 +++-\n> t/t4205-log-pretty-formats.sh | 23 +++++++++\n> t/t6300-for-each-ref.sh | 23 +++++++++\n> t/t7501-commit.sh | 16 ++++++\n> t/t7513-interpret-trailers.sh | 42 ++++++++++++++++\n> trailer.c | 62 +++++++++++++-----------\n> trailer.h | 4 +-\n> 14 files changed, 184 insertions(+), 39 deletions(-)\n>\n> -Peff\n"},{"id":"356440","messageId":"20180824072629.GA11977@sigill.intra.peff.net","threadId":"49179","inReplyTo":"xmqqefephr4x.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/9] trailer-parsing false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-24T07:26:29Z","receivedAt":"2018-08-24T07:26:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 23, 2018 at 11:30:54AM -0700, Junio C Hamano wrote:\n\n> > Now here's the tricky part. I think patches 1-8 are mostly sensible.\n> \n> Yeah, nothing that made me go \"Huh?\" in these 8 patches. Thanks.\n> \n> > So I think there may be further opportunities for cleanup here. I'm not\n> > sure if we'd need to retain this behavior for git-interpret-trailers.\n> > AFAICT it is not documented, and I suspect is mostly historical\n> > accident, and not anything anybody ever wanted.\n> \n> I tend to think the behaviour was not designed but it \"just happens\n> to work that way\".\n\nOK, so I've slept on it as promised, and looked at ignore_non_trailer()\na bit more closely.\n\nIt actually ignores three things:\n\n - comment blocks\n\n - \"commit -v\" cut lines\n\n - \"Conflicts:\" blocks for merges\n\nI think I could see an argument for handling the third type everywhere,\nif we wanted trailers to go above such a block (since they really are\npart of the actual commit message). Of course, we try to make those\n\"Conflicts\" blocks into comments these days. And looking at our history,\nI had trouble finding many examples, since most of the old merges do not\nhave sign-offs in the first place.\n\nThere are a handful with the S-o-b below the conflict block (e.g.,\na24a32ddb). There are even some gems like 425b139313 and a5db4b127b)\nwhere there is one sign-off above the block and one below. In our\nhistory, there's nothing more recent than 2015, which is not incredibly\nlong after 261f315beb (merge & sequencer: turn \"Conflicts:\" hint into a\ncomment, 2014-10-28).\n\nBut in linux.git, I can find many examples from this year of Conflicts\nblocks with the signoffs afterwards. And a few with the signoffs before\n(e.g., ed09f6d05c). I think the current code will do the right thing\neither way when parsing those (if the trailers are after, we won't\nexclude those, but if they're before, then we'll skip over the Conflicts\nblock).\n\nSo frankly, that all makes me afraid of touching any of it. I do think\nit's probably doing the wrong thing in some cases, and we should\nprobably just make a rule like \"trailers always go at the bottom, end of\nstory\". But there are enough weird and historical cases, not to mention\npotential interactions with git-commit, that I'd be quite likely to\nregress some other case.\n\nSo my inclination is to punt on it for now, and go with my patches 1-8,\nwhich fix an actual case that we saw in the real world, without creating\nother problems.\n\nI think patch 9 is not hurting anything and may later help us, but I\ncould take or leave it.\n\n> > If we do keep it by default, then the \"--no-divider\" option I added in\n> > patch 4 should probably get a more generic name and cover this.\n> > Something like \"--verbatim-input\".\n> \n> Perhaps. Even if this is not covered, --verbatim-input would be a\n> good name for the option ;-)\n\nPossibly. :) What I was worried about is realizing that it's not really\n\"verbatim\", though, but rather \"some mystical set of rules including\nnonsense like git-commit cut-lines\". And so we should not over-promise\nwith the option name.\n\nI'd also be OK to call it \"verbatim\" and consider it a to-be-fixed bug\nthat it still respects these weird rules. I'm just not sure I want to\nspend more time digging on those weird rules now.\n\n-Peff\n"},{"id":"357087","messageId":"8736uud0gq.fsf@evledraar.gmail.com","threadId":"49179","inReplyTo":"20180821190701.GE30764@sigill.intra.peff.net","subject":"Re: [PATCH 5/6] pack-bitmap: save \"have\" bitmap from walk","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-31T15:23:17Z","receivedAt":"2018-08-31T15:23:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 21 2018, Jeff King wrote:\n\n> +int bitmap_has_sha1_in_uninteresting(struct bitmap_index *bitmap_git,\n> +\t\t\t\t const unsigned char *sha1)\n> +{\n> +\tint pos;\n> +\n> +\tif (!bitmap_git)\n> +\t\treturn 0; /* no bitmap loaded */\n> +\tif (!bitmap_git->result)\n> +\t\tBUG(\"failed to perform bitmap walk before querying\");\n\nSome part of what calls this completely breaks pushing from the \"next\"\nbranch when you have local bitmaps (we *really* should have some tests\nfor this...).\n\nI don't have time to dig now, but just on my local git.git on next @\nb1634b371dc2e46f9b43c45fd1857c2e2688f96e:\n\n u git (next $=) $ git repack -Ad --window=10 --depth=10 --write-bitmap-index --pack-kept-objects\n Enumerating objects: 616909, done.\n Counting objects: 100% (616909/616909), done.\n Delta compression using up to 8 threads\n Compressing objects: 100% (146475/146475), done.\n Writing objects: 100% (616909/616909), done.\n Selecting bitmap commits: 168027, done. BBuilding bitmaps: 100% (338/338), done.\n Total 616909 (delta 497494), reused 582609 (delta 467530)\n u git (next $=) $ ./git --exec-path=$PWD push avar next:avar/next-push\n Enumerating objects: 1330, done.\n BUG: pack-bitmap.c:1132: failed to perform bitmap walk before querying\n error: pack-objects died of signal 6\n error: pack-objects died of signal 6\n error: remote unpack failed: eof before pack header was fully read\n error: failed to push some refs to 'git@github.com:avar/git.git'\n\nRemoving the bitmap makes it work again:\n\n u git (next $=) $ rm .git/objects/pack/pack-496088d9464cd79dfcac50dd0d72dcd6faee8253.bitmap\n rm: remove write-protected regular file '.git/objects/pack/pack-496088d9464cd79dfcac50dd0d72dcd6faee8253.bitmap'? y\n u git (next $=) $ ./git --exec-path=$PWD push avar next:avar/next-push\n Enumerating objects: 2834, done.\n Counting objects: 100% (1799/1799), done.\n Delta compression using up to 8 threads\n Compressing objects: 100% (591/591), done.\n Writing objects: 100% (1390/1390), 430.11 KiB | 15.36 MiB/s, done.\n Total 1390 (delta 1100), reused 1072 (delta 798)\n remote: Resolving deltas: 100% (1100/1100), completed with 214 local objects.\n To github.com:avar/git.git\n * [new branch] next -> avar/next-push\n\nToday I deployed next + my patches @ work. Broke pushes in one place\nwhere repacks were being done manually with --write-bitmap-index.\n"},{"id":"357142","messageId":"20180831225558.GA22917@sigill.intra.peff.net","threadId":"49179","inReplyTo":"8736uud0gq.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/6] pack-bitmap: save \"have\" bitmap from walk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-31T22:55:58Z","receivedAt":"2018-08-31T22:56:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 31, 2018 at 05:23:17PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Aug 21 2018, Jeff King wrote:\n> \n> > +int bitmap_has_sha1_in_uninteresting(struct bitmap_index *bitmap_git,\n> > +\t\t\t\t const unsigned char *sha1)\n> > +{\n> > +\tint pos;\n> > +\n> > +\tif (!bitmap_git)\n> > +\t\treturn 0; /* no bitmap loaded */\n> > +\tif (!bitmap_git->result)\n> > +\t\tBUG(\"failed to perform bitmap walk before querying\");\n> \n> Some part of what calls this completely breaks pushing from the \"next\"\n> branch when you have local bitmaps (we *really* should have some tests\n> for this...).\n\nYikes, thanks for reporting. I agree we need better tests here.\n\nThis assertion is totally bogus, as traverse_bitmap_commit_list() will\nactually free (and NULL) the result field! This is a holdover from the\noriginal bitmap code, where bitmap_git was a static, and this is how you\nmight prepare a second walk (though AFAIK only ever do one walk per\nprocess). These days prepare_bitmap_walk() actually returns a fresh\nbitmap_index struct.\n\nSo there's no way to know if traverse_bitmap_commit_list() was called.\nBut as it turns out, we don't care. We want to know if\n\"bitmap_git->have\" was set up, which is done in prepare_bitmap_walk().\nSo there's no way[1] to get a bitmap_git that hasn't been properly set\nup. This BUG() check can just go away.\n\nI'll prepare a patch and some tests later tonight.\n\n-Peff\n\n[1] Actually, there is also prepare_bitmap_git(), but it is not really\n for general use by callers. It should be made static, or better yet,\n I suspect it can be folded into its callers.\n"},{"id":"357148","messageId":"20180901074145.GA24023@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180831225558.GA22917@sigill.intra.peff.net","subject":"[PATCH 0/4] un-breaking pack-objects with bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-01T07:41:45Z","receivedAt":"2018-09-01T07:41:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 31, 2018 at 06:55:58PM -0400, Jeff King wrote:\n\n> On Fri, Aug 31, 2018 at 05:23:17PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> > On Tue, Aug 21 2018, Jeff King wrote:\n> > \n> > > +int bitmap_has_sha1_in_uninteresting(struct bitmap_index *bitmap_git,\n> > > +\t\t\t\t const unsigned char *sha1)\n> > > +{\n> > > +\tint pos;\n> > > +\n> > > +\tif (!bitmap_git)\n> > > +\t\treturn 0; /* no bitmap loaded */\n> > > +\tif (!bitmap_git->result)\n> > > +\t\tBUG(\"failed to perform bitmap walk before querying\");\n> > \n> > Some part of what calls this completely breaks pushing from the \"next\"\n> > branch when you have local bitmaps (we *really* should have some tests\n> > for this...).\n> \n> Yikes, thanks for reporting. I agree we need better tests here.\n\nOK, here is the fix. Since the problem is in 'next', this is done as a\npatch on top of jk/pack-delta-reuse-with-bitmap. But since we're set to\nrewind 'next' post-release anyway, we could squash it directly into\n30cdc33fba from the original series. That would help later bisections\nfrom running into it, which may be worth it as it's a pretty severe\nbreakage. Or maybe not:\n\n 1. The test suite doesn't actually fail, because it's toy repos are\n too small.\n\n 2. It only triggers in the real-world if you have bitmaps turned on,\n which are not the default.\n\nSo it may not be that likely in practice to bother a hypothetical future\nbisecting developer.\n\n> [1] Actually, there is also prepare_bitmap_git(), but it is not really\n> for general use by callers. It should be made static, or better yet,\n> I suspect it can be folded into its callers.\n\nThis actually turned out not to work. There's a caller over in\npack-bitmap-write.c, and it makes things worse to try to expand the\nlogic there. So it technically _is_ possible to have a bitmap_index\nwithout a \"have\" field, but it also doesn't make sense to ask about\n\"uninteresting\" objects there. You haven't done (and cannot do) a\ntraversal on such an object.\n\nWhich I think goes back to Stefan's original question: is this just a\ncrappy API? And the answer is \"yes, to some degree\". There are really\ntwo uses of bitmaps:\n\n - you want to do a traverse_commit_list() walk, but faster\n\n - you want to selectively query the on-disk bitmaps (e.g., you are\n walking for --contains and want to ask \"do we have a bitmap for this\n object?\"\n\nThose currently use the same struct bitmap_index, but with two different\nconstructors (prepare_bitmap_git and prepare_bitmap_walk). It probably\nought to be two different ones (with the \"walk\" variant using the\n\"query\" variant under the hood). I've punted on that full conversion for\nnow, but did clean up a few confusing bits.\n\n [1/4]: bitmap_has_sha1_in_uninteresting(): drop BUG check\n\n The actual fix. This should get merged to next ASAP (or the original\n topic just reverted).\n\n [2/4]: t5310: test delta reuse with bitmaps\n\n I did this separately to give us flexibility to squash or merge\n quickly. But it does find Ævar's bug on a git without patch 1.\n\n [3/4]: traverse_bitmap_commit_list(): don't free result\n\n The original assert should have simply been useless, but it was the\n surprising behavior of this function that turned it into a bug.\n\n [4/4]: pack-bitmap: drop \"loaded\" flag\n\n And this is just an annoyance I ran into, which is a fallout from\n our conversion to using an allocated bitmap_index struct.\n\n pack-bitmap.c | 14 ++-----\n pack-bitmap.h | 2 +-\n t/t5310-pack-bitmaps.sh | 93 +++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 97 insertions(+), 12 deletions(-)\n\n-Peff\n"},{"id":"357149","messageId":"20180901074448.GA25461@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180901074145.GA24023@sigill.intra.peff.net","subject":"[PATCH 1/4] bitmap_has_sha1_in_uninteresting(): drop BUG check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-01T07:44:48Z","receivedAt":"2018-09-01T07:44:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 30cdc33fba (pack-bitmap: save \"have\" bitmap from\nwalk, 2018-08-21) introduced a new function for looking at\nthe \"have\" side of a bitmap walk. Because it only makes\nsense to do so after we've finished the walk, we added an\nextra safety assertion, making sure that bitmap_git->result\nis non-NULL.\n\nHowever, this safety is misguided. It was trying to catch\nthe case where we had called prepare_bitmap_walk() to give\nus a \"struct bitmap_index\", but had not yet called\ntraverse_bitmap_commit_list() to walk it. But all of the\ninteresting computation (including setting up the result and\n\"have\" bitmaps) happens in the first function! The latter\nfunction only delivers the result to a callback function.\n\nSo the case we were worried about is impossible; if you get\na non-NULL result from prepare_bitmap_walk(), then its\n\"have\" field will be fully formed.\n\nBut much worse, traverse_bitmap_commit_list() actually frees\nthe result field as it finishes. Which means that this\nassertion is worse than useless: it's almost guaranteed to\ntrigger!\n\nOur test suite didn't catch this because the function isn't\nactually exercised at all. The only caller comes from\n6a1e32d532 (pack-objects: reuse on-disk deltas for thin\n\"have\" objects, 2018-08-21), and that's triggered only when\nyou fetch or push history that contains an object with a\nbase that is found deep in history. Our test suite fetches\nand pushes either don't use bitmaps, or use too-small\nexample repositories. But any reasonably-sized real-world\npush or fetch (with bitmaps) would trigger this.\n\nThis patch drops the harmful assertion and tweaks the\ndocstring for the function to make the precondition clear.\nThe tests need to be improved to exercise this new\npack-objects feature, but we'll do that in a separate\ncommit.\n\nSigned-off-by: Jeff King \n---\n pack-bitmap.c | 2 --\n pack-bitmap.h | 2 +-\n 2 files changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex c3231ef9ef..76fd93a3de 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1128,8 +1128,6 @@ int bitmap_has_sha1_in_uninteresting(struct bitmap_index *bitmap_git,\n \n \tif (!bitmap_git)\n \t\treturn 0; /* no bitmap loaded */\n-\tif (!bitmap_git->result)\n-\t\tBUG(\"failed to perform bitmap walk before querying\");\n \tif (!bitmap_git->haves)\n \t\treturn 0; /* walk had no \"haves\" */\n \ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex c633bf5238..189dd68ad3 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -54,7 +54,7 @@ int rebuild_existing_bitmaps(struct bitmap_index *, struct packing_data *mapping\n void free_bitmap_index(struct bitmap_index *);\n \n /*\n- * After a traversal has been performed on the bitmap_index, this can be\n+ * After a traversal has been performed by prepare_bitmap_walk(), this can be\n * queried to see if a particular object was reachable from any of the\n * objects flagged as UNINTERESTING.\n */\n-- \n2.19.0.rc1.549.g6ce4dc0f08\n\n"},{"id":"357150","messageId":"20180901074813.GB25461@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180901074145.GA24023@sigill.intra.peff.net","subject":"[PATCH 2/4] t5310: test delta reuse with bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-01T07:48:13Z","receivedAt":"2018-09-01T07:48:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 6a1e32d532 (pack-objects: reuse on-disk deltas for\nthin \"have\" objects, 2018-08-21) taught pack-objects a new\noptimization trick. Since this wasn't meant to change\nuser-visible behavior, but only produce smaller packs more\nquickly, testing focused on t/perf/p5311.\n\nHowever, since people don't run perf tests very often, we\nshould make sure that the feature is exercised in the\nregular test suite. This patch does so.\n\nSigned-off-by: Jeff King \n---\nSide note: If we do squash patch 1 into the eariler series, that will\ninvalidate the commit id mentioned in the commit message here.\n\n t/t5310-pack-bitmaps.sh | 93 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 93 insertions(+)\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 557bd0d0c0..af38776054 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -342,4 +342,97 @@ test_expect_success 'truncated bitmap fails gracefully' '\n \ttest_i18ngrep corrupt stderr\n '\n \n+# have_delta \n+#\n+# Note that because this relies on cat-file, it might find _any_ copy of an\n+# object in the repository. The caller is responsible for making sure\n+# there's only one (e.g., via \"repack -ad\", or having just fetched a copy).\n+have_delta () {\n+\techo $2 >expect &&\n+\techo $1 | git cat-file --batch-check=\"%(deltabase)\" >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+# Create a state of history with these properties:\n+#\n+# - refs that allow a client to fetch some new history, while sharing some old\n+# history with the server; we use branches delta-reuse-old and\n+# delta-reuse-new here\n+#\n+# - the new history contains an object that is stored on the server as a delta\n+# against a base that is in the old history\n+#\n+# - the base object is not immediately reachable from the tip of the old\n+# history; finding it would involve digging down through history we know the\n+# other side has\n+#\n+# This should result in a state where fetching from old->new would not\n+# traditionally reuse the on-disk delta (because we'd have to dig to realize\n+# that the client has it), but we will do so if bitmaps can tell us cheaply\n+# that the other side has it.\n+test_expect_success 'set up thin delta-reuse parent' '\n+\t# This first commit contains the buried base object.\n+\ttest-tool genrandom delta 16384 >file &&\n+\tgit add file &&\n+\tgit commit -m \"delta base\" &&\n+\tbase=$(git rev-parse --verify HEAD:file) &&\n+\n+\t# These intermediate commits bury the base back in history.\n+\t# This becomes the \"old\" state.\n+\tfor i in 1 2 3 4 5\n+\tdo\n+\t\techo $i >file &&\n+\t\tgit commit -am \"intermediate $i\" || return 1\n+\tdone &&\n+\tgit branch delta-reuse-old &&\n+\n+\t# And now our new history has a delta against the buried base. Note\n+\t# that this must be smaller than the original file, since pack-objects\n+\t# prefers to create deltas from smaller objects to larger.\n+\ttest-tool genrandom delta 16300 >file &&\n+\tgit commit -am \"delta result\" &&\n+\tdelta=$(git rev-parse --verify HEAD:file) &&\n+\tgit branch delta-reuse-new &&\n+\n+\t# Repack with bitmaps and double check that we have the expected delta\n+\t# relationship.\n+\tgit repack -adb &&\n+\thave_delta $delta $base\n+'\n+\n+# Now we can sanity-check the non-bitmap behavior (that the server is not able\n+# to reuse the delta). This isn't strictly something we care about, so this\n+# test could be scrapped in the future. But it makes sure that the next test is\n+# actually triggering the feature we want.\n+#\n+# Note that our tools for working with on-the-wire \"thin\" packs are limited. So\n+# we actually perform the fetch, retain the resulting pack, and inspect the\n+# result.\n+test_expect_success 'fetch without bitmaps ignores delta against old base' '\n+\ttest_config pack.usebitmaps false &&\n+\ttest_when_finished \"rm -rf client.git\" &&\n+\tgit init --bare client.git &&\n+\t(\n+\t\tcd client.git &&\n+\t\tgit config transfer.unpackLimit 1 &&\n+\t\tgit fetch .. delta-reuse-old:delta-reuse-old &&\n+\t\tgit fetch .. delta-reuse-new:delta-reuse-new &&\n+\t\thave_delta $delta $ZERO_OID\n+\t)\n+'\n+\n+# And do the same for the bitmap case, where we do expect to find the delta.\n+test_expect_success 'fetch with bitmaps can reuse old base' '\n+\ttest_config pack.usebitmaps true &&\n+\ttest_when_finished \"rm -rf client.git\" &&\n+\tgit init --bare client.git &&\n+\t(\n+\t\tcd client.git &&\n+\t\tgit config transfer.unpackLimit 1 &&\n+\t\tgit fetch .. delta-reuse-old:delta-reuse-old &&\n+\t\tgit fetch .. delta-reuse-new:delta-reuse-new &&\n+\t\thave_delta $delta $base\n+\t)\n+'\n+\n test_done\n-- \n2.19.0.rc1.549.g6ce4dc0f08\n\n"},{"id":"357151","messageId":"20180901074948.GC25461@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180901074145.GA24023@sigill.intra.peff.net","subject":"[PATCH 3/4] traverse_bitmap_commit_list(): don't free result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-01T07:49:48Z","receivedAt":"2018-09-01T07:49:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since it was introduced in fff42755ef (pack-bitmap: add\nsupport for bitmap indexes, 2013-12-21), this function has\nfreed the result after traversing it. That is an artifact of\nthe early days of the bitmap code, when we had a single\nstatic \"struct bitmap_index\". Back then, it was intended\nthat you would do:\n\n prepare_bitmap_walk(&revs);\n traverse_bitmap_commit_list(&revs);\n\nSince the actual bitmap_index struct was totally behind the\nscenes, it was convenient for traverse_bitmap_commit_list()\nto clean it up, clearing the way for another traversal.\n\nBut since 3ae5fa0768 (pack-bitmap: remove bitmap_git global\nvariable, 2018-06-07), the caller explicitly manages the\nbitmap_index struct itself, like this:\n\n b = prepare_bitmap_walk(&revs);\n traverse_bitmap_commit_list(b, &revs);\n free_bitmap_index(b);\n\nIt no longer makes sense to auto-free the result after the\ntraversal. If you want to do another traversal, you'd just\ncreate a new bitmap_index. And while nobody tries to call\ntraverse_bitmap_commit_list() twice, the fact that it throws\naway the result might be surprising, and is better avoided.\n\nNote that in the \"old\" way it was possible for two walks to\namortize the cost of opening the on-disk .bitmap file (since\nit was stored in the global bitmap_index), but we lost that\nin 3ae5fa0768. However, no code actually does this, so it's\nnot worth addressing now. The solution might involve a new:\n\n reset_bitmap_walk(b, &revs);\n\ncall. Or we might even attach the bitmap data to its\nmatching packed_git struct, so that multiple\nprepare_bitmap_walk() calls could use it. That can wait\nuntil somebody actually has need of the optimization (and\nuntil then, we'll do the correct, unsurprising thing).\n\nSigned-off-by: Jeff King \n---\n pack-bitmap.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 76fd93a3de..8c1af1cca2 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -848,9 +848,6 @@ void traverse_bitmap_commit_list(struct bitmap_index *bitmap_git,\n \t\tOBJ_TAG, show_reachable);\n \n \tshow_extended_objects(bitmap_git, show_reachable);\n-\n-\tbitmap_free(bitmap_git->result);\n-\tbitmap_git->result = NULL;\n }\n \n static uint32_t count_object_type(struct bitmap_index *bitmap_git,\n-- \n2.19.0.rc1.549.g6ce4dc0f08\n\n"},{"id":"357152","messageId":"20180901075056.GD25461@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180901074145.GA24023@sigill.intra.peff.net","subject":"[PATCH 4/4] pack-bitmap: drop \"loaded\" flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-01T07:50:57Z","receivedAt":"2018-09-01T07:51:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the early days of the bitmap code, there was a single\nstatic bitmap_index struct that was used behind the scenes,\nand any bitmap-related functions could lazily check\nbitmap_git.loaded to see if they needed to read the on-disk\ndata.\n\nBut since 3ae5fa0768 (pack-bitmap: remove bitmap_git global\nvariable, 2018-06-07), the caller is responsible for the\nlifetime of the bitmap_index struct, and we return it from\nprepare_bitmap_git() and prepare_bitmap_walk(), both of\nwhich load the on-disk data (or return NULL).\n\nSo outside of these functions, it's not possible to have a\nbitmap_index for which the loaded flag is not true. Nor is\nit possible to accidentally pass an already-loaded\nbitmap_index to the loading function (which is static-local\nto the file).\n\nWe can drop this unnecessary and confusing flag.\n\nSigned-off-by: Jeff King \n---\n pack-bitmap.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 8c1af1cca2..40debd5e20 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -91,8 +91,6 @@ struct bitmap_index {\n \n \t/* Version of the bitmap index */\n \tunsigned int version;\n-\n-\tunsigned loaded : 1;\n };\n \n static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)\n@@ -306,7 +304,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \n static int load_pack_bitmap(struct bitmap_index *bitmap_git)\n {\n-\tassert(bitmap_git->map && !bitmap_git->loaded);\n+\tassert(bitmap_git->map);\n \n \tbitmap_git->bitmaps = kh_init_sha1();\n \tbitmap_git->ext_index.positions = kh_init_sha1_pos();\n@@ -321,7 +319,6 @@ static int load_pack_bitmap(struct bitmap_index *bitmap_git)\n \tif (load_bitmap_entries_v1(bitmap_git) < 0)\n \t\tgoto failed;\n \n-\tbitmap_git->loaded = 1;\n \treturn 0;\n \n failed:\n@@ -336,7 +333,7 @@ static int open_pack_bitmap(struct bitmap_index *bitmap_git)\n \tstruct packed_git *p;\n \tint ret = -1;\n \n-\tassert(!bitmap_git->map && !bitmap_git->loaded);\n+\tassert(!bitmap_git->map);\n \n \tfor (p = get_packed_git(the_repository); p; p = p->next) {\n \t\tif (open_pack_bitmap_1(bitmap_git, p) == 0)\n@@ -738,7 +735,7 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)\n \t * from disk. this is the point of no return; after this the rev_list\n \t * becomes invalidated and we must perform the revwalk through bitmaps\n \t */\n-\tif (!bitmap_git->loaded && load_pack_bitmap(bitmap_git) < 0)\n+\tif (load_pack_bitmap(bitmap_git) < 0)\n \t\tgoto cleanup;\n \n \tobject_array_clear(&revs->pending);\n-- \n2.19.0.rc1.549.g6ce4dc0f08\n"},{"id":"357153","messageId":"20180901080316.GA25852@sigill.intra.peff.net","threadId":"49179","inReplyTo":"20180901074813.GB25461@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] t5310: test delta reuse with bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-01T08:03:16Z","receivedAt":"2018-09-01T08:03:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 01, 2018 at 03:48:13AM -0400, Jeff King wrote:\n\n> Commit 6a1e32d532 (pack-objects: reuse on-disk deltas for\n> thin \"have\" objects, 2018-08-21) taught pack-objects a new\n> optimization trick. Since this wasn't meant to change\n> user-visible behavior, but only produce smaller packs more\n> quickly, testing focused on t/perf/p5311.\n> \n> However, since people don't run perf tests very often, we\n> should make sure that the feature is exercised in the\n> regular test suite. This patch does so.\n\nThis, by the way, is the crux of how such an obvious and severe bug made\nit to 'next'.\n\nThe original series was tested quite extensively via t/perf and in\nproduction at GitHub. When I re-rolled v2, the only change was the\naddition of the assertion, so I didn't bother re-doing the perf tests,\nsince they're slow and there wouldn't be a measurable impact.\n\nI did run the normal test suite (as I'm sure Junio did, too) as a\ndouble-check for correctness, but as we noticed, the code wasn't\nactually exercised there.\n\nNor had I yet backported the revised series to the version we run at\nGitHub, so it hadn't been run there, either.\n\nAnd all of that coupled with the fact that it only triggers with\nbitmaps, so day-to-day use of the buggy Git (like Junio trying to push\nout the result ;) ) wouldn't show it.\n\nAnyway. Not that exciting, and kind of obviously dumb in retrospect. But\nI think it was worth analyzing to see what went wrong. If there's an\nimmediate lesson, it is probably: add tests even for changes that aren't\nreally user-visible to make sure the code is exercised.\n\nThere may be a larger lesson about tracking code coverage, but I don't\nknow that most general code coverage tools would have helped (any\noverall percentage number would be too large to move). A tool that\nlooked at the diff and said \"of the N lines you added/touched, this\npercent is exercised in the test suite\" might have been useful.\n\n-Peff\n"},{"id":"357162","messageId":"87va7parmi.fsf@evledraar.gmail.com","threadId":"49179","inReplyTo":"20180901080316.GA25852@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] t5310: test delta reuse with bitmaps","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-01T20:29:25Z","receivedAt":"2018-09-01T20:29:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Sep 01 2018, Jeff King wrote:\n\n> On Sat, Sep 01, 2018 at 03:48:13AM -0400, Jeff King wrote:\n>\n>> Commit 6a1e32d532 (pack-objects: reuse on-disk deltas for\n>> thin \"have\" objects, 2018-08-21) taught pack-objects a new\n>> optimization trick. Since this wasn't meant to change\n>> user-visible behavior, but only produce smaller packs more\n>> quickly, testing focused on t/perf/p5311.\n>>\n>> However, since people don't run perf tests very often, we\n>> should make sure that the feature is exercised in the\n>> regular test suite. This patch does so.\n>\n> This, by the way, is the crux of how such an obvious and severe bug made\n> it to 'next'.\n>\n> The original series was tested quite extensively via t/perf and in\n> production at GitHub. When I re-rolled v2, the only change was the\n> addition of the assertion, so I didn't bother re-doing the perf tests,\n> since they're slow and there wouldn't be a measurable impact.\n>\n> I did run the normal test suite (as I'm sure Junio did, too) as a\n> double-check for correctness, but as we noticed, the code wasn't\n> actually exercised there.\n>\n> Nor had I yet backported the revised series to the version we run at\n> GitHub, so it hadn't been run there, either.\n>\n> And all of that coupled with the fact that it only triggers with\n> bitmaps, so day-to-day use of the buggy Git (like Junio trying to push\n> out the result ;) ) wouldn't show it.\n>\n> Anyway. Not that exciting, and kind of obviously dumb in retrospect. But\n> I think it was worth analyzing to see what went wrong. If there's an\n> immediate lesson, it is probably: add tests even for changes that aren't\n> really user-visible to make sure the code is exercised.\n\nTest-wise, isn't the problem rather that that we didn't have something\nlike what's described in t/README as \"Running tests with special setups\"\nfor bitmaps? I.e. stuff like GIT_TEST_SPLIT_INDEX=, or running it\nwith GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all to stress the fsmonitor\ncode.\n\nThat comment b.t.w. is not meant as a \"you should have done that!\"\nblame, but just musings on how we could make things better.\n\nGit has things like bitmaps, midx, commit graph, and probably a few\nother things I'm forgetting which all have their own tests, but really\nfall more in the category of something like the split index in that they\ncan potentially impact every test in some unexpected way.\n\nSo we could add some option to the test suite to e.g. run a custom\ncommand before every \"git push\" or \"git fetch\", and then just do a gc\nwith a repack/commit graph write/midx write etc. in that codepath, along\nwith (in the case of stuff like midx) setting any neede config knobs to\nturn it on.\n\nOf course the utility of that sort of thing is limited unless we have\nsome dedicated smoke testers or CI capacity to run the various\ncombinations of those options. But FWIW when I build our own in-house\ngit I build the package with:\n\n # Set \"false\" to test the build procedure itself\n if true\n then\n export BKNG_GIT_HARNESS_OPTIONS=\"%{?_smp_mflags} --state=failed,slow,save --timer\"\n echo Testing without any custom options:\n (cd t && /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)\n\n echo Testing while roundtripping everything through the fsmonitor codepath:\n (cd t && GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all GIT_SKIP_TESTS=\"t3404.7 t7411.3 t7411.4\" /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)\n\n echo Testing split index\n (cd t && GIT_TEST_SPLIT_INDEX=true GIT_SKIP_TESTS=\"t3903 t4015.77\" /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)\n\n echo Testing uncommon pack modes. See ci/run-tests.sh in git\n (cd t && GIT_TEST_FULL_IN_PACK_ARRAY=true GIT_TEST_OE_SIZE=10 /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)\n fi\n\nThose skipped tests are various intermittent bugs related to those\ncodpaths which I haven't had time to track down / report yet.\n\nSo if there was a \"test bitmaps everywhere\" mode that would have been\ncaught during the build, unless I've misunderstood how this particular\nbug manifests, but then again, it happened on just a plain git.git after\nrepack, so wasn't any bitmap + push pretty much all that was needed?, I\nhaven't read your patches in any detail.\n\nB.t.w. for Ben or anyone else who knows about the fsmonitor part of\nthis: I've long been running the whole test suite with\n`GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all prove ...` (also along with\nGIT_TEST_SPLIT_INDEX=) after all the main tests pass as additional\nstress testing.\n\nIt's not documented under the \"special setups\" section. So I was going\nto add it, but I see that in 5c8cdcfd80 (\"fsmonitor: add test cases for\nfsmonitor extension\", 2017-09-22) it's documented that you should also\nset GIT_FORCE_PRELOAD_TEST=true, is that needed for GIT_FSMONITOR_TEST?\nOr is it yet another mode, and if so to be combined with fsmonitor in\nparticular, or stand-alone?\n\n> There may be a larger lesson about tracking code coverage, but I don't\n> know that most general code coverage tools would have helped (any\n> overall percentage number would be too large to move). A tool that\n> looked at the diff and said \"of the N lines you added/touched, this\n> percent is exercised in the test suite\" might have been useful.\n\nThis would be very useful.\n"},{"id":"357166","messageId":"ececf0e9-f569-dd07-9ab1-dc65ebb0a814@gmail.com","threadId":"49179","inReplyTo":"87va7parmi.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/4] t5310: test delta reuse with bitmaps","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-09-01T22:46:40Z","receivedAt":"2018-09-01T22:46:45Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 9/1/2018 4:29 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> \n> B.t.w. for Ben or anyone else who knows about the fsmonitor part of\n> this: I've long been running the whole test suite with\n> `GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all prove ...` (also along with\n> GIT_TEST_SPLIT_INDEX=) after all the main tests pass as additional\n> stress testing.\n> \n> It's not documented under the \"special setups\" section. So I was going\n> to add it, but I see that in 5c8cdcfd80 (\"fsmonitor: add test cases for\n> fsmonitor extension\", 2017-09-22) it's documented that you should also\n> set GIT_FORCE_PRELOAD_TEST=true, is that needed for GIT_FSMONITOR_TEST?\n> Or is it yet another mode, and if so to be combined with fsmonitor in\n> particular, or stand-alone?\n> \n\nI was unaware of the \"special setups\" section until recently so if you \nwould add the fsmonitor information that would be great.\n\nI added the GIT_FORCE_PRELOAD_TEST as when I went to test fsmonitor I \nrealized there was no way to manually override the default behavior and \nforce preload to happen. I added tests into t7519-status-fsmonitor.sh \nthat use this capability to test fsmonitor with and without preload to \nensure they play nice together as I had to add logic into the preload \ncode to mark cache entries as clean for fsmonitor.\n\nGIT_FORCE_PRELOAD_TEST should probably be added to the \"special setups\" \nas well so that the entire test suite can be used to exercise that code \npath.\n\nUltimately, GIT_FSMONITOR_TEST and GIT_FORCE_PRELOAD_TEST can be used to \ntest their respective code paths using the test suite. The challenge is \nthat the number of potential combinations (including split index, \nuncommon pack modes, etc) gets large pretty quickly.\n\n>> There may be a larger lesson about tracking code coverage, but I don't\n>> know that most general code coverage tools would have helped (any\n>> overall percentage number would be too large to move). A tool that\n>> looked at the diff and said \"of the N lines you added/touched, this\n>> percent is exercised in the test suite\" might have been useful.\n> \n> This would be very useful.\n> \n"},{"id":"357176","messageId":"20180902055159.GB21324@sigill.intra.peff.net","threadId":"49179","inReplyTo":"87va7parmi.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/4] t5310: test delta reuse with bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-02T05:51:59Z","receivedAt":"2018-09-02T05:52:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 01, 2018 at 10:29:25PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Anyway. Not that exciting, and kind of obviously dumb in retrospect. But\n> > I think it was worth analyzing to see what went wrong. If there's an\n> > immediate lesson, it is probably: add tests even for changes that aren't\n> > really user-visible to make sure the code is exercised.\n> \n> Test-wise, isn't the problem rather that that we didn't have something\n> like what's described in t/README as \"Running tests with special setups\"\n> for bitmaps? I.e. stuff like GIT_TEST_SPLIT_INDEX=, or running it\n> with GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all to stress the fsmonitor\n> code.\n\nI'm a little skeptical of that as a general approach, for two reasons:\n\n - the test suite is filled with toy examples, so they often don't\n trigger the interesting cases\n\n - the tests are often looking for very particular outcomes or\n repository state; munging that state is going to confuse them\n\n> So we could add some option to the test suite to e.g. run a custom\n> command before every \"git push\" or \"git fetch\", and then just do a gc\n> with a repack/commit graph write/midx write etc. in that codepath, along\n> with (in the case of stuff like midx) setting any neede config knobs to\n> turn it on.\n\nWe can try that out with something like this:\n\ndiff --git a/builtin/upload-pack.c b/builtin/upload-pack.c\nindex 42dc4da5a1..f40e0b7a04 100644\n--- a/builtin/upload-pack.c\n+++ b/builtin/upload-pack.c\n@@ -30,6 +30,14 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n+\t/*\n+\t * This environment variable hackery is necessary because repack in a\n+\t * partial clone might actually try to fetch, spawning an infinite\n+\t * recursion.\n+\t */\n+\tsystem(\"test -z \\\"$SUPPRESS_BITMAP_MAGIC\\\" && \"\n+\t \"SUPPRESS_BITMAP_MAGIC=1 git repack -adb >/dev/null 2>&1\");\n+\n \tpacket_trace_identity(\"upload-pack\");\n \tread_replace_refs = 0;\n \n\nIt actually _does_ find the bug in question (which I wasn't at all sure\nit would). But it also turns up several other unrelated test failures.\n\nAnd it's only triggering in some limited cases (fetches). If we tried to\nget better coverage of bitmaps in general, I'm not sure where we would\nslip in such a repack command. But I think you'd get even more false\npositives.\n\n> Of course the utility of that sort of thing is limited unless we have\n> some dedicated smoke testers or CI capacity to run the various\n> combinations of those options. But FWIW when I build our own in-house\n> git I build the package with:\n\nYes, it also gets really expensive. That can be helped with more CI\nmachines, but even neglecting cost, I'm not sure our CI workflow is that\ngreat right now (for example, I still haven't figured out the simple\nfeature of: if I push up a commit that fails, I'd like to get an email).\n\n> So if there was a \"test bitmaps everywhere\" mode that would have been\n> caught during the build, unless I've misunderstood how this particular\n> bug manifests, but then again, it happened on just a plain git.git after\n> repack, so wasn't any bitmap + push pretty much all that was needed?, I\n> haven't read your patches in any detail.\n\nThe test patch describes the minimal scenario you need. Which would be\npretty common on any decent sized repo, but rare on toy ones (though as\nI said above, there are a few cases in the test suite big enough to\ntrigger this).\n\n-Peff\n"},{"id":"357326","messageId":"CAGZ79ka8e2-f4fYgy+=HUDdvugefvQ5TnDG0v8YmUn7kGhTdaQ@mail.gmail.com","threadId":"49179","inReplyTo":"20180901080316.GA25852@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] t5310: test delta reuse with bitmaps","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T19:05:58Z","receivedAt":"2018-09-04T19:06:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Sep 1, 2018 at 1:03 AM Jeff King wrote:\n>\n> On Sat, Sep 01, 2018 at 03:48:13AM -0400, Jeff King wrote:\n>\n> > Commit 6a1e32d532 (pack-objects: reuse on-disk deltas for\n> > thin \"have\" objects, 2018-08-21) taught pack-objects a new\n> > optimization trick. Since this wasn't meant to change\n> > user-visible behavior, but only produce smaller packs more\n> > quickly, testing focused on t/perf/p5311.\n> >\n> > However, since people don't run perf tests very often, we\n> > should make sure that the feature is exercised in the\n> > regular test suite. This patch does so.\n>\n> This, by the way, is the crux of how such an obvious and severe bug made\n> it to 'next'.\n>\n> The original series was tested quite extensively via t/perf and in\n> production at GitHub. When I re-rolled v2, the only change was the\n> addition of the assertion, so I didn't bother re-doing the perf tests,\n> since they're slow and there wouldn't be a measurable impact.\n>\n> I did run the normal test suite (as I'm sure Junio did, too) as a\n> double-check for correctness, but as we noticed, the code wasn't\n> actually exercised there.\n>\n> Nor had I yet backported the revised series to the version we run at\n> GitHub, so it hadn't been run there, either.\n>\n> And all of that coupled with the fact that it only triggers with\n> bitmaps, so day-to-day use of the buggy Git (like Junio trying to push\n> out the result ;) ) wouldn't show it.\n>\n> Anyway. Not that exciting, and kind of obviously dumb in retrospect. But\n> I think it was worth analyzing to see what went wrong. If there's an\n> immediate lesson, it is probably: add tests even for changes that aren't\n> really user-visible to make sure the code is exercised.\n\nYeah, maybe we need to ask for more tests in the 'real' test suite, and not\njust in some special corner (such as t/perf/ or any of the environment\nvariable proposals nearby).\n\nI wonder if we can make use of git.git in the test suite for similar things,\ne.g. after reading the thread about \"index corruption with git commit -p\" [1],\nI thought that we only have these toy examples in the test suite. Toy examples\nshow that the new feature barely works, and doesn't show it is working at scale.\n\n[1] https://public-inbox.org/git/20180901214157.hxlqmbz3fds7hsdl@ltop.local/\n\n> There may be a larger lesson about tracking code coverage, but I don't\n> know that most general code coverage tools would have helped (any\n> overall percentage number would be too large to move). A tool that\n> looked at the diff and said \"of the N lines you added/touched, this\n> percent is exercised in the test suite\" might have been useful.\n\nFrom some offline discussion, maybe we want to adapt a philosophy of\n\n Each patch needs to add a test, that fails when the patch\n is not applied, but succeeds when it is applied. This shows\n that _some_ code in the patch is exercised at least.\n\n(and automatically/strongly enforce this going forwards; however\nenforcing such a strict thing is hard, not sure how we'd do it.)\n\nStefan\n"},{"id":"357328","messageId":"CAGZ79kZtXOmKo-86x=x4Z+tA0uiSmy4ntX+U4aCUYSjy4_DVDw@mail.gmail.com","threadId":"49179","inReplyTo":"20180901074145.GA24023@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] un-breaking pack-objects with bitmaps","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-04T19:30:14Z","receivedAt":"2018-09-04T19:30:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> [1/4]: bitmap_has_sha1_in_uninteresting(): drop BUG check\n>\n> The actual fix. This should get merged to next ASAP (or the original\n> topic just reverted).\n>\n> [2/4]: t5310: test delta reuse with bitmaps\n>\n> I did this separately to give us flexibility to squash or merge\n> quickly. But it does find Ævar's bug on a git without patch 1.\n>\n> [3/4]: traverse_bitmap_commit_list(): don't free result\n>\n> The original assert should have simply been useless, but it was the\n> surprising behavior of this function that turned it into a bug.\n>\n> [4/4]: pack-bitmap: drop \"loaded\" flag\n>\n> And this is just an annoyance I ran into, which is a fallout from\n> our conversion to using an allocated bitmap_index struct.\n\nFWIW, the whole series is\nReviewed-by: Stefan Beller \nI am sorry for suggesting the BUG() in the first place.\n\nStefan\n"},{"id":"357333","messageId":"xmqqin3l82sl.fsf@gitster-ct.c.googlers.com","threadId":"49179","inReplyTo":"CAGZ79ka8e2-f4fYgy+=HUDdvugefvQ5TnDG0v8YmUn7kGhTdaQ@mail.gmail.com","subject":"Re: [PATCH 2/4] t5310: test delta reuse with bitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-04T19:45:30Z","receivedAt":"2018-09-04T19:45:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> From some offline discussion, maybe we want to adapt a philosophy of\n>\n> Each patch needs to add a test, that fails when the patch\n> is not applied, but succeeds when it is applied. This shows\n> that _some_ code in the patch is exercised at least.\n>\n> (and automatically/strongly enforce this going forwards; however\n> enforcing such a strict thing is hard, not sure how we'd do it.)\n\nSome patches lack test, but when one comes with a test, I do revert\nthe code change and try to run the tests before I create a topic\nbranch for it, when I have time.\n\nAn enforcement mechanism may be good (e.g. submitGit could learn to\ndo that \"does it come with a test\" check followed by \"let's see if\nthe new test fail without the change to the code part\" before\nallowing the patch to escape to the list), but \n"},{"id":"357337","messageId":"20180904200224.GB17481@sigill.intra.peff.net","threadId":"49179","inReplyTo":"CAGZ79ka8e2-f4fYgy+=HUDdvugefvQ5TnDG0v8YmUn7kGhTdaQ@mail.gmail.com","subject":"Re: [PATCH 2/4] t5310: test delta reuse with bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-04T20:02:25Z","receivedAt":"2018-09-04T20:02:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 04, 2018 at 12:05:58PM -0700, Stefan Beller wrote:\n\n> Yeah, maybe we need to ask for more tests in the 'real' test suite, and not\n> just in some special corner (such as t/perf/ or any of the environment\n> variable proposals nearby).\n> \n> I wonder if we can make use of git.git in the test suite for similar things,\n> e.g. after reading the thread about \"index corruption with git commit -p\" [1],\n> I thought that we only have these toy examples in the test suite. Toy examples\n> show that the new feature barely works, and doesn't show it is working at scale.\n\nI think the toy examples do both. Often they drill down directly to a\nuseful but rare corner case that _wouldn't_ be hit during normal\noperation. And being toys, they are a lot quicker to set up then trying\nto work with a 50MB repository.\n\nTake the \"commit -p\" one for example. It's not really about the\nrepository shape but about a particular set of actions. If you don't\ntest those actions, you won't reproduce the bug.\n\n> > There may be a larger lesson about tracking code coverage, but I don't\n> > know that most general code coverage tools would have helped (any\n> > overall percentage number would be too large to move). A tool that\n> > looked at the diff and said \"of the N lines you added/touched, this\n> > percent is exercised in the test suite\" might have been useful.\n> \n> From some offline discussion, maybe we want to adapt a philosophy of\n> \n> Each patch needs to add a test, that fails when the patch\n> is not applied, but succeeds when it is applied. This shows\n> that _some_ code in the patch is exercised at least.\n> \n> (and automatically/strongly enforce this going forwards; however\n> enforcing such a strict thing is hard, not sure how we'd do it.)\n\nI wouldn't want a hard-and-fast rule like that. If you're fixing a bug,\nsure, I think it's good to make sure it's exercised. And if you're\nadding a feature, you almost always add some basic tests (and almost\ncertainly leave some corner without code coverage).\n\nBut if you're writing an optimization, there's often no before/after\ntest. Presumably it worked before, and hopefully it still works after,\nand it just got faster. You're generally relying on existing regression\ntests (from when that code was introduced) to save you from bugs. You\nmight need to _write_ those tests if nobody did before. But it's hard to\nknow without digging if there are decent tests or not. That's why I\nthink code coverage of the lines in your diff is probably the best\nmeasure.\n\n-Peff\n"},{"id":"357338","messageId":"20180904200342.GC17481@sigill.intra.peff.net","threadId":"49179","inReplyTo":"CAGZ79kZtXOmKo-86x=x4Z+tA0uiSmy4ntX+U4aCUYSjy4_DVDw@mail.gmail.com","subject":"Re: [PATCH 0/4] un-breaking pack-objects with bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-04T20:03:43Z","receivedAt":"2018-09-04T20:03:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 04, 2018 at 12:30:14PM -0700, Stefan Beller wrote:\n\n> > [1/4]: bitmap_has_sha1_in_uninteresting(): drop BUG check\n> >\n> > The actual fix. This should get merged to next ASAP (or the original\n> > topic just reverted).\n> >\n> > [2/4]: t5310: test delta reuse with bitmaps\n> >\n> > I did this separately to give us flexibility to squash or merge\n> > quickly. But it does find Ævar's bug on a git without patch 1.\n> >\n> > [3/4]: traverse_bitmap_commit_list(): don't free result\n> >\n> > The original assert should have simply been useless, but it was the\n> > surprising behavior of this function that turned it into a bug.\n> >\n> > [4/4]: pack-bitmap: drop \"loaded\" flag\n> >\n> > And this is just an annoyance I ran into, which is a fallout from\n> > our conversion to using an allocated bitmap_index struct.\n> \n> FWIW, the whole series is\n> Reviewed-by: Stefan Beller \n> I am sorry for suggesting the BUG() in the first place.\n\nHeh. You only asked a question about the interface. I was the one who\nwas _supposed_ to be familiar with the code, and put in the actual\nassertion. So if your suggestion was dumb, my response was doubly so. :)\n\n-Peff\n"},{"id":"357681","messageId":"87d0toqyj6.fsf@evledraar.gmail.com","threadId":"49179","inReplyTo":"20180901074145.GA24023@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] un-breaking pack-objects with bitmaps","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-08T06:43:57Z","receivedAt":"2018-09-08T06:44:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Sep 01 2018, Jeff King wrote:\n\n> On Fri, Aug 31, 2018 at 06:55:58PM -0400, Jeff King wrote:\n>\n>> On Fri, Aug 31, 2018 at 05:23:17PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> > On Tue, Aug 21 2018, Jeff King wrote:\n>> >\n>> > > +int bitmap_has_sha1_in_uninteresting(struct bitmap_index *bitmap_git,\n>> > > +\t\t\t\t const unsigned char *sha1)\n>> > > +{\n>> > > +\tint pos;\n>> > > +\n>> > > +\tif (!bitmap_git)\n>> > > +\t\treturn 0; /* no bitmap loaded */\n>> > > +\tif (!bitmap_git->result)\n>> > > +\t\tBUG(\"failed to perform bitmap walk before querying\");\n>> >\n>> > Some part of what calls this completely breaks pushing from the \"next\"\n>> > branch when you have local bitmaps (we *really* should have some tests\n>> > for this...).\n>>\n>> Yikes, thanks for reporting. I agree we need better tests here.\n>\n> OK, here is the fix. Since the problem is in 'next', this is done as a\n> patch on top of jk/pack-delta-reuse-with-bitmap. But since we're set to\n> rewind 'next' post-release anyway, we could squash it directly into\n> 30cdc33fba from the original series. That would help later bisections\n> from running into it, which may be worth it as it's a pretty severe\n> breakage. Or maybe not:\n\nJunio: Just a reminder that next is still broken with this, and I see\ne.g. the Debian \"experimental\" has the bug but not the fix at this\npoint.\n\nI'm just reverting jk/pack-delta-reuse-with-bitmap out of next when\nbuilding my own package of git, but I think this really should be fixed\nin that branch, either by merging the fix down or reverting the original\nseries out of next, I think just merging the fix down makes sense, but\nhave no strong opinion on it.\n"},{"id":"357795","messageId":"xmqq1sa1uwd7.fsf@gitster-ct.c.googlers.com","threadId":"49179","inReplyTo":"87d0toqyj6.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/4] un-breaking pack-objects with bitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-10T16:53:56Z","receivedAt":"2018-09-10T16:54:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I'm just reverting jk/pack-delta-reuse-with-bitmap out of next when\n> building my own package of git, but I think this really should be fixed\n> in that branch, either by merging the fix down or reverting the original\n> series out of next, I think just merging the fix down makes sense, but\n> have no strong opinion on it.\n\nEither is fine. I am not moving 'next' beyond what is necessary for\nthis release cycle during the pre-release freeze period, and because\nI thought that Peff was in favor of squashing in the changes to the\noriginal when the next cycle starts, I haven't bothered to merge the\nfix there yet.\n"},{"id":"357814","messageId":"20180910184817.GB20678@sigill.intra.peff.net","threadId":"49179","inReplyTo":"xmqq1sa1uwd7.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/4] un-breaking pack-objects with bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-10T18:48:17Z","receivedAt":"2018-09-10T18:48:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2018 at 09:53:56AM -0700, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > I'm just reverting jk/pack-delta-reuse-with-bitmap out of next when\n> > building my own package of git, but I think this really should be fixed\n> > in that branch, either by merging the fix down or reverting the original\n> > series out of next, I think just merging the fix down makes sense, but\n> > have no strong opinion on it.\n> \n> Either is fine. I am not moving 'next' beyond what is necessary for\n> this release cycle during the pre-release freeze period, and because\n> I thought that Peff was in favor of squashing in the changes to the\n> original when the next cycle starts, I haven't bothered to merge the\n> fix there yet.\n\nI think Ævar's point is just that the current tip of next is badly\nbroken if you use bitmaps, and it's worth hot-fixing that in the\nmeantime.\n\nAfter the release, I'm OK with either one of squashing that first patch\nin, or just merging as-is.\n\n-Peff\n"},{"id":"357819","messageId":"xmqqwortrwak.fsf@gitster-ct.c.googlers.com","threadId":"49179","inReplyTo":"20180910184817.GB20678@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] un-breaking pack-objects with bitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-10T19:23:47Z","receivedAt":"2018-09-10T19:23:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Either is fine. I am not moving 'next' beyond what is necessary for\n>> this release cycle during the pre-release freeze period, and because\n>> I thought that Peff was in favor of squashing in the changes to the\n>> original when the next cycle starts, I haven't bothered to merge the\n>> fix there yet.\n>\n> I think Ævar's point is just that the current tip of next is badly\n> broken if you use bitmaps, and it's worth hot-fixing that in the\n> meantime.\n\nI know that ;-)\n\nMy point is that anybody who *needs* handholding from the upstream\nproject (i.e. not Ævar, but those whom he is trying to help with the\nsuggestion) should not be looking at 'next' during the prerelease\nfreeze. They should be looking at 'master' instead, and that is why\nI am not spending more than minimum cycles of my own worrying about\n'next' during the prerelease.\n\nIn any case, I think the final is ready to go, almost. I still need\nto do the preformatted docs, though.\n\n"}]}