This is an archive of the discontinued Mercurial Phabricator instance.

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

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
2553

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?

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

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
2444

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.

@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
2446

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

2572–2575

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

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.