git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/2] pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 8, 2016, 19:26 UTC
Message-ID
<xmqqoa53dpue.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<20160808181942.GD3995@teco.navytux.spb.ru>
Kirill Smelkov <kirr@nexedi.com> writes:
> ---- 8< ----
> From: Kirill Smelkov <kirr@nexedi.com>
> Subject: [PATCH v3] pack-objects: Teach --use-bitmap-index codepath to respect
>  --local, --honor-pack-keep and --incremental
(Not a question to Kirill)

Hmph. I suspect that handling of in-body header by mailinfo not prepared to see RFC2822 header folding. "am -c" gives a single line subject with " --local ..." as its first line in the body.

I'll leave it as a low-hanging fruit for somebody to fix ;-)
	Subject: pack-objects: respect --local, etc. when bitmap is in use
might be shorter and more to the point, anyway.
Show 27 quoted lines
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index c4c2a3c..e06c1bf 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -944,13 +944,45 @@ static int have_duplicate_entry(const unsigned char *sha1,
>  	return 1;
>  }
>  
> +static int want_found_object(int exclude, struct packed_git *p)
> +{
> +	if (exclude)
> +		return 1;
> +	if (incremental)
> +		return 0;
> +
> +	/*
> +	 * When asked to do --local (do not include an
> +	 * object that appears in a pack we borrow
> +	 * from elsewhere) or --honor-pack-keep (do not
> +	 * include an object that appears in a pack marked
> +	 * with .keep), we need to make sure no copy of this
> +	 * object come from in _any_ pack that causes us to
> +	 * omit it, and need to complete this loop.  When
> +	 * neither option is in effect, we know the object
> +	 * we just found is going to be packed, so break
> +	 * out of the search loop now.
> +	 */

The blame is mine, but "no copy of this object appears in _any_ pack" would be more correct and easier to read.

This code is no longer in a search loop; its caller is. Further rephrasing is needed. "When asked to do ...these things..., finding a pack that matches the criteria is sufficient for us to decide to omit it. However, even if this pack does not satisify the criteria, we need to make sure no copy of this object appears in _any_ pack that makes us to omit the object, so we need to check all the packs. Signal that by returning -1 to the caller." or something along that line.

Show 10 quoted lines
>  /*
>   * Check whether we want the object in the pack (e.g., we do not want
>   * objects found in non-local stores if the "--local" option was used).
>   *
> - * As a side effect of this check, we will find the packed version of this
> - * object, if any. We therefore pass out the pack information to avoid having
> - * to look it up again later.
> + * As a side effect of this check, if object's pack entry was not already found,
> + * we will find the packed version of this object, if any. We therefore pass
> + * out the pack information to avoid having to look it up again later.

The reasoning leading to "We therefore" is understandable, but "pass out the pack information" is not quite. Is this meant to explain the fact that *found_pack and *found_offset are in-out parameters?

The explanation to justify why *found_pack and *found_offset that used to be out parameters are made in-out parameters belongs to the log message. We do not want this in-code comment to explain the updated code relative to what the code used to do; that is not useful to those who read the code for the first time in the context of the committed state.

        /* 
         * Check whether we want to pack the object in the pack (e.g. ...).
         *
         * If the caller already knows an existing pack it wants to
         * take the object from, that is passed in *found_pack and
         * *found_offset; otherwise this function finds if there is
         * any pack that has the object and returns the pack and its
         * offset in these variables.
         */
Show 12 quoted lines
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 3893afd..e71caa4 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -7,6 +7,19 @@ objpath () {
>  	echo ".git/objects/$(echo "$1" | sed -e 's|\(..\)|\1/|')"
>  }
>  
> +# show objects present in pack ($1 should be associated *.idx)
> +packobjects () {
> +	git show-index <$1 | cut -d' ' -f2
> +}

That is a misleading name for a helper function that produces a list of objects that were packed. "list_packed_objects", perhaps.

Show 7 quoted lines
> +# hasany pattern-file content-file
> +# tests whether content-file has any entry from pattern-file with entries being
> +# whole lines.
> +hasany () {
> +	# NOTE `grep -f` is not portable
> +	git grep --no-index -qFf $1 $2
> +}

I doubt "grep -f pattern_file" is not portable, but in any case, it is probably a good idea to have this helper function to make the caller easier to read. Please name it "has_any", though, and quote "$1" and "$2" as they are meant to be able to take any filename.

> +test_expect_success 'pack-objects respects --local (non-local loose)' '
> +	mkdir -p alt_objects/pack &&

I'd really really prefer to see an empty repository created for this. Even though the original intent was .git/objects/ alone, i.e. GIT_OBJECT_DIRECTORY can exist without associated refs, we discovered that it is in general not a good idea (think: "gc").

> +	echo $(pwd)/alt_objects >.git/objects/info/alternates &&
> +	echo content1 >file1 &&
> +	# non-local loose object which is not present in bitmapped pack
> +	objsha1=$(GIT_OBJECT_DIRECTORY=alt_objects git hash-object -w file1) &&

Don't say "sha" when you mean "object name". Otherwise you would end up introducing funky variable names like $objsha2 we see below that is confusing (we don't use SHA-2).

