This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4125

narrow: add '--import-rules' flag to tracked command
ClosedPublic

Authored by pulkit on Aug 6 2018, 7:07 AM.

Details

Reviewers
durin42
martinvonz
Group Reviewers
hg-reviewers
Commits
rHGab20ee07b82d: narrow: add '--import-rules' flag to tracked command
Summary

This patch adds a --import-rules flag to tracked command provided by narrow
extension. Using the --import-rules flag, you can pass a filename from which
narrowspecs should be read and added to main narrowspec.

A lot of times, in automation or manually also, when you are working with big
repo, specifying each path name on commandline using '--addinclude' and
'--addexclude' is tedious and something which can scale. So we needed something
where we can pass a file to extend the narrowspecs.

Nice thing about this is that the automations which reads some file to change
the sparse profile, can now read the same file for changing narrowspecs too.

Tests are added for the new feature.

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

pulkit created this revision.Aug 6 2018, 7:07 AM
durin42 added a subscriber: martinvonz.Aug 9 2018, 2:17 PM
durin42 accepted this revision as: durin42.

Seems fine to me, though thinking we should probably spend some time to make sure the sparse and narrow patterns are cross-compatible like this change seems to hope for.

@martinvonz maybe take a look at this with @pulkit when you're back?

pulkit added a comment.Aug 9 2018, 2:25 PM
In D4125#64732, @durin42 wrote:

Seems fine to me, though thinking we should probably spend some time to make sure the sparse and narrow patterns are cross-compatible like this change seems to hope for.

I already did that. https://www.mercurial-scm.org/repo/hg-committed/rev/f64ebe7d2259

pulkit added a comment.Aug 22 2018, 6:58 AM

ping for review.

durin42 added a comment.Aug 22 2018, 12:37 PM

needs rebased, but looks fine to me

martinvonz added inline comments.Aug 22 2018, 1:33 PM
hgext/narrow/narrowcommands.py
330

nit: --extend may be too generic. How about --addfromfile? Can be done in a follow-up if you agree.

379

Why is it required to be in the working directory? Why not accept any kind of path? I don't know if it's intentional or not, but I think this also means that cd dir; hg tracked --extend foo will attempt to read foo from the repo root and not from the current directory, which may be confusing to users.

387

could you send a patch to fix the typoed "supported" on narrowspec.py too?

388–389

nit: is the list conversion necessary?

tests/test-narrow-trackedcmd.t
4–7

are treemanifests relevant to this test?

53–87

any reason not to add these from the beginning (i.e. before creating the clone)?

92

This is part of why I don't like --extend: it sounds like it can be given without a parameter

119

nit: s/>>/>/

pulkit planned changes to this revision.Aug 23 2018, 5:03 PM
pulkit added inline comments.
hgext/narrow/narrowcommands.py
330

I thought about --addfromfile and dropped it because you can both add and remove includes and excludes using this option. Add in other flags means adding either include or exclude specs.

I am fine if that looks good to you. I don't feel strongly.

379

That's a nice idea. I just copied sparse behavior here. I will update with suggested idea.

387

Sure!

388–389

not sure, I will check and remove if not required.

tests/test-narrow-trackedcmd.t
4–7

Nope, I will use testcase here and add test for both cases.

53–87

I just copied these from one of the other existing test files.

92

hm, do you have a functionality in mind for hg tracked --extend without a parameter?

martinvonz added inline comments.Aug 23 2018, 5:07 PM
hgext/narrow/narrowcommands.py
330

It looks like it just adds includes and excludes (but yes, adding excludes means removing files), so I think the name makes sense

tests/test-narrow-trackedcmd.t
92

Nope, nothing in mind :) It just sounds like it might do something without a parameter.

foozy added a subscriber: foozy.Aug 24 2018, 9:08 AM
foozy added inline comments.
hgext/narrow/narrowcommands.py
330

Just curious, is it intentional that the name of this option is different from --import-rules of debugsparse ?

pulkit added inline comments.Aug 24 2018, 10:06 AM
hgext/narrow/narrowcommands.py
330

I didn't know about that. That likes a good name, @martinvonz what do you think?

pulkit edited the summary of this revision. (Show Details)Aug 25 2018, 3:35 PM
pulkit retitled this revision from narrow: add '--extend' flag to tracked command to narrow: add '--import-rules' flag to tracked command.
pulkit added a child revision: D4374: narrowspec: fix a typoed 'supported'.
pulkit updated this revision to Diff 10564.
pulkit added a comment.Sep 3 2018, 6:10 AM

gentle ping for review

