Thanks for the review. Did something go wrong in a rebase? I'm confused as to why there's two self.vfs.unlink calls in the diff displayed by phabricator.

In D6776#100506, @durin42 wrote:

In D6776#100487, @valentin.gatienbaron wrote:

I showed in D6847 the same change but implemented in exchange._processcompared. Tests pass.
I think it'd make for a simpler final state: with the current change, the client sees that the bookmark is going to move sideways, decides this is fine, requests that the server validates that the bookmark is indeed moving sideways (which it does), but in the end the server rejects the move. In the suggested change, the client sees that the bookmark is going sideways and rejects it. This should be consistent with the way new heads or new branches or diverging rewrites are prevented.

Isn't that a client-side change only though, so we still need functionality on the server to reject bad pushes? (I could be missing something.)

I showed in D6847 the same change but implemented in exchange._processcompared. Tests pass.
I think it'd make for a simpler final state: with the current change, the client sees that the bookmark is going to move sideways, decides this is fine, requests that the server validates that the bookmark is indeed moving sideways (which it does), but in the end the server rejects the move. In the suggested change, the client sees that the bookmark is going sideways and rejects it. This should be consistent with the way new heads or new branches or diverging rewrites are prevented.

(just interested in this change, I don't have any power to actually accept it)

Thanks! I didn't expect this would make it for 5.1.

In D6643#97247, @mharbison72 wrote:

The update worked for me, thanks.

In D6643#97171, @mharbison72 wrote:

Don't take this as a complete list, but it looks like repository.py, remotefilelog, eol, and probably keyword need to be taught the new kwarg. Note that the keyword wrapper is called kwcommitctx, so it isn't enough to grep for def commitctx.

@mharbison72 I dealt with the problem of convert in the previous commit. I didn't add a config option to disable the new code path, because it's really awkward to document, and I think it would be annoying to use if I made more fixes (the other case where files are spuriously listed, involving exec bits, probably has little impact on repo size/pull performance but it may be worth fixing anyway because it causes confusion). But if there are downsides to what I did, the config option is still on the table.

In D6613#96908, @mharbison72 wrote:

In D6613#96822, @valentin.gatienbaron wrote:

Ok. Maybe it would be simpler or more robust to do the direct thing: optionally treat the files list in the input commit as input and reuse them blindly in the resulting commit, when doing a hg->hg conversion without filemap.

Maybe. I've been wondering if it's possible to just pass along the manifest and changelog instead of recalculating it. It seems there have been other issues over the years[1]. @yuja fixed something manifest related around the time of that thread IIRC. Even if a filemap is in use, the map may not modify early commits. And when those are changed unexpectedly, it makes me wonder what got lost/mangled. I looked through the repos I converted last year without any file mapping, and there were manifest node changes, but also differences in files and files+ in the changelog.

Ok. Maybe it would be simpler or more robust to do the direct thing: optionally treat the files list in the input commit as input and reuse them blindly in the resulting commit, when doing a hg->hg conversion without filemap.
(btw, a tweak to test-merge-combinations shows the cases where convert is not the identity: 12-- and -1--, so cases where a file is absent in p1, that p2 added or modified, and the merge redeletes the file. Commit shows such files are modified (rightly) but convert doesn't)

In D6613#96743, @mharbison72 wrote:

If this goes forward, can this be gated by a config option (even if it is on by default)? IIUC, this will change the hash for merges, and then cascade. That seems problematic for convert operations.
Most converts will do things that change hashes anyway, but the default options don't change anything. This is a convenient behavior for easily migrating to/from LFS, for example. (In theory, anyway. In practice, I've seen repos where a convert alters the changelog.) If there is a config option, convert should probably disable it by default to maintain the current behavior. I can see a use case for enabling it though, to clean up the repo.

I hadn't seen the bug you linked to, or your recent change, though I saw ctx.filesadded() and friends (when writing this) and suspected they were written the way they are to work nicely on merges.