Add ensure-file parser package.

Add ensure-file parser package.

This breaks out the ensurefile parsing logic into its own package with tests. It doesn't alter cipd to take advantage of this parser yet, to simplify this CL.

R=vadimsh@chromium.org
BUG=663843
Review-Url: https://codereview.chromium.org/2651863002
Committed: https://github.com/luci/luci-go/commit/46649a0e29a25732c07c49fc2030369620993fce

[iannucci, 2017-01-24 00:51:53]

Description was changed from

==========
Add ensure-file parser package.

R=vadimsh@chromium.org
BUG=663843
==========

to

==========
Add ensure-file parser package.

This breaks out the ensurefile parsing logic into its own package with tests. It
doesn't alter cipd to take advantage of this parser yet, to simplify this CL.

R=vadimsh@chromium.org
BUG=663843
==========

[iannucci, 2017-01-24 00:52:32]

On 2017/01/24 00:50:19, iannucci wrote:

PTAL, this is just the parser, there's no affect on cipd's correctness here.

Please let me know if you see any missing test cases that you'd like to ensure
are covered.

[Vadim Sh., 2017-01-24 01:28:17]

https://codereview.chromium.org/2651863002/diff/1/cipd/client/cipd/ensure/doc.go
File cipd/client/cipd/ensure/doc.go (right):

https://codereview.chromium.org/2651863002/diff/1/cipd/client/cipd/ensure/doc...
cipd/client/cipd/ensure/doc.go:9: // at which versions, and in which
subdirectories they should be installed.
'they' doesn't grammatically fit here

https://codereview.chromium.org/2651863002/diff/1/cipd/client/cipd/ensure/fil...
File cipd/client/cipd/ensure/file.go (right):

https://codereview.chromium.org/2651863002/diff/1/cipd/client/cipd/ensure/fil...
cipd/client/cipd/ensure/file.go:29: var rootRegexp =
regexp.MustCompile((`^(|([a-z0-9_ \-]+/)*[a-z0-9_ \-]+)$`))
what about A-Z? 

This regexp in general seems too restrictive to me (and unreadable, like they
all are). Why not something based on path.Clean, e.g. check that root ==
path.Clean(root) and doesn't start with "./" or "../" or "/"

https://codereview.chromium.org/2651863002/diff/1/cipd/client/cipd/ensure/fil...
cipd/client/cipd/ensure/file.go:42: // rslv MUST be provided (this will panic if
rslv is nil).
stale

https://codereview.chromium.org/2651863002/diff/1/cipd/client/cipd/ensure/fil...
cipd/client/cipd/ensure/file.go:50: curRoot := ""
please make it more generic right away by using

settings := map[string]string{}
directives := map[string]string{}

and delaying value validations and construction of File until the very end. So
main scanner loop does only syntax scan, and semantics is checked later. 

I don't think it'll add any significant amount of code. I think it'll make
scanner loop even simpler.

[iannucci, 2017-01-24 02:11:43]

Much better, ptal

https://codereview.chromium.org/2651863002/diff/1/cipd/client/cipd/ensure/doc.go
File cipd/client/cipd/ensure/doc.go (right):

https://codereview.chromium.org/2651863002/diff/1/cipd/client/cipd/ensure/doc...
cipd/client/cipd/ensure/doc.go:9: // at which versions, and in which
subdirectories they should be installed.
On 2017/01/24 01:28:17, Vadim Sh. wrote:
> 'they' doesn't grammatically fit here

I think it might, but it's really awkward (it was referring to 'packages').
Fixed.

https://codereview.chromium.org/2651863002/diff/1/cipd/client/cipd/ensure/fil...
File cipd/client/cipd/ensure/file.go (right):

