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(, N_("be verbose")),
OPT_BOOL('H', "human-readable", _readable,
 N_("print sizes in human readable format")),
+   OPT_BOOL(0, "list-alternates", _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();

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

[Jeff King]

We recursively expand alternates repositories, so that if A
borrows from B which borrows from C, A can see all objects.

For the root object database, we allow relative paths, so A
can point to B as "../B/objects". However, we currently do
not allow relative paths when recursing, so B must use an
absolute path to reach C.

That is an ancient protection from c2f493a (Transitively
read alternatives, 2006-05-07) that tries to avoid adding
the same alternate through two different paths. But since
5bdf0a8 (sha1_file: normalize alt_odb path before comparing
and storing, 2011-09-07), we use a normalized absolute path
for each alt_odb entry.

So this protection is no longer necessary; we will detect
the duplicate no matter how we got there.  And it's a good
idea to get rid of it, as it creates an unnecessary
complication when setting up recursive alternates (B has to
know that A is going to borrow from it and make sure to use
an absolute path).

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.

Signed-off-by: Jeff King 
---
 sha1_file.c   | 7 +--
 t/t5613-info-alternate.sh | 4 ++--
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index b9c1fa3..9a79c19 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -343,12 +343,7 @@ static void link_alt_odb_entries(const char *alt, int len, 
int sep,
const char *entry = entries.items[i].string;
if (entry[0] == '\0' || entry[0] == '#')
continue;
-   if (!is_absolute_path(entry) && depth) {
-   error("%s: ignoring relative alternate object store %s",
-   relative_base, entry);
-   } else {
-   link_alt_odb_entry(entry, relative_base, depth, 
objdirbuf.buf);
-   }
+   link_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);
}
string_list_clear(, 0);
free(alt_copy);
diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
index 9cd2626..b429707 100755
--- a/t/t5613-info-alternate.sh
+++ b/t/t5613-info-alternate.sh
@@ -102,9 +102,9 @@ test_valid_repo'
 cd "$base_dir"
 
 test_expect_success \
-'that relative alternate is only possible for current dir' '
+'that relative alternate is recursive' '
 cd D &&
-! (test_valid_repo)
+test_valid_repo
 '
 
 cd "$base_dir"
-- 
2.10.0.618.g82cc264