On Tue, Jan 21, 2020 at 01:38:12AM +0000, Derrick Stolee via GitGitGadget wrote:
> Update the documentation to clarify how '--refmap=""' works and
> create tests to guarantee this behavior remains in the future.
Yeah, this looks like a good solution to me.
> This can be accomplished by overriding the configured refspec using
> '--refmap=' along with a custom refspec:
>
> git fetch <remote> --refmap= +refs/heads/*:refs/hidden/<remote>/*
This isn't strictly related to your patch, but since the rationale here
describes the concept of a background job and people might end up using
it as a reference, do you want to add in --no-tags to help them out?
> Thanks for all the feedback leading to absolutely no code change. It's
> good we already have the flexibility for this. I'm a bit embarrassed
> that I did not discover this, so perhaps this doc change and new tests
> will help clarify the behavior.
If it makes you feel better, I only found --refmap because I was the one
who implemented the original "update based on refspecs" logic, and while
looking for that commit with "git log --grep=opportunistic" I stumbled
onto Junio's commit adding --refmap, which referenced mine. Maybe this
also works as a good case study in why we should write good commit
messages and refer to related work. :)
Anyway, I wasn't at all sure that a blank --refmap= would do what you
want until I tried it. But it was always intended to work that way. From
c5558f80c3 (fetch: allow explicit --refmap to override configuration,
2014-05-29):
+static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
+{
+ ALLOC_GROW(refmap_array, refmap_nr + 1, refmap_alloc);
+
+ /*
+ * "git fetch --refmap='' origin foo"
+ * can be used to tell the command not to store anywhere
+ */
+ if (*arg)
+ refmap_array[refmap_nr++] = arg;
+ return 0;
+}
At first I thought the comment was wrong, since we don't actually
increment refmap_nr. But the ALLOC_GROW makes refmap_array non-NULL,
which is what triggers the "do not use configured refspecs" logic.
This code switched to refspec_append() in e4cffacc80 (fetch: convert
refmap to use struct refspec, 2018-05-16), and I think we actually do
push an empty string onto the list. Which then triggers the "do not use
configured refspecs" logic, but doesn't match anything itself. I'm not
sure whether that behavior was entirely planned, or just what the code
happens to do. So it's doubly useful to add a test here covering the
expected behavior.
> Documentation/fetch-options.txt | 5 ++++-
> t/t5510-fetch.sh | 24 ++++++++++++++++++++++++
> 2 files changed, 28 insertions(+), 1 deletion(-)
The patch looks good to me.
-Peff
