Try again before avoiding death by recursion

[darktrojan]

I'm not sure about this. I've made it so that multiple occurrences at the same time are ignored.

But that's not what libical does (at least the ancient version that ships with Thunderbird). That gives every occurrence, even if it's the same as the previous one. In the first test case I added, the expected output would be:

2022-01-31T08:00:00
2022-01-31T08:00:00 <-- same as before
2022-02-28T08:00:00
2022-03-31T08:00:00
2022-03-31T08:00:00 <-- same as before
2022-04-30T08:00:00
2022-05-31T08:00:00
2022-05-31T08:00:00 <-- same as before
2022-06-30T08:00:00

The same behaviour can be replicated here easily, just by pulling out the "death by recursion" error completely.

Does anyone have insight into which is the right thing to do here?

[coveralls]

Pull Request Test Coverage Report for Build 3416467056

• 8 of 10 (80.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.009%) to 98.147%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/ical/recur_iterator.js	3	5	60.0%

Totals
Change from base Build 3275454147:	0.009%
Covered Lines:	9202
Relevant Lines:	9360

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 3416467056

• 8 of 10 (80.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.009%) to 98.147%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/ical/recur_iterator.js	3	5	60.0%

Totals
Change from base Build 3275454147:	0.009%
Covered Lines:	9202
Relevant Lines:	9360

---

💛 - Coveralls

[darktrojan]

Something else that occurred to me while looking at this: if the data isn't in order, e.g. BYMONTHDAY=3,1,2 then the occurrences will not be in chronological order, they'll be in the same order as the rule, e.g. 20221103, 20221101, 20221102. That may or may not be a problem in practice (I think Thunderbird always wants all occurrences in a range so the order doesn't matter) and libical does the same thing, but it doesn't seem right.

[darktrojan]

In the interests of fixing bad bugs, I might just remove the error in Thunderbird for now, since that produces the same behaviour as we got with libical.

[darktrojan]

In the interests of fixing bad bugs, I might just remove the error in Thunderbird for now, since that produces the same behaviour as we got with libical.

I did end up removing the error in Thunderbird. We haven't shipped the changes in this patch.

[kewisch]

Thanks for the update. How confident do you feel about these changes? Should we give it a try and wait for feedback?

[kewisch]

@darktrojan checking in on this

[darktrojan]

It's certainly an improvement to the current code, and I can't think of any other cases that might trigger the error. Let's ship it and find out what happens.