On Mon, Sep 04, 2023 at 11:05:22AM +0100, Phillip Wood wrote:
On 03/09/2023 21:18, Oswald Buddenhagen wrote:
On Sun, Sep 03, 2023 at 08:48:14PM +0100, Phillip Wood wrote:
On 03/09/2023 20:25, Oswald Buddenhagen wrote:
On Sun, Sep 03, 2023 at 07:40:00PM +0100, Phillip Wood wrote:
it only matters for "cherry-pick --skip"
that doesn't seem right. a --skip is just a --continue with a prior
reset, more or less.
sequencer_skip() calls rollback_is_safe() which checks the abort
safety file.
that's weird. can you think of a good reason for doing that?
I think it is clear from the code - so it does not reset changes after
the user has committed the conflict resolution.
yeah, i've researched that meanwhile.
the background is that the machinery that was originally introduced for
abort safety was later (ab-)used for checking whether a --skip makes
sense (de81ca3f36, "cherry-pick/revert: add --skip option", 2019-07-02).
this made the state file a misnomer (not that "abort-safety" was a
particularly good name to start with - i'd have used "latest-head" or
some such).
but the implementation made no sense to me, so i read the mailing list
archive. the result is the attached patch.
however, even so, it seems kinda wrong to me: going by HEAD means that
dropping commits would also trigger it, which would make the given
advice misleading.
in fact, the situation this code path is covering is fundamentally
different from the normal merge conflict: rather than letting the user
resolve it and us finishing the commit, we are rescheduling the pick.
but that means that --skip needs to skip whatever the next command is?
that doesn't sound right.
also, i just tried --continue after a path conflict, and it apparently
did the same as --skip, so something is really wrong.
also, when we have no _HEAD, actually attempting to `reset --merge` is
pointless, no?
oh, and i just noticed that the git-prompt is buggy: it doesn't tell me
about the interrupted multi-pick nested into an interrupted rebase.
ugh, and rebase lets me continue despite still being in the multi-pick.
and the path conflict check is made ineffective by the file in question
being in .gitignore?! (i force-added config.mak.autogen for testing, and
cherry-picking over it goes through just fine.)
i think i'd aim for an object-oriented-ish design with an
encapsulated state, lazy loading getters, lazy setters, and a commit
entry point (or maybe several partial ones). no idea how that would
play out.
I've been working on something similar
awesome!
(well, except for the rebase nightmare in my own series i expect because
of this.)
to only write the state to disc when the sequencer stops for user
interaction.
note that this must cover ctrl-c as well, because the sequencer state
must be consistent with HEAD. of course one could also delay updating
HEAD, but that hinges on no relevant hooks being present, i think?
git-replay has a huge advantage here ...
regards
>From eb81dc1d5ecb7d9d3cf4608b93c30250392f6fc7 Mon Sep 17 00:00:00 2001
From: Oswald Buddenhagen <oswald.buddenhagen@xxxxxx>
Date: Mon, 4 Sep 2023 13:07:04 +0200
Subject: [PATCH] sequencer: improve comment in sequencer_skip()
It wasn't clear under which circumstances the described path would be
relevant.
Change-Id: Ie9fd8a619dad4daf163c5efdb2f9a9eccf17307d
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@xxxxxx>
---
sequencer.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index c0ff165b83..11d2368ab1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3359,12 +3359,13 @@ int sequencer_skip(struct repository *r, struct replay_opts *opts)
* If the corresponding .git/<ACTION>_HEAD exists, we know that the
* action is in progress and we can skip the commit.
*
- * Otherwise we check that the last instruction was related to the
- * particular subcommand we're trying to execute and barf if that's not
- * the case.
- *
- * Finally we check that the rollback is "safe", i.e., has the HEAD
- * moved? In this case, it doesn't make sense to "reset the merge" and
+ * But if the action would have overwritten an untracked file, no
+ * corresponding _HEAD file exists.
+ * In this case we fall back to checking that the last instruction was
+ * related to the particular subcommand we're trying to execute and barf
+ * if that's not the case.
+ * We also check that the rollback is "safe", i.e., whether the HEAD
+ * moved. If it did, it doesn't make sense to "reset the merge" and
* "skip the commit" as the user already handled this by committing. But
* we'd not want to barf here, instead give advice on how to proceed. We
* only need to check that when .git/<ACTION>_HEAD doesn't exist because
--
2.42.0.324.gb1ea313d68