https://codereview.chromium.org/2651863002/diff/1/cipd/client/cipd/ensure/fil...
cipd/client/cipd/ensure/file.go:29: var rootRegexp =
regexp.MustCompile((`^(|([a-z0-9_ \-]+/)*[a-z0-9_ \-]+)$`))
On 2017/01/24 01:28:17, Vadim Sh. wrote:
> what about A-Z? 
> 
> This regexp in general seems too restrictive to me (and unreadable, like they
> all are). Why not something based on path.Clean, e.g. check that root ==
> path.Clean(root) and doesn't start with "./" or "../" or "/"

I was worried about case-insensitive filesystems, but I'm also OK with
path.Clean. Done (and added some more tests).

https://codereview.chromium.org/2651863002/diff/1/cipd/client/cipd/ensure/fil...
cipd/client/cipd/ensure/file.go:42: // rslv MUST be provided (this will panic if
rslv is nil).
On 2017/01/24 01:28:17, Vadim Sh. wrote:
> stale

Done.

https://codereview.chromium.org/2651863002/diff/1/cipd/client/cipd/ensure/fil...
cipd/client/cipd/ensure/file.go:50: curRoot := ""
On 2017/01/24 01:28:17, Vadim Sh. wrote:
> please make it more generic right away by using
> 
> settings := map[string]string{}
> directives := map[string]string{}
> 
> and delaying value validations and construction of File until the very end. So
> main scanner loop does only syntax scan, and semantics is checked later. 
> 
> I don't think it'll add any significant amount of code. I think it'll make
> scanner loop even simpler.

Done :)

[iannucci, 2017-01-24 02:17:06]

Moved item parser stuff to its own file with better docs. Hopefully it makes it
very clear how to extend the parser.

[Vadim Sh., 2017-01-24 21:52:17]

