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

login
register
mail settings
Submitter Valters Vingolds
Date Jan. 5, 2017, 8:21 a.m.
Message ID <e34ac9f0b311708613ae.1483604467@w540>
Download mbox | patch
Permalink /patch/18100/
State Changes Requested
Headers show

Comments

Valters Vingolds - Jan. 5, 2017, 8:21 a.m.
# HG changeset patch
# User Valters Vingolds <valters@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.
Sean Farley - Jan. 5, 2017, 6:55 p.m.
Valters Vingolds <valters@vingolds.ch> writes:

> # HG changeset patch
> # User Valters Vingolds <valters@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.

Looks fine to me.
Pierre-Yves David - Jan. 6, 2017, 3:35 p.m.
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,
timeless - Jan. 9, 2017, 4:27 a.m.
Gregory Szorc wrote:
> 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.

+1
Pierre-Yves David - Jan. 9, 2017, 10:34 a.m.
On 01/09/2017 05:27 AM, timeless wrote:
> Gregory Szorc wrote:
>> 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.
>
> +1

Actually, I did mention we should augment 'checkunfinished()' and 
'bailifchanged()' to be able to provide more context ;-)
So +1 for improving these too (but as mentioned in my initial reply, 
that's a scope bloat so I too the debug version for now).

(third paragraph of
https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-January/092041.html 
)

Cheers,

Patch

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')
+            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
+  $ 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: