Hi all,

yesterday I did a little reorg of one project, moving stuff around. In
particular, I had a sub/subfolder that I moved to the top level, and a
few other files moved into it as well.

Trying to distill a simple recipe to trigger the behaviour, I was even
more surprised to come up with two *very* similar scripts that produce
different output...

This first script:

    darcs --version
    mkdir p
    cd p
    darcs init
    mkdir -p folder/subfolder
    touch folder/subfolder/file_a
    touch folder/subfolder/file_b
    mkdir other
    touch other/file_c
    darcs add --recursive folder other
    darcs record -a -m "1st"
    mkdir another
    touch another/file_z
    darcs add --recursive another
    darcs record -a -m "2nd"
    darcs mv folder/subfolder promoted
    darcs mv other/file_c promoted/new_name
    darcs mv another/file_z promoted
    darcs whatsnew -s
    darcs record -a -m "3rd"
    darcs changes -s --last=1
    cd ..
    rm -rf p

outputs this:

    $ sh -x pp.sh
    + darcs --version
    2.3.0 (release)
    + mkdir p
    + cd p
    + darcs init
    + mkdir -p folder/subfolder
    + touch folder/subfolder/file_a
    + touch folder/subfolder/file_b
    + mkdir other
    + touch other/file_c
    + darcs add --recursive folder other
    + darcs record -a -m 1st
    Finished recording patch '1st'
    + mkdir another
    + touch another/file_z
    + darcs add --recursive another
    + darcs record -a -m 2nd
    Finished recording patch '2nd'
    + darcs mv folder/subfolder promoted
    + darcs mv other/file_c promoted/new_name
    + darcs mv another/file_z promoted
    + darcs whatsnew -s
     ./another/file_z -> ./folder/subfolder/file_z
     ./folder/subfolder -> ./promoted
     ./other/file_c -> ./promoted/new_name
    + darcs record -a -m 3rd
    Finished recording patch '3rd'
    + darcs changes -s --last=1
    Thu Aug 13 11:16:31 CEST 2009  lele@nautilus.homeip.net
      * 3rd

         ./another/file_z -> ./folder/subfolder/file_z
         ./folder/subfolder -> ./promoted
         ./other/file_c -> ./promoted/new_name
    + cd ..
    + rm -rf p

And you can see the two different ways darcs handled the move of
"another/file_z" and "other/file_c" into the renamed folder.

This other script, that AFAICS differs from the first *only* on the
path names (what was "another" in the first is "third" here...):

    darcs --version
    mkdir p
    cd p
    darcs init
    mkdir -p folder/subfolder
    touch folder/subfolder/file_a
    touch folder/subfolder/file_b
    mkdir other
    touch other/file_c
    darcs add --recursive folder other
    darcs record -a -m "1st"
    mkdir third
    touch third/file_z
    darcs add --recursive third
    darcs record -a -m "2nd"
    darcs mv folder/subfolder promoted
    darcs mv other/file_c promoted/new_name
    darcs mv third/file_z promoted
    darcs whatsnew -s
    darcs record -a -m "3rd"
    darcs changes -s --last=1
    cd ..
    rm -rf p

does not produce the same:

    $ sh -x p.sh
    + darcs --version
    2.3.0 (release)
    + mkdir p
    + cd p
    + darcs init
    + mkdir -p folder/subfolder
    + touch folder/subfolder/file_a
    + touch folder/subfolder/file_b
    + mkdir other
    + touch other/file_c
    + darcs add --recursive folder other
    + darcs record -a -m 1st
    Finished recording patch '1st'
    + mkdir third
    + touch third/file_z
    + darcs add --recursive third
    + darcs record -a -m 2nd
    Finished recording patch '2nd'
    + darcs mv folder/subfolder promoted
    + darcs mv other/file_c promoted/new_name
    + darcs mv third/file_z promoted
    + darcs whatsnew -s
     ./folder/subfolder -> ./promoted
     ./other/file_c -> ./promoted/new_name
     ./third/file_z -> ./promoted/file_z
    + darcs record -a -m 3rd
    Finished recording patch '3rd'
    + darcs changes -s --last=1
    Thu Aug 13 11:17:16 CEST 2009  lele@nautilus.homeip.net
      * 3rd

         ./folder/subfolder -> ./promoted
         ./other/file_c -> ./promoted/new_name
         ./third/file_z -> ./promoted/file_z
    + cd ..
    + rm -rf p

