From: Matthieu Moy Date: Thu, 25 Aug 2011 07:09:26 GMT Subject: Re: [PATCH] rebase -i: clean error message for --continue after failed exec Message-ID: In-Reply-To: <7vy5yiod1h.fsf@alter.siamese.dyndns.org> Junio C Hamano writes: > Junio C Hamano writes: > >> Matthieu Moy 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/