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

[Gregory Szorc]

On Fri, Jan 6, 2017 at 12:48 AM, Valters Vingolds <valters at vingolds.ch>
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.
>

Your reasoning about users wanting extra context to help them debug is
sound. But I'm surprised nobody mentioned that the "error.Abort" exception
takes an optional keyword "hint" argument that prints an extra message to
provide that context. When we know why an abort occurred and/or what
actions can be taken to prevent it, the "hint" message should provide that
context.

I still think there is room to pass a hint to bailifchanged() (you would
need to teach that function to accept that argument). e.g. "commit or clean
working directory to use --rebase; or remove --rebase to pull without
rebasing." Although getting the wording "just right" is a bit difficult
since there are so many ways to resolve the issue.


>
> 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]...
>
>
> On Fri, Jan 6, 2017 at 7:56 AM, Pierre-Yves David <
> pierre-yves.david at ens-lyon.org> wrote:
>
>> (that was a fast new version, thanks for the reactivity ☺)
>>
>> On 01/05/2017 09:21 AM, Valters Vingolds wrote:
>>
>>> # HG changeset patch
>>> # User Valters Vingolds <valters at vingolds.ch>
>>> # Date 1483272989 -3600
>>> #      Sun Jan 01 13:16:29 2017 +0100
>>> # Node ID e34ac9f0b311708613ae5d18b5791768a90e3582
>>> # Parent  0064a1eb28e246ded9b726c696d048143d1b23f1
>>> rebase: fail-fast the pull if working dir is not clean (BC)
>>>
>>> Refuse to run 'hg pull --rebase' if there are uncommitted changes:
>>> so that instead of going ahead with fetching changes and then suddenly
>>> aborting
>>> the rebase, we can warn user of uncommitted changes (or unclean repo
>>> state)
>>> right up front.
>>>
>>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>>> --- a/hgext/rebase.py
>>> +++ b/hgext/rebase.py
>>> @@ -1316,6 +1316,10 @@
>>>                  ui.debug('--update and --rebase are not compatible,
>>> ignoring '
>>>                           'the update flag\n')
>>>
>>> +            ui.debug('before rebase: ensure working dir is clean\n')
>>>
>>
>> The debug output seems a bit superfluous, I don't think we have similar
>> output for other command, so I would rather drop it (unless I missed
>> something).
>>
>> +            cmdutil.checkunfinished(repo)
>>> +            cmdutil.bailifchanged(repo)
>>> +
>>>              revsprepull = len(repo)
>>>              origpostincoming = commands.postincoming
>>>              def _dummy(*args, **kwargs):
>>> diff --git a/tests/test-rebase-pull.t b/tests/test-rebase-pull.t
>>> --- a/tests/test-rebase-pull.t
>>> +++ b/tests/test-rebase-pull.t
>>> @@ -72,6 +72,19 @@
>>>    searching for changes
>>>    no changes found
>>>
>>> +Abort pull early if working dir is not clean:
>>> +
>>> +  $ echo L1-mod > L1
>>> +  $ hg pull --rebase
>>> +  abort: uncommitted changes
>>> +  [255]
>>> +  $ hg update --clean --quiet
>>> +  $ touch .hg/rebasestate # make rebase think there's one in progress
>>> right now
>>>
>>
>> That seems a bit too hacky. Can we change this to having an actual
>> interrupted operation going on ?
>>
>> +  $ hg pull --rebase
>>> +  abort: rebase in progress
>>> +  (use 'hg rebase --continue' or 'hg rebase --abort')
>>> +  [255]
>>> +  $ rm .hg/rebasestate
>>>
>>>  Invoke pull --rebase and nothing to rebase:
>>>
>>
>> Cheers,
>>
>> --
>> Pierre-Yves David
>>
>
>
>
> --
> Valters Vingolds
> http://www.linkedin.com/in/valters
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170108/36d29acb/attachment.html>