This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8223

merge: respect ui.relative-paths for some warning messages
AbandonedPublic

Authored by martinvonz on Mar 5 2020, 2:58 AM.

Download Raw Diff

Details

Reviewers

marmoute

Group Reviewers

hg-reviewers

Summary

I didn't bother adding tests. It doesn't seem worth having tests for
every place we use getuipathfn(). I've tested it manually.

Diff Detail

Repository

rHG Mercurial

Branch

default

Lint

No Linters Available

Unit

No Unit Test Coverage

Event Timeline

martinvonz created this revision.Mar 5 2020, 2:58 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptMar 5 2020, 2:58 AM

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

Adding test for this would be better. It clarify the intend and prevent regression.

This revision now requires changes to proceed.Mar 5 2020, 3:32 AM

In D8223#122355, @marmoute wrote:

Adding test for this would be better. It clarify the intend and prevent regression.

Tests for each of the strings?

I think we are talking about a handful of different string. Are we not?

In D8223#122398, @marmoute wrote:

I think we are talking about a handful of different string. Are we not?

Yes, but I trust the uipathfn to work as it does elsewhere. I'll let reviewers decide to take this as is or not. Otherwise I'll just abandon it.

I am confused. Why can't we get a handful of case added to the test ? I assume these message are alredy tested and would be easy to retest.

Chatted with Raphaël to try to understand where a misunderstanding could lie here.

So to clarify this code path is never tested for in the relative case, my issue is not with this change, but with the lack of testing of these relative path altogether. Without testing of the relative case, any future code update discarding the uipathfn call would silently regress your fix.

In D8223#122410, @marmoute wrote:

Chatted with Raphaël to try to understand where a misunderstanding could lie here.
So to clarify this code path is never tested for in the relative case, my issue is not with this change, but with the lack of testing of these relative path altogether. Without testing of the relative case, any future code update discarding the uipathfn call would silently regress your fix.

Yes, that's correct.

Okay, then we should add tests for the relative case to make sure we do not regress these in the future. Can you do this ?

In D8223#122412, @marmoute wrote:

Okay, then we should add tests for the relative case to make sure we do not regress these in the future. Can you do this ?

Ah, I think I now understand where the confusion comes from. I initially said "I trust the uipathfn to work as it does elsewhere.", but that was only half the truth -- the other half is that I spotted this little bug and decided to fix it, but I don't really care much about it. Sorry I wasn't clear about that part. So I don't care enough to write tests with relative paths for theses cases. (I also don't care enough to add retroactive tests for D5916, D5936, D5937, D5939, D6264, and probably many more. And I definitely care enough to rewrite all our tests to be have more directories and for commands to be run in subdirectories, even though that would have been nice to have.) I'm happy to add tests if someone else writes them, though.

And I definitely care enough to rewrite all our tests to be have more directories and for commands to be run in subdirectories, even though that would have been nice to have.) I'm happy to add tests if someone else writes them, though.

I don't understand that part, is there some negation missing?

In D8223#122425, @marmoute wrote:

And I definitely care enough to rewrite all our tests to be have more directories and for commands to be run in subdirectories, even though that would have been nice to have.) I'm happy to add tests if someone else writes them, though.

I don't understand that part, is there some negation missing?

Oops, should have been "And I *don't* definitely care enough" :P

Since forever, the option relative path option never worked with this output. So, no user should be "surprised" if it don't. However if you fix it people may start to rely on it. Without tests it can regress, breaking user expectation.

I understant this is a side-quest cleanup for you, but I woudl rather see this slow cleanup advance twice as slow with test that at the current pace without tests.

In D8223#122728, @marmoute wrote:

Since forever, the option relative path option never worked with this output. So, no user should be "surprised" if it don't. However if you fix it people may start to rely on it. Without tests it can regress, breaking user expectation.
I understant this is a side-quest cleanup for you, but I woudl rather see this slow cleanup advance twice as slow with test that at the current pace without tests.

nit: s/twice/10 times/

Fine, I am sure you will speed up with time. (I assume all these message are already tested, changing the pwd from where the command run should not be insane)

In D8223#122733, @marmoute wrote:

Fine, I am sure you will speed up with time. (I assume all these message are already tested, changing the pwd from where the command run should not be insane)

I've said before that I'll let other reviewers decide if they want the patch or if I should abandon it. Talking about it more just makes me tired.

