Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pack-objects: Create an alternative name hash algorithm (recreated) #1823

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Documentation/git-pack-objects.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ SYNOPSIS
[--revs [--unpacked | --all]] [--keep-pack=<pack-name>]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Taylor Blau wrote (reply to this):

On Tue, Nov 05, 2024 at 03:05:01AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[email protected]>
>
> The pack_name_hash() method has not been materially changed since it was
> introduced in ce0bd64299a (pack-objects: improve path grouping
> heuristics., 2006-06-05). The intention here is to group objects by path
> name, but also attempt to group similar file types together by making
> the most-significant digits of the hash be focused on the final
> characters.
>
> Here's the crux of the implementation:
>
> 	/*
> 	 * This effectively just creates a sortable number from the
> 	 * last sixteen non-whitespace characters. Last characters
> 	 * count "most", so things that end in ".c" sort together.
> 	 */
> 	while ((c = *name++) != 0) {
> 		if (isspace(c))
> 			continue;
> 		hash = (hash >> 2) + (c << 24);
> 	}

Hah. I like that the existing implementation is small enough to fit (in
its entirety!) into the commit message!

> As the comment mentions, this only cares about the last sixteen
> non-whitespace characters. This cause some filenames to collide more
> than others. Here are some examples that I've seen while investigating
> repositories that are growing more than they should be:
>
>  * "/CHANGELOG.json" is 15 characters, and is created by the beachball
>    [1] tool. Only the final character of the parent directory can
>    differntiate different versions of this file, but also only the two

s/differntiate/differentiate ;-).

>    most-significant digits. If that character is a letter, then this is
>    always a collision. Similar issues occur with the similar
>    "/CHANGELOG.md" path, though there is more opportunity for
>    differences in the parent directory.
>
>  * Localization files frequently have common filenames but differentiate
>    via parent directories. In C#, the name "/strings.resx.lcl" is used
>    for these localization files and they will all collide in name-hash.
>
> [1] https://github.com/microsoft/beachball
>
> I've come across many other examples where some internal tool uses a
> common name across multiple directories and is causing Git to repack
> poorly due to name-hash collisions.
>
> It is clear that the existing name-hash algorithm is optimized for
> repositories with short path names, but also is optimized for packing a
> single snapshot of a repository, not a repository with many versions of
> the same file. In my testing, this has proven out where the name-hash
> algorithm does a good job of finding peer files as delta bases when
> unable to use a historical version of that exact file.

I'm not sure I entirely agree with the suggestion that the existing hash
function is only about packing repositories with short pathnames. I
think an important part of the existing implementation is that tries to
group similar files together, regardless of whether or not they appear
in the same tree.

As you have shown, this can be a problem when the fact two files that
happen to end in "CHANGELOG.json" end up in vastly different trees and
*aren't* related. I don't think that nailing all of these details in the
commit message is necessary, but I do think it's worth adjusting what
the original commit message says in terms of what the existing algorithm
is optimized for.

> However, for repositories that have many versions of most files and
> directories, it is more important that the objects that appear at the
> same path are grouped together.
>
> Create a new pack_full_name_hash() method and a new --full-name-hash
> option for 'git pack-objects' to call that method instead. Add a simple
> pass-through for 'git repack --full-name-hash' for additional testing in
> the context of a full repack, where I expect this will be most
> effective.
>
> The hash algorithm is as simple as possible to be reasonably effective:
> for each character of the path string, add a multiple of that character
> and a large prime number (chosen arbitrarily, but intended to be large
> relative to the size of a uint32_t). Then, shift the current hash value
> to the right by 5, with overlap. The addition and shift parameters are
> standard mechanisms for creating hard-to-predict behaviors in the bits
> of the resulting hash.
>
> This is not meant to be cryptographic at all, but uniformly distributed
> across the possible hash values. This creates a hash that appears
> pseudorandom. There is no ability to consider similar file types as
> being close to each other.

I think you hint at this in the series' cover letter, but I suspect that
this pseduorandom behavior hurts in some small number of cases and that
the full-name hash idea isn't a pure win, e.g., when we really do want
to delta two paths that both end in CHAGNELOG.json despite being in
different parts of the tree.

You have some tables here below that demonstrate a significant
improvement with the full-name hash in use, which I think is good worth
keeping in my own opinion. It may be worth updating those to include the
new examples you highlighted in your revised cover letter as well.

> In a later change, a test-tool will be added so the effectiveness of
> this hash can be demonstrated directly.
>
> For now, let's consider how effective this mechanism is when repacking a
> repository with and without the --full-name-hash option. Specifically,

Is this repository publicly available? If so, it may be worth mentioning
here.

> let's use 'git repack -adf [--full-name-hash]' as our test.
>
> On the Git repository, we do not expect much difference. All path names
> are short. This is backed by our results:
>
> | Stage                 | Pack Size | Repack Time |
> |-----------------------|-----------|-------------|
> | After clone           | 260 MB    | N/A         |
> | Standard Repack       | 127MB     | 106s        |
> | With --full-name-hash | 126 MB    | 99s         |

Ahh. Here's a great example of it helping to a smaller extent. Thanks
for including this as part of demonstrating the full picture (both the
benefits and drawbacks).

> This example demonstrates how there is some natural overhead coming from
> the cloned copy because the server is hosting many forks and has not
> optimized for exactly this set of reachable objects. But the full repack
> has similar characteristics with and without --full-name-hash.

Good.

> However, we can test this in a repository that uses one of the
> problematic naming conventions above. The fluentui [2] repo uses
> beachball to generate CHANGELOG.json and CHANGELOG.md files, and these
> files have very poor delta characteristics when comparing against
> versions across parent directories.
>
> | Stage                 | Pack Size | Repack Time |
> |-----------------------|-----------|-------------|
> | After clone           | 694 MB    | N/A         |
> | Standard Repack       | 438 MB    | 728s        |
> | With --full-name-hash | 168 MB    | 142s        |
>
> [2] https://github.com/microsoft/fluentui
>
> In this example, we see significant gains in the compressed packfile
> size as well as the time taken to compute the packfile.

Amazing!

> Using a collection of repositories that use the beachball tool, I was
> able to make similar comparisions with dramatic results. While the
> fluentui repo is public, the others are private so cannot be shared for
> reproduction. The results are so significant that I find it important to
> share here:
>
> | Repo     | Standard Repack | With --full-name-hash |
> |----------|-----------------|-----------------------|
> | fluentui |         438 MB  |               168 MB  |
> | Repo B   |       6,255 MB  |               829 MB  |
> | Repo C   |      37,737 MB  |             7,125 MB  |
> | Repo D   |     130,049 MB  |             6,190 MB  |
>
> Future changes could include making --full-name-hash implied by a config
> value or even implied by default during a full repack.
>
> It is important to point out that the name hash value is stored in the
> .bitmap file format, so we must disable the --full-name-hash option when
> bitmaps are being read or written. Later, the bitmap format could be
> updated to be aware of the name hash version so deltas can be quickly
> computed across the bitmapped/not-bitmapped boundary.

Agreed.

