Code review comment for ~nacc/git-ubuntu:bug-fixes-3

On 10.01.2018 [12:26:35 -0000], Andreas Hasenack wrote:
> Review: Needs Information
>
> Comments inline.
>
> I see some duplication of code, which was already there, but is
> unfortunate. Something for a follow-up branch?

Absolutely. Patches welcome, as well :)

> Diff comments:
>
> > diff --git a/gitubuntu/build.py b/gitubuntu/build.py
> > index 072d3e7..06b491f 100644
> > --- a/gitubuntu/build.py
> > +++ b/gitubuntu/build.py
> > @@ -878,16 +873,32 @@ def do_build_host_exitstack(
> > old_pardir_contents = set(os.listdir(os.pardir))
> > run(['dpkg-buildpackage'] + list(rem_args))
> > new_pardir_contents = set(os.listdir(os.pardir))
>
> Do we still need to take this snapshot of the pardir before and after
> the build? See below my other comment when we parse the changes file.

Hrm, you're right, probably not. We can just look that all the contents
of the changes file are present. I believe here I was trying to avoid
copying the orig tarball twice.

> > - temp_pardir_contents = new_pardir_contents - old_pardir_contents
> > - built_pardir_contents = []
> > - for f in temp_pardir_contents:
> > - shutil.copy(
> > - os.path.join(os.pardir, f),
> > - os.path.join(oldcwd, os.pardir)
> > - )
> > - built_pardir_contents.append(os.path.join(os.pardir, f))
> >
> > - return built_pardir_contents
> > + changes_file = None
> > + built_pardir_contents = []
> > + changes_files = [f for f in new_pardir_contents if 'changes' in f]
>
> Suggestion to be more specific about the file we want:
> changes_files = [f for f in new_pardir_contents if f.endswith('.changes')]

Yes, probably worth doing.

> > + if len(changes_files) == 0:
> > + logging.error("No changes file")
> > + elif len(changes_files) > 1:
> > + logging.error("Multiple changes files?")
> > + else:
> > + changes_file = changes_files.pop()
> > + # Intersect changes file contents with the new files in /tmp
> > + # to determine what was newly built
> > + with open(os.path.join(os.pardir, changes_file), 'rb') as changes_fobj:
>
> Do we still need this intersection? Won't the changes file have an
> exact list of what we need?

Good point.

>
> > + changes = Changes(changes_fobj)
> > + changes_named_files = [f['name'] for f in changes['files']]
> > + for new_file in new_pardir_contents:
> > + if new_file in changes_named_files:
> > + shutil.copy(
> > + os.path.join(os.pardir, new_file),
> > + os.path.join(oldcwd, os.pardir),
> > + )
> > + built_pardir_contents.append(
> > + os.path.join(os.pardir, new_file)
> > + )
> > +
> > + return [changes_file,] + built_pardir_contents
> >
> > def get_cmd_in_origpath(cmd):
> > return shutil.which(
> > @@ -1141,7 +1164,32 @@ def do_build_lxd_exitstack(
> > if filename_stripped
> > )
> >
> > - built_temp_contents = new_temp_contents - orig_temp_contents
> > + changes_file = None
> > + built_temp_contents = []
> > + changes_files = [f for f in new_temp_contents if 'changes' in f]
>
> Suggestion to be more specific about the file we want:
> changes_files = [f for f in new_pardir_contents if f.endswith('.changes')]

Ack.

> > + if len(changes_files) == 0:
> > + logging.error("No changes file")
> > + elif len(changes_files) > 1:
> > + logging.error("Multiple changes files?")
> > + else:
> > + changes_file = changes_files.pop()
> > + # Intersect changes file contents with the new files in /tmp
> > + # to determine what was newly built
> > + cmd = [
> > + lxc,
> > + 'file',
> > + 'pull',
> > + '%s%s' % (container_name, os.path.join('/tmp', changes_file)),
> > + os.pardir,
> > + ]
> > + run(cmd, env=env_unset_SNAP)
> > + with open(os.path.join(os.pardir, changes_file), 'rb') as changes_fobj:
> > + changes = Changes(changes_fobj)
> > + changes_named_files = [f['name'] for f in changes['files']]
> > + for new_file in new_temp_contents:
> > + if new_file in changes_named_files:
>
> Do we still need this intersection? Won't the changes file have an
> exact list of what we need?

Ditto.

> > + built_temp_contents.append(new_file)
> > +
> > cmd = [lxc, 'file', 'pull']
> > cmd.extend([
> > '%s%s' % (container_name, os.path.join('/tmp', f))
> > diff --git a/snap/snapcraft.yaml b/snap/snapcraft.yaml
> > index 0fbad2b..f8e6e5d 100644
> > --- a/snap/snapcraft.yaml
> > +++ b/snap/snapcraft.yaml
> > @@ -51,8 +50,7 @@ parts:
> > after: [git, pristine-tar, python3]
> > git:
> > plugin: autotools
> > - # want to use an upstream release of 2.14.2 or later, once available
> > - source: https://github.com/nacc/git/archive/v2.14.1_nacc_fixes.tar.gz
> > + source: https://github.com/git/git/archive/v2.14.3.tar.gz
>
> Any particular reason for these version changes? Just to grab the
> latest, or do we need something specific?

I should break this to its own commit, it's purely cleanup, as I was
maintaining a fork with a backport that is now upstream.

--
Nishanth Aravamudan
Ubuntu Server
Canonical Ltd