Re: [Gnu-arch-users] PyArch patches

[David Allouche]

On Fri, Jun 18, 2004 at 07:41:57PM -0400, Aaron Bentley wrote:
> Overall comment: thanks for cleaning up my patches.
> 
> I've made a new branch, based on 
> address@hidden/pyarch--abentley--0.5.1--patch-17.  It's here:
> address@hidden/pyarch--devo--0.1

Okay, then I'll be merging those into my main branch soon.

> I appreciate you changing the default iteration method of 
> exec_safe_iter_stdout.  The ChildProcess stuff looks right to me.

The former really was a bugfix, exec_safe_iter_stdout was meant to
provide real-time incremental output.

The ChildProcess stuff was mostly an overdue refactoring of the
tla-spawning primitives. The next step there is supporting a poll-based
interface for all cases where threads can be avoided, that is any case
not involving complex pipes, read anything that _tla.py depends on.

> I'll see about teaching vim to use only spaces for indenting.

Thanks.

> iter_ancestry and friends
> I'm not sure what you dislike.  If it's the implementation, I can look 
> into fixing it.  If it's the concept, I'll stick it back in pyaba.

Two things:

The concept is not clear to me. I understand that patchlog-based
ancestry can be useful, especially for merging operators, where it is
much faster than archive-based ancestry for remote archives. However
that also involves a number of tricky corner cases with missing
patchlogs which are not documented and do not seem supported.

The implementation also looks dirty to me. The usefulness of
_iter_ancestry as a separate function seems questionable, and actually I
do not understand how that code can actually do what it claims to do...
since you are actually using it, I assume it is probably correct, but
just obscure. I have not yet examined your new patches.

If the design issues are addressed, or at least documented, I'll be
happy to merge these features. But probably within the ArchSourceTree
class, and without exposing iter_vchange as a separate building-block
function, but rather using options or providing a set of clearly defined
all-in-one methods.

This code has also been subjected to the usual heavy handed style cleanup
and docstring augmentation. I stored the changes in

    address@hidden/pyarch--log-ancestry--0

for my further reference and your merging pleasure.


> Unsolved problems:
> tla hooks can potentially hose the FileChange output.  This is a problem 
> inherited from tla.  Ideally, tla would produce a message so users could 
> distinguish hook output from tla output, and then we could use that too.

I have no strong opinion on that problem.

Hooks are a relatively obscure feature of tla (read: I do not use them),
and users who use hooks can probably be expected to diagnose problems
caused by those hooks. We can either:

  -- require the hooks to mangle their output to avoid making the output
     confusing to context-free output parsers

  -- make the classifier stateful and guard hook output

My preference goes toward requiring hook output to be nice to
context-free parsers. In any case that's an issue of documentation and
implementation in tla, and I let you argue the point with Tom, you are
so much better at it than I am :-)

> It would be nice if delta and friends produced FileModified items, but 
> it's also important to be able to get at the process return status. 
> I've got alternative implementations of "delta", "show-changeset", 
> "apply-changeset", "archive-mirror", and "commit" but they return 
> exec_safe_status_stdout iterators, and I classify the output in the 
> calling function.

I have not examined those yet, but what you say sounds like what I think
is the right way to do it. Iterating over incremental output is actually
event-driven programming. If some extra data must be given back to the
caller, like the Changeset instance for "delta", that should be done
either by yielding a special message object, or using a callback handler.

Maybe non-zero exit status should generally raise exceptions instead of
being passed up by a callback or a return value, for example
apply-changeset could raise "arch.errors.ChangesetConflict".

> I'm also not quite sure what to do with diff output.  In theory, I can 
> have a PatchedFile class, containing PatchHunks, containing 
> UnchangedLines, AddedLines, and RemovedLines.  You'd be able to filter 
> out {arch} files that way, for example.  But I'm not sure whether this 
> is really the right approach.  I do want my colourized diffs, though.

That seems right to me. It is possible to make nested iterators which
are robust for depth-first _and_ breadth first iteration.

You can find a proof-of-concept, untested, implementation of robust
nested iterators in:

    address@hidden/pyarch--iter-merges--0

Johannes, can you please test that code with the graphing tool and tell
us if it works properly? I have been having it around (in changesets)
for a long time, pending proper test suite support, but it is more than
time that this become public and be tested in the field.

-- 
                                                            -- ddaa