This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Download Raw Diff

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

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Sep 11 2018, 2:52 PM

Herald added a reviewer: durin42. · View Herald TranscriptSep 11 2018, 2:52 PM

Herald added a reviewer: hg-reviewers. · View Herald Transcript

Herald added a subscriber: mercurial-devel. · View Herald Transcript

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

indygreg added a child revision: D4530: narrow: drop support for remote expansion (BC).Sep 11 2018, 6:59 PM

indygreg updated this revision to Diff 10902.

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

			Path	Packages
M			hgext/narrow/narrowcommands.py (3 lines)
M			tests/test-narrow-clone-no-ellipsis.t (4 lines)
M			tests/test-narrow-clone.t (5 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	10889		Sep 11 2018, 2:52 PM	★	★
Diff 2	10898		Sep 11 2018, 6:28 PM	★	★
Diff 3	10902		Sep 11 2018, 6:59 PM	★	★
Diff 4	10907	rHG8301741e1f893ab1f3ba2c73d8df717cf8271eeb	Sep 11 2018, 2:47 PM	★	★

Status	Author	Revision
Closed	indygreg	D4541 hg: write narrow patterns after repo creation
Closed	indygreg	D4540 narrow: don't wrap exchange.pull() during clone
Closed	indygreg	D4539 exchange: support defining narrow file patterns for pull
Closed	indygreg	D4538 commands: pass include and exclude options to hg.clone()
Closed	indygreg	D4537 localrepo: add requirement when narrow files creation option present
Closed	indygreg	D4536 hg: recognize include and exclude patterns when cloning
Closed	indygreg	D4535 hg: allow extra arguments to be passed to repo creation
Closed	indygreg	D4534 localrepo: move repo creation logic out of localrepository.__init__
Closed	indygreg	D4533 localrepo: pass ui to newreporequirements()
Closed	indygreg	D4532 narrow: set opts['narrow'] instead of local variable
Closed	indygreg	D4530 narrow: drop support for remote expansion (BC)
Closed	indygreg	D4526 narrow: validate spec files are well-formed during clone (BC)
Closed	indygreg	D4525 narrow: validate patterns on incoming bundle2 part
Closed	indygreg	D4524 narrowspec: validate patterns when loading and saving spec file
Closed	indygreg	D4523 narrow: validate patterns returned by expandnarrow
Closed	indygreg	D4522 narrowspec: limit patterns to path: and rootfilesin: (BC)

Diff 10907

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]