and actually this is what I was expecting to see...

Just to be picky:

    $ diff -u pp.sh p.sh
    --- pp.sh       2009-08-13 11:15:47.163840847 +0200
    +++ p.sh        2009-08-13 11:12:07.872139599 +0200
    @@ -9,13 +9,13 @@
     touch other/file_c
     darcs add --recursive folder other
     darcs record -a -m "1st"
    -mkdir another
    -touch another/file_z
    -darcs add --recursive another
    +mkdir third
    +touch third/file_z
    +darcs add --recursive third
     darcs record -a -m "2nd"
     darcs mv folder/subfolder promoted
     darcs mv other/file_c promoted/new_name
    -darcs mv another/file_z promoted
    +darcs mv third/file_z promoted
     darcs whatsnew -s
     darcs record -a -m "3rd"
     darcs changes -s --last=1

This is not a problem at all of course, after all everything is going
ok, but I tag this as "problematic", in the sense that tools like
tailor and trac-darcs have to do tricky dance to understand what's
going on.

thank you,
ciao, lele.
-- 
nickname: Lele Gaifax    | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas    | comincerò ad aver paura di chi mi copia.
lele@nautilus.homeip.net |                 -- Fortunato Depero, 1929.

We know (but I cannot find the issue on the tracker...) that darcs
emits inconditionally moves hunks first, then the other kind of
hunks. Could it be that it's also using some kind of sorting on those
moves? That's because I notice that the difference between the two
scripts is in "another" being sorted before the other in one case,
"third" instead comes last...

This seems to confirm that:

    $ mkdir p
    $ cd p
    $ darcs init
    $ touch a b c d e
    $ darcs add ?
    $ darcs rec -a -m "1st"
    Finished recording patch '1st'
    $ darcs mv a d_a
    $ darcs mv e b_e
    $ darcs wha -s
     ./a -> ./d_a
     ./e -> ./b_e
    $ darcs mv c b_c
    $ darcs wha -s
     ./a -> ./d_a
     ./c -> ./b_c
     ./e -> ./b_e

that is, the moves aren't in the same order as the effective
operations.

So again, I think the real bug here is that kind of reordering that
darcs performs on moves: is there any evidence of that being a needed
step in the theory of things?

ciao, lele.
-- 
nickname: Lele Gaifax    | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas    | comincerò ad aver paura di chi mi copia.
lele@nautilus.homeip.net |                 -- Fortunato Depero, 1929.

Hi Lele, I think you may be thinking of issue183, and looking at your scripts, I
also think I agree that darcs appears to be sorting the patches.

Note that this is "harmless" as far as Darcs is concerned, but it does lead to
surprising output.

We need somebody to look at the source code and figure out why this is
happening.  I think the answer is in Darcs.Patch.Prim and order comes from
comparePrim.  There's also a function there called 'sort_coalesce'.

I can see why coalescing patches is useful, but why do we feel compelled to sort
them?

Can we forgo the sorting?

Whoops, did not mean to assign you there, Lele (although if you want to have a
look, great!)

Hello again,

