{"thread":{"id":"52205","subject":"[BUG] \"fatal: bad object .alternate\" during fetch with alternates","startedAt":"2019-11-06T19:48:31Z","lastAt":"2019-11-08T08:54:16Z","messageCount":6,"participants":["Johannes Schindelin","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"385621","messageId":"nycvar.QRO.7.76.6.1911041053190.46@tvgsbejvaqbjf.bet","threadId":"52205","inReplyTo":null,"subject":"[BUG] \"fatal: bad object .alternate\" during fetch with alternates","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-06T19:48:05Z","receivedAt":"2019-11-06T19:48:31Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nI only saw this problem arise on Sunday and wrote it off as a fluke\nwhile fetching in parallel, but it persisted today, under more\ncontrolled circumstances. I wanted to fast-forward a local branch:\n\n$ git fetch origin master:master\nfatal: bad object .alternate\nerror: https://github.com/git-for-windows/git did not send all necessary objects\n\n`git grep`ing around, I saw that you added the code producing that error\nmessage, in 39b44ba771a (check_everything_connected: assume alternate\nref tips are valid, 2019-07-01) and I verified manually through a\ncombination of debugging and instrumenting that\n`add_one_alternate_ref()` is indeed responsible for that `.alternate`.\n\nNow, I think the two factors that trigger this bug over here are:\n\n- I had all the objects locally already, as I had pushed from a topic\n  branch to `master` [*1*].\n\n- My worktree's `.git/objects` is connected to an alternate that is\n  connected to the current Git repository (yes, it is circular, long\n  story...) and has refs pointing to commits its alternate that have\n  been gc'ed away.\n\nSo I see two problems with this error message:\n\n- It is not helpful. It should not say `.alternate`, it should mention\n  the ref itself, and ideally even the path of the alternate.\n\n- Shouldn't the code be made smart enough to simply ignore (maybe with a\n  warning) refs that point to gc'ed commits?\n\nCiao,\nDscho\n\nFootnote *1*: No, I do not usually push directly to `master`. The\nscenario here is that I had rebased Git for Windows' patches on top of\nv2.24.0, opened a PR for the PR build, everything checked out all right,\nand then I wanted to \"merge\" the PR, _without_ an extra merge commit on\ntop.\n"},{"id":"385623","messageId":"20191106205907.GA4122@sigill.intra.peff.net","threadId":"52205","inReplyTo":"nycvar.QRO.7.76.6.1911041053190.46@tvgsbejvaqbjf.bet","subject":"Re: [BUG] \"fatal: bad object .alternate\" during fetch with alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-06T20:59:07Z","receivedAt":"2019-11-06T20:59:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 06, 2019 at 08:48:05PM +0100, Johannes Schindelin wrote:\n\n> Now, I think the two factors that trigger this bug over here are:\n> \n> - I had all the objects locally already, as I had pushed from a topic\n>   branch to `master` [*1*].\n> \n> - My worktree's `.git/objects` is connected to an alternate that is\n>   connected to the current Git repository (yes, it is circular, long\n>   story...) and has refs pointing to commits its alternate that have\n>   been gc'ed away.\n\nI think this second one is the crux of the issue. Your alternate is a\ncorrupt repository, and I don't think that's something we ought to be\nworried about supporting in general.\n\nThat said, those alternate objects should be used as UNINTERESTING\ntraversal tips. And rev-list usually tries to avoid bailing out for\nmissing UNINTERESTING objects. I suspect it's less aggressive about\ndoing so for the actual tips (because usually for_each_ref()'s internal\nlogic would skip broken refs entirely).\n\n> So I see two problems with this error message:\n> \n> - It is not helpful. It should not say `.alternate`, it should mention\n>   the ref itself, and ideally even the path of the alternate.\n\nIt doesn't know the refname. The data transfer between the alternate and\nthe borrowing repo was tightened to just pass over object names. We\ncould probably pass the alternate path, though.\n\n> - Shouldn't the code be made smart enough to simply ignore (maybe with a\n>   warning) refs that point to gc'ed commits?\n\nI'll take a look at the rev-list thing I mentioned above.\n\n-Peff\n"},{"id":"385624","messageId":"20191106212559.GA8521@sigill.intra.peff.net","threadId":"52205","inReplyTo":"20191106205907.GA4122@sigill.intra.peff.net","subject":"Re: [BUG] \"fatal: bad object .alternate\" during fetch with alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-06T21:25:59Z","receivedAt":"2019-11-06T21:26:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 06, 2019 at 03:59:07PM -0500, Jeff King wrote:\n\n> On Wed, Nov 06, 2019 at 08:48:05PM +0100, Johannes Schindelin wrote:\n> \n> > Now, I think the two factors that trigger this bug over here are:\n> > \n> > - I had all the objects locally already, as I had pushed from a topic\n> >   branch to `master` [*1*].\n> > \n> > - My worktree's `.git/objects` is connected to an alternate that is\n> >   connected to the current Git repository (yes, it is circular, long\n> >   story...) and has refs pointing to commits its alternate that have\n> >   been gc'ed away.\n> \n> I think this second one is the crux of the issue. Your alternate is a\n> corrupt repository, and I don't think that's something we ought to be\n> worried about supporting in general.\n\nThinking on this a bit more, the whole thing is a bit subtle.\n\nImagine what would happen in the fetch or push code paths from before my\npatches. We'd tell the other side \"hey, I have object X\" when in fact we\ndon't. So we'd end up missing some objects from the transfer.\n\nBefore my patches, we'd have done a full connectivity check, with no\nregard to the alternate, and complained.\n\nAfter my patches, we make the assumption that the alternate isn't\ncorrupt, and trust its refs. So there's an opportunity for corruption in\nthe alternate to spread to the child repository. We're actually saved\nsomewhat by the current behavior where rev-list bails on the broken\nrefs, rather than accepting them at face value. But it wouldn't protect\nus from deeper corruptions in the alternate.\n\nI have trouble getting too worked up about that, though. If your\nalternate is corrupt, this is only one of many ways that the corruption\ncan spread to your repository.\n\nHowever, it would make sense to me that if we can cheaply notice a\ncorruption in the alternate, that we should avoid it spreading. And\nnoticing that the object pointed to by a ref is missing is reasonably\ncheap (in fact, it's done by most ref iteration; for-each-ref explicitly\nuses FILTER_REFS_INCLUDE_BROKEN). So I think the right direction is\nprobably to teach for-each-ref an \"--omit-broken\" option, and use that\nfor enumerating the alternate refs.\n\nThat would let us not only notice this corruption, but we'd \"route\naround\" it by avoiding advertising the broken alternate tip in the first\nplace.\n\nAnd it would fix your problem, too. In your case it sounds like you're\nnot working with any of the corrupted objects at all; rather it's just\nan unrelated corruption that's causing rev-list to bail.\n\n-Peff\n"},{"id":"385626","messageId":"20191106214201.GB8521@sigill.intra.peff.net","threadId":"52205","inReplyTo":"20191106205907.GA4122@sigill.intra.peff.net","subject":"Re: [BUG] \"fatal: bad object .alternate\" during fetch with alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-06T21:42:01Z","receivedAt":"2019-11-06T21:42:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 06, 2019 at 03:59:07PM -0500, Jeff King wrote:\n\n> > So I see two problems with this error message:\n> > \n> > - It is not helpful. It should not say `.alternate`, it should mention\n> >   the ref itself, and ideally even the path of the alternate.\n> \n> It doesn't know the refname. The data transfer between the alternate and\n> the borrowing repo was tightened to just pass over object names. We\n> could probably pass the alternate path, though.\n\nThe patch to do that is pretty simple (there's a little collateral\ndamage from having to update the callback signature). I've included it\nat the end of this message.\n\nBut I feel a little iffy stuffing arbitrary strings into what's usually\na refname (or a syntactically invalid pseudo-refname like \".alternate\").\n\nE.g., here's the output of t5618 with this patch:\n\n  expecting success of 5618.6 'log --source shows .alternate marker': \n  \tgit log --oneline --source --remotes=origin >expect.orig &&\n  \tsed \"s/origin.* /.alternate /\" <expect.orig >expect &&\n  \tgit log --oneline --source --alternate-refs >actual &&\n  \ttest_cmp expect actual\n  \n  --- expect\t2019-11-06 21:37:06.435427982 +0000\n  +++ actual\t2019-11-06 21:37:06.439427949 +0000\n  @@ -1,3 +1,3 @@\n  -e9067a7\t.alternate two\n  -eae7140\t.alternate one\n  -00f38a4\t.alternate base\n  +e9067a7\t.alternate from /home/peff/compile/git/t/trash directory.t5618-alternate-refs/.git two\n  +eae7140\t.alternate from /home/peff/compile/git/t/trash directory.t5618-alternate-refs/.git one\n  +00f38a4\t.alternate from /home/peff/compile/git/t/trash directory.t5618-alternate-refs/.git base\n  not ok 6 - log --source shows .alternate marker\n\n\nI wonder if a better approach would be to improve the \"bad object\"\nmessage. It prints the \"name\", but never even mentions the bogus oid it\nfound! So with this much simpler change:\n\n  diff --git a/revision.c b/revision.c\n  index 309cd31488..4e55a9248c 100644\n  --- a/revision.c\n  +++ b/revision.c\n  @@ -380,7 +380,7 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n                          return object;\n                  if (revs->exclude_promisor_objects && is_promisor_object(oid))\n                          return NULL;\n  -               die(\"bad object %s\", name);\n  +               die(_(\"bad object %s (from %s)\"), oid_to_hex(oid), name);\n          }\n          object->flags |= flags;\n          return object;\n\nYou'd get something like:\n\n  fatal: bad object: 1234abcd... (from refs/heads/master)\n\nor\n\n  fatal: bad object: 1234abcd... (from .alternate)\n\nwhich seems strictly better in the first case, and passably less\nconfusing in the second.\n\n-Peff\n\n---\nHere's the \"name objects after their alternate path\" patch:\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 411e0b4d99..300249b44a 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -280,7 +280,8 @@ static int show_ref_cb(const char *path_full, const struct object_id *oid,\n \treturn 0;\n }\n \n-static void show_one_alternate_ref(const struct object_id *oid,\n+static void show_one_alternate_ref(const char *altpath,\n+\t\t\t\t   const struct object_id *oid,\n \t\t\t\t   void *data)\n {\n \tstruct oidset *seen = data;\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 0130b44112..553741341b 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -75,7 +75,8 @@ struct alternate_object_cache {\n \tsize_t nr, alloc;\n };\n \n-static void cache_one_alternate(const struct object_id *oid,\n+static void cache_one_alternate(const char *altpath,\n+\t\t\t\tconst struct object_id *oid,\n \t\t\t\tvoid *vcache)\n {\n \tstruct alternate_object_cache *cache = vcache;\ndiff --git a/object-store.h b/object-store.h\nindex 7f7b3cdd80..0066a14f66 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -33,7 +33,7 @@ void prepare_alt_odb(struct repository *r);\n char *compute_alternate_path(const char *path, struct strbuf *err);\n typedef int alt_odb_fn(struct object_directory *, void *);\n int foreach_alt_odb(alt_odb_fn, void*);\n-typedef void alternate_ref_fn(const struct object_id *oid, void *);\n+typedef void alternate_ref_fn(const char *altpath, const struct object_id *oid, void *);\n void for_each_alternate_ref(alternate_ref_fn, void *);\n \n /*\ndiff --git a/revision.c b/revision.c\nindex 0e39b2b8a5..077a47f2e6 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1560,27 +1560,34 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n struct add_alternate_refs_data {\n \tstruct rev_info *revs;\n \tunsigned int flags;\n+\tstruct strbuf *name;\n };\n \n-static void add_one_alternate_ref(const struct object_id *oid,\n+static void add_one_alternate_ref(const char *altpath,\n+\t\t\t\t  const struct object_id *oid,\n \t\t\t\t  void *vdata)\n {\n-\tconst char *name = \".alternate\";\n \tstruct add_alternate_refs_data *data = vdata;\n \tstruct object *obj;\n \n-\tobj = get_reference(data->revs, name, oid, data->flags);\n-\tadd_rev_cmdline(data->revs, obj, name, REV_CMD_REV, data->flags);\n-\tadd_pending_object(data->revs, obj, name);\n+\tstrbuf_reset(data->name);\n+\tstrbuf_addf(data->name, \".alternate from %s\", altpath);\n+\n+\tobj = get_reference(data->revs, data->name->buf, oid, data->flags);\n+\tadd_rev_cmdline(data->revs, obj, data->name->buf, REV_CMD_REV, data->flags);\n+\tadd_pending_object(data->revs, obj, data->name->buf);\n }\n \n static void add_alternate_refs_to_pending(struct rev_info *revs,\n \t\t\t\t\t  unsigned int flags)\n {\n+\tstruct strbuf name = STRBUF_INIT;\n \tstruct add_alternate_refs_data data;\n \tdata.revs = revs;\n \tdata.flags = flags;\n+\tdata.name = &name;\n \tfor_each_alternate_ref(add_one_alternate_ref, &data);\n+\tstrbuf_release(&name);\n }\n \n static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 188de57634..e1709fdf30 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -793,7 +793,7 @@ static void read_alternate_refs(const char *path,\n \t\t\tbreak;\n \t\t}\n \n-\t\tcb(&oid, data);\n+\t\tcb(path, &oid, data);\n \t}\n \n \tfclose(fh);\n"},{"id":"385704","messageId":"nycvar.QRO.7.76.6.1911071349590.46@tvgsbejvaqbjf.bet","threadId":"52205","inReplyTo":"20191106214201.GB8521@sigill.intra.peff.net","subject":"Re: [BUG] \"fatal: bad object .alternate\" during fetch with alternates","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-07T12:58:04Z","receivedAt":"2019-11-07T12:58:27Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 6 Nov 2019, Jeff King wrote:\n\n> On Wed, Nov 06, 2019 at 03:59:07PM -0500, Jeff King wrote:\n>\n> > > So I see two problems with this error message:\n> > >\n> > > - It is not helpful. It should not say `.alternate`, it should mention\n> > >   the ref itself, and ideally even the path of the alternate.\n> >\n> > It doesn't know the refname. The data transfer between the alternate and\n> > the borrowing repo was tightened to just pass over object names. We\n> > could probably pass the alternate path, though.\n>\n> The patch to do that is pretty simple (there's a little collateral\n> damage from having to update the callback signature). I've included it\n> at the end of this message.\n>\n> But I feel a little iffy stuffing arbitrary strings into what's usually\n> a refname (or a syntactically invalid pseudo-refname like \".alternate\").\n\nHmm. I hoped that it would be simpler than this, indeed.\n\nSo what about the idea of ignoring (with a warning) them instead,\nwithout bothering to try saying much about the alternate itself? I.e.\nsomething like this patch (which is admittedly a bit verbose because it\n_also_ has to update a signature):\n\n-- snip --\ndiff --git a/revision.c b/revision.c\nindex 0e39b2b8a59..0170ae16166 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -357,7 +357,7 @@ void add_head_to_pending(struct rev_info *revs)\n\n static struct object *get_reference(struct rev_info *revs, const char *name,\n \t\t\t\t    const struct object_id *oid,\n-\t\t\t\t    unsigned int flags)\n+\t\t\t\t    unsigned int flags, int ignore_missing)\n {\n \tstruct object *object;\n\n@@ -376,7 +376,7 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n \t}\n\n \tif (!object) {\n-\t\tif (revs->ignore_missing)\n+\t\tif (revs->ignore_missing || ignore_missing)\n \t\t\treturn object;\n \t\tif (revs->exclude_promisor_objects && is_promisor_object(oid))\n \t\t\treturn NULL;\n@@ -389,7 +389,7 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n void add_pending_oid(struct rev_info *revs, const char *name,\n \t\t      const struct object_id *oid, unsigned int flags)\n {\n-\tstruct object *object = get_reference(revs, name, oid, flags);\n+\tstruct object *object = get_reference(revs, name, oid, flags, 0);\n \tadd_pending_object(revs, object, name);\n }\n\n@@ -1356,7 +1356,7 @@ static int handle_one_ref(const char *path, const struct object_id *oid,\n \tif (ref_excluded(cb->all_revs->ref_excludes, path))\n \t    return 0;\n\n-\tobject = get_reference(cb->all_revs, path, oid, cb->all_flags);\n+\tobject = get_reference(cb->all_revs, path, oid, cb->all_flags, 0);\n \tadd_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, cb->all_flags);\n \tadd_pending_oid(cb->all_revs, path, oid, cb->all_flags);\n \treturn 0;\n@@ -1569,7 +1569,11 @@ static void add_one_alternate_ref(const struct object_id *oid,\n \tstruct add_alternate_refs_data *data = vdata;\n \tstruct object *obj;\n\n-\tobj = get_reference(data->revs, name, oid, data->flags);\n+\tif (!(obj = get_reference(data->revs, name, oid, data->flags, 1))) {\n+\t\twarning(\"ignoring stale alternate reference to '%s'\",\n+\t\t\toid_to_hex(oid));\n+\t\treturn;\n+\t}\n \tadd_rev_cmdline(data->revs, obj, name, REV_CMD_REV, data->flags);\n \tadd_pending_object(data->revs, obj, name);\n }\n@@ -1600,7 +1604,7 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n \tif (get_oid_committish(arg, &oid))\n \t\treturn 0;\n \twhile (1) {\n-\t\tit = get_reference(revs, arg, &oid, 0);\n+\t\tit = get_reference(revs, arg, &oid, 0, 0);\n \t\tif (!it && revs->ignore_missing)\n \t\t\treturn 0;\n \t\tif (it->type != OBJ_TAG)\n@@ -1905,7 +1909,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\treturn revs->ignore_missing ? 0 : -1;\n \tif (!cant_be_filename)\n \t\tverify_non_filename(revs->prefix, arg);\n-\tobject = get_reference(revs, arg, &oid, flags ^ local_flags);\n+\tobject = get_reference(revs, arg, &oid, flags ^ local_flags, 0);\n \tif (!object)\n \t\treturn revs->ignore_missing ? 0 : -1;\n \tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n@@ -2647,7 +2651,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\tstruct object_context oc;\n \t\tif (get_oid_with_context(revs->repo, revs->def, 0, &oid, &oc))\n \t\t\tdiagnose_missing_default(revs->def);\n-\t\tobject = get_reference(revs, revs->def, &oid, 0);\n+\t\tobject = get_reference(revs, revs->def, &oid, 0, 0);\n \t\tadd_pending_object_with_mode(revs, object, revs->def, oc.mode);\n \t}\n-- snap --\n\nWhat do you think?\n\nCiao,\nDscho\n\n>\n> E.g., here's the output of t5618 with this patch:\n>\n>   expecting success of 5618.6 'log --source shows .alternate marker':\n>   \tgit log --oneline --source --remotes=origin >expect.orig &&\n>   \tsed \"s/origin.* /.alternate /\" <expect.orig >expect &&\n>   \tgit log --oneline --source --alternate-refs >actual &&\n>   \ttest_cmp expect actual\n>\n>   --- expect\t2019-11-06 21:37:06.435427982 +0000\n>   +++ actual\t2019-11-06 21:37:06.439427949 +0000\n>   @@ -1,3 +1,3 @@\n>   -e9067a7\t.alternate two\n>   -eae7140\t.alternate one\n>   -00f38a4\t.alternate base\n>   +e9067a7\t.alternate from /home/peff/compile/git/t/trash directory.t5618-alternate-refs/.git two\n>   +eae7140\t.alternate from /home/peff/compile/git/t/trash directory.t5618-alternate-refs/.git one\n>   +00f38a4\t.alternate from /home/peff/compile/git/t/trash directory.t5618-alternate-refs/.git base\n>   not ok 6 - log --source shows .alternate marker\n>\n>\n> I wonder if a better approach would be to improve the \"bad object\"\n> message. It prints the \"name\", but never even mentions the bogus oid it\n> found! So with this much simpler change:\n>\n>   diff --git a/revision.c b/revision.c\n>   index 309cd31488..4e55a9248c 100644\n>   --- a/revision.c\n>   +++ b/revision.c\n>   @@ -380,7 +380,7 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n>                           return object;\n>                   if (revs->exclude_promisor_objects && is_promisor_object(oid))\n>                           return NULL;\n>   -               die(\"bad object %s\", name);\n>   +               die(_(\"bad object %s (from %s)\"), oid_to_hex(oid), name);\n>           }\n>           object->flags |= flags;\n>           return object;\n>\n> You'd get something like:\n>\n>   fatal: bad object: 1234abcd... (from refs/heads/master)\n>\n> or\n>\n>   fatal: bad object: 1234abcd... (from .alternate)\n>\n> which seems strictly better in the first case, and passably less\n> confusing in the second.\n>\n> -Peff\n>\n> ---\n> Here's the \"name objects after their alternate path\" patch:\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 411e0b4d99..300249b44a 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -280,7 +280,8 @@ static int show_ref_cb(const char *path_full, const struct object_id *oid,\n>  \treturn 0;\n>  }\n>\n> -static void show_one_alternate_ref(const struct object_id *oid,\n> +static void show_one_alternate_ref(const char *altpath,\n> +\t\t\t\t   const struct object_id *oid,\n>  \t\t\t\t   void *data)\n>  {\n>  \tstruct oidset *seen = data;\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 0130b44112..553741341b 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -75,7 +75,8 @@ struct alternate_object_cache {\n>  \tsize_t nr, alloc;\n>  };\n>\n> -static void cache_one_alternate(const struct object_id *oid,\n> +static void cache_one_alternate(const char *altpath,\n> +\t\t\t\tconst struct object_id *oid,\n>  \t\t\t\tvoid *vcache)\n>  {\n>  \tstruct alternate_object_cache *cache = vcache;\n> diff --git a/object-store.h b/object-store.h\n> index 7f7b3cdd80..0066a14f66 100644\n> --- a/object-store.h\n> +++ b/object-store.h\n> @@ -33,7 +33,7 @@ void prepare_alt_odb(struct repository *r);\n>  char *compute_alternate_path(const char *path, struct strbuf *err);\n>  typedef int alt_odb_fn(struct object_directory *, void *);\n>  int foreach_alt_odb(alt_odb_fn, void*);\n> -typedef void alternate_ref_fn(const struct object_id *oid, void *);\n> +typedef void alternate_ref_fn(const char *altpath, const struct object_id *oid, void *);\n>  void for_each_alternate_ref(alternate_ref_fn, void *);\n>\n>  /*\n> diff --git a/revision.c b/revision.c\n> index 0e39b2b8a5..077a47f2e6 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1560,27 +1560,34 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n>  struct add_alternate_refs_data {\n>  \tstruct rev_info *revs;\n>  \tunsigned int flags;\n> +\tstruct strbuf *name;\n>  };\n>\n> -static void add_one_alternate_ref(const struct object_id *oid,\n> +static void add_one_alternate_ref(const char *altpath,\n> +\t\t\t\t  const struct object_id *oid,\n>  \t\t\t\t  void *vdata)\n>  {\n> -\tconst char *name = \".alternate\";\n>  \tstruct add_alternate_refs_data *data = vdata;\n>  \tstruct object *obj;\n>\n> -\tobj = get_reference(data->revs, name, oid, data->flags);\n> -\tadd_rev_cmdline(data->revs, obj, name, REV_CMD_REV, data->flags);\n> -\tadd_pending_object(data->revs, obj, name);\n> +\tstrbuf_reset(data->name);\n> +\tstrbuf_addf(data->name, \".alternate from %s\", altpath);\n> +\n> +\tobj = get_reference(data->revs, data->name->buf, oid, data->flags);\n> +\tadd_rev_cmdline(data->revs, obj, data->name->buf, REV_CMD_REV, data->flags);\n> +\tadd_pending_object(data->revs, obj, data->name->buf);\n>  }\n>\n>  static void add_alternate_refs_to_pending(struct rev_info *revs,\n>  \t\t\t\t\t  unsigned int flags)\n>  {\n> +\tstruct strbuf name = STRBUF_INIT;\n>  \tstruct add_alternate_refs_data data;\n>  \tdata.revs = revs;\n>  \tdata.flags = flags;\n> +\tdata.name = &name;\n>  \tfor_each_alternate_ref(add_one_alternate_ref, &data);\n> +\tstrbuf_release(&name);\n>  }\n>\n>  static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n> diff --git a/sha1-file.c b/sha1-file.c\n> index 188de57634..e1709fdf30 100644\n> --- a/sha1-file.c\n> +++ b/sha1-file.c\n> @@ -793,7 +793,7 @@ static void read_alternate_refs(const char *path,\n>  \t\t\tbreak;\n>  \t\t}\n>\n> -\t\tcb(&oid, data);\n> +\t\tcb(path, &oid, data);\n>  \t}\n>\n>  \tfclose(fh);\n>\n"},{"id":"385768","messageId":"20191108085413.GA4356@sigill.intra.peff.net","threadId":"52205","inReplyTo":"nycvar.QRO.7.76.6.1911071349590.46@tvgsbejvaqbjf.bet","subject":"Re: [BUG] \"fatal: bad object .alternate\" during fetch with alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-08T08:54:13Z","receivedAt":"2019-11-08T08:54:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 07, 2019 at 01:58:04PM +0100, Johannes Schindelin wrote:\n\n> So what about the idea of ignoring (with a warning) them instead,\n> without bothering to try saying much about the alternate itself? I.e.\n> something like this patch (which is admittedly a bit verbose because it\n> _also_ has to update a signature):\n>\n> [...]\n> @@ -1569,7 +1569,11 @@ static void add_one_alternate_ref(const struct object_id *oid,\n>  \tstruct add_alternate_refs_data *data = vdata;\n>  \tstruct object *obj;\n> \n> -\tobj = get_reference(data->revs, name, oid, data->flags);\n> +\tif (!(obj = get_reference(data->revs, name, oid, data->flags, 1))) {\n> +\t\twarning(\"ignoring stale alternate reference to '%s'\",\n> +\t\t\toid_to_hex(oid));\n> +\t\treturn;\n> +\t}\n>  \tadd_rev_cmdline(data->revs, obj, name, REV_CMD_REV, data->flags);\n>  \tadd_pending_object(data->revs, obj, name);\n>  }\n\nI don't think it makes sense to tie this to \"--alternate-refs\" in this\nway. I think there are two ways to look at resolving this:\n\n  - for the UNINTERESTING side of a traversal, we might be willing to\n    ignore a missing object. But then it would apply equally to\n    non-alternates, too. (And we do this already, but not for ref tips,\n    since those aren't just \"stale\" but rather indicative of a\n    corruption).\n\n  - any alternate-ref borrowing is an optimization and thus best-effort.\n    So we should treat them specially, whether UNINTERESTING or not. But\n    then I think the right place to do that is not inside rev-list, but\n    in for_each_alternate_ref(), or even in the for-each-ref run inside\n    the alternate repository. Then it would help rev-list, but also\n    avoid advertising the bogus object across a push/fetch.\n\n-Peff\n"}]}