Show 9 quoted lines
> +	# non-local loose object which is also present in bitmapped pack
> +	git cat-file blob $blob | GIT_OBJECT_DIRECTORY=alt_objects git hash-object -w --stdin &&
> +	git add file1 &&
> +	test_tick &&
> +	git commit -m commit_file1 &&
> +	echo HEAD | git pack-objects --local --stdout --revs >1.pack &&
> +	git index-pack 1.pack &&
> +	packobjects 1.idx >1.objects &&
> +	printf "$objsha1\n$blob\n" >nonlocal-loose &&
I think Peff meant to suggest this instead:
	printf "%s\n" "$objsha1" "$blob"
> +	if hasany nonlocal-loose 1.objects; then
> +		echo "Non-local object present in pack generated with --local"
> +		return 1
> +	fi
Just saying
	! has_any nonlocal-loose 1.objects

is sufficient. Same comment for all other uses of these verbose output.

Besides, we spell "if/then/fi" like this:
	if condition
        then
        	body
	fi
without a semicolon.
> +test_expect_success 'pack-objects respects --honor-pack-keep (local non-bitmapped pack)' '
> +...
> +	touch .git/objects/pack/pack2-$pack2.keep &&

Please don't do "touch" _unless_ you care about the timestamp of the file. Redirect an empty command into it, i.e.

	>.git/objects/pack/pack2-$pack2.keep
or
	echo "reason to keep it" >.git/objects/pack/pack2-$pack2.keep
instead.
Show 14 quoted lines
> +test_expect_success 'pack-objects respects --honor-pack-keep (local bitmapped pack)' '
> +	ls .git/objects/pack/ | grep bitmap >output &&
> +	test_line_count = 1 output &&
> +	packbitmap=$(basename $(cat output) .bitmap) &&
> +	packobjects .git/objects/pack/$packbitmap.idx >packbitmap.objects &&
> +	touch .git/objects/pack/$packbitmap.keep &&
> +	echo HEAD | git pack-objects --honor-pack-keep --stdout --revs >3a.pack &&
> +	git index-pack 3a.pack &&
> +	packobjects 3a.idx >3a.objects &&
> +	if hasany packbitmap.objects 3a.objects; then
> +		echo "Object from .keeped bitmapped pack present in pack generated with --honour-pack-keep"
> +		return 1
> +	fi &&
> +	rm .git/objects/pack/$packbitmap.keep

Arrange this removal to happen even when any earlier step fails, so that later tests will not get affected by stray existence of this file, by using test_when_finished. E.g.

	list_packed_objects .git/objects/pack/$packbitmap.idx >packbitmap.objects &&
	test_when_finished "rm -f .git/objects/pack/$packbitmap.keep" &&
	>.git/objects/pack/$packbitmap.keep" &&
