On Sun, Mar 21, 2010 at 11:01:23 +0000, gh wrote:
> Sun Mar 21 11:56:38 CET 2010 Guillaume Hoffmann <guillaumh@gmail.com>
> * accept issue1427 accept gzipped patch bundles in darcs apply
>
> Sun Mar 21 11:59:08 CET 2010 Guillaume Hoffmann <guillaumh@gmail.com>
> * resolve issue1427 accept gzipped patch bundles in darcs apply
I'll apply these two (thanks!), with a request for minor follow-up work
for the test case and some general commentary for the meat.
accept issue1427 accept gzipped patch bundles in darcs apply
------------------------------------------------------------
> Guillaume Hoffmann <guillaumh@gmail.com>**20100321105638
> Ignore-this: 4607b02530f8de85e9e9165f993b1080
> ] addfile ./tests/issue1427_apply_gz.sh
Please add license headers as in EXAMPLE.sh
> +#!/usr/bin/env bash
> +set -ev
> +
> +rm -rf temp1 temp2
> +mkdir temp1 temp2
> +
> +cd temp2
> +darcs init
> +
> +cd ../temp1
It's easier to copy and paste blocks of testing script if you
write this out longhand, eg
cd temp2
darcs init
cd ..
cd temp1
...
This makes each block more fully self-contained.
> +cd ../temp2
Might as well do the darcs init here
Also, I was thinking it might be worthwhile to make sure some crazy
stuff still works like sending patches where a gzipped file in them...
but I guess that's somewhat pointless due to the way Darcs encodes
binaries.
But basic spirit is to not only test the straightforward stuff, but
maybe try to think of where the corner cases lie. [I say this
hypocritically, of course]
resolve issue1427 accept gzipped patch bundles in darcs apply
-------------------------------------------------------------
> +-- | Read standard input, which may or may not be gzip compressed, directly
> +-- into a 'B.ByteString'.
> +gzReadStdin :: IO B.ByteString
> +gzReadStdin = do header <- B.hGet stdin 2
A stdin counterpart to gzReadFile.
It may make sense to just generalise this to hGzGetContents
> + rest <- B.hGetContents stdin
> + let allStdin = B.concat [header,rest]
> + return $
> + if header /= BC.pack "\31\139"
I wonder if it'd be useful to refactor this with isGzFile somehow? I
imagine that the difficulty is that we have no B.hLookAhead function
that we could use?
> + then allStdin
> + else let decompress = fst . gzDecompress Nothing
> + compressed = BL.fromChunks [allStdin]
> + in
> + B.concat $ decompress compressed
Trent dislikes these one-time-use variables. I think he might instead
suggest something like
B.concat $ fst . gzDecompress Nothing $ BL.fromChunks [allStdin]
I'm not sure how I feel about it either way, myself...
> hunk ./src/Darcs/Commands/Apply.lhs 66
> applyCmd opts [unfixed_patchesfile] = withRepoLock opts $- \repository -> do
> patchesfile <- fixFilePathOrStd opts unfixed_patchesfile
> - ps <- useAbsoluteOrStd (B.readFile . toFilePath) (B.hGetContents stdin) patchesfile
> + ps <- useAbsoluteOrStd (gzReadFilePS . toFilePath) gzReadStdin patchesfile
> let from_whom = getFrom ps
This seems right. Allow both the patch file and the input from stdin to
be gzipped.
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
|
