This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5491

amend: added config option to update time to current in hg amend(issue5828)
ClosedPublic

Authored by taapas1128 on Jan 4 2019, 10:07 AM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG79f8f032c706: amend: add config option to update time to current in hg amend (issue5828)
rHGec51a430ec51: amend: added config option to update time to current in hg amend(issue5828)
Summary

The given config option i.e. rewrite.update-timestamp updates date to current when True. However when only date is to be updated to current with the working directory clean and no other attributes changing then it does not amend as stated in issue 5828. Further when --date flag is specified along with the new config option then --date is given priority over the config option.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

taapas1128 created this revision.Jan 4 2019, 10:07 AM
pulkit added a subscriber: pulkit.Jan 4 2019, 10:36 AM
pulkit added inline comments.
mercurial/cmdutil.py
2548

If you move this change after the if statement at line 2565 on this side and drop changes to that if, it should work, right?

taapas1128 added a comment.Jan 4 2019, 10:56 AM

no the date is updated in 2560. So below that it wont work

pulkit added a comment.Jan 4 2019, 11:33 AM
In D5491#81291, @taapas1128 wrote:

no the date is updated in 2560. So below that it wont work

I overlooked that, thanks for pointing.

test-check-code.t and test-check-commit.t says hi!

Also, the test output is flaky since dateutil.makedate() returns time.time() which always changes, this also leads to hash being changed in test output. You should glob time and hash I think.
test-amend.t has two cases namely obsstore-on and obsstore-off. Output of both should be updated. You can do #if obsstore-on else endif kind of thing which is done in rest of the test.

Also, please run all the tests.

mercurial/cmdutil.py
2445

Let's move the date changing logic here, it will be cleaner.

taapas1128 retitled this revision from amend:added config option to update time to current in hg amend(issue5828) to amend: added config option to update time to current in hg amend (issue5828).Jan 4 2019, 2:43 PM
taapas1128 updated this revision to Diff 13010.
taapas1128 marked an inline comment as done.EditedJan 4 2019, 2:57 PM

@pulkit dealt with test-check-code.t and test-check-commit.t . And dealt with test-amend.t by making mockmakedate() as in test-journal.t . I have run all the tests. Please review.

pulkit added a comment.Jan 5 2019, 2:44 PM

Nice work! Can you update description of this patch with details in commit message about behavior of the new config option and how it solves the given issue?

taapas1128 edited the summary of this revision. (Show Details)Jan 5 2019, 3:30 PM
taapas1128 retitled this revision from amend: added config option to update time to current in hg amend (issue5828) to amend: added config option to update time to current and avoid update when other attribs are unchanged (issue5828).Jan 5 2019, 3:37 PM
taapas1128 updated this revision to Diff 13012.
taapas1128 edited the summary of this revision. (Show Details)Jan 5 2019, 3:47 PM
taapas1128 retitled this revision from amend: added config option to update time to current and avoid update when other attribs are unchanged (issue5828) to amend: added config option to update time to current in hg amend(issue5828).
taapas1128 updated this revision to Diff 13013.
taapas1128 added a comment.Jan 5 2019, 3:47 PM

@pulkit done . Please review.

pulkit accepted this revision.Jan 7 2019, 6:14 AM

Queueing this, many thanks!

I have left some inline comments, can you follow-up on them?

mercurial/cmdutil.py
2447

This one can be simplified as if ui.configbool(..) and not opts.get('date').

2573–2576

IIUC, this one can be if (date ==old.date() or not opts.get('date')):

Closed by commit rHGec51a430ec51: amend: added config option to update time to current in hg amend(issue5828) (authored by taapas1128). · Explain WhyJan 7 2019, 6:18 AM
This revision was automatically updated to reflect the committed changes.
taapas1128 added a comment.EditedJan 7 2019, 7:05 AM

Thanks for queueing. I will make the follow up patch for the inline comments that will make it more concise.

Revision Contents
Changeset List

PathPackages
Mmercurial/cmdutil.py (12 lines)
Mmercurial/configitems.py (3 lines)
Mmercurial/help/config.txt (7 lines)
Mtests/test-amend.t (42 lines)

Diff 13006

mercurial/cmdutil.py

# Update extra dict from amended commit (e.g. to preserve graft # Update extra dict from amended commit (e.g. to preserve graft
# source) # source)
extra.update(old.extra()) extra.update(old.extra())
# Also update it from the from the wctx # Also update it from the from the wctx
extra.update(wctx.extra()) extra.update(wctx.extra())
user = opts.get('user') or old.user() user = opts.get('user') or old.user()
date = opts.get('date') or old.date() date = opts.get('date') or old.date()
pulkitUnsubmitted
Done

Let's move the date changing logic here, it will be cleaner.

pulkit: Let's move the date changing logic here, it will be cleaner.
# Parse the date to allow comparison between date and old.date() # Parse the date to allow comparison between date and old.date()
pulkitUnsubmitted
Not Done

This one can be simplified as if ui.configbool(..) and not opts.get('date').

pulkit: This one can be simplified as `if ui.configbool(..) and not opts.get('date')`.
date = dateutil.parsedate(date) date = dateutil.parsedate(date)
if len(old.parents()) > 1: if len(old.parents()) > 1:
# ctx.files() isn't reliable for merges, so fall back to the # ctx.files() isn't reliable for merges, so fall back to the
# slower repo.status() method # slower repo.status() method
files = set([fn for st in base.status(old)[:3] files = set([fn for st in base.status(old)[:3]
for fn in st]) for fn in st])
else: else:
if not message: if not message:
editor = getcommiteditor(edit=True, editform=editform) editor = getcommiteditor(edit=True, editform=editform)
message = old.description() message = old.description()
pureextra = extra.copy() pureextra = extra.copy()
extra['amend_source'] = old.hex() extra['amend_source'] = old.hex()
if ui.configbool('rewrite','update-timestamp'):
pulkitUnsubmitted
Not Done

