This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4526

narrow: validate spec files are well-formed during clone (BC)
ClosedPublic

Authored by indygreg on Sep 11 2018, 2:52 PM.

Details

Reviewers
durin42
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG8301741e1f89: narrow: validate spec files are well-formed during clone (BC)
Summary

Previously, specfiles would get read then normalized. We want
specfiles to be normalized on read so there is no confusion about
what the format of specfiles should be.

This commit validates the parsed result of --specfile. If entries
aren't prefixed, an error is raised.

Previously, validation would occur at exchange time, hence why we
dropped a line of test output related to server iteraction.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Sep 11 2018, 2:52 PM
martinvonz added a subscriber: martinvonz.Sep 11 2018, 6:23 PM
martinvonz added inline comments.
tests/test-narrow-clone-no-ellipsis.t
133

Maybe "file:" was just a typo for "path:" so it's better to replace it by "path:" (instead of having both)?

tests/test-narrow-clone.t
281

Any idea why this changed? Should it actually change in this patch?

indygreg updated this revision to Diff 10898.Sep 11 2018, 6:29 PM
indygreg added inline comments.Sep 11 2018, 6:36 PM
tests/test-narrow-clone-no-ellipsis.t
133

I was wondering about that too. I agree it feels wrong. I didn't feel like spending the brain cycles on it.

tests/test-narrow-clone.t
281

