This is an archive of the discontinued Mercurial Phabricator instance.

salvaged: record salvaged in ChangingFiles at commit time
ClosedPublic

Authored by marmoute on Sep 29 2020, 6:37 AM.

Download Raw Diff

Details

Reviewers

Alphare
pulkit

Group Reviewers

hg-reviewers

Commits

rHG64a4b85c4a00: salvaged: record salvaged in ChangingFiles at commit time
rHG751d9436141b: salvaged: record salvaged in ChangingFiles at commit time
rHG8d6d7040ac47: salvaged: record salvaged in ChangingFiles at commit time

Summary

The new code is a simple but effective way to detect this information. We might
be able to move it inside the various conditionnal above, but I want to focus
on simplicity until we have a full working stack.

It is worth noting that if we record the information in the ChangingFiles
object, it is not persisted yet. This will comes with later changesets.

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

marmoute created this revision.Sep 29 2020, 6:37 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptSep 29 2020, 6:37 AM

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

marmoute added a child revision: D9123: changing-files: split the changing files computation from encoding.Sep 30 2020, 9:10 AM

marmoute updated this revision to Diff 22942.

Other than my comment, the logic seems sound.

mercurial/commit.py
150	Coming from a Rust development perspective, `_stateextras` would be a private method and you wouldn't be able to access it here. I've seen a lot of "abstraction leakage" in the Python codebase, so I'm wondering if this is justified, as in "this is a temporary solution and will disappear in a few patches or "this would require a huge refactor otherwise", or even "this should be a public attribute".

marmoute added inline comments.Oct 2 2020, 8:59 AM

mercurial/commit.py
150	This is the junction of my work and Pulkit work. My plan here is to first make it work so that we can see what data we need when. From there, we can cleanup the code and add the necessary API.

Alphare added inline comments.Oct 2 2020, 9:08 AM

mercurial/commit.py
150	Sounds good

pulkit added a subscriber: pulkit.Oct 2 2020, 9:12 AM

pulkit added inline comments.

mercurial/commit.py
150	Yes, I need to make a public function/attribute for reading all extras of a mergestate.

pulkit accepted this revision.Oct 7 2020, 8:05 AM

This revision is now accepted and ready to land.Oct 7 2020, 8:05 AM

marmoute added a commit: rHG8d6d7040ac47: salvaged: record salvaged in ChangingFiles at commit time.Oct 8 2020, 11:23 AM

Closed by commit rHG8d6d7040ac47: salvaged: record salvaged in ChangingFiles at commit time (authored by marmoute). · Explain Why

This revision was automatically updated to reflect the committed changes.

marmoute added a commit: rHG751d9436141b: salvaged: record salvaged in ChangingFiles at commit time.Oct 8 2020, 12:17 PM

marmoute added a commit: rHG64a4b85c4a00: salvaged: record salvaged in ChangingFiles at commit time.Oct 9 2020, 2:28 AM

Revision Contents
Changeset List

			Path	Packages
