Page MenuHomePhabricator

cmdutil: fix newandmodified file accounting for --interactive commits
ClosedPublic

Authored by dploch on Jul 2 2021, 4:17 PM.

Details

Summary

originalchunks is a misleading name, because it only contains header objects, which are flattened to selected hunks by the filter function. As such, chunks not in originalchunks is always True and misleading, because hunk objects never compare equal to header objects. This change fixes the internal naming and removes the useless parameter from the method.

This change also fixes issue6533, by considering the filtered headers, rather than the hunks, when determining new and modified files. If a file is renamed + edited, and the edited hunks are deselected (but the file is not), the filtered chunks will contain a header for the file (because it's .special()) but but no hunks.

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

dploch created this revision.Jul 2 2021, 4:17 PM
mharbison72 added inline comments.
tests/test-commit-interactive.t
1747

I’m not familiar with this area of code, so I’d rather someone else queue it. But I want to point out that it’s probably better to replace the hash here with ., and restore the glob matching below, based on the comment below about the #if execbit block above.

Alphare requested changes to this revision.Jul 5 2021, 5:26 AM
Alphare added a subscriber: Alphare.

Agreed with @mharbison72 about the having the hash explicitly there. The dates should also be globed, since they depend on system speed and do not provide anything useful to the test.
Could you also edit your commit message to contain "issue6533"? This way bz will close it automatically in the next release.
The change looks good aside from this.

mercurial/cmdutil.py
521

nit: this could be named original_headers for clarity

This revision now requires changes to proceed.Jul 5 2021, 5:26 AM
dploch marked 2 inline comments as done.Jul 7 2021, 12:34 PM
dploch edited the summary of this revision. (Show Details)
dploch updated this revision to Diff 28939.

The dates should also be globed, since they depend on system speed and do not provide anything useful to the test.

The dates are explicitly set by hg commit -d throughout the entire file more or less; they do not appear dependent on system speed. I assumed they were specified ascending for some reason; I updated the tail of the file to be consistent with the rest of the file. If there's no point to the date then it should probably all be glob-ified in a separate change.

Could you also edit your commit message to contain "issue6533"? This way bz will close it automatically in the next release.

Done.

The change looks good aside from this.

tests/test-commit-interactive.t
1747

Do you mean to replace the hash with * (glob)? diff -r <hash> is output, not a command.
Otherwise done.

Alphare accepted this revision.Jul 9 2021, 4:02 AM

The dates are explicitly set by hg commit -d throughout the entire file more or less; they do not appear dependent on system speed. I assumed they were specified ascending for some reason; I updated the tail of the file to be consistent with the rest of the file. If there's no point to the date then it should probably all be glob-ified in a separate change.

Yeah I'm sorry I'm not sure where I got that idea, this is indeed fine, thanks.

This revision is now accepted and ready to land.Jul 9 2021, 4:02 AM

Note: this change broke the formatting test, so I'm fixing it in flight.