Re: [PATCH v2] fetch: document and test --refmap=""

From: Jeff King <peff@peff.net>
Date: Jan 21, 2020, 16:24 UTC
Message-ID: <20200121162433.GA6215@coredump.intra.peff.net>
In-Reply-To: <pull.532.v2.git.1579570692766.gitgitgadget@gmail.com>

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