| Message ID | 6e8f65a16dc0be84234d2be93bb4a5c9a585dd57.1537555544.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Filter alternate references | expand |
Taylor Blau <me@ttaylorr.com> writes: > +core.alternateRefsPrefixes:: > + When listing references from an alternate, list only references that begin > + with the given prefix. Prefixes match as if they were given as arguments to > + linkgit:git-for-each-ref[1]. To list multiple prefixes, separate them with > + whitespace. If `core.alternateRefsCommand` is set, setting > + `core.alternateRefsPrefixes` has no effect. We do not allow anything elaborate like "refs/tags/release-*" but we still allow "refs/tags/" and "refs/heads/" by listing them together, and because these are only prefixes, whitespace is a reasonable list separator as they cannot appear anywhere in a refname. OK. Why is this "core"? I thought this was more about receive-pack; even if this is going to be extended to upload-pack's negotiation, "core" is way too wide a hierarchy. We have "transport.*" for things like this, no? The exact same comment applies to 2/3, of course. > diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh > index 2f21f1cb8f..b656c9b30c 100755 > --- a/t/t5410-receive-pack.sh > +++ b/t/t5410-receive-pack.sh > @@ -51,4 +51,12 @@ test_expect_success 'with core.alternateRefsCommand' ' > test_cmp expect actual.haves > ' > > +test_expect_success 'with core.alternateRefsPrefixes' ' > + test_config -C fork core.alternateRefsPrefixes "refs/tags" && > + expect_haves one three two >expect && > + printf "0000" | git receive-pack fork >actual && > + extract_haves <actual >actual.haves && > + test_cmp expect actual.haves > +' > + > test_done > diff --git a/transport.c b/transport.c > index e7d2cdf00b..9323e5c3cd 100644 > --- a/transport.c > +++ b/transport.c > @@ -1341,6 +1341,11 @@ static void fill_alternate_refs_command(struct child_process *cmd, > argv_array_pushf(&cmd->args, "--git-dir=%s", repo_path); > argv_array_push(&cmd->args, "for-each-ref"); > argv_array_push(&cmd->args, "--format=%(objectname) %(refname)"); > + > + if (!git_config_get_value("core.alternateRefsPrefixes", &value)) { > + argv_array_push(&cmd->args, "--"); > + argv_array_split(&cmd->args, value); > + } > } > > cmd->env = local_repo_env;
On Fri, Sep 21, 2018 at 02:14:17PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > +core.alternateRefsPrefixes:: > > + When listing references from an alternate, list only references that begin > > + with the given prefix. Prefixes match as if they were given as arguments to > > + linkgit:git-for-each-ref[1]. To list multiple prefixes, separate them with > > + whitespace. If `core.alternateRefsCommand` is set, setting > > + `core.alternateRefsPrefixes` has no effect. > > We do not allow anything elaborate like "refs/tags/release-*" but we > still allow "refs/tags/" and "refs/heads/" by listing them together, > and because these are only prefixes, whitespace is a reasonable list > separator as they cannot appear anywhere in a refname. OK. > > Why is this "core"? I thought this was more about receive-pack; > even if this is going to be extended to upload-pack's negotiation, > "core" is way too wide a hierarchy. We have "transport.*" for > things like this, no? There's no extension necessary; these should already affect upload-pack as well. I agree transport.* would cover both upload-pack and receive-pack. If we extend it to check_everything_connected(), would it make sense as part of transport.*, too? I dunno. I guess I could see an argument either way. If we do add "rev-list --alternate-refs", that pushes it even further away from transport.*, though. -Peff
Jeff King <peff@peff.net> writes: > There's no extension necessary; these should already affect upload-pack > as well. I agree transport.* would cover both upload-pack and > receive-pack. If we extend it to check_everything_connected(), would it > make sense as part of transport.*, too? > > I dunno. I guess I could see an argument either way. Sorry but I do not quite follow. Are you saying that something that covers check-everything-connected would the result be too wide to fit inside transport.*? or something that does not cover check-everything-connected falls short of transport.*? Or something else? Either way, core.* is way too wide for what this hook does, I would think.
On Fri, Sep 21, 2018 at 03:06:43PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > There's no extension necessary; these should already affect upload-pack > > as well. I agree transport.* would cover both upload-pack and > > receive-pack. If we extend it to check_everything_connected(), would it > > make sense as part of transport.*, too? > > > > I dunno. I guess I could see an argument either way. > > Sorry but I do not quite follow. Are you saying that something that > covers check-everything-connected would the result be too wide to > fit inside transport.*? or something that does not cover > check-everything-connected falls short of transport.*? Or something > else? Either way, core.* is way too wide for what this hook does, I > would think. I was suggesting that check_everything_connected() is not strictly transport-related, so would be inappropriate for transport.*, and we'd need a more generic name. And my "either way" was that I could see an argument that it _is_ transport related, since we only call it now when receiving a pack. But that doesn't have to be the case, and certainly implementing it with "rev-list --alternate-refs" muddies that considerably. I agree that core.* is kind of a kitchen sink, but I'm not sure that's all that bad. Is "here is how Git finds refs in an alternate" any more or less core than "here is how Git invokes ssh"? -Peff
On Fri, Sep 21, 2018 at 3:18 PM Jeff King <peff@peff.net> wrote: > I agree that core.* is kind of a kitchen sink, but I'm not sure that's > all that bad. Is "here is how Git finds refs in an alternate" any more This touches both "refs" and "alternates", which are Git concepts whereas ssh is not. > or less core than "here is how Git invokes ssh"? Arguably core.sshCommand should be deprecated and re-introduced as transport."ssh".command. :-P
Jeff King <peff@peff.net> writes: > I was suggesting that check_everything_connected() is not strictly > transport-related, so would be inappropriate for transport.*, and we'd > need a more generic name. And my "either way" was that I could see > an argument that it _is_ transport related, since we only call it now > when receiving a pack. But that doesn't have to be the case, and > certainly implementing it with "rev-list --alternate-refs" muddies that > considerably. Even after 7043c707 ("check_everything_connected: use a struct with named options", 2016-07-15) unified many into check_connected(), there still are different reasons why we call to find out about the connectivity, and I doubt we can afford to have a single knob that is shared both for transport and other kind of connectivity checks (like fsck or repack). Do we want to be affected by "we pretend that these are the only refs exported from that alternate object store" when repacking and pruning only local objects and keep us rely on the alternate, for example? In any case it is good that these configuration variables are defined on _our_ side, not in the alternate---it means that we do not have to worry about the case where the alternateRefsCommand lies and tells us that an object that the alternate does not actually have exists at a tip of a ref in an attempt to confuse us, etc.
On Mon, Sep 24, 2018 at 08:17:14AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > I was suggesting that check_everything_connected() is not strictly > > transport-related, so would be inappropriate for transport.*, and we'd > > need a more generic name. And my "either way" was that I could see > > an argument that it _is_ transport related, since we only call it now > > when receiving a pack. But that doesn't have to be the case, and > > certainly implementing it with "rev-list --alternate-refs" muddies that > > considerably. > > Even after 7043c707 ("check_everything_connected: use a struct with > named options", 2016-07-15) unified many into check_connected(), > there still are different reasons why we call to find out about the > connectivity, and I doubt we can afford to have a single knob that > is shared both for transport and other kind of connectivity checks > (like fsck or repack). Do we want to be affected by "we pretend > that these are the only refs exported from that alternate object > store" when repacking and pruning only local objects and keep us > rely on the alternate, for example? Actually, yes, I think there is value in a single knob. At least that's what I'd want for our (GitHub's) use case. Remember that these alternate refs might not exist at all (the alternates mechanism can work with just a bare "objects" directory, unconnected from a real git repo). So I think anything using them has to view it as a "best effort" optimization: we might or might not know about some ref tips that might or might not cover the whole set of objects in the alternate. They're the things we _guarantee_ that the alternate has full connectivity for, and it might have more. So I think it's conceptually consistent to always show a subset. I did qualify with "for our use case" because some people might be primarily concerned with the bandwidth of sending .haves across the network. Whereas at our scale, even enumerating them at all is prohibitively expensive. One thing we could do is add a "core" config now (whether it's in core.* or wherever). And then if later somebody wants receive-pack to behave differently, we have an out: we can add transfer.alternateRefsCommand or even receive.alternateRefsCommand that take precedence in those situations. Of course we could add the more restricted ones now, and add the "core" one later as new uses grow. But that's more work now, since we'd have to plumb through that context to the for_each_alternate_ref() interface. I'd rather punt on that work until later (because I suspect that "later" will never actually come). > In any case it is good that these configuration variables are > defined on _our_ side, not in the alternate---it means that we do > not have to worry about the case where the alternateRefsCommand lies > and tells us that an object that the alternate does not actually > have exists at a tip of a ref in an attempt to confuse us, etc. Yes. It also makes it easy to use "git -c" to override the scheme if you want to (as opposed to mucking with on-disk files in the alternate). -Peff
Jeff King <peff@peff.net> writes:
> So I think it's conceptually consistent to always show a subset.
OK. Then I agree with you that it is a good approach to first adopt
core.* knobs that universally apply, and add specialized ones as
they are needed later.
Thanks.
On Mon, Sep 24, 2018 at 01:32:26PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > So I think it's conceptually consistent to always show a subset. > > OK. Then I agree with you that it is a good approach to first adopt > core.* knobs that universally apply, and add specialized ones as > they are needed later. Thanks. There's one other major decision for this series, I think. Do you have an opinion on whether for_each_alternate_refs() interface should stop passing back refnames? By the "they may not even exist" rationale in this sub-thread, I think it's probably foolish for any caller to actually depend on the names being meaningful. We need to decide now because the idea of which data is relevant is getting baked into the documented alternateRefsCmd output format. -Peff
On Mon, Sep 24, 2018 at 04:50:22PM -0400, Jeff King wrote: > On Mon, Sep 24, 2018 at 01:32:26PM -0700, Junio C Hamano wrote: > > > Jeff King <peff@peff.net> writes: > > > > > So I think it's conceptually consistent to always show a subset. > > > > OK. Then I agree with you that it is a good approach to first adopt > > core.* knobs that universally apply, and add specialized ones as > > they are needed later. > > Thanks. There's one other major decision for this series, I think. > > Do you have an opinion on whether for_each_alternate_refs() interface > should stop passing back refnames? By the "they may not even exist" > rationale in this sub-thread, I think it's probably foolish for any > caller to actually depend on the names being meaningful. > > We need to decide now because the idea of which data is relevant is > getting baked into the documented alternateRefsCmd output format. Just to sketch it out further, I was thinking that we'd do something like this at the front of Taylor's series (with the rest rebased as appropriate on top). -- >8 -- Subject: [PATCH] transport: drop refnames from for_each_alternate_ref None of the current callers use the refname parameter we pass to their callbacks. In theory somebody _could_ do so, but it's actually quite weird if you think about it: it's a ref in somebody else's repository. So the name has no meaning locally, and in fact there may be duplicates if there are multiple alternates. The users of this interface really only care about seeing some ref tips, since that promises that the alternate has the full commit graph reachable from there. So let's keep the information we pass back to the bare minimum. Signed-off-by: Jeff King <peff@peff.net> --- builtin/receive-pack.c | 3 +-- fetch-pack.c | 3 +-- transport.c | 6 +++--- transport.h | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index a3bb13af10..39993f2bcf 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -281,8 +281,7 @@ static int show_ref_cb(const char *path_full, const struct object_id *oid, return 0; } -static void show_one_alternate_ref(const char *refname, - const struct object_id *oid, +static void show_one_alternate_ref(const struct object_id *oid, void *data) { struct oidset *seen = data; diff --git a/fetch-pack.c b/fetch-pack.c index 75047a4b2a..b643de143b 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -76,8 +76,7 @@ struct alternate_object_cache { size_t nr, alloc; }; -static void cache_one_alternate(const char *refname, - const struct object_id *oid, +static void cache_one_alternate(const struct object_id *oid, void *vcache) { struct alternate_object_cache *cache = vcache; diff --git a/transport.c b/transport.c index 1c76d64aba..2e0bc414d0 100644 --- a/transport.c +++ b/transport.c @@ -1336,7 +1336,7 @@ static void read_alternate_refs(const char *path, cmd.git_cmd = 1; argv_array_pushf(&cmd.args, "--git-dir=%s", path); argv_array_push(&cmd.args, "for-each-ref"); - argv_array_push(&cmd.args, "--format=%(objectname) %(refname)"); + argv_array_push(&cmd.args, "--format=%(objectname)"); cmd.env = local_repo_env; cmd.out = -1; @@ -1348,13 +1348,13 @@ static void read_alternate_refs(const char *path, struct object_id oid; if (get_oid_hex(line.buf, &oid) || - line.buf[GIT_SHA1_HEXSZ] != ' ') { + line.buf[GIT_SHA1_HEXSZ]) { warning(_("invalid line while parsing alternate refs: %s"), line.buf); break; } - cb(line.buf + GIT_SHA1_HEXSZ + 1, &oid, data); + cb(&oid, data); } fclose(fh); diff --git a/transport.h b/transport.h index 01e717c29e..9baeca2d7a 100644 --- a/transport.h +++ b/transport.h @@ -261,6 +261,6 @@ int transport_refs_pushed(struct ref *ref); void transport_print_push_status(const char *dest, struct ref *refs, int verbose, int porcelain, unsigned int *reject_reasons); -typedef void alternate_ref_fn(const char *refname, const struct object_id *oid, void *); +typedef void alternate_ref_fn(const struct object_id *oid, void *); extern void for_each_alternate_ref(alternate_ref_fn, void *); #endif
Jeff King <peff@peff.net> writes: > Do you have an opinion on whether for_each_alternate_refs() interface > should stop passing back refnames? By the "they may not even exist" > rationale in this sub-thread, I think it's probably foolish for any > caller to actually depend on the names being meaningful. I personally do not mind they were all ".have" or unnamed. The primary motivatgion behind for-each-alternate-refs was that we wanted to find more anchoring points to help the common ancestry negotiation and for-each-*-ref was the obvious way to do so; the user did not care anything about names.
On Mon, Sep 24, 2018 at 02:55:57PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Do you have an opinion on whether for_each_alternate_refs() interface > > should stop passing back refnames? By the "they may not even exist" > > rationale in this sub-thread, I think it's probably foolish for any > > caller to actually depend on the names being meaningful. > > I personally do not mind they were all ".have" or unnamed. > > The primary motivatgion behind for-each-alternate-refs was that we > wanted to find more anchoring points to help the common ancestry > negotiation and for-each-*-ref was the obvious way to do so; the > user did not care anything about names. Right, I think that is totally fine for the current uses. I guess my question was: do you envision cutting the interface down to only the oids to bite us in the future? I was on the fence during past discussions, but I think I've come over to the idea that the refnames actively confuse things. -Peff
Jeff King <peff@peff.net> writes: > Right, I think that is totally fine for the current uses. I guess my > question was: do you envision cutting the interface down to only the > oids to bite us in the future? > > I was on the fence during past discussions, but I think I've come over > to the idea that the refnames actively confuse things. Alternates are sort-of repositories that you interact with via more normal transports like fetch or push, and at the object store level (i.e. the one that helps you build your local history) you do not really care what refnames other people use in their repository. E.g. it does not matter if a pull request to you asks you to pull their 'frotz' branch or 'nitfol' branch, as long as the work they did on that branch is what you expected them to do. And I think "I am aware that I can get to the objects that are reachable from these objects I can borrow from that alternate when I need them" is quite similar in spirit; the borrower has even less need to be aware of the refnames as there isn't even a need to "git pull" from it (at that only one single point, you would care what name they used in their pull request). So, I think we probably are better off without names.
On Tue, Sep 25, 2018 at 10:41:18AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Right, I think that is totally fine for the current uses. I guess my > > question was: do you envision cutting the interface down to only the > > oids to bite us in the future? > > > > I was on the fence during past discussions, but I think I've come over > > to the idea that the refnames actively confuse things. > > [ ... ] > > So, I think we probably are better off without names. Sorry for re-entering the thread a little later. I was travelling yesterday, and was surprised when I discovered that our "grep | sed" vs. "sed" discussion had grown so much ;-). 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). Does this all sound sane to you (and match your recollection/reading of the thread)? If so, I'll send v3 hopefully tomorrow. Sorry for repeating what's already been said in this thread, but I felt it was important to ensure that we had matching understandings of one another. Thanks, Taylor [1]: https://public-inbox.org/git/xmqqa7o6skkl.fsf@gitster-ct.c.googlers.com/
Taylor Blau <me@ttaylorr.com> 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). 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. 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. 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" I guess?
On Tue, Sep 25, 2018 at 04:56:11PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> 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). > > 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. > > 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. > > 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" > > I guess? Perfect -- we are in agreement on how the rerolled series should be organized. I don't anticipate much further comment on v2 in this thread, but I'll let it sit overnight to make sure that the dust has all settled after my new mail. I have a version of what will likely become 'v3', pushed here: [1]. Thanks, Taylor [1]: https://github.com/ttaylorr/git/tree/tb/alternate-refs-cmd
On Tue, Sep 25, 2018 at 04:56:11PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> 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
diff --git a/Documentation/config.txt b/Documentation/config.txt index 526557e494..7df6c22925 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -627,6 +627,13 @@ alternate's references as ".have"'s. For example, to only advertise branch heads, configure `core.alternateRefsCommand` to the path of a script which runs `git --git-dir="$1" for-each-ref refs/heads`. +core.alternateRefsPrefixes:: + When listing references from an alternate, list only references that begin + with the given prefix. Prefixes match as if they were given as arguments to + linkgit:git-for-each-ref[1]. To list multiple prefixes, separate them with + whitespace. If `core.alternateRefsCommand` is set, setting + `core.alternateRefsPrefixes` has no effect. + core.bare:: If true this repository is assumed to be 'bare' and has no working directory associated with it. If this is the case a diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh index 2f21f1cb8f..b656c9b30c 100755 --- a/t/t5410-receive-pack.sh +++ b/t/t5410-receive-pack.sh @@ -51,4 +51,12 @@ test_expect_success 'with core.alternateRefsCommand' ' test_cmp expect actual.haves ' +test_expect_success 'with core.alternateRefsPrefixes' ' + test_config -C fork core.alternateRefsPrefixes "refs/tags" && + expect_haves one three two >expect && + printf "0000" | git receive-pack fork >actual && + extract_haves <actual >actual.haves && + test_cmp expect actual.haves +' + test_done diff --git a/transport.c b/transport.c index e7d2cdf00b..9323e5c3cd 100644 --- a/transport.c +++ b/transport.c @@ -1341,6 +1341,11 @@ static void fill_alternate_refs_command(struct child_process *cmd, argv_array_pushf(&cmd->args, "--git-dir=%s", repo_path); argv_array_push(&cmd->args, "for-each-ref"); argv_array_push(&cmd->args, "--format=%(objectname) %(refname)"); + + if (!git_config_get_value("core.alternateRefsPrefixes", &value)) { + argv_array_push(&cmd->args, "--"); + argv_array_split(&cmd->args, value); + } } cmd->env = local_repo_env;
The recently-introduced "core.alternateRefsCommand" allows callers to specify with high flexibility the tips that they wish to advertise from alternates. This flexibility comes at the cost of some inconvenience when the caller only wishes to limit the advertisement to one or more prefixes. For example, to advertise only tags, a caller using 'core.alternateRefsCommand' would have to do: $ git config core.alternateRefsCommand ' \ git -C "$1" for-each-ref refs/tags \ --format="%(objectname) %(refname)" \ ' The above is cumbersome to write, so let's introduce a "core.alternateRefsPrefixes" to address this common case. Instead, the caller can run: $ git config core.alternateRefsPrefixes 'refs/tags' Which will behave identically to the longer example using "core.alternateRefsCommand". Since the value of "core.alternateRefsPrefixes" is appended to 'git for-each-ref' and then executed, include a "--" before taking the configured value to avoid misinterpreting arguments as flags to 'git for-each-ref'. In the case that the caller wishes to specify multiple prefixes, they may separate them by whitespace. If "core.alternateRefsCommand" is set, it will take precedence over "core.alternateRefsPrefixes". Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/config.txt | 7 +++++++ t/t5410-receive-pack.sh | 8 ++++++++ transport.c | 5 +++++ 3 files changed, 20 insertions(+)