On Tue, Sep 25, 2018 at 04:56:11PM -0700, Junio C Hamano wrote:
> Taylor Blau <[email protected]> writes:
>
> > My reading of this is threefold:
> >
> > 1. There are some cosmetic changes that need to occur in t5410 and
> > documentation, which are mentioned above. Those seem self
> > explanatory, and I've applied the necessary bits already on my
> > local version of this topic.
> >
> > 2. The core.alternateRefsCommand vs transport.* discussion was
> > resolved in [1] as "let's use core.alternateRefsCommand and
> > core.alternateRefsPrefixes" for now, and others contributors can
> > change this as is needed.
> >
> > 3. We can apply Peff's patch to remove the refname requirement before
> > mine, as well as any relevant changes in my series as have been
> > affected by Peff's patch (e.g., documentation mentioning
> > '%(refname)', etc).
Yeah, these three sound right to me.
> I do think it makes sense to allow alternateRefsCommand to output
> just the object names without adding any refnames, and to keep the
> parser simple, we should not even make the refname optional
> (i.e. "allow" above becomes "require"), and make the default one
> done via an invocation of for-each-ref also do the same.
Yeah, making it optional is just the worst of both worlds, IMHO. Then
callers sometimes get a real value and sometimes just whatever garbage
we fill in, and can't rely on it.
> I do not think there was a strong concensus that we need to change
> the internal C API signature, though. If the function signature for
> the callback between each_ref_fn and alternate_ref_fn were the same,
> I would have opposed to the change, but because they are already
> different, I do not think it is necessary to keep the dummy refname
> parameter that is always passed a meaningless value.
Agreed. I adjusted my "rev-list --alternate-refs" patch for the proposed
new world order (just because it's the likely user of the refname
field). Since the function signatures aren't the same, I already had a
custom callback. It did chain to the existing each_ref_fn one, so I had
to adjust it like so:
diff --git a/revision.c b/revision.c
index 3988275fde..8dfe2fd4c0 100644
--- a/revision.c
+++ b/revision.c
@@ -1396,11 +1396,10 @@ void add_index_objects_to_pending(struct rev_info
*revs, unsigned int flags)
free_worktrees(worktrees);
}
-static void handle_one_alternate_ref(const char *refname,
- const struct object_id *oid,
+static void handle_one_alternate_ref(const struct object_id *oid,
void *data)
{
- handle_one_ref(refname, oid, 0, data);
+ handle_one_ref(".have", oid, 0, data);
}
static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
But I think that's fine. We have to handle the lack of name _somewhere_
in the call stack, so I'd just as soon it be here in the callback, where
we know what it will be used for (or not used at all).
> The final series would be
>
> 1/4: peff's "refnames in alternates do nto matter"
>
> 2/4: your "hardcoded for-each-ref becomes just a default"
>
> 3/4: your "config can affect what command enumerates alternate's tips"
>
> 4/4: your "with prefix config, you don't need a fully custom command"
Yep, that's what I'd expect from the new series.
-Peff
