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

[Jeff King <peff@peff.net>]

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);