| 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 <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.
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,
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
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: