diff mbox

pkg-download.mk: Fix shallow clone

Message ID 1354445767-15308-1-git-send-email-sho@relinux.de
State Accepted
Headers show

Commit Message

Stephan Hoffmann Dec. 2, 2012, 10:56 a.m. UTC
Some git versions (e.g. 1.7.0) do not treat trying to shallow clone
a non existing branch or tag as a fatal error but report a warning
and clone HEAD instead. Thus the fallback mechanism does not work
in this case.

This patch introduces a check for the presence of the requested
version as a branch or tag before trying the shallow clone. It
also removes the need to do two clones when a sha1 is given as
a packege version.

Signed-off-by: Stephan Hoffmann <sho@relinux.de>
---
 package/pkg-download.mk |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

Comments

Peter Korsgaard Dec. 2, 2012, 4:05 p.m. UTC | #1
>>>>> "Stephan" == Stephan Hoffmann <sho@relinux.de> writes:

 Stephan> Some git versions (e.g. 1.7.0) do not treat trying to shallow clone
 Stephan> a non existing branch or tag as a fatal error but report a warning
 Stephan> and clone HEAD instead. Thus the fallback mechanism does not work
 Stephan> in this case.

 Stephan> This patch introduces a check for the presence of the requested
 Stephan> version as a branch or tag before trying the shallow clone. It
 Stephan> also removes the need to do two clones when a sha1 is given as
 Stephan> a packege version.

Thanks, committed.

I really do think we need to move the git handling into an external
script, because this isn't really readable anymore - But that's for
post-2012.11.

I've changed the cut -b 42- into cut -f2- as that's more robust (and
readable imho).
Danomi Manchego Dec. 6, 2012, 2:14 a.m. UTC | #2
Hi all,