Show 10 quoted lines
> +test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '
> +	mv .git/objects/pack/$packbitmap.* alt_objects/pack/ &&
> +	echo HEAD | git pack-objects --local --stdout --revs >3b.pack &&
> +	git index-pack 3b.pack &&
> +	packobjects 3b.idx >3b.objects &&
> +	if hasany packbitmap.objects 3b.objects; then
> +		echo "Non-local object from bitmapped pack present in pack generated with --local"
> +		return 1
> +	fi &&
> +	mv alt_objects/pack/$packbitmap.* .git/objects/pack/
Ditto on potential use of test_when_finished.
Previous: Kirill SmelkovNext: Kirill Smelkov
Message 25 of 62 in “pack-objects: Use reachability bitmap index when generating non-stdout pack too”
  1. pack-objects: Use reachability bitmap index when generating non-stdout pack tooKirill Smelkov, Jul 7, 2016
  2. Jeff KingJul 7, 2016
  3. Kirill SmelkovJul 8, 2016
  4. Kirill SmelkovJul 12, 2016
  5. Jeff KingJul 13, 2016
  6. Kirill SmelkovJul 13, 2016
  7. Kirill SmelkovJul 17, 2016
  8. Jeff KingJul 19, 2016
  9. Kirill SmelkovJul 19, 2016
  10. Jeff KingJul 25, 2016
  11. Jeff KingJul 25, 2016
  12. Kirill SmelkovJul 27, 2016
  13. Junio C HamanoJul 27, 2016
  14. Kirill SmelkovJul 28, 2016
  15. Junio C HamanoJul 28, 2016
  16. Kirill SmelkovJul 29, 2016
  17. 1/2 pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incrementalKirill Smelkov, Jul 29, 2016
  18. Junio C HamanoAug 1, 2016
  19. Kirill SmelkovAug 8, 2016
  20. Jeff KingAug 8, 2016
  21. Jeff KingAug 8, 2016
  22. Junio C HamanoAug 8, 2016
  23. Kirill SmelkovAug 8, 2016
  24. pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incrementalKirill Smelkov, Aug 8, 2016
  25. Junio C HamanoAug 8, 2016
  26. Kirill SmelkovAug 9, 2016
  27. 1/2 pack-objects: respect --local/--honor-pack-keep/--incremental when bitmap is in useKirill Smelkov, Aug 9, 2016
  28. Junio C HamanoAug 9, 2016
  29. Kirill SmelkovAug 9, 2016
  30. 1/2 pack-objects: respect --local/--honor-pack-keep/--incremental when bitmap is in useKirill Smelkov, Aug 9, 2016
  31. Jeff KingAug 18, 2016
  32. Kirill SmelkovSep 10, 2016
  33. 1/2 pack-objects: respect --local/--honor-pack-keep/--incremental when bitmap is in useKirill Smelkov, Sep 10, 2016
  34. Junio C HamanoSep 13, 2016
  35. Kirill SmelkovSep 13, 2016
  36. t/perf/run: Don't forget to copy config.mak.autogen & friends to build areaKirill Smelkov, Sep 10, 2016
  37. Junio C HamanoSep 12, 2016
  38. Junio C HamanoSep 12, 2016
  39. Junio C HamanoSep 12, 2016
  40. Kirill SmelkovSep 13, 2016
  41. Junio C HamanoSep 12, 2016
  42. 2/2 pack-objects: use reachability bitmap index when generating non-stdout packKirill Smelkov, Aug 9, 2016
  43. Jeff KingAug 18, 2016
  44. Kirill SmelkovSep 10, 2016
  45. 2/2 pack-objects: use reachability bitmap index when generating non-stdout packKirill Smelkov, Sep 10, 2016
  46. Junio C HamanoSep 12, 2016
  47. Junio C HamanoAug 9, 2016
  48. Junio C HamanoAug 8, 2016
  49. Jeff KingAug 8, 2016
  50. Junio C HamanoAug 8, 2016
  51. 2/2 pack-objects: Teach it to use reachability bitmap index when generating non-stdout pack tooKirill Smelkov, Jul 29, 2016
  52. Jeff KingAug 8, 2016
  53. Kirill SmelkovAug 8, 2016
  54. Junio C HamanoAug 8, 2016
  55. Kirill SmelkovAug 8, 2016
  56. Junio C HamanoAug 8, 2016
  57. Kirill SmelkovAug 8, 2016
  58. pack-objects: teach it to use reachability bitmap index when generating non-stdout pack tooKirill Smelkov, Aug 8, 2016
  59. Junio C HamanoAug 8, 2016
  60. Kirill SmelkovAug 9, 2016
  61. 2/2 pack-objects: use reachability bitmap index when generating non-stdout packKirill Smelkov, Aug 9, 2016
  62. Jeff KingJul 13, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.