This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10936

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

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

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG00ae1fb6c459: cmdutil: fix newandmodified file accounting for --interactive commits
rHGaa92c5d702ce: cmdutil: fix newandmodified file accounting for --interactive commits
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 a subscriber: mharbison72.Jul 3 2021, 12:21 AM
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.
dploch added a comment.Jul 7 2021, 12:34 PM
In D10936#167716, @Alphare wrote:

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
In D10936#168357, @dploch wrote:

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
Alphare added a comment.Jul 9 2021, 5:36 AM

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

Revision Contents
Changeset List

PathPackages
Mmercurial/cmdutil.py (37 lines)
Mtests/test-commit-interactive.t (56 lines)

Diff 29032

mercurial/cmdutil.py

raise error.InputError(_(b"note cannot contain a newline")) raise error.InputError(_(b"note cannot contain a newline"))
def ishunk(x): def ishunk(x):
hunkclasses = (crecordmod.uihunk, patch.recordhunk) hunkclasses = (crecordmod.uihunk, patch.recordhunk)
return isinstance(x, hunkclasses) return isinstance(x, hunkclasses)
def newandmodified(chunks, originalchunks): def isheader(x):
headerclasses = (crecordmod.uiheader, patch.header)
return isinstance(x, headerclasses)
def newandmodified(chunks):
newlyaddedandmodifiedfiles = set() newlyaddedandmodifiedfiles = set()
alsorestore = set() alsorestore = set()
for chunk in chunks: for chunk in chunks:
if ( if isheader(chunk) and chunk.isnewfile():
ishunk(chunk) newlyaddedandmodifiedfiles.add(chunk.filename())
and chunk.header.isnewfile()
and chunk not in originalchunks
):
newlyaddedandmodifiedfiles.add(chunk.header.filename())
alsorestore.update( alsorestore.update(
set(chunk.header.files()) - {chunk.header.filename()} set(chunk.files()) - {chunk.filename()}
) )
return newlyaddedandmodifiedfiles, alsorestore return newlyaddedandmodifiedfiles, alsorestore
def parsealiases(cmd): def parsealiases(cmd):
base_aliases = cmd.split(b"|") base_aliases = cmd.split(b"|")
all_aliases = set(base_aliases) all_aliases = set(base_aliases)
extra_aliases = [] extra_aliases = []
whitespace=True, whitespace=True,
section=b'commands', section=b'commands',
configprefix=b'commit.interactive.', configprefix=b'commit.interactive.',
) )
diffopts.nodates = True diffopts.nodates = True
diffopts.git = True diffopts.git = True
diffopts.showfunc = True diffopts.showfunc = True
originaldiff = patch.diff(repo, changes=status, opts=diffopts) originaldiff = patch.diff(repo, changes=status, opts=diffopts)
originalchunks = patch.parsepatch(originaldiff) original_headers = patch.parsepatch(originaldiff)
AlphareUnsubmitted
Done

nit: this could be named original_headers for clarity

