[PATCH V3] rebase: fail-fast the pull if working dir is not clean (BC)

[Pierre-Yves David]

note: you sent a V4, before we solved the question in this email. On a 
general basis it is better to wait for open question to be solved before 
sending a new version. It is now a huge deal but it help reduce 
confusion on my side. I'll reply to this email based on V3.

On 01/06/2017 09:48 AM, Valters Vingolds wrote:
> Hi, here's my thinking: the default output might be too terse:
> $ hg pull --rebase
> abort: uncommitted changes
>
> So if a person feels confused by this response ("hold up, what's going
> on?"), and passes "--debug" flag they will receive following output:
> $ hg pull --rebase --debug
> before rebase: ensure working dir is clean
> abort: uncommitted changes
>
> In my mind, this is valuable bit of context: oh, that's because "rebase"
> is in the mix, and is stopping the pull now.

Ha, right, the terse output can be confusing, and the debug output 
helps, "sold".

I don't think --debug is the best answer here, as I don't expect user to 
see about it and if they use it that might scare them. But having it is 
better than nothing.

A better answer would be to improves the abort message by providing a 
way to specify the "action" check uncommited and unfinished for. That 
does not seems to complicated to do but that is still a scope bloat from 
your current work so I'll take a version with the debug things.

> Regarding fake .hg/rebasestate in test, that's literally all that
> cmdutil.checkunfinished() does:
>   if repo.vfs.exists(f):
>       raise error.Abort(msg, hint=hint)
>
> It only checks for existence of named file. And "unfinishedstates" is a
> list of file names where plugins register their "operation in progress"
> status files...
>
> Conversely, in rebase plugin, to store its state, we only do: "f =
> repo.vfs("rebasestate", "w")" and go ahead to write stuff into it.
>
> To test properly, I would have to induce an aborted rebase: generate a
> conflict during rebase, and there are existing tests that do it in
> rebase-abort testcases. (Or maybe aborted graft is easier to set up.)
>
> I'll think about this, but it is not clear to me if the upside (proper
> test) outweighs the downside (complex, maybe brittle set-up), to avoid
> internal knowledge of cmdutil.checkunfinished() behavior [which, we
> know, in the end only will check for an existence of state-file in .hg/
> dir]...

Testing normal user flow seems a better idea than testing implementation 
details. I can think of simpler way to check for unfinished state. For 
example a small histedit session (using edit) can create such condition 
in a simpler way. That seems simple enough to setup with be worth 
trading the hack for it. What do you think ?

Cheers,

-- 
Pierre-Yves David