Contributed by niallo on from the cluebat-to-openrcs-hackers dept.
rcs -o'range' fileWhile 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.
(Comments are closed)