Page MenuHomeSoftware Heritage

Add mapping of sha1 with swh ID
ClosedPublic

Authored by TG1999 on Jan 8 2021, 5:28 PM.

Details

Summary

Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Add mapping of sha1 with swh ID

Build has FAILED

Patch application report for D4829 (id=17118)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit 7cb20067fc6438834fe29b1f08a1b2591a7e388d
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Sat Jan 9 10:49:54 2021 +0530

    Add mapping of sha1 with swh ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

Link to build: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/18/
See console output for more information: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/18/console

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 9 2021, 6:20 AM
Harbormaster failed remote builds in B18260: Diff 17118!

Add mapping of sha1 with swh ID

Build has FAILED

Patch application report for D4829 (id=17119)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit a4c4853361f59da81ac8ccd920e00b7a05b10cd8
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Sat Jan 9 16:10:11 2021 +0530

    Add mapping of sha1 with swh ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

Link to build: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/19/
See console output for more information: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/19/console

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 9 2021, 11:41 AM
Harbormaster failed remote builds in B18261: Diff 17119!
  • Ignore mypy for psycopg2

Build is green

Patch application report for D4829 (id=17207)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit 530f4bcab595994b2ea939d379f47c4f23fa1d62
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Thu Jan 14 16:22:10 2021 +0530

    Ignore mypy for psycopg2
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

commit a4c4853361f59da81ac8ccd920e00b7a05b10cd8
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Sat Jan 9 16:10:11 2021 +0530

    Add mapping of sha1 with swh ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

See https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/20/ for more details.

vlorentz added a subscriber: vlorentz.

(marking this diff as "changes requested" because it needs tests, as discussed on IRC)

This revision now requires changes to proceed.Jan 14 2021, 12:14 PM
  • Add tests for sha1 mapping with SWHID

Build has FAILED

Patch application report for D4829 (id=17282)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit f4d53ae1b7be721365b3a28f36606454449a5cf4
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Sat Jan 16 11:57:59 2021 +0530

    Add tests for sha1 mapping with SWHID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

commit 530f4bcab595994b2ea939d379f47c4f23fa1d62
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Thu Jan 14 16:22:10 2021 +0530

    Ignore mypy for psycopg2
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

commit a4c4853361f59da81ac8ccd920e00b7a05b10cd8
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Sat Jan 9 16:10:11 2021 +0530

    Add mapping of sha1 with swh ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

Link to build: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/21/
See console output for more information: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/21/console

Add tests for mapping of sha1 with SWHID

Build has FAILED

Patch application report for D4829 (id=17284)

Could not rebase; Attempt merge onto 31111a6fe2...

Updating 31111a6..7689644
Fast-forward
 conftest.py                              |  1 +
 mypy.ini                                 |  3 +++
 requirements-swh.txt                     |  1 +
 requirements.txt                         | 15 +++++++++++----
 swh/clearlydefined/mapping_utils.py      | 23 +++++++++++++++++++++++
 swh/clearlydefined/tests/test_mapping.py | 22 ++++++++++++++++++++++
 6 files changed, 61 insertions(+), 4 deletions(-)
 create mode 100644 conftest.py
 create mode 100644 swh/clearlydefined/mapping_utils.py
 create mode 100644 swh/clearlydefined/tests/test_mapping.py
Changes applied before test
commit 7689644b85d367f365f446a61be3a0d99361573f
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Sat Jan 16 12:04:21 2021 +0530

    Add tests for mapping of sha1 with SWHID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

commit 530f4bcab595994b2ea939d379f47c4f23fa1d62
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Thu Jan 14 16:22:10 2021 +0530

    Ignore mypy for psycopg2
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

commit a4c4853361f59da81ac8ccd920e00b7a05b10cd8
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Sat Jan 9 16:10:11 2021 +0530

    Add mapping of sha1 with swh ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

Link to build: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/22/
See console output for more information: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/22/console

Add mapping of sha1 with swh ID

Build has FAILED

Patch application report for D4829 (id=17285)

Could not rebase; Attempt merge onto 31111a6fe2...

Updating 31111a6..51b3f61
Fast-forward
 conftest.py                              |  1 +
 mypy.ini                                 |  3 +++
 requirements-swh.txt                     |  1 +
 requirements.txt                         | 15 +++++++++++----
 swh/clearlydefined/mapping_utils.py      | 23 +++++++++++++++++++++++
 swh/clearlydefined/tests/test_mapping.py | 22 ++++++++++++++++++++++
 6 files changed, 61 insertions(+), 4 deletions(-)
 create mode 100644 conftest.py
 create mode 100644 swh/clearlydefined/mapping_utils.py
 create mode 100644 swh/clearlydefined/tests/test_mapping.py
Changes applied before test
commit 51b3f6165a6e6a543eac1a4f0d57af9ccd8d3dac
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Sat Jan 16 12:08:35 2021 +0530

    Add mapping of sha1 with SWHID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

commit a4c4853361f59da81ac8ccd920e00b7a05b10cd8
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Sat Jan 9 16:10:11 2021 +0530

    Add mapping of sha1 with swh ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

Link to build: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/23/
See console output for more information: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/23/console

Add mapping of sha1 with swh ID

Build has FAILED

Patch application report for D4829 (id=17286)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit e0d98a3df14ccf3b1c0ce68f556087bd5bf791b5
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Sat Jan 16 12:15:47 2021 +0530

    Add mapping sha1 with SWHID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

Link to build: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/24/
See console output for more information: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/24/console

Add mapping of sha1 with swh ID

Build has FAILED

Patch application report for D4829 (id=17287)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit f76960b144759c278ebcc8b3a834728a6e3e4ca4
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Sat Jan 16 12:23:27 2021 +0530

    Add mapping sha1 with SWHID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

