index-pack: improve performance on NFS

The index-pack command determines if a sha1 collision test is needed by checking the existence of a loose object with the given hash. In my tests, I can improve performance of “git clone” on Amazon EFS by 8x when used with a non-default mount option (lookupcache=pos) that's required for a Gitlab HA setup. My assumption is that this check is unnecessary when cloning into a new repository because the repository will be empty.

Quoted 6 lines · Geert

The index-pack command determines if a sha1 collision test is needed by checking the existence of a loose object with the given hash. In my tests, I can improve performance of “git clone” on Amazon EFS by 8x when used with a non-default mount option (lookupcache=pos) that's required for a Gitlab HA setup. My assumption is that this check is unnecessary when cloning into a new repository because the repository will be empty.

My knee-jerk reaction is that your insight that we can skip the "dup check" when starting from emptiness is probably correct, but your use of .cloning flag as an approximation of "are we starting from emptiness?" is probably wrong, at least for two reasons.

- "git clone --reference=..." does not strictly start from emptiness, and would still have to make sure that incoming pack does not try to inject an object with different contents but with the same name.

- "git init && git fetch ..." starts from emptiness and would want to benefit from the same optimization as you are implementing here.

As to the implementation, I think the patch adjusts the right "if()" condition to skip the collision test here.

builtin/index-pack.c static void sha1_object(const void *data, struct object_entry *obj_entry, · line 801

Quoted 5 lines

builtin/index-pack.c +1 -1

-	if (startup_info->have_repository) {
+	if (startup_info->have_repository && !cloning) {
 		read_lock();
 		collision_test_needed =
 			has_sha1_file_with_flags(oid->hash, OBJECT_INFO_QUICK);

I just do not think !cloning is quite correct.

But as Junio notes the devil's in the details, another one I thought of is:

GIT_OBJECT_DIRECTORY=/some/other/repository git clone ...

It seems to me that ...

Quoted 5 lines · Ævar Arnfjörð Bjarmason

But as Junio notes the devil's in the details, another one I thought of is:

GIT_OBJECT_DIRECTORY=/some/other/repository git clone ...

Quoted 1 line

It seems to me that ...

Actually I take all of that back ;-)

For the purpose of this patch, use of existing .cloning field in the transport is fine, as the sole existing user of the field wants the field to mean "Are we starting with an empty object store?", and not "Are we running the command whose name is 'git clone'?".

Quoted 7 lines · Junio C Hamano

Quoted 5 lines · Ævar Arnfjörð Bjarmason

But as Junio notes the devil's in the details, another one I thought of is:

GIT_OBJECT_DIRECTORY=/some/other/repository git clone ...

Quoted 8 lines

Quoted 1 line

It seems to me that ...

Actually I take all of that back ;-)

For the purpose of this patch, use of existing .cloning field in the transport is fine, as the sole existing user of the field wants the field to mean "Are we starting with an empty object store?", and not "Are we running the command whose name is 'git clone'?".

Taking one step back, the root problem in this thread is that stat() on non-existing files is slow (which makes has_sha1_file slow).

One solution there is to cache the results of looking in .git/objects (or any alternate object store) for loose files. And indeed, this whole scheme is just a specialized form of that: it's a flag to say "hey, we do not have any objects yet, so do not bother looking".

Could we implement that in a more direct and central way? And could we implement it in a way that catches more cases? E.g., if I have _one_ object, that defeats this specialized optimization, but it is probably still beneficial to cache that knowledge (and the reasonable cutoff is probably not 1, but some value of N loose objects).

Of course any cache raises questions of cache invalidation, but I think we've already dealt with that for this case. When we use OBJECT_INFO_QUICK, that is a sign that we want to make this kind of accuracy/speed tradeoff (which does a similar caching thing with packfiles).

So putting that all together, could we have something like:

object-store.h +12 -0

index 63b7605a3e..28cde568a0 100644
@@ -135,6 +135,18 @@ struct raw_object_store {
 	 */
 	struct packed_git *all_packs;
 
+	/*
+	 * A set of all loose objects we have. This probably ought to be split
+	 * into a set of 256 caches so that we can fault in one directory at a
+	 * time.
+	 */
+	struct oid_array loose_cache;
+	enum {
+		LOOSE_CACHE_UNFILLED = 0,
+		LOOSE_CACHE_INVALID,
+		LOOSE_CACHE_VALID
+	} loose_cache_status;
+
 	/*
 	 * A fast, rough count of the number of objects in the repository.
 	 * These two fields are not meant for direct access. Use

packfile.c +2 -0

index 86074a76e9..68ca4fff0e 100644
@@ -990,6 +990,8 @@ void reprepare_packed_git(struct repository *r)
 	r->objects->approximate_object_count_valid = 0;
 	r->objects->packed_git_initialized = 0;
 	prepare_packed_git(r);
+	oid_array_clear(&r->objects->loose_cache);
+	r->objects->loose_cache_status = LOOSE_CACHE_UNFILLED;
 }
 
 struct packed_git *get_packed_git(struct repository *r)

sha1-file.c +8 -0

index dd0b6aa873..edbe037eaa 100644
@@ -1172,6 +1172,40 @@ int parse_sha1_header(const char *hdr, unsigned long *sizep)
 	return parse_sha1_header_extended(hdr, &oi, 0);
 }
 
+/* probably should be configurable? */
+#define LOOSE_OBJECT_CACHE_MAX 65536
+
+static int fill_loose_cache(const struct object_id *oid,
+			    const char *path,
+			    void *data)
+{
+	struct oid_array *cache = data;

I left the door open for that in the new config option 4/4 implements, but I suspect for Geert's purposes this is something he'd prefer to turn off in git on clone entirely, i.e. because it may be running on some random Amazon's customer's EFS instance, and they won't know about this new core.checkCollisions option.

But maybe I'm wrong about that and Geert is happy to just turn on core.checkCollisions=false and use this series instead.

Maybe a better option would be to check for the non-existence of the [00-ff] directories under .git/objects.

Add a new core.checkCollisions setting. On by default, it can be set to 'false' to disable the check for existing objects in sha1_object().
...

builtin/index-pack.c +2 -1

index 2004e25da2..4a3508aa9f 100644
@@ -791,23 +791,24 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
 {
 	void *new_data = NULL;
 	int collision_test_needed = 0;
+	int do_coll_check = git_config_get_collision_check();
 
 	assert(data || obj_entry);
 
-	if (startup_info->have_repository) {
+	if (do_coll_check && startup_info->have_repository) {
 		read_lock();
 		collision_test_needed =
 			has_sha1_file_with_flags(oid->hash, OBJECT_INFO_QUICK);
 		read_unlock();
 	}

Quoted 23 lines · Ævar Arnfjörð Bjarmason

Add a new core.checkCollisions setting. On by default, it can be set to 'false' to disable the check for existing objects in sha1_object().
...

builtin/index-pack.c +2 -1

index 2004e25da2..4a3508aa9f 100644
@@ -791,23 +791,24 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
 {
 	void *new_data = NULL;
 	int collision_test_needed = 0;
+	int do_coll_check = git_config_get_collision_check();
 
 	assert(data || obj_entry);
 
-	if (startup_info->have_repository) {
+	if (do_coll_check && startup_info->have_repository) {
 		read_lock();
 		collision_test_needed =
 			has_sha1_file_with_flags(oid->hash, OBJECT_INFO_QUICK);
 		read_unlock();
 	}
 

builtin/index-pack.c static void sha1_object(const void *data, struct object_entry *obj_entry, · line 811

Quoted 9 lines

builtin/index-pack.c +2 -2

-	if (collision_test_needed && !data) {
+	if (do_coll_check && collision_test_needed && !data) {
 		read_lock();
 		if (!check_collison(obj_entry))
 			collision_test_needed = 0;
 		read_unlock();
 	}
-	if (collision_test_needed) {
+	if (do_coll_check && collision_test_needed) {

If I am reading the patch correctly, The latter two changes are totally unnecessary. c-t-needed is true only when dO-coll_check allowed the initial "do we even have that object?" check to kick in and never set otherwise.

I am not all that enthused to the idea of sending a wrong message to our users, i.e. it is sometimes OK to sacrifice the security of collision detection.

A small change like this is easy to adjust to apply to the codebase, even after today's codebase undergoes extensive modifications; quite honestly, I'd prefer not having to worry about it so close to the pre-release feature freeze.

Quoted 8 lines

Taking one step back, the root problem in this thread is that stat() on non-existing files is slow (which makes has_sha1_file slow).

One solution there is to cache the results of looking in .git/objects (or any alternate object store) for loose files. And indeed, this whole scheme is just a specialized form of that: it's a flag to say "hey, we do not have any objects yet, so do not bother looking".

Quoted 5 lines

Could we implement that in a more direct and central way? And could we implement it in a way that catches more cases? E.g., if I have _one_ object, that defeats this specialized optimization, but it is probably still beneficial to cache that knowledge (and the reasonable cutoff is probably not 1, but some value of N loose objects).

[...]

The assumption with making it exactly 0 objects and not any value of >0 is that we can safely assume that a "clone" or initial "fetch"[1] is special in ways that a clone isn't. I.e. we're starting out with nothing and doing the initial population, that's probably not as true in an existing repo that's getting concurrent fetches, commits, rebases etc.

But in the spirit of taking a step back, maybe we should take two steps back and consider why we're doing this at all.

Three of our tests fail if we compile git like this, and cloning is much faster (especially on NFS):

    diff --git a/builtin/index-pack.c b/builtin/index-pack.c
    index 2004e25da2..0c2d008ee0 100644
    --- a/builtin/index-pack.c
    +++ b/builtin/index-pack.c
    @@ -796,3 +796,3 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,

    -       if (startup_info->have_repository) {
    +       if (0) {
                    read_lock();

Even on a local disk I'm doing 262759 lstat() calls cloning git.git and spending 5% of my time on that.

But why do we have this in the first place? It's because of 8685da4256 ("don't ever allow SHA1 collisions to exist by fetching a pack", 2007-03-20) and your 51054177b3 ("index-pack: detect local corruption in collision check", 2017-04-01).

I.e. we are worried about (and those tests check for):

a) A malicious user trying to give us repository where they have created an object with the same SHA-1 that's different, as in the SHAttered attack.

I remember (but have not dug up) an old E-Mail from Linus saying that this was an important security aspect of git, i.e. even if SHA-1 was broken you couldn't easily propagate bad objects.

b) Cases where we've ended up with different content for a SHA-1 due to e.g. a local FS corruption. Which is the subject of your commit in 2017.

c) Are there cases where fetch.fsckObjects is off and we just flip a bit on the wire and don't notice? I think not because we always check the pack checksum (don't we), but I'm not 100% sure.

Even if someone wants to make the argument that this is behavior that we absolutely *MUST* keep and not make configurable, there's still much smarter ways to do it.

We could e.g. just unconditionally write out the packfile into a quarantine environment (see 720dae5a19 ("config doc: elaborate on fetch.fsckObjects security", 2018-07-27)), *then* loop over the loose objects and packs we have and see if any of those exist in the new pack, if they do, do the current assertion, and if not (and fetch.fsckObjects passes) move it out of the quarantine.

Quoted 3 lines

Even if someone wants to make the argument that this is behavior that we absolutely *MUST* keep and not make configurable, there's still much smarter ways to do it.

I don't have any real object to a configuration like this, if people want to experiment with it. But in contrast, the patch I showed earlier:

- is safe enough to just turn on all the time, without the user having to configure anything nor make a safety tradeoff

- speeds up all the other spots that use OBJECT_INFO_QUICK (like fetch's tag-following, or what appears to be the exact same optimization done manually inside mark_complete_and_common-ref()).

Quoted 15 lines · Ævar Arnfjörð Bjarmason

Quoted 13 lines

Taking one step back, the root problem in this thread is that stat() on non-existing files is slow (which makes has_sha1_file slow).

One solution there is to cache the results of looking in .git/objects (or any alternate object store) for loose files. And indeed, this whole scheme is just a specialized form of that: it's a flag to say "hey, we do not have any objects yet, so do not bother looking".

Could we implement that in a more direct and central way? And could we implement it in a way that catches more cases? E.g., if I have _one_ object, that defeats this specialized optimization, but it is probably still beneficial to cache that knowledge (and the reasonable cutoff is probably not 1, but some value of N loose objects).

[...]

Quoted 5 lines

The assumption with making it exactly 0 objects and not any value of >0 is that we can safely assume that a "clone" or initial "fetch"[1] is special in ways that a clone isn't. I.e. we're starting out with nothing and doing the initial population, that's probably not as true in an existing repo that's getting concurrent fetches, commits, rebases etc.

I assume you mean s/that a clone isn't/that a fetch isn't/.

I agree there are cases where you might be able to go further if you assume a full "0". But my point is that "clone" is an ambiguous concept, and it doesn't map completely to what's actually slow here. So if you only look at "are we cloning", then:

- you have a bunch of cases which are "clones", but aren't actually starting from scratch

- you get zero benefit in the non-clone cases, when we could be scaling the benefit smoothly

Quoted 2 lines

But in the spirit of taking a step back, maybe we should take two steps back and consider why we're doing this at all.

OK, I think it's worth discussing, and I'll do that below. But first I want to say...

Quoted 13 lines

Three of our tests fail if we compile git like this, and cloning is much faster (especially on NFS):

    diff --git a/builtin/index-pack.c b/builtin/index-pack.c
    index 2004e25da2..0c2d008ee0 100644
    --- a/builtin/index-pack.c
    +++ b/builtin/index-pack.c
    @@ -796,3 +796,3 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,

    -       if (startup_info->have_repository) {
    +       if (0) {
                    read_lock();

Quoted 2 lines

Even on a local disk I'm doing 262759 lstat() calls cloning git.git and spending 5% of my time on that.

With the caching patch I posted earlier, I see roughly the same speedup on an index-pack of git.git as I do with disabling the collision check entirely (I did see about a 1% difference in favor of what you wrote above, which was within the noise, but may well be valid due to slightly reduced lock contention).

TBH I'm not sure if any of this is actually worth caring about on a normal Linux system, though. There stat() is fast. It might be much more interesting on macOS or Windows, or on a Linux system on NFS.

Quoted 11 lines

But why do we have this in the first place? It's because of 8685da4256 ("don't ever allow SHA1 collisions to exist by fetching a pack", 2007-03-20) and your 51054177b3 ("index-pack: detect local corruption in collision check", 2017-04-01).

I.e. we are worried about (and those tests check for):

a) A malicious user trying to give us repository where they have created an object with the same SHA-1 that's different, as in the SHAttered attack.

Quoted 3 lines

I remember (but have not dug up) an old E-Mail from Linus saying that this was an important security aspect of git, i.e. even if SHA-1 was broken you couldn't easily propagate bad objects.

Yeah, especially given recent advances in SHA-1 attacks, I'm not super comfortable with the idea of disabling the duplicate-object check at this point.

Quoted 3 lines

b) Cases where we've ended up with different content for a SHA-1 due to e.g. a local FS corruption. Which is the subject of your commit in 2017.

Sort of. We actually detected it before my patch, but we just gave a really crappy error message. ;)

Quoted 3 lines

c) Are there cases where fetch.fsckObjects is off and we just flip a bit on the wire and don't notice? I think not because we always check the pack checksum (don't we), but I'm not 100% sure.

We'd detect bit-blips on the wire due to the pack checksum. But what's more interesting are bit-flips on the disk of the sender, which would then put the bogus data into the pack checksum they generate on the fly.

However, we do detect such a bit-flip, even without fsckObjects, because the sender does not tell us the expected sha-1 of each object. It gives us a stream of objects, and the receiver computes the sha-1's themselves. So a bit flip manifests in the connectivity-check when we say "hey, the other side should have sent us object X but did not" (we do not say "gee, what is this object Y they sent?" because after not seeing X, we do not know which objects would have been reachable, so we have a whole bunch of such Y's).

fetch.fsckObjects is purely about doing semantic object-quality checks. They're not even that expensive to do. The main reason they're disabled is that there are many historical objects floating around that fail them (I think it would be a useful exercise to sort the existing checks by priority, downgrading many of them to warnings, and then setting the default for fetch.fsckObjects to "reject anything above warning").

Quoted 3 lines

Even if someone wants to make the argument that this is behavior that we absolutely *MUST* keep and not make configurable, there's still much smarter ways to do it.

I don't have any real object to a configuration like this, if people want to experiment with it. But in contrast, the patch I showed earlier:

- is safe enough to just turn on all the time, without the user having to configure anything nor make a safety tradeoff

- speeds up all the other spots that use OBJECT_INFO_QUICK (like fetch's tag-following, or what appears to be the exact same optimization done manually inside mark_complete_and_common-ref()).

Quoted 6 lines

We could e.g. just unconditionally write out the packfile into a quarantine environment (see 720dae5a19 ("config doc: elaborate on fetch.fsckObjects security", 2018-07-27)), *then* loop over the loose objects and packs we have and see if any of those exist in the new pack, if they do, do the current assertion, and if not (and fetch.fsckObjects passes) move it out of the quarantine.

Yes, I agree that would work, though it's a bigger architecture change.

Quoted 3 lines

Yeah, especially given recent advances in SHA-1 attacks, I'm not super comfortable with the idea of disabling the duplicate-object check at this point.

I'd be comfortable with it in my setup since it's been limited to collision attacks that are computationally prohibitive, and there being no sign of preimage attacks, which is the case we really need to worry about.

It does introduce a race condition where you can introduce a colliding object to the repository by doing two concurrent pushes, but as you note in
https://public-inbox.org/git/[email protected]/
this already applies to packs, so you can trigger that with the right sized push (depending on transfer.unpackLimit), and we also have this in existing forms for other stuff.

I do think it's amazingly paranoid to be worried about SHA-1 collisions in the first place, and a bit odd to leave the door open on these race conditions. I.e. it's hard to imagine a state-level[1] actor with sufficient motivation to exploit this who wouldn't find some way to make the race condition work as an escape hatch.

In a busy repo that gets a lot of branches / branch deletions (so not quite as extreme as [2], but close) and the default expiry policy you can easily have 20-100K loose objects (something near the lower bound of that is the current live state of one server I'm looking at).

A recursive opendir()/readdir() on that on local disk is really fast if it's in cache, but can easily be 1-5 seconds on NFS. So for a push we'd now pay up to 5s just populating a cache we'll bearly use to accept some tiny push with just a few objects.

* Re-roll my 4 patch series to include the patch you have in <[email protected]>

Quoted 2 lines · Ævar Arnfjörð Bjarmason

* Re-roll my 4 patch series to include the patch you have in <[email protected]>

I don't think it's quite ready for inclusion as-is. I hope to brush it up a bit, but I have quite a backlog of stuff to review, as well.

Quoted 6 lines · Jeff King

Quoted 2 lines · Ævar Arnfjörð Bjarmason

* Re-roll my 4 patch series to include the patch you have in <[email protected]>

I don't think it's quite ready for inclusion as-is. I hope to brush it up a bit, but I have quite a backlog of stuff to review, as well.

We're still quite keen to get this patch included. Is there anything I can do to help?

Also I just re-read your comments on maximum cache size. I think you were arguing both sides of the equation and I wasn't sure where you'd ended up. :) A larger cache size potentially takes more time to fill up especially on NFS while a smaller cache size obviously would less effective. That said a small cache is still effective for the "clone" case where the repo is empty.

It also occurred to me that as a performance optimization your patch could read the the loose object directories in parallel using a thread pool. At least on Amazon EFS this should result in al almost linear performance increase. I'm not sure how much this would help for local file systems. In any case this may be best done as a follow-up patch (that I'd be happy to volunteer for).

Yes, testing and review. :)

I won't send the series out just yet, as I suspect it could use another read-through on my part. But if you want to peek at it or try some timings, it's available at:

https://github.com/peff/git jk/loose-cache

Quoted 7 lines · Jeff King

Yes, testing and review. :)

I won't send the series out just yet, as I suspect it could use another read-through on my part. But if you want to peek at it or try some timings, it's available at:

https://github.com/peff/git jk/loose-cache

I gave this branch a go. There's a performance regression as I'm getting a clone speed of about 100 KiB/s while with the previous patch I got around 20 MiB/s. The culprint appears to be a very large number of stat() calls on ".git/objects/info/alternates". The call stack is:

-> quick_has_loose()
-> prepare_alt_odb()
-> read_info_alternates()

Heh, indeed. Try this on top:

sha1-file.c +0 -0

index bc35b28e17..9ff27f92ed 100644

Quoted 11 lines · Geert Jansen

Quoted 6 lines · Jeff King

Quoted 2 lines · Ævar Arnfjörð Bjarmason

* Re-roll my 4 patch series to include the patch you have in <[email protected]>

I don't think it's quite ready for inclusion as-is. I hope to brush it up a bit, but I have quite a backlog of stuff to review, as well.

We're still quite keen to get this patch included. Is there anything I can do to help?

Yes, testing and review. :)

I won't send the series out just yet, as I suspect it could use another read-through on my part. But if you want to peek at it or try some timings, it's available at:

https://github.com/peff/git jk/loose-cache

Quoted 14 lines · Jeff King

Quoted 11 lines · Geert Jansen

Quoted 6 lines · Jeff King

Quoted 2 lines · Ævar Arnfjörð Bjarmason

* Re-roll my 4 patch series to include the patch you have in <[email protected]>

I don't think it's quite ready for inclusion as-is. I hope to brush it up a bit, but I have quite a backlog of stuff to review, as well.

We're still quite keen to get this patch included. Is there anything I can do to help?

Quoted 7 lines

Yes, testing and review. :)

I won't send the series out just yet, as I suspect it could use another read-through on my part. But if you want to peek at it or try some timings, it's available at:

https://github.com/peff/git jk/loose-cache

Just a comment on this from the series:

Note that it is possible for this to actually be _slower_. We'll do a full readdir() to fill the cache, so if you have a very large number of loose objects and a very small number of lookups, that readdir() may end up more expensive.

In practice, though, having a large number of loose objects is already a performance problem, which should be fixed by repacking or pruning via git-gc. So on balance, this should be a good tradeoff.

Our biggest repo has a very large number of loose objects at any given time, but the vast majority of these are because gc *is* happening very frequently and the default expiry policy of 2wks is in effect.

Having a large number of loose objects is not per-se a performance problem.

It's a problem if you end up "faulting" to from packs to the loose object directory a lot because those objects are still reachable, but if they're not reachable that number can grow very large if your ref churn is large (so lots of expired loose object production).

Anyway, the series per-se looks good to me. It's particularly nice to have some of the ODB cleanup + cleanup in fetch-pack.c

Just wanted to note that in our default (reasonable) config we do produce scenarios where this change can still be somewhat pathological, so I'm still interested in disabling it entirely given the implausibility of what it's guarding against.

Just a comment on this from the series:

Note that it is possible for this to actually be _slower_. We'll do a full readdir() to fill the cache, so if you have a very large number of loose objects and a very small number of lookups, that readdir() may end up more expensive.

In practice, though, having a large number of loose objects is already a performance problem, which should be fixed by repacking or pruning via git-gc. So on balance, this should be a good tradeoff.

Our biggest repo has a very large number of loose objects at any given time, but the vast majority of these are because gc *is* happening very frequently and the default expiry policy of 2wks is in effect.

Having a large number of loose objects is not per-se a performance problem.

The run-command API makes no promises about what is left in a struct child_process after a command finishes, and it's not safe to simply reuse it again for a similar command. In particular:

- if you use child->args or child->env_array, they are cleared after finish_command()

- likewise, start_command() may point child->argv at child->args->argv; reusing that would lead to accessing freed memory

- the in/out/err may hold pipe descriptors from the previous run

These two calls are _probably_ OK because they do not use any of those features. But it's only by chance, and may break in the future; let's reinitialize our struct for each program we run.

Signed-off-by Jeff King <[email protected]>

---
  builtin/fsck.c | 6 ++++++
  1 file changed, 6 insertions(+)

builtin/fsck.c +3 -0

index 06eb421720..b10f2b154c 100644
@@ -841,6 +841,9 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 
  		prepare_alt_odb(the_repository);
  		for (alt =  the_repository->objects->alt_odb_list; alt; alt = alt->next) {
+			child_process_init(&commit_graph_verify);
+			commit_graph_verify.argv = verify_argv;
+			commit_graph_verify.git_cmd = 1;
  			verify_argv[2] = "--object-dir";
  			verify_argv[3] = alt->path;
  			if (run_command(&commit_graph_verify))

Using strip_suffix() lets us avoid repeating ourselves. It also makes the handling of "/" a bit less subtle (we strip one less character than we matched in order to leave it in place, but we can just as easily include the "/" when we add more path components).

Signed-off-by Jeff King <[email protected]>

In preparation for unifying the handling of alt odb's and the normal repo object directory, let's use a more neutral name. This patch is purely mechanical, swapping the type name, and converting any variables named "alt" to "odb". There should be no functional change, but it will reduce the noise in subsequent diffs.

Signed-off-by Jeff King <[email protected]>

---
I waffled on calling this object_database instead of object_directory. But really, it is very specifically about the directory (packed storage, including packs from alternates, is handled elsewhere).

  builtin/count-objects.c     |  4 ++--
  builtin/fsck.c              | 16 ++++++-------
  builtin/submodule--helper.c |  6 ++---
  commit-graph.c              | 10 ++++----
  object-store.h              | 14 +++++------
  object.c                    | 10 ++++----
  packfile.c                  |  8 +++----
  sha1-file.c                 | 48 ++++++++++++++++++-------------------
  sha1-name.c                 | 20 ++++++++--------
  transport.c                 |  2 +-
  10 files changed, 69 insertions(+), 69 deletions(-)

builtin/count-objects.c +2 -2

index a7cad052c6..3fae474f6f 100644
@@ -78,10 +78,10 @@ static int count_cruft(const char *basename, const char *path, void *data)
  	return 0;
  }
 
-static int print_alternate(struct alternate_object_database *alt, void *data)
+static int print_alternate(struct object_directory *odb, void *data)
  {
  	printf("alternate: ");
-	quote_c_style(alt->path, NULL, stdout, 0);
+	quote_c_style(odb->path, NULL, stdout, 0);
  	putchar('\n');
  	return 0;
  }

builtin/fsck.c +8 -8

index b10f2b154c..55153cf92a 100644
@@ -688,7 +688,7 @@ static struct option fsck_opts[] = {
  int cmd_fsck(int argc, const char **argv, const char *prefix)
  {
  	int i;
-	struct alternate_object_database *alt;
+	struct object_directory *odb;
 
  	/* fsck knows how to handle missing promisor objects */
  	fetch_if_missing = 0;

@@ -725,14 +725,14 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
  		for_each_loose_object(mark_loose_for_connectivity, NULL, 0);
  		for_each_packed_object(mark_packed_for_connectivity, NULL, 0);
  	} else {
-		struct alternate_object_database *alt_odb_list;
+		struct object_directory *alt_odb_list;
 
  		fsck_object_dir(get_object_directory());
 
  		prepare_alt_odb(the_repository);
  		alt_odb_list = the_repository->objects->alt_odb_list;
-		for (alt = alt_odb_list; alt; alt = alt->next)
-			fsck_object_dir(alt->path);
+		for (odb = alt_odb_list; odb; odb = odb->next)
+			fsck_object_dir(odb->path);
 
  		if (check_full) {
  			struct packed_git *p;

@@ -840,12 +840,12 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
  			errors_found |= ERROR_COMMIT_GRAPH;
 
  		prepare_alt_odb(the_repository);
-		for (alt =  the_repository->objects->alt_odb_list; alt; alt = alt->next) {
+		for (odb = the_repository->objects->alt_odb_list; odb; odb = odb->next) {
  			child_process_init(&commit_graph_verify);
  			commit_graph_verify.argv = verify_argv;
  			commit_graph_verify.git_cmd = 1;
  			verify_argv[2] = "--object-dir";
-			verify_argv[3] = alt->path;
+			verify_argv[3] = odb->path;
  			if (run_command(&commit_graph_verify))
  				errors_found |= ERROR_COMMIT_GRAPH;
  		}

@@ -861,12 +861,12 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
  			errors_found |= ERROR_COMMIT_GRAPH;
 
  		prepare_alt_odb(the_repository);
-		for (alt =  the_repository->objects->alt_odb_list; alt; alt = alt->next) {
+		for (odb = the_repository->objects->alt_odb_list; odb; odb = odb->next) {
  			child_process_init(&midx_verify);
  			midx_verify.argv = midx_argv;
  			midx_verify.git_cmd = 1;
  			midx_argv[2] = "--object-dir";
-			midx_argv[3] = alt->path;
+			midx_argv[3] = odb->path;
  			if (run_command(&midx_verify))
  				errors_found |= ERROR_COMMIT_GRAPH;
  		}

builtin/submodule--helper.c +3 -3

index 28b9449e82..3ae451bc46 100644
@@ -1265,7 +1265,7 @@ struct submodule_alternate_setup {
  	SUBMODULE_ALTERNATE_ERROR_IGNORE, NULL }
 
  static int add_possible_reference_from_superproject(
-		struct alternate_object_database *alt, void *sas_cb)
+		struct object_directory *odb, void *sas_cb)
  {
  	struct submodule_alternate_setup *sas = sas_cb;
  	size_t len;

@@ -1274,11 +1274,11 @@ static int add_possible_reference_from_superproject(
  	 * If the alternate object store is another repository, try the
  	 * standard layout with .git/(modules/<name>)+/objects
  	 */
-	if (strip_suffix(alt->path, "/objects", &len)) {
+	if (strip_suffix(odb->path, "/objects", &len)) {
  		char *sm_alternate;
  		struct strbuf sb = STRBUF_INIT;
  		struct strbuf err = STRBUF_INIT;
-		strbuf_add(&sb, alt->path, len);
+		strbuf_add(&sb, odb->path, len);
 
  		/*
  		 * We need to end the new path with '/' to mark it as a dir,

commit-graph.c +5 -5

index 40c855f185..5dd3f5b15c 100644
@@ -230,7 +230,7 @@ static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
   */
  static int prepare_commit_graph(struct repository *r)
  {
-	struct alternate_object_database *alt;
+	struct object_directory *odb;
  	char *obj_dir;
  	int config_value;
 

@@ -255,10 +255,10 @@ static int prepare_commit_graph(struct repository *r)
  	obj_dir = r->objects->objectdir;
  	prepare_commit_graph_one(r, obj_dir);
  	prepare_alt_odb(r);
-	for (alt = r->objects->alt_odb_list;
-	     !r->objects->commit_graph && alt;
-	     alt = alt->next)
-		prepare_commit_graph_one(r, alt->path);
+	for (odb = r->objects->alt_odb_list;
+	     !r->objects->commit_graph && odb;
+	     odb = odb->next)
+		prepare_commit_graph_one(r, odb->path);
  	return !!r->objects->commit_graph;
  }
 

object-store.h +7 -7

index 63b7605a3e..122d5f75e2 100644
@@ -7,8 +7,8 @@
  #include "sha1-array.h"
  #include "strbuf.h"
 
-struct alternate_object_database {
-	struct alternate_object_database *next;
+struct object_directory {
+	struct object_directory *next;
 
  	/* see alt_scratch_buf() */
  	struct strbuf scratch;

@@ -32,14 +32,14 @@ struct alternate_object_database {
  };
  void prepare_alt_odb(struct repository *r);
  char *compute_alternate_path(const char *path, struct strbuf *err);
-typedef int alt_odb_fn(struct alternate_object_database *, void *);
+typedef int alt_odb_fn(struct object_directory *, void *);
  int foreach_alt_odb(alt_odb_fn, void*);
 
  /*
   * Allocate a "struct alternate_object_database" but do _not_ actually
   * add it to the list of alternates.
   */
-struct alternate_object_database *alloc_alt_odb(const char *dir);
+struct object_directory *alloc_alt_odb(const char *dir);
 
  /*
   * Add the directory to the on-disk alternates file; the new entry will also

@@ -60,7 +60,7 @@ void add_to_alternates_memory(const char *dir);
   * alternate. Always use this over direct access to alt->scratch, as it
   * cleans up any previous use of the scratch buffer.
   */
-struct strbuf *alt_scratch_buf(struct alternate_object_database *alt);
+struct strbuf *alt_scratch_buf(struct object_directory *odb);
 
  struct packed_git {
  	struct packed_git *next;

@@ -100,8 +100,8 @@ struct raw_object_store {
  	/* Path to extra alternate object database if not NULL */
  	char *alternate_db;
 
-	struct alternate_object_database *alt_odb_list;
-	struct alternate_object_database **alt_odb_tail;
+	struct object_directory *alt_odb_list;
+	struct object_directory **alt_odb_tail;
 
  	/*
  	 * Objects that should be substituted by other objects

object.c +5 -5

index e54160550c..6af8e908bb 100644
@@ -482,17 +482,17 @@ struct raw_object_store *raw_object_store_new(void)
  	return o;
  }
 
-static void free_alt_odb(struct alternate_object_database *alt)
+static void free_alt_odb(struct object_directory *odb)
  {
-	strbuf_release(&alt->scratch);
-	oid_array_clear(&alt->loose_objects_cache);
-	free(alt);
+	strbuf_release(&odb->scratch);
+	oid_array_clear(&odb->loose_objects_cache);
+	free(odb);
  }
 
  static void free_alt_odbs(struct raw_object_store *o)
  {
  	while (o->alt_odb_list) {
-		struct alternate_object_database *next;
+		struct object_directory *next;
 
  		next = o->alt_odb_list->next;
  		free_alt_odb(o->alt_odb_list);

packfile.c +4 -4

index f2850a00b5..d6d511cfd2 100644
@@ -966,16 +966,16 @@ static void prepare_packed_git_mru(struct repository *r)
 
  static void prepare_packed_git(struct repository *r)
  {
-	struct alternate_object_database *alt;
+	struct object_directory *odb;
 
  	if (r->objects->packed_git_initialized)
  		return;
  	prepare_multi_pack_index_one(r, r->objects->objectdir, 1);
  	prepare_packed_git_one(r, r->objects->objectdir, 1);
  	prepare_alt_odb(r);
-	for (alt = r->objects->alt_odb_list; alt; alt = alt->next) {
-		prepare_multi_pack_index_one(r, alt->path, 0);
-		prepare_packed_git_one(r, alt->path, 0);
+	for (odb = r->objects->alt_odb_list; odb; odb = odb->next) {
+		prepare_multi_pack_index_one(r, odb->path, 0);
+		prepare_packed_git_one(r, odb->path, 0);
  	}
  	rearrange_packed_git(r);
 

sha1-file.c +24 -24

index dd0b6aa873..a3cc650a0a 100644
@@ -353,16 +353,16 @@ void sha1_file_name(struct repository *r, struct strbuf *buf, const unsigned cha
  	fill_sha1_path(buf, sha1);
  }
 
-struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)
+struct strbuf *alt_scratch_buf(struct object_directory *odb)
  {
-	strbuf_setlen(&alt->scratch, alt->base_len);
-	return &alt->scratch;
+	strbuf_setlen(&odb->scratch, odb->base_len);
+	return &odb->scratch;
  }
 
-static const char *alt_sha1_path(struct alternate_object_database *alt,
+static const char *alt_sha1_path(struct object_directory *odb,
  				 const unsigned char *sha1)
  {
-	struct strbuf *buf = alt_scratch_buf(alt);
+	struct strbuf *buf = alt_scratch_buf(odb);
  	fill_sha1_path(buf, sha1);
  	return buf->buf;
  }

@@ -374,7 +374,7 @@ static int alt_odb_usable(struct raw_object_store *o,
  			  struct strbuf *path,
  			  const char *normalized_objdir)
  {
-	struct alternate_object_database *alt;
+	struct object_directory *odb;
 
  	/* Detect cases where alternate disappeared */
  	if (!is_directory(path->buf)) {

@@ -388,8 +388,8 @@ static int alt_odb_usable(struct raw_object_store *o,
  	 * Prevent the common mistake of listing the same
  	 * thing twice, or object directory itself.
  	 */
-	for (alt = o->alt_odb_list; alt; alt = alt->next) {
-		if (!fspathcmp(path->buf, alt->path))
+	for (odb = o->alt_odb_list; odb; odb = odb->next) {
+		if (!fspathcmp(path->buf, odb->path))
  			return 0;
  	}
  	if (!fspathcmp(path->buf, normalized_objdir))

@@ -402,7 +402,7 @@ static int alt_odb_usable(struct raw_object_store *o,
   * Prepare alternate object database registry.
   *
   * The variable alt_odb_list points at the list of struct
- * alternate_object_database.  The elements on this list come from
+ * object_directory.  The elements on this list come from
   * non-empty elements from colon separated ALTERNATE_DB_ENVIRONMENT
   * environment variable, and $GIT_OBJECT_DIRECTORY/info/alternates,
   * whose contents is similar to that environment variable but can be

@@ -419,7 +419,7 @@ static void read_info_alternates(struct repository *r,
  static int link_alt_odb_entry(struct repository *r, const char *entry,
  	const char *relative_base, int depth, const char *normalized_objdir)
  {
-	struct alternate_object_database *ent;
+	struct object_directory *ent;
  	struct strbuf pathbuf = STRBUF_INIT;
 
  	if (!is_absolute_path(entry) && relative_base) {

@@ -540,9 +540,9 @@ static void read_info_alternates(struct repository *r,
  	free(path);
  }
 
-struct alternate_object_database *alloc_alt_odb(const char *dir)
+struct object_directory *alloc_alt_odb(const char *dir)
  {
-	struct alternate_object_database *ent;
+	struct object_directory *ent;
 
  	FLEX_ALLOC_STR(ent, path, dir);
  	strbuf_init(&ent->scratch, 0);

@@ -684,7 +684,7 @@ char *compute_alternate_path(const char *path, struct strbuf *err)
 
  int foreach_alt_odb(alt_odb_fn fn, void *cb)
  {
-	struct alternate_object_database *ent;
+	struct object_directory *ent;
  	int r = 0;
 
  	prepare_alt_odb(the_repository);

@@ -743,10 +743,10 @@ static int check_and_freshen_local(const struct object_id *oid, int freshen)
 
  static int check_and_freshen_nonlocal(const struct object_id *oid, int freshen)
  {
-	struct alternate_object_database *alt;
+	struct object_directory *odb;
  	prepare_alt_odb(the_repository);
-	for (alt = the_repository->objects->alt_odb_list; alt; alt = alt->next) {
-		const char *path = alt_sha1_path(alt, oid->hash);
+	for (odb = the_repository->objects->alt_odb_list; odb; odb = odb->next) {
+		const char *path = alt_sha1_path(odb, oid->hash);
  		if (check_and_freshen_file(path, freshen))
  			return 1;
  	}

@@ -893,7 +893,7 @@ int git_open_cloexec(const char *name, int flags)
  static int stat_sha1_file(struct repository *r, const unsigned char *sha1,
  			  struct stat *st, const char **path)
  {
-	struct alternate_object_database *alt;
+	struct object_directory *odb;
  	static struct strbuf buf = STRBUF_INIT;
 
  	strbuf_reset(&buf);

@@ -905,8 +905,8 @@ static int stat_sha1_file(struct repository *r, const unsigned char *sha1,
 
  	prepare_alt_odb(r);
  	errno = ENOENT;
-	for (alt = r->objects->alt_odb_list; alt; alt = alt->next) {
-		*path = alt_sha1_path(alt, sha1);
+	for (odb = r->objects->alt_odb_list; odb; odb = odb->next) {
+		*path = alt_sha1_path(odb, sha1);
  		if (!lstat(*path, st))
  			return 0;
  	}

@@ -922,7 +922,7 @@ static int open_sha1_file(struct repository *r,
  			  const unsigned char *sha1, const char **path)
  {
  	int fd;
-	struct alternate_object_database *alt;
+	struct object_directory *odb;
  	int most_interesting_errno;
  	static struct strbuf buf = STRBUF_INIT;
 

@@ -936,8 +936,8 @@ static int open_sha1_file(struct repository *r,
  	most_interesting_errno = errno;
 
  	prepare_alt_odb(r);
-	for (alt = r->objects->alt_odb_list; alt; alt = alt->next) {
-		*path = alt_sha1_path(alt, sha1);
+	for (odb = r->objects->alt_odb_list; odb; odb = odb->next) {
+		*path = alt_sha1_path(odb, sha1);
  		fd = git_open(*path);
  		if (fd >= 0)
  			return fd;

@@ -2139,14 +2139,14 @@ struct loose_alt_odb_data {
  	void *data;
  };
 
-static int loose_from_alt_odb(struct alternate_object_database *alt,
+static int loose_from_alt_odb(struct object_directory *odb,
  			      void *vdata)
  {
  	struct loose_alt_odb_data *data = vdata;
  	struct strbuf buf = STRBUF_INIT;
  	int r;
 
-	strbuf_addstr(&buf, alt->path);
+	strbuf_addstr(&buf, odb->path);
  	r = for_each_loose_file_in_objdir_buf(&buf,
  					      data->cb, NULL, NULL,
  					      data->data);

sha1-name.c +10 -10

index faa60f69e3..2594aa79f8 100644
@@ -95,8 +95,8 @@ static int match_sha(unsigned, const unsigned char *, const unsigned char *);
  static void find_short_object_filename(struct disambiguate_state *ds)
  {
  	int subdir_nr = ds->bin_pfx.hash[0];
-	struct alternate_object_database *alt;
-	static struct alternate_object_database *fakeent;
+	struct object_directory *odb;
+	static struct object_directory *fakeent;
 
  	if (!fakeent) {
  		/*

@@ -110,24 +110,24 @@ static void find_short_object_filename(struct disambiguate_state *ds)
  	}
  	fakeent->next = the_repository->objects->alt_odb_list;
 
-	for (alt = fakeent; alt && !ds->ambiguous; alt = alt->next) {
+	for (odb = fakeent; odb && !ds->ambiguous; odb = odb->next) {
  		int pos;
 
-		if (!alt->loose_objects_subdir_seen[subdir_nr]) {
-			struct strbuf *buf = alt_scratch_buf(alt);
+		if (!odb->loose_objects_subdir_seen[subdir_nr]) {
+			struct strbuf *buf = alt_scratch_buf(odb);
  			for_each_file_in_obj_subdir(subdir_nr, buf,
  						    append_loose_object,
  						    NULL, NULL,
-						    &alt->loose_objects_cache);
-			alt->loose_objects_subdir_seen[subdir_nr] = 1;
+						    &odb->loose_objects_cache);
+			odb->loose_objects_subdir_seen[subdir_nr] = 1;
  		}
 
-		pos = oid_array_lookup(&alt->loose_objects_cache, &ds->bin_pfx);
+		pos = oid_array_lookup(&odb->loose_objects_cache, &ds->bin_pfx);
  		if (pos < 0)
  			pos = -1 - pos;
-		while (!ds->ambiguous && pos < alt->loose_objects_cache.nr) {
+		while (!ds->ambiguous && pos < odb->loose_objects_cache.nr) {
  			const struct object_id *oid;
-			oid = alt->loose_objects_cache.oid + pos;
+			oid = odb->loose_objects_cache.oid + pos;
  			if (!match_sha(ds->len, ds->bin_pfx.hash, oid->hash))
  				break;
  			update_candidates(ds, oid);

transport.c +1 -1

index 5a74b609ff..040e92c134 100644
@@ -1433,7 +1433,7 @@ struct alternate_refs_data {
  	void *data;
  };
 
-static int refs_from_alternate_cb(struct alternate_object_database *e,
+static int refs_from_alternate_cb(struct object_directory *e,
  				  void *data)
  {
  	struct strbuf path = STRBUF_INIT;

Quoted 11 lines · Jeff King

In preparation for unifying the handling of alt odb's and the normal repo object directory, let's use a more neutral name. This patch is purely mechanical, swapping the type name, and converting any variables named "alt" to "odb". There should be no functional change, but it will reduce the noise in subsequent diffs.

Signed-off-by Jeff King <[email protected]>

---
I waffled on calling this object_database instead of object_directory. But really, it is very specifically about the directory (packed storage, including packs from alternates, is handled elsewhere).

That makes sense. Each alternate makes its own object directory, but is part of a larger object database. It also helps clarify a difference from the object_store.

My only complaint is that you have a lot of variable names with "odb" which are now object_directory pointers. Perhaps "odb" -> "objdir"? Or is that just too much change?

Quoted 13 lines · Derrick Stolee

Quoted 11 lines · Jeff King

In preparation for unifying the handling of alt odb's and the normal repo object directory, let's use a more neutral name. This patch is purely mechanical, swapping the type name, and converting any variables named "alt" to "odb". There should be no functional change, but it will reduce the noise in subsequent diffs.

Signed-off-by Jeff King <[email protected]>

---
I waffled on calling this object_database instead of object_directory. But really, it is very specifically about the directory (packed storage, including packs from alternates, is handled elsewhere).

Quoted 7 lines

That makes sense. Each alternate makes its own object directory, but is part of a larger object database. It also helps clarify a difference from the object_store.

My only complaint is that you have a lot of variable names with "odb" which are now object_directory pointers. Perhaps "odb" -> "objdir"? Or is that just too much change?

Yeah, that was part of my waffling. ;)

Quoted 1 line

From my conversions, usually "objdir" is a string holding the pathname,

though that's not set in stone. I also like that "odb" is the same short length as "alt", which helps with conversion.

Since we're changing the semantics, let's take the opportunity to give it a more hash-neutral name (which will also catch any callers from topics in flight).

When we generate loose file paths for the main object directory, the caller provides a buffer to loose_object_path (formerly sha1_file_name). The callers generally keep their own static buffer to avoid excessive reallocations.

But for alternate directories, each struct carries its own scratch buffer. This is needlessly different; let's unify them.

We could go either direction here, but this patch moves the alternates struct over to the main directory style (rather than vice-versa). Technically the alternates style is more efficient, as it avoids rewriting the object directory name on each call. But this is unlikely to matter in practice, as we avoid reallocations either way (and nobody has ever noticed or complained that the main object directory is copying a few extra bytes before making a much more expensive system call).

And this has the advantage that the reusable buffers are tied to particular calls, which makes the invalidation rules simpler (for example, the return value from stat_sha1_file() used to be invalidated by basically any other object call, but now it is affected only by other calls to stat_sha1_file()).

We do steal the trick from alt_sha1_path() of returning a pointer to the filled buffer, which makes a few conversions more convenient.

Signed-off-by Jeff King <[email protected]>

---
  object-store.h | 14 +-------------
  object.c       |  1 -
  sha1-file.c    | 44 ++++++++++++++++----------------------------
  sha1-name.c    |  8 ++++++--
  4 files changed, 23 insertions(+), 44 deletions(-)

object-store.h +1 -13

index fefa17e380..b2fa0d0df0 100644
@@ -10,10 +10,6 @@
  struct object_directory {
  	struct object_directory *next;
 
-	/* see alt_scratch_buf() */
-	struct strbuf scratch;
-	size_t base_len;
-
  	/*
  	 * Used to store the results of readdir(3) calls when searching
  	 * for unique abbreviated hashes.  This cache is never

@@ -54,14 +50,6 @@ void add_to_alternates_file(const char *dir);
   */
  void add_to_alternates_memory(const char *dir);
 
-/*
- * Returns a scratch strbuf pre-filled with the alternate object directory,
- * including a trailing slash, which can be used to access paths in the
- * alternate. Always use this over direct access to alt->scratch, as it
- * cleans up any previous use of the scratch buffer.
- */
-struct strbuf *alt_scratch_buf(struct object_directory *odb);
-
  struct packed_git {
  	struct packed_git *next;
  	struct list_head mru;

@@ -157,7 +145,7 @@ void raw_object_store_clear(struct raw_object_store *o);
   * Put in `buf` the name of the file in the local object database that
   * would be used to store a loose object with the specified sha1.
   */
-void loose_object_path(struct repository *r, struct strbuf *buf, const unsigned char *sha1);
+const char *loose_object_path(struct repository *r, struct strbuf *buf, const unsigned char *sha1);
 
  void *map_sha1_file(struct repository *r, const unsigned char *sha1, unsigned long *size);
 

object.c +0 -1

index 6af8e908bb..dd485ac629 100644
@@ -484,7 +484,6 @@ struct raw_object_store *raw_object_store_new(void)
 
  static void free_alt_odb(struct object_directory *odb)
  {
-	strbuf_release(&odb->scratch);
  	oid_array_clear(&odb->loose_objects_cache);
  	free(odb);
  }

sha1-file.c +16 -28

index 478eac326b..15db6b61a9 100644
@@ -346,27 +346,20 @@ static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)
  	}
  }
 
-void loose_object_path(struct repository *r, struct strbuf *buf,
-		       const unsigned char *sha1)
+static const char *odb_loose_path(const char *path, struct strbuf *buf,
+				  const unsigned char *sha1)
  {
  	strbuf_reset(buf);
-	strbuf_addstr(buf, r->objects->objectdir);
+	strbuf_addstr(buf, path);
  	strbuf_addch(buf, '/');
  	fill_sha1_path(buf, sha1);
+	return buf->buf;
  }
 
-struct strbuf *alt_scratch_buf(struct object_directory *odb)
+const char *loose_object_path(struct repository *r, struct strbuf *buf,
+			      const unsigned char *sha1)
  {
-	strbuf_setlen(&odb->scratch, odb->base_len);
-	return &odb->scratch;
-}
-
-static const char *alt_sha1_path(struct object_directory *odb,
-				 const unsigned char *sha1)
-{
-	struct strbuf *buf = alt_scratch_buf(odb);
-	fill_sha1_path(buf, sha1);
-	return buf->buf;
+	return odb_loose_path(r->objects->objectdir, buf, sha1);
  }
 
  /*

@@ -547,9 +540,6 @@ struct object_directory *alloc_alt_odb(const char *dir)
  	struct object_directory *ent;
 
  	FLEX_ALLOC_STR(ent, path, dir);
-	strbuf_init(&ent->scratch, 0);
-	strbuf_addf(&ent->scratch, "%s/", dir);
-	ent->base_len = ent->scratch.len;
 
  	return ent;
  }

@@ -745,10 +735,12 @@ static int check_and_freshen_local(const struct object_id *oid, int freshen)
  static int check_and_freshen_nonlocal(const struct object_id *oid, int freshen)
  {
  	struct object_directory *odb;
+	static struct strbuf path = STRBUF_INIT;
+
  	prepare_alt_odb(the_repository);
  	for (odb = the_repository->objects->alt_odb_list; odb; odb = odb->next) {
-		const char *path = alt_sha1_path(odb, oid->hash);
-		if (check_and_freshen_file(path, freshen))
+		odb_loose_path(odb->path, &path, oid->hash);
+		if (check_and_freshen_file(path.buf, freshen))
  			return 1;
  	}
  	return 0;

@@ -889,7 +881,7 @@ int git_open_cloexec(const char *name, int flags)
   *
   * The "path" out-parameter will give the path of the object we found (if any).
   * Note that it may point to static storage and is only valid until another
- * call to loose_object_path(), etc.
+ * call to stat_sha1_file().
   */
  static int stat_sha1_file(struct repository *r, const unsigned char *sha1,
  			  struct stat *st, const char **path)

@@ -897,16 +889,14 @@ static int stat_sha1_file(struct repository *r, const unsigned char *sha1,
  	struct object_directory *odb;
  	static struct strbuf buf = STRBUF_INIT;
 
-	loose_object_path(r, &buf, sha1);
-	*path = buf.buf;
-
+	*path = loose_object_path(r, &buf, sha1);
  	if (!lstat(*path, st))
  		return 0;
 
  	prepare_alt_odb(r);
  	errno = ENOENT;
  	for (odb = r->objects->alt_odb_list; odb; odb = odb->next) {
-		*path = alt_sha1_path(odb, sha1);
+		*path = odb_loose_path(odb->path, &buf, sha1);
  		if (!lstat(*path, st))
  			return 0;
  	}

@@ -926,9 +916,7 @@ static int open_sha1_file(struct repository *r,
  	int most_interesting_errno;
  	static struct strbuf buf = STRBUF_INIT;
 
-	loose_object_path(r, &buf, sha1);
-	*path = buf.buf;
-
+	*path = loose_object_path(r, &buf, sha1);
  	fd = git_open(*path);
  	if (fd >= 0)
  		return fd;

@@ -936,7 +924,7 @@ static int open_sha1_file(struct repository *r,
 
  	prepare_alt_odb(r);
  	for (odb = r->objects->alt_odb_list; odb; odb = odb->next) {
-		*path = alt_sha1_path(odb, sha1);
+		*path = odb_loose_path(odb->path, &buf, sha1);
  		fd = git_open(*path);
  		if (fd >= 0)
  			return fd;

sha1-name.c +6 -2

index 2594aa79f8..96a8e71482 100644
@@ -97,6 +97,7 @@ static void find_short_object_filename(struct disambiguate_state *ds)
  	int subdir_nr = ds->bin_pfx.hash[0];
  	struct object_directory *odb;
  	static struct object_directory *fakeent;
+	struct strbuf buf = STRBUF_INIT;
 
  	if (!fakeent) {
  		/*

@@ -114,8 +115,9 @@ static void find_short_object_filename(struct disambiguate_state *ds)
  		int pos;
 
  		if (!odb->loose_objects_subdir_seen[subdir_nr]) {
-			struct strbuf *buf = alt_scratch_buf(odb);
-			for_each_file_in_obj_subdir(subdir_nr, buf,
+			strbuf_reset(&buf);
+			strbuf_addstr(&buf, odb->path);
+			for_each_file_in_obj_subdir(subdir_nr, &buf,
  						    append_loose_object,
  						    NULL, NULL,
  						    &odb->loose_objects_cache);

@@ -134,6 +136,8 @@ static void find_short_object_filename(struct disambiguate_state *ds)
  			pos++;
  		}
  	}
+
+	strbuf_release(&buf);
  }
 
  static int match_sha(unsigned len, const unsigned char *a, const unsigned char *b)

Quoted 7 lines

We could go either direction here, but this patch moves the alternates struct over to the main directory style (rather than vice-versa). Technically the alternates style is more efficient, as it avoids rewriting the object directory name on each call. But this is unlikely to matter in practice, as we avoid reallocations either way (and nobody has ever noticed or complained that the main object directory is copying a few extra bytes before making a much more expensive system call).

Hm. I've complained in the past [1] about a simple method like strbuf_addf() over loose objects, but that was during abbreviation checks so we were adding the string for every loose object but not actually reading the objects.

[1]
https://public-inbox.org/git/[email protected]/

Quoted 12 lines · Derrick Stolee

Quoted 7 lines

We could go either direction here, but this patch moves the alternates struct over to the main directory style (rather than vice-versa). Technically the alternates style is more efficient, as it avoids rewriting the object directory name on each call. But this is unlikely to matter in practice, as we avoid reallocations either way (and nobody has ever noticed or complained that the main object directory is copying a few extra bytes before making a much more expensive system call).

Hm. I've complained in the past [1] about a simple method like strbuf_addf() over loose objects, but that was during abbreviation checks so we were adding the string for every loose object but not actually reading the objects.

Quoted 2 lines

[1]
https://public-inbox.org/git/[email protected]/

I suspect that had more to do with the cost of snprintf() than the extra bytes being copied. And here we'd still be using addstr and addch exclusively. I'm open to numeric arguments to the contrary, though. :)

There's actually a lot of low-hanging fruit there for pre-sizing, too. E.g., fill_sha1_path() calls strbuf_addch() in a loop, but it could quite easily grow the 41 bytes it needs ahead of time. I wouldn't want to change that without finding a measurable improvement, though. It might not be a big deal due to fec501dae8 (strbuf_addch: avoid calling strbuf_grow, 2015-04-16).

-Peff

Our handling of alternate object directories is needlessly different from the main object directory. As a result, many places in the code basically look like this:

   do_something(r->objects->objdir);

   for (odb = r->objects->alt_odb_list; odb; odb = odb->next)
         do_something(odb->path);

That gets annoying when do_something() is non-trivial, and we've resorted to gross hacks like creating fake alternates (see find_short_object_filename()).

Instead, let's give each raw_object_store a unified list of object_directory structs. The first will be the main store, and everything after is an alternate. Very few callers even care about the distinction, and can just loop over the whole list (and those who care can just treat the first element differently).

A few observations:

- we don't need r->objects->objectdir anymore, and can just

     mechanically convert that to r->objects->odb->path

- object_directory's path field needs to become a real pointer rather than a FLEX_ARRAY, in order to fill it with expand_base_dir()

- we'll call prepare_alt_odb() earlier in many functions (i.e., outside of the loop). This may result in us calling it even when our function would be satisfied looking only at the main odb.

But this doesn't matter in practice. It's not a very expensive operation in the first place, and in the majority of cases it will be a noop. We call it already (and cache its results) in prepare_packed_git(), and we'll generally check packs before loose objects. So essentially every program is going to call it immediately once per program.

Arguably we should just prepare_alt_odb() immediately upon setting up the repository's object directory, which would save us sprinkling calls throughout the code base (and forgetting to do so has been a source of subtle bugs in the past). But I've stopped short of that here, since there are already a lot of other moving parts in this patch.

- Most call sites just get shorter. The check_and_freshen() functions are an exception, because they have entry points to handle local and nonlocal directories separately.

Signed-off-by Jeff King <[email protected]>

---
If the "the first one is the main store, the rest are alternates" bit is too subtle, we could mark each "struct object_directory" with a bit for "is_local".

  builtin/fsck.c |  21 ++-------
  builtin/grep.c |   2 +-
  commit-graph.c |   5 +-
  environment.c  |   4 +-
  object-store.h |  27 ++++++-----
  object.c       |  19 ++++----
  packfile.c     |  10 ++--
  path.c         |   2 +-
  repository.c   |   8 +++-
  sha1-file.c    | 122 ++++++++++++++++++-------------------------------
  sha1-name.c    |  17 ++-----
  11 files changed, 90 insertions(+), 147 deletions(-)

builtin/fsck.c +3 -18

index 55153cf92a..15338bd178 100644
@@ -725,13 +725,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
  		for_each_loose_object(mark_loose_for_connectivity, NULL, 0);
  		for_each_packed_object(mark_packed_for_connectivity, NULL, 0);
  	} else {
-		struct object_directory *alt_odb_list;
-
-		fsck_object_dir(get_object_directory());
-
  		prepare_alt_odb(the_repository);
-		alt_odb_list = the_repository->objects->alt_odb_list;
-		for (odb = alt_odb_list; odb; odb = odb->next)
+		for (odb = the_repository->objects->odb; odb; odb = odb->next)
  			fsck_object_dir(odb->path);
 
  		if (check_full) {

@@ -834,13 +829,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
  		struct child_process commit_graph_verify = CHILD_PROCESS_INIT;
  		const char *verify_argv[] = { "commit-graph", "verify", NULL, NULL, NULL };
 
-		commit_graph_verify.argv = verify_argv;
-		commit_graph_verify.git_cmd = 1;
-		if (run_command(&commit_graph_verify))
-			errors_found |= ERROR_COMMIT_GRAPH;
-
  		prepare_alt_odb(the_repository);
-		for (odb = the_repository->objects->alt_odb_list; odb; odb = odb->next) {
+		for (odb = the_repository->objects->odb; odb; odb = odb->next) {
  			child_process_init(&commit_graph_verify);
  			commit_graph_verify.argv = verify_argv;
  			commit_graph_verify.git_cmd = 1;

@@ -855,13 +845,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
  		struct child_process midx_verify = CHILD_PROCESS_INIT;
  		const char *midx_argv[] = { "multi-pack-index", "verify", NULL, NULL, NULL };
 
-		midx_verify.argv = midx_argv;
-		midx_verify.git_cmd = 1;
-		if (run_command(&midx_verify))
-			errors_found |= ERROR_COMMIT_GRAPH;
-
  		prepare_alt_odb(the_repository);
-		for (odb = the_repository->objects->alt_odb_list; odb; odb = odb->next) {
+		for (odb = the_repository->objects->odb; odb; odb = odb->next) {
  			child_process_init(&midx_verify);
  			midx_verify.argv = midx_argv;
  			midx_verify.git_cmd = 1;

builtin/grep.c +1 -1

index d8508ddf79..714c8d91ba 100644
@@ -441,7 +441,7 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,
  	 * object.
  	 */
  	grep_read_lock();
-	add_to_alternates_memory(submodule.objects->objectdir);
+	add_to_alternates_memory(submodule.objects->odb->path);
  	grep_read_unlock();
 
  	if (oid) {

commit-graph.c +1 -4

index 5dd3f5b15c..99163c244b 100644
@@ -231,7 +231,6 @@ static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
  static int prepare_commit_graph(struct repository *r)
  {
  	struct object_directory *odb;
-	char *obj_dir;
  	int config_value;
 
  	if (r->objects->commit_graph_attempted)

@@ -252,10 +251,8 @@ static int prepare_commit_graph(struct repository *r)
  	if (!commit_graph_compatible(r))
  		return 0;
 
-	obj_dir = r->objects->objectdir;
-	prepare_commit_graph_one(r, obj_dir);
  	prepare_alt_odb(r);
-	for (odb = r->objects->alt_odb_list;
+	for (odb = r->objects->odb;
  	     !r->objects->commit_graph && odb;
  	     odb = odb->next)
  		prepare_commit_graph_one(r, odb->path);

environment.c +2 -2

index 3f3c8746c2..441ce56690 100644
@@ -274,9 +274,9 @@ const char *get_git_work_tree(void)
 
  char *get_object_directory(void)
  {
-	if (!the_repository->objects->objectdir)
+	if (!the_repository->objects->odb)
  		BUG("git environment hasn't been setup");
-	return the_repository->objects->objectdir;
+	return the_repository->objects->odb->path;
  }
 
  int odb_mkstemp(struct strbuf *temp_filename, const char *pattern)

object-store.h +13 -14

index b2fa0d0df0..30faf7b391 100644
@@ -24,19 +24,14 @@ struct object_directory {
  	 * Path to the alternative object store. If this is a relative path,
  	 * it is relative to the current working directory.
  	 */
-	char path[FLEX_ARRAY];
+	char *path;
  };
+
  void prepare_alt_odb(struct repository *r);
  char *compute_alternate_path(const char *path, struct strbuf *err);
  typedef int alt_odb_fn(struct object_directory *, void *);
  int foreach_alt_odb(alt_odb_fn, void*);
 
-/*
- * Allocate a "struct alternate_object_database" but do _not_ actually
- * add it to the list of alternates.
- */
-struct object_directory *alloc_alt_odb(const char *dir);
-
  /*
   * Add the directory to the on-disk alternates file; the new entry will also
   * take effect in the current process.

@@ -80,17 +75,21 @@ struct multi_pack_index;
 
  struct raw_object_store {
  	/*
-	 * Path to the repository's object store.
-	 * Cannot be NULL after initialization.
+	 * Set of all object directories; the main directory is first (and
+	 * cannot be NULL after initialization). Subsequent directories are
+	 * alternates.
  	 */
-	char *objectdir;
+	struct object_directory *odb;
+	struct object_directory **odb_tail;
+	int loaded_alternates;
 
-	/* Path to extra alternate object database if not NULL */
+	/*
+	 * A list of alternate object directories loaded from the environment;
+	 * this should not generally need to be accessed directly, but will
+	 * populate the "odb" list when prepare_alt_odb() is run.
+	 */
  	char *alternate_db;
 
-	struct object_directory *alt_odb_list;
-	struct object_directory **alt_odb_tail;
-
  	/*
  	 * Objects that should be substituted by other objects
  	 * (see git-replace(1)).

object.c +10 -9

index dd485ac629..79d636091c 100644
@@ -482,26 +482,26 @@ struct raw_object_store *raw_object_store_new(void)
  	return o;
  }
 
-static void free_alt_odb(struct object_directory *odb)
+static void free_object_directory(struct object_directory *odb)
  {
+	free(odb->path);
  	oid_array_clear(&odb->loose_objects_cache);
  	free(odb);
  }
 
-static void free_alt_odbs(struct raw_object_store *o)
+static void free_object_directories(struct raw_object_store *o)
  {
-	while (o->alt_odb_list) {
+	while (o->odb) {
  		struct object_directory *next;
 
-		next = o->alt_odb_list->next;
-		free_alt_odb(o->alt_odb_list);
-		o->alt_odb_list = next;
+		next = o->odb->next;
+		free_object_directory(o->odb);
+		o->odb = next;
  	}
  }
 
  void raw_object_store_clear(struct raw_object_store *o)
  {
-	FREE_AND_NULL(o->objectdir);
  	FREE_AND_NULL(o->alternate_db);
 
  	oidmap_free(o->replace_map, 1);

@@ -511,8 +511,9 @@ void raw_object_store_clear(struct raw_object_store *o)
  	o->commit_graph = NULL;
  	o->commit_graph_attempted = 0;
 
-	free_alt_odbs(o);
-	o->alt_odb_tail = NULL;
+	free_object_directories(o);
+	o->odb_tail = NULL;
+	o->loaded_alternates = 0;
 
  	INIT_LIST_HEAD(&o->packed_git_mru);
  	close_all_packs(o);

packfile.c +0 -0

index d6d511cfd2..1eda33247f 100644
@@ -970,12 +970,12 @@ static void prepare_packed_git(struct repository *r)
 
  	if (r->objects->packed_git_initialized)

Quoted 3 lines

If the "the first one is the main store, the rest are alternates" bit is too subtle, we could mark each "struct object_directory" with a bit for "is_local".

This is probably a good thing to do proactively. We have the equivalent in the packed_git struct, but that's also because they get out of order. At the moment, I can't think of a read-only action that needs to treat the local object directory more carefully. The closest I know about is 'git pack-objects --local', but that also writes a pack-file.

I assume that when we write a pack-file to the "default location" we use get_object_directory() instead of referring to the default object_directory?

Quoted 10 lines

packfile.c +1 -2

index d6d511cfd2..1eda33247f 100644
@@ -970,12 +970,12 @@ static void prepare_packed_git(struct repository *r)
  	if (r->objects->packed_git_initialized)
  		return;
-	prepare_multi_pack_index_one(r, r->objects->objectdir, 1);
-	prepare_packed_git_one(r, r->objects->objectdir, 1);
+

packfile.c static void prepare_packed_git(struct repository *r) · line 976

Quoted 6 lines

packfile.c +2 -3

  	prepare_alt_odb(r);
-	for (odb = r->objects->alt_odb_list; odb; odb = odb->next) {
-		prepare_multi_pack_index_one(r, odb->path, 0);
-		prepare_packed_git_one(r, odb->path, 0);
+	for (odb = r->objects->odb; odb; odb = odb->next) {
+		int local = (odb == r->objects->odb);

Here seems to be a place where odb->is_local would help.

Quoted 4 lines · Derrick Stolee

Quoted 3 lines

If the "the first one is the main store, the rest are alternates" bit is too subtle, we could mark each "struct object_directory" with a bit for "is_local".

Quoted 8 lines

This is probably a good thing to do proactively. We have the equivalent in the packed_git struct, but that's also because they get out of order. At the moment, I can't think of a read-only action that needs to treat the local object directory more carefully. The closest I know about is 'git pack-objects --local', but that also writes a pack-file.

I assume that when we write a pack-file to the "default location" we use get_object_directory() instead of referring to the default object_directory?

Generally, yes, though that should eventually be going away in favor of accessing it via a "struct repository". And after my series, get_object_directory() is just returning the_repository->objects->odb->path (i.e., using the "first one is main" rule).

One thing that makes me nervous about a "local" flag in each struct is that it implies that it's the source of truth for where to write to. So what does git_object_directory() look like after that? Do we leave it with the "first one is main" rule? Or does it become:

  for (odb = the_repository->objects->odb; odb; odb = odb->next) {
        if (odb->local)
                return odb->path;
  }
  return NULL; /* yikes? */

? That feels like it's making things more complicated, not less.