Two patchs for the command show dependencies, thoughts and a patch for the option minimize-context.

Hi everybody!

Here goes two patchs, one is the implementation of the function
that computes the dependencies and the other is the implementation
of the command "darcs show dependencies" whose output is a 
"digraph" that can be converted to pdf by doing:

$ darcs show dep | dot -Tpdf -o [FILE].pdf

Now, I take advantage of this patches for comment a couple of things
I found in the way of implement the option minimize-context for the
"darcs send". First of all, here goes some preliminary comments of
what things I assume in my implementation of the option:

1. If exist a tag, I search for dependencies in the patchs after
the tag.
2. If not exist dependencies after the tag, the only patch in the
context of the bundle to send is the tag. This is nice for helping the
command "darcs apply" to merge the patches to apply.
3. If not exist a tag, the search is made in all the repository. Here
is a more or less big assumption, that the user will use tags. Luckily,
I suppose that even the search in 700~ patches of 10~ patches to
send should have a decent performance.

So now I can pass to comment my big doubt about the way, i
understood, we want to compute the dependencies.
Here is a script that shows the fail, in fact the script makes the
repositories ready to test, one can see the bad behavior following
the mini-guide of the script file.
Link: http://hub.darcs.net/alegadea/ExamplesRepos
SH: getDepsDoesntAlwaysWork.sh

Step to comment quickly the problem:

Suppose we have two identical repositories r1, r2 with the following
patches,

m2 = m(odifying)2
tag
m1 = m(odifying)1
a1  = a(dding)1

suppose also that r2 make a third modification m3 and want to send
a bundle to applying in r1. Another assumption is that mi con i = 1,2,3
doesnt replace lines between them, and of course all modification are
for the same file adding for a1.

First of all, if we make a bundle for send without the option minimize-
context, then the context of the bundle would be:

m2
tag

But now let's find out what happens if we compute the dependencies
to find out what is the context. For (1) we need to try to commute m2
with m3, which success but of course the end result is a m3' and a m2'
because the lines which modifies every one now are different, but beyond
that for our definition of dependency we can drop m2, so the final context
of the bundle with minimize-context is:

tag

Concluding, that is a problem because now if we make darcs apply of the
"minimized bundle" the merge is made not considering m2 and the final
state of the file isn't right. For example, if we are talking about that m3
add a function in some "empty" place, this could end in m3 adding the
function in the middle of an existing function.

Finishing, I'm not sure of what is a proper solution. I mean, the way I
see it, one way can be change some things in the apply(not my favorite
choice) the other is adjust the way we think the dependencies, I was
thinking in something like "Direct Dependency" and "Indirect Dependency".
The "Direct Dependency" is a dependencie as already know it and the
"Indirect Dependency" is that if we have,

p1,p2 patches, and p1', p2' are the results of commute the patches, then
if p1 /= p1' we conclude that p1 indirectly depends of p2.

I have something almost finished of this final idea, but I'm not so sure
of a couple of things, for example:

- How to correctly compare the patches (p1 /= p1')
- In my unclean implementation I have to carry on MyEq for all over the
code and it's very ugly I think.
- Not remember now :)

Well, congratulations if you read it all :) 
Sorry for the long mail, I hope it has been clear. It would be nice some
comments for know at least if I'm not mistaken.

Cheers!

3 patches for repository http://darcs.net:

patch 8acd3432897864a1047d93bc8b917727482d2384
Author: Ale Gadea <alex.aegf@gmail.com>
Date:   Thu Jul 31 21:33:18 ART 2014
  * Implementing function getDeps.

patch bf2a93892dfee4d4df14808d2c9795c4310042bc
Author: Ale Gadea <alex.aegf@gmail.com>
Date:   Thu Jul 31 21:39:40 ART 2014
  * Implementation of the command "dependencies" for the supercommand "show".

patch 6a4913d393182f5d2551dcaedaff753b8bbbaf7f
Author: Ale Gadea <alex.aegf@gmail.com>
Date:   Fri Aug  1 04:21:35 ART 2014
  * Implementation of the option "minimize-context" for the command "send".

Attachments

• implementing-function-getdeps_.dpatch
• patch-preview.txt
• unnamed

I looked at the first one (getDeps), and I think I understand the
function, I just have a few remarks about variable naming to make the
code more understadable. Also beware of trailing whitespaces.

About the second one (show dependencies), the description of the
command can contain how to use it:

    darcs show dependencies | dot -Tpdf -o [FILE].pdf

I haven't looked at the code in detail but on small repositories it
works well and it's quite fun to use. On a local mirror of darcs.net
I've never seens it finish even on a fast desktop machine..

Still have to look at the 3rd one.

Attachments

• patch1_review.dpatch

Hi!


> I looked at the first one (getDeps), and I think I understand the
> function, I just have a few remarks about variable naming to make the
> code more understadable. Also beware of trailing whitespaces.
>
 Ah... the var names are always a thing :)