> Signed-off-by: Derrick Stolee <[email protected]>
> ---
>  Documentation/git-pack-objects.txt |  3 ++-
>  builtin/pack-objects.c             | 25 ++++++++++++++++++++-----
>  builtin/repack.c                   |  5 +++++
>  pack-objects.h                     | 21 +++++++++++++++++++++
>  t/t5300-pack-object.sh             | 15 +++++++++++++++
>  5 files changed, 63 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
> index e32404c6aae..93861d9f85b 100644
> --- a/Documentation/git-pack-objects.txt
> +++ b/Documentation/git-pack-objects.txt
> @@ -15,7 +15,8 @@ SYNOPSIS
>  	[--revs [--unpacked | --all]] [--keep-pack=<pack-name>]
>  	[--cruft] [--cruft-expiration=<time>]
>  	[--stdout [--filter=<filter-spec>] | <base-name>]
> -	[--shallow] [--keep-true-parents] [--[no-]sparse] < <object-list>
> +	[--shallow] [--keep-true-parents] [--[no-]sparse]
> +	[--full-name-hash] < <object-list>

OK, I see that --full-name-hash is now listed in the synopsis, but I
don't see a corresponding description of what the option does later on
in this file. I took a look through the remaining patches in this series
and couldn't find any further changes to git-pack-objects(1) either.

