OK, so the context for this patch is that when you do 'darcs changes' you should get all the changes, but that when you do 'darcs changes foo', you should get only changes to foo. The problem is that darcs filters out invalid paths, so 'darcs changes /foo' is treated like 'darcs changes'. Minor UI nuisance.
This patch attempts to address this problem... On Sun, Mar 14, 2010 at 22:59:27 +0000, Alexey Levan wrote: > * Resolve issue 1397: distinguish between "no arguments" and "no valid > arguments" cases in darcs changes. The short patch name should ideally be 72 characters or less http://wiki.darcs.net/Development/GettingStarted Here's my attempt: Resolve issue1397: distinguish no args/no valid args in darcs changes. Yeah, it's not always easy :-) > Arguments to 'darcs changes' stored in Maybe [FilePath] structure, so args to > 'darcs changes' represented as Nothing, and args to 'darcs changes invalid > args' are Just []. Thanks for the high-level explanation. My only real comment for the whole change is that you should have a look at the fixSubPaths function and decide whether it makes more sense to make the change there, or separately for each command or something else. It's a trade-off. On the one hand, doing this in fixSubPaths means that you get it for free in all commands. On the other hand, it's more imperative/side-effect based (that is, if you use fail to abort out of fixSubPaths when all args are bad). That said, in a sense, it already is a little bit imperative (the warnings it prints out, for example). Maybe the whole thing needs to be cleaned up? In any case, note that there are other commands where this sort of thing happens as well, so it'd be a good idea to have a look and see. Anyway, why don't you investigate this and report back on what you think we should do? Also: would it make sense to have a test case for this? It feels a little silly to ask, though. Perhaps there is a certain threshold where it really is not worthwhile to have a test for something... Resolve issue 1397: distinguish between "no arguments" and "no valid arguments" cases in darcs changes. ------------------------------------------------------------------------------------------------------- > - filtered_changes p = maybe_reverse $ getChangesInfo opts filez p > + filtered_changes p = maybe_reverse $ getChangesInfo opts (if null args > then Nothing else Just filez) p As Alexey says above: Nothing - no args, OK (darcs changes) Just [] - bad args only BAD (darcs changes /not/in/repo/x) Just xs - some good args OK (darcs changes /not/in/repo/x foo) > -filterPatchesByNames maxcount [] (hp:ps) = > - (hp, []) -:- filterPatchesByNames (subtract 1 `fmap` maxcount) [] ps > +filterPatchesByNames maxcount Nothing (p:ps) = (p, []) -:- > filterPatchesByNames > + (subtract 1 `fmap` maxcount) Nothing ps OK: now we handle the case where the user explicitly passes in no arguments... (grab all patches). By the way, I personally prefer for formatting-only changes to be made separately so that the meat of patches is more apparent. It's not a big deal and anyway you already had to touch the lines involved, I guess... > -filterPatchesByNames maxcount fs ((Sealed2 hp):ps) > - | Just p <- hopefullyM hp = > +filterPatchesByNames maxcount (Just fs) (sp@(Sealed2 hp):ps) > + | Just p <- hopefullyM hp = Another formatting and style-only change here (although the sp@ trick seems like a good idea here) > +filterPatchesByNames _ (Just []) _ = ([], [], empty) New case specifically to handle case with bad args only > case look_touch fs (invert p) of > - (True, []) -> ([(Sealed2 hp, fs)], fs, empty) > + (True, []) -> ([(sp, fs)], fs, empty) > - (True, fs') -> (Sealed2 hp, fs) -:- filterPatchesByNames > - (subtract 1 `fmap` maxcount) fs' ps > + (True, fs') -> (sp, fs) -:- filterPatchesByNames > + (subtract 1 `fmap` maxcount) (Just fs') ps > - (False, fs') -> filterPatchesByNames maxcount fs' ps > + (False, fs') -> filterPatchesByNames maxcount (Just fs') ps Nothing here seems that surprising. Some whitespace changes, and accounting for having to wrap the arguments in Just > hunk ./src/Darcs/Commands/Changes.lhs 209 > - ([], [], text "Can't find changes prior to:" $$ description hp) > + ([], [], text "Can't find changes prior to:" $$ description hp) Nothing to see here, I think > hunk ./src/Darcs/Commands/Changes.lhs 304 > do > (FlippedSeal hr) <- return . headRL $ slightly_optimize_patchset r > putDocLnWith simplePrinters $ changelog opts' NilRL $ > - getChangesInfo opts' [] (hr :<: NilRL) > + getChangesInfo opts' Nothing (hr :<: NilRL) Straightforward consequence of the change above. -- Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow> PGP Key ID: 08AC04F9
pgpRMMqdmn23g.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
