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

Junio C Hamano [off-list ref] writes:

Junio C Hamano [off-list ref] writes:

quoted

Matthieu Moy [off-list ref] writes:

quoted

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/