diff mbox series

[v2,1/4] fetch2/git: Add support for fast initial shallow fetch

Message ID 20250214155057.1748020-1-stefan-koch@siemens.com
State New
Headers show
Series [v2,1/4] fetch2/git: Add support for fast initial shallow fetch | expand

Commit Message

Stefan Koch Feb. 14, 2025, 3:50 p.m. UTC
When `ud.shallow_fast == 1`:
- Prefer an initial shallow clone over an initial full bare clone,
  while still utilizing any already existing full bare clones.
- `ud.shallow` will be automatically set to "1".

This improves:
- Resolves timeout issues during initial clones on slow internet connections
  by reducing the amount of data transferred.
- Eliminates the need to use an HTTPS tarball `SRC_URI`
  to reduce data transfer.
- Allows SSH-based authentication (e.g. cert and agent-based) when
  using non-public repos, so additional HTTPS tokens may not be required.

Signed-off-by: Stefan Koch <stefan-koch@siemens.com>
---
 lib/bb/fetch2/git.py | 96 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 75 insertions(+), 21 deletions(-)

Comments

Richard Purdie Feb. 17, 2025, 10:21 p.m. UTC | #1
On Fri, 2025-02-14 at 16:50 +0100, Koch, Stefan via lists.openembedded.org wrote:
> When `ud.shallow_fast == 1`:
> - Prefer an initial shallow clone over an initial full bare clone,
>   while still utilizing any already existing full bare clones.
> - `ud.shallow` will be automatically set to "1".
> 
> This improves:
> - Resolves timeout issues during initial clones on slow internet connections
>   by reducing the amount of data transferred.
> - Eliminates the need to use an HTTPS tarball `SRC_URI`
>   to reduce data transfer.
> - Allows SSH-based authentication (e.g. cert and agent-based) when
>   using non-public repos, so additional HTTPS tokens may not be required.
> 
> Signed-off-by: Stefan Koch <stefan-koch@siemens.com>
> ---
>  lib/bb/fetch2/git.py | 96 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 75 insertions(+), 21 deletions(-)

I'm left wondering if we should simply adopt this behaviour by default?
Do we really need to have users select the fast option? What would be
the reason someone would do that?

I continue to be quite worried about piling lots of different code
paths into the git fetcher and turning something that should be
relatively simple into something so complex nobody understand all the
different codepath combinations.

>              init_cmd += " --bare"
>          runfetchcmd(init_cmd, d, workdir=dest)
> -        runfetchcmd("%s remote add origin %s" % (ud.basecmd, ud.clonedir), d, workdir=dest)
> +        # Use repourl when creating a fast initial shallow clone
> +        runfetchcmd("%s remote add origin %s" % (ud.basecmd, shlex.quote(repourl) if ud.shallow_fast and not os.path.exists(ud.clonedir) else ud.clonedir), d, workdir=dest)
>  

I'd comment that the logic hidden here is just about unreadable and
should be split into two fetchcmds even if it does duplicate some code.

Cheers,

Richard
Stefan Koch Feb. 19, 2025, 4:29 p.m. UTC | #2
On Mon, 2025-02-17 at 22:21 +0000, Richard Purdie wrote:
> On Fri, 2025-02-14 at 16:50 +0100, Koch, Stefan via
> lists.openembedded.org wrote:
> > When `ud.shallow_fast == 1`:
> > - Prefer an initial shallow clone over an initial full bare clone,
> >   while still utilizing any already existing full bare clones.
> > - `ud.shallow` will be automatically set to "1".
> > 
> > This improves:
> > - Resolves timeout issues during initial clones on slow internet
> > connections
> >   by reducing the amount of data transferred.
> > - Eliminates the need to use an HTTPS tarball `SRC_URI`
> >   to reduce data transfer.
> > - Allows SSH-based authentication (e.g. cert and agent-based) when
> >   using non-public repos, so additional HTTPS tokens may not be
> > required.
> > 
> > Signed-off-by: Stefan Koch <stefan-koch@siemens.com>
> > ---
> >  lib/bb/fetch2/git.py | 96 ++++++++++++++++++++++++++++++++++------
> > ----
> >  1 file changed, 75 insertions(+), 21 deletions(-)
> 
> I'm left wondering if we should simply adopt this behaviour by
> default?
> Do we really need to have users select the fast option? What would be
> the reason someone would do that?
Yes, that makes sense.
If one wants to use a cached bare clone and updating only small blobs
it's enough to just unset BB_GIT_SHALLOW at all.

