[BUG] "fatal: bad object .alternate" during fetch with alternates

[BUG] "fatal: bad object .alternate" during fetch with alternates

[Johannes Schindelin]

Hi Peff,

I only saw this problem arise on Sunday and wrote it off as a fluke
while fetching in parallel, but it persisted today, under more
controlled circumstances. I wanted to fast-forward a local branch:

$ git fetch origin master:master
fatal: bad object .alternate
error: https://github.com/git-for-windows/git did not send all necessary objects

`git grep`ing around, I saw that you added the code producing that error
message, in 39b44ba771a (check_everything_connected: assume alternate
ref tips are valid, 2019-07-01) and I verified manually through a
combination of debugging and instrumenting that
`add_one_alternate_ref()` is indeed responsible for that `.alternate`.

Now, I think the two factors that trigger this bug over here are:

- I had all the objects locally already, as I had pushed from a topic
  branch to `master` [*1*].

- My worktree's `.git/objects` is connected to an alternate that is
  connected to the current Git repository (yes, it is circular, long
  story...) and has refs pointing to commits its alternate that have
  been gc'ed away.

So I see two problems with this error message:

- It is not helpful. It should not say `.alternate`, it should mention
  the ref itself, and ideally even the path of the alternate.

- Shouldn't the code be made smart enough to simply ignore (maybe with a
  warning) refs that point to gc'ed commits?

Ciao,
Dscho

Footnote *1*: No, I do not usually push directly to `master`. The
scenario here is that I had rebased Git for Windows' patches on top of
v2.24.0, opened a PR for the PR build, everything checked out all right,
and then I wanted to "merge" the PR, _without_ an extra merge commit on
top.

Re: [BUG] "fatal: bad object .alternate" during fetch with alternates

[Jeff King]

On Wed, Nov 06, 2019 at 08:48:05PM +0100, Johannes Schindelin wrote:

> Now, I think the two factors that trigger this bug over here are:
> 
> - I had all the objects locally already, as I had pushed from a topic
>   branch to `master` [*1*].
> 
> - My worktree's `.git/objects` is connected to an alternate that is
>   connected to the current Git repository (yes, it is circular, long
>   story...) and has refs pointing to commits its alternate that have
>   been gc'ed away.

I think this second one is the crux of the issue. Your alternate is a
corrupt repository, and I don't think that's something we ought to be
worried about supporting in general.

That said, those alternate objects should be used as UNINTERESTING
traversal tips. And rev-list usually tries to avoid bailing out for
missing UNINTERESTING objects. I suspect it's less aggressive about
doing so for the actual tips (because usually for_each_ref()'s internal
logic would skip broken refs entirely).

> So I see two problems with this error message:
> 
> - It is not helpful. It should not say `.alternate`, it should mention
>   the ref itself, and ideally even the path of the alternate.

It doesn't know the refname. The data transfer between the alternate and
the borrowing repo was tightened to just pass over object names. We
could probably pass the alternate path, though.

> - Shouldn't the code be made smart enough to simply ignore (maybe with a
>   warning) refs that point to gc'ed commits?

I'll take a look at the rev-list thing I mentioned above.

-Peff

Re: [BUG] "fatal: bad object .alternate" during fetch with alternates

[Jeff King]

On Wed, Nov 06, 2019 at 03:59:07PM -0500, Jeff King wrote:

> On Wed, Nov 06, 2019 at 08:48:05PM +0100, Johannes Schindelin wrote:
> 
> > Now, I think the two factors that trigger this bug over here are:
> > 
> > - I had all the objects locally already, as I had pushed from a topic
> >   branch to `master` [*1*].
> > 
> > - My worktree's `.git/objects` is connected to an alternate that is
> >   connected to the current Git repository (yes, it is circular, long
> >   story...) and has refs pointing to commits its alternate that have
> >   been gc'ed away.
> 
> I think this second one is the crux of the issue. Your alternate is a
> corrupt repository, and I don't think that's something we ought to be
> worried about supporting in general.

