[08/11] support/download: implement source-check in git backend
diff mbox series

Message ID 20190103204026.23512-9-patrickdepinguin@gmail.com
State Superseded
Headers show
Series
  • support/download: fix scp and reintroduce source-check
Related show

Commit Message

Thomas De Schampheleire Jan. 3, 2019, 8:40 p.m. UTC
From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

The implementation is the same as originally was present.
It suffers from the disadvantage that an invalid revision on a valid URL
will not be detected.

However, git does not seem to allow a good way to remotely check the
validity of a revision, without cloning the repository.

For source-check, we don't want to do such download which can be large.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 support/download/git | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Comments

Thomas Petazzoni Jan. 3, 2019, 8:59 p.m. UTC | #1
Hello Thomas,

On Thu,  3 Jan 2019 21:40:23 +0100, Thomas De Schampheleire wrote:
> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> 
> The implementation is the same as originally was present.
> It suffers from the disadvantage that an invalid revision on a valid URL
> will not be detected.
> 
> However, git does not seem to allow a good way to remotely check the
> validity of a revision, without cloning the repository.
> 
> For source-check, we don't want to do such download which can be large.

While I understand the limitation, I don't really agree with the
conclusion: we should go ahead and download the full thing. Indeed the
selling argument for source-check in your cover letter is precisely to
verify that the version of the package that has been committed by
someone is *really* available. If there's no version check in the git,
bzr and cvs source-check implementation, it makes the selling point of
the cover letter a bit moot, no?

Of course, I realize that your primary interest is in hg, and hg has
this capability. But still we should ensure git/bzr/cvs provide the same
guarantees, by falling back to the slower but working method of
downloading everything.

Best regards,

Thomas
Yann E. MORIN Jan. 3, 2019, 10:18 p.m. UTC | #2
Thomas, DS, Thomas P, All,

On 2019-01-03 21:59 +0100, Thomas Petazzoni spake thusly:
> On Thu,  3 Jan 2019 21:40:23 +0100, Thomas De Schampheleire wrote:
> > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> > 
> > The implementation is the same as originally was present.
> > It suffers from the disadvantage that an invalid revision on a valid URL
> > will not be detected.
> > 
> > However, git does not seem to allow a good way to remotely check the
> > validity of a revision, without cloning the repository.
> > 
> > For source-check, we don't want to do such download which can be large.
> 
> While I understand the limitation, I don't really agree with the
> conclusion: we should go ahead and download the full thing. Indeed the
> selling argument for source-check in your cover letter is precisely to
> verify that the version of the package that has been committed by
> someone is *really* available. If there's no version check in the git,
> bzr and cvs source-check implementation, it makes the selling point of
> the cover letter a bit moot, no?

Agreed here. *If* we were to re-add support for source-check, it should
really behave as it says on the can: check that the source really
exists. If a backend can't do a cheap check, then it has to download
everything.

> Of course, I realize that your primary interest is in hg, and hg has
> this capability. But still we should ensure git/bzr/cvs provide the same
> guarantees, by falling back to the slower but working method of
> downloading everything.

Yup.

But this is not an endorsement of re-adding source-check, mind you1 ;-)
I'm still unconvinced we need it.

Regards,
Yann E. MORIN.
Thomas De Schampheleire Jan. 8, 2019, 12:12 p.m. UTC | #3
El jue., 3 ene. 2019 a las 22:00, Thomas Petazzoni
(<thomas.petazzoni@bootlin.com>) escribió:
>
> Hello Thomas,
>
> On Thu,  3 Jan 2019 21:40:23 +0100, Thomas De Schampheleire wrote:
> > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
> >
> > The implementation is the same as originally was present.
> > It suffers from the disadvantage that an invalid revision on a valid URL
> > will not be detected.
> >
> > However, git does not seem to allow a good way to remotely check the
> > validity of a revision, without cloning the repository.
> >
> > For source-check, we don't want to do such download which can be large.
>
> While I understand the limitation, I don't really agree with the
> conclusion: we should go ahead and download the full thing. Indeed the
> selling argument for source-check in your cover letter is precisely to
> verify that the version of the package that has been committed by
> someone is *really* available. If there's no version check in the git,
> bzr and cvs source-check implementation, it makes the selling point of
> the cover letter a bit moot, no?
>
> Of course, I realize that your primary interest is in hg, and hg has
> this capability. But still we should ensure git/bzr/cvs provide the same
> guarantees, by falling back to the slower but working method of
> downloading everything.

Yes, I can agree with this. If you accept adding source-check in the
first place, then I'll improve these backends.

/Thomas

Patch
diff mbox series

diff --git a/support/download/git b/support/download/git
index 17ca04eb98..54f3494c61 100755
--- a/support/download/git
+++ b/support/download/git
@@ -7,6 +7,7 @@  set -E
 #
 # Options:
 #   -q          Be quiet.
+#   -C          Only check that the changeset exists in the remote repository
 #   -r          Clone and archive sub-modules.
 #   -o FILE     Generate archive in FILE.
 #   -u URI      Clone from repository at URI.
@@ -48,6 +49,7 @@  recurse=0
 while getopts "${BR_BACKEND_DL_GETOPTS}" OPT; do
     case "${OPT}" in
     q)  verbose=-q; exec >/dev/null;;
+    C)  checkonly=1;;
     r)  recurse=1;;
     o)  output="${OPTARG}";;
     u)  uri="${OPTARG}";;
@@ -61,6 +63,20 @@  done
 
 shift $((OPTIND-1)) # Get rid of our options
 
+# Caller needs to single-quote its arguments to prevent them from
+# being expanded a second time (in case there are spaces in them)
+_git() {
+    eval GIT_DIR="${git_cache}/.git" ${GIT} "${@}"
+}
+
+if [ -n "${checkonly}" ]; then
+    # TODO this check only checks that the remote repository exists, not that
+    # it actually contains the requested revision. And for checkonly, we want
+    # to avoid creating a clone first.
+    _git ls-remote --heads "${@}" "'${uri}'" > /dev/null
+    exit ${?}
+fi
+
 # Create and cd into the directory that will contain the local git cache
 git_cache="${dl_dir}/git"
 mkdir -p "${git_cache}"
@@ -69,12 +85,6 @@  pushd "${git_cache}" >/dev/null
 # Any error now should try to recover
 trap _on_error ERR
 
-# Caller needs to single-quote its arguments to prevent them from
-# being expanded a second time (in case there are spaces in them)
-_git() {
-    eval GIT_DIR="${git_cache}/.git" ${GIT} "${@}"
-}
-
 # Create a warning file, that the user should not use the git cache.
 # It's ours. Our precious.
 cat <<-_EOF_ >"${dl_dir}/git.readme"