M			mercurial/commit.py (13 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	22915		Sep 29 2020, 6:37 AM	★	★
Diff 2	22942		Sep 30 2020, 9:10 AM	★	★
Diff 3	23086	rHG8d6d7040ac479845537278611349c970d7d910bd	Sep 14 2020, 5:47 PM	★	★

Status	Author	Revision
Closed	marmoute	D9141 copies: move `merged` testing sooner
Closed	marmoute	D9140 copies: return None instead of ChangingFiles when relevant
Closed	marmoute	D9139 copies: add a HASCOPIESINFO flag to highlight rev with useful data
Closed	marmoute	D9130 salvaged: properly deal with salvaged file during copy tracing
Closed	marmoute	D9129 salvaged: persist the salvaged set on disk
Closed	marmoute	D9128 changing-files: add clean computation of changed file for merges
Closed	marmoute	D9127 changing-files: add clean computation of changed files for linear changesets
Closed	marmoute	D9126 changing-files: add clean computation of changed files for roots
Closed	marmoute	D9125 changing-files: add a debug command display changed files
Closed	marmoute	D9124 side-data: add a test to check sidedata upgrade
Closed	marmoute	D9123 changing-files: split the changing files computation from encoding
Closed	marmoute	D9120 salvaged: record salvaged in ChangingFiles at commit time
Closed	marmoute	D9119 salvaged: track removal-candidates in more cases
Closed	marmoute	D9122 salvaged: explicitly skip salvaged file while encoding
Closed	marmoute	D9118 changing-files: add a "salvaged" set to track file that were not removed
Closed	marmoute	D9117 copies: directly pass a changes object to the copy tracing code
Closed	marmoute	D9116 copies: no longer change the sidedata flag
Closed	marmoute	D9115 copies: use dedicated `_revinfo_getter` function and call
Closed	marmoute	D9114 copies: make two version of the changeset centric algorithm
Closed	marmoute	D9113 copies: rename some function to the new naming scheme
Closed	marmoute	D9112 changing-files: cache the various property
Closed	marmoute	D9111 changing-files: always use `mark_touched` to update the touched set
Closed	marmoute	D9092 changing-files: retrieve changelogrevision.files from the sidedata block
Closed	marmoute	D9091 changing-files: drop the now useless changelogrevision argument
Closed	marmoute	D9090 changing-files: rework the way we store changed files in side-data
Closed	marmoute	D9143 changing-files: fix docstring

Diff 23086

mercurial/commit.py

	origfiles = origctx.files()			origfiles = origctx.files()
	assert files.touched.issubset(origfiles)			assert files.touched.issubset(origfiles)
	files.update_touched(origfiles)			files.update_touched(origfiles)

	if writechangesetcopy:			if writechangesetcopy:
	files.update_copies_from_p1(ctx.p1copies())			files.update_copies_from_p1(ctx.p1copies())
	files.update_copies_from_p2(ctx.p2copies())			files.update_copies_from_p2(ctx.p2copies())

				copy_sd = ctx.repo().filecopiesmode == b'changeset-sidedata'
				if copy_sd and len(ctx.parents()) > 1:
				# XXX this `mergestate.read` could be duplicated with a the merge state
				# reading in _process_files So we could refactor further to reuse it in
				# some cases.
				ms = mergestate.mergestate.read(repo)
				if ms.active():
				for fname in sorted(ms._stateextras.keys()):
				AlphareUnsubmitted Not Done Coming from a Rust development perspective, `_stateextras` would be a private method and you wouldn't be able to access it here. I've seen a lot of "abstraction leakage" in the Python codebase, so I'm wondering if this is justified, as in "this is a temporary solution and will disappear in a few patches or "this would require a huge refactor otherwise", or even "this should be a public attribute". Alphare: Coming from a Rust development perspective, `_stateextras` would be a private method and you…
				marmouteAuthorUnsubmitted Done This is the junction of my work and Pulkit work. My plan here is to first make it work so that we can see what data we need when. From there, we can cleanup the code and add the necessary API. marmoute: This is the junction of my work and Pulkit work. My plan here is to first make it work so that…
				AlphareUnsubmitted Not Done Sounds good Alphare: Sounds good
				pulkitUnsubmitted Not Done Yes, I need to make a public function/attribute for reading all extras of a mergestate. pulkit: Yes, I need to make a public function/attribute for reading all extras of a mergestate.
				might_removed = ms.extras(fname).get(b'merge-removal-candidate')
				if might_removed == b'yes':
				if fname in ctx:
				files.mark_salvaged(fname)

	return mn, files			return mn, files


	def _process_files(tr, ctx, error=False):			def _process_files(tr, ctx, error=False):
	repo = ctx.repo()			repo = ctx.repo()
	p1 = ctx.p1()			p1 = ctx.p1()
	p2 = ctx.p2()			p2 = ctx.p2()