Integrated permissions/ownership diff output for etckeeper/bzr

[Thierry Carrez]

Binary package hint: etckeeper

Since the VCS doesn't handle full permissions/ownership information, etckeeper keeps track of the differences in permissions/ownership in a .etckeeper file.

To make etckeeper more user-friendly, it would be nice to integrate the ownership/permissions changes into the diff output at each file level (instead than showing the .etckeeper text diff at the end).

This can be done at the bzr etckeeper plugin level, by making bzr aware of the changes present in .etckeeper and produce an integrated output.

[Thierry Carrez (ttx)]

I think we need to hook in two places:

- InterTree.iter_changes (tree.py:928)
We need to yield file_ids that happen to just have a permissions/ownership change. A hook allowing to arbitrarily yield some entries (given their file_id) would definitely help.

- DiffTree._show_diff (diff.py:867)
We need to add permissions/ownership changes to properties_changed. A hook allowing to do that would be great. The alternative is to use the existing diff_factory system register a DiffPath that would represent the permissions/ownership change as a "content change". Less intrusive but somehow inexact.

Those shouldn't require any API change. I'll try to do a PoC for them.

[Thierry Carrez (ttx)]

Hmm... hooking at InterTree.iter_changes fails to catch all cases unfortunately (for example bzr diffing between current state and last revision uses InterDirStateTree instead). We probably need to hook at a higher level, like merging entries coming from extra iterators (that our hook would provide) directly in DiffTree._show_diff.

[Thierry Carrez (ttx)]

I managed to build a PoC integrated diff using two hooks in bzr:

* First one (called showdiff_modifychanges in the patch) allows to modify the changes iterator just before it's used in diff.py:_show_diff, so that you can add the files that just have a metadata difference

* Second one (called showdiff_extrachanges in the patch), also in diff.py:_show_diff, allows for each file in the iterator to append to the properties changetext (prop_str) a string describing metadata change, if any

See patch for details. I'd welcome review from bzr developers to confirm it is the best place to hook for the desired result.

[Robert Collins (lifeless)]

I would make the iter_changes modifier a Tree hook. Should it modify
iter_changes for things other than diff? e.g. status (I assume yes)),
commit? revert?

I'd do Tree.hooks 'iter_changes' as the hook point.
parameter list of old_tree, new_tree, changes, [rest of iter_changes
normal parameters]. Thats so that you can correctly handle 'diff foo'
for instance.

For the hook to change the output diff shows, as its not structured or
semantic it should be up at the UI/command layer. I'd consider
CommandDiff.hooks or something like that. Again, ask yourself if it
should be getting used in things like status output, commit output etc.

Generally this is heading in a good direction, but I suspect it should
get good broad coverage to answer the questions I've been raising.

-Rob

[Thierry Carrez (ttx)]

Thanks Robert for your advice.

You are right it makes more sense to change iter_changes in all cases, since the idea is to cover all cases.

Note that my idea was to use the hook to add files that happen just to have their metadata changed (so that they show as "changed" for all purposes), but also to remove ".etckeeper" (the file containing metadata info) from the iterator so that it gets ignored in the diff output. If the iterator is modified globally, I guess I should keep .etckeeper in so that it's properly used in all other operations, and filter it out at the command/UI layer ?

[Robert Collins (lifeless)]

Something like that. It seems to me that you may need two sets of hooks - core and UI ones. It may even be that all the things you are hooking are UI level.

[Thierry Carrez (ttx)]

Don't have time to work on it right now, so if anyone is interested...

[Jelmer Vernooij (jelmer)]

The new feature flags feature might be useful for this.