Re: [PATCH] rebase -i: clean error message for --continue after failed exec

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Aug 25, 2011, 07:09 UTC
Message-ID: <vpqzkiydl21.fsf@bauges.imag.fr>
In-Reply-To: <7vy5yiod1h.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 15 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>>
>>> If after a failed "exec" instruction there are staged changes,...
>>
>> I have to wonder why whatever "exec" runs is mucking with the index in the
>> first place. Shouldn't we forbid it?
>
> I suspect your patch amounts to the same thing of forbidding, but
> detecting the lack of $author_script feels like it is covering up the
> symptom and not directly going for the cause of the symptom.
>
> I wonder if doing something like this would be more direct approach to
> achieve the same thing.

Not the same thing, but both patches could go well together.

Mine covers

  pick deadbeef
  exec make test
  # :-( make test failed, I'm going to fix it
  hack hack hack
  git add changes
  # OK, seems fixed.
  git rebase --continue
  # --> rebase tells me I forgot to commit my fixup patch

i.e. the user changed the index interactively, not within exec. Yours covers the case where the command itself changes the index.

> +		# Run in subshell because require_clean_work_tree can die.
> +		dirty=f
> +		(require_clean_work_tree "rebase") || dirty=t

This will display error messages like

  Cannot rebase: You have unstaged changes

and right after

>  			warn "Execution failed: $rest"
> +			test "$dirty" = f ||
> +			warn "and made changes to the index and/or the working tree"

which sounds redundant. This should probably be

(require_clean_work_tree "rebase" 2>/dev/null) || dirty=t

but looking more closely at the patch, you're not the one introducing this, it was already there since 92c62a3f.

>  		fi
> +
>  		;;
>  	*)

I think this one can be removed, there's usually no blank line before ;; in the code.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/