From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 7AF061F46C for ; Tue, 21 Jan 2020 16:24:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729273AbgAUQYf (ORCPT ); Tue, 21 Jan 2020 11:24:35 -0500 Received: from cloud.peff.net ([104.130.231.41]:41188 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1727817AbgAUQYf (ORCPT ); Tue, 21 Jan 2020 11:24:35 -0500 Received: (qmail 7466 invoked by uid 109); 21 Jan 2020 16:24:34 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Tue, 21 Jan 2020 16:24:34 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 11364 invoked by uid 111); 21 Jan 2020 16:31:27 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Tue, 21 Jan 2020 11:31:27 -0500 Authentication-Results: peff.net; auth=none Date: Tue, 21 Jan 2020 11:24:33 -0500 From: Jeff King To: Derrick Stolee via GitGitGadget Cc: git@vger.kernel.org, jrnieder@gmail.com, Derrick Stolee Subject: Re: [PATCH v2] fetch: document and test --refmap="" Message-ID: <20200121162433.GA6215@coredump.intra.peff.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --refmap= +refs/heads/*:refs/hidden//* 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