Add a default target for symbolic reference branches
ClosedPublic
Actions

Authored by olasd on May 28 2020, 11:02 AM.

Details

Reviewers

vlorentz
ardumont

Group Reviewers

Reviewers

Commits

rDLDGee22325a193d: Add a default target for symbolic reference branches

Summary

Some repositories are wrongly configured and they have symbolic references (e.g.
a HEAD) that points to a non-existent branch. In the general case, we still want
to load these origins; Create a dangling branch for these symbolic references to
point to.

Test Plan

new tox test added

Diff Detail

Repository

rDLDG Git loader

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

olasd created this revision.May 28 2020, 11:02 AM

Herald added a reviewer: Reviewers. · View Herald TranscriptMay 28 2020, 11:02 AM

Build is green

Patch application report for D3188 (id=11330)

Rebasing onto dfd5be7b14...

Current branch diff-target is up to date.

Changes applied before test

commit 0c31a06b94f6f7cccd1db85317afce2032229461
Author: Nicolas Dandrimont <nicolas@dandrimont.eu>
Date:   Thu May 28 10:31:14 2020 +0200

    Add a default target for symbolic reference branches
    
    Some repositories are wrongly configured and they have symbolic references (e.g.
    a HEAD) that points to a non-existent branch. In the general case, we still want
    to load these origins; Create a dangling branch for these symbolic references to
    point to.

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

Harbormaster completed remote builds in B12523: Diff 11330.May 28 2020, 11:03 AM

olasd added a child revision: D3189: Make branch filtering a common behavior between the disk and remote loaders.May 28 2020, 11:12 AM

LGTM, but I think it's missing some tests:

Logging on dangling branch, and absence of logging when there isn't any. (Helpful pytest fixture: https://docs.pytest.org/en/latest/logging.html#caplog-fixture )
Checking there is no warning if we first get a symref from A to B, then a reference from B to C.

This revision now requires changes to proceed.May 28 2020, 11:27 AM

In D3188#77722, @vlorentz wrote:

LGTM, but I think it's missing some tests:

Logging on dangling branch, and absence of logging when there isn't any. (Helpful pytest fixture: https://docs.pytest.org/en/latest/logging.html#caplog-fixture )

Yeah, I know of the caplog fixture, thanks. I will not be adding pytest fixtures in the mess of unittest class inheritance that these tests currently are.

Checking there is no warning if we first get a symref from A to B, then a reference from B to C.

As we don't really control what comes "first" (dulwich is reading the symbolic refs, not us) I don't think this (very) edge case can really be tested for reliably. I don't even know if chains of symbolic references exist in practice.

ardumont added a subscriber: ardumont.May 28 2020, 11:39 AM

ardumont added inline comments.

swh/loader/git/utils.py
1	I am "the pre-commit hook" for copyright headers (I missed those yesterday... ;) Can you please update those (we do for other repos ;)?

Update copyright years

Build is green

Patch application report for D3188 (id=11335)

Rebasing onto dfd5be7b14...

Current branch diff-target is up to date.

Changes applied before test

commit ee22325a193dbc2b52b37496f3374c6bf0c1d1e1
Author: Nicolas Dandrimont <nicolas@dandrimont.eu>
Date:   Thu May 28 10:31:14 2020 +0200

    Add a default target for symbolic reference branches
    
    Some repositories are wrongly configured and they have symbolic references (e.g.
    a HEAD) that points to a non-existent branch. In the general case, we still want
    to load these origins; Create a dangling branch for these symbolic references to
    point to.

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

Harbormaster completed remote builds in B12528: Diff 11335.May 28 2020, 12:02 PM

ardumont added inline comments.May 28 2020, 12:43 PM