Developer Blog: joris

Joris Vink (joris@) writes "The other day Niall O'Higgins (niallo@) and I had a very harsh time tracking down some silly bug in openrcs, I thought it would be fun to blog about it just a little bit. So follow me into the world of apples, mangoes, and oranges!" The original RCS program provides an option to delete a range of revisions from an RCS file:

rcs -o'range' file

While this might sound like something not a lot of people would use, we still need to support it.

niallo@ wrote the code to support the option in rcs(1), which wasn't too hard - most of the real work was done by the rcs_rev_remove() function in the RCS API. However, he then came to the realisation that rcs_rev_remove() wasn't properly implemented. It would actually break the RCS file! The problem was that rcs_rev_remove() was very naive. The RCS file format was described in a previous article. Each deltatext (patch) is dependent on the previous one. So if you simply delete one, the file is no longer consistent. Furthermore, rcs_rev_remove() was not correctly updating the "next" pointers, nor was it freeing memory.

No big deal! We thought at the time, so niallo@ continued working on rcs_rev_remove(), we discussed which approach would be the best and most suitable for our implementation. So we talked and talked and came up with something that should've worked in theory!

After niallo@ wrote the code he tested it and found that it actually passed the regression test he wrote for -o! Too bad it was commented out and wasn't even being run. After this small debacle we found out that the code was messing something up, something really weird. Niallo@ got "invalid line specification in RCS patch" as error and sent his stuff over to me to have a look. I joked that if I could find the bug in less than 30 seconds I would get a free night of beers from him at c2k6.

Not 30 seconds later that I received his diff in my mailbox I replied to him with something that I thought would have fixed the problem!

HURRAY FREE BEER, at least that is what I thought. The fix I sent him was a one-liner:

	diff_format = D_RCSDIFF;

He forgot to set diff_format to D_RCSDIFF to make our diff code produce an RCS diff which is obviously used in the RCS file format. Without this, a normal format diff will be produced.

After he fixed this things still didn't work properly, he needed a break considering he was working on this stuff for the entire afternoon. I decided to have a closer look as to what was really going wrong!

After about half an hour of reading and testing with different debug options (yes, printf's are really helpfull) I noticed that somehow the wrong deltatext was being replaced, I took a closer look to the code and noticed how we were merely using the revisions in the wrong place!

                /* calculate new diff */
                strlcpy(path_tmp1, tmpdir, sizeof(path_tmp1));
                strlcat(path_tmp1, "/diff1.XXXXXXXXXX", sizeof(path_tmp1));
                cvs_buf_write_stmp(nextbuf, path_tmp1, 0600);
                cvs_buf_free(nextbuf);
 
                strlcpy(path_tmp2, tmpdir, sizeof(path_tmp2));
                strlcat(path_tmp2, "/diff2.XXXXXXXXXX", sizeof(path_tmp2));
                cvs_buf_write_stmp(prevbuf, path_tmp2, 0600);
                cvs_buf_free(prevbuf);

Basically "next" and "prev" were the wrong way round! They should have been:

                /* calculate new diff */
                strlcpy(path_tmp1, tmpdir, sizeof(path_tmp1));
                strlcat(path_tmp1, "/diff1.XXXXXXXXXX", sizeof(path_tmp1));
                cvs_buf_write_stmp(prevbuf, path_tmp1, 0600);
                cvs_buf_free(prevbuf);
 
                strlcpy(path_tmp2, tmpdir, sizeof(path_tmp2));
                strlcat(path_tmp2, "/diff2.XXXXXXXXXX", sizeof(path_tmp2));
                cvs_buf_write_stmp(nextbuf, path_tmp2, 0600);
                cvs_buf_free(nextbuf);

--

                if (rcs_deltatext_set(rf, prevrdp->rd_num, newdeltatext) < 0)
                        fatal("error setting new deltatext");

should have been

                if (rcs_deltatext_set(rf, nextrdp->rd_num, newdeltatext) < 0)
                        fatal("error setting new deltatext");

--

                rcsnum_cpy(prevrdp->rd_num, nextrdp->rd_next, 0);

should have been

                rcsnum_cpy(nextrdp->rd_num, prevrdp->rd_next, 0);

Oh my gosh I figured, if this is true me and niallo@ deserve a smacking on our head for getting this wrong!

We are currently waiting to get our heads smacked.

.joris