Re: [PATCH 6/9] sha1-file: use an object_directory for the main object dir
On Mon, Nov 12, 2018 at 11:04:52AM -0800, Stefan Beller wrote:
> > 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.
>
> It depends if the caller cares about the local flag.
>
> I'd think we can have more than one local, eventually?
> Just think of the partial clone stuff that may have a local
> set of promised stuff and another set of actual objects,
> which may be stored in different local odbs.
Yeah, but I think the definition of "local" gets very tricky there, and
we'll have to think about what it means. So I'd actually prefer to punt
on doing anything too clever at this point.
> If the caller cares about the distinction, they would need
> to write out this loop as above themselves.
> If they don't care, we could migrate them to not
> use this function, so we can get rid of it?
Yes, I do think in the long run we'd want to get rid of most calls to
get_object_directory(). Not only because it uses the_repository, but
because most callers should be asking for a specific action: I want to
write an object, or I want to read an object.
-Peff
Re: [PATCH 6/9] sha1-file: use an object_directory for the main object dir
On Mon, Nov 12, 2018 at 8:09 AM Jeff King wrote:
>
> On Mon, Nov 12, 2018 at 10:48:36AM -0500, Derrick Stolee wrote:
>
> > > 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?
>
> 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:
s/git/get/ ;-) get_object_directory is very old and was introduced in
e1b10391ea (Use git config file for committer name and email info,
2005-10-11) by Linus.
I would argue that we might want to get rid of that function now,
actually as it doesn't seem to add value to the code (assuming the
BUG never triggers), and using a_repo->objects->objectdir
or after this series a_repo->objects->odb->path; is just as short.
$ git grep get_object_directory |wc -l
30
$ git grep -- "->objects->objectdir" |wc -l
10
Ah well, we're not there yet.
> 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.
It depends if the caller cares about the local flag.
I'd think we can have more than one local, eventually?
Just think of the partial clone stuff that may have a local
set of promised stuff and another set of actual objects,
which may be stored in different local odbs.
If the caller cares about the distinction, they would need
to write out this loop as above themselves.
If they don't care, we could migrate them to not
use this function, so we can get rid of it?
> > > - 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.
>
> Yes, though I don't mind this spot in particular, as the check is pretty
> straight-forward.
>
> I think an example that would benefit more is the check_and_freshen()
> stuff. There we have two almost-the-same wrappers, one of which operates
> on just the first element of the list, and the other of which operates
> on all of the elements after the first.
>
> It could become:
>
> static int check_and_freshen_odb(struct object_directory *odb_list,
>const struct object_id *oid,
>int freshen,
>int local)
> {
> struct object_directory *odb;
>
> for (odb = odb_list; odb; odb = odb->next) {
> static struct strbuf path = STRBUF_INIT;
>
> if (odb->local != local)
> continue;
>
> odb_loose_path(odb, , oid->hash);
> return check_and_freshen_file(path.buf, freshen);
> }
> }
>
> int check_and_freshen_local(const struct object_id *oid, int freshen)
> {
> return check_and_freshen_odb(the_repository->objects->odb, oid,
> freshen, 1);
> }
>
> int check_and_freshen_nonlocal(const struct object_id *oid, int freshen)
> {
> return check_and_freshen_odb(the_repository->objects->odb, oid,
> freshen, 0);
> }
>
I am fine with (a maybe better documented) "first is local" rule, but
the code above looks intriguing, except a little wasteful
(we need two full loops in check_and_freshen, but ideally we
can do by just one loop).
What does the local flag mean anyway in a world where we
have many odbs in a repository, that are not distinguishable
except by their order? AFAICT it is actually to be used for differentiating
how much we care in fsck/cat-file/packing, as it may be borrowed
from an alternate, so maybe the flag is rather to be named
after
Re: [PATCH 6/9] sha1-file: use an object_directory for the main object dir
On Mon, Nov 12, 2018 at 7:48 AM Derrick Stolee wrote: > [... lots of quoted text...] Some email readers are very good at recognizing unchanged quoted text and collapse it, not so at https://public-inbox.org/git/421d3b43-3425-72c9-218e-facd86e28...@gmail.com/ which I use to read through this series. It would help if you'd cut most of the (con)text that is not nearby to your reply, as I read the context email just before your reply. Thanks, Stefan
Re: [PATCH 6/9] sha1-file: use an object_directory for the main object dir
On Mon, Nov 12, 2018 at 10:48:36AM -0500, Derrick Stolee wrote:
> > 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?
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.
> > diff --git a/packfile.c b/packfile.c
> > index d6d511cfd2..1eda33247f 100644
> > --- a/packfile.c
> > +++ b/packfile.c
> > @@ -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);
> > +
> > 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.
Yes, though I don't mind this spot in particular, as the check is pretty
straight-forward.
I think an example that would benefit more is the check_and_freshen()
stuff. There we have two almost-the-same wrappers, one of which operates
on just the first element of the list, and the other of which operates
on all of the elements after the first.
It could become:
static int check_and_freshen_odb(struct object_directory *odb_list,
const struct object_id *oid,
int freshen,
int local)
{
struct object_directory *odb;
for (odb = odb_list; odb; odb = odb->next) {
static struct strbuf path = STRBUF_INIT;
if (odb->local != local)
continue;
odb_loose_path(odb, , oid->hash);
return check_and_freshen_file(path.buf, freshen);
}
}
int check_and_freshen_local(const struct object_id *oid, int freshen)
{
return check_and_freshen_odb(the_repository->objects->odb, oid,
freshen, 1);
}
int check_and_freshen_nonlocal(const struct object_id *oid, int freshen)
{
return check_and_freshen_odb(the_repository->objects->odb, oid,
freshen, 0);
}
I'm not sure that is a big improvement over the patch we're replying to,
though.
-Peff
Re: [PATCH 6/9] sha1-file: use an object_directory for the main object dir
On 11/12/2018 9:50 AM, Jeff King wrote:
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
---
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?
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(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 55153cf92a..15338bd178 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -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(_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(_graph_verify);
commit_graph_verify.argv = verify_argv;
[PATCH 6/9] sha1-file: use an object_directory for the main object dir
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
---
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(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 55153cf92a..15338bd178 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -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(_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(_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(_verify))
- errors_found |= ERROR_COMMIT_GRAPH;
-
prepare_alt_odb(the_repository);
- for (odb =