>  DESCRIPTION
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 08007142671..85595dfcd88 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -266,6 +266,14 @@ struct configured_exclusion {
>  static struct oidmap configured_exclusions;
>
>  static struct oidset excluded_by_config;
> +static int use_full_name_hash;
> +
> +static inline uint32_t pack_name_hash_fn(const char *name)
> +{
> +	if (use_full_name_hash)
> +		return pack_full_name_hash(name);
> +	return pack_name_hash(name);
> +}
>
>  /*
>   * stats
> @@ -1698,7 +1706,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
>  		return 0;
>  	}
>
> -	create_object_entry(oid, type, pack_name_hash(name),
> +	create_object_entry(oid, type, pack_name_hash_fn(name),
>  			    exclude, name && no_try_delta(name),
>  			    found_pack, found_offset);
>  	return 1;
> @@ -1912,7 +1920,7 @@ static void add_preferred_base_object(const char *name)
>  {
>  	struct pbase_tree *it;
>  	size_t cmplen;
> -	unsigned hash = pack_name_hash(name);
> +	unsigned hash = pack_name_hash_fn(name);
>
>  	if (!num_preferred_base || check_pbase_path(hash))
>  		return;
> @@ -3422,7 +3430,7 @@ static void show_object_pack_hint(struct object *object, const char *name,
>  	 * here using a now in order to perhaps improve the delta selection
>  	 * process.
>  	 */
> -	oe->hash = pack_name_hash(name);
> +	oe->hash = pack_name_hash_fn(name);
>  	oe->no_try_delta = name && no_try_delta(name);
>
>  	stdin_packs_hints_nr++;
> @@ -3572,7 +3580,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type
>  	entry = packlist_find(&to_pack, oid);
>  	if (entry) {
>  		if (name) {
> -			entry->hash = pack_name_hash(name);
> +			entry->hash = pack_name_hash_fn(name);
>  			entry->no_try_delta = no_try_delta(name);
>  		}
>  	} else {
> @@ -3595,7 +3603,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type
>  			return;
>  		}
>
> -		entry = create_object_entry(oid, type, pack_name_hash(name),
> +		entry = create_object_entry(oid, type, pack_name_hash_fn(name),
>  					    0, name && no_try_delta(name),
>  					    pack, offset);
>  	}
> @@ -4429,6 +4437,8 @@ int cmd_pack_objects(int argc,
>  		OPT_STRING_LIST(0, "uri-protocol", &uri_protocols,
>  				N_("protocol"),
>  				N_("exclude any configured uploadpack.blobpackfileuri with this protocol")),
> +		OPT_BOOL(0, "full-name-hash", &use_full_name_hash,
> +			 N_("optimize delta compression across identical path names over time")),
>  		OPT_END(),
>  	};
>
> @@ -4576,6 +4586,11 @@ int cmd_pack_objects(int argc,
>  	if (pack_to_stdout || !rev_list_all)
>  		write_bitmap_index = 0;
>
> +	if (write_bitmap_index && use_full_name_hash) {
> +		warning(_("currently, the --full-name-hash option is incompatible with --write-bitmap-index"));
> +		use_full_name_hash = 0;
> +	}
> +

Good, we determine this early on in the command, so we don't risk
computing different hash functions within the same process.

I wonder if it's worth guarding against mixing the hash functions within
the pack_name_hash() and pack_full_name_hash() functions themselves. I'm
thinking something like:

    static inline uint32_t pack_name_hash(const char *name)
    {
        if (use_full_name_hash)
            BUG("called pack_name_hash() with --full-name-hash")
        /* ... */
    }

and the inverse in pack_full_name_hash(). I don't think it's strictly
necessary, but it would be a nice guard against someone calling, e.g.,
pack_full_name_hash() directly instead of pack_name_hash_fn().

The other small thought I had here is that we should use the convenience
function die_for_incompatible_opt3() here, since it uses an existing
translation string for pairs of incompatible options.

(As an aside, though that function is actually implemented in the
_opt4() variant, and it knows how to handle a pair, trio, and quartet of
mutually incompatible options, there is no die_for_incompatible_opt2()
function. It may be worth adding one here since I'm sure there are other
spots which would benefit from such a function).

> diff --git a/builtin/repack.c b/builtin/repack.c
> index d6bb37e84ae..ab2a2e46b20 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c

I'm surprised to see the new option plumbed into repack in this commit.
I would have thought that it'd appear in the subsequent commit instead.
The implementation below looks good, I just imagined it would be placed
in the next commit instead of this one.

The remaining parts of this change look good to me.

Thanks,
Taylor

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Taylor Blau wrote (reply to this):

On Thu, Nov 21, 2024 at 03:08:09PM -0500, Taylor Blau wrote:
> The remaining parts of this change look good to me.

Oops, one thing I forgot (which reading Peff's message in [1] reminded
me of) is that I think we need to disable full-name hashing when we're
reusing existing packfiles as is the case with try_partial_reuse().

There we're always looking at classic name hash values, so mixing the
two would be a mistake. I think that amounts to:

--- 8< ---
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 762949e4c8..7e370bcfc9 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4070,6 +4070,8 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
 	if (!(bitmap_git = prepare_bitmap_walk(revs, 0)))
 		return -1;

+	use_full_name_hash = 0;
+
 	if (pack_options_allow_reuse())
 		reuse_partial_packfile_from_bitmap(bitmap_git,
 						   &reuse_packfiles,
--- >8 ---

Thanks,
Taylor

[1]: https://lore.kernel.org/git/[email protected]/

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Taylor Blau <[email protected]> writes:

> On Thu, Nov 21, 2024 at 03:08:09PM -0500, Taylor Blau wrote:
>> The remaining parts of this change look good to me.
>
> Oops, one thing I forgot (which reading Peff's message in [1] reminded
> me of) is that I think we need to disable full-name hashing when we're
> reusing existing packfiles as is the case with try_partial_reuse().
>
> There we're always looking at classic name hash values, so mixing the
> two would be a mistake. I think that amounts to:
>
> --- 8< ---
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 762949e4c8..7e370bcfc9 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -4070,6 +4070,8 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
>  	if (!(bitmap_git = prepare_bitmap_walk(revs, 0)))
>  		return -1;
>
> +	use_full_name_hash = 0;

Hmph, is this early enough, or has some other code path already
computed the name hashes for the paths for the files to be packed?

    ... Goes and looks ...

This is called from get_object_list() which

 - is not called under --stdin-packs,
 - is not called in cruft mode,
 - is not called when reading object list from --stdin

so we are looking at the bog-standard "objects to be packed are
given in the form of rev-list command line options from our command
line".  And in the function, we walk the history near the end, which
makes show_object calls that adds object-entry with the name-hash.
So the call to get_object_list_from_bitmap() happens way before the
first use of the name-hash function, so this is probably safe.

And obviously get_object_list_from_bitmap() is the only place we
select objects to be packed from an existing pack and a bitmap file,
so even if we gain new callers in the future, it is very likely that
the new callers would benefit from this change.

OK.  Nicely done.

>  	if (pack_options_allow_reuse())
>  		reuse_partial_packfile_from_bitmap(bitmap_git,
>  						   &reuse_packfiles,
> --- >8 ---
>
> Thanks,
> Taylor
>
> [1]: https://lore.kernel.org/git/[email protected]/

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/21/24 4:35 PM, Taylor Blau wrote:
> On Thu, Nov 21, 2024 at 03:08:09PM -0500, Taylor Blau wrote:
>> The remaining parts of this change look good to me.
> > Oops, one thing I forgot (which reading Peff's message in [1] reminded
> me of) is that I think we need to disable full-name hashing when we're
> reusing existing packfiles as is the case with try_partial_reuse().
> > There we're always looking at classic name hash values, so mixing the
> two would be a mistake. I think that amounts to:
> > --- 8< ---
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 762949e4c8..7e370bcfc9 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -4070,6 +4070,8 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
>   	if (!(bitmap_git = prepare_bitmap_walk(revs, 0)))
>   		return -1;
> > +	use_full_name_hash = 0;
> +
Thanks. I have applied this code change with a comment detailing
the context around the bitmap file storing only the default name-hash
(for now) but that it can change in the future.

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/21/24 3:08 PM, Taylor Blau wrote:
> On Tue, Nov 05, 2024 at 03:05:01AM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <[email protected]>

>> It is clear that the existing name-hash algorithm is optimized for
>> repositories with short path names, but also is optimized for packing a
>> single snapshot of a repository, not a repository with many versions of
>> the same file. In my testing, this has proven out where the name-hash
>> algorithm does a good job of finding peer files as delta bases when
>> unable to use a historical version of that exact file.
> > I'm not sure I entirely agree with the suggestion that the existing hash
> function is only about packing repositories with short pathnames. I
> think an important part of the existing implementation is that tries to
> group similar files together, regardless of whether or not they appear
> in the same tree.

I'll be more explicit about the design for "hash locality" earlier in
the message, but also pointing out that the locality only makes sense as
a benefit when there are not enough versions of a file in history, since
it's nearly always better to choose a previous version of the same file
instead of a different path with a name-hash collision. Directory renames
are on place where this is a positive decision, but those are typically
rare compared to the full history of a large repo.

>> This is not meant to be cryptographic at all, but uniformly distributed
>> across the possible hash values. This creates a hash that appears
>> pseudorandom. There is no ability to consider similar file types as
>> being close to each other.
> > I think you hint at this in the series' cover letter, but I suspect that
> this pseduorandom behavior hurts in some small number of cases and that
> the full-name hash idea isn't a pure win, e.g., when we really do want
> to delta two paths that both end in CHAGNELOG.json despite being in
> different parts of the tree.

I mention that this doesn't work well in all cases when operating under
a 'git push' or in a shallow clone. Shallow clones are disabled in a later
commit and we don't have the necessary implementation to make this hash
function be selected within 'git push'.

> You have some tables here below that demonstrate a significant
> improvement with the full-name hash in use, which I think is good worth
> keeping in my own opinion. It may be worth updating those to include the
> new examples you highlighted in your revised cover letter as well.

I'll try to remember to move the newer examples to the cover letter.

>> In a later change, a test-tool will be added so the effectiveness of
>> this hash can be demonstrated directly.
>>
>> For now, let's consider how effective this mechanism is when repacking a
>> repository with and without the --full-name-hash option. Specifically,
> > Is this repository publicly available? If so, it may be worth mentioning
> here.

Here, by "when repacking a repository" I mean "we are going to test
repacking a number of example repositories, that will be listed in detail
in the coming tables".

>> Using a collection of repositories that use the beachball tool, I was
>> able to make similar comparisions with dramatic results. While the
>> fluentui repo is public, the others are private so cannot be shared for
>> reproduction. The results are so significant that I find it important to
>> share here:
>>
>> | Repo     | Standard Repack | With --full-name-hash |
>> |----------|-----------------|-----------------------|
>> | fluentui |         438 MB  |               168 MB  |
>> | Repo B   |       6,255 MB  |               829 MB  |
>> | Repo C   |      37,737 MB  |             7,125 MB  |
>> | Repo D   |     130,049 MB  |             6,190 MB  |

These repos B, C, and D are _not_ publicly available, though.

>> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
>> index e32404c6aae..93861d9f85b 100644
>> --- a/Documentation/git-pack-objects.txt
>> +++ b/Documentation/git-pack-objects.txt
>> @@ -15,7 +15,8 @@ SYNOPSIS
>>   	[--revs [--unpacked | --all]] [--keep-pack=<pack-name>]
>>   	[--cruft] [--cruft-expiration=<time>]
>>   	[--stdout [--filter=<filter-spec>] | <base-name>]
>> -	[--shallow] [--keep-true-parents] [--[no-]sparse] < <object-list>
>> +	[--shallow] [--keep-true-parents] [--[no-]sparse]
>> +	[--full-name-hash] < <object-list>
> > OK, I see that --full-name-hash is now listed in the synopsis, but I
> don't see a corresponding description of what the option does later on
> in this file. I took a look through the remaining patches in this series
> and couldn't find any further changes to git-pack-objects(1) either.

I'll fix that. Thanks. As well as moving the 'git repack' changes out
of this patch. I'll adjust the commit message to say "packing all objects'
instead of 'git repack' to be clear that this can be done with a direct
call to 'git pack-objects' instead of needing 'git repack'.

>> +	if (write_bitmap_index && use_full_name_hash) {
>> +		warning(_("currently, the --full-name-hash option is incompatible with --write-bitmap-index"));
>> +		use_full_name_hash = 0;
>> +	}
>> +
> > Good, we determine this early on in the command, so we don't risk
> computing different hash functions within the same process.
> > I wonder if it's worth guarding against mixing the hash functions within
> the pack_name_hash() and pack_full_name_hash() functions themselves. I'm
> thinking something like:
> >      static inline uint32_t pack_name_hash(const char *name)
>      {
>          if (use_full_name_hash)
>              BUG("called pack_name_hash() with --full-name-hash")
>          /* ... */
>      }
> > and the inverse in pack_full_name_hash(). I don't think it's strictly
> necessary, but it would be a nice guard against someone calling, e.g.,
> pack_full_name_hash() directly instead of pack_name_hash_fn().

I think this is interesting defensive programming for future contributions.

We essentially want the methods to only be called by pack_name_hash_fn()
and don't have method privacy. We could extract it to its own header file
but then would need to modify the prototype to include the signal for
which hash type to use, but that would cause us to lose our ability to
check for a bug like this.

It may be even better to store a static value for the value of
use_full_name_hash when it first executes, so it can exit if it notices
a different value. (This is becoming large enough for its own patch.)

> The other small thought I had here is that we should use the convenience
> function die_for_incompatible_opt3() here, since it uses an existing
> translation string for pairs of incompatible options.
> > (As an aside, though that function is actually implemented in the
> _opt4() variant, and it knows how to handle a pair, trio, and quartet of
> mutually incompatible options, there is no die_for_incompatible_opt2()
> function. It may be worth adding one here since I'm sure there are other
> spots which would benefit from such a function).

Interesting. I've not considered these functions before.

>> diff --git a/builtin/repack.c b/builtin/repack.c
>> index d6bb37e84ae..ab2a2e46b20 100644
>> --- a/builtin/repack.c
>> +++ b/builtin/repack.c
> > I'm surprised to see the new option plumbed into repack in this commit.
> I would have thought that it'd appear in the subsequent commit instead.
> The implementation below looks good, I just imagined it would be placed
> in the next commit instead of this one.

Yes, I should delay that to patch 2.

Thanks,
-Stole

[--cruft] [--cruft-expiration=<time>]
[--stdout [--filter=<filter-spec>] | <base-name>]
[--shallow] [--keep-true-parents] [--[no-]sparse] < <object-list>
[--shallow] [--keep-true-parents] [--[no-]sparse]
[--full-name-hash] < <object-list>


DESCRIPTION
Expand Down
13 changes: 12 additions & 1 deletion Documentation/git-repack.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ git-repack - Pack unpacked objects in a repository
SYNOPSIS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Taylor Blau wrote (reply to this):

On Tue, Nov 05, 2024 at 03:05:04AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[email protected]>
>
> This also adds the '--full-name-hash' option introduced in the previous
> change and adds newlines to the synopsis.

I think "the previous change" is not quite accurate here, even if
you move the implementation to pass through '--full-name-hash' via
repack into the second patch.

It would be nice to have the option added in 'repack' in the same commit
as adjusts the documentation instead of splitting them apart.

Thanks,
Taylor

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/21/24 3:17 PM, Taylor Blau wrote:
> On Tue, Nov 05, 2024 at 03:05:04AM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <[email protected]>
>>
>> This also adds the '--full-name-hash' option introduced in the previous
>> change and adds newlines to the synopsis.
> > I think "the previous change" is not quite accurate here, even if
> you move the implementation to pass through '--full-name-hash' via
> repack into the second patch.

Ah, I should definitely rearrange the commits.

> It would be nice to have the option added in 'repack' in the same commit
> as adjusts the documentation instead of splitting them apart.
Part of the point of the split was that the synopsis in builtin/repack.c
needs more than just the addition of the --full-name-hash option in order
to make it match the Documentation synopsis.

But you're right, the code change is small enough that these things can
be combined.

Thanks,
-Stolee

--------
[verse]
'git repack' [-a] [-A] [-d] [-f] [-F] [-l] [-n] [-q] [-b] [-m] [--window=<n>] [--depth=<n>] [--threads=<n>] [--keep-pack=<pack-name>] [--write-midx]
'git repack' [-a] [-A] [-d] [-f] [-F] [-l] [-n] [-q] [-b] [-m]
[--window=<n>] [--depth=<n>] [--threads=<n>] [--keep-pack=<pack-name>]
[--write-midx] [--full-name-hash]

DESCRIPTION
-----------
Expand Down Expand Up @@ -249,6 +251,15 @@ linkgit:git-multi-pack-index[1]).
Write a multi-pack index (see linkgit:git-multi-pack-index[1])
containing the non-redundant packs.

--full-name-hash::
By default, Git uses a locality-preserving hash function to group
similar files together to find opportunities to find delta pairs.
In some repositories this can cause unintended collisions and
cause poor delta selection. This option changes the hash function
to be more uniformly pseudorandom based on the full name of a file.
In some repositories, this leads to a significant reduction in
packfile size.

CONFIGURATION
-------------

Expand Down
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,7 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o
TEST_BUILTINS_OBJS += test-match-trees.o
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Taylor Blau wrote (reply to this):

On Tue, Nov 05, 2024 at 03:05:07AM +0000, Derrick Stolee via GitGitGadget wrote:
> Test                                              this tree
> -----------------------------------------------------------------
> 5314.1: paths at head                                        4.5K
> 5314.2: number of distinct name-hashes                       4.1K
> 5314.3: number of distinct full-name-hashes                  4.5K
> 5314.4: maximum multiplicity of name-hashes                    13
> 5314.5: maximum multiplicity of fullname-hashes                 1
>
> Here, the maximum collision multiplicity is 13, but around 10% of paths
> have a collision with another path.

Neat.

> diff --git a/t/helper/test-name-hash.c b/t/helper/test-name-hash.c
> new file mode 100644
> index 00000000000..e4ecd159b76
> --- /dev/null
> +++ b/t/helper/test-name-hash.c
> @@ -0,0 +1,24 @@
> +/*
> + * test-name-hash.c: Read a list of paths over stdin and report on their
> + * name-hash and full name-hash.
> + */
> +
> +#include "test-tool.h"
> +#include "git-compat-util.h"
> +#include "pack-objects.h"
> +#include "strbuf.h"
> +
> +int cmd__name_hash(int argc UNUSED, const char **argv UNUSED)
> +{
> +	struct strbuf line = STRBUF_INIT;
> +
> +	while (!strbuf_getline(&line, stdin)) {
> +		uint32_t name_hash = pack_name_hash(line.buf);
> +		uint32_t full_hash = pack_full_name_hash(line.buf);
> +
> +		printf("%10"PRIu32"\t%10"PRIu32"\t%s\n", name_hash, full_hash, line.buf);

I'm definitely nitpicking, but having a tab to separate these two 32-bit
values feels odd when we know already that they will be at most
10-characters wide.

I probably would have written:

    printf("%10"PRIu32" %10"PRIu32"\t%s\n", name_hash, full_hash, line.buf);

instead, but this is obviously not a big deal either way ;-).

> diff --git a/t/perf/p5314-name-hash.sh b/t/perf/p5314-name-hash.sh
> new file mode 100755
> index 00000000000..9fe26612fac
> --- /dev/null
> +++ b/t/perf/p5314-name-hash.sh
> @@ -0,0 +1,41 @@
> +#!/bin/sh
> +
> +test_description='Tests pack performance using bitmaps'
> +. ./perf-lib.sh
> +
> +GIT_TEST_PASSING_SANITIZE_LEAK=0
> +export GIT_TEST_PASSING_SANITIZE_LEAK

Does this conflict with Patrick's series to remove these leak checking
annotations? I think it might, which is not unexpected given this series
was written before that one (and it's my fault for not reviewing it
earlier).

> +test_perf_large_repo
> +
> +test_size 'paths at head' '
> +	git ls-tree -r --name-only HEAD >path-list &&
> +	wc -l <path-list
> +'
> +
> +test_size 'number of distinct name-hashes' '
> +	cat path-list | test-tool name-hash >name-hashes &&
> +	cat name-hashes | awk "{ print \$1; }" | sort -n | uniq -c >name-hash-count &&

In these two (and a handful of others lower down in this same script)
the "cat ... |" is unnecessary. I think this one should be written as:

    test-tool name-hash <path-list >name-hashes &&
    awk "{ print \$1; }" <name-hashes | sort | uniq -c >name-hash-count &&

(sort -n is unnecessary, since we just care about getting the list in
sorted order so that "uniq -c" can count the number of unique values).

> +	wc -l <name-hash-count
> +'
> +
> +test_size 'number of distinct full-name-hashes' '
> +	cat name-hashes | awk "{ print \$2; }" | sort -n | uniq -c >full-name-hash-count &&
> +	wc -l <full-name-hash-count
> +'
> +
> +test_size 'maximum multiplicity of name-hashes' '
> +	cat name-hash-count | \
> +		sort -nr | \
> +		head -n 1 | \
> +		awk "{ print \$1; }"
> +'
> +
> +test_size 'maximum multiplicity of fullname-hashes' '
> +	cat full-name-hash-count | \
> +		sort -nr | \
> +		head -n 1 | \
> +		awk "{ print \$1; }"

Nitpicking again, but you could extract the "sort | head | awk" pipeline
into a function.

Thanks,
Taylor

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jonathan Tan wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes:
> From: Derrick Stolee <[email protected]>
> 
> Add a new test-tool helper, name-hash, to output the value of the
> name-hash algorithms for the input list of strings, one per line.

I've looked at all 7 patches.

I didn't really understand the concern with shallow in patch 6 (in
particular, the documentation of "git pack-objects --shallow" seems
to imply that it's for use by a server to a shallow client, but at
the point that the server would need such a feature, it probably would
already have bitmaps packed with the new hash algorithm). I didn't look
at it further, though, since I had an algorithm that seemed to also do
OK in the shallow test. So we might be able to drop patch 6.

Other than that, and other than all my comments and Taylor's comments,
this series looks good.
 

TEST_BUILTINS_OBJS += test-mergesort.o
TEST_BUILTINS_OBJS += test-mktemp.o
TEST_BUILTINS_OBJS += test-name-hash.o
TEST_BUILTINS_OBJS += test-online-cpus.o
TEST_BUILTINS_OBJS += test-pack-mtimes.o
TEST_BUILTINS_OBJS += test-parse-options.o
Expand Down
43 changes: 38 additions & 5 deletions builtin/pack-objects.c
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,14 @@ struct configured_exclusion {
static struct oidmap configured_exclusions;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Taylor Blau wrote (reply to this):

On Tue, Nov 05, 2024 at 03:05:03AM +0000, Derrick Stolee via GitGitGadget wrote:
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index 2e28d02b20f..75b40f07bbd 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -30,6 +30,7 @@ linux-TEST-vars)
>  	export GIT_TEST_NO_WRITE_REV_INDEX=1
>  	export GIT_TEST_CHECKOUT_WORKERS=2
>  	export GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1
> +	export GIT_TEST_FULL_NAME_HASH=1
>  	;;
>  linux-clang)
>  	export GIT_TEST_DEFAULT_HASH=sha1

Hmm. I appreciate what this new GIT_TEST_ variable is trying to do, but
I am somewhat saddened to see this list in linux-TEST-vars growing
rather than shrinking.

I'm most definitely part of the problem here, but I think too often we
add new entries to this list and let them languish without ever removing
them after they have served their intended purpose.

So I think the question is: what do we hope to get out of running the
test suite in a mode where we use the full-name hash all of the time? I
can't imagine any interesting breakage (other than individual tests'
sensitivity to specific delta/base pairs) that would be caught by merely
changing the hash function here.

I dunno. Maybe there is some exotic behavior that this shook out for you
during development which I'm not aware of. If that were the case, I
think that keeping this variable around makes sense, since the appearance
of that exotic behavior proves that the variable is useful at shaking
out bugs.

But assuming not, I think that I would just as soon avoid this test
variable entirely, which I think in this case amounts to dropping this
patch from the series.

Thanks,
Taylor

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/21/24 3:15 PM, Taylor Blau wrote:
> On Tue, Nov 05, 2024 at 03:05:03AM +0000, Derrick Stolee via GitGitGadget wrote:
>> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
>> index 2e28d02b20f..75b40f07bbd 100755
>> --- a/ci/run-build-and-tests.sh
>> +++ b/ci/run-build-and-tests.sh
>> @@ -30,6 +30,7 @@ linux-TEST-vars)
>>   	export GIT_TEST_NO_WRITE_REV_INDEX=1
>>   	export GIT_TEST_CHECKOUT_WORKERS=2
>>   	export GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1
>> +	export GIT_TEST_FULL_NAME_HASH=1
>>   	;;
>>   linux-clang)
>>   	export GIT_TEST_DEFAULT_HASH=sha1
> > Hmm. I appreciate what this new GIT_TEST_ variable is trying to do, but
> I am somewhat saddened to see this list in linux-TEST-vars growing
> rather than shrinking.
You make good points that this does not need to be here.

