[1/2] fetch2/git: Add verbose logging support

Currently when git fetches fail, hardly any log is generated that
figures out what went wrong. To ease debugging introduced a flag,
BB_GIT_VERBOSE_FETCH, when enabled git fetch happens with verbose
logging support, thus user can share required logs at one shot.

Signed-off-by: Viswanath Kraleti <quic_vkraleti@quicinc.com>
---
 lib/bb/fetch2/git.py | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

On Fri, 2024-03-08 at 05:33 -0800, Viswanath Kraleti wrote:
> Currently when git fetches fail, hardly any log is generated that
> figures out what went wrong. To ease debugging introduced a flag,
> BB_GIT_VERBOSE_FETCH, when enabled git fetch happens with verbose
> logging support, thus user can share required logs at one shot.
> 
> Signed-off-by: Viswanath Kraleti <quic_vkraleti@quicinc.com>
> ---
>  lib/bb/fetch2/git.py | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> index b9dc576d..8193ce0c 100644
> --- a/lib/bb/fetch2/git.py
> +++ b/lib/bb/fetch2/git.py
> @@ -201,11 +201,15 @@ class Git(FetchMethod):
>  
>          ud.noshared = d.getVar("BB_GIT_NOSHARED") == "1"
>  
> +        verbose = d.getVar("BB_GIT_VERBOSE_FETCH") == "1"
> +
>          ud.cloneflags = "-n"
>          if not ud.noshared:
>              ud.cloneflags += " -s"
>          if ud.bareclone:
>              ud.cloneflags += " --mirror"
> +        if verbose:
> +            ud.cloneflags += " --verbose"
>  
>          ud.shallow = d.getVar("BB_GIT_SHALLOW") == "1"
>          ud.shallow_extra_refs = (d.getVar("BB_GIT_SHALLOW_EXTRA_REFS") or "").split()
> @@ -433,6 +437,7 @@ class Git(FetchMethod):
>          else:
>              needs_clone = True
>  
> +        verbose = d.getVar("BB_GIT_VERBOSE_FETCH") == "1"
>          # If the repo still doesn't exist, fallback to cloning it
>          if needs_clone:
>              # We do this since git will use a "-l" option automatically for local urls where possible,
> @@ -443,6 +448,8 @@ class Git(FetchMethod):
>                  if os.path.isdir(objects) and not os.path.islink(objects):
>                      repourl = repourl_path
>              clone_cmd = "LANG=C %s clone --bare --mirror %s %s --progress" % (ud.basecmd, shlex.quote(repourl), ud.clonedir)
> +            if verbose:
> +                clone_cmd = "GIT_TRACE_PACKET=1 GIT_TRACE=2 GIT_CURL_VERBOSE=1 " + clone_cmd + " --verbose"
>              if ud.proto.lower() != 'file':
>                  bb.fetch2.check_network_access(d, clone_cmd, ud.url)
>              progresshandler = GitProgressHandler(d)
> @@ -460,6 +467,8 @@ class Git(FetchMethod):
>                  fetch_cmd = "LANG=C %s fetch -f --progress %s refs/*:refs/*" % (ud.basecmd, shlex.quote(repourl))
>              else:
>                  fetch_cmd = "LANG=C %s fetch -f --progress %s refs/heads/*:refs/heads/* refs/tags/*:refs/tags/*" % (ud.basecmd, shlex.quote(repourl))
> +            if verbose:
> +                fetch_cmd = "GIT_TRACE_PACKET=1 GIT_TRACE=2 GIT_CURL_VERBOSE=1 " + fetch_cmd + " --verbose"
>              if ud.proto.lower() != 'file':
>                  bb.fetch2.check_network_access(d, fetch_cmd, ud.url)
>              progresshandler = GitProgressHandler(d)
> @@ -555,7 +564,11 @@ class Git(FetchMethod):
>  
>          The upstream url of the new clone isn't set at this time, as it'll be
>          set correctly when unpacked."""
> -        runfetchcmd("%s clone %s %s %s" % (ud.basecmd, ud.cloneflags, ud.clonedir, dest), d)
> +        verbose = d.getVar("BB_GIT_VERBOSE_FETCH") == "1"
> +        verbose_env_vars = ""
> +        if verbose:
> +            verbose_env_vars = " GIT_TRACE_PACKET=1 GIT_TRACE=2 GIT_CURL_VERBOSE=1"
> +        runfetchcmd("%s%s clone %s %s %s" % (verbose_env_vars, ud.basecmd, ud.cloneflags, ud.clonedir, dest), d)
>  
>          to_parse, shallow_branches = [], []
>          for name in ud.names:
> @@ -645,7 +658,11 @@ class Git(FetchMethod):
>  
>          clonedir_is_up_to_date = not self.clonedir_need_update(ud, d)
>          if clonedir_is_up_to_date:
> -            runfetchcmd("%s clone %s %s/ %s" % (ud.basecmd, ud.cloneflags, ud.clonedir, destdir), d)
> +            verbose_env_vars = ""
> +            verbose = d.getVar("BB_GIT_VERBOSE_FETCH") == "1"
> +            if verbose:
> +                verbose_env_vars = " GIT_TRACE_PACKET=1 GIT_TRACE=2 GIT_CURL_VERBOSE=1"
> +            runfetchcmd("%s%s clone %s %s/ %s" % (verbose_env_vars, ud.basecmd, ud.cloneflags, ud.clonedir, destdir), d)
>              source_found = True
>          else:
>              source_error.append("clone directory not available or not up to date: " + ud.clonedir)

I was asked about this patch. Apologies it didn't get a reply at the time.

I tend to get nervous about adding new options, it always raises the
question of why we don't have good defaults in the first place as I'd
prefer logs were generally useful. It makes it hard to people to find
them, then when they do, they'll set them in their config for evermore.

I then worry that this patch was something what was useful to debug a
specific issue once but in general might not be needed and it might not
be generally useful.

So I guess my question is how often you're hitting issues which this
needs and whether we could use the --vebose options to git in general
or not? Did you really need all the environment variables and the
verbose options or are some of them more useful than others?

I don't have a good feel for how much log output this generates either
so a bit more information/an example might help with that.

Cheers,

Richard