Re: [PATCH V3] rebase: fail-fast the pull if working dir is not clean (BC)
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, -- Pierre-Yves David ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH V3] rebase: fail-fast the pull if working dir is not clean (BC)
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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH V3] rebase: fail-fast the pull if working dir is not clean (BC)
On Fri, Jan 6, 2017 at 12: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.
>
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.da...@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
>>> # 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@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
___
Merc
Re: [PATCH V3] rebase: fail-fast the pull if working dir is not clean (BC)
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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH V3] rebase: fail-fast the pull if working dir is not clean (BC)
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.
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.da...@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
>> # 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@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH V3] rebase: fail-fast the pull if working dir is not clean (BC)
(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
# 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH V3] rebase: fail-fast the pull if working dir is not clean (BC)
Valters Vingolds writes: > # HG changeset patch > # User Valters Vingolds > # 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. ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
[PATCH V3] rebase: fail-fast the pull if working dir is not clean (BC)
# HG changeset patch
# User Valters Vingolds
# 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')
+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:
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
