From patchwork Tue Sep 5 07:12:59 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13374555 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D62AAC83F2C for ; Tue, 5 Sep 2023 16:00:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233539AbjIEQAM (ORCPT ); Tue, 5 Sep 2023 12:00:12 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48212 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1353682AbjIEHNF (ORCPT ); Tue, 5 Sep 2023 03:13:05 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5EA7CCC2 for ; Tue, 5 Sep 2023 00:13:02 -0700 (PDT) Received: (qmail 13685 invoked by uid 109); 5 Sep 2023 07:13:01 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 05 Sep 2023 07:13:01 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 19786 invoked by uid 111); 5 Sep 2023 07:13:02 -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, 05 Sep 2023 03:13:02 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 5 Sep 2023 03:12:59 -0400 From: Jeff King To: =?utf-8?b?UmVuw6k=?= Scharfe Cc: git@vger.kernel.org, Junio C Hamano Subject: [PATCH v3 04/10] checkout-index: delay automatic setting of to_tempfile Message-ID: <20230905071259.GE199565@coredump.intra.peff.net> References: <20230831211637.GA949188@coredump.intra.peff.net> <20230831212051.GD949469@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Sat, Sep 02, 2023 at 08:20:43AM +0200, René Scharfe wrote: > Am 31.08.23 um 23:20 schrieb Jeff King: > > @@ -269,6 +268,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) > > state.base_dir = ""; > > state.base_dir_len = strlen(state.base_dir); > > > > + if (to_tempfile < 0) > > + to_tempfile = (checkout_stage == CHECKOUT_ALL); > > + if (!to_tempfile && checkout_stage == CHECKOUT_ALL) > > + die("--stage=all and --no-temp are incompatible"); > > How about making this message translatable from the start and following > the convention from 12909b6b8a (i18n: turn "options are incompatible" > into "cannot be used together", 2022-01-05) to reuse the existing > translations? Good catch. I forgot that we had standardized some of these. The other error messages in the file aren't translated, but I don't think that's an intentional choice (even though it is plumbing). Some of them are obviously quite old and don't match our usual style (like starting with "checkout-index: "). Rather than re-send the whole series, I _think_ this is the only patch I would change in a re-roll (if you buy me "sure, go ahead and send it on top" evasions in my other responses). So here's a replacement patch 4 that fixes up the message. -- >8 -- Subject: checkout-index: delay automatic setting of to_tempfile Using --stage=all requires writing to tempfiles, since we cannot put multiple stages into a single file. So --stage=all implies --temp. But we do so by setting to_tempfile in the options callback for --stage, rather than after all options have been parsed. This leads to two bugs: 1. If you run "checkout-index --stage=all --stage=2", this should not imply --temp, but it currently does. The callback cannot just unset to_tempfile when it sees the "2" value, because it no longer knows if its value was from the earlier --stage call, or if the user specified --temp explicitly. 2. If you run "checkout-index --stage=all --no-temp", the --no-temp will overwrite the earlier implied --temp. But this mode of operation cannot work, and the command will fail with " already exists" when trying to write the higher stages. We can fix both by lazily setting to_tempfile. We'll make it a tristate, with -1 as "not yet given", and have --stage=all enable it only after all options are parsed. Likewise, after all options are parsed we can detect and reject the bogus "--no-temp" case. Note that this does technically change the behavior for "--stage=all --no-temp" for paths which have only one stage present (which accidentally worked before, but is now forbidden). But this behavior was never intended, and you'd have to go out of your way to try to trigger it. The new tests cover both cases, as well the general "--stage=all implies --temp", as most of the other tests explicitly say "--temp". Ironically, the test "checkout --temp within subdir" is the only one that _doesn't_ use "--temp", and so was implicitly covering this case. But it seems reasonable to have a more explicit test alongside the other related ones. Suggested-by: Junio C Hamano Signed-off-by: Jeff King --- builtin/checkout-index.c | 9 +++++++-- t/t2004-checkout-cache-temp.sh | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index f62f13f2b5..526c210bcb 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -24,7 +24,7 @@ static int nul_term_line; static int checkout_stage; /* default to checkout stage0 */ static int ignore_skip_worktree; /* default to 0 */ -static int to_tempfile; +static int to_tempfile = -1; static char topath[4][TEMPORARY_FILENAME_LENGTH + 1]; static struct checkout state = CHECKOUT_INIT; @@ -196,7 +196,6 @@ static int option_parse_stage(const struct option *opt, BUG_ON_OPT_NEG(unset); if (!strcmp(arg, "all")) { - to_tempfile = 1; checkout_stage = CHECKOUT_ALL; } else { int ch = arg[0]; @@ -269,6 +268,12 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) state.base_dir = ""; state.base_dir_len = strlen(state.base_dir); + if (to_tempfile < 0) + to_tempfile = (checkout_stage == CHECKOUT_ALL); + if (!to_tempfile && checkout_stage == CHECKOUT_ALL) + die(_("options '%s' and '%s' cannot be used together"), + "--stage=all", "--no-temp"); + /* * when --prefix is specified we do not want to update cache. */ diff --git a/t/t2004-checkout-cache-temp.sh b/t/t2004-checkout-cache-temp.sh index b16d69ca4a..45dd1bc858 100755 --- a/t/t2004-checkout-cache-temp.sh +++ b/t/t2004-checkout-cache-temp.sh @@ -117,6 +117,26 @@ test_expect_success 'checkout all stages/one file to temporary files' ' test $(cat $s3) = tree3path1) ' +test_expect_success '--stage=all implies --temp' ' + rm -f path* .merge_* actual && + git checkout-index --stage=all -- path1 && + test_path_is_missing path1 +' + +test_expect_success 'overriding --stage=all resets implied --temp' ' + rm -f path* .merge_* actual && + git checkout-index --stage=all --stage=2 -- path1 && + echo tree2path1 >expect && + test_cmp expect path1 +' + +test_expect_success '--stage=all --no-temp is rejected' ' + rm -f path* .merge_* actual && + test_must_fail git checkout-index --stage=all --no-temp -- path1 2>err && + grep -v "already exists" err && + grep "options .--stage=all. and .--no-temp. cannot be used together" err +' + test_expect_success 'checkout some stages/one file to temporary files' ' rm -f path* .merge_* actual && git checkout-index --stage=all --temp -- path2 >actual &&