Page MenuHomePhabricator

visibility: improve the message when accessing filtered obsolete rev
ClosedPublic

Authored by lothiraldan on Dec 5 2017, 9:11 AM.

Details

Summary

When trying to access filtered revision, it is likely because they have been
obsoleted by an obs-marker. The current message shows how to access the
revision anyway:

abort: hidden revision '13bedc178fce'!

But in the case of an obsoleted revision, the user is likely to want to update
to or use the successor of the revision.

We update the message to display more information about the obsolescence fate
of the revision in the following cases:

abort: hidden revision '13bedc178fce' is pruned!

abort: hidden revision '13bedc178fce' has diverged!

abort: hidden revision '13bedc178fce' was rewritten as X, Y and 2 more!

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lothiraldan created this revision.Dec 5 2017, 9:11 AM
durin42 added a subscriber: durin42.Dec 5 2017, 5:20 PM
durin42 added inline comments.
tests/test-log.t
1830

would it be better to put the verb in the abort top-line, isntead of the hint? that is:

abort: hidden revision 'a' is pruned!
(use --hidden to access hidden revisions)

or

abort: hidden revision 'a' is obsolete with successor: eb5a0daa2192
(use --hidden to access hidden revisions)

What do you think?

lothiraldan added inline comments.Dec 6 2017, 4:32 AM
tests/test-log.t
1830

It's an interesting proposition, I don't have a strong opinion. The current behavior is the same as Evolve so it may be better to keep the same, but again no strong opinion.

After thinking more about your proposition, I also think that would be a nice improvement. This way we won't pollute the line helping people to access it anyway.

What I'm proposing:

abort: hidden revision '13bedc178fce' is pruned!
abort: hidden revision '13bedc178fce' has diverged!
abort: hidden revision '13bedc178fce' was rewritten as X, Y and 2 more!

The proposed phrase without divergence is closer to obsfate this way.

@durin42 what do you think?

After thinking more about your proposition, I also think that would be a nice improvement. This way we won't pollute the line helping people to access it anyway.
What I'm proposing:

abort: hidden revision '13bedc178fce' is pruned!
abort: hidden revision '13bedc178fce' has diverged!
abort: hidden revision '13bedc178fce' was rewritten as X, Y and 2 more!

The proposed phrase without divergence is closer to obsfate this way.
@durin42 what do you think?

Works for me, make it so.

durin42 requested changes to this revision.Dec 6 2017, 11:07 AM
This revision now requires changes to proceed.Dec 6 2017, 11:07 AM
pulkit added a subscriber: pulkit.Dec 6 2017, 12:03 PM

After thinking more about your proposition, I also think that would be a nice improvement. This way we won't pollute the line helping people to access it anyway.
What I'm proposing:

abort: hidden revision '13bedc178fce' is pruned!
abort: hidden revision '13bedc178fce' has diverged!
abort: hidden revision '13bedc178fce' was rewritten as X, Y and 2 more!

The proposed phrase without divergence is closer to obsfate this way.

+1. I also like this way.

lothiraldan updated this revision to Diff 4156.Dec 6 2017, 12:48 PM
pulkit requested changes to this revision.Dec 7 2017, 4:33 PM

Apart from the comments, this patch looks good to me.

mercurial/context.py
457

nit: I think in such cases we pass repo as first argument.

Also, I don't think this is a good place for this function, maybe move this to obsutil.py too.

mercurial/obsutil.py
757

s/his/it's

761

s/in the following list/ from the following values

tests/test-obshistory.t
288

We should follow the graph description here too i.e. saying split as x,y,z.

This revision now requires changes to proceed.Dec 7 2017, 4:33 PM
lothiraldan updated this revision to Diff 4250.Dec 8 2017, 5:13 AM
pulkit requested changes to this revision.Dec 8 2017, 9:50 AM
pulkit added inline comments.
tests/test-obshistory.t
191

This one looks odd. Please make this one similar to other split message.

This revision now requires changes to proceed.Dec 8 2017, 9:50 AM
lothiraldan updated this revision to Diff 4527.Dec 18 2017, 7:18 AM
lothiraldan marked an inline comment as done.Dec 18 2017, 7:19 AM
lothiraldan added inline comments.
tests/test-obshistory.t
191

Fixed, thank you for the catch!

Looks good to me but you forgot to update the commit message.

lothiraldan edited the summary of this revision. (Show Details)Dec 21 2017, 3:15 AM
krbullock requested changes to this revision.Dec 21 2017, 5:48 PM
krbullock added a subscriber: krbullock.

One nit, otherwise looks nice!

mercurial/obsutil.py
780

These should be 'superseded' and 'superseded_split' (in the docstring above as well as here)

This revision now requires changes to proceed.Dec 21 2017, 5:48 PM
lothiraldan updated this revision to Diff 4679.Jan 2 2018, 5:17 AM
lothiraldan marked an inline comment as done.Jan 2 2018, 5:17 AM
lothiraldan added inline comments.
mercurial/obsutil.py
780

Thank you, it's fixed now.

krbullock accepted this revision.Jan 4 2018, 10:02 PM

Queued, thanks

krbullock requested changes to this revision.Jan 4 2018, 10:26 PM

This has new failures in test-obshistory.t and test-directaccess.t, can you rebase?

This revision now requires changes to proceed.Jan 4 2018, 10:26 PM
lothiraldan marked an inline comment as done.Jan 5 2018, 3:42 AM
lothiraldan updated this revision to Diff 4703.
lothiraldan added a comment.EditedJan 5 2018, 3:45 AM

@krbullock Done

I pulled from somewhere (likely hg-commited) and get an obs-marker authored by you:

x  72ca90721675 (40698) visibility: improve the message when accessing filtered obsolete rev
|    rewritten(meta, date) as 65044dd2280c by Boris Feld <boris.feld@octobus.net> (Fri Jan 05 09:12:08 2018 +0100)
|    rewritten as f34ff7e24012 by Kevin Bullock <kbullock+mercurial@ringworld.org> (Thu Jan 04 21:01:44 2018 -0600)

But I didn't receive f34ff7e24012, do you know what happened? I'm trying to check if there is a bug involved somewhere. Maybe you stripped f34ff7e24012 with a version of Mercurial that doesn't strip related markers?

lothiraldan updated this revision to Diff 4746.Jan 10 2018, 2:33 AM
durin42 accepted this revision.Jan 10 2018, 5:37 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Jan 11 2018, 9:10 AM
yuja added inline comments.
mercurial/context.py
446

Not translatable.

Needs a table of

{'pruned': _("hidden revision '%s' is pruned"),
 ...,
}

Can you send a followup?

yuja added inline comments.Jan 11 2018, 9:20 AM
mercurial/obsutil.py
871

Maybe it's okay to pass a "filtered" repo to this function as
templatekw.showsuccessorssets() does.