[bitbake-devel] [PATCH 5/7] fetch2/git: prevent access to non-existing clonedir

[Urs Fässler]

On Thu, 2018-10-04 at 10:12 +0100, Richard Purdie wrote:
> On Wed, 2018-10-03 at 12:04 +0000, Urs Fässler wrote:
> > A user friendly error is throw when neither the clonedir nor
> > fullshallow exist. Without the check, a difficult to interpret
> > error
> > is throw from within the fetch command.
> > 
> > Signed-off-by: Urs Fässler <urs.fassler at bbv.ch>
> > Signed-off-by: Pascal Bach <pascal.bach at siemens.com>
> > ---
> >  lib/bb/fetch2/git.py  |  4 +++-
> >  lib/bb/tests/fetch.py | 13 +++++++++++++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> > index f13a25f9..fbd30011 100644
> > --- a/lib/bb/fetch2/git.py
> > +++ b/lib/bb/fetch2/git.py
> > @@ -479,8 +479,10 @@ class Git(FetchMethod):
> >          if ud.shallow and os.path.exists(ud.fullshallow) and
> > self.clonedir_need_update(ud, d):
> >              bb.utils.mkdirhier(destdir)
> >              runfetchcmd("tar -xzf %s" % ud.fullshallow, d,
> > workdir=destdir)
> > -        else:
> > +        elif not self.clonedir_need_update(ud, d):
> >              runfetchcmd("%s clone %s %s/ %s" % (ud.basecmd,
> > ud.cloneflags, ud.clonedir, destdir), d)
> > +        else:
> > +            bb.fatal("no up to date source found")
> 
> I appreciate there are other places the fetcher code calls bb.fatal()
> but this is problematic.
> 
> The intent is to make the fetcher code a library which other code
> call
> call into and this is already how OE treats it. Python libraries
> should
> raise exceptions on errors and allow the parent code to handle it,
> not
> raise SystemExit() exceptions.
> 
> Is there some other way we can raise a decent error even if means
> adding a new exception (which can subclass one of the other fetch
> exceptions)?

There is the UnpackError exception for "General fetcher exception when
something happens incorrectly when unpacking" I can use.

When I use exceptions, I don't need the LogRecord class I extracted in
patch "[PATCH 4/7] tests/data: extract LogRecord into own file"
anymore.
I am not sure if I should remove that from the patch-set since it is
unrelated. But it imho improves the code base a bit.

Regards
Urs