Edgewall Software
Modify

Opened 15 years ago

Closed 15 years ago

Last modified 15 years ago

#4595 closed task (fixed)

versioncontrol api problem

Reported by: thomas.moschny@… Owned by: Christian Boos
Priority: normal Milestone: 0.11
Component: version control Version: devel
Severity: minor Keywords: api documentation
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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.

Attachments (0)

Change History (7)

comment:1 by moschny, 15 years ago

Component: generalversion control

Forgot to set the component right, sorry.

in reply to:  description ; comment:2 by Matthew Good, 15 years ago

Milestone: 0.11
Resolution: invalid
Status: newclosed

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.

in reply to:  2 comment:3 by Christian Boos, 15 years ago

Keywords: api documentation added
Milestone: 0.11
Resolution: invalid
Severity: normalminor
Status: closedreopened
Type: defecttask

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.

comment:4 by Christian Boos, 15 years ago

Resolution: fixed
Status: reopenedclosed

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.

in reply to:  4 ; comment:5 by moschny, 15 years ago

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

     
    274274
    275275            # -- getting the change summary from the Changeset.get_changes
    276276            def get_changes():
     277
     278                def is_related(path_a, path_b):
     279                    return (path_a == path_b or                # same path
     280                            path_a.startswith(path_b + '/') or # path_a is below
     281                            path_b.startswith(path_a + '/'))   # path_a is above
     282
    277283                for npath, kind, change, opath, orev in chgset.get_changes():
    278284                    old_node = new_node = None
    279                     if (restricted and
    280                         not (npath == path or                # same path
    281                              npath.startswith(path + '/') or # npath is below
    282                              path.startswith(npath + '/'))): # npath is above
    283                         continue
     285                    show_change = not restricted
    284286                    if change != Changeset.ADD:
     287                        show_change = show_change or is_related(path, opath)
    285288                        old_node = repos.get_node(opath, orev)
    286289                    if change != Changeset.DELETE:
     290                        show_change = show_change or is_related(path, npath)
    287291                        new_node = repos.get_node(npath, rev)
    288                     yield old_node, new_node, kind, change
     292                    if show_change:
     293                        yield old_node, new_node, kind, change
     294                    else:
     295                        continue
    289296
    290297            def _changeset_title(rev):
    291298                if restricted:

in reply to:  5 ; comment:6 by Christian Boos, 15 years ago

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?

in reply to:  6 comment:7 by moschny, 15 years ago

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?

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.