Patchwork D4876: amend: add config to skip amend if only date is changed (issue5828)

login
register
mail settings
Submitter phabricator
Date Oct. 3, 2018, 10:33 p.m.
Message ID <differential-rev-PHID-DREV-l3nb433jic223xgcqw7q-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/35431/
State New
Headers show

Comments

phabricator - Oct. 3, 2018, 10:33 p.m.
Zharaskhan created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  When --date flag specified with `hg amend` and working copy is clean,
  `hg amend` amends, which is generally undesirable behavior.
  But it is commonly used to change date on each amend.
  I added experimental.amend.dateonly config option to not amend
  if only thing that changed is date.
  I also added tests for this.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D4876

AFFECTED FILES
  mercurial/cmdutil.py
  mercurial/configitems.py
  tests/test-amend.t

CHANGE DETAILS




To: Zharaskhan, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 4, 2018, 7:10 p.m.
pulkit accepted this revision.
pulkit added a comment.


  This looks good to me but since I helped Zharas in preparing the patch, I won't queue it.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D4876

To: Zharaskhan, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel
phabricator - Oct. 8, 2018, 5:13 a.m.
martinvonz added a comment.


  I'm not sure how I feel about this. I think we talked within the team at Google about always passing a `-D now` and decided not to do it because of the issue you're fixing here (I may be confusing the discussion with one about using `hg metaedit`). Still, I feel like this patch is a workaround for the real problem. I assume that the real problem is that there the date is not normally updated when you amend a commit (i.e. you want to always pass `-D now`, not a specific other timestamp). If that's the problem we're trying address, should we instead introduce a config called something like `amend.updatetimestamp` that makes it update the timestamp to the current time? That would naturally not update the timestamp if the timestamp was the only thing that changed (just like this patch does it). The natural generalization of `amend.updatetimestamp` is something like `rewrite.updatetimestamp` that would be respected by amend, histedit, rebase, etc.
  
  What do you think? Is that the use case you're trying to address with this patch?

INLINE COMMENTS

> configitems.py:440-442
> +coreconfigitem('experimental', 'amend.dateonly',
> +    default=False,
> +)

"amend.dateonly" sounds like it only updates the date. Maybe "amend.skipdateonly" or "amend.nodateonly"? It doesn't matter much because we'll need to update it anyway later if we drop the "experimental" label.

> test-amend.t:372-386
> +  $ hg export
> +  # HG changeset patch
> +  # User test
> +  # Date 315532860 0
> +  #      Tue Jan 01 00:01:00 1980 +0000
> +  # Node ID 67c8ed970566d909b4759ad6db6048ea9080743b
> +  # Parent  cb9a9f314b8b07ba71012fcdbc544b5a4d82ff5b

You only care about the date here, right? You can probably do something like `hg log -T '{date|isodate}\n'` to make the test clearer.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D4876

To: Zharaskhan, #hg-reviewers, pulkit
Cc: martinvonz, pulkit, mercurial-devel
phabricator - Oct. 8, 2018, 4:03 p.m.
pulkit added a comment.


  (Replying because I adviced Zharas to take up this issue and he is a new mercurial user and have less context on the problem)
  
  In https://phab.mercurial-scm.org/D4876#74017, @martinvonz wrote:
  
  > I'm not sure how I feel about this. I think we talked within the team at Google about always passing a `-D now` and decided not to do it because of the issue you're fixing here (I may be confusing the discussion with one about using `hg metaedit`). Still, I feel like this patch is a workaround for the real problem. I assume that the real problem is that there the date is not normally updated when you amend a commit (i.e. you want to always pass `-D now`, not a specific other timestamp). If that's the problem we're trying address, should we instead introduce a config called something like `amend.updatetimestamp` that makes it update the timestamp to the current time? That would naturally not update the timestamp if the timestamp was the only thing that changed (just like this patch does it). The natural generalization of `amend.updatetimestamp` is something like `rewrite.updatetimestamp` that would be respected by amend, histedit, rebase, etc.
  >
  > What do you think? Is that the use case you're trying to address with this patch?
  
  
  I agree that the real problem is amend not updating the timestamp. People have aliased `amend` to `amend -D now` or just like you said, they always pass `-D now`. In such cases, even if the working directory is not changed, we end up creating a new commit with just date change. This patch tries to solve the problem of creating a new commit when only date has changed for people who do `amend -D now` always by preventing that if the config option is turned on.
  
  I like your idea of having a config option which is respected by all the commands.
  
  That said, will you like to see a v2 of this patch with inline comments addressed or we abandon this idea in favour of proposed config option to change the date.

INLINE COMMENTS

> martinvonz wrote in configitems.py:440-442
> "amend.dateonly" sounds like it only updates the date. Maybe "amend.skipdateonly" or "amend.nodateonly"? It doesn't matter much because we'll need to update it anyway later if we drop the "experimental" label.

"amend.skipdateonly" sounds good.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D4876

