[GSoC PATCH v4 0/5] preserve promisor files content after repack

[LorenzoPegorari <lorenzo.pegorari2002@gmail.com>]

The goal of this patch is to solve the NEEDSWORK comment added by
5374a290 (fetch-pack: write fetched refs to .promisor, 14/10/2019). This
is done by adding a helper function that takes the content of all
.promisor files in the `repository`, and copies it inside the first
.promisor file created by the repack.

Also, I added a comment explaining what is the purpose of the content of
the .promisor files, since this wasn't explained anywhere (I found
information regarding this only in the message of the previously cited
commit).

Finally, I added some tests to "t7700-repack.sh" and
"t7703-repack-geometric.sh" that check if the content of .promisor files
are correctly copied into the .promisor files created by a repack.


If Eric Sunshine, Tian Yuchen (for the patch 2/5 "pack-write: add helper
to fill promisor file after repack") and Junio Hamano (for all patches)
want to be added with a `<Reviewed-by>` tag, please let me know (and, of
course, thanks a lot for the help)!


QUESTION:
The "CodingGuidelines" explicitly state that:
 "A C file must directly include the header files that declare the
  functions and the types it uses, except for the functions and types
  that are made available to it by including one of the header files
  it must include by the previous rule"
where "the previous rule" is (if I understand correctly), the one related
to "<git-compat-util.h>". From what I understand then, I should have
added an include for "strmap.h" (which is needed for `strset`), correct?
And if I am correct, shouldn't "strbuf.h", "hash.h", "odb.h",
"string-list.h" and "strvec.h" also be included?


V4 DIFF:
 * `copy_promisor_content()` now prints timestamps in Unix time format.
 * `copy_promisor_content()` now doesn't use a list of `strbuf`, but
   instead uses the more lightweight `string_list`.
 * improved the tests.
 * fixed issue (that showed up in the GitHub Actions-based CI) where
   sometimes the 2 packs created in the second new test inside "t7700"
   were not both repacked into a single new pack. 


LorenzoPegorari (5):
  pack-write: add explanation to promisor file content
  pack-write: add helper to fill promisor file after repack
  repack-promisor: preserve content of promisor files after repack
  t7700: test for promisor file content after repack
  t7703: test for promisor file content after geometric repack

 Documentation/git-repack.adoc |   4 +-
 pack-write.c                  |   9 +++
 repack-promisor.c             | 146 +++++++++++++++++++++++++++++++---
 t/t7700-repack.sh             |  60 ++++++++++++++
 t/t7703-repack-geometric.sh   |  33 ++++++++
 5 files changed, 237 insertions(+), 15 deletions(-)

Range-diff against v3:
1:  eb1964dca8 = 1:  b4990fcdf0 pack-write: add explanation to promisor file content
2:  3cd1542919 ! 2:  34c4e79311 pack-write: add helper to fill promisor file after repack
    @@ Commit message
         stored as lines structured like this: "<oid> <ref>".
     
         Create a `copy_promisor_content()` helper function that allows this
    -    debugging info to not be lost after a `repack`, by coping it inside a new
    -    ".promisor" file.
    +    debugging info to not be lost after a `repack`, by copying it inside a
    +    new ".promisor" file.
     
         The function logic is the following:
          * Take all ".promisor" files contained inside the given `repo`.
    @@ Commit message
            (intended to be used for packfiles that have not been repacked).
          * Read each line of the remaining ".promisor" files, which can be:
             * "<oid> <ref>" if the ".promisor" file was never repacked. If so,
    -          add the time at which the ".promisor" file was last modified <time>
    -          to the line to create the string: "<oid> <ref> <time>".
    +          add the time (in Unix time) at which the ".promisor" file was last
    +          modified <time> to the line, to obtain: "<oid> <ref> <time>".
             * "<oid> <ref> <time>" if the ".promisor" file was repacked. If so,
               don't modify it.
          * Ignore the line if its <oid> is not present inside the
    @@ Commit message
         The function assumes that the contents of all ".promisor" files are
         correctly formed.
     
    -    The time of last data modification is used in place of the time of file
    -    creation, because the former is much easier to obtain than the latter
    -    one.
    +    The time of last data modification, for never-repacked ".promisor" file,
    +    can be used when comparing the entries in it with entries in another
    +    ".promisor" file that did get repacked. With these timestamps, the
    +    debugger will be able to tell at which time the refs at the remote
    +    repository pointed at what object. Also, when looking at already
    +    repacked ".promisor" files, the same ref may appear multiple times, and
    +    having timestamps will help understanding what happened over time.
     
         Signed-off-by: LorenzoPegorari <lorenzo.pegorari2002@gmail.com>
     
    @@ repack-promisor.c: static int write_oid(const struct object_id *oid,
     + * Each line of a never repacked .promisor file is: "<oid> <ref>" (as described
     + * in the write_promisor_file() function).
     + * After a repack, the copied lines will be: "<oid> <ref> <time>", where <time>
    -+ * is the time at which the .promisor file was last modified.
    ++ * is the time (in Unix time) at which the .promisor file was last modified.
     + * Only the lines whose <oid> is present inside "<packtmp>-<dest_hex>.idx" will
     + * be copied.
     + * The contents of all .promisor files are assumed to be correctly formed.
    @@ repack-promisor.c: static int write_oid(const struct object_id *oid,
     +		source = xfopen(source_promisor_name.buf, "r");
     +
     +		while (strbuf_getline(&line, source) != EOF) {
    -+			struct strbuf **parts;
    ++			struct string_list line_sections = STRING_LIST_INIT_DUP;
     +			struct object_id oid;
     +
     +			/* Split line into <oid>, <ref> and <time> (if <time> exists) */
    -+			parts = strbuf_split_max(&line, ' ', 3);
    ++			string_list_split(&line_sections, line.buf, " ", 3);
     +
     +			/* Ignore the lines where <oid> doesn't appear in the dest_pack */
    -+			strbuf_rtrim(parts[0]);
    -+			get_oid_hex_algop(parts[0]->buf, &oid, repo->hash_algo);
    -+			if (!find_pack_entry_one(&oid, dest_pack))
    ++			get_oid_hex_algop(line_sections.items[0].string, &oid, repo->hash_algo);
    ++			if (!find_pack_entry_one(&oid, dest_pack)) {
    ++				string_list_clear(&line_sections, 0);
     +				continue;
    ++			}
     +
     +			/* If <time> doesn't exist, retrieve it and add it to line */
    -+			if (!parts[2]) {
    -+				struct tm tm;
    -+				localtime_r(&source_stat.st_mtim.tv_sec, &tm),
    -+				strbuf_addch(&line, ' ');
    -+				strbuf_addftime(&line, "%Y/%m/%d-%H:%M:%S", &tm, 0, 0);
    -+			}
    ++			if (line_sections.nr < 3)
    ++				strbuf_addf(&line, " %lld", (long long int)source_stat.st_mtim.tv_sec);
     +
     +			/*
     +			 * Add the finalized line to dest_to_write and dest_content if it
    @@ repack-promisor.c: static int write_oid(const struct object_id *oid,
     +				strbuf_addch(&dest_to_write, '\n');
     +			}
     +
    -+			strbuf_list_free(parts);
    ++			string_list_clear(&line_sections, 0);
     +		}
     +
     +		err = ferror(source);
3:  c16b1198fd = 3:  72ef2378b9 repack-promisor: preserve content of promisor files after repack
4:  8e58c1263d ! 4:  0aceaed480 t7700: test for promisor file content after repack
    @@ Metadata
      ## Commit message ##
         t7700: test for promisor file content after repack
     
    -    Add tests that checks if the content of ".promisor" files are correctly
    +    Add tests that check if the content of ".promisor" files are correctly
         copied inside the ".promisor" files created by a repack.
     
    +    The `-f` flag is used when repacking to ensure that all the packs
    +    (created with `test_commit_bulk`) are repacked into a single new pack.
    +
         Signed-off-by: LorenzoPegorari <lorenzo.pegorari2002@gmail.com>
     
      ## t/t7700-repack.sh ##
    @@ t/t7700-repack.sh: test_expect_success 'pending objects are repacked appropriate
      '
      
     +test_expect_success 'check one .promisor file content after repack' '
    -+	test_when_finished rm -rf prom_test &&
    ++	test_when_finished rm -rf prom_test prom_before_repack &&
     +	git init prom_test &&
     +	path=prom_test/.git/objects/pack &&
     +
     +	(
    -+		test_commit_bulk -C prom_test --start=1 1 &&
    -+		
    ++		test_commit_bulk -C prom_test 1 &&
    ++
     +		# Simulate .promisor file by creating it manually
     +		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
     +		oid=$(git -C prom_test rev-parse HEAD) &&
    -+		echo "$oid ref" >$prom &&
    ++		echo "$oid ref" >"$prom" &&
     +
    -+		# Save the current .promisor content, repack, and check if correct
    -+		prom_before_repack=$(cat $prom) &&
    -+		git -C prom_test repack -a -d &&
    -+		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
    -+		# $prom should contain "$prom_before_repack <date>"
    -+		test_grep "$prom_before_repack " $prom &&
    ++		# Repack, and check if correct
    ++		git -C prom_test repack -a -d -f &&
    ++		prom=$(ls $path/*.promisor) &&
    ++		# $prom should contain "$oid ref <time>"
    ++		test_grep "$prom_before_repack " "$prom" &&
     +
     +		# Save the current .promisor content, repack, and check if correct
    -+		cat $prom >prom_before_repack &&
    -+		git -C prom_test repack -a -d &&
    -+		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
    ++		cp "$prom" prom_before_repack &&
    ++		git -C prom_test repack -a -d -f &&
    ++		prom=$(ls $path/*.promisor) &&
     +		# $prom should be exactly the same as prom_before_repack
    -+		test_cmp prom_before_repack $prom
    ++		test_cmp prom_before_repack "$prom"
     +	)
     +'
     +
     +test_expect_success 'check multiple .promisor file content after repack' '
    -+	test_when_finished rm -rf prom_test &&
    ++	test_when_finished rm -rf prom_test prom_before_repack &&
     +	git init prom_test &&
     +	path=prom_test/.git/objects/pack &&
     +
     +	(
     +		# Create 2 packs and simulate .promisor files by creating them manually
    -+		test_commit_bulk -C prom_test --start=1 1 &&
    ++		test_commit_bulk -C prom_test 1 &&
     +		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
    -+		oid=$(git -C prom_test rev-parse HEAD) &&
    -+		echo "$oid ref" >$prom &&
    -+		prom_before_repack1=$(cat $prom) &&
    -+		test_commit_bulk -C prom_test --start=1 1 &&
    -+		prom=$(ls -t $path/*.pack | head -n 1 | sed "s/\.pack/.promisor/") &&
    -+		oid=$(git -C prom_test rev-parse HEAD) &&
    -+		echo "$oid ref" >$prom &&
    -+		prom_before_repack2=$(cat $prom) &&
    ++		oid1=$(git -C prom_test rev-parse HEAD) &&
    ++		echo "$oid1 ref1" >"$prom" &&
    ++		test_commit_bulk -C prom_test 1 &&
    ++		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/; \|$prom|d") &&
    ++		oid2=$(git -C prom_test rev-parse HEAD) &&
    ++		echo "$oid2 ref2" >"$prom" &&
     +
    -+		# Repack, and check if correct compared to previous saved .promisor content
    -+		git -C prom_test repack -a -d &&
    -+		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
    -+		# $prom should contain "$prom_before_repack1 <date>" & "$prom_before_repack2 <date>"
    -+		test_grep "$prom_before_repack1 " $prom &&
    -+		test_grep "$prom_before_repack2 " $prom &&
    ++		# Repack, and check if correct
    ++		git -C prom_test repack -a -d -f &&
    ++		prom=$(ls $path/*.promisor) &&
    ++		# $prom should contain "$oid1 ref1 <time>" & "$oid2 ref2 <time>"
    ++		test_grep "$oid1 ref1 " "$prom" &&
    ++		test_grep "$oid2 ref2 " "$prom" &&
     +
     +		# Save the current .promisor content, repack, and check if correct
    -+		cat $prom >prom_before_repack &&
    -+		git -C prom_test repack -a -d &&
    -+		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
    ++		cp "$prom" prom_before_repack &&
    ++		git -C prom_test repack -a -d -f &&
    ++		prom=$(ls $path/*.promisor) &&
     +		# $prom should be exactly the same as prom_before_repack
    -+		test_cmp prom_before_repack $prom
    ++		test_cmp prom_before_repack "$prom"
     +	)
     +'
     +
5:  1533fa96a8 ! 5:  d9f6341481 t7703: test for promisor file content after geometric repack
    @@ t/t7703-repack-geometric.sh: test_expect_success 'geometric repack works with pr
     +	(
     +		# Create 2 packs with 3 objs each, and manually create .promisor files
     +		test_commit_bulk -C prom_test --start=1 1 &&  # 3 objects
    -+		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
    -+		oid=$(git -C prom_test rev-parse HEAD) &&
    -+		echo "$oid ref" >$prom &&
    -+		prom_before_repack1=$(cat $prom) &&
    ++		prom1=$(ls $path/*.pack | sed "s/\.pack/.promisor/") &&
    ++		oid1=$(git -C prom_test rev-parse HEAD) &&
    ++		echo "$oid1 ref1" >"$prom1" &&
     +		test_commit_bulk -C prom_test --start=2 1 &&  # 3 objects
    -+		prom=$(ls -t $path/*.pack | head -n 1 | sed "s/\.pack/.promisor/") &&
    -+		oid=$(git -C prom_test rev-parse HEAD) &&
    -+		echo "$oid ref" >$prom &&
    -+		prom_before_repack2=$(cat $prom) &&
    ++		prom2=$(ls $path/*.pack | sed "s/\.pack/.promisor/; \|$prom1|d") &&
    ++		oid2=$(git -C prom_test rev-parse HEAD) &&
    ++		echo "$oid2 ref2" >"$prom2" &&
     +
    -+		# Create 2 packs with 12 and 24 objs, and manually create .promisor files
    ++		# Create 1 pack with 12 objs, and manually create .promisor file
     +		test_commit_bulk -C prom_test --start=3 4 &&  # 12 objects
    -+		prom=$(ls -t $path/*.pack | head -n 1 | sed "s/\.pack/.promisor/") &&
    -+		oid=$(git -C prom_test rev-parse HEAD) &&
    -+		echo "$oid ref" >$prom &&
    -+		prom_before_repack3=$(cat $prom) &&
    -+		test_commit_bulk -C prom_test --start=7 8 &&  # 24 objects
    -+		prom=$(ls -t $path/*.pack | head -n 1 | sed "s/\.pack/.promisor/") &&
    -+		oid=$(git -C prom_test rev-parse HEAD) &&
    -+		echo "$oid ref" >$prom &&
    -+		prom_before_repack4=$(cat $prom) &&
    ++		prom3=$(ls $path/*.pack | sed "s/\.pack/.promisor/; \|$prom1|d; \|$prom2|d") &&
    ++		oid3=$(git -C prom_test rev-parse HEAD) &&
    ++		echo "$oid3 ref3" >"$prom3" &&
     +
    -+		# Geometric repack, and check if correct compared to previous saved .promisor content
    ++		# Geometric repack, and check if correct
     +		git -C prom_test repack --geometric 2 -d &&
    -+		prom=$(ls -t $path/*.pack | head -n 1 | sed "s/\.pack/.promisor/") &&
    -+		# $prom should have repacked only the first 2 small packs, so it should only contain
    -+		# the following: "$prom_before_repack1 <date>" & "$prom_before_repack2 <date>"
    -+		test_grep "$prom_before_repack1 " $prom &&
    -+		test_grep "$prom_before_repack2 " $prom &&
    -+		test_grep ! $prom_before_repack3 $prom &&
    -+		test_grep ! $prom_before_repack4 $prom
    ++		prom=$(ls $path/*.pack | sed "s/\.pack/.promisor/; \|$prom3|d") &&
    ++		# $prom should have repacked only the first 2 small packs, so it should only
    ++		# contain the following: "$oid1 ref1 <time>" & "$oid2 ref2 <time>"
    ++		test_grep "$oid1 ref1 " "$prom" &&
    ++		test_grep "$oid2 ref2 " "$prom" &&
    ++		test_grep ! "$oid3 ref3" "$prom"
     +	)
     +'
     +
-- 
2.53.0.585.ge25071d955