I ran into a minor problem with the shallow clone changes.  The test for
doing a git shallow clone uses a grep.  From pkg-download.mk:

   ((test `git ls-remote  $($(PKG)_SITE) | cut -f 2- | grep
$($(PKG)_DL_VERSION)` && \

If the grep returns multiple matches, then the "test" will fail, and then
go on to do a full clone. The failure prints out:

   bash: test: too many arguments

It's because the grep filter matches multiple items in the ls-remote
output.  For example, if you try to get your kernel from "
http://arago-project.org/git/projects/linux-davinci.git", specifying a
version of "03.22.00.02", then you will get two matches:

   $ git ls-remote
http://arago-project.org/git/projects/linux-davinci.git| cut -f 2- |
grep 03.22.00.02
    refs/heads/03.22.00.02
    refs/tags/DEV_DAVINCIPSP_03.22.00.02

There are ways to massage the grep to do better (maybe use "grep -q", or
grep for "/string$" instead of just "string"), but the ls-remote command
seems to take a ref directly.  I.e.

   $ git ls-remote
http://arago-project.org/git/projects/linux-davinci.git03.22.00.02 |
cut -f 2-
   refs/heads/03.22.00.02
   $
   $ git ls-remote
http://arago-project.org/git/projects/linux-davinci.gitDEV_DAVINCIPSP_03.22.00.02
| cut -f 2-
   refs/tags/DEV_DAVINCIPSP_03.22.00.02
   $
   $ git ls-remote
http://arago-project.org/git/projects/linux-davinci.gitJUNK | cut -f
2-
   $

So I'm wondering why the recent changes in this area used grep at all,
rather than asking ls-remote directly.  I.e.:

 ((test `git ls-remote $($(PKG)_SITE) $($(PKG)_DL_VERSION) | cut -f 2-` && \

If there is feedback that the above is desirable, then I can send in the
patch.

Danomi -
Arnout Vandecappelle Dec. 6, 2012, 7:03 a.m. UTC | #3
On 06/12/12 03:14, Danomi Manchego wrote:
> So I'm wondering why the recent changes in this area used grep at all, rather than asking ls-remote directly.  I.e.:
>
> ((test `git ls-remote $($(PKG)_SITE) $($(PKG)_DL_VERSION) | cut -f 2-` && \
>
> If there is feedback that the above is desirable, then I can send in the patch.

  The argument of test should be quoted to avoid getting the same problem again
when the ref matches more than once for some reason.

  Or even better: use the exit code:

git ls-remote --exit-code $($(PKG)_SITE) $($(PKG)_DL_VERSION) > /dev/null 2>&1 && \

(hopefully the --exit-code option exists in older gits as well...)

  Regards,
  Arnout
Stephan Hoffmann Dec. 6, 2012, 10:34 a.m. UTC | #4
Am 06.12.2012 03:14, schrieb Danomi Manchego:
> Hi all,
>
> I ran into a minor problem with the shallow clone changes.  The test
> for doing a git shallow clone uses a grep.  From pkg-download.mk
> <http://pkg-download.mk>:
>
>   ((test `git ls-remote  $($(PKG)_SITE) | cut -f 2- | grep
> $($(PKG)_DL_VERSION)` && \
>
> If the grep returns multiple matches, then the "test" will fail, and
> then go on to do a full clone. The failure prints out:
>
>    bash: test: too many arguments
>
> It's because the grep filter matches multiple items in the ls-remote
> output.  For example, if you try to get your kernel from
> "http://arago-project.org/git/projects/linux-davinci.git", specifying
> a version of "03.22.00.02", then you will get two matches:
>
>    $ git ls-remote
> http://arago-project.org/git/projects/linux-davinci.git | cut -f 2- |
> grep 03.22.00.02
>     refs/heads/03.22.00.02 <http://03.22.00.02>
>     refs/tags/DEV_DAVINCIPSP_03.22.00.02
>
> There are ways to massage the grep to do better (maybe use "grep -q",
> or grep for "/string$" instead of just "string"), but the ls-remote
> command seems to take a ref directly.  I.e.
>
>    $ git ls-remote
> http://arago-project.org/git/projects/linux-davinci.git 03.22.00.02 |
> cut -f 2-
>    refs/heads/03.22.00.02 <http://03.22.00.02>
>    $
>    $ git ls-remote
> http://arago-project.org/git/projects/linux-davinci.git
> DEV_DAVINCIPSP_03.22.00.02 | cut -f 2-
>    refs/tags/DEV_DAVINCIPSP_03.22.00.02
>    $
>    $ git ls-remote
> http://arago-project.org/git/projects/linux-davinci.git JUNK | cut -f 2-
>    $
>
> So I'm wondering why the recent changes in this area used grep at all,
> rather than asking ls-remote directly.  I.e.:
>
> ((test `git ls-remote $($(PKG)_SITE) $($(PKG)_DL_VERSION) | cut -f 2-`
> && \
Hello Danomi,

this is quite simple to answer: because I didn't find out this possibility.
>
> If there is feedback that the above is desirable, then I can send in
> the patch.
If you send a patch I am quite sure to be able to test it.

Kind regards

Stephan
>
> Danomi -
>
Stephan Hoffmann Dec. 6, 2012, 10:59 a.m. UTC | #5
Am 06.12.2012 08:03, schrieb Arnout Vandecappelle:
> On 06/12/12 03:14, Danomi Manchego wrote:
>> So I'm wondering why the recent changes in this area used grep at
>> all, rather than asking ls-remote directly.  I.e.:
>>
>> ((test `git ls-remote $($(PKG)_SITE) $($(PKG)_DL_VERSION) | cut -f
>> 2-` && \
>>
>> If there is feedback that the above is desirable, then I can send in
>> the patch.
>
>  The argument of test should be quoted to avoid getting the same
> problem again
> when the ref matches more than once for some reason.
>
>  Or even better: use the exit code:
>
> git ls-remote --exit-code $($(PKG)_SITE) $($(PKG)_DL_VERSION) >
> /dev/null 2>&1 && \
>
> (hopefully the --exit-code option exists in older gits as well...)
Hi,

sorry to say, but my git 1.7.0 does not know anything about a
--exit-code option and ls-remote always gives a exit code of 0 as long
as the repository can be connected.

Maybe its worth to think about adding a host-git to buildroot to
overcome such compatibility issues?

Kind regards

Stephan
>
>  Regards,
>  Arnout
>
Danomi Manchego Dec. 6, 2012, 11:28 a.m. UTC | #6
Arnout - thanks for the feedback.
Stephan - thanks for looking into the the --exit-status on older gits.

Since the --exit-status isn't universally available, maybe the simplest
thing to do is to add quotes, and drop the "cut" and "grep":

 ((test "`git ls-remote $($(PKG)_SITE) $($(PKG)_DL_VERSION)`" && \

Assume no objections or better ideas come in, I'll send in a patch tonight.

Thanks,
Danomi -
diff mbox

Patch

diff --git a/package/pkg-download.mk b/package/pkg-download.mk
index 6c57244..627e22d 100644
--- a/package/pkg-download.mk
+++ b/package/pkg-download.mk
@@ -74,12 +74,19 @@  domainseparator=$(if $(1),$(1),/)
 ################################################################################
 
 # Try a shallow clone - but that only works if the version is a ref (tag or
-# branch). Fall back on a full clone if it's a generic sha1.
+# branch). Before trying to do a shallow clone we check if $($(PKG)_DL_VERSION)
+# is in the list provided by git ls-remote. If not we fall back on a full clone.
+#
+# Messages for the type of clone used are provided to ease debugging in case of
+# problems
 define DOWNLOAD_GIT
 	test -e $(DL_DIR)/$($(PKG)_SOURCE) || \
 	(pushd $(DL_DIR) > /dev/null && \
-	 ($(GIT) clone --depth 1 -b $($(PKG)_DL_VERSION) --bare $($(PKG)_SITE) $($(PKG)_BASE_NAME) || \
-	  $(GIT) clone --bare $($(PKG)_SITE) $($(PKG)_BASE_NAME)) && \
+	 ((test `git ls-remote  $($(PKG)_SITE) | cut -b 42- | grep $($(PKG)_DL_VERSION)` && \
+	   echo "Doing shallow clone" && \
+	   $(GIT) clone --depth 1 -b $($(PKG)_DL_VERSION) --bare $($(PKG)_SITE) $($(PKG)_BASE_NAME)) || \
+	  (echo "Doing full clone" && \
+	   $(GIT) clone --bare $($(PKG)_SITE) $($(PKG)_BASE_NAME))) && \
 	pushd $($(PKG)_BASE_NAME) > /dev/null && \
 	$(GIT) archive --format=tar --prefix=$($(PKG)_BASE_NAME)/ $($(PKG)_DL_VERSION) | \
 		gzip -c > $(DL_DIR)/$($(PKG)_SOURCE) && \