It's enough that someone could manually check the test suite
with this test variable to make sure that enough of the other
options are tested with this feature.

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jonathan Tan wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes:
> Second, there are two tests in t5616-partial-clone.sh that I believe are
> actually broken scenarios. 

I took a look...this is a tricky one.

> While the client is set up to clone the
> 'promisor-server' repo via a treeless partial clone filter (tree:0),
> that filter does not translate to the 'server' repo. Thus, fetching from
> these repos causes the server to think that the client has all reachable
> trees and blobs from the commits advertised as 'haves'. This leads the
> server to providing a thin pack assuming those objects as delta bases.

It is expected that the server sometimes sends deltas based on objects
that the client doesn't have. In fact, this test tests the ability of
Git to lazy-fetch delta bases.

> Changing the name-hash algorithm presents new delta bases and thus
> breaks the expectations of these tests.

To be precise, the change resulted in no deltas being sent (before this
change, one delta was sent). Here's what is meant to happen. The server has:

 commitB - treeB - file1 ("make the tree big\nanother line\n"), file2...file100
  |
 commitA - treeA - file1...file100 ("make the tree big\n")

The client only has commitA. (The client does not have treeA or any
blob, since it was cloned with --filter=tree:0.)

When GIT_TEST_FULL_NAME_HASH=0 (matching the current behavior), the
server sends a non-delta commitB, a delta treeB (with base treeA), and
a non-delta blob "make the tree big\nanother line\n". This triggers a
lazy fetch of treeA, and thus treeB is inflated successfully. During
the subsequent connectivity check (with --exclude-promisor-objects,
see connected.c), it is noticed that the "make the tree big\n" blob is
missing, but since it is a promisor object (referenced by treeA, which
was fetched from the promisor remote), the connectivity check since
passes.

