Patchwork [bitbake-devel] fetch2: Cleanup file checksum generation

login
register
mail settings
Submitter Jacob Kroon
Date April 26, 2014, 2:32 p.m.
Message ID <1398522738-8979-1-git-send-email-jacob.kroon@mikrodidakt.se>
Download mbox | patch
Permalink /patch/71019/
State New
Headers show

Comments

Jacob Kroon - April 26, 2014, 2:32 p.m.
Cleanup the fix done in f9416e76e272ba3249abff099f6f3a47fe82e03e.

Instead of adding continue statements we can just move the last
statements into the final else-clause.

Signed-off-by: Jacob Kroon <jacob.kroon@mikrodidakt.se>
---
 lib/bb/fetch2/__init__.py | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
Paul Eggleton - April 28, 2014, 11:23 a.m.
On Saturday 26 April 2014 16:32:18 Jacob Kroon wrote:
> Cleanup the fix done in f9416e76e272ba3249abff099f6f3a47fe82e03e.
> 
> Instead of adding continue statements we can just move the last
> statements into the final else-clause.
> 
> Signed-off-by: Jacob Kroon <jacob.kroon@mikrodidakt.se>
> ---
>  lib/bb/fetch2/__init__.py | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> index 5a03a0e..bc4e8fd 100644
> --- a/lib/bb/fetch2/__init__.py
> +++ b/lib/bb/fetch2/__init__.py
> @@ -985,15 +985,12 @@ def get_file_checksums(filelist, pn):
>                      checksum = checksum_file(f)
>                      if checksum:
>                          checksums.append((f, checksum))

Note the above, then see below.

> -            continue
>          elif os.path.isdir(pth):
>              checksums.extend(checksum_dir(pth))
> -            continue
>          else:
>              checksum = checksum_file(pth)
> -
> -        if checksum:
> -            checksums.append((pth, checksum))
> +            if checksum:
> +                checksums.append((pth, checksum))
> 
>      checksums.sort(key=operator.itemgetter(1))
>      return checksums

This cleanup seems innocuous but unfortunately it does cause a change in 
behaviour - now for the glob case we will get the last item duplicated because 
it will be added to the list a second time at the end.

Richard can you please revert this?

Cheers,
Paul
Jacob Kroon - April 28, 2014, 11:55 a.m.
On Mon, Apr 28, 2014 at 1:23 PM, Paul Eggleton <
paul.eggleton@linux.intel.com> wrote:

> On Saturday 26 April 2014 16:32:18 Jacob Kroon wrote:
> > Cleanup the fix done in f9416e76e272ba3249abff099f6f3a47fe82e03e.
> >
> > Instead of adding continue statements we can just move the last
> > statements into the final else-clause.
> >
> > Signed-off-by: Jacob Kroon <jacob.kroon@mikrodidakt.se>
> > ---
> >  lib/bb/fetch2/__init__.py | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> > index 5a03a0e..bc4e8fd 100644
> > --- a/lib/bb/fetch2/__init__.py
> > +++ b/lib/bb/fetch2/__init__.py
> > @@ -985,15 +985,12 @@ def get_file_checksums(filelist, pn):
> >                      checksum = checksum_file(f)
> >                      if checksum:
> >                          checksums.append((f, checksum))
>
> Note the above, then see below.
>
> > -            continue
> >          elif os.path.isdir(pth):
> >              checksums.extend(checksum_dir(pth))
> > -            continue
> >          else:
> >              checksum = checksum_file(pth)
> > -
> > -        if checksum:
> > -            checksums.append((pth, checksum))
> > +            if checksum:
> > +                checksums.append((pth, checksum))
> >
> >      checksums.sort(key=operator.itemgetter(1))
> >      return checksums
>
> This cleanup seems innocuous but unfortunately it does cause a change in
> behaviour - now for the glob case we will get the last item duplicated
> because
> it will be added to the list a second time at the end.
>
>
Sorry..
But I can't see why this would cause a different behaviour. Both previous
'continue' statements applied to the outer for-loop. Unless they have an
effect on the if-statements aswell ?
Paul Eggleton - April 28, 2014, 12:08 p.m.
On Monday 28 April 2014 13:55:48 Jacob Kroon wrote:
> On Mon, Apr 28, 2014 at 1:23 PM, Paul Eggleton <
> 
> paul.eggleton@linux.intel.com> wrote:
> > On Saturday 26 April 2014 16:32:18 Jacob Kroon wrote:
> > > Cleanup the fix done in f9416e76e272ba3249abff099f6f3a47fe82e03e.
> > > 
> > > Instead of adding continue statements we can just move the last
> > > statements into the final else-clause.
> > > 
> > > Signed-off-by: Jacob Kroon <jacob.kroon@mikrodidakt.se>
> > > ---
> > > 
> > >  lib/bb/fetch2/__init__.py | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> > > index 5a03a0e..bc4e8fd 100644
> > > --- a/lib/bb/fetch2/__init__.py
> > > +++ b/lib/bb/fetch2/__init__.py
> > > 
> > > @@ -985,15 +985,12 @@ def get_file_checksums(filelist, pn):
> > >                      checksum = checksum_file(f)
> > >                      
> > >                      if checksum:
> > >                          checksums.append((f, checksum))
> > 
> > Note the above, then see below.
> > 
> > > -            continue
> > > 
> > >          elif os.path.isdir(pth):
> > >              checksums.extend(checksum_dir(pth))
> > > 
> > > -            continue
> > > 
> > >          else:
> > >              checksum = checksum_file(pth)
> > > 
> > > -
> > > -        if checksum:
> > > -            checksums.append((pth, checksum))
> > > +            if checksum:
> > > +                checksums.append((pth, checksum))
> > > 
> > >      checksums.sort(key=operator.itemgetter(1))
> > >      return checksums
> > 
> > This cleanup seems innocuous but unfortunately it does cause a change in
> > behaviour - now for the glob case we will get the last item duplicated
> > because
> > it will be added to the list a second time at the end.
> 
> Sorry..
> But I can't see why this would cause a different behaviour. Both previous
> 'continue' statements applied to the outer for-loop. Unless they have an
> effect on the if-statements aswell ?