Alphare: nit: this could be named `original_headers` for clarity
match = scmutil.match(repo[None], pats) match = scmutil.match(repo[None], pats)
# 1. filter patch, since we are intending to apply subset of it # 1. filter patch, since we are intending to apply subset of it
try: try:
chunks, newopts = filterfn(ui, originalchunks, match) chunks, newopts = filterfn(ui, original_headers, match)
except error.PatchError as err: except error.PatchError as err:
raise error.InputError(_(b'error parsing patch: %s') % err) raise error.InputError(_(b'error parsing patch: %s') % err)
opts.update(newopts) opts.update(newopts)
# We need to keep a backup of files that have been newly added and # We need to keep a backup of files that have been newly added and
# modified during the recording process because there is a previous # modified during the recording process because there is a previous
# version without the edit in the workdir. We also will need to restore # version without the edit in the workdir. We also will need to restore
# files that were the sources of renames so that the patch application # files that were the sources of renames so that the patch application
# works. # works.
newlyaddedandmodifiedfiles, alsorestore = newandmodified( newlyaddedandmodifiedfiles, alsorestore = newandmodified(chunks)
chunks, originalchunks
)
contenders = set() contenders = set()
for h in chunks: for h in chunks:
try: if isheader(h):
contenders.update(set(h.files())) contenders.update(set(h.files()))
except AttributeError:
pass
changed = status.modified + status.added + status.removed changed = status.modified + status.added + status.removed
newfiles = [f for f in changed if f in contenders] newfiles = [f for f in changed if f in contenders]
if not newfiles: if not newfiles:
ui.status(_(b'no changes to record\n')) ui.status(_(b'no changes to record\n'))
return 0 return 0
modified = set(status.modified) modified = set(status.modified)
operation = b'keep' operation = b'keep'
else: else:
operation = b'discard' operation = b'discard'
if operation == b'apply': if operation == b'apply':
diff = patch.diff(repo, None, ctx.node(), m, opts=diffopts) diff = patch.diff(repo, None, ctx.node(), m, opts=diffopts)
else: else:
diff = patch.diff(repo, ctx.node(), None, m, opts=diffopts) diff = patch.diff(repo, ctx.node(), None, m, opts=diffopts)
originalchunks = patch.parsepatch(diff) original_headers = patch.parsepatch(diff)
try: try:
chunks, opts = recordfilter( chunks, opts = recordfilter(
repo.ui, originalchunks, match, operation=operation repo.ui, original_headers, match, operation=operation
) )
if operation == b'discard': if operation == b'discard':
chunks = patch.reversehunks(chunks) chunks = patch.reversehunks(chunks)
except error.PatchError as err: except error.PatchError as err:
raise error.Abort(_(b'error parsing patch: %s') % err) raise error.Abort(_(b'error parsing patch: %s') % err)
# FIXME: when doing an interactive revert of a copy, there's no way of # FIXME: when doing an interactive revert of a copy, there's no way of
# performing a partial revert of the added file, the only option is # performing a partial revert of the added file, the only option is
# "remove added file <name> (Yn)?", so we don't need to worry about the # "remove added file <name> (Yn)?", so we don't need to worry about the
# alsorestore value. Ideally we'd be able to partially revert # alsorestore value. Ideally we'd be able to partially revert
# copied/renamed files. # copied/renamed files.
newlyaddedandmodifiedfiles, unusedalsorestore = newandmodified( newlyaddedandmodifiedfiles, unusedalsorestore = newandmodified(chunks)
chunks, originalchunks
)
if tobackup is None: if tobackup is None:
tobackup = set() tobackup = set()
# Apply changes # Apply changes
fp = stringio() fp = stringio()
# chunks are serialized per file, but files aren't sorted # chunks are serialized per file, but files aren't sorted
for f in sorted({c.header.filename() for c in chunks if ishunk(c)}): for f in sorted({c.header.filename() for c in chunks if ishunk(c)}):
prntstatusmsg(b'revert', f) prntstatusmsg(b'revert', f)
files = set() files = set()

tests/test-commit-interactive.t

@@ -11,3 +11,4 @@ 8 @@ -11,3 +11,4 @@ 8
9 9
10 10
11 11
+somechange +somechange
record this change to 'plain3'? record this change to 'plain3'?
(enter ? for help) [Ynesfdaq?] y (enter ? for help) [Ynesfdaq?] y
Rename file but discard edits
$ echo content > new-file
$ hg add -q new-file
$ hg commit -qm 'new file'
$ hg mv new-file renamed-file
$ echo new-content >> renamed-file
$ hg commit -i -d '24 0' -m content-rename<<EOF
> y
> n
> EOF
diff --git a/new-file b/renamed-file
rename from new-file
rename to renamed-file
1 hunks, 1 lines changed
examine changes to 'new-file' and 'renamed-file'?
(enter ? for help) [Ynesfdaq?] y
@@ -1,1 +1,2 @@
content
+new-content
record this change to 'renamed-file'?
(enter ? for help) [Ynesfdaq?] n
$ hg status
M renamed-file
? editedfile.orig
? editedfile.rej
? editor.sh
$ hg diff
diff -r * renamed-file (glob)
mharbison72Unsubmitted
Done

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.