Thinking on this a bit more, the whole thing is a bit subtle.

Imagine what would happen in the fetch or push code paths from before my
patches. We'd tell the other side "hey, I have object X" when in fact we
don't. So we'd end up missing some objects from the transfer.

Before my patches, we'd have done a full connectivity check, with no
regard to the alternate, and complained.

After my patches, we make the assumption that the alternate isn't
corrupt, and trust its refs. So there's an opportunity for corruption in
the alternate to spread to the child repository. We're actually saved
somewhat by the current behavior where rev-list bails on the broken
refs, rather than accepting them at face value. But it wouldn't protect
us from deeper corruptions in the alternate.

I have trouble getting too worked up about that, though. If your
alternate is corrupt, this is only one of many ways that the corruption
can spread to your repository.

However, it would make sense to me that if we can cheaply notice a
corruption in the alternate, that we should avoid it spreading. And
noticing that the object pointed to by a ref is missing is reasonably
cheap (in fact, it's done by most ref iteration; for-each-ref explicitly
uses FILTER_REFS_INCLUDE_BROKEN). So I think the right direction is
probably to teach for-each-ref an "--omit-broken" option, and use that
for enumerating the alternate refs.

That would let us not only notice this corruption, but we'd "route
around" it by avoiding advertising the broken alternate tip in the first
place.

And it would fix your problem, too. In your case it sounds like you're
not working with any of the corrupted objects at all; rather it's just
an unrelated corruption that's causing rev-list to bail.

-Peff

Re: [BUG] "fatal: bad object .alternate" during fetch with alternates

[Jeff King]

On Wed, Nov 06, 2019 at 03:59:07PM -0500, Jeff King wrote:

> > So I see two problems with this error message:
> > 
> > - It is not helpful. It should not say `.alternate`, it should mention
> >   the ref itself, and ideally even the path of the alternate.
> 
> It doesn't know the refname. The data transfer between the alternate and
> the borrowing repo was tightened to just pass over object names. We
> could probably pass the alternate path, though.

The patch to do that is pretty simple (there's a little collateral
damage from having to update the callback signature). I've included it
at the end of this message.

But I feel a little iffy stuffing arbitrary strings into what's usually
a refname (or a syntactically invalid pseudo-refname like ".alternate").

E.g., here's the output of t5618 with this patch:

  expecting success of 5618.6 'log --source shows .alternate marker': 
  	git log --oneline --source --remotes=origin >expect.orig &&
  	sed "s/origin.* /.alternate /" <expect.orig >expect &&
  	git log --oneline --source --alternate-refs >actual &&
  	test_cmp expect actual
  
  --- expect	2019-11-06 21:37:06.435427982 +0000
  +++ actual	2019-11-06 21:37:06.439427949 +0000
  @@ -1,3 +1,3 @@
  -e9067a7	.alternate two
  -eae7140	.alternate one
  -00f38a4	.alternate base
  +e9067a7	.alternate from /home/peff/compile/git/t/trash directory.t5618-alternate-refs/.git two
  +eae7140	.alternate from /home/peff/compile/git/t/trash directory.t5618-alternate-refs/.git one
  +00f38a4	.alternate from /home/peff/compile/git/t/trash directory.t5618-alternate-refs/.git base
  not ok 6 - log --source shows .alternate marker


I wonder if a better approach would be to improve the "bad object"
message. It prints the "name", but never even mentions the bogus oid it
found! So with this much simpler change:

  diff --git a/revision.c b/revision.c
  index 309cd31488..4e55a9248c 100644
  --- a/revision.c
  +++ b/revision.c
  @@ -380,7 +380,7 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
                          return object;
                  if (revs->exclude_promisor_objects && is_promisor_object(oid))
                          return NULL;
  -               die("bad object %s", name);
  +               die(_("bad object %s (from %s)"), oid_to_hex(oid), name);
          }
          object->flags |= flags;
          return object;

