Switch data_source dependency to GitPython #409

tardyp · 2021-03-29T10:22:58Z

While neither pygit2 or GitPython have a formidable API and documentation

GitPython has the advantage of being pure python and support
same proxy feature as git.

I removed some of the tests, which to me are much over mocked, and do not really test much.

I also changed the diff algorithm so that it only diff the whole thing instead of considering diff commit by commits.

pombredanne

Thank you++ 👍
I added a few nitpickings for your consideration.
Also you may want to add the pinned deps of gitpython to the requirements file?

pombredanne · 2021-03-29T13:32:11Z

vulnerabilities/data_source.py

-            if previous_commit is None:
-                previous_commit = commit
-                continue
+        if cutoff_commit is None:


Could if cutoff_commit be enough here?

I usually restrict to None comparaison when possible (explicit is better than implicit) and I can see that most of this file is also using this principle

pombredanne · 2021-03-29T13:34:21Z

vulnerabilities/data_source.py

+            if d.new_file:
+                added_files.add(abspath)
+            elif d.a_blob and d.b_blob and d.a_path != d.b_path:
+                added_files.add(abspath)  # consider moved files as added


If you do not mind, I like to avoid end of line comments when possible

Suggested change

added_files.add(abspath) # consider moved files as added

# consider moved files as added

added_files.add(abspath)

pombredanne · 2021-03-29T13:37:21Z

vulnerabilities/data_source.py

+            abspath = os.path.join(self.config.working_directory, d.b_path)
+            if d.new_file:
+                added_files.add(abspath)
+            elif d.a_blob and d.b_blob and d.a_path != d.b_path:


Would it help if the part d.a_blob and d.b_blob here and below was refactored in one test followed by two sub-tests: one on path and one blob?

vulnerabilities/tests/test_data_source.py

pombredanne · 2021-03-29T13:44:46Z

vulnerabilities/tests/test_data_source.py

+    repo.index.commit("Added a new file")
+
+    yield str(git_dir)
+    shutil.rmtree(git_dir)


What about putting the shutil.rmtree( in a try/finally block?

tardyp · 2021-03-29T18:44:10Z

@pombredanne I think I have answered to your remarks.

i fixed the remaining tests, and nix build.

While neither pygit2 or GitPython have a formidable API and documentation GitPython has the advantage of being pure python and support same proxy feature as git. Signed-off-by: Pierre Tardy <[email protected]>

sbs2001 · 2021-04-05T07:54:25Z

vulnerabilities/data_source.py

+        def _is_binary(d: DiffIndex):
+            if not d.b_blob:
+                return False
+            try:
+                d.b_blob.data_stream.read().decode()
+            except UnicodeDecodeError:
+                return True
+            return False


What is this function trying to do ? Is it used to check whether the diff blob consists of binary ? This would return true if the diff is about something like a tar ball ?

Indeed, it checks by trying to decode it with default encoding settings.

The previous code had some logic of checking if the data was binary to not count it as a diff.

The is_binary semantic was provided by libgit2. The logic is quite complicated:
https://github.com/libgit2/libgit2/blob/508361401fbb5d87118045eaeae3356a729131aa/src/buf_text.c#L268

@sbs2001 as you suggested in the Gitter chat I guess we could use binaryornot which does a decent job there and is used extensively in scancode-toolkit:

https://github.com/audreyfeldroy/binaryornot

https://github.com/nexB/thirdparty-packages/blob/main/pypi/binaryornot-0.4.4.tar.gz.ABOUT

https://github.com/nexB/typecode/blob/157b7be15c6cb4be754f915bde7974a3e0284961/src/typecode/contenttype.py#L926

I changed the PR to use binaryornot. I am not sure why the nix CI is failing :-/

Signed-off-by: Pierre Tardy <[email protected]>

sbs2001 · 2021-04-16T05:45:16Z

vulnerabilities/data_source.py

-                # simplistic. This does not cover file renames, copies &
-                # deletions.
-                if d.status == pygit2.GIT_DELTA_ADDED:
+        for d in cutoff_commit.diff(self._repo.head.commit):


This errors out with older versions of git (mine was 2.25.1). See gitpython-developers/GitPython#1016.

We could just add a note about this in the README I guess ?

sbs2001 · 2021-04-16T05:56:51Z

@tardyp this looks good to me aside from the issue with git version . IMHO the nix test failure has nothing to do with your changes. Those are probably caused to due to some sync issues. @rolfschr ping ! .

Thanks for this, and sorry for being late.

sbs2001 · 2021-04-19T07:37:04Z

etc/nix/flake.nix

@@ -74,7 +74,7 @@
            name = "vulnerablecode-${version}";
            src = vulnerablecode-src;
            dontConfigure = true; # do not use ./configure
-            propagatedBuildInputs = [ pythonEnv postgresql ];
+            propagatedBuildInputs = [ pythonEnv postgresql gitMinimal];


@rolfschr is this fine on your side ?

Look good to me. @tardyp I gues you could remove the ssl cert file fix for pygit2 in the nix test if you remove pygit2 anyways.

diff --git a/etc/nix/flake.nix b/etc/nix/flake.nix index 1a65045..84ed672 100644 --- a/etc/nix/flake.nix +++ b/etc/nix/flake.nix @@ -119,10 +119,6 @@ buildInputs = [ wget vulnerablecode ]; - # Used by pygit2. - # See https://github.com/NixOS/nixpkgs/pull/72544#issuecomment-582674047. - SSL_CERT_FILE = "${pkgs.cacert}/etc/ssl/certs/ca-bundle.crt"; - unpackPhase = "true"; buildPhase = ''

sbs2001

Merging this, I'll take care of nix tests and docs in another PR.
Thanks @tardyp :)