If you move this change after the if statement at line 2565 on this side and drop changes to that if, it should work, right?

pulkit: If you move this change after the `if` statement at line 2565 on this side and drop changes to…
if opts.get('date'):
pass
else:
date = dateutil.makedate()
new = context.memctx(repo, new = context.memctx(repo,
parents=[base.node(), old.p2().node()], parents=[base.node(), old.p2().node()],
text=message, text=message,
files=files, files=files,
filectxfn=filectxfn, filectxfn=filectxfn,
user=user, user=user,
date=date, date=date,
extra=extra, extra=extra,
editor=editor) editor=editor)
newdesc = changelog.stripdesc(new.description()) newdesc = changelog.stripdesc(new.description())
if ((not changes) if ((not changes)
and newdesc == old.description() and newdesc == old.description()
and user == old.user() and user == old.user()
and date == old.date()
and pureextra == old.extra()): and pureextra == old.extra()):
# nothing changed. continuing here would create a new node # nothing changed. continuing here would create a new node
# anyway because of the amend_source noise. # anyway because of the amend_source noise.
# #
# This not what we expect from amend. # This not what we expect from amend.
if (date == old.date() or
(ui.configbool('rewrite','update-timestamp') and
not opts.get('date'))):
return old.node() return old.node()
pulkitUnsubmitted
Not Done

IIUC, this one can be if (date ==old.date() or not opts.get('date')):

pulkit: IIUC, this one can be `if (date ==old.date() or not opts.get('date')):`
commitphase = None commitphase = None
if opts.get('secret'): if opts.get('secret'):
commitphase = phases.secret commitphase = phases.secret
newid = repo.commitctx(new) newid = repo.commitctx(new)
# Reroute the working copy parent to the new changeset # Reroute the working copy parent to the new changeset
repo.setparents(newid, nullid) repo.setparents(newid, nullid)

mercurial/configitems.py

default=0.1, default=0.1,
) )
coreconfigitem('progress', 'width', coreconfigitem('progress', 'width',
default=dynamicdefault, default=dynamicdefault,
) )
coreconfigitem('push', 'pushvars.server', coreconfigitem('push', 'pushvars.server',
default=False, default=False,
) )
coreconfigitem('rewrite', 'update-timestamp',
default=False,
)
coreconfigitem('storage', 'new-repo-backend', coreconfigitem('storage', 'new-repo-backend',
default='revlogv1', default='revlogv1',
) )
coreconfigitem('storage', 'revlog.optimize-delta-parent-choice', coreconfigitem('storage', 'revlog.optimize-delta-parent-choice',
default=True, default=True,
alias=[('format', 'aggressivemergedeltas')], alias=[('format', 'aggressivemergedeltas')],
) )
coreconfigitem('server', 'bookmarks-pushkey-compat', coreconfigitem('server', 'bookmarks-pushkey-compat',

mercurial/help/config.txt

Default to False, when True allow creating divergence when performing Default to False, when True allow creating divergence when performing
rebase of obsolete changesets. rebase of obsolete changesets.
``revsetalias`` ``revsetalias``
--------------- ---------------
Alias definitions for revsets. See :hg:`help revsets` for details. Alias definitions for revsets. See :hg:`help revsets` for details.
``rewrite``
-----------
``update-timestamp``
If true updates the date and time of the changeset to current.It is only
applicable for hg amend in current version.
``storage`` ``storage``
----------- -----------
Control the strategy Mercurial uses internally to store history. Options in this Control the strategy Mercurial uses internally to store history. Options in this
category impact performance and repository size. category impact performance and repository size.
``revlog.optimize-delta-parent-choice`` ``revlog.optimize-delta-parent-choice``
When storing a merge revision, both parents will be equally considered as When storing a merge revision, both parents will be equally considered as

tests/test-amend.t

$ cat << EOF >> $HGRCPATH $ cat << EOF >> $HGRCPATH
> [ui] > [ui]
> history-editing-backup = False > history-editing-backup = False
> EOF > EOF
$ echo fixed > b $ echo fixed > b
$ hg amend $ hg amend
#endif #endif
When updatetimestamp is False
$ hg amend --date '1997-1-1 0:1'
$ hg log --limit 1
changeset: 1:771a1be14d80
tag: tip
user: test
date: Wed Jan 01 00:01:00 1997 +0000
summary: b
When update-timestamp is True and no other change than the date
$ hg amend --config rewrite.update-timestamp=True
nothing changed
[1]
$ hg log --limit 1
changeset: 1:771a1be14d80
tag: tip
user: test
date: Wed Jan 01 00:01:00 1997 +0000
summary: b
When update-timestamp is True and there is other change than the date
$ hg amend --user foobar --config rewrite.update-timestamp=True
$ hg log --limit 1
changeset: 1:9713399c1302
tag: tip
user: foobar
date: Fri Jan 04 15:00:37 2019 +0000
summary: b
When date option is applicable and update-timestamp is True
$ hg amend --date '1998-1-1 0:1' --config rewrite.update-timestamp=True
$ hg log --limit 1
changeset: 1:5cec47d99566
tag: tip
user: foobar
date: Thu Jan 01 00:01:00 1998 +0000
summary: b