Differential D6558

cvsclient: handle files which lack a trailing newline
ClosedPublic
Actions

Authored by stsp on Oct 27 2021, 12:24 PM.

Details

Reviewers

vlorentz

Group Reviewers

Reviewers

Maniphest Tasks

T3691: Implement CVS loader

Commits

rDLDCVSd3b3344bc26d: cvsclient: handle files which lack a trailing newline

Summary

CVS uses \n as a protocol message separator, which forces us
to read protocol message line-by-line. File content sent by
the server has a length known and is transmitted in bytes.
The server appends a final "ok\n" message (or perhaps an error
message) when it is done sending file contents.

Properly handle the case where this final message gets buffered
along with file contents and is not delimited from file contents
by \n because the file lacks a trailing newline. Previously, the
final protocol message ended up being written out to file contents
in this case.

Found while testing ingestion of the GNU dino CVS repository from
cvs.savannah.gnu.org/sources/dino.

Related to T3691

Diff Detail

Repository

rDLDCVS CVS Loader

Lint

Automatic diff as part of commit; lint not applicable.

Unit

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

Event Timeline

stsp created this revision.Oct 27 2021, 12:24 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptOct 27 2021, 12:24 PM

Build is green

Patch application report for D6558 (id=23832)

Rebasing onto 7f761b8550...

Current branch diff-target is up to date.

Changes applied before test

commit d3b3344bc26d90acb081fe827aed60257d931cfe
Author: Stefan Sperling <stsp@stsp.name>
Date:   Wed Oct 27 11:55:04 2021 +0200

    cvsclient: handle files which lack a trailing newline
    
    CVS uses \n as a protocol message separator, which forces us
    to read protocol message line-by-line. File content sent by
    the server has a length known and is transmitted in bytes.
    The server appends a final "ok\n" message (or perhaps an error
    message) when it is done sending file contents.
    
    Properly handle the case where this final message gets buffered
    along with file contents and is not delimited from file contents
    by \n because the file lacks a trailing newline. Previously, the
    final protocol message ended up being written out to file contents
    in this case.
    
    Found while testing ingestion of the GNU dino CVS repository from
    cvs.savannah.gnu.org/sources/dino.

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

Harbormaster completed remote builds in B24715: Diff 23832.Oct 27 2021, 12:26 PM

stsp requested review of this revision.Oct 27 2021, 12:26 PM

Could you add a test?

stsp mentioned this in T3691: Implement CVS loader.Oct 27 2021, 12:36 PM

ardumont edited the summary of this revision. (Show Details)Oct 27 2021, 12:48 PM

ardumont added a task: T3691: Implement CVS loader.

In D6558#170324, @vlorentz wrote:

Could you add a test?

Yes, I have a test in progress.

It would be easier to add the test later, because the test will require other outstanding pserver protocol fixes to be merged as well.
Otherwise the pserver protocol handling is too broken and triggers unrelated failures.

I could add the test stacked on top of all required fixes now, but perhaps we can simply do this once all the fixes are merged?

In D6558#170347, @stsp wrote:

perhaps we can simply do this once all the fixes are merged?

Sure!

This revision is now accepted and ready to land.Oct 27 2021, 1:48 PM

Closed by commit rDLDCVSd3b3344bc26d: cvsclient: handle files which lack a trailing newline (authored by stsp). · Explain WhyOct 27 2021, 3:45 PM

This revision was automatically updated to reflect the committed changes.

stsp added a commit: rDLDCVSd3b3344bc26d: cvsclient: handle files which lack a trailing newline.

Revision Contents
Changeset List

Path

Size

swh/

loader/

cvs/

cvsclient.py

30 lines

Diff 23847

View Options

swh/loader/cvs/cvsclient.py

cvsclient: handle files which lack a trailing newlineClosedPublicActions