This is an archive of the discontinued Mercurial Phabricator instance.

revert: fix the inconsistency of status msgs in --interactive mode
ClosedPublic

Authored by khanchi97 on Aug 26 2018, 7:06 PM.

Details

Summary

Before this patch we were priting every action msg before actually
performing that action and that was resulting in inconsistencies;
like in --interactive session if user decided to not revert any
changes in a file foo, still there will be a msg on console saying
"reverting foo".

To fix this, I have made some changes to print status msg just
before the action it is going to perform, no matter if --interactive
or not.

Changes made in test-revert-interactive.t reflect the changed behavior.
There are also some changes in test-revert.t because of change in the
order of messages.

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

khanchi97 created this revision.Aug 26 2018, 7:06 PM
yuja added a subscriber: yuja.Aug 28 2018, 9:00 AM
$ hg revert a

+ undeleting a

As you can see, there was no status message if "a" is an explicitly specified
file.

@@ -3021,7 +3021,8 @@

if ui.verbose or not exact:
    if not isinstance(msg, bytes):
        msg = msg(abs)
  • ui.status(msg % rel)

+ if opts.get('dry_run'):
+ ui.status(msg % rel)

Before a relative path was printed, but new code would probably use the
repository-absolute path.

Perhaps we can pass in the matcher m to _performrevert() to translate
abs path to relative, and to test if status message should be printed.

+ repo.ui.status(_("%s") % (actions['forget'][1] % f))

Nit: _("%s") is noop. Just print actions['forget'][1] % f.

@@ -3141,14 +3147,21 @@

    tobackup = set()
# Apply changes
fp = stringio()

+ # fnames keep track of filenames for which we have initiated changes,
+ # to make sure that we print status msg only once for a file.
+ fnames = []

Nit: perhaps set() is better here since we have to test the existence
many times.

FWIW, no idea why revert() has to be such complicated.

khanchi97 edited the summary of this revision. (Show Details)Aug 30 2018, 4:55 PM
khanchi97 updated this revision to Diff 10679.

Made changes. Ready to review.

In D4380#67537, @yuja wrote:
$ hg revert a

+ undeleting a

As you can see, there was no status message if "a" is an explicitly specified
file.

Oh yes, added exact conditional.

@@ -3021,7 +3021,8 @@

if ui.verbose or not exact:
    if not isinstance(msg, bytes):
        msg = msg(abs)
  • ui.status(msg % rel)

+ if opts.get('dry_run'):
+ ui.status(msg % rel)

Before a relative path was printed, but new code would probably use the
repository-absolute path.
Perhaps we can pass in the matcher m to _performrevert() to translate
abs path to relative, and to test if status message should be printed.

I passed the names dict as it already have the required information.

+ repo.ui.status(_("%s") % (actions['forget'][1] % f))

Nit: _("%s") is noop. Just print actions['forget'][1] % f.

@@ -3141,14 +3147,21 @@

    tobackup = set()
# Apply changes
fp = stringio()

+ # fnames keep track of filenames for which we have initiated changes,
+ # to make sure that we print status msg only once for a file.
+ fnames = []

Nit: perhaps set() is better here since we have to test the existence
many times.
FWIW, no idea why revert() has to be such complicated.

mercurial/cmdutil.py
3024

Does this conditional doing something useful because I don't see any change in the tests if I remove line 3023,3024?

yuja added a comment.Aug 31 2018, 7:53 AM

+ if ui.verbose or not exact:
+ if not isinstance(msg, bytes):
+ msg = msg(abs)

Does this conditional doing something useful because I don't see any change in the tests if I remove line 3023,3024?

Probably no. msg should never be a callable since 0d57bf80c7cb.

yuja added a comment.Aug 31 2018, 7:53 AM

@@ -3034,7 +3035,19 @@

prefetch(repo, [ctx.rev()],
         matchfiles(repo,
                    [f for sublist in oplist for f in sublist]))
  • _performrevert(repo, parents, ctx, actions, interactive, tobackup)

+ # give status messages for actions to be performed
+ acts = "add drop undelete forget remove revert".split()
+ if interactive:
+ # exclude ("forget", "remove", "revert") as status msg for these
+ # actions will be handled in interactive session.
+ acts = acts[:3]
+ for act in acts:
+ for f in actions[act][0]:
+ rel, exact = names[f]
+ if ui.verbose or not exact:
+ ui.status(actions[act][1] % rel)
+ _performrevert(repo, parents, ctx, names, actions, interactive,
+ tobackup)

Looks generally good, but I think the original patch was better in that
status message is printed just before the action, no matter if interactive
or not.

+ rel, exact = names[f]
+ if repo.ui.verbose or not exact:
+ repo.ui.status(actions['forget'][1] % rel)

This can be factored out to a closure function if you think it's too verbose
to copy around.

khanchi97 edited the summary of this revision. (Show Details)Sep 3 2018, 1:53 AM
khanchi97 updated this revision to Diff 10715.

Ready to review.

yuja added a comment.Sep 3 2018, 7:32 AM

Queued, thanks.

This revision was automatically updated to reflect the committed changes.