tardyp · 2021-04-22T12:36:09Z

Sorry I was in vacation.. thanks for merging.

pombredanne · 2021-04-22T13:18:28Z

Sorry I was in vacation.. thanks for merging.

Sure thing. Thanks for the contrib!

tardyp force-pushed the gitpython branch 2 times, most recently from e63af64 to 892dd7a Compare March 29, 2021 11:39

pombredanne requested changes Mar 29, 2021

View reviewed changes

tardyp force-pushed the gitpython branch 2 times, most recently from 3b02bc6 to 0c5b28e Compare March 29, 2021 18:38

tardyp force-pushed the gitpython branch from a868c92 to ff83fbd Compare March 31, 2021 09:57

Switch data_source dependency to GitPython

85d15d2

While neither pygit2 or GitPython have a formidable API and documentation GitPython has the advantage of being pure python and support same proxy feature as git. Signed-off-by: Pierre Tardy <[email protected]>

tardyp force-pushed the gitpython branch from ff83fbd to 85d15d2 Compare March 31, 2021 09:59

sbs2001 reviewed Apr 5, 2021

View reviewed changes

use binaryornot instead of handmade function

9be7cee

Signed-off-by: Pierre Tardy <[email protected]>

sbs2001 reviewed Apr 16, 2021

View reviewed changes

sbs2001 mentioned this pull request Apr 19, 2021

Fix nix Github workflow #444

Merged

sbs2001 reviewed Apr 19, 2021

View reviewed changes

Merge branch 'main' into gitpython

74a1afa

sbs2001 self-requested a review April 22, 2021 11:41

sbs2001 approved these changes Apr 22, 2021

View reviewed changes

sbs2001 merged commit 13eaa01 into aboutcode-org:main Apr 22, 2021

tardyp deleted the gitpython branch April 22, 2021 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch data_source dependency to GitPython #409

Switch data_source dependency to GitPython #409

tardyp commented Mar 29, 2021

pombredanne left a comment

pombredanne Mar 29, 2021

tardyp Mar 29, 2021

pombredanne Mar 29, 2021

pombredanne Mar 29, 2021

pombredanne Mar 29, 2021

tardyp commented Mar 29, 2021 •

edited

Loading

sbs2001 Apr 5, 2021

tardyp Apr 5, 2021

pombredanne Apr 14, 2021

tardyp Apr 15, 2021

sbs2001 Apr 16, 2021

sbs2001 commented Apr 16, 2021

sbs2001 Apr 19, 2021

rolfschr Apr 19, 2021

sbs2001 left a comment

tardyp commented Apr 22, 2021

pombredanne commented Apr 22, 2021

	added_files.add(abspath) # consider moved files as added
	# consider moved files as added
	added_files.add(abspath)

Switch data_source dependency to GitPython #409

Switch data_source dependency to GitPython #409

Conversation

tardyp commented Mar 29, 2021

pombredanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tardyp commented Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbs2001 commented Apr 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbs2001 left a comment

Choose a reason for hiding this comment

tardyp commented Apr 22, 2021

pombredanne commented Apr 22, 2021

tardyp commented Mar 29, 2021 •

edited

Loading