To: Zharaskhan, #hg-reviewers, pulkit
Cc: martinvonz, pulkit, mercurial-devel
phabricator - Oct. 8, 2018, 4:17 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D4876#74021, @pulkit wrote:
  
  > (Replying because I adviced Zharas to take up this issue and he is a new mercurial user and have less context on the problem)
  >
  > In https://phab.mercurial-scm.org/D4876#74017, @martinvonz wrote:
  >
  > > I'm not sure how I feel about this. I think we talked within the team at Google about always passing a `-D now` and decided not to do it because of the issue you're fixing here (I may be confusing the discussion with one about using `hg metaedit`). Still, I feel like this patch is a workaround for the real problem. I assume that the real problem is that there the date is not normally updated when you amend a commit (i.e. you want to always pass `-D now`, not a specific other timestamp). If that's the problem we're trying address, should we instead introduce a config called something like `amend.updatetimestamp` that makes it update the timestamp to the current time? That would naturally not update the timestamp if the timestamp was the only thing that changed (just like this patch does it). The natural generalization of `amend.updatetimestamp` is something like `rewrite.updatetimestamp` that would be respected by amend, histedit, rebase, etc.
  > >
  > > What do you think? Is that the use case you're trying to address with this patch?
  >
  >
  > I agree that the real problem is amend not updating the timestamp. People have aliased `amend` to `amend -D now` or just like you said, they always pass `-D now`. In such cases, even if the working directory is not changed, we end up creating a new commit with just date change. This patch tries to solve the problem of creating a new commit when only date has changed for people who do `amend -D now` always by preventing that if the config option is turned on.
  >
  > I like your idea of having a config option which is respected by all the commands.
  >
  > That said, will you like to see a v2 of this patch with inline comments addressed or we abandon this idea in favour of proposed config option to change the date.
  
  
  I should be easy to introduce such an option and make amend respect it (similar amount of code as this patch), so I'd just like to hear if others think it's a good idea first. If others agree with it, then it seems better to introduce the generic option right away. I can't/won't access IRC right now, but feel free to check there what others think and update this review when you have.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D4876

To: Zharaskhan, #hg-reviewers, pulkit
Cc: martinvonz, pulkit, mercurial-devel
Yuya Nishihara - Oct. 9, 2018, 12:21 p.m.
>   I should be easy to introduce such an option and make amend respect it (similar amount of code as this patch), so I'd just like to hear if others think it's a good idea first. If others agree with it, then it seems better to introduce the generic option right away. I can't/won't access IRC right now, but feel free to check there what others think and update this review when you have.

SGTM.

Perhaps, `ui.history-editing-backup` can be moved under the same config group.
phabricator - Oct. 9, 2018, 12:21 p.m.
yuja added a comment.


  >   I should be easy to introduce such an option and make amend respect it (similar amount of code as this patch), so I'd just like to hear if others think it's a good idea first. If others agree with it, then it seems better to introduce the generic option right away. I can't/won't access IRC right now, but feel free to check there what others think and update this review when you have.
  
  SGTM.
  
  Perhaps, `ui.history-editing-backup` can be moved under the same config group.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D4876

To: Zharaskhan, #hg-reviewers, pulkit
Cc: yuja, martinvonz, pulkit, mercurial-devel
phabricator - Oct. 14, 2018, 3:31 p.m.
durin42 requested changes to this revision.
durin42 added a comment.
This revision now requires changes to proceed.


  It sounds like @pulkit has some ideas on what needs to be done follow-up wise.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D4876

To: Zharaskhan, #hg-reviewers, pulkit, durin42
Cc: durin42, yuja, martinvonz, pulkit, mercurial-devel

Patch

diff --git a/tests/test-amend.t b/tests/test-amend.t
--- a/tests/test-amend.t
+++ b/tests/test-amend.t
@@ -365,3 +365,40 @@ 
   $ hg amend
 
 #endif
+
+  $ hg status
+  $ hg diff
+  $ hg amend --date '1980-1-1 0:1'
+  $ hg export
+  # HG changeset patch
+  # User test
+  # Date 315532860 0
+  #      Tue Jan 01 00:01:00 1980 +0000
+  # Node ID 67c8ed970566d909b4759ad6db6048ea9080743b
+  # Parent  cb9a9f314b8b07ba71012fcdbc544b5a4d82ff5b
+  b
+  
+  diff --git a/b b/b
+  new file mode 100644
+  --- /dev/null
+  +++ b/b
+  @@ -0,0 +1,1 @@
+  +fixed
+  $ hg amend --date '1990-1-1 0:1' --config experimental.amend.dateonly=True
+  nothing changed
+  [1]
+  $ hg export
+  # HG changeset patch
+  # User test
+  # Date 315532860 0
+  #      Tue Jan 01 00:01:00 1980 +0000
+  # Node ID 67c8ed970566d909b4759ad6db6048ea9080743b
+  # Parent  cb9a9f314b8b07ba71012fcdbc544b5a4d82ff5b
+  b
+  
+  diff --git a/b b/b
+  new file mode 100644
+  --- /dev/null
+  +++ b/b
+  @@ -0,0 +1,1 @@
+  +fixed
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -437,6 +437,9 @@ 
 coreconfigitem('email', 'to',
     default=None,
 )
+coreconfigitem('experimental', 'amend.dateonly',
+    default=False,
+)
 coreconfigitem('experimental', 'archivemetatemplate',
     default=dynamicdefault,
 )
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2558,13 +2558,17 @@ 
         if ((not changes)
             and newdesc == old.description()
             and user == old.user()
-            and date == old.date()
             and pureextra == old.extra()):
-            # nothing changed. continuing here would create a new node
-            # anyway because of the amend_source noise.
-            #
-            # This not what we expect from amend.
-            return old.node()
+
+            # If the date is same or
+            # the user doesn't want to create a date only change amend
+            if date == old.date() or ui.configbool('experimental',
+                                                   'amend.dateonly'):
+                # nothing changed. continuing here would create a new node
+                # anyway because of the amend_source noise.
+                #
+                # This not what we expect from amend.
+                return old.node()
 
         commitphase = None
         if opts.get('secret'):