Link to build: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/25/
See console output for more information: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/25/console

Add mapping of sha1 with swh ID

Build has FAILED

Patch application report for D4829 (id=17288)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit 57e1a0658b2a26773b7a2d80f77bd1f497503551
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Sat Jan 16 12:26:34 2021 +0530

    Add mapping sha1 with SWHID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

Link to build: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/26/
See console output for more information: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/26/console

Add mapping of sha1 with swh ID

Build is green

Patch application report for D4829 (id=17289)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit dbc33fc6f3816fd13a870934cbd213e8d7bce4b7
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Sat Jan 16 12:29:34 2021 +0530

    Add mapping sha1 with SWHID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

See https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/27/ for more details.

Thanks

Some things to improve:

  • Phabricator colors the column on the right side of the diff as orange or blue; orange means the line is not covered by tests, so you should add tests that trigger these lines
  • If I replace this line: cur.execute("SELECT sha1_git FROM content where sha1= %s;", (sha1,)) with this: cur.execute("SELECT sha1_git FROM content") this would add a bug, but tests wouldn't catch it.
  • Add type annotations and a docstring
  • Use bytes to represent hashes passed to postgresql. For example sha1 = hash_to_bytes(sha1) instead of sha1 = "\\x{sha1}".format(sha1=sha1). Your way works (which I just learned by reading it), but we already use bytes throughout the codebase, so it's better for consistency. It also prevents small mistakes in the future as it's a different type.
  • Move line breaks to be in a logical separation, instead of wherever the 89th column happened to be (eg. not in the middle of the value of blake2s256_byte)
  • Improve variable names to reflect what they actually contain (eg. sha1_byte is a string and not bytes, sha1_git = cur.fetchall() assignes a tuple of tuple not directly a sha1, etc.)
  • Fix the date in the existing copyright notice, and add one in the tests
  • Remove unrelated changes to requirements.txt (I'm not sure what they are for)
This revision now requires changes to proceed.Jan 16 2021, 10:52 AM

Add mapping of sha1 with swh ID

Build has FAILED

Patch application report for D4829 (id=17291)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit 6d9393940c491beaa5208fc41faf422083d3c1ec
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Mon Jan 18 11:22:59 2021 +0530

    Add mapping of sha1 with SWH ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

Link to build: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/28/
See console output for more information: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/28/console

Add mapping of sha1 with swh ID

Build has FAILED

Patch application report for D4829 (id=17292)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit 3a837fd9d75e3e810dcb861dba77f28e93eae818
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Mon Jan 18 11:29:35 2021 +0530

    Add mapping of sha1 with SWH ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

Link to build: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/29/
See console output for more information: https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/29/console

Add mapping of sha1 with swh ID

Build is green

Patch application report for D4829 (id=17293)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit 55ef20716d4dfcf4073d6d257858982704cbe269
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Mon Jan 18 11:36:10 2021 +0530

    Add mapping of sha1 with SWH ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

See https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/30/ for more details.

Add mapping of sha1 with swh ID

Add mapping of sha1 with swh ID

Build is green

Patch application report for D4829 (id=17330)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit e5448a0a69b3c82b77d067e33f6f050e01ce9510
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Tue Jan 19 16:33:54 2021 +0530

    Add mapping of sha1 with SWH ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

See https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/31/ for more details.

Build is green

Patch application report for D4829 (id=17332)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit a5b104f37b7fc55b84d039a54d5edf4da7319483
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Tue Jan 19 16:34:45 2021 +0530

    Add mapping of sha1 with SWH ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

See https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/32/ for more details.

vlorentz added inline comments.
swh/clearlydefined/tests/test_mapping.py
13–36

I was about to ask you to use hash_to_bytes here as well, but on second thoughts, it's more readable the way you did it, so let's keep it that way :)

61–65

This test doesn't check anything:

  1. if the function returns None like it should, then it does assert True and the test succeeds (as it should)
  1. if the function returns anything else, then the assert True doesn't run, and the test succeeds (but it shouldn't)

Could you update the test to fix that?

68–73

same issue here

This revision now requires changes to proceed.Jan 19 2021, 12:21 PM

Add mapping of sha1 with swh ID

Build is green

Patch application report for D4829 (id=17368)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit 6c2e0a8915c7e3a410e59c4d77725d289c0ff92e
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Wed Jan 20 11:40:00 2021 +0530

    Add mapping of sha1 with SWH ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

See https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/33/ for more details.

Add mapping of sha1 with swh ID

This revision is now accepted and ready to land.Jan 20 2021, 10:04 AM

Build is green

Patch application report for D4829 (id=17369)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit 3de47ec8a425252e07d4096e89488fb3623593ed
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Wed Jan 20 14:33:08 2021 +0530

    Add mapping of sha1 with SWH ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

See https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/34/ for more details.

vlorentz added inline comments.
swh/clearlydefined/tests/test_mapping.py
11–12

one last thing: this connection is not used for reading

This revision now requires changes to proceed.Jan 20 2021, 10:05 AM

Add mapping of sha1 with swh ID

Build is green

Patch application report for D4829 (id=17371)

Rebasing onto 31111a6fe2...

Current branch diff-target is up to date.
Changes applied before test
commit 6d70494b713e7708f2cbe2294e9cc128bc88be3b
Author: Tushar Goel <tushar.goel.dav@gmail.com>
Date:   Wed Jan 20 14:41:56 2021 +0530

    Add mapping of sha1 with SWH ID
    
    Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>

See https://jenkins.softwareheritage.org/job/DMFCD/job/tests-on-diff/35/ for more details.

This revision is now accepted and ready to land.Jan 20 2021, 10:20 AM
This revision was automatically updated to reflect the committed changes.