It changed because there are multiple layers of validation. And the layer added by this patch occurs before the validation is performed in the exchange layer (the exchange layer is what's printing the removed line).

The new behavior feels better to me because it fails faster.

martinvonz accepted this revision.Sep 11 2018, 6:38 PM
This revision is now accepted and ready to land.Sep 11 2018, 6:38 PM
Closed by commit rHG8301741e1f89: narrow: validate spec files are well-formed during clone (BC) (authored by indygreg). · Explain WhySep 11 2018, 7:42 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 10898

hgext/narrow/narrowcommands.py

raise error.Abort(_("cannot read narrowspecs from '%s': %s") % raise error.Abort(_("cannot read narrowspecs from '%s': %s") %
(filepath, encoding.strtolocal(inst.strerror))) (filepath, encoding.strtolocal(inst.strerror)))
includes, excludes, profiles = sparse.parseconfig(ui, fdata, 'narrow') includes, excludes, profiles = sparse.parseconfig(ui, fdata, 'narrow')
if profiles: if profiles:
raise error.Abort(_("cannot specify other files using '%include' in" raise error.Abort(_("cannot specify other files using '%include' in"
" narrowspec")) " narrowspec"))
narrowspec.validatepatterns(includes)
narrowspec.validatepatterns(excludes)
# narrowspec is passed so we should assume that user wants narrow clone # narrowspec is passed so we should assume that user wants narrow clone
opts_narrow = True opts_narrow = True
opts['include'].extend(includes) opts['include'].extend(includes)
opts['exclude'].extend(excludes) opts['exclude'].extend(excludes)
if opts_narrow: if opts_narrow:
def pullbundle2extraprepare_widen(orig, pullop, kwargs): def pullbundle2extraprepare_widen(orig, pullop, kwargs):
# Create narrow spec patterns from clone flags # Create narrow spec patterns from clone flags

tests/test-narrow-clone-no-ellipsis.t

$ cd .. $ cd ..
Testing the --narrowspec flag to clone Testing the --narrowspec flag to clone
$ cat >> narrowspecs <<EOF $ cat >> narrowspecs <<EOF
> %include foo > %include foo
> [include] > [include]
> path:dir/tests/ > path:dir/tests/
> file:dir/src/f12 > path:file:dir/src/f12
martinvonzUnsubmitted
Not Done

Maybe "file:" was just a typo for "path:" so it's better to replace it by "path:" (instead of having both)?

martinvonz: Maybe "file:" was just a typo for "path:" so it's better to replace it by "path:" (instead of…
indygregAuthorUnsubmitted
Not Done

I was wondering about that too. I agree it feels wrong. I didn't feel like spending the brain cycles on it.

indygreg: I was wondering about that too. I agree it feels wrong. I didn't feel like spending the brain…
> EOF > EOF
$ hg clone ssh://user@dummy/master specfile --narrowspec narrowspecs $ hg clone ssh://user@dummy/master specfile --narrowspec narrowspecs
reading narrowspec from '$TESTTMP/narrowspecs' reading narrowspec from '$TESTTMP/narrowspecs'
abort: cannot specify other files using '%include' in narrowspec abort: cannot specify other files using '%include' in narrowspec
[255] [255]
$ cat > narrowspecs <<EOF $ cat > narrowspecs <<EOF
> [include] > [include]
> path:dir/tests/ > path:dir/tests/
> file:dir/src/f12 > path:file:dir/src/f12
> EOF > EOF
$ hg clone ssh://user@dummy/master specfile --narrowspec narrowspecs $ hg clone ssh://user@dummy/master specfile --narrowspec narrowspecs
reading narrowspec from '$TESTTMP/narrowspecs' reading narrowspec from '$TESTTMP/narrowspecs'
requesting all changes requesting all changes
adding changesets adding changesets
adding manifests adding manifests
adding file changes adding file changes

tests/test-narrow-clone.t

$ cd .. $ cd ..
Testing the --narrowspec flag to clone Testing the --narrowspec flag to clone
$ cat >> narrowspecs <<EOF $ cat >> narrowspecs <<EOF
> %include foo > %include foo
> [include] > [include]
> path:dir/tests/ > path:dir/tests/
> dir/src/f12 > path:file:dir/src/f12
> EOF > EOF
$ hg clone ssh://user@dummy/master specfile --narrowspec narrowspecs $ hg clone ssh://user@dummy/master specfile --narrowspec narrowspecs
reading narrowspec from '$TESTTMP/narrowspecs' reading narrowspec from '$TESTTMP/narrowspecs'
abort: cannot specify other files using '%include' in narrowspec abort: cannot specify other files using '%include' in narrowspec
[255] [255]
$ cat > narrowspecs <<EOF $ cat > narrowspecs <<EOF
> [include] > [include]
> path:dir/tests/ > path:dir/tests/
> file:dir/src/f12 > path:file:dir/src/f12
> EOF > EOF
$ hg clone ssh://user@dummy/master specfile --narrowspec narrowspecs $ hg clone ssh://user@dummy/master specfile --narrowspec narrowspecs
reading narrowspec from '$TESTTMP/narrowspecs' reading narrowspec from '$TESTTMP/narrowspecs'
requesting all changes requesting all changes
adding changesets adding changesets
adding manifests adding manifests
adding file changes adding file changes
$ cat > narrowspecs <<EOF $ cat > narrowspecs <<EOF
> [include] > [include]
> glob:** > glob:**
> EOF > EOF
$ hg clone ssh://user@dummy/master badspecfile --narrowspec narrowspecs $ hg clone ssh://user@dummy/master badspecfile --narrowspec narrowspecs
reading narrowspec from '$TESTTMP/narrowspecs' reading narrowspec from '$TESTTMP/narrowspecs'
requesting all changes
martinvonzUnsubmitted
Not Done

Any idea why this changed? Should it actually change in this patch?

martinvonz: Any idea why this changed? Should it actually change in this patch?
indygregAuthorUnsubmitted
Not Done

It changed because there are multiple layers of validation. And the layer added by this patch occurs before the validation is performed in the exchange layer (the exchange layer is what's printing the removed line).

The new behavior feels better to me because it fails faster.

indygreg: It changed because there are multiple layers of validation. And the layer added by this patch…
abort: invalid prefix on narrow pattern: glob:** abort: invalid prefix on narrow pattern: glob:**
(narrow patterns must begin with one of the following: path:, rootfilesin:) (narrow patterns must begin with one of the following: path:, rootfilesin:)
[255] [255]