investigating on an trac-darcs issue caused by this, I spotted the
patch that trac-darcs doesn't digest very well, from one of zooko's
public repositories:

    $ darcs changes -s -p "mv base32 into subdirectory named base32"
    Sat Mar 12 22:30:13 CET 2005  zooko@zooko.com
      * mv base32 into subdirectory named base32

         ./base32 -> ./tempdirname
         ./DESIGN -> ./base32/DESIGN
         ./GNUmakefile -> ./base32/GNUmakefile
         ./README -> ./base32/README
         ./TODO -> ./base32/TODO
         ./base32.c -> ./base32/base32.c
         ./base32.h -> ./base32/base32.h
         ./base32id.py -> ./base32/base32id.py
         ./setup.py -> ./base32/setup.py
         ./tempdirname -> ./base32/base32
         ./test.c -> ./base32/test.c
        A ./base32/

As you may notice, it's a rather convoluted patch, but it shows
the devil's gift of the reordering being done: the patch is ambiguos
to say the least (is "DESIGN" being added to the new "base32" subdir
added at the end? or is instead going into the /old/ "base32" before
it's renamed to "tempdirname"?) because of course the very first two
hunks cannot be in that sequence.

Having discovered "annotate -p" recently, I tried with that:

    $ darcs annotate -p "mv base32 into subdirectory named base32"
    [mv base32 into subdirectory named base32
    zooko@zooko.com**20050312213013] {
    move ./base32 ./tempdirname
    adddir ./base32
    move ./DESIGN ./base32/DESIGN
    move ./GNUmakefile ./base32/GNUmakefile
    move ./README ./base32/README
    move ./TODO ./base32/TODO
    move ./base32.c ./base32/base32.c
    move ./base32.h ./base32/base32.h
    move ./base32id.py ./base32/base32id.py
    move ./setup.py ./base32/setup.py
    move ./tempdirname ./base32/base32
    move ./test.c ./base32/test.c
    }

Bingo! That shows the hunks in the right order!!

This is great news, and I could use this way of "slurping" darcs patches
metadata, in trac-darcs and tailor as well so to immensely simplify
the logic there.

The draw back may be speed: using "annotate -p" I'd need to execute a
darcs subprocess for any single patch, whereas it currently spawns a
single "darcs changes --xml [--from-match]"...

After all, the real problem is still the reordering being done by the
"changes" command: as I may see the point of having a sorted view
(given that we have the more important [from the scripting point of
view] "annotate -p"), I think that [at least] in the --xml case it
should *not* perform the reordering of hunks:

    $ darcs changes --xml -s -p "mv base32 into subdirectory named base32"
    <changelog>
    <patch author='zooko@zooko.com' date='20050312213013' local_date='Sat Mar 12 22:30:13 CET 2005' inverted='False' hash='20050312213013-92b7f-881ac256b499fae70330a09b958a912d6c8d8db9.gz'>
            <name>mv base32 into subdirectory named base32</name>
        <summary>
        <move from="base32" to="tempdirname"/>
        <move from="DESIGN" to="base32/DESIGN"/>
        <move from="GNUmakefile" to="base32/GNUmakefile"/>
        <move from="README" to="base32/README"/>
        <move from="TODO" to="base32/TODO"/>
        <move from="base32.c" to="base32/base32.c"/>
        <move from="base32.h" to="base32/base32.h"/>
        <move from="base32id.py" to="base32/base32id.py"/>
        <move from="setup.py" to="base32/setup.py"/>
        <move from="tempdirname" to="base32/base32"/>
        <move from="test.c" to="base32/test.c"/>
        <add_directory>
        base32
        </add_directory>
        </summary>
    </patch>
    </changelog>

Hope this helps,
ciao, lele.
-- 
nickname: Lele Gaifax    | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas    | comincerò ad aver paura di chi mi copia.
lele@nautilus.homeip.net |                 -- Fortunato Depero, 1929.

Hi Lele,

That's interesting.  Could you tell me if the non-summarised changes output and
also the patch contents are in the right order?

This makes me think that the problem is much more superficial than I first imagined.

In fact, I notice this bit of code in Darcs.Patch.Viewing:
  themods = map summ $ combine $ sort $ concatMap s2 p

Perhaps you'd like to try and see what happens without the sort?

It'd be good if we worked out why we thought the sort was a good idea though...

Thanks!

Actually, please disregard my comment about the code.  I notice that...

gen_summary :: Bool -> [IsConflictedPrim] -> Doc
gen_summary use_xml p
    = vcat themoves
   $$ vcat themods

Already we're printing a moves section (issue183) before a mods section,
although at this level, I can't see why your moves are getting sorted.

Looking deeper into that function, I get the impression that the separation was
done just because it was easier to write the code that way and that we could
just rewrite gen_summary which looks likes it needs a good cleaning anyway.

So Lele, if you could just confirm about the patch contents, that'd be good

On Mon, 17 Aug 2009 13:16:07 +0000
Eric Kow <bugs@darcs.net> wrote:

> That's interesting.  Could you tell me if the non-summarised changes
> output and also the patch contents are in the right order?

What do you mean with that? The non-summarised output... does not
output the hunks. Wrt to "patch contents", I bet that "annotate -p"
just echos it on stdout...

I spent some time on the Viewing.lhs as you suggested, but it does not
seems doable, since the combine steps take advantage of the fact the
list of hunks is sorted...

And even "annotate -p" does not solve *all* problems: I'd need the
more selective --match option, since I cannot use only the patch name,
but that needs a better haskeller than me, sigh :-\

ciao, lele.
-- 
nickname: Lele Gaifax    | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas    | comincerò ad aver paura di chi mi copia.
lele@nautilus.homeip.net |                 -- Fortunato Depero, 1929.

Hi Lele, Could you help me to find this Zooko repository and the patch
you mentioned?

Looking at your test case in msg8125, I notice that one issue is that
moves are sorted in alphabetical order in darcs whatsnew (which makes me
thing that there is some sort of sorting of the pending patch going on).

On the other hand, the stuff involving Zooko's patch involves a patch
which is not actually in alphabetical order, but seems to be displayed
inconsistently (sometimes alphabetical, in darcs changes --summary,
sometimes in actual order in darcs changes --xml).  It sounds like we
need a test case for this happening.

Some things that confuse me:
- I think we need to split this bug into two (one for the darcs
whatsnew/pending issue), and I'll probably do so once I get some feedback)
- How do I create a patch that's not sorted?  Maybe amend-record?
- Is this actually a problem, now that issue183 is solved (and move
patches are no longer moved to the front)?

Mentioned Zooko's repository is

    http://allmydata.org/source/zbase32/trunk-hashedformat

BTW, with a newer darcs this is what I get on that:

    $ darcs --version
    2.4 (release)
    $ darcs changes --xml -s -p "mv base32 into subdirectory named base32"
    <changelog>
    <patch author='zooko@zooko.com' date='20050312213013' local_date='Sat Mar 12 22:30:13 CET 2005' inverted='False' hash='20050312213013-92b7f-881ac256b499fae70330a09b958a912d6c8d8db9.gz'>
            <name>mv base32 into subdirectory named base32</name>
        <summary>
        <move from="base32" to="tempdirname"/>
        <add_directory>
        base32
        </add_directory>
        <move from="DESIGN" to="base32/DESIGN"/>
        <move from="GNUmakefile" to="base32/GNUmakefile"/>
        <move from="README" to="base32/README"/>
        <move from="TODO" to="base32/TODO"/>
        <move from="base32.c" to="base32/base32.c"/>
        <move from="base32.h" to="base32/base32.h"/>
        <move from="base32id.py" to="base32/base32id.py"/>
        <move from="setup.py" to="base32/setup.py"/>
        <move from="tempdirname" to="base32/base32"/>
        <move from="test.c" to="base32/test.c"/>
        </summary>
    </patch>
    </changelog>

which seems correct.
-- 
nickname: Lele Gaifax    | Quando vivrò di quello che ho pensato ieri
real: Emanuele Gaifas    | comincerò ad aver paura di chi mi copia.
lele@nautilus.homeip.net |                 -- Fortunato Depero, 1929.

On Tue, Mar 30, 2010 at 13:00:04 +0000, Lele Gaifax wrote:
>     http://allmydata.org/source/zbase32/trunk-hashedformat

Thanks!

> BTW, with a newer darcs this is what I get on that:
> 
>     $ darcs --version
>     2.4 (release)
>     $ darcs changes --xml -s -p "mv base32 into subdirectory named base32"
>     <changelog>
>     <patch author='zooko@zooko.com' date='20050312213013' local_date='Sat Mar 12 22:30:13 CET 2005' inverted='False' hash='20050312213013-92b7f-881ac256b499fae70330a09b958a912d6c8d8db9.gz'>
>             <name>mv base32 into subdirectory named base32</name>
>         <summary>
>         <move from="base32" to="tempdirname"/>
>         <add_directory>
>         base32
>         </add_directory>
>         <move from="DESIGN" to="base32/DESIGN"/>
>         <move from="GNUmakefile" to="base32/GNUmakefile"/>
>         <move from="README" to="base32/README"/>
>         <move from="TODO" to="base32/TODO"/>
>         <move from="base32.c" to="base32/base32.c"/>
>         <move from="base32.h" to="base32/base32.h"/>
>         <move from="base32id.py" to="base32/base32id.py"/>
>         <move from="setup.py" to="base32/setup.py"/>
>         <move from="tempdirname" to="base32/base32"/>
>         <move from="test.c" to="base32/test.c"/>
>         </summary>
>     </patch>
>     </changelog>
> 
> which seems correct.

Well, I think that's with issue183 (moves being put before other
operations) fixed.  But here the concern is about the moves being
put into alphabetical order, which still appears to be case.

