82143 - Reversing dependencies causes "circular dependency chain" error

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Stephen Lee]

This is probably in part a side-effect of the fix for bug 51670.

If I have two related bugs, and make bug 1 depend on bug 2, and then later 
realise that I meant it the other way round (e.g. typed into the wrong field), 
it is not possible to reverse the dependency in a single step.

Intuitively I would expect to be able to delete the '2' from the "depends on" 
of bug 1, type '2' into the "blocks", and submit the form.

Bugzilla tells me that "The change you are making to dependencies has caused a 
circular dependency chain.", when in fact it has not, but merely reversed the 
direction of the existing chain.

In order to acheive the required operation, I have to first remove one 
dependency and then add the other as a separate operation which makes it less 
obvious what is happening in the bugmails.

Comment 1

[Stephen Lee]

Looking more closely, it seems that the point at which this error is generated 
is from code that pre-exists the fix for bug 51670.

After that fix, the original dependency loop check is unnecessary, as all real 
attempted dependency loops will by picked up by the code added to fix bug 51670.

The following patch (to be attached) allows a dependency to be reversed in a 
single step by removing the original dependency check that was effectively 
superseded by the fix for bug 51670.

The patch is against the original bugzilla 2.12, but 
http://bonsai.mozilla.org/cvsblame.cgi shows no other changes within the 
context area at present.

Comment 2

[Stephen Lee]

Attachment: Patch to remove old dependency loop check, so that dependencies can be reversed in a single step.

Comment 3

[Dave Miller [:justdave]]

*** Bug 92572 has been marked as a duplicate of this bug. ***

Comment 4

[Dave Miller [:justdave]]

*** Bug 98477 has been marked as a duplicate of this bug. ***

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

->New (old by now?) product Bugzilla

Comment 6

[Gervase Markham [:gerv]]

Comment on attachment 43222 [details] [diff] [review]
Patch to remove old dependency loop check, so that dependencies can be reversed in a single step.

r=gerv.

Gerv

Comment 7

[Jacob Steenhagen]

Comment on attachment 43222 [details] [diff] [review]
Patch to remove old dependency loop check, so that dependencies can be reversed in a single step.

With this patch applied, it's not possible to set a simple loop (such as 1
depends on and blocks 2), but it is possible to set a more complicated loop
(such as 1 blocks 2 which blocks 3 which blocks 1).  This complicated loop is
not allowed without this patch applied.

Comment 8

[Stephen Lee]

Attachment: Updated patch allowing detection of non-trivial circular loops

This patch fixes the additional fault reported above, by making the lists used
by the code added for bug 51670 use a complete dependency tree (as was
presumably originally intended) rather than only cross-checking the first-level
dependencies. Multi-level dependencies are now pushed onto the deps{$target}
array as well as the temporary stack.

This is actually a different bug that pre-exists the original patch, but is
made more visible by it.

Can someone mark my old patch obsolete, as I don't have permission.

Comment 9

[Stephen Lee]

Attachment: Further updated patch fixing unwanted side-effects

The most recent patch had an unwanted side-effect. All indirect dependencies
were added as direct dependencies.

This version preserves the cross-checking of indirect dependencies, but does
not add them as direct dependencies. A $deptree array is added that contains
all dependencies, while the $deps remains as only the first level dependencies.
Obsoletes both existing patches.

Comment 10

[Stephen Lee]

> Additional Comment #7 From Jake 2001-11-23 20:04
[...]
> This complicated loop is not allowed without this patch applied.

... unless both sides of the loop are created simultaneously. See bug 113250 / 
http://bugzilla.mozilla.org/show_activity.cgi?id=113250 for an example.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: applies on latest tip

This patch applies on the tip.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 72643 [details] [diff] [review]
patch v4: applies on latest tip

Works, can't break it. r=myk

Comment 13

[Bradley Baetz (:bbaetz)]

If I try a complicated loop involving 4 bugs, I get:

"This bug can't be both blocked and dependent on bug #2 #3 #4 !"

That error message isn't really very clear to me.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

I skippped over this error message in my review, but it bothered me too at the
time.  Ideally the message would say something like the following:

---
You tried to make bug W dependent on bug X, but that would mean it was also
dependent on bug Z via the X -> Y -> Z dependency chain, which would create a
circular dependency chain since bug Z is dependent on bug W.  Circular
dependency chains are not allowed in Bugzilla.
---
or
---
You tried to make bug W block bug X, but that would mean it also blocks bug Z
via the X -> Y -> Z dependency chain, which would create a circular dependency
chain since bug Z blocks bug W.  Circular dependency chains are not allowed in
Bugzilla.
---

Comment 15

[Bradley Baetz (:bbaetz)]

That seems complicated. Maybe just adding -> between the links, and ->
<current-bug-#> at the end would do? The text would then be:

Adding this dependancy would cause a loop, which is not permitted:
<current>->x->y->z-><current>.

Linkify the bug numbers too, I guess.

Comment 16

[Stephen Lee]

This would be misleading if there are multiple dependency loops, or a forked 
dependency loop. The list given should be interpreted only as an unordered list 
of bugs that would be involved in a dependency loop if the requested changes 
were committed.

Perhaps something like:
The following bug(s) would appear on both the "depends on" and "blocks" parts 
of the dependency tree if these changes are committed: #2 #3 #4
This would create a circular dependency, which is not allowed.

Comment 17

[Bradley Baetz (:bbaetz)]

Well, there can't be more than one loop, surely?

We only find one such loop, so wording along the lines of "at least one such
loop is". That case is re;latively rare - I don't think we need to worry about
mentioning them all. There can only be one from each dependancy added, though,
so maybe we could list them all.

Comment 18

[Gervase Markham [:gerv]]

What's the status here? Are we changing the error message or not? This blocks a
2.16 blocker...

Gerv

Comment 19

[Dave Miller [:justdave]]

Attachment: Patch v5: save as v4 with updated error message

Uses Stephen's suggested error message, linkifies the bug numbers.

Comment 20

[Dave Miller [:justdave]]

Comment on attachment 77035 [details] [diff] [review]
Patch v5: save as v4 with updated error message

r= justdave for functionality.	Since I modified the error message,
second-reviewer will need to stamp that for me.

Comment 21

[Stephen Lee]

Something along the lines of Myk's suggestions in comment 14 would be ideal. We 
just don't have the information available to do that reliably at present within 
the data structures produced by the checking algorithm.

My proposed error message is intended to be non-misleading, and clarify what 
the existing information presented is. Hopefully it should be fairly obvious 
what is going on in most normal circumstances.

This bug already deals with 2 different issues, so can perhaps someone who 
wants it could raise the additional requirement of more detailed reporting of 
dependency loops as a new enhancement rather than further delaying this fix.

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 77035 [details] [diff] [review]
Patch v5: save as v4 with updated error message

Looks good. r=myk

Comment 23

[Dave Miller [:justdave]]

reassigning to patch author

Comment 24

[Dave Miller [:justdave]]

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.119; previous revision: 1.118
done