This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6848

narrow: add option for automatically removing unused includes
ClosedPublic

Authored by martinvonz on Sep 13 2019, 1:42 AM.

Details

Reviewers
durin42
pulkit
Group Reviewers
hg-reviewers
Commits
rHG40f78072fda9: narrow: add option for automatically removing unused includes
Summary

It's been a somewhat common request among our users to have Mercurial
automatically pick includes to remove. This patch adds an option for
that: hg tracked --auto-remove-includes. I'm not sure if this is the
right name and semantics for it. Perhaps the feature should also add
excludes of large subdirectories even if other files in the include
are needed? Narrow clones are experimental, so we can change the name
and/or semantics later if necessary.

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

martinvonz created this revision.Sep 13 2019, 1:42 AM
rdamazio added a subscriber: rdamazio.Sep 13 2019, 4:53 PM
rdamazio added inline comments.
tests/test-narrow.t
457

This obsolete commit is completely lost afterwards, which kind of violates the principle of "all changes are kept forever" - should it dump this somewhere for safekeeping?
Also, what if you just don't get rid of the files, and simply remove it from the narrowspec? It'll stop processing/pulling/pushing the directory, but no data is lost.

martinvonz added inline comments.Sep 13 2019, 5:16 PM
tests/test-narrow.t
457

This obsolete commit is completely lost afterwards, which kind of violates the principle of "all changes are kept forever" - should it dump this somewhere for safekeeping?

Yes, it does dump it to a backup bundle (see output on line 474).

Also, what if you just don't get rid of the files, and simply remove it from the narrowspec? It'll stop processing/pulling/pushing the directory, but no data is lost.

That's just about the working directory, right? This file is unchanged in the working directory at the time of hg tracked --auto-remove-includes...

rdamazio added inline comments.Sep 13 2019, 8:40 PM
tests/test-narrow.t
457

That's just about the working directory, right? This file is unchanged in the working directory at the time of hg tracked --auto-remove-includes...

No, I meant what if you don't get rid of the .i files.
But you raise a good point - shouldn't you also clean up the working directory?

Alternatively, or in addition, maybe a confirmation prompt pointing out which commits and narrowspec entries will be dropped may be useful.

martinvonz added inline comments.Sep 13 2019, 8:54 PM
tests/test-narrow.t
457

No, I meant what if you don't get rid of the .i files.

This is not changing which .i files we get rid of; the behavior should be the same as if you had typed hg tracked --removeinclude path:d0 --removeinclude path:d2.

But you raise a good point - shouldn't you also clean up the working directory?

Yes, it should be, but it doesn't print those files if they were clean. I've added a call to hg files to the test so you can see that they are cleaned up.

Alternatively, or in addition, maybe a confirmation prompt pointing out which commits and
narrowspec entries will be dropped may be useful.

I considered that but I wasn't sure if it would be more annoying than helpful. Want me to add that?

martinvonz updated this revision to Diff 16532.Sep 13 2019, 8:54 PM
martinvonz added inline comments.Sep 14 2019, 1:45 AM
tests/test-narrow.t
457

I added that prompt. We also talked about adding a --dry-run option instead of the prompt, but that seemed harder because it would need to be handled in many different cases, including --update-working-copy, even if we just decide to error out.

pulkit added a subscriber: pulkit.Sep 17 2019, 1:44 PM
pulkit added inline comments.
hgext/narrow/narrowcommands.py
332

Can you add some help text below on how these unused includes are calculated?

martinvonz added inline comments.Sep 17 2019, 1:45 PM
hgext/narrow/narrowcommands.py
332

Even though it's marked EXPERIMENTAL? Or maybe I should do that and just drop the EXPERIMENTAL since the whole feature (narrow clones) is experimental anyway?

pulkit added inline comments.Sep 17 2019, 1:50 PM
hgext/narrow/narrowcommands.py
331

Following pattern of other flags, auto-removeinclude(s) seems better.

332

I don't have a strong opinion on having or dropping EXPERIMENTAL, but we should have some help text. It can be added under verbose section.

martinvonz marked 2 inline comments as done.Sep 17 2019, 1:58 PM
martinvonz edited the summary of this revision. (Show Details)
martinvonz updated this revision to Diff 16563.
martinvonz added inline comments.Sep 17 2019, 1:58 PM
hgext/narrow/narrowcommands.py
331