martinvonz abandoned this revision.Mar 13 2020, 1:17 PM

Revision Contents
Changeset List

			Path	Packages
M			mercurial/merge.py (10 lines)

Commit	Parents	Author	Summary	Date
0d637c3bf829	5a5aee8ba980	Martin von Zweigbergk		Mar 5 2020, 1:45 AM

Diff 20492

mercurial/merge.py

	ACTION_EXEC,			ACTION_EXEC,
	ACTION_REMOVE,			ACTION_REMOVE,
	ACTION_PATH_CONFLICT_RESOLVE,			ACTION_PATH_CONFLICT_RESOLVE,
	):			):
	msg = _(b"conflicting changes")			msg = _(b"conflicting changes")
	hint = _(b"commit or update --clean to discard changes")			hint = _(b"commit or update --clean to discard changes")
	raise error.Abort(msg, hint=hint)			raise error.Abort(msg, hint=hint)

				uipathfn = scmutil.getuipathfn(repo)

	# Prompt and create actions. Most of this is in the resolve phase			# Prompt and create actions. Most of this is in the resolve phase
	# already, but we can't handle .hgsubstate in filemerge or			# already, but we can't handle .hgsubstate in filemerge or
	# subrepoutil.submerge yet so we have to keep prompting for it.			# subrepoutil.submerge yet so we have to keep prompting for it.
	if b'.hgsubstate' in actionbyfile:			if b'.hgsubstate' in actionbyfile:
	f = b'.hgsubstate'			f = b'.hgsubstate'
	m, args, msg = actionbyfile[f]			m, args, msg = actionbyfile[f]
	prompts = filemerge.partextras(labels)			prompts = filemerge.partextras(labels)
	prompts[b'f'] = f			prompts[b'f'] = uipathfn(f)
	if m == ACTION_CHANGED_DELETED:			if m == ACTION_CHANGED_DELETED:
	if repo.ui.promptchoice(			if repo.ui.promptchoice(
	_(			_(
	b"local%(l)s changed %(f)s which other%(o)s deleted\n"			b"local%(l)s changed %(f)s which other%(o)s deleted\n"
	b"use (c)hanged version or (d)elete?"			b"use (c)hanged version or (d)elete?"
	b"$$ &Changed $$ &Delete"			b"$$ &Changed $$ &Delete"
	)			)
	% prompts,			% prompts,

	# divergent renames			# divergent renames
	for f, fl in sorted(pycompat.iteritems(diverge)):			for f, fl in sorted(pycompat.iteritems(diverge)):
	repo.ui.warn(			repo.ui.warn(
	_(			_(
	b"note: possible conflict - %s was renamed "			b"note: possible conflict - %s was renamed "
	b"multiple times to:\n"			b"multiple times to:\n"
	)			)
	% f			% uipathfn(f)
	)			)
	for nf in sorted(fl):			for nf in sorted(fl):
	repo.ui.warn(b" %s\n" % nf)			repo.ui.warn(b" %s\n" % nf)

	# rename and delete			# rename and delete
	for f, fl in sorted(pycompat.iteritems(renamedelete)):			for f, fl in sorted(pycompat.iteritems(renamedelete)):
	repo.ui.warn(			repo.ui.warn(
	_(			_(
	b"note: possible conflict - %s was deleted "			b"note: possible conflict - %s was deleted "
	b"and renamed to:\n"			b"and renamed to:\n"
	)			)
	% f			% uipathfn(f)
	)			)
	for nf in sorted(fl):			for nf in sorted(fl):
	repo.ui.warn(b" %s\n" % nf)			repo.ui.warn(b" %s\n" % uipathfn(nf))

	### apply phase			### apply phase
	if not branchmerge: # just jump to the new rev			if not branchmerge: # just jump to the new rev
	fp1, fp2, xp1, xp2 = fp2, nullid, xp2, b''			fp1, fp2, xp1, xp2 = fp2, nullid, xp2, b''
	# If we're doing a partial update, we need to skip updating			# If we're doing a partial update, we need to skip updating
	# the dirstate.			# the dirstate.
	always = matcher is None or matcher.always()			always = matcher is None or matcher.always()
	updatedirstate = updatedirstate and always and not wc.isinmemory()			updatedirstate = updatedirstate and always and not wc.isinmemory()

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	20492			Mar 5 2020, 2:58 AM	★	★