When GIT_TEST_FULL_NAME_HASH=1, the server sends a non-delta commitB,
a non-delta treeB, and a non-delta blob "make the tree big\nanother
line\n". No lazy fetch is triggered. During the subsequent connectivity
check, the "make the tree big\n" blob (referenced by treeB) is missing.
There is nothing that can vouch for it (the client does not have treeA,
remember) so the client does not consider it a promisor object, and thus
the connectivity check fails.

Investigating this was made a bit harder due to a missing "git -C
promisor-remote config --local uploadpack.allowfilter 1" in the test.
The above behavior is after this is included in the test.

I think the solution is to have an algorithm that preserves the property
that treeB is sent as a delta object - if not, we need to find another
way to test the lazy-fetch of delta bases. My proposal in [1] does do
that.

[1] https://lore.kernel.org/git/[email protected]/

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jonathan Tan <[email protected]> writes:

> ... During the subsequent connectivity
> check, the "make the tree big\n" blob (referenced by treeB) is missing.
> There is nothing that can vouch for it (the client does not have treeA,
> remember) so the client does not consider it a promisor object, and thus
> the connectivity check fails.

It is sad that it is a (probably unfixable) flaw in the "promisor
object" concept that the "promisor object"-ness of blobA depends on
the lazy-fetch status of treeA.  This is not merely a test failure,
but it would cause blobA pruned if such a lazy fetch happens in the
wild and then "git gc" triggers, no?  It may not manifest as a
repository corruption, since we would lazily fetch it again if the
user requests to fully fetch what commitA and treeA need, but it
does feel somewhat suboptimal.