I was also wondering about that. Maybe those should be changed instead? I'm not sure, but I think we're slowly transitioning option names to use hyphens just like we do with config names? I don't care much, so let me know if you'd still prefer to drop the second hyphen.

332

I decided to drop EXPERIMENTAL.

pulkit added inline comments.Sep 17 2019, 2:14 PM
hgext/narrow/narrowcommands.py
369

sorry, but currently visible commits sounds like not correct as this can be used in case of non-ellipses repository also.

martinvonz updated this revision to Diff 16564.Sep 17 2019, 2:18 PM
martinvonz marked an inline comment as done.Sep 17 2019, 2:19 PM
martinvonz added inline comments.
hgext/narrow/narrowcommands.py
369

Good point. I've updated this a bit. See how this sounds.

pulkit accepted this revision.Sep 17 2019, 3:33 PM
This revision is now accepted and ready to land.Sep 17 2019, 3:33 PM
martinvonz marked an inline comment as done.Sep 17 2019, 3:57 PM
martinvonz added a commit: rHG40f78072fda9: narrow: add option for automatically removing unused includes.
Closed by commit rHG40f78072fda9: narrow: add option for automatically removing unused includes (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 16570

hgext/narrow/narrowcommands.py

repo.setnewnarrowpats() repo.setnewnarrowpats()
narrowspec.updateworkingcopy(repo) narrowspec.updateworkingcopy(repo)
narrowspec.copytoworkingcopy(repo) narrowspec.copytoworkingcopy(repo)
# TODO(rdamazio): Make new matcher format and update description # TODO(rdamazio): Make new matcher format and update description
@command('tracked', @command('tracked',
[('', 'addinclude', [], _('new paths to include')), [('', 'addinclude', [], _('new paths to include')),
('', 'removeinclude', [], _('old paths to no longer include')), ('', 'removeinclude', [], _('old paths to no longer include')),
('', 'auto-remove-includes', False,
pulkitUnsubmitted
Not Done

Following pattern of other flags, auto-removeinclude(s) seems better.

pulkit: Following pattern of other flags, `auto-removeinclude(s)` seems better.
martinvonzAuthorUnsubmitted
Done

I was also wondering about that. Maybe those should be changed instead? I'm not sure, but I think we're slowly transitioning option names to use hyphens just like we do with config names? I don't care much, so let me know if you'd still prefer to drop the second hyphen.

martinvonz: I was also wondering about that. Maybe those should be changed instead? I'm not sure, but I…
_('automatically choose unused includes to remove')),
pulkitUnsubmitted
Done

Can you add some help text below on how these unused includes are calculated?

pulkit: Can you add some help text below on how these unused includes are calculated?
martinvonzAuthorUnsubmitted
Done

Even though it's marked EXPERIMENTAL? Or maybe I should do that and just drop the EXPERIMENTAL since the whole feature (narrow clones) is experimental anyway?

martinvonz: Even though it's marked EXPERIMENTAL? Or maybe I should do that and just drop the EXPERIMENTAL…
pulkitUnsubmitted
Done

I don't have a strong opinion on having or dropping EXPERIMENTAL, but we should have some help text. It can be added under verbose section.

pulkit: I don't have a strong opinion on having or dropping EXPERIMENTAL, but we should have some help…
martinvonzAuthorUnsubmitted
Done

I decided to drop EXPERIMENTAL.

martinvonz: I decided to drop EXPERIMENTAL.
('', 'addexclude', [], _('new paths to exclude')), ('', 'addexclude', [], _('new paths to exclude')),
('', 'import-rules', '', _('import narrowspecs from a file')), ('', 'import-rules', '', _('import narrowspecs from a file')),
('', 'removeexclude', [], _('old paths to no longer exclude')), ('', 'removeexclude', [], _('old paths to no longer exclude')),
('', 'clear', False, _('whether to replace the existing narrowspec')), ('', 'clear', False, _('whether to replace the existing narrowspec')),
('', 'force-delete-local-changes', False, ('', 'force-delete-local-changes', False,
_('forces deletion of local changes when narrowing')), _('forces deletion of local changes when narrowing')),
('', 'update-working-copy', False, ('', 'update-working-copy', False,
_('update working copy when the store has changed')), _('update working copy when the store has changed')),
The options allow you to add or remove included and excluded expressions. The options allow you to add or remove included and excluded expressions.
If --clear is specified, then all previous includes and excludes are DROPPED If --clear is specified, then all previous includes and excludes are DROPPED
and replaced by the new ones specified to --addinclude and --addexclude. and replaced by the new ones specified to --addinclude and --addexclude.
If --clear is specified without any further options, the narrowspec will be If --clear is specified without any further options, the narrowspec will be
empty and will not match any files. empty and will not match any files.
If --auto-remove-includes is specified, then those includes that don't match
any files modified by currently visible local commits (those not shared by
pulkitUnsubmitted
Done

sorry, but currently visible commits sounds like not correct as this can be used in case of non-ellipses repository also.

pulkit: sorry, but `currently visible commits` sounds like not correct as this can be used in case of…
martinvonzAuthorUnsubmitted
Done

Good point. I've updated this a bit. See how this sounds.

martinvonz: Good point. I've updated this a bit. See how this sounds.
the remote) will be added to the set of explicitly specified includes to
remove.
--import-rules accepts a path to a file containing rules, allowing you to --import-rules accepts a path to a file containing rules, allowing you to
add --addinclude, --addexclude rules in bulk. Like the other include and add --addinclude, --addexclude rules in bulk. Like the other include and
exclude switches, the changes are applied immediately. exclude switches, the changes are applied immediately.
""" """
opts = pycompat.byteskwargs(opts) opts = pycompat.byteskwargs(opts)
if repository.NARROW_REQUIREMENT not in repo.requirements: if repository.NARROW_REQUIREMENT not in repo.requirements:
raise error.Abort(_('the tracked command is only supported on ' raise error.Abort(_('the tracked command is only supported on '
'repositories cloned with --narrow')) 'repositories cloned with --narrow'))
"is not supported in narrowspec")) "is not supported in narrowspec"))
opts['addinclude'].extend(includepats) opts['addinclude'].extend(includepats)
opts['addexclude'].extend(excludepats) opts['addexclude'].extend(excludepats)
addedincludes = narrowspec.parsepatterns(opts['addinclude']) addedincludes = narrowspec.parsepatterns(opts['addinclude'])
removedincludes = narrowspec.parsepatterns(opts['removeinclude']) removedincludes = narrowspec.parsepatterns(opts['removeinclude'])
addedexcludes = narrowspec.parsepatterns(opts['addexclude']) addedexcludes = narrowspec.parsepatterns(opts['addexclude'])
removedexcludes = narrowspec.parsepatterns(opts['removeexclude']) removedexcludes = narrowspec.parsepatterns(opts['removeexclude'])
autoremoveincludes = opts['auto_remove_includes']
update_working_copy = opts['update_working_copy'] update_working_copy = opts['update_working_copy']
only_show = not (addedincludes or removedincludes or addedexcludes or only_show = not (addedincludes or removedincludes or addedexcludes or
removedexcludes or newrules or update_working_copy) removedexcludes or newrules or autoremoveincludes or
update_working_copy)
oldincludes, oldexcludes = repo.narrowpats oldincludes, oldexcludes = repo.narrowpats
# filter the user passed additions and deletions into actual additions and # filter the user passed additions and deletions into actual additions and
# deletions of excludes and includes # deletions of excludes and includes
addedincludes -= oldincludes addedincludes -= oldincludes
removedincludes &= oldincludes removedincludes &= oldincludes
addedexcludes -= oldexcludes addedexcludes -= oldexcludes
return 0 return 0
if update_working_copy: if update_working_copy:
with repo.wlock(), repo.lock(), repo.transaction('narrow-wc'): with repo.wlock(), repo.lock(), repo.transaction('narrow-wc'):
narrowspec.updateworkingcopy(repo) narrowspec.updateworkingcopy(repo)
narrowspec.copytoworkingcopy(repo) narrowspec.copytoworkingcopy(repo)
return 0 return 0
if not widening and not narrowing: if not (widening or narrowing or autoremoveincludes):
ui.status(_("nothing to widen or narrow\n")) ui.status(_("nothing to widen or narrow\n"))
return 0 return 0
with repo.wlock(), repo.lock(): with repo.wlock(), repo.lock():
cmdutil.bailifchanged(repo) cmdutil.bailifchanged(repo)
# Find the revisions we have in common with the remote. These will # Find the revisions we have in common with the remote. These will
# be used for finding local-only changes for narrowing. They will # be used for finding local-only changes for narrowing. They will
# also define the set of revisions to update for widening. # also define the set of revisions to update for widening.
remotepath = ui.expandpath(remotepath or 'default') remotepath = ui.expandpath(remotepath or 'default')
url, branches = hg.parseurl(remotepath) url, branches = hg.parseurl(remotepath)
ui.status(_('comparing with %s\n') % util.hidepassword(url)) ui.status(_('comparing with %s\n') % util.hidepassword(url))
remote = hg.peer(repo, opts, url) remote = hg.peer(repo, opts, url)
# check narrow support before doing anything if widening needs to be # check narrow support before doing anything if widening needs to be
# performed. In future we should also abort if client is ellipses and # performed. In future we should also abort if client is ellipses and
# server does not support ellipses # server does not support ellipses
if widening and wireprototypes.NARROWCAP not in remote.capabilities(): if widening and wireprototypes.NARROWCAP not in remote.capabilities():
raise error.Abort(_("server does not support narrow clones")) raise error.Abort(_("server does not support narrow clones"))
commoninc = discovery.findcommonincoming(repo, remote) commoninc = discovery.findcommonincoming(repo, remote)
if autoremoveincludes:
outgoing = discovery.findcommonoutgoing(repo, remote,
commoninc=commoninc)
ui.status(_('looking for unused includes to remove\n'))
localfiles = set()
for n in itertools.chain(outgoing.missing, outgoing.excluded):
localfiles.update(repo[n].files())
suggestedremovals = []
for include in sorted(oldincludes):
match = narrowspec.match(repo.root, [include], oldexcludes)
if not any(match(f) for f in localfiles):
suggestedremovals.append(include)
if suggestedremovals:
for s in suggestedremovals:
ui.status('%s\n' % s)
if (ui.promptchoice(_('remove these unused includes (yn)?'
'$$ &Yes $$ &No')) == 0):
removedincludes.update(suggestedremovals)
narrowing = True
else:
ui.status(_('found no unused includes\n'))
if narrowing: if narrowing:
newincludes = oldincludes - removedincludes newincludes = oldincludes - removedincludes
newexcludes = oldexcludes | addedexcludes newexcludes = oldexcludes | addedexcludes
_narrow(ui, repo, remote, commoninc, oldincludes, oldexcludes, _narrow(ui, repo, remote, commoninc, oldincludes, oldexcludes,
newincludes, newexcludes, newincludes, newexcludes,
opts['force_delete_local_changes']) opts['force_delete_local_changes'])
# _narrow() updated the narrowspec and _widen() below needs to # _narrow() updated the narrowspec and _widen() below needs to
# use the updated values as its base (otherwise removed includes # use the updated values as its base (otherwise removed includes

tests/test-narrow-trackedcmd.t

hg tracked [OPTIONS]... [REMOTE] hg tracked [OPTIONS]... [REMOTE]
show or change the current narrowspec show or change the current narrowspec
options ([+] can be repeated): options ([+] can be repeated):
--addinclude VALUE [+] new paths to include --addinclude VALUE [+] new paths to include
--removeinclude VALUE [+] old paths to no longer include --removeinclude VALUE [+] old paths to no longer include
--auto-remove-includes automatically choose unused includes to
remove
--addexclude VALUE [+] new paths to exclude --addexclude VALUE [+] new paths to exclude
--import-rules VALUE import narrowspecs from a file --import-rules VALUE import narrowspecs from a file
--removeexclude VALUE [+] old paths to no longer exclude --removeexclude VALUE [+] old paths to no longer exclude
--clear whether to replace the existing narrowspec --clear whether to replace the existing narrowspec
--force-delete-local-changes forces deletion of local changes when --force-delete-local-changes forces deletion of local changes when
narrowing narrowing
--update-working-copy update working copy when the store has --update-working-copy update working copy when the store has
changed changed

tests/test-narrow.t

^[0-9a-f]{12}$ (re) ^[0-9a-f]{12}$ (re)
^[0-9a-f]{12}$ (re) ^[0-9a-f]{12}$ (re)
^[0-9a-f]{12}$ (re) ^[0-9a-f]{12}$ (re)
^[0-9a-f]{12}$ (re) ^[0-9a-f]{12}$ (re)
^[0-9a-f]{12}$ (re) ^[0-9a-f]{12}$ (re)
abort: local changes found abort: local changes found
(use --force-delete-local-changes to ignore) (use --force-delete-local-changes to ignore)
[255] [255]
$ cd ..
Test --auto-remove-includes
$ hg clone --narrow ssh://user@dummy/master narrow-auto-remove -q \
> --include d0 --include d1 --include d2
$ cd narrow-auto-remove
$ echo a >> d0/f
$ hg ci -m 'local change to d0'
rdamazioUnsubmitted
Not Done

This obsolete commit is completely lost afterwards, which kind of violates the principle of "all changes are kept forever" - should it dump this somewhere for safekeeping?
Also, what if you just don't get rid of the files, and simply remove it from the narrowspec? It'll stop processing/pulling/pushing the directory, but no data is lost.

rdamazio: This obsolete commit is completely lost afterwards, which kind of violates the principle of…
martinvonzAuthorUnsubmitted
Done

This obsolete commit is completely lost afterwards, which kind of violates the principle of "all changes are kept forever" - should it dump this somewhere for safekeeping?

Yes, it does dump it to a backup bundle (see output on line 474).

Also, what if you just don't get rid of the files, and simply remove it from the narrowspec? It'll stop processing/pulling/pushing the directory, but no data is lost.

That's just about the working directory, right? This file is unchanged in the working directory at the time of hg tracked --auto-remove-includes...

martinvonz: > This obsolete commit is completely lost afterwards, which kind of violates the principle of…
rdamazioUnsubmitted
Not Done

That's just about the working directory, right? This file is unchanged in the working directory at the time of hg tracked --auto-remove-includes...

No, I meant what if you don't get rid of the .i files.
But you raise a good point - shouldn't you also clean up the working directory?

Alternatively, or in addition, maybe a confirmation prompt pointing out which commits and narrowspec entries will be dropped may be useful.

rdamazio: > That's just about the working directory, right? This file is unchanged in the working…
martinvonzAuthorUnsubmitted
Done

No, I meant what if you don't get rid of the .i files.

This is not changing which .i files we get rid of; the behavior should be the same as if you had typed hg tracked --removeinclude path:d0 --removeinclude path:d2.

But you raise a good point - shouldn't you also clean up the working directory?

Yes, it should be, but it doesn't print those files if they were clean. I've added a call to hg files to the test so you can see that they are cleaned up.

Alternatively, or in addition, maybe a confirmation prompt pointing out which commits and
narrowspec entries will be dropped may be useful.

I considered that but I wasn't sure if it would be more annoying than helpful. Want me to add that?

martinvonz: > No, I meant what if you don't get rid of the .i files. This is not changing which .i files…
martinvonzAuthorUnsubmitted
Done

I added that prompt. We also talked about adding a --dry-run option instead of the prompt, but that seemed harder because it would need to be handled in many different cases, including --update-working-copy, even if we just decide to error out.

martinvonz: I added that prompt. We also talked about adding a `--dry-run` option instead of the prompt…
$ hg co '.^'
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ echo a >> d1/f
$ hg ci -m 'local change to d1'
created new head
$ hg debugobsolete $(hg log -T '{node}' -r 'desc("local change to d0")')
1 new obsolescence markers
obsoleted 1 changesets
$ echo n | hg tracked --auto-remove-includes --config ui.interactive=yes
comparing with ssh://user@dummy/master
searching for changes
looking for unused includes to remove
path:d0
path:d2
remove these unused includes (yn)? n
$ hg tracked --auto-remove-includes
comparing with ssh://user@dummy/master
searching for changes
looking for unused includes to remove
path:d0
path:d2
remove these unused includes (yn)? y
looking for local changes to affected paths
saved backup bundle to $TESTTMP/narrow-auto-remove/.hg/strip-backup/*-narrow.hg (glob)
deleting data/d0/f.i
deleting data/d2/f.i
deleting meta/d0/00manifest.i (tree !)
deleting meta/d2/00manifest.i (tree !)
$ hg tracked
I path:d1
$ hg files
d1/f
$ hg tracked --auto-remove-includes
comparing with ssh://user@dummy/master
searching for changes
looking for unused includes to remove
found no unused includes