Because if we expand a glob we add each expanded item to the list, but because 
we use the "checksum" variable in that inner loop as well it will be left 
assigned to the last added item; thus without the continue when it is checked 
again at the end it will still be assigned and will add it to the list a 
second time.

Cheers,
Paul
Paul Eggleton - April 28, 2014, 12:24 p.m.
On Monday 28 April 2014 13:08:06 Paul Eggleton wrote:
> On Monday 28 April 2014 13:55:48 Jacob Kroon wrote:
> > On Mon, Apr 28, 2014 at 1:23 PM, Paul Eggleton <
> > 
> > paul.eggleton@linux.intel.com> wrote:
> > > On Saturday 26 April 2014 16:32:18 Jacob Kroon wrote:
> > > > Cleanup the fix done in f9416e76e272ba3249abff099f6f3a47fe82e03e.
> > > > 
> > > > Instead of adding continue statements we can just move the last
> > > > statements into the final else-clause.
> > > > 
> > > > Signed-off-by: Jacob Kroon <jacob.kroon@mikrodidakt.se>
> > > > ---
> > > > 
> > > >  lib/bb/fetch2/__init__.py | 7 ++-----
> > > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> > > > index 5a03a0e..bc4e8fd 100644
> > > > --- a/lib/bb/fetch2/__init__.py
> > > > +++ b/lib/bb/fetch2/__init__.py
> > > > 
> > > > @@ -985,15 +985,12 @@ def get_file_checksums(filelist, pn):
> > > >                      checksum = checksum_file(f)
> > > >                      
> > > >                      if checksum:
> > > >                          checksums.append((f, checksum))
> > > 
> > > Note the above, then see below.
> > > 
> > > > -            continue
> > > > 
> > > >          elif os.path.isdir(pth):
> > > >              checksums.extend(checksum_dir(pth))
> > > > 
> > > > -            continue
> > > > 
> > > >          else:
> > > >              checksum = checksum_file(pth)
> > > > 
> > > > -
> > > > -        if checksum:
> > > > -            checksums.append((pth, checksum))
> > > > +            if checksum:
> > > > +                checksums.append((pth, checksum))
> > > > 
> > > >      checksums.sort(key=operator.itemgetter(1))
> > > >      return checksums
> > > 
> > > This cleanup seems innocuous but unfortunately it does cause a change in
> > > behaviour - now for the glob case we will get the last item duplicated
> > > because
> > > it will be added to the list a second time at the end.
> > 
> > Sorry..
> > But I can't see why this would cause a different behaviour. Both previous
> > 'continue' statements applied to the outer for-loop. Unless they have an
> > effect on the if-statements aswell ?
> 
> Because if we expand a glob we add each expanded item to the list, but
> because we use the "checksum" variable in that inner loop as well it will
> be left assigned to the last added item; thus without the continue when it
> is checked again at the end it will still be assigned and will add it to
> the list a second time.

Oops, it seems I missed that the second checksums.append() is now part of the 
else block and thus I'm talking complete nonsense above. No need to revert.

Sorry for the noise...

Cheers,
Paul

Patch

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index 5a03a0e..bc4e8fd 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -985,15 +985,12 @@  def get_file_checksums(filelist, pn):
                     checksum = checksum_file(f)
                     if checksum:
                         checksums.append((f, checksum))
-            continue
         elif os.path.isdir(pth):
             checksums.extend(checksum_dir(pth))
-            continue
         else:
             checksum = checksum_file(pth)
-
-        if checksum:
-            checksums.append((pth, checksum))
+            if checksum:
+                checksums.append((pth, checksum))
 
     checksums.sort(key=operator.itemgetter(1))
     return checksums