Thanks for a detailed explanation on what is going on.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jonathan Tan wrote (reply to this):

Junio C Hamano <[email protected]> writes:
> It is sad that it is a (probably unfixable) flaw in the "promisor
> object" concept that the "promisor object"-ness of blobA depends on
> the lazy-fetch status of treeA.  This is not merely a test failure,
> but it would cause blobA pruned if such a lazy fetch happens in the
> wild and then "git gc" triggers, no?  

Right now, it won't be pruned since we never prune promisor objects
(we just concatenate all of them into one file). But in the future, we
might only keep reachable promisor objects, in which case, yes, blobA
will be pruned. In this case, though, I think blobA is like any other
unreachable object in git. If a user memorizes a commit hash but does
not point a ref to it (or point a ref to one of its descendants), that
commit is still subject to being lost by GC. I think it's the same case
here.


static struct oidset excluded_by_config;
static int use_full_name_hash = -1;

static inline uint32_t pack_name_hash_fn(const char *name)
{
if (use_full_name_hash)
return pack_full_name_hash(name);
return pack_name_hash(name);
}

/*
* stats
Expand Down Expand Up @@ -1698,7 +1706,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
return 0;
}

create_object_entry(oid, type, pack_name_hash(name),
create_object_entry(oid, type, pack_name_hash_fn(name),
exclude, name && no_try_delta(name),
found_pack, found_offset);
return 1;
Expand Down Expand Up @@ -1912,7 +1920,7 @@ static void add_preferred_base_object(const char *name)
{
struct pbase_tree *it;
size_t cmplen;
unsigned hash = pack_name_hash(name);
unsigned hash = pack_name_hash_fn(name);

if (!num_preferred_base || check_pbase_path(hash))
return;
Expand Down Expand Up @@ -3422,7 +3430,7 @@ static void show_object_pack_hint(struct object *object, const char *name,
* here using a now in order to perhaps improve the delta selection
* process.
*/
oe->hash = pack_name_hash(name);
oe->hash = pack_name_hash_fn(name);
oe->no_try_delta = name && no_try_delta(name);

