Closed Bug 82143 Opened 23 years ago Closed 23 years ago

Reversing dependencies causes "circular dependency chain" error

Categories

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

2.12

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: bugzilla-mozilla, Assigned: bugzilla-mozilla)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Target Milestone: --- → Bugzilla 2.16
Priority: -- → P3
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.
Depends on: 51670
Keywords: patch, review
*** Bug 92572 has been marked as a duplicate of this bug. ***
*** Bug 98477 has been marked as a duplicate of this bug. ***
->New (old by now?) product Bugzilla
Assignee: tara → myk
Component: Bugzilla → Creating/Changing Bugs
Product: Webtools → Bugzilla
Version: Bugzilla 2.12 → 2.12
Severity: minor → major
Summary: Reversing dependencies is difficult → Reversing dependencies causes "circular dependency chain" error
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
Attachment #43222 - Flags: review+
Priority: P3 → P1
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.
Attachment #43222 - Flags: review-
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.
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.
Blocks: 113250
No longer blocks: 113250
Attachment #60143 - Attachment is obsolete: true
Attachment #43222 - Attachment is obsolete: true
> 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.
Blocks: 95594
Attached patch patch v4: applies on latest tip (obsolete) — Splinter Review
This patch applies on the tip.
Attachment #60152 - Attachment is obsolete: true
Comment on attachment 72643 [details] [diff] [review]
patch v4: applies on latest tip

Works, can't break it. r=myk
Attachment #72643 - Flags: review+
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.
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.
---
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.
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.
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.
What's the status here? Are we changing the error message or not? This blocks a
2.16 blocker...

Gerv
Uses Stephen's suggested error message, linkifies the bug numbers.
Attachment #72643 - Attachment is obsolete: true
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.
Attachment #77035 - Flags: review+
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 on attachment 77035 [details] [diff] [review]
Patch v5: save as v4 with updated error message

Looks good. r=myk
Attachment #77035 - Flags: review+
reassigning to patch author
Assignee: myk → slee
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
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: