Re: [PATCH] am: support --quit

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 23, 2018, 18:00 UTC
Message-ID: <xmqq606nvb68.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CACsJy8CUmpXNZDR6jvXP=hQkuTmCqzYYqitXJyYYR31Yw7DXXw@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 10 quoted lines

>> The internal implementation detail of am_abort() is leaking out
>> here, by saying "rerere-clear" is the only special thing other than
>> recovering the HEAD and working tree state when abort happens.  It
>> makes readers wonder if am_rerere_clear() should become part of
>> am_destroy().  I dunno.
>
> I think the original design is am_destroy takes care of
> $GIT_DIR/rebase-apply and nothing else. --abort has to clean up things
> outside (index, HEAD, rerere) while a successful operation should not
> leave anything else to clean up (except rebase-apply dir).

Yes, and that is why I think the code would have been nicer to understand if the update to add 'reset' had turned the existing am_abort() into a helper that is one level higher in the abstraction (perhaps even by renaming the function) that the caller can tell which part to clear (i.e. e.g. am_finish(&state, AM_CLEAR_ALL) vs am_finish(&state, AM_CLEAR_STEP_ONLY)). Stepping back even further, perhaps the call made to am_destroy() in the normal exit case at the end of am_run() could have been using the same helper.