mharbison72: I’m not familiar with this area of code, so I’d rather someone else queue it. But I want to…
dplochAuthorUnsubmitted
Done

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

dploch: Do you mean to replace the hash with `* (glob)`? `diff -r <hash>` is output, not a command.
--- a/renamed-file Thu Jan 01 00:00:24 1970 +0000
+++ b/renamed-file Thu Jan 01 00:00:00 1970 +0000
@@ -1,1 +1,2 @@
content
+new-content
The #if execbit block above changes the hash here on some systems The #if execbit block above changes the hash here on some systems
$ hg status -A plain3 $ hg status -A plain3
C plain3 C plain3
$ hg tip $ hg tip
changeset: 32:* (glob) changeset: 34:* (glob)
tag: tip tag: tip
user: test user: test
date: Thu Jan 01 00:00:23 1970 +0000 date: Thu Jan 01 00:00:24 1970 +0000
summary: moving_files summary: content-rename
Editing patch of newly added file Editing patch of newly added file
$ hg update -C . $ hg update -C .
0 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ cat > editor.sh << '__EOF__' $ cat > editor.sh << '__EOF__'
> cat "$1" | sed "s/first/very/g" > tt > cat "$1" | sed "s/first/very/g" > tt
> mv tt "$1" > mv tt "$1"
> __EOF__ > __EOF__
$ cat > newfile << '__EOF__' $ cat > newfile << '__EOF__'
> This is the first line > This is the first line
> This is the second line > This is the second line
> This is the third line > This is the third line
> __EOF__ > __EOF__
$ hg add newfile $ hg add newfile
$ HGEDITOR="\"sh\" \"`pwd`/editor.sh\"" hg commit -i -d '23 0' -medit-patch-new <<EOF $ HGEDITOR="\"sh\" \"`pwd`/editor.sh\"" hg commit -i -d '25 0' -medit-patch-new <<EOF
> y > y
> e > e
> EOF > EOF
diff --git a/newfile b/newfile diff --git a/newfile b/newfile
new file mode 100644 new file mode 100644
examine changes to 'newfile'? examine changes to 'newfile'?
(enter ? for help) [Ynesfdaq?] y (enter ? for help) [Ynesfdaq?] y
Add new file from within a subdirectory Add new file from within a subdirectory
$ hg update -C . $ hg update -C .
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ mkdir folder $ mkdir folder
$ cd folder $ cd folder
$ echo "foo" > bar $ echo "foo" > bar
$ hg add bar $ hg add bar
$ hg commit -i -d '23 0' -mnewfilesubdir <<EOF $ hg commit -i -d '26 0' -mnewfilesubdir <<EOF
> y > y
> y > y
> EOF > EOF
diff --git a/folder/bar b/folder/bar diff --git a/folder/bar b/folder/bar
new file mode 100644 new file mode 100644
examine changes to 'folder/bar'? examine changes to 'folder/bar'?
(enter ? for help) [Ynesfdaq?] y (enter ? for help) [Ynesfdaq?] y
@@ -0,0 +1,1 @@ @@ -0,0 +1,1 @@
+foo +foo
record this change to 'folder/bar'? record this change to 'folder/bar'?
(enter ? for help) [Ynesfdaq?] y (enter ? for help) [Ynesfdaq?] y
The #if execbit block above changes the hashes here on some systems The #if execbit block above changes the hashes here on some systems
$ hg tip -p $ hg tip -p
changeset: 34:* (glob) changeset: 36:* (glob)
tag: tip tag: tip
user: test user: test
date: Thu Jan 01 00:00:23 1970 +0000 date: Thu Jan 01 00:00:26 1970 +0000
summary: newfilesubdir summary: newfilesubdir
diff -r * -r * folder/bar (glob) diff -r * -r * folder/bar (glob)
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/folder/bar Thu Jan 01 00:00:23 1970 +0000 +++ b/folder/bar Thu Jan 01 00:00:26 1970 +0000
@@ -0,0 +1,1 @@ @@ -0,0 +1,1 @@
+foo +foo
$ cd .. $ cd ..
$ hg status -A folder/bar $ hg status -A folder/bar
C folder/bar C folder/bar