martinvonz requested changes to this revision.Sep 4 2018, 11:36 AM
martinvonz added inline comments.
hgext/narrow/narrowcommands.py
377–381

Perhaps we should use util.readfile(). I saw that that's what e.g. hg commit --logfile does. See https://www.mercurial-scm.org/repo/hg/file/2df3271ef139/mercurial/cmdutil.py#l844 and other callers for examples.

(Even if we decided to not use that helper, we should probably move fp.read() inside the try-block.)

tests/test-narrow-trackedcmd.t
80

We should switch to {rev} here like I did for the other narrow tests in another patch

This revision now requires changes to proceed.Sep 4 2018, 11:36 AM
pulkit updated this revision to Diff 10741.Sep 4 2018, 12:11 PM
pulkit marked 2 inline comments as done.Sep 4 2018, 12:12 PM
martinvonz accepted this revision.Sep 4 2018, 12:17 PM
This revision is now accepted and ready to land.Sep 4 2018, 12:17 PM
Closed by commit rHGab20ee07b82d: narrow: add '--import-rules' flag to tracked command (authored by pulkit). · Explain WhySep 4 2018, 7:32 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 10778

hgext/narrow/narrowcommands.py

# narrowcommands.py - command modifications for narrowhg extension # narrowcommands.py - command modifications for narrowhg extension
# #
# Copyright 2017 Google, Inc. # Copyright 2017 Google, Inc.
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import itertools import itertools
import os import os
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial import ( from mercurial import (
cmdutil, cmdutil,
commands, commands,
discovery, discovery,
encoding,
error, error,
exchange, exchange,
extensions, extensions,
hg, hg,
merge, merge,
narrowspec, narrowspec,
node, node,
pycompat, pycompat,
mctx=repo['.'], overwrite=False) mctx=repo['.'], overwrite=False)
merge.recordupdates(repo, actions, branchmerge=False) merge.recordupdates(repo, actions, branchmerge=False)
# 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')),
('', 'addexclude', [], _('new paths to exclude')), ('', 'addexclude', [], _('new paths to exclude')),
('', 'import-rules', '', _('import narrowspecs from a file')),
martinvonzUnsubmitted
Not Done

nit: --extend may be too generic. How about --addfromfile? Can be done in a follow-up if you agree.

martinvonz: nit: `--extend` may be too generic. How about `--addfromfile`? Can be done in a follow-up if…
pulkitAuthorUnsubmitted
Not Done

I thought about --addfromfile and dropped it because you can both add and remove includes and excludes using this option. Add in other flags means adding either include or exclude specs.

I am fine if that looks good to you. I don't feel strongly.

pulkit: I thought about `--addfromfile` and dropped it because you can both add and remove includes and…
martinvonzUnsubmitted
Not Done

It looks like it just adds includes and excludes (but yes, adding excludes means removing files), so I think the name makes sense

martinvonz: It looks like it just adds includes and excludes (but yes, adding excludes means removing…
foozyUnsubmitted
Not Done

Just curious, is it intentional that the name of this option is different from --import-rules of debugsparse ?

foozy: Just curious, is it intentional that the name of this option is different from `--import-rules`…
pulkitAuthorUnsubmitted
Not Done

I didn't know about that. That likes a good name, @martinvonz what do you think?

pulkit: I didn't know about that. That likes a good name, @martinvonz what do you think?
('', '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')),
] + commands.remoteopts, ] + commands.remoteopts,
_('[OPTIONS]... [REMOTE]'), _('[OPTIONS]... [REMOTE]'),
inferrepo=True) inferrepo=True)
def trackedcmd(ui, repo, remotepath=None, *pats, **opts): def trackedcmd(ui, repo, remotepath=None, *pats, **opts):
return 1 return 1
# Before supporting, decide whether it "hg tracked --clear" should mean # Before supporting, decide whether it "hg tracked --clear" should mean
# tracking no paths or all paths. # tracking no paths or all paths.
if opts['clear']: if opts['clear']:
ui.warn(_('The --clear option is not yet supported.\n')) ui.warn(_('The --clear option is not yet supported.\n'))
return 1 return 1
# import rules from a file
newrules = opts.get('import_rules')
if newrules:
try:
filepath = os.path.join(pycompat.getcwd(), newrules)
fdata = util.readfile(filepath)
martinvonzUnsubmitted
Not Done

Why is it required to be in the working directory? Why not accept any kind of path? I don't know if it's intentional or not, but I think this also means that cd dir; hg tracked --extend foo will attempt to read foo from the repo root and not from the current directory, which may be confusing to users.

