Re: [PATCH 2/6] sha1_file: always allow relative paths to alternates

[Jeff King]

On Sun, Oct 02, 2016 at 11:38:56AM -0400, Jeff King wrote:

> On Sun, Oct 02, 2016 at 11:07:39AM +0200, René Scharfe wrote:
> 
> > diff --git a/builtin/count-objects.c b/builtin/count-objects.c
> > index ba92919..b2afe36 100644
> > --- a/builtin/count-objects.c
> > +++ b/builtin/count-objects.c
> > @@ -73,6 +73,12 @@ static int count_cruft(const char *basename, const char 
> > *path, void *data)
> > return 0;
> >  }
> >  
> > +static int print_alt_odb(struct alternate_object_database *alt, void *data)
> > +{
> > +   puts(alt->base);
> > +   return 0;
> > +}
> 
> This turns out to be wrong, because alt->base _isn't_ just the base;
> it's also the scratch buffer we write into to form pathnames. So if
> we've used the buffer to look up an object, we'll get that object name
> here, not just the base.
> 
> It tends to work for your command because we do nothing except list the
> alternates and exit, but I'm not sure if there are code paths which
> might access an object.
> 
> I think giving a known state to the callback should be the
> responsibility of foreach_alt_odb(), though.

Actually, it looks like there are a lot of opportunities for cleanups in
that area, and maybe some bugfixes (e.g., we should consistently be
using fspathcmp and not memcmp to check for duplicate paths). I started
on a series, but don't have anything to show yet (just FYI in case you
were thinking about doing the same).

-Peff

Re: [PATCH 2/6] sha1_file: always allow relative paths to alternates

[Jeff King]

On Sun, Oct 02, 2016 at 11:07:39AM +0200, René Scharfe wrote:

> diff --git a/builtin/count-objects.c b/builtin/count-objects.c
> index ba92919..b2afe36 100644
> --- a/builtin/count-objects.c
> +++ b/builtin/count-objects.c
> @@ -73,6 +73,12 @@ static int count_cruft(const char *basename, const char 
> *path, void *data)
>   return 0;
>  }
>  
> +static int print_alt_odb(struct alternate_object_database *alt, void *data)
> +{
> + puts(alt->base);
> + return 0;
> +}

This turns out to be wrong, because alt->base _isn't_ just the base;
it's also the scratch buffer we write into to form pathnames. So if
we've used the buffer to look up an object, we'll get that object name
here, not just the base.

It tends to work for your command because we do nothing except list the
alternates and exit, but I'm not sure if there are code paths which
might access an object.

I think giving a known state to the callback should be the
responsibility of foreach_alt_odb(), though.

-Peff

Re: [PATCH 2/6] sha1_file: always allow relative paths to alternates

[Jeff King]

On Sun, Oct 02, 2016 at 11:07:39AM +0200, René Scharfe wrote:

> Am 30.09.2016 um 21:36 schrieb Jeff King:
> > We adjust the test script here to demonstrate that this now
> > works. Unfortunately, we can't demonstrate that the
> > duplicate is suppressed, since it has no user-visible
> > behavior (it's just one less place for our object lookups to
> > go). But you can verify it manually via gdb, with something
> > like:
> > 
> > for i in a b c; do
> > git init --bare $i
> > blob=$(echo $i | git -C $i hash-object -w --stdin)
> > done
> > echo "../../b/objects" >a/objects/info/alternates
> > echo "../../c/objects" >>a/objects/info/alternates
> > echo "../../c/objects" >b/objects/info/alternates
> > gdb --args git cat-file -e $blob
> > 
> > After prepare_alt_odb() runs, we have only a single copy of
> > "/path/to/c/objects/" in the alt_odb list.
> 
> A better way would be to provide a UI for that.  We could easily bolt
> it to the side of count-objects like in the patch below.  Feels a bit
> hackish, though.

Yeah, I thought about something like that, but I'm a bit hesitant to add
public interfaces that exist just for testing. OTOH, "count-objects
--list-alternates" is conceivably a useful debugging tool for "did I set
up my alternates correctly?".

-Peff

Re: [PATCH 2/6] sha1_file: always allow relative paths to alternates

[René Scharfe]

Am 30.09.2016 um 21:36 schrieb Jeff King:
> We adjust the test script here to demonstrate that this now
> works. Unfortunately, we can't demonstrate that the
> duplicate is suppressed, since it has no user-visible
> behavior (it's just one less place for our object lookups to
> go). But you can verify it manually via gdb, with something
> like:
> 
> for i in a b c; do
> git init --bare $i
> blob=$(echo $i | git -C $i hash-object -w --stdin)
> done
> echo "../../b/objects" >a/objects/info/alternates
> echo "../../c/objects" >>a/objects/info/alternates
> echo "../../c/objects" >b/objects/info/alternates
> gdb --args git cat-file -e $blob
> 
> After prepare_alt_odb() runs, we have only a single copy of
> "/path/to/c/objects/" in the alt_odb list.

A better way would be to provide a UI for that.  We could easily bolt
it to the side of count-objects like in the patch below.  Feels a bit
hackish, though.

Per-ODB counting would be nice, but a lot more involved, I guess
(didn't try).


diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index ba92919..b2afe36 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -73,6 +73,12 @@ static int count_cruft(const char *basename, const char 
*path, void *data)
return 0;
 }
 
+static int print_alt_odb(struct alternate_object_database *alt, void *data)
+{
+   puts(alt->base);
+   return 0;
+}
+
 static char const * const count_objects_usage[] = {
N_("git count-objects [-v] [-H | --human-readable]"),
NULL
@@ -81,10 +87,13 @@ static char const * const count_objects_usage[] = {
 int cmd_count_objects(int argc, const char **argv, const char *prefix)
 {
int human_readable = 0;
+   int list_alt_odb = 0;
struct option opts[] = {
OPT__VERBOSE(&verbose, N_("be verbose")),
OPT_BOOL('H', "human-readable", &human_readable,
 N_("print sizes in human readable format")),
+   OPT_BOOL(0, "list-alternates", &list_alt_odb,
+N_("print list of alternate object databases")),
OPT_END(),
};
 
@@ -92,6 +101,12 @@ int cmd_count_objects(int argc, const char **argv, const 
char *prefix)
/* we do not take arguments other than flags for now */
if (argc)
usage_with_options(count_objects_usage, opts);
+
+   if (list_alt_odb) {
+   foreach_alt_odb(print_alt_odb, NULL);
+   return 0;
+   }
+
if (verbose) {
report_garbage = real_report_garbage;
report_linked_checkout_garbage();