This is an archive of the discontinued Mercurial Phabricator instance.

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

Authored by taapas1128 on Dec 25 2018, 8:45 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

taapas1128 created this revision.Dec 25 2018, 8:45 AM
yuja added a subscriber: yuja.Dec 26 2018, 8:02 AM

+#if updatetime option is turned on
+ $ hg amend --date '1997-1-1 0:1'

Use --config unless you need to re-run the whole tests with various options.

+ $ hg log --limit 2
+ # changeset: 40473:a092f9df5a43
+ # branch: stable
+ # bookmark: @
+ # tag: tip
+ # parent: 40406:7b48c616431d
+ # user: Taapas Agrawal<taapas2897@gmail.com>
+ # date: Wed Jan 01 00:01:00 1997 +0530
+ # summary: amend:Added current time update config option(issue5828)

Huh? What happened here?

+coreconfigitem('experimental', 'updatetime',
+ default=False,
+)

Martin showed a better name. rewrite.update-timestamp sounds good to me.
(I don't know which is a proper term, timestamp or time-stamp.)

https://phab.mercurial-scm.org/D4876#74017

+ update = ui.configbool('experimental','updatetime')
+ if update==True:
+ date=dateutil.parsedate(datetime.datetime.today().strftime("%Y-%m-%d %H:%M:%S"))

See commands.graft() for how to get a current timestamp.

In the tests I added two parts one with config option False and and another with True. I will correct the heading that part is with config turned off.
Okay I will change the config option to

rewrite.updatetimestamp

and will see how to get current timestamp from commands.graft() .

taapas1128 retitled this revision from amend:Added current time update config option(issue5828) to amend:Added rewrite.updatetimestamp config option(issue5828).Dec 26 2018, 9:02 AM
taapas1128 updated this revision to Diff 12979.

@yuja I have updated the revision . Please review.

taapas1128 retitled this revision from amend:Added rewrite.updatetimestamp config option(issue5828) to amend:added rewrite.updatetimestamp config option(issue5828).Dec 26 2018, 9:06 AM
yuja added a comment.Dec 27 2018, 7:12 AM

+$ hg amend --config rewrite.updatetimestamp=True
+$ hg log --limit 2
+changeset: 40473:e64479cbaf37

Okay, so these tests don't run because these lines don't start with $.

+coreconfigitem('rewrite', 'updatetimestamp',

Need dash per https://www.mercurial-scm.org/wiki/UIGuideline#config

And please update mercurial/help/config.txt as well.

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py

  • a/mercurial/cmdutil.py

+++ b/mercurial/cmdutil.py
@@ -2545,6 +2545,10 @@

pureextra = extra.copy()
extra['amend_source'] = old.hex()

+ updatetime = ui.configbool('rewrite','updatetimestamp')
+ if updatetime==True:

if updatetime: (or simply if ui.configbool(...):)

taapas1128 updated this revision to Diff 12987.Dec 27 2018, 8:47 AM

@yuja Corrected the tests. Added in config.txt and changed the config option according to the guideline. Please review when you are free.

@yuja Please review this

pulkit added a subscriber: pulkit.Jan 2 2019, 8:14 AM

Hi, thanks for the patch.

  1. After this patch, amend will set the date to current date even if --date is passed, which is not preferred. The correct order should be, --date if passed, else the current date if new config is set, else old date. Also the current date should only be set if there are more changes to amend.
  1. If I understand correctly, you are manually updating the tests. You should have a look at https://www.mercurial-scm.org/wiki/WritingTests#Running_the_test_suite.
  1. Please add more details in the commit message about the behavior of the new config option.
taapas1128 retitled this revision from amend:added rewrite.updatetimestamp config option(issue5828) to amend:added config option to update time to current in hg amend(issue5828).Jan 3 2019, 7:49 AM
taapas1128 updated this revision to Diff 12998.

@pulkit I have updated the revision. Sorry about the tests.Please review when you are free.

pulkit added a comment.Jan 4 2019, 5:44 AM

The patch is going in a good direction. We now need to make sure that we don't amend if date is the only thing which has changed.

tests/test-amend.t
382

This one should not amend as there are no other changes than the date change.

taapas1128 marked an inline comment as done.Jan 4 2019, 8:23 AM
taapas1128 updated this revision to Diff 13005.

@pulkit I have updated the revision accordingly. Please review

pulkit added a comment.Jan 4 2019, 8:39 AM

Nice! I left an inline comment.

How are you sending patches? I am unable to see the whole file in phabricator. If you are not using hg phabsend, please use that next time.

For how to use hg phabsend: https://www.mercurial-scm.org/wiki/Phabricator

mercurial/cmdutil.py
2559

How about if we leave this if statement as it is and do following after the if statement:

  • check whether --date is passed or not
  • if not, change the date to current date

nit: Return value of ui.configbool() is a boolean and you don't need to compare that tp True.

taapas1128 added a comment.EditedJan 4 2019, 9:41 AM

I think that would not be correct because we need to avoid date change update-timestamp is true and when no option is passed and no attribute is changed . Only when there is change in any of the attributes then only date is to be updated which can only be tested this way only.

oh i was using update diff button to update the patch . I will resend a new patch using

hg phabsend
taapas1128 abandoned this revision.Jan 4 2019, 2:58 PM