>
> About the second one (show dependencies), the description of the
> command can contain how to use it:
>
>     darcs show dependencies | dot -Tpdf -o [FILE].pdf
>
Check.


> I haven't looked at the code in detail but on small repositories it
> works well and it's quite fun to use. On a local mirror of darcs.net
> I've never seens it finish even on a fast desktop machine..
>
Ah well, the thing is that I dont think it's going to finish for the entire
darcs repo(11000~ patches). For example, for 500~ patches:

$ time darcs show dep --from-tag=2.9.5
real 1m53.730s
user 1m51.343s
sys 0m1.939s

I guess that it would be nice if *darcs show dep* for more than, say...
2000 patches prints a warning message informing about the possible
not ending. And maybe with the suggestion of use --from-X or --last
options.

Attachments

• unnamed

Review of the first two patches; function "getDeps" and command "show dependencies".
2 patches for repository http://darcs.net:

patch 4106c0d7aa679783803b5691b53c108dee9d47ef
Author: Ale Gadea <alex.aegf@gmail.com>
Date:   Thu Aug  7 19:32:56 ART 2014
  * Implementing function getDeps.

patch 694682844842e396b087b8e6bdd89ceec584814a
Author: Ale Gadea <alex.aegf@gmail.com>
Date:   Thu Aug  7 20:07:12 ART 2014
  * Implementation of the command "dependencies" for the supercommand "show".

Attachments

• implementing-function-getdeps_.dpatch
• patch-preview.txt
• unnamed

About "Implementing function getDeps", you forgot to change
getTaggedDeps' argument 'ts' into 'patchnames'.

And please remove the trailing whitespaces.

Now for the minimize-context script you sent. Wow, indeed there's a
problem. The automatically minimized bundle MBundle is incorrect. I've
modified your script at

   
http://hub.darcs.net/alegadea/ExamplesRepos/browse/getDepsDoesntAlwaysWork.sh

And added a "manual minimization" step at the end (simply using
obliterate) to see what should be the correct bundle (MBundle2).

Sorry, my version is at:

    http://hub.darcs.net/gh/ExamplesRepos/browse/getDepsDoesntAlwaysWork.sh

2 patches for repository http://darcs.net:

patch 694682844842e396b087b8e6bdd89ceec584814a
Author: Ale Gadea <alex.aegf@gmail.com>
Date:   Thu Aug  7 20:07:12 ART 2014
  * Implementation of the command "dependencies" for the supercommand "show".

patch 9d9622798290e93ed48cf2c26566ebbb2349d87d
Author: Ale Gadea <alex.aegf@gmail.com>
Date:   Fri Aug  8 19:00:30 ART 2014
  * Implementing function getDeps.

Attachments

• implementation-of-the-command-_dependencies_-for-the-supercommand-_show__.dpatch
• patch-preview.txt
• unnamed

Is this ready to apply?

No, the minimize-context patch is obsoleted by
http://bugs.darcs.net/patch1199 .

The show-dependencies feature works in practice, but I remember it
relies on a new function getDeps which forgets to commute the patch it
is given, when outputting its minimal context (this is what caused
problems with Ale's implementation of minimize-context). So this code
needs more scrutiny.

I amended the getDeps patch so that it no longer conflicts against
screened in darcs.cabal.

2 patches for repository http://darcs.net:

patch 9d9622798290e93ed48cf2c26566ebbb2349d87d
Author: Ale Gadea <alex.aegf@gmail.com>
Date:   Fri Aug  8 19:00:30 ART 2014
  * Implementing function getDeps.

patch 6c5fd679ef494e67b81c9422e0cb1c029be4cd99
Author: Ale Gadea <alex.aegf@gmail.com>
Date:   Thu Oct 16 19:02:44 ART 2014
  * Implementation of the command "dependencies" for the supercommand "show".

Attachments

• implementing-function-getdeps_.dpatch
• patch-preview.txt
• unnamed

Marking as review-in-progress since it isn't suitable for applying as-
is.

I like the idea of producing a dot file to display the dependencies.
Could we separate this out, clean up (if necessary), and perhaps add it
as a 2.10 goal?

As regards minimizing contexts, I already stumble over the initial
hypothesis:

"""
1. If exist a tag, I search for dependencies in the patchs after
the tag.
2. If not exist dependencies after the tag, the only patch in the
context of the bundle to send is the tag. This is nice for helping the
command "darcs apply" to merge the patches to apply.
"""

I think this is wrong. Assume this sequence:

repo a: record A
repo b: clone a
repo b: record B
repo a: tag
repo b: pull from a

Now we have in b: A, B, tag. That is, the tag comes last but does not
depend on B. It would be a different matter if we could assume that
darcs automatically re-orders patches so that every tag depends on every
patch before it, but I guess not even 'darcs optimize reorder'
guarantees this for all tags in the repo, just the latest one.

BTW, this is also a problem when using 'darcs convert export', which, I
think, currently acts as if this were the case. It would be very nice if
we had a command to perfom such a "deep" re-ordering.

Obsoleted by http://bugs.darcs.net/patch1397