(https://img.memecdn.com/soon-my-friend-soon_o_175785.jpg)
(I didn't forget)

[iannucci, 2017-01-24 22:02:56]

On 2017/01/24 21:52:17, Vadim Sh. wrote:
> (https://img.memecdn.com/soon-my-friend-soon_o_175785.jpg)
> (I didn't forget)

haha :D

[Vadim Sh., 2017-01-24 23:31:04]

lgtm with minor nits

It is still a bit more entangled that I'd like, but good enough. At least it's
clear how to extend it.

Just FYI here's how I'd do it :P

Ideally I'd like the parser to have no knowledge of semantics of vars, e.g. you
give it a file and it spits out tuple ([]Directive, []PackageDef), where:

type Directive {
  LineNo int

  Name string
  Value string
}

type PackageDef {
  LineNo int
  
  Name string // template or not
  Version string
  
  Vars []Var // @Root and what not
}

type Var struct {
  LineNo int
  
  Name string
  Value string
}

And this output is validated in a second pass to produce properly structured
objects, e.g. File in your CL.

(In my ancient experience of writing parsers I found that it pays to have very
clear separation between stages, even if it entails slightly more code).

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
File cipd/client/cipd/ensure/file.go (right):

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/file.go:44: return fmt.Errorf("bad root path: absolute
paths not allowed: %q", root)
check what happens if "C:\" is specified. In fact, I think we should just forbid
":" there.

Also check there's no unexpected magic if using "//host/path".

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/file.go:80: tokens = append(tokens, "") // no value
means a value of ""
setting empty root will look awkward :(

$Root  # huh?

Maybe we should introduce '='?

$Root = # still huh, but less so

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/file.go:84: switch c := tok1[0]; c {
strictly speaking, this is not UTF-8 safe, it takes first byte, not first rune.
I assume there are no runes that start with '@' or '$', so it's fine?

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/file.go:105: switch err {
if err != nil && err != errSkipTemplate {
  return nil, err
}

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/file.go:161: err = nil
this is not necessary

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
File cipd/client/cipd/ensure/item_parsers.go (right):

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/item_parsers.go:32: return fmt.Errorf("$ServiceURL may
only be set once per file")
so we are not enforcing that $settings appear on top?

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
File cipd/client/cipd/ensure/resolved_file.go (right):

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/resolved_file.go:7: type ResolvedFile struct {
I'd prefer this to be defined closer to where File is.

[iannucci, 2017-01-25 03:03:35]

PTAL @ $settings change

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
File cipd/client/cipd/ensure/file.go (right):

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/file.go:44: return fmt.Errorf("bad root path: absolute
paths not allowed: %q", root)
On 2017/01/24 23:31:04, Vadim Sh. wrote:
> check what happens if "C:\" is specified. In fact, I think we should just
forbid
> ":" there.
> 
> Also check there's no unexpected magic if using "//host/path".

I ban all backslashes above, and Clean is supposed to remove subsequent slashes.
I'll add a check for ':' and more tests.

Done

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/file.go:80: tokens = append(tokens, "") // no value
means a value of ""
On 2017/01/24 23:31:04, Vadim Sh. wrote:
> setting empty root will look awkward :(
> 
> $Root  # huh?
> 
> Maybe we should introduce '='?
> 
> $Root = # still huh, but less so

discussed offline, will keep empty token. it's weird but *shrug*.

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/file.go:84: switch c := tok1[0]; c {
On 2017/01/24 23:31:04, Vadim Sh. wrote:
> strictly speaking, this is not UTF-8 safe, it takes first byte, not first
rune.
> I assume there are no runes that start with '@' or '$', so it's fine?

It is actually utf8 safe, because any multi-byte rune MUST have the high bit set
in the first byte.

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/file.go:105: switch err {
On 2017/01/24 23:31:04, Vadim Sh. wrote:
> if err != nil && err != errSkipTemplate {
>   return nil, err
> }

yeah this code is wayyy leftover :D. fixed.

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/file.go:161: err = nil
On 2017/01/24 23:31:04, Vadim Sh. wrote:
> this is not necessary

Hm... it WAS at some point. done.

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
File cipd/client/cipd/ensure/item_parsers.go (right):

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/item_parsers.go:32: return fmt.Errorf("$ServiceURL may
only be set once per file")
On 2017/01/24 23:31:04, Vadim Sh. wrote:
> so we are not enforcing that $settings appear on top?

ah, I meant to do that. I'll do that. PTAL

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
File cipd/client/cipd/ensure/resolved_file.go (right):

https://codereview.chromium.org/2651863002/diff/60001/cipd/client/cipd/ensure...
cipd/client/cipd/ensure/resolved_file.go:7: type ResolvedFile struct {
On 2017/01/24 23:31:04, Vadim Sh. wrote:
> I'd prefer this to be defined closer to where File is.

I thought there was going to be more stuff here, moved to file.go

[Vadim Sh., 2017-01-25 03:13:19]

lgtm

[iannucci, 2017-01-25 04:19:02]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2017-01-25 04:19:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 04:26:02]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485317942234550,
"parent_rev": "be2fce189470736da7baa8f5127cb7209ea4aae3", "commit_rev":
"46649a0e29a25732c07c49fc2030369620993fce"}

[commit-bot: I haz the power, 2017-01-25 04:26:09]

Description was changed from

==========
Add ensure-file parser package.

This breaks out the ensurefile parsing logic into its own package with tests. It
doesn't alter cipd to take advantage of this parser yet, to simplify this CL.

R=vadimsh@chromium.org
BUG=663843
==========

to

==========
Add ensure-file parser package.

This breaks out the ensurefile parsing logic into its own package with tests. It
doesn't alter cipd to take advantage of this parser yet, to simplify this CL.

R=vadimsh@chromium.org
BUG=663843

Review-Url: https://codereview.chromium.org/2651863002
Committed:
https://github.com/luci/luci-go/commit/46649a0e29a25732c07c49fc2030369620993fce
==========

[commit-bot: I haz the power, 2017-01-25 04:26:10]

Committed patchset #5 (id:80001) as
https://github.com/luci/luci-go/commit/46649a0e29a25732c07c49fc2030369620993fce