Reinier Lamers <[email protected]> added the comment: Here's a review. I have two questions below that I'd like to see answered before I apply it. It doesn't look like they are problems with the code though.
> [Accept issue1898: set-default notification system. > Eric Kow <[email protected]>**20100811141903 > > Ignore-this: d33212de428eaf5e2fd85aa4a6cc644a > > ] > [Accept issue1875: darcs does not honor no-set-default on fetch. > Eric Kow <[email protected]>**20100812152637 > > Ignore-this: 32573c47c25ec3e5ad187a5537f50c73 > > ] These look alright. > [Generalise issue1875 test on not setting default. > Eric Kow <[email protected]>**20100812154827 > hunk ./tests/failing-issue1875-honor-no-set-default.sh 32 > > darcs init --repo R # Create our test repos. > darcs get R S --no-set-default > not find S/_darcs/prefs/defaultrepo > > +rm -rf S > + > +darcs init --repo S > +cd S > +darcs push ../R --dry-run > +not grep '/R$' _darcs/prefs/defaultrepo > +darcs push ../R > +cd .. What are you testing with that last "darcs push ../R" there? > [Resolve issue1875: avoid accidentally setting default. > Eric Kow <[email protected]>**20100812154847 > > Ignore-this: d03cfc6111805515ae4f1ca467beab2c > > Two cases fixed: > - setting default on dry-run > - setting default on darcs get --no-set-default > > ] move ./tests/failing-issue1875-honor-no-set-default.sh > ./tests/issue1875-honor-no-set-default.sh > hunk ./src/Darcs/Repository/Prefs.lhs 450 > setDefaultrepo :: String -> [DarcsFlag] -> IO () > > -setDefaultrepo r opts = do doit <- if (NoSetDefault `notElem` opts && > DryRun `notElem` opts && r_is_not_tmp) > - then return True > - else do olddef <- > - getPreflist "defaultrepo" > - return (olddef == []) > +setDefaultrepo r opts = do olddef <- getPreflist "defaultrepo" > + let doit = NoSetDefault `notElem` opts > + && wetRun > + && not rIsTmp > + && (olddef /= [r] || olddef == []) > > when doit > > (setPreflist "defaultrepo" [r]) How does this fix setting the default on "darcs get --no-set-default"? The strange thing is that I can check that it really does, but I can't find the cause in the code. In both versions, doit cannot be true if NoSetDefault is in opts. And besides the definition of doit, nothing appears to have changed. > hunk ./src/Darcs/Repository/Prefs.lhs 460 > where > > - r_is_not_tmp = not $ r `elem` [x | RemoteRepo x <- opts] > + wetRun = DryRun `notElem` opts > + rIsTmp = r `elem` [x | RemoteRepo x <- opts] Looks cleaner :) > [Fix the remote-repo flag if it's not a URL. > Eric Kow <[email protected]>**20100812150920 > > Ignore-this: 10082e2dc200ece25ece1519242962e2 > The word 'fix' here refers to the filepath canonicalisation mechanism > that makes it easier to check filepath equality. > > ] > hunk ./src/Darcs/Arguments.lhs 476 > +fixUrlFlag :: [DarcsFlag] -> DarcsFlag -> IO DarcsFlag > +fixUrlFlag opts (RemoteRepo f) = RemoteRepo `fmap` fixUrl opts f > +fixUrlFlag _ f = return f > + > > fixUrl :: [DarcsFlag] -> String -> IO String > fixUrl opts f = if isFile f > > then toFilePath `fmap` fixFilePath opts f > So we make a function that fixes the path in flags that may contain an URL. The function "fixUrl" that fixes a string only if it is a file already exists. Now the function "isFile" that is uses checks that its argument is a filename by checking that it doesn't contain a ':', which appears a bit low-tech to me. But that's not what this review is about. > import Darcs.ArgumentDefaults ( getDefaultFlags ) > > hunk ./src/Darcs/RunCommand.hs 144 > else do let fixFlag = FixFilePath here cwd > > - (commandCommand cmd) (fixFlag : os) ex > + fixedOs <- mapM (fixUrlFlag [fixFlag]) os > + (commandCommand cmd) (fixFlag : fixedOs) ex So here we fix any remote-repo options with a path in our arguments. It seems that the fixFlag is an extra DarcsFlag that's added to communicate our current directory to fixFlag and fixUrlFlag. > [Resolve issue1898: notify user when they can use set-default. > Eric Kow <[email protected]>**20100812155901 > > Ignore-this: 638b575b32d700cfae9f057293cd5aa8 > > ] move ./tests/failing-issue1898-set-default.sh > ./tests/issue1898-set-default-notification.sh > hunk ./src/Darcs/Repository/Prefs.lhs 451 > > setDefaultrepo :: String -> [DarcsFlag] -> IO () > setDefaultrepo r opts = do olddef <- getPreflist "defaultrepo" > > - let doit = NoSetDefault `notElem` opts > - && wetRun > - && not rIsTmp > - && (olddef /= [r] || olddef == []) > - when doit > - (setPreflist "defaultrepo" [r]) > + let doit = NoSetDefault `notElem` opts && > greenLight > + greenLight = wetRun > + && not rIsTmp > + && (olddef /= [r] || olddef == []) > + if doit > + then setPreflist "defaultrepo" [r] > + else when greenLight $ putStr . unlines $ > + -- the nuance here is that we should > only notify when the > + -- reason we're not setting default is > the --no-set-default > + -- flag, not the various automatic > show stoppers > + [ "Note: if you want to change the > default remote repository to" > + , r ++ "," > + , "quit now and issue the same command > with the --set-default flag." > + ] > > addToPreflist "repos" r > > `catchall` return () -- we don't care if this fails! > > where This looks beautifully self-explanatory. Notify the user only when the default wasn't set because of the --no-set-default flag. __________________________________ Darcs bug tracker <[email protected]> <http://bugs.darcs.net/patch345> __________________________________ _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