Are you saying that this ordering of moves isn't actually a problem
in practice?  If so, I'd be quite happy to wont-fix this bug :-)

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

On Tue, 30 Mar 2010 13:10:27 +0000
Eric Kow <bugs@darcs.net> wrote:

> Well, I think that's with issue183 (moves being put before other
> operations) fixed.  But here the concern is about the moves being
> put into alphabetical order, which still appears to be case.

Yes, it was just to point out that the specific problem that
originated all this, that tricky patch in Zooko's repository, is now
handled correctly, as far as trac-darcs is concerned :-)

> Are you saying that this ordering of moves isn't actually a problem
> in practice?  If so, I'd be quite happy to wont-fix this bug :-)

No, I still agree that ideally darcs should never try to out-smart its
user, arbitrarily reordering things. At the very least, not when asked
for the XML version.

On Sat, Apr 03, 2010 at 13:07:34 +0000, Lele Gaifax wrote:
> > Are you saying that this ordering of moves isn't actually a problem
> > in practice?  If so, I'd be quite happy to wont-fix this bug :-)
> 
> No, I still agree that ideally darcs should never try to out-smart its
> user, arbitrarily reordering things. At the very least, not when asked
> for the XML version.

Thanks!  I doubt Darcs is trying to be clever here so much as keeping
the implementation simple.  (I think our attempts at sorting the pending
patch may be related to coalescing).

May I request that you submit your msg8125 test as a formal test case so
we can study it more?

I'll warn you that we may not actually accept it if we determine that
the sorting is actually necessary (to keep us sane), but I think it may
be useful to have it as concretely and automated as possible.

See http://wiki.darcs.net/Development/RegressionTests

Note that there are really two things to test for

- If I darcs mv twice, what does darcs whatsnew say?

- If I manage to record a patch in the expected order (perhaps
  try this by hand by manually rewriting the pending patch and
  see what happens), what does darcs changes say?

Thanks,

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

Hmm, I don't know why the me of 2010 thought this was about the pending 
patch.  The issue here is that file move prims in named patches are 
displayed in the alphabetical order rather than the order performed, 
which can be confusing from a UI point of view.

I don't think there's much we can do about this one.  It may have to be 
a wont-fix.

See msg8125 for a potential test case