versioncontrol api problem

[thomas.moschny@…]

Changeset.get_changes() is supposed to return tuples of type (path, kind, change, base_path, base_rev). For an implementor of the vc api it is however not clear what to return for path in case change is a DELETE.

In analogy to Repository.get_changes() (resp. its description) one would expect to be required to pass None in that case (as there is no current/new path for a deleted node.)

This however leads to an oops for restricted changeset views at this point: trunk/trac/versioncontrol/web_ui/changeset.py@4634#L281​.

The code in the if (restricted) block should ignore the value of npath iff change is a DELETE.

Furthermore, the documentation for Changeset.get_changes() could be updated accordingly.

[moschny]

Forgot to set the component right, sorry.

[Matthew Good]

Replying to thomas.moschny@gmx.de:

Changeset.get_changes() is supposed to return tuples of type (path, kind, change, base_path, base_rev). For an implementor of the vc api it is however not clear what to return for path in case change is a DELETE.

I believe that the path should be the path that was deleted.

Please post a topic on the development mailing list to confirm the expected return value from this method. If necessary this could be repopened as a "task" for updating the API documentation to clarify this.

[Christian Boos]

Replying to mgood:

Replying to thomas.moschny@gmx.de:

Changeset.get_changes() is supposed to return tuples of type (path, kind, change, base_path, base_rev). For an implementor of the vc api it is however not clear what to return for path in case change is a DELETE.

I believe that the path should be the path that was deleted.

You're both right, currently the path is assumed to be the path that was deleted, but it's neither documented, nor intuitive. For now, I'll update the doc to reflect the current usage.

[Christian Boos]

Doc fixed in r4635. If you think the API should be changed in further releases, please discuss that on the trac-devel mailing list, as matt suggested.

[moschny]

Replying to cboos:

Doc fixed in r4635.

Thanks.

Nevertheless, I think there's a flaw in the handling of the restricted case. Shouldn't a change be included if either opath or npath are affected?

The following patch tries to achive that and additionally fixes the oops I mentioned earlier. It would obsolete the doc change made r4635, but not really change the api, because it wasn't clearly defined anyway it completely ignores the first argument for a deletion.

trac/versioncontrol/web_ui/changeset.py

@@ -274 +274 @@
 
             # -- getting the change summary from the Changeset.get_changes
             def get_changes():
+
+                def is_related(path_a, path_b):
+                    return (path_a == path_b or                # same path
+                            path_a.startswith(path_b + '/') or # path_a is below
+                            path_b.startswith(path_a + '/'))   # path_a is above
+
                 for npath, kind, change, opath, orev in chgset.get_changes():
                     old_node = new_node = None
-                    if (restricted and
-                        not (npath == path or                # same path
-                             npath.startswith(path + '/') or # npath is below
-                             path.startswith(npath + '/'))): # npath is above
-                        continue
+                    show_change = not restricted
                     if change != Changeset.ADD:
+                        show_change = show_change or is_related(path, opath)
                         old_node = repos.get_node(opath, orev)
                     if change != Changeset.DELETE:
+                        show_change = show_change or is_related(path, npath)
                         new_node = repos.get_node(npath, rev)
-                    yield old_node, new_node, kind, change
+                    if show_change:
+                        yield old_node, new_node, kind, change
+                    else:
+                        continue
 
             def _changeset_title(rev):
                 if restricted:

[Christian Boos]

Replying to moschny:

Replying to cboos:

Doc fixed in r4635.

Thanks.

Nevertheless, I think there's a flaw in the handling of the restricted case. Shouldn't a change be included if either opath or npath are affected?

I'm not sure but I don't think so… Say you have a copy /from -> /to, with modifications. Now you'd like to look at the changeset for /from, what will you see? … the modifications for /to. That would be a bit confusing, no?

[moschny]

Replying to cboos:

Say you have a copy /from -> /to, with modifications. Now you'd like to look at the changeset for /from, what will you see? … the modifications for /to. That would be a bit confusing, no?

Hmm, yes. Maybe I carried this to excess a bit.

On the other hand, in Monotone, we don't have copies, but real moves. And a move /from -> /to implies that the affected node(s) are no longer visible in /from. However, even when looking at (resp. restricting to) /from, one might want to see the "leaving" nodes, I think.

So imho the mentioned code block in changeset.py might need some more differentiation:

• look at npath for moves, edits, copies
• look at opath for moves, deletions

Or maybe I am on the wrong track?