stdin_packs_hints_nr++;
Expand Down Expand Up @@ -3572,7 +3580,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type
entry = packlist_find(&to_pack, oid);
if (entry) {
if (name) {
entry->hash = pack_name_hash(name);
entry->hash = pack_name_hash_fn(name);
entry->no_try_delta = no_try_delta(name);
}
} else {
Expand All @@ -3595,7 +3603,7 @@ static void add_cruft_object_entry(const struct object_id *oid, enum object_type
return;
}

entry = create_object_entry(oid, type, pack_name_hash(name),
entry = create_object_entry(oid, type, pack_name_hash_fn(name),
0, name && no_try_delta(name),
pack, offset);
}
Expand Down Expand Up @@ -4069,6 +4077,15 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
if (!(bitmap_git = prepare_bitmap_walk(revs, 0)))
return -1;

/*
* For now, disable the --full-name-hash option since the
* bitmaps only store the standard name-hash value. When the
* bitmaps have the ability to specify which name-hash is used,
* then this can be changed to choose the version serialized to
* the bitmap file.
*/
use_full_name_hash = 0;

if (pack_options_allow_reuse())
reuse_partial_packfile_from_bitmap(bitmap_git,
&reuse_packfiles,
Expand Down Expand Up @@ -4429,6 +4446,8 @@ int cmd_pack_objects(int argc,
OPT_STRING_LIST(0, "uri-protocol", &uri_protocols,
N_("protocol"),
N_("exclude any configured uploadpack.blobpackfileuri with this protocol")),
OPT_BOOL(0, "full-name-hash", &use_full_name_hash,
N_("optimize delta compression across identical path names over time")),
OPT_END(),
};

Expand Down Expand Up @@ -4576,6 +4595,20 @@ int cmd_pack_objects(int argc,
if (pack_to_stdout || !rev_list_all)
write_bitmap_index = 0;

if (use_full_name_hash < 0)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Taylor Blau wrote (reply to this):

On Tue, Nov 05, 2024 at 03:05:06AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[email protected]>
>
> As demonstrated in the previous change, the --full-name-hash option of
> 'git pack-objects' is less effective in a trunctated history. Thus, even
> when the option is selected via a command-line option or config, disable
> this option when the '--shallow' option is specified. This will help
> performance in servers that choose to enable the --full-name-hash option
> by default for a repository while not regressing their ability to serve
> shallow clones.
>
> This will not present a compatibility issue in the future when the full
> name hash values are stored in the reachability bitmaps, since shallow
> clones disable bitmaps.
>
> Signed-off-by: Derrick Stolee <[email protected]>
> ---
>  builtin/pack-objects.c       | 6 ++++++
>  t/perf/p5313-pack-objects.sh | 1 +
>  2 files changed, 7 insertions(+)

I appreciate demonstrating the value of declaring --shallow and
--full-name-hash incompatible by showing the performance numbers in the
previous patch.

But TBH I think that it would be equally fine or slightly better to say
up front "when combined with --shallow, this option produces larger
packs during testing, so the two are incompatible for now". You could
include some performance numbers there to illustrate that difference in
the commit log too if you wanted.

But I don't think it's worth introducing the pair as compatible only to
mark them incompatible later on in the same series.

Thanks,
Taylor

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/21/24 3:33 PM, Taylor Blau wrote:
> On Tue, Nov 05, 2024 at 03:05:06AM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <[email protected]>
>>
>> As demonstrated in the previous change, the --full-name-hash option of
>> 'git pack-objects' is less effective in a trunctated history. Thus, even
>> when the option is selected via a command-line option or config, disable
>> this option when the '--shallow' option is specified. This will help
>> performance in servers that choose to enable the --full-name-hash option
>> by default for a repository while not regressing their ability to serve
>> shallow clones.
>>
>> This will not present a compatibility issue in the future when the full
>> name hash values are stored in the reachability bitmaps, since shallow
>> clones disable bitmaps.
>>
>> Signed-off-by: Derrick Stolee <[email protected]>
>> ---
>>   builtin/pack-objects.c       | 6 ++++++
>>   t/perf/p5313-pack-objects.sh | 1 +
>>   2 files changed, 7 insertions(+)
> > I appreciate demonstrating the value of declaring --shallow and
> --full-name-hash incompatible by showing the performance numbers in the
> previous patch.
> > But TBH I think that it would be equally fine or slightly better to say
> up front "when combined with --shallow, this option produces larger
> packs during testing, so the two are incompatible for now". You could
> include some performance numbers there to illustrate that difference in
> the commit log too if you wanted.
> > But I don't think it's worth introducing the pair as compatible only to
> mark them incompatible later on in the same series.
I disagree and here's why: they are not functionally incompatible. This
performance-focused change is worth justifying with performance test data
_and_ isolating from the initial implementation with its own reasoning
for future history spelunkers. Having these warning lines blame to this
patch instead of the initial implementation will make it much easier to
understand the justification of this change.

But maybe this patch can be removed if we use Jonathan's function. I'll
check the performance tests to see if this continues to be justified.

Thanks,
-Stolee

use_full_name_hash = git_env_bool("GIT_TEST_FULL_NAME_HASH", 0);

if (shallow && use_full_name_hash > 0 &&
!git_env_bool("GIT_TEST_USE_FULL_NAME_HASH_WITH_SHALLOW", 0)) {
use_full_name_hash = 0;
warning("the --full-name-hash option is disabled with the --shallow option");
}

if (write_bitmap_index && use_full_name_hash > 0) {
warning(_("currently, the --full-name-hash option is incompatible with --write-bitmap-index"));
use_full_name_hash = 0;
}

if (use_delta_islands)
strvec_push(&rp, "--topo-order");

Expand Down
9 changes: 8 additions & 1 deletion builtin/repack.c
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ static int run_update_server_info = 1;
static char *packdir, *packtmp_name, *packtmp;

static const char *const git_repack_usage[] = {
N_("git repack [<options>]"),
N_("git repack [-a] [-A] [-d] [-f] [-F] [-l] [-n] [-q] [-b] [-m]\n"
"[--window=<n>] [--depth=<n>] [--threads=<n>] [--keep-pack=<pack-name>]\n"
"[--write-midx] [--full-name-hash]"),
NULL
};

Expand All @@ -58,6 +60,7 @@ struct pack_objects_args {
int no_reuse_object;
int quiet;
int local;
int full_name_hash;
struct list_objects_filter_options filter_options;
};

Expand Down Expand Up @@ -306,6 +309,8 @@ static void prepare_pack_objects(struct child_process *cmd,
strvec_pushf(&cmd->args, "--no-reuse-delta");
if (args->no_reuse_object)
strvec_pushf(&cmd->args, "--no-reuse-object");
if (args->full_name_hash)
strvec_pushf(&cmd->args, "--full-name-hash");
if (args->local)
strvec_push(&cmd->args, "--local");
if (args->quiet)
Expand Down Expand Up @@ -1203,6 +1208,8 @@ int cmd_repack(int argc,
N_("pass --no-reuse-delta to git-pack-objects")),
OPT_BOOL('F', NULL, &po_args.no_reuse_object,
N_("pass --no-reuse-object to git-pack-objects")),
OPT_BOOL(0, "full-name-hash", &po_args.full_name_hash,
N_("pass --full-name-hash to git-pack-objects")),
OPT_NEGBIT('n', NULL, &run_update_server_info,
N_("do not run git-update-server-info"), 1),
OPT__QUIET(&po_args.quiet, N_("be quiet")),
Expand Down
21 changes: 21 additions & 0 deletions pack-objects.h
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,27 @@ static inline uint32_t pack_name_hash(const char *name)
return hash;
}

static inline uint32_t pack_full_name_hash(const char *name)
{
const uint32_t bigp = 1234572167U;
uint32_t c, hash = bigp;

if (!name)
return 0;

/*
* Do the simplest thing that will resemble pseudo-randomness: add
* random multiples of a large prime number with a binary shift.
* The goal is not to be cryptographic, but to be generally
* uniformly distributed.
*/
while ((c = *name++) != 0) {
hash += c * bigp;
hash = (hash >> 5) | (hash << 27);
}
return hash;
}

static inline enum object_type oe_type(const struct object_entry *e)
{
return e->type_valid ? e->type_ : OBJ_BAD;
Expand Down
4 changes: 4 additions & 0 deletions t/README
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,10 @@ a test and then fails then the whole test run will abort. This can help to make
sure the expected tests are executed and not silently skipped when their
dependency breaks or is simply not present in a new environment.

GIT_TEST_FULL_NAME_HASH=<boolean>, when true, sets the default name-hash
function in 'git pack-objects' to be the one used by the --full-name-hash
option.

Naming Tests
------------

Expand Down
24 changes: 24 additions & 0 deletions t/helper/test-name-hash.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* test-name-hash.c: Read a list of paths over stdin and report on their
* name-hash and full name-hash.
*/

#include "test-tool.h"
#include "git-compat-util.h"
#include "pack-objects.h"
#include "strbuf.h"

int cmd__name_hash(int argc UNUSED, const char **argv UNUSED)
{
struct strbuf line = STRBUF_INIT;

while (!strbuf_getline(&line, stdin)) {
uint32_t name_hash = pack_name_hash(line.buf);
uint32_t full_hash = pack_full_name_hash(line.buf);

printf("%10"PRIu32"\t%10"PRIu32"\t%s\n", name_hash, full_hash, line.buf);
}

strbuf_release(&line);
return 0;
}
1 change: 1 addition & 0 deletions t/helper/test-tool.c
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ static struct test_cmd cmds[] = {
{ "match-trees", cmd__match_trees },
{ "mergesort", cmd__mergesort },
{ "mktemp", cmd__mktemp },
{ "name-hash", cmd__name_hash },
{ "online-cpus", cmd__online_cpus },
{ "pack-mtimes", cmd__pack_mtimes },
{ "parse-options", cmd__parse_options },
Expand Down
1 change: 1 addition & 0 deletions t/helper/test-tool.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ int cmd__lazy_init_name_hash(int argc, const char **argv);
int cmd__match_trees(int argc, const char **argv);
int cmd__mergesort(int argc, const char **argv);
int cmd__mktemp(int argc, const char **argv);
int cmd__name_hash(int argc, const char **argv);
int cmd__online_cpus(int argc, const char **argv);
int cmd__pack_mtimes(int argc, const char **argv);
int cmd__parse_options(int argc, const char **argv);
Expand Down
97 changes: 97 additions & 0 deletions t/perf/p5313-pack-objects.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
#!/bin/sh
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Taylor Blau wrote (reply to this):

On Tue, Nov 05, 2024 at 03:05:05AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[email protected]>
>
> As custom options are added to 'git pack-objects' and 'git repack' to
> adjust how compression is done, use this new performance test script to
> demonstrate their effectiveness in performance and size.

Nicely done, thank you for adding a perf test to allow readers to easily
verify these changes themselves.

> In the case of the Git repository, these numbers show some of the issues
> with this approach:
>
> [...]
>
> The thin pack that simulates a push is much worse with --full-name-hash
> in this case. The name hash values are doing a lot to assist with delta
> bases, it seems. The big pack and shallow clone cases are slightly worse
> with the --full-name-hash option. Only the full repack gains some
> benefits in size.

Not a problem with your patch, but just thinking aloud: do you think
there is an easy/straightforward way to suggest when to use
--full-name-hash or not?

> ---
>  t/perf/p5313-pack-objects.sh | 94 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
>  create mode 100755 t/perf/p5313-pack-objects.sh
>
> diff --git a/t/perf/p5313-pack-objects.sh b/t/perf/p5313-pack-objects.sh
> new file mode 100755
> index 00000000000..dfa29695315
> --- /dev/null
> +++ b/t/perf/p5313-pack-objects.sh
> @@ -0,0 +1,94 @@
> +#!/bin/sh
> +
> +test_description='Tests pack performance using bitmaps'
> +. ./perf-lib.sh
> +
> +GIT_TEST_PASSING_SANITIZE_LEAK=0
> +export GIT_TEST_PASSING_SANITIZE_LEAK
> +
> +test_perf_large_repo
> +
> +test_expect_success 'create rev input' '
> +	cat >in-thin <<-EOF &&
> +	$(git rev-parse HEAD)
> +	^$(git rev-parse HEAD~1)
> +	EOF
> +
> +	cat >in-big <<-EOF &&
> +	$(git rev-parse HEAD)
> +	^$(git rev-parse HEAD~1000)
> +	EOF
> +
> +	cat >in-shallow <<-EOF
> +	$(git rev-parse HEAD)
> +	--shallow $(git rev-parse HEAD)
> +	EOF
> +'

I was going to comment that these could probably be moved into the
individual perf test that cares about reading each of these inputs. But
having them shared here makes sense since we are naturally comparing
generating two packs with the same input (with and without
--full-name-hash). So the shared setup here makes sense to me.

> +
> +test_perf 'thin pack' '
> +	git pack-objects --thin --stdout --revs --sparse  <in-thin >out
> +'
> +
> +test_size 'thin pack size' '
> +	test_file_size out
> +'

Nice. I always forget about this and end up writing 'wc -c <out'.

> +test_perf 'thin pack with --full-name-hash' '
> +	git pack-objects --thin --stdout --revs --sparse --full-name-hash <in-thin >out
> +'
> +
> +test_size 'thin pack size with --full-name-hash' '
> +	test_file_size out
> +'
> +
> +test_perf 'big pack' '
> +	git pack-objects --stdout --revs --sparse  <in-big >out
> +'
> +
> +test_size 'big pack size' '
> +	test_file_size out
> +'
> +
> +test_perf 'big pack with --full-name-hash' '
> +	git pack-objects --stdout --revs --sparse --full-name-hash <in-big >out
> +'
> +
> +test_size 'big pack size with --full-name-hash' '
> +	test_file_size out
> +'
> +
> +test_perf 'shallow fetch pack' '
> +	git pack-objects --stdout --revs --sparse --shallow <in-shallow >out
> +'
> +
> +test_size 'shallow pack size' '
> +	test_file_size out
> +'
> +
> +test_perf 'shallow pack with --full-name-hash' '
> +	git pack-objects --stdout --revs --sparse --shallow --full-name-hash <in-shallow >out
> +'
> +
> +test_size 'shallow pack size with --full-name-hash' '
> +	test_file_size out
> +'
> +
> +test_perf 'repack' '
> +	git repack -adf
> +'
> +
> +test_size 'repack size' '
> +	pack=$(ls .git/objects/pack/pack-*.pack) &&
> +	test_file_size "$pack"

Here and below, I think it's fine to inline this as in:

    test_file_size "$(ls .git/objects/pack/pack-*.pack)"

...but I wonder: will using ".git" break this test in bare repositories?
Should we write instead:

    pack="$(ls $(git rev-parse --git-dir)/objects/pack/pack-*.pack)" &&
    test_file_size

?

Thanks,
Taylor

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/21/24 3:31 PM, Taylor Blau wrote:
> On Tue, Nov 05, 2024 at 03:05:05AM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <[email protected]>

>> The thin pack that simulates a push is much worse with --full-name-hash
>> in this case. The name hash values are doing a lot to assist with delta
>> bases, it seems. The big pack and shallow clone cases are slightly worse
>> with the --full-name-hash option. Only the full repack gains some
>> benefits in size.
> > Not a problem with your patch, but just thinking aloud: do you think
> there is an easy/straightforward way to suggest when to use
> --full-name-hash or not?

The kinds of heuristics I would use are:

1. Are there enough commits that enough files have enough versions
   across history that it's very important to keep deltas within a path?

2. Is the repository at least 500MB such that there is actually room for
   a "meaningful" change in size?

3. Are there a lot of name-hash collisions? (The last patch in the series
   helps do this through a test-helper, but isn't something we can expect
   end users to check themselves.)


>> +	cat >in-shallow <<-EOF
>> +	$(git rev-parse HEAD)
>> +	--shallow $(git rev-parse HEAD)
>> +	EOF
>> +'
> > I was going to comment that these could probably be moved into the
> individual perf test that cares about reading each of these inputs. But
> having them shared here makes sense since we are naturally comparing
> generating two packs with the same input (with and without
> --full-name-hash). So the shared setup here makes sense to me.

I also wanted to avoid having these commands be part of the time
measurement, even if they are extremely small.

>> +
>> +test_perf 'thin pack' '
>> +	git pack-objects --thin --stdout --revs --sparse  <in-thin >out
>> +'
>> +
>> +test_size 'thin pack size' '
>> +	test_file_size out
>> +'
> > Nice. I always forget about this and end up writing 'wc -c <out'.

I believe this is a Junio recommendation from an earlier version.

>> +test_size 'repack size' '
>> +	pack=$(ls .git/objects/pack/pack-*.pack) &&
>> +	test_file_size "$pack"
> > Here and below, I think it's fine to inline this as in:
> >      test_file_size "$(ls .git/objects/pack/pack-*.pack)"

Generally I prefer to split things into stages so the verbose output
provides a clear definition of the value when calling the Git command.

> ...but I wonder: will using ".git" break this test in bare repositories?
> Should we write instead:
> >      pack="$(ls $(git rev-parse --git-dir)/objects/pack/pack-*.pack)" &&
>      test_file_size
> > ?
While this would break a bare repo, the perf lib makes a bare repo be
copied into a non-bare repo as follows:

test_perf_copy_repo_contents () {
	for stuff in "$1"/*
	do
		case "$stuff" in
		*/objects|*/hooks|*/config|*/commondir|*/gitdir|*/worktrees|*/fsmonitor--daemon*)
			;;
		*)
			cp -R "$stuff" "$repo/.git/" || exit 1
			;;
		esac
	done
}

I'll still add the `git rev-parse` suggestion because it's safest.

Thanks,
-Stolee


test_description='Tests pack performance using bitmaps'
. ./perf-lib.sh

GIT_TEST_PASSING_SANITIZE_LEAK=0
export GIT_TEST_PASSING_SANITIZE_LEAK

test_perf_large_repo

test_expect_success 'create rev input' '
cat >in-thin <<-EOF &&
$(git rev-parse HEAD)
^$(git rev-parse HEAD~1)
EOF

cat >in-big <<-EOF &&
$(git rev-parse HEAD)
^$(git rev-parse HEAD~1000)
EOF

cat >in-shallow <<-EOF
$(git rev-parse HEAD)
--shallow $(git rev-parse HEAD)
EOF
'

test_perf 'thin pack' '
git pack-objects --thin --stdout --revs --sparse <in-thin >out
'

test_size 'thin pack size' '
test_file_size out
'

test_perf 'thin pack with --full-name-hash' '
git pack-objects --thin --stdout --revs --sparse --full-name-hash <in-thin >out
'

test_size 'thin pack size with --full-name-hash' '
test_file_size out
'

test_perf 'big pack' '
git pack-objects --stdout --revs --sparse <in-big >out
'

test_size 'big pack size' '
test_file_size out
'

test_perf 'big pack with --full-name-hash' '
git pack-objects --stdout --revs --sparse --full-name-hash <in-big >out
'

test_size 'big pack size with --full-name-hash' '
test_file_size out
'

test_perf 'shallow fetch pack' '
git pack-objects --stdout --revs --sparse --shallow <in-shallow >out
'

test_size 'shallow pack size' '
test_file_size out
'

test_perf 'shallow pack with --full-name-hash' '
GIT_TEST_USE_FULL_NAME_HASH_WITH_SHALLOW=1 \
git pack-objects --stdout --revs --sparse --shallow --full-name-hash <in-shallow >out
'

test_size 'shallow pack size with --full-name-hash' '
test_file_size out
'

test_perf 'repack' '
git repack -adf
'

test_size 'repack size' '
gitdir=$(git rev-parse --git-dir) &&
pack=$(ls $gitdir/objects/pack/pack-*.pack) &&
test_file_size "$pack"
'

test_perf 'repack with --full-name-hash' '
git repack -adf --full-name-hash
'

test_size 'repack size with --full-name-hash' '
gitdir=$(git rev-parse --git-dir) &&
pack=$(ls $gitdir/objects/pack/pack-*.pack) &&
test_file_size "$pack"
'

test_done
Loading
Loading