Control: tags -1 + pending
Hi Paul,
Thanks very much for the thorough review.
> This looks like the only thing that would block the upload:
>
> Some of the copyright holders are missing from debian/copyright.
I'm not sure what you're referring to here - `licensecheck --copyright` and grep both seem to agree with the information in d/copyright with only two differences: (1) I changed the aliases in the copyright headers to the contributors' full names from the vcs and (2) the year range for the copyrights attributed to Plowshare Team vary somewhat between headers where upstream hasn't updated those files in a while. Everyone seems to be attributed though as far as I can tell.
> Whoa that is a lot of shell. shellcheck says it is probably
> buggy/insecure shell too. Personally Python seems a better choice for
> writing this sort of software but I guess upstream wouldn't want to
> rewrite everything...
Yeah, the 4 in plowshare4 refers to a rewrite based on bash 4 that happened not too long ago. They're unlikely to want to rewrite the whole thing for the moment but it may happen in the future, I suppose.
> Upstream might want to switch from their homebrew version stuff to autorevision:
>
> https://packages.debian.org/sid/autorevision
>
> Is there any reason for not using the upstream `make install` target?
> It seems to be very much correct. The only issue might be the git
> version stuff but you can override that with GIT_VERSION until
> upstream switches to autorevision (or if they do not want to).
Thanks, I didn't know about autorevision so I'll have a go with it and talk to upstream. I had problems getting the upstream makefile to pick up the correct version string from the vcs and eventually found it much simpler to use d/rules overrides instead. I'll see if upstream is willing to accept some changes to tidy up the makefile so that I can use it in the package unchanged.
> I would suggest running `wrap-and-sort -sa` so that diffs of the
> source package are easier to read.
I had used wrap-and-sort but without the -sa. I'll change that.
> The README, plowup manual page and Makefile use of /tmp in various
> examples. Using /tmp can cause vulnerabilities on multi-user systems
> or systems where an attacker has a shell and is looking to escalate
> that access. It would be better for the examples to use a path in the
> home directory using ~/ or $HOME depending on the example.
Thanks, I'll add a patch and forward it upstream.
> Upstream has some tests but the build does not run them. Is that
> because they require network to work? Network can't be used on the
> buildds but you could run them using DEP-8:
>
> http://dep.debian.net/deps/dep8/
> http://ci.debian.net/
The tests as far as I can see are intended to check for changes in the APIs of the various file hosts rather than to test if the code has installed properly. It seems like more of a developers' diagnostic than something which should be run automatically so I didn't include it.
> You might want to add some upstream metadata:
>
> https://wiki.debian.org/UpstreamMetadata
Thanks, I'll look into this.
> You might want to add a watch file:
>
> https://wiki.debian.org/debian/watch
> https://qa.debian.org/cgi-bin/fakeupstream.cgi?upstream=vcs/git/google/plowshare
Thanks! I tried for ages to get uscan to interact properly with googlecode but it didn't play nicely since the version information was only in the vcs tags. I didn't know about this fakeupstream.cgi so it should be easy now.
> You might want to add some debtags:
>
> http://debtags.debian.net/edit/plowshare4
Done.
> You might want to add some screenshots of typical usage:
>
> https://screenshots.debian.net/upload/plowshare4
I didn't realise that this was relevant to CLI applications. I'll add a screenshot.
> P: plowshare4: no-upstream-changelog
I wasn't sure what to do about this, any my previous sponsor said to ignore it. Upstream has a wiki page summarising significant changes (https://code.google.com/p/plowshare/wiki/PlowshareChanges) but that's currently sorted by month rather than version and not contained in the source at all. Should I maintain a separate upstream changelog even though they don't include one? Or just try and ask upstream to add it to the source?
> $ cme check dpkg
> ...
> Warning in 'control binary:plowshare4 Depends:0' value 'bash (>=4.1)':
> unnecessary versioned dependency: bash >= 4.1. Debian has squeeze ->
> 4.1-3; squeeze-lts -> 4.1-3+deb6u2; wheezy-security ->
> 4.2+dfsg-0.1+deb7u3; wheezy -> 4.2+dfsg-0.1+deb7u3; jessie -> 4.3-11;
> sid -> 4.3-11;
Done.
> $ codespell --quiet-level=3
> ./src/probe.sh:25: HELPFULL ==> HELPFUL
> ./src/probe.sh:279: HELPFULL ==> HELPFUL
> ./src/download.sh:25: HELPFULL ==> HELPFUL
> ./src/download.sh:758: HELPFULL ==> HELPFUL
> ./src/download.sh:865: sucessive ==> successive
> ./src/list.sh:25: HELPFULL ==> HELPFUL
> ./src/list.sh:182: HELPFULL ==> HELPFUL
> ./src/core.sh:2547: existance ==> existence
> ./src/delete.sh:25: HELPFULL ==> HELPFUL
> ./src/delete.sh:97: HELPFULL ==> HELPFUL
> ./src/upload.sh:25: HELPFULL ==> HELPFUL
> ./src/upload.sh:261: HELPFULL ==> HELPFUL
> ./src/modules/freakshare.sh:136: cant ==> can't
> ./src/modules/ryushare.sh:87: adminstrator ==> administrator
> ./src/modules/ge_tt.sh:194: recieve ==> receive
> ./src/modules/nowdownload_co.sh:215: transfered ==> transferred
> ./src/modules/nowdownload_co.sh:216: transfered ==> transferred
> ./src/modules/hipfile.sh:360: successfull ==> successful
> ./src/modules/sendspace.sh:178: folowing ==> following
> ./src/modules/filemonkey.sh:94: occured ==> occurred
> ./src/modules/bitshare.sh:98: cant ==> can't
Thanks for pointing me to this tool. I'll prepare and forward a patch.
> $ perlcritic -1 .
> <lots of stuff>
I'm not sure that this check is relevant since all of the output is about perl and this project is pure bash shell script.
> $ find -type f -iname '*.sh' -exec shellcheck {} +
I'll check through this output, but there is the fact that these shell scripts depend on bash behaviour rather than POSIX shell so perhaps much of this is not relevant or overly defensive.
> $ find -type f | xargs isutf8
> ./src/modules/dl_free_fr.sh: line 71, char 1, byte offset 29: invalid UTF-8 code
I'll check with upstream about this.
> $ egrep -ri 'fixme|todo' .
> <lots of stuff>
Is this necessarily an issue? The project is being actively developed and these are just notes that will be addressed in the future but are not critical for the time being.
I'll address all of these points properly in the package in the coming days when I get a chance. Thanks again for your review.
Cheers,
Carl
Attachment:
signature.asc
Description: OpenPGP digital signature