git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Nov 7, 2019, 12:58 UTC
Message-ID
<nycvar.QRO.7.76.6.1911071349590.46@tvgsbejvaqbjf.bet>
In-Reply-To
<20191106214201.GB8521@sigill.intra.peff.net>
Hi Peff,
On Wed, 6 Nov 2019, Jeff King wrote:
Show 17 quoted lines
> 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);
>
Previous: Jeff KingNext: Jeff King
Message 5 of 6 in “[BUG] "fatal: bad object .alternate" during fetch with alternates”
  1. Johannes SchindelinNov 6, 2019
  2. Jeff KingNov 6, 2019
  3. Jeff KingNov 6, 2019
  4. Jeff KingNov 6, 2019
  5. Johannes SchindelinNov 7, 2019
  6. Jeff KingNov 8, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.