From: Oswald Buddenhagen Date: Thu, 23 Mar 2023 23:23:17 GMT Subject: Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure Message-ID: In-Reply-To: On Thu, Mar 23, 2023 at 01:16:47PM -0700, Junio C Hamano wrote: >Oswald Buddenhagen writes: > >> Creating a suitable todo file is a potentially labor-intensive process, >> so be less cavalier about discarding it when something goes wrong (e.g., >> the user messed with the repo while editing the todo). > >Is there a reason why we do not always keep it? Why is the file >sometimes precious but not precious at all in other times? > the unedited initial todo just isn't precious. that implies that in a non-interactive rebase, it is always worthless at the time of the initial reset. >Tying the previous bit to "-i was explicitly given" feels a bit >unintuitive---when the sequencer machinery was implicitly chosen, >and gives the control back to the user, should a user be forbidden >to muck with the todo list? > that would be an --edit-todo and --continue during a mid-rebase stop. rather different case. >No // comments, please. > (apparently with a special exception for examples in the apidocs, presumably because escaping nested comments would be just too ugly.) >> - test_path_is_missing .git/rebase-merge && >> + test_path_is_dir .git/rebase-merge && > >Are we happy to just see that the directory still exists? I thought >the original motivation explained in the proposed log message was to >keep the todo list file, so shouldn't you be checking if the file is >there > fair point. >(and if you can reliably ensure that the file has contents >that are expected, that would be even better)? > i could grep for a shortened sha1 i would obtain from the branch. but given that the error scenario of a present but somehow corrupted todo seems implausible given the circumstances, that seems like overkill. >Also, as the keeping of the todo list is now conditional, we should >have another test that checks that the file is gone when that >condition ("INTERACTIVE_EXPLICIT"?) does not trigger, I think. > that would be for t3400-rebase.sh. i suppose we could extend 'Show verbose error when HEAD could not be detached'.