Mon Mar  8 06:41:07 EET 2010  exlevan@gmail.com
  * Resolve issue 1397: distinguish between "no arguments" and "no valid arguments" cases in darcs changes.

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 [].

Attachments

• resolve-issue-1397_-distinguish-between-_no-arguments_-and-_no-valid-arguments_-cases-in-darcs-changes_.dpatch
• unnamed

http://bugs.darcs.net/patch179 and http://bugs.darcs.net/patch172 are
duplicates of this patch

This is a duplicate of patch178

Err, this *is* patch178, whoops!

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

Alexey Levan <bugs@darcs.net> writes:

> New submission from Alexey Levan <exlevan@gmail.com>:
>
> Mon Mar  8 06:41:07 EET 2010  exlevan@gmail.com
>   * Resolve issue 1397: distinguish between "no arguments" and "no valid arguments" cases in darcs changes.

FYI, if you use "darcs send --subject '[patch123]'" you can send
amendments to an earlier submission (where 123 is the ID of earlier
submission).  This saves Eric having to link them by hand.

Thanks for the review! So, I looked what's going on with fixSubPaths...

Actually, there's no problem with the fixSubPaths function - it just
filters out incorrect absolute paths, and it does it just fine. However,
the real problem is how this function is (mis)used.

There's no consistent policy of using fixSubPaths, and if you use darcs
command with invalid absolute paths, the following may happen:
  * everything is OK, because invalid paths are handled explicitly
(Record.lhs, Rollback.lhs)
  * everything is OK, because command with no args isn't special case
(Diff.lhs)
  * you get message like "Nothing specified, nothing to do". Message is
misleading, but nothing bad happens (Remove.lhs, Add.lhs)
  * the worst: you get error (Move.lhs, try 'darcs move /foo /bar /baz'!).

So, how do we fix all this mess? I propose following:
  1. In Darcs/Arguments.lhs, create function 'maybeFixSubPaths ::
[DarcsFlag] -> [FilePath] -> IO [Maybe SubPaths]', which maps invalid
paths to Nothing.
  2. fixSubPaths gets implemented in terms of maybeFixSubPaths:
fixSubPaths flags fs = catMaybes <$> maybeFixSubPaths flags fs
  3. In each command that takes filenames as arguments, implement
commands with different semantics (such as 'darcs changes' and 'darcs
changes [args]') as different functions, and pass them already checked
arguments, doing all error handling before that. Here's example draft
for darcs move:

moveCmd opts args = case () of
  _ | length args < 2 -> fail "The `darcs move' command requires at
least two arguments."
    | length args == 2 -> do
        case maybeFixSubPaths args of
          [Just from, Just to] -> if {- to is dir -}
            then moveFileToDir opts from to
            else moveFile opts from to
          _ -> fail {- both args must be valid -}
    | length args > 2 -> let (froms, to) = (init args, last args) in
        case maybeFixSubPath to of
          Nothing -> fail "invalid destination directory"
          Just to' -> case fixSubPaths froms of
            [] -> fail "Nothing to move"
            froms' -> moveFilesToDir opts froms' to'
-- functions doing all the work
moveFile :: [DarcsFlag] -> SubPath -> SubPath -> IO ()
moveFileToDir :: [DarcsFlag] -> SubPath -> SubPath -> IO ()
moveFilesToDir :: [DarcsFlag] -> [SubPath] -> SubPath -> IO ()

That way all error handling and args mangling would became separate from
the code that actually does some work. That's plenty of work, but I
think only that way will resolve all those 'invalid absolute path' bugs.
As a bonus, code in Commands/ will become much more clear and maintainable.

So, if nobody's mind, I'll start working on it. Or maybe there are
better ideas around?

On Mon, Mar 15, 2010 at 19:35:33 +0000, Alexey Levan wrote:
> Actually, there's no problem with the fixSubPaths function - it just
> filters out incorrect absolute paths, and it does it just fine. However,
> the real problem is how this function is (mis)used.

Thanks!  I really appreciate it when Darcs hackers take the extra time
out to look at the wider context of their work, which seems to be what
you have done (I hadn't even considered the users of fixSubPaths; was
just thinking of sticking a fail in there)

> There's no consistent policy of using fixSubPaths, and if you use darcs
> command with invalid absolute paths, the following may happen:
>   * everything is OK, because invalid paths are handled explicitly
> (Record.lhs, Rollback.lhs)
>   * everything is OK, because command with no args isn't special case
> (Diff.lhs)
>   * you get message like "Nothing specified, nothing to do". Message is
> misleading, but nothing bad happens (Remove.lhs, Add.lhs)
>   * the worst: you get error (Move.lhs, try 'darcs move /foo /bar /baz'!).

I have not really checked the specifics of the code, but at a high
level, that sounds like the right sort of thing to do: harmonise,
modularise

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

Hi,

Eric Kow <kowey@darcs.net> writes:
>> There's no consistent policy of using fixSubPaths, and if you use darcs
>> command with invalid absolute paths, the following may happen:
>>   * everything is OK, because invalid paths are handled explicitly
>> (Record.lhs, Rollback.lhs)
>>   * everything is OK, because command with no args isn't special case
>> (Diff.lhs)
>>   * you get message like "Nothing specified, nothing to do". Message is
>> misleading, but nothing bad happens (Remove.lhs, Add.lhs)
>>   * the worst: you get error (Move.lhs, try 'darcs move /foo /bar /baz'!).
>
> I have not really checked the specifics of the code, but at a high
> level, that sounds like the right sort of thing to do: harmonise,
> modularise

there's also a somewhat related patch queued in the noslurps work, that
unifies the "WARNING: foo does not exist" messages (in patch156). You
may want check that out (and maybe work on top to avoid conflicting with
it).

Yours,
   Petr.

Tue Mar 16 02:38:17 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Add Maybe variant of fixSubPaths

Wed Mar 17 00:02:11 EET 2010  Alexey Levan <exlevan@gmail.com>
  * Separate argument handling from repository work in Move.lhs

This is first part of commands' clean-up series I described above.

maybeFixSubPaths maps invalid absolute paths to Nothing, and resolving valid
paths to Just Subpath, thus preserving original list's structure. So after
call to maybeFixSubPaths, we can see what exactly arguments were invalid,
handle errors better and provide users with clearer error messages.

The code in Move.lhs was first changed to use maybeFixSubPaths. Now moveCmd
checks all errors it can without connecting to repository. Actual
functionality has become splitted between moveFile and moveFilesToDir.

Attachments

• add-maybe-variant-of-fixsubpaths.dpatch
• unnamed

Oops, just noticed mail from Trent with suggestion to write regression
tests. Working on them right now.

On Tue, Mar 16, 2010 at 23:10:09 +0000, Alexey Levan wrote:
> Oops, just noticed mail from Trent with suggestion to write regression
> tests. Working on them right now.

You might also want to have a look at the work Petr pointed out,
just in case :-)

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-bb6818bc0d9894c37762b621736e45fc58c1dbaa

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-bb6818bc0d9894c37762b621736e45fc58c1dbaa