martinvonz: Why is it required to be in the working directory? Why not accept any kind of path? I don't…
pulkitAuthorUnsubmitted
Not Done

That's a nice idea. I just copied sparse behavior here. I will update with suggested idea.

pulkit: That's a nice idea. I just copied sparse behavior here. I will update with suggested idea.
except IOError as inst:
raise error.Abort(_("cannot read narrowspecs from '%s': %s") %
martinvonzUnsubmitted
Done

Perhaps we should use util.readfile(). I saw that that's what e.g. hg commit --logfile does. See https://www.mercurial-scm.org/repo/hg/file/2df3271ef139/mercurial/cmdutil.py#l844 and other callers for examples.

(Even if we decided to not use that helper, we should probably move fp.read() inside the try-block.)

martinvonz: Perhaps we should use `util.readfile()`. I saw that that's what e.g. `hg commit --logfile`…
(filepath, encoding.strtolocal(inst.strerror)))
includepats, excludepats, profiles = sparse.parseconfig(ui, fdata,
'narrow')
if profiles:
raise error.Abort(_("including other spec files using '%include' "
"is not supported in narrowspec"))
martinvonzUnsubmitted
Not Done

could you send a patch to fix the typoed "supported" on narrowspec.py too?

martinvonz: could you send a patch to fix the typoed "supported" on narrowspec.py too?
pulkitAuthorUnsubmitted
Not Done

Sure!

pulkit: Sure!
opts['addinclude'].extend(includepats)
opts['addexclude'].extend(excludepats)
martinvonzUnsubmitted
Not Done

nit: is the list conversion necessary?

martinvonz: nit: is the `list` conversion necessary?
pulkitAuthorUnsubmitted
Not Done

not sure, I will check and remove if not required.

pulkit: not sure, I will check and remove if not required.
if narrowspec.needsexpansion(opts['addinclude'] + opts['addexclude']): if narrowspec.needsexpansion(opts['addinclude'] + opts['addexclude']):
raise error.Abort('Expansion not yet supported on widen/narrow') raise error.Abort('Expansion not yet supported on widen/narrow')
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'])
widening = addedincludes or removedexcludes widening = addedincludes or removedexcludes

tests/test-narrow-trackedcmd.t

  • This file was added.
#testcases flat tree
$ . "$TESTDIR/narrow-library.sh"
#if tree
$ cat << EOF >> $HGRCPATH
> [experimental]
> treemanifest = 1
martinvonzUnsubmitted
Not Done

are treemanifests relevant to this test?

martinvonz: are treemanifests relevant to this test?
pulkitAuthorUnsubmitted
Not Done

Nope, I will use testcase here and add test for both cases.

pulkit: Nope, I will use testcase here and add test for both cases.
> EOF
#endif
$ hg init master
$ cd master
$ cat >> .hg/hgrc <<EOF
> [narrow]
> serveellipses=True
> EOF
$ mkdir inside
$ echo 'inside' > inside/f
$ hg add inside/f
$ hg commit -m 'add inside'
$ mkdir widest
$ echo 'widest' > widest/f
$ hg add widest/f
$ hg commit -m 'add widest'
$ mkdir outside
$ echo 'outside' > outside/f
$ hg add outside/f
$ hg commit -m 'add outside'
$ cd ..
narrow clone the inside file
$ hg clone --narrow ssh://user@dummy/master narrow --include inside
requesting all changes
adding changesets
adding manifests
adding file changes
added 2 changesets with 1 changes to 1 files
new changesets *:* (glob)
updating to branch default
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ cd narrow
$ hg tracked
I path:inside
$ ls
inside
$ cat inside/f
inside
$ cd ..
add more upstream files which we will include in a wider narrow spec
$ cd master
$ mkdir wider
$ echo 'wider' > wider/f
$ hg add wider/f
$ echo 'widest v2' > widest/f
$ hg commit -m 'add wider, update widest'
$ echo 'widest v3' > widest/f
$ hg commit -m 'update widest v3'
$ echo 'inside v2' > inside/f
$ hg commit -m 'update inside'
$ mkdir outside2
$ echo 'outside2' > outside2/f
$ hg add outside2/f
$ hg commit -m 'add outside2'
$ echo 'widest v4' > widest/f
$ hg commit -m 'update widest v4'
$ hg log -T "{if(ellipsis, '...')}{rev}: {desc}\n"
7: update widest v4
martinvonzUnsubmitted
Done

We should switch to {rev} here like I did for the other narrow tests in another patch

