This is an archive of the discontinued Mercurial Phabricator instance.

amend: add config to skip amend if only date is changed (issue5828)
Needs RevisionPublic

Authored by Zharaskhan on Oct 3 2018, 6:33 PM.

Download Raw Diff

Details

Reviewers

pulkit
durin42

Group Reviewers

hg-reviewers

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.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

Zharaskhan created this revision.Oct 3 2018, 6:33 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptOct 3 2018, 6:33 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

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

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?

mercurial/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.
tests/test-amend.t
372–386	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.

(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 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.

mercurial/configitems.py
440–442	"amend.skipdateonly" sounds good.

In 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 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.

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.

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

This revision now requires changes to proceed.Oct 14 2018, 11:31 AM

Revision Contents
Changeset List

			Path	Packages
M			mercurial/cmdutil.py (16 lines)
M			mercurial/configitems.py (3 lines)
M			tests/test-amend.t (37 lines)

Commit	Parents	Author	Summary	Date
		Zharaskhan Aman		Oct 3 2018, 6:21 PM

Diff 11660

mercurial/cmdutil.py

	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()):

				# 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			# 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.
	return old.node()			return old.node()

	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='smtp',			default='smtp',
	)			)
	coreconfigitem('email', 'reply-to',			coreconfigitem('email', 'reply-to',
	default=None,			default=None,
	)			)
	coreconfigitem('email', 'to',			coreconfigitem('email', 'to',
	default=None,			default=None,
	)			)
				coreconfigitem('experimental', 'amend.dateonly',
				default=False,
				)
				martinvonzUnsubmitted Not Done "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. martinvonz: "amend.dateonly" sounds like it only updates the date. Maybe "amend.skipdateonly" or "amend.
				pulkitUnsubmitted Not Done "amend.skipdateonly" sounds good. pulkit: "amend.skipdateonly" sounds good.
	coreconfigitem('experimental', 'archivemetatemplate',			coreconfigitem('experimental', 'archivemetatemplate',
	default=dynamicdefault,			default=dynamicdefault,
	)			)
	coreconfigitem('experimental', 'bundle-phases',			coreconfigitem('experimental', 'bundle-phases',
	default=False,			default=False,
	)			)
	coreconfigitem('experimental', 'bundle2-advertise',			coreconfigitem('experimental', 'bundle2-advertise',
	default=True,			default=True,

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

				$ 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
				martinvonzUnsubmitted Not Done 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. martinvonz: You only care about the date here, right? You can probably do something like `hg log -T…
				$ 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	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	11660			Oct 3 2018, 6:32 PM	★	★