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