I'll send updated v3 patch, soon.
> 
> I continue to be quite worried about piling lots of different code
> paths into the git fetcher and turning something that should be
> relatively simple into something so complex nobody understand all the
> different codepath combinations.
> 
> >              init_cmd += " --bare"
> >          runfetchcmd(init_cmd, d, workdir=dest)
> > -        runfetchcmd("%s remote add origin %s" % (ud.basecmd,
> > ud.clonedir), d, workdir=dest)
> > +        # Use repourl when creating a fast initial shallow clone
> > +        runfetchcmd("%s remote add origin %s" % (ud.basecmd,
> > shlex.quote(repourl) if ud.shallow_fast and not
> > os.path.exists(ud.clonedir) else ud.clonedir), d, workdir=dest)
> >  
> 
> I'd comment that the logic hidden here is just about unreadable and
> should be split into two fetchcmds even if it does duplicate some
> code.
I'll send v3 patch, that uses regular if else with setting a repourl
variable.
> 
> Cheers,
> 
> Richard

Regards

Stefan
diff mbox series

Patch

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 6badda597..a38f6bf0b 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -207,7 +207,8 @@  class Git(FetchMethod):
         if ud.bareclone:
             ud.cloneflags += " --mirror"
 
-        ud.shallow = d.getVar("BB_GIT_SHALLOW") == "1"
+        ud.shallow_fast = d.getVar("BB_GIT_SHALLOW_FAST") == "1"
+        ud.shallow = d.getVar("BB_GIT_SHALLOW") == "1" or ud.shallow_fast
         ud.shallow_extra_refs = (d.getVar("BB_GIT_SHALLOW_EXTRA_REFS") or "").split()
 
         depth_default = d.getVar("BB_GIT_SHALLOW_DEPTH")
@@ -253,6 +254,7 @@  class Git(FetchMethod):
                 all(ud.shallow_depths[n] == 0 for n in ud.names)):
             # Shallow disabled for this URL
             ud.shallow = False
+            ud.shallow_fast = False
 
         if ud.usehead:
             # When usehead is set let's associate 'HEAD' with the unresolved
@@ -366,6 +368,33 @@  class Git(FetchMethod):
     def tarball_need_update(self, ud):
         return ud.write_tarballs and not os.path.exists(ud.fullmirror)
 
+    def lfs_fetch(self, ud, d, clonedir, revision, fetchall=False, progresshandler=None):
+        """Helper method for fetching Git LFS data"""
+        try:
+            if self._need_lfs(ud) and self._contains_lfs(ud, d, clonedir) and self._find_git_lfs(d) and len(revision):
+                # Using worktree with the revision because .lfsconfig may exists
+                worktree_add_cmd = "%s worktree add wt %s" % (ud.basecmd, revision)
+                runfetchcmd(worktree_add_cmd, d, log=progresshandler, workdir=clonedir)
+                lfs_fetch_cmd = "%s lfs fetch %s" % (ud.basecmd, "--all" if fetchall else "")
+                runfetchcmd(lfs_fetch_cmd, d, log=progresshandler, workdir=(clonedir + "/wt"))
+                worktree_rem_cmd = "%s worktree remove -f wt" % ud.basecmd
+                runfetchcmd(worktree_rem_cmd, d, log=progresshandler, workdir=clonedir)
+        except:
+            logger.warning("Fetching LFS did not succeed.")
+
+    @contextmanager
+    def create_atomic(self, filename):
+        """Create as a temp file and move atomically into position to avoid races"""
+        fd, tfile = tempfile.mkstemp(dir=os.path.dirname(filename))
+        try:
+            yield tfile
+            umask = os.umask(0o666)
+            os.umask(umask)
+            os.chmod(tfile, (0o666 & ~umask))
+            os.rename(tfile, filename)
+        finally:
+            os.close(fd)
+
     def try_premirror(self, ud, d):
         # If we don't do this, updating an existing checkout with only premirrors
         # is not possible
@@ -446,7 +475,40 @@  class Git(FetchMethod):
             if ud.proto.lower() != 'file':
                 bb.fetch2.check_network_access(d, clone_cmd, ud.url)
             progresshandler = GitProgressHandler(d)
-            runfetchcmd(clone_cmd, d, log=progresshandler)
+
+            # When ud.shallow_fast is enabled:
+            # Try creating an initial shallow clone
+            shallow_fast_state = False
+            if ud.shallow_fast:
+                tempdir = tempfile.mkdtemp(dir=d.getVar('DL_DIR'))
+                shallowclone = os.path.join(tempdir, 'git')
+                try:
+                    self.clone_shallow_local(ud, shallowclone, d)
+                    shallow_fast_state = True
+                except:
+                    logger.warning("Creating initial shallow clone failed, try regular clone now.")
+
+                # When the shallow clone has succeeded:
+                # Create shallow tarball
+                if shallow_fast_state:
+                    logger.info("Creating tarball of git repository")
+                    with self.create_atomic(ud.fullshallow) as tfile:
+                        runfetchcmd("tar -czf %s ." % tfile, d, workdir=shallowclone)
+                    runfetchcmd("touch %s.done" % ud.fullshallow, d)
+
+                # Always cleanup tempdir
+                bb.utils.remove(tempdir, recurse=True)
+
+                # When the shallow clone has succeeded:
+                # Use shallow tarball
+                if shallow_fast_state:
+                    ud.localpath = ud.fullshallow
+                    return
+
+            # When ud.shallow_fast is disabled or the shallow clone failed:
+            # Create an initial regular clone
+            if not shallow_fast_state:
+                runfetchcmd(clone_cmd, d, log=progresshandler)
 
         # Update the checkout if needed
         if self.clonedir_need_update(ud, d):