You'd get something like:

  fatal: bad object: 1234abcd... (from refs/heads/master)

or

  fatal: bad object: 1234abcd... (from .alternate)

which seems strictly better in the first case, and passably less
confusing in the second.

-Peff

---
Here's the "name objects after their alternate path" patch:

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 411e0b4d99..300249b44a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -280,7 +280,8 @@ static int show_ref_cb(const char *path_full, const struct object_id *oid,
 	return 0;
 }
 
-static void show_one_alternate_ref(const struct object_id *oid,
+static void show_one_alternate_ref(const char *altpath,
+				   const struct object_id *oid,
 				   void *data)
 {
 	struct oidset *seen = data;
diff --git a/fetch-pack.c b/fetch-pack.c
index 0130b44112..553741341b 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -75,7 +75,8 @@ struct alternate_object_cache {
 	size_t nr, alloc;
 };
 
-static void cache_one_alternate(const struct object_id *oid,
+static void cache_one_alternate(const char *altpath,
+				const struct object_id *oid,
 				void *vcache)
 {
 	struct alternate_object_cache *cache = vcache;
diff --git a/object-store.h b/object-store.h
index 7f7b3cdd80..0066a14f66 100644
--- a/object-store.h
+++ b/object-store.h
@@ -33,7 +33,7 @@ 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*);
-typedef void alternate_ref_fn(const struct object_id *oid, void *);
+typedef void alternate_ref_fn(const char *altpath, const struct object_id *oid, void *);
 void for_each_alternate_ref(alternate_ref_fn, void *);
 
 /*
diff --git a/revision.c b/revision.c
index 0e39b2b8a5..077a47f2e6 100644
--- a/revision.c
+++ b/revision.c
@@ -1560,27 +1560,34 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 struct add_alternate_refs_data {
 	struct rev_info *revs;
 	unsigned int flags;
+	struct strbuf *name;
 };
 
-static void add_one_alternate_ref(const struct object_id *oid,
+static void add_one_alternate_ref(const char *altpath,
+				  const struct object_id *oid,
 				  void *vdata)
 {
-	const char *name = ".alternate";
 	struct add_alternate_refs_data *data = vdata;
 	struct object *obj;
 
-	obj = get_reference(data->revs, name, oid, data->flags);
-	add_rev_cmdline(data->revs, obj, name, REV_CMD_REV, data->flags);
-	add_pending_object(data->revs, obj, name);
+	strbuf_reset(data->name);
+	strbuf_addf(data->name, ".alternate from %s", altpath);
+
+	obj = get_reference(data->revs, data->name->buf, oid, data->flags);
+	add_rev_cmdline(data->revs, obj, data->name->buf, REV_CMD_REV, data->flags);
+	add_pending_object(data->revs, obj, data->name->buf);
 }
 
 static void add_alternate_refs_to_pending(struct rev_info *revs,
 					  unsigned int flags)
 {
+	struct strbuf name = STRBUF_INIT;
 	struct add_alternate_refs_data data;
 	data.revs = revs;
 	data.flags = flags;
+	data.name = &name;
 	for_each_alternate_ref(add_one_alternate_ref, &data);
+	strbuf_release(&name);
 }
 
 static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
diff --git a/sha1-file.c b/sha1-file.c
index 188de57634..e1709fdf30 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -793,7 +793,7 @@ static void read_alternate_refs(const char *path,
 			break;
 		}
 
-		cb(&oid, data);
+		cb(path, &oid, data);
 	}
 
 	fclose(fh);

Re: [BUG] "fatal: bad object .alternate" during fetch with alternates

[Johannes Schindelin]

Hi Peff,

On Wed, 6 Nov 2019, Jeff King wrote:

> On Wed, Nov 06, 2019 at 03:59:07PM -0500, Jeff King wrote:
>
> > > So I see two problems with this error message:
> > >
> > > - It is not helpful. It should not say `.alternate`, it should mention
> > >   the ref itself, and ideally even the path of the alternate.
> >
> > It doesn't know the refname. The data transfer between the alternate and
> > the borrowing repo was tightened to just pass over object names. We
> > could probably pass the alternate path, though.
>
> The patch to do that is pretty simple (there's a little collateral
> damage from having to update the callback signature). I've included it
> at the end of this message.
>
> But I feel a little iffy stuffing arbitrary strings into what's usually
> a refname (or a syntactically invalid pseudo-refname like ".alternate").

Hmm. I hoped that it would be simpler than this, indeed.

So what about the idea of ignoring (with a warning) them instead,
without bothering to try saying much about the alternate itself? I.e.
something like this patch (which is admittedly a bit verbose because it
_also_ has to update a signature):

-- snip --
diff --git a/revision.c b/revision.c
index 0e39b2b8a59..0170ae16166 100644
--- a/revision.c
+++ b/revision.c
@@ -357,7 +357,7 @@ void add_head_to_pending(struct rev_info *revs)

 static struct object *get_reference(struct rev_info *revs, const char *name,
 				    const struct object_id *oid,
-				    unsigned int flags)
+				    unsigned int flags, int ignore_missing)
 {
 	struct object *object;

@@ -376,7 +376,7 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
 	}

 	if (!object) {
-		if (revs->ignore_missing)
+		if (revs->ignore_missing || ignore_missing)
 			return object;
 		if (revs->exclude_promisor_objects && is_promisor_object(oid))
 			return NULL;
@@ -389,7 +389,7 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
 void add_pending_oid(struct rev_info *revs, const char *name,
 		      const struct object_id *oid, unsigned int flags)
 {
-	struct object *object = get_reference(revs, name, oid, flags);
+	struct object *object = get_reference(revs, name, oid, flags, 0);
 	add_pending_object(revs, object, name);
 }

@@ -1356,7 +1356,7 @@ static int handle_one_ref(const char *path, const struct object_id *oid,
 	if (ref_excluded(cb->all_revs->ref_excludes, path))
 	    return 0;

-	object = get_reference(cb->all_revs, path, oid, cb->all_flags);
+	object = get_reference(cb->all_revs, path, oid, cb->all_flags, 0);
 	add_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, cb->all_flags);
 	add_pending_oid(cb->all_revs, path, oid, cb->all_flags);
 	return 0;
@@ -1569,7 +1569,11 @@ static void add_one_alternate_ref(const struct object_id *oid,
 	struct add_alternate_refs_data *data = vdata;
 	struct object *obj;

-	obj = get_reference(data->revs, name, oid, data->flags);
+	if (!(obj = get_reference(data->revs, name, oid, data->flags, 1))) {
+		warning("ignoring stale alternate reference to '%s'",
+			oid_to_hex(oid));
+		return;
+	}
 	add_rev_cmdline(data->revs, obj, name, REV_CMD_REV, data->flags);
 	add_pending_object(data->revs, obj, name);
 }
@@ -1600,7 +1604,7 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
 	if (get_oid_committish(arg, &oid))
 		return 0;
 	while (1) {
-		it = get_reference(revs, arg, &oid, 0);
+		it = get_reference(revs, arg, &oid, 0, 0);
 		if (!it && revs->ignore_missing)
 			return 0;
 		if (it->type != OBJ_TAG)
@@ -1905,7 +1909,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
 		return revs->ignore_missing ? 0 : -1;
 	if (!cant_be_filename)
 		verify_non_filename(revs->prefix, arg);
-	object = get_reference(revs, arg, &oid, flags ^ local_flags);
+	object = get_reference(revs, arg, &oid, flags ^ local_flags, 0);
 	if (!object)
 		return revs->ignore_missing ? 0 : -1;
 	add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);
@@ -2647,7 +2651,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		struct object_context oc;
 		if (get_oid_with_context(revs->repo, revs->def, 0, &oid, &oc))
 			diagnose_missing_default(revs->def);
-		object = get_reference(revs, revs->def, &oid, 0);
+		object = get_reference(revs, revs->def, &oid, 0, 0);
 		add_pending_object_with_mode(revs, object, revs->def, oc.mode);
 	}
-- snap --

What do you think?

Ciao,
Dscho

>
> E.g., here's the output of t5618 with this patch:
>
>   expecting success of 5618.6 'log --source shows .alternate marker':
>   	git log --oneline --source --remotes=origin >expect.orig &&
>   	sed "s/origin.* /.alternate /" <expect.orig >expect &&
>   	git log --oneline --source --alternate-refs >actual &&
>   	test_cmp expect actual
>
>   --- expect	2019-11-06 21:37:06.435427982 +0000
>   +++ actual	2019-11-06 21:37:06.439427949 +0000
>   @@ -1,3 +1,3 @@
>   -e9067a7	.alternate two
>   -eae7140	.alternate one
>   -00f38a4	.alternate base
>   +e9067a7	.alternate from /home/peff/compile/git/t/trash directory.t5618-alternate-refs/.git two
>   +eae7140	.alternate from /home/peff/compile/git/t/trash directory.t5618-alternate-refs/.git one
>   +00f38a4	.alternate from /home/peff/compile/git/t/trash directory.t5618-alternate-refs/.git base
>   not ok 6 - log --source shows .alternate marker
>
>
> I wonder if a better approach would be to improve the "bad object"
> message. It prints the "name", but never even mentions the bogus oid it
> found! So with this much simpler change:
>
>   diff --git a/revision.c b/revision.c
>   index 309cd31488..4e55a9248c 100644
>   --- a/revision.c
>   +++ b/revision.c
>   @@ -380,7 +380,7 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
>                           return object;
>                   if (revs->exclude_promisor_objects && is_promisor_object(oid))
>                           return NULL;
>   -               die("bad object %s", name);
>   +               die(_("bad object %s (from %s)"), oid_to_hex(oid), name);
>           }
>           object->flags |= flags;
>           return object;
>
> You'd get something like:
>
>   fatal: bad object: 1234abcd... (from refs/heads/master)
>
> or
>
>   fatal: bad object: 1234abcd... (from .alternate)
>
> which seems strictly better in the first case, and passably less
> confusing in the second.
>
> -Peff
>
> ---
> Here's the "name objects after their alternate path" patch:
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 411e0b4d99..300249b44a 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -280,7 +280,8 @@ static int show_ref_cb(const char *path_full, const struct object_id *oid,
>  	return 0;
>  }
>
> -static void show_one_alternate_ref(const struct object_id *oid,
> +static void show_one_alternate_ref(const char *altpath,
> +				   const struct object_id *oid,
>  				   void *data)
>  {
>  	struct oidset *seen = data;
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 0130b44112..553741341b 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -75,7 +75,8 @@ struct alternate_object_cache {
>  	size_t nr, alloc;
>  };
>
> -static void cache_one_alternate(const struct object_id *oid,
> +static void cache_one_alternate(const char *altpath,
> +				const struct object_id *oid,
>  				void *vcache)
>  {
>  	struct alternate_object_cache *cache = vcache;
> diff --git a/object-store.h b/object-store.h
> index 7f7b3cdd80..0066a14f66 100644
> --- a/object-store.h
> +++ b/object-store.h
> @@ -33,7 +33,7 @@ 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*);
> -typedef void alternate_ref_fn(const struct object_id *oid, void *);
> +typedef void alternate_ref_fn(const char *altpath, const struct object_id *oid, void *);
>  void for_each_alternate_ref(alternate_ref_fn, void *);
>
>  /*
> diff --git a/revision.c b/revision.c
> index 0e39b2b8a5..077a47f2e6 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1560,27 +1560,34 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
>  struct add_alternate_refs_data {
>  	struct rev_info *revs;
>  	unsigned int flags;
> +	struct strbuf *name;
>  };
>
> -static void add_one_alternate_ref(const struct object_id *oid,
> +static void add_one_alternate_ref(const char *altpath,
> +				  const struct object_id *oid,
>  				  void *vdata)
>  {
> -	const char *name = ".alternate";
>  	struct add_alternate_refs_data *data = vdata;
>  	struct object *obj;
>
> -	obj = get_reference(data->revs, name, oid, data->flags);
> -	add_rev_cmdline(data->revs, obj, name, REV_CMD_REV, data->flags);
> -	add_pending_object(data->revs, obj, name);
> +	strbuf_reset(data->name);
> +	strbuf_addf(data->name, ".alternate from %s", altpath);
> +
> +	obj = get_reference(data->revs, data->name->buf, oid, data->flags);
> +	add_rev_cmdline(data->revs, obj, data->name->buf, REV_CMD_REV, data->flags);
> +	add_pending_object(data->revs, obj, data->name->buf);
>  }
>
>  static void add_alternate_refs_to_pending(struct rev_info *revs,
>  					  unsigned int flags)
>  {
> +	struct strbuf name = STRBUF_INIT;
>  	struct add_alternate_refs_data data;
>  	data.revs = revs;
>  	data.flags = flags;
> +	data.name = &name;
>  	for_each_alternate_ref(add_one_alternate_ref, &data);
> +	strbuf_release(&name);
>  }
>
>  static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
> diff --git a/sha1-file.c b/sha1-file.c
> index 188de57634..e1709fdf30 100644
> --- a/sha1-file.c
> +++ b/sha1-file.c
> @@ -793,7 +793,7 @@ static void read_alternate_refs(const char *path,
>  			break;
>  		}
>
> -		cb(&oid, data);
> +		cb(path, &oid, data);
>  	}
>
>  	fclose(fh);
>

Re: [BUG] "fatal: bad object .alternate" during fetch with alternates

[Jeff King]

On Thu, Nov 07, 2019 at 01:58:04PM +0100, Johannes Schindelin wrote:

> So what about the idea of ignoring (with a warning) them instead,
> without bothering to try saying much about the alternate itself? I.e.
> something like this patch (which is admittedly a bit verbose because it
> _also_ has to update a signature):
>
> [...]
> @@ -1569,7 +1569,11 @@ static void add_one_alternate_ref(const struct object_id *oid,
>  	struct add_alternate_refs_data *data = vdata;
>  	struct object *obj;
> 
> -	obj = get_reference(data->revs, name, oid, data->flags);
> +	if (!(obj = get_reference(data->revs, name, oid, data->flags, 1))) {
> +		warning("ignoring stale alternate reference to '%s'",
> +			oid_to_hex(oid));
> +		return;
> +	}
>  	add_rev_cmdline(data->revs, obj, name, REV_CMD_REV, data->flags);
>  	add_pending_object(data->revs, obj, name);
>  }

I don't think it makes sense to tie this to "--alternate-refs" in this
way. I think there are two ways to look at resolving this:

  - for the UNINTERESTING side of a traversal, we might be willing to
    ignore a missing object. But then it would apply equally to
    non-alternates, too. (And we do this already, but not for ref tips,
    since those aren't just "stale" but rather indicative of a
    corruption).

  - any alternate-ref borrowing is an optimization and thus best-effort.
    So we should treat them specially, whether UNINTERESTING or not. But
    then I think the right place to do that is not inside rev-list, but
    in for_each_alternate_ref(), or even in the for-each-ref run inside
    the alternate repository. Then it would help rev-list, but also
    avoid advertising the bogus object across a push/fetch.

-Peff