Re: [PATCH 1/4] builtin/history: perform revwalk checks before asking for user input

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> When setting up the revision walk in git-history(1) we also perform some
> verifications whether the request actually looks sane. Unfortunately,
> these verifications come _after_ we have already asked the user for the
> commit message of the commit that is to be rewritten. So in case any of
> the verifications fails, the user will have lost their modifications.
>
> Extract the function to set up the revision walk and call it before we
> ask for user input to fix this.

That's a huge usability improvement.  Nice.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/history.c         | 69 +++++++++++++++++++++++++++++------------------
>  t/t3451-history-reword.sh |  2 +-
>  2 files changed, 44 insertions(+), 27 deletions(-)

> diff --git a/t/t3451-history-reword.sh b/t/t3451-history-reword.sh
> index 3594421b68..6775ed62f9 100755
> --- a/t/t3451-history-reword.sh
> +++ b/t/t3451-history-reword.sh
> @@ -263,7 +263,7 @@ test_expect_success '--ref-action=head updates only HEAD' '
>  
>  		# When told to update HEAD, only, the command will refuse to
>  		# rewrite commits that are not an ancestor of HEAD.
> -		test_must_fail git history reword --ref-action=head theirs 2>err &&
> +		test_must_fail git -c core.editor=false history reword --ref-action=head theirs 2>err &&
>  		test_grep "rewritten commit must be an ancestor of HEAD" err &&

This ensures that the editor is never consulted?  How?  Running the
"false" editor would give us a different error, like "your editor
exited with non-zero status, telling us to abort" or something?

Thanks.