martinvonz: We should switch to {rev} here like I did for the other narrow tests in another patch
6: add outside2
5: update inside
4: update widest v3
3: add wider, update widest
2: add outside
1: add widest
0: add inside
martinvonzUnsubmitted
Not Done

any reason not to add these from the beginning (i.e. before creating the clone)?

martinvonz: any reason not to add these from the beginning (i.e. before creating the clone)?
pulkitAuthorUnsubmitted
Not Done

I just copied these from one of the other existing test files.

pulkit: I just copied these from one of the other existing test files.
$ cd ..
Testing the --import-rules flag of `hg tracked` command
martinvonzUnsubmitted
Not Done

This is part of why I don't like --extend: it sounds like it can be given without a parameter

martinvonz: This is part of why I don't like `--extend`: it sounds like it can be given without a parameter
pulkitAuthorUnsubmitted
Not Done

hm, do you have a functionality in mind for hg tracked --extend without a parameter?

pulkit: hm, do you have a functionality in mind for `hg tracked --extend` without a parameter?
martinvonzUnsubmitted
Not Done

Nope, nothing in mind :) It just sounds like it might do something without a parameter.

martinvonz: Nope, nothing in mind :) It just sounds like it might do something without a parameter.
$ cd narrow
$ hg tracked --import-rules
hg tracked: option --import-rules requires argument
hg tracked [OPTIONS]... [REMOTE]
show or change the current narrowspec
options ([+] can be repeated):
--addinclude VALUE [+] new paths to include
--removeinclude VALUE [+] old paths to no longer include
--addexclude VALUE [+] new paths to exclude
--import-rules VALUE import narrowspecs from a file
--removeexclude VALUE [+] old paths to no longer exclude
--clear whether to replace the existing narrowspec
--force-delete-local-changes forces deletion of local changes when
narrowing
-e --ssh CMD specify ssh command to use
--remotecmd CMD specify hg command to run on the remote side
--insecure do not verify server certificate (ignoring
web.cacerts config)
(use 'hg tracked -h' to show more help)
[255]
$ hg tracked --import-rules doesnotexist
abort: cannot read narrowspecs from '$TESTTMP/narrow/doesnotexist': $ENOENT$
[255]
martinvonzUnsubmitted
Not Done

nit: s/>>/>/

martinvonz: nit: s/>>/>/
$ cat > specs <<EOF
> %include foo
> [include]
> path:widest/
> [exclude]
> path:inside/
> EOF
$ hg tracked --import-rules specs
abort: including other spec files using '%include' is not supported in narrowspec
[255]
$ cat > specs <<EOF
> [include]
> outisde
> [exclude]
> inside
> EOF
$ hg tracked --import-rules specs
comparing with ssh://user@dummy/master
searching for changes
looking for local changes to affected paths
deleting data/inside/f.i
deleting meta/inside/00manifest.i (tree !)
no changes found
saved backup bundle to $TESTTMP/narrow/.hg/strip-backup/*-widen.hg (glob)
adding changesets
adding manifests
adding file changes
added 2 changesets with 0 changes to 0 files
new changesets *:* (glob)
$ hg tracked
I path:outisde
X path:inside
Testing the --import-rules flag with --addinclude and --addexclude
$ cat > specs <<EOF
> [include]
> widest
> EOF
$ hg tracked --import-rules specs --addinclude 'wider/'
comparing with ssh://user@dummy/master
searching for changes
no changes found
saved backup bundle to $TESTTMP/narrow/.hg/strip-backup/*-widen.hg (glob)
adding changesets
adding manifests
adding file changes
added 3 changesets with 1 changes to 1 files
new changesets *:* (glob)
$ hg tracked
I path:outisde
I path:wider
I path:widest
X path:inside
$ cat > specs <<EOF
> [exclude]
> outside2
> EOF
$ hg tracked --import-rules specs --addexclude 'widest'
comparing with ssh://user@dummy/master
searching for changes
looking for local changes to affected paths
deleting data/widest/f.i
deleting meta/widest/00manifest.i (tree !)
$ hg tracked
I path:outisde
I path:wider
X path:inside
X path:outside2
X path:widest
$ hg tracked --import-rules specs --clear
The --clear option is not yet supported.
[1]
Testing with passing a out of wdir file
$ cat > ../nspecs <<EOF
> [include]
> widest
> EOF
$ hg tracked --import-rules ../nspecs
comparing with ssh://user@dummy/master
searching for changes
no changes found
saved backup bundle to $TESTTMP/narrow/.hg/strip-backup/*-widen.hg (glob)
adding changesets
adding manifests
adding file changes
added 3 changesets with 0 changes to 0 files
new changesets *:* (glob)