[PATCH v3 04/10] checkout-index: delay automatic setting of to_tempfile

On Sat, Sep 02, 2023 at 08:20:43AM +0200, René Scharfe wrote:

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 "<path>
     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 <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/checkout-index.c       |  9 +++++++--
 t/t2004-checkout-cache-temp.sh | 20 ++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)