search

sourcegraph / srclib

srclib is a polyglot code analysis library, built for hackability. It consists of language analysis toolchains (currently for Go and Java, with Python, JavaScript, and Ruby in beta) with a common output format, and a CLI tool for running the analysis.
https://srclib.org
Other
942 stars 62 forks source link

Updated sqs/fileset #276

Closed alexsaveliev closed 8 years ago

alexsaveliev commented 8 years ago
dmitshur commented 8 years ago

Updating sqs/fileset dependency to sqs/fileset@012a6d31cbc4d10f3148918e0a37cd333db58a34, which is its latest master version, LGTM.

But I am very puzzled by the .gitignore of this repo ignoring all of /vendor/ contents except vendor.json:

https://github.com/sourcegraph/srclib/blob/488ce0095f1a04e713bed8c715017b28796b3610/.gitignore#L4

As a result, only vendor.json changes in this PR, instead of the expected diff that looks more like this.

How does that work, and what's the motivation to do vendoring while not actually committing anything to the repository?

alexsaveliev commented 8 years ago

How does that work, and what's the motivation to do vendoring while not actually committing anything to the repository?

Default make target does govendor sync which pulls all the dependencies. I think having both vendor.json descriptor and source code of dependencies is incorrect. The same way NodeJS-based source code is organized - package.json checked in into SCM, dependencies are fetched on demand. If you think we should keep dependencies source code - please let me know and I'll add it back. See also similar PR sourcegraph/srclib-go#80. Please note that srclib-go had an example why it's bad to keep both descriptor and source code - Godeps referred to the non-existent hash while dependency source code was there. Looks like somebody forgot to update either hash or dependency source code

dmitshur commented 8 years ago

If you think we should keep dependencies source code - please let me know and I'll add it back.

I don't have strong opinions on how vendoring is done, so no need to make immediate changes. It was a question.

I didn't know about govendor sync command, but I see how it works now, thanks for explaining.