@@ -509,20 +571,6 @@  class Git(FetchMethod):
                     runfetchcmd("tar -cf - lfs | tar -xf - -C %s" % ud.clonedir, d, workdir="%s/.git" % ud.destdir)
 
     def build_mirror_data(self, ud, d):
-
-        # Create as a temp file and move atomically into position to avoid races
-        @contextmanager
-        def create_atomic(filename):
-            fd, tfile = tempfile.mkstemp(dir=os.path.dirname(filename))
-            try:
-                yield tfile
-                umask = os.umask(0o666)
-                os.umask(umask)
-                os.chmod(tfile, (0o666 & ~umask))
-                os.rename(tfile, filename)
-            finally:
-                os.close(fd)
-
         if ud.shallow and ud.write_shallow_tarballs:
             if not os.path.exists(ud.fullshallow):
                 if os.path.islink(ud.fullshallow):
@@ -533,7 +581,7 @@  class Git(FetchMethod):
                     self.clone_shallow_local(ud, shallowclone, d)
 
                     logger.info("Creating tarball of git repository")
-                    with create_atomic(ud.fullshallow) as tfile:
+                    with self.create_atomic(ud.fullshallow) as tfile:
                         runfetchcmd("tar -czf %s ." % tfile, d, workdir=shallowclone)
                     runfetchcmd("touch %s.done" % ud.fullshallow, d)
                 finally:
@@ -543,7 +591,7 @@  class Git(FetchMethod):
                 os.unlink(ud.fullmirror)
 
             logger.info("Creating tarball of git repository")
-            with create_atomic(ud.fullmirror) as tfile:
+            with self.create_atomic(ud.fullmirror) as tfile:
                 mtime = runfetchcmd("{} log --all -1 --format=%cD".format(ud.basecmd), d,
                         quiet=True, workdir=ud.clonedir)
                 runfetchcmd("tar -czf %s --owner oe:0 --group oe:0 --mtime \"%s\" ."
@@ -557,12 +605,15 @@  class Git(FetchMethod):
         - For BB_GIT_SHALLOW_REVS: git fetch --shallow-exclude=<revs> rev
         """
 
+        progresshandler = GitProgressHandler(d)
+        repourl = self._get_repo_url(ud)
         bb.utils.mkdirhier(dest)
         init_cmd = "%s init -q" % ud.basecmd
         if ud.bareclone:
             init_cmd += " --bare"
         runfetchcmd(init_cmd, d, workdir=dest)
-        runfetchcmd("%s remote add origin %s" % (ud.basecmd, ud.clonedir), d, workdir=dest)
+        # Use repourl when creating a fast initial shallow clone
+        runfetchcmd("%s remote add origin %s" % (ud.basecmd, shlex.quote(repourl) if ud.shallow_fast and not os.path.exists(ud.clonedir) else ud.clonedir), d, workdir=dest)
 
         # Check the histories which should be excluded
         shallow_exclude = ''
@@ -600,10 +651,14 @@  class Git(FetchMethod):
             # The ud.clonedir is a local temporary dir, will be removed when
             # fetch is done, so we can do anything on it.
             adv_cmd = 'git branch -f advertise-%s %s' % (revision, revision)
-            runfetchcmd(adv_cmd, d, workdir=ud.clonedir)
+            if not ud.shallow_fast:
+                runfetchcmd(adv_cmd, d, workdir=ud.clonedir)
 
             runfetchcmd(fetch_cmd, d, workdir=dest)
             runfetchcmd("%s update-ref %s %s" % (ud.basecmd, ref, revision), d, workdir=dest)
+            # Fetch Git LFS data for fast shallow clones
+            if ud.shallow_fast:
+                self.lfs_fetch(ud, d, dest, ud.revisions[ud.names[0]])
 
         # Apply extra ref wildcards
         all_refs_remote = runfetchcmd("%s ls-remote origin 'refs/*'" % ud.basecmd, \
@@ -629,7 +684,6 @@  class Git(FetchMethod):
             runfetchcmd("%s update-ref %s %s" % (ud.basecmd, ref, revision), d, workdir=dest)
 
         # The url is local ud.clonedir, set it to upstream one
-        repourl = self._get_repo_url(ud)
         runfetchcmd("%s remote set-url origin %s" % (ud.basecmd, shlex.quote(repourl)), d, workdir=dest)
 
     def unpack(self, ud, destdir, d):