Applying fts2.patch to fts3.

Porting fts2.patch to fts3. fts2.patch was created in
http://codereview.chromium.org/243068 and fts2.c was patched in
http://codereview.chromium.org/216026.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=48812

[Scott Hess - ex-Googler, 2010-04-30 18:29:45]

checkpoint.  will try to deliver more after lunch.

http://codereview.chromium.org/1638019/diff/11001/12002
File third_party/sqlite/ext/fts3/fts3.c (right):

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1661
third_party/sqlite/ext/fts3/fts3.c:1661: if( rc!=SQLITE_OK ) break;
I think this needs DataBufferDestroy(&one).  Additionally, once past this point
you need to destroy both buffers, as at the end of the scope.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1680
third_party/sqlite/ext/fts3/fts3.c:1680: plwInit(&plwriter, &writer,
dlrDocid(dlrAtEnd(&dr1)?&dr2:&dr1));
I was going to say that after this, failure would need a plwDestroy(), but ... I
see that this code is very inconsistent with balancing *Init() and *Destroy(),
presumably because they imagine it's unnecessary (AFAICT the compiler would just
optimize it away in release mode).  Someday someone will probably regret
omitting those calls :-).  But it would be inappropriate to hold you to a higher
standard for this patch, I think.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1690
third_party/sqlite/ext/fts3/fts3.c:1690: break;
In keeping with the rest of this code, I'd say:
  if( rc!=SQLITE_OK ) break;
so that readers don't see the code sometimes plrDestroy() and other times not
and imagine that was intentional.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1738
third_party/sqlite/ext/fts3/fts3.c:1738: static int docListAndMerge(
Wow - really annoying that codereview.* decided that this was a big new change,
and it should compare old docListAndMerge with new docListOrMerge.  Sigh.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1751
third_party/sqlite/ext/fts3/fts3.c:1751: dlrInit(&right, DL_DOCIDS, pRight,
nRight);
Missing rc =

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1799
third_party/sqlite/ext/fts3/fts3.c:1799: return;
return SQLITE_OK;

[Err, has this guy been compiled?]

http://codereview.chromium.org/1638019/diff/11001/12002#newcode3930
third_party/sqlite/ext/fts3/fts3.c:3930: dataBufferDestroy(pResult);
pResult should have been an initialized DataBuffer before this call and should
be in charge of destroying it.  Note that res "stole" the old one and replaced
it with an empty.  You could empty it, here, but should not destroy it. 
[Emptying is probably not warranted, the caller shouldn't be inspecting it in
case of error.]

http://codereview.chromium.org/1638019/diff/11001/12002#newcode4682
third_party/sqlite/ext/fts3/fts3.c:4682: if( !n || nTerm<0 || n
Term>pReader->nData-n) return SQLITE_CORRUPT_BKPT;
s/n Term/nTerm/

http://codereview.chromium.org/1638019/diff/11001/12002#newcode4729
third_party/sqlite/ext/fts3/fts3.c:4729: if( nPrefix<0 ||
nPrefix>pReader->termp.nData ) return SQLITE_CORRUPT_BKPT;
s/termp/term/

[Scott Hess - ex-Googler, 2010-04-30 19:23:17]

OK, I've made a full pass.

My primary approach was to diff the fts2 version of this patch against the fts3
version and look for things that seemed wrong.  Unfortunately, it will be hard
to say what this might be missing ... please make sure you go over the call
points for functions which changed from void->int return, and double-check that
all of them now are checked.

You need to update README.google.

Also, you should build the sqlite test suite and run the relevant fts3 tests.  I
believe there were comments in the google readme for how to manage this for
Linux.  Given what's involved, I don't think running it on multiple platforms is
necessary.  You should probably run it before/after so that you know which tests
we already fail :-).

http://codereview.chromium.org/1638019/diff/11001/12002
File third_party/sqlite/ext/fts3/fts3.c (right):

http://codereview.chromium.org/1638019/diff/11001/12002#newcode5321
third_party/sqlite/ext/fts3/fts3.c:5321: dataBufferAppend(&pReader->term,
pReader->pData+n, nSuffix);
I think you missed a line, here.  pData += n a few lines up, so no +n here.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode5975
third_party/sqlite/ext/fts3/fts3.c:5975: interiorReaderStep(&reader);
Missing the tests around this.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode6162
third_party/sqlite/ext/fts3/fts3.c:6162: dlrDestroy(&readers[1]);
s/1/0/;

http://codereview.chromium.org/1638019/diff/11001/12002#newcode6633
third_party/sqlite/ext/fts3/fts3.c:6633: optLeavesReaderData(&readers[0]),
In the fts2 patch this has pData.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode6849
third_party/sqlite/ext/fts3/fts3.c:6849: /* Corrupt if we get back different
tyeps than we stored. */
s/tyeps/types/;

[dumi, 2010-05-08 02:25:17]

Thanks for such a careful review!

I've successfully compiled fts3 on Linux and Win after applying this patch.

Also, I ran all sqlite tests on Linux, and only the 2 tests mentioned in
README.chromium have failed before and after applying the patch. In fact, the
output of the 2 runs was pretty much identical.

http://codereview.chromium.org/1638019/diff/11001/12002
File third_party/sqlite/ext/fts3/fts3.c (right):

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1661
third_party/sqlite/ext/fts3/fts3.c:1661: if( rc!=SQLITE_OK ) break;
On 2010/04/30 18:29:45, shess wrote:
> I think this needs DataBufferDestroy(&one).  Additionally, once past this
point
> you need to destroy both buffers, as at the end of the scope.

it seems like the sqlite authors don't care too much about freeing memory in
case of errors. do we need to care about them? is there a way to make sure these
buffers are destroyed without adding dataBufferDestroy() calls before every
single break/goto instruction in this function?

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1680
third_party/sqlite/ext/fts3/fts3.c:1680: plwInit(&plwriter, &writer,
dlrDocid(dlrAtEnd(&dr1)?&dr2:&dr1));
On 2010/04/30 18:29:45, shess wrote:
> I was going to say that after this, failure would need a plwDestroy(), but ...
I
> see that this code is very inconsistent with balancing *Init() and *Destroy(),
> presumably because they imagine it's unnecessary (AFAICT the compiler would
just
> optimize it away in release mode).  Someday someone will probably regret
> omitting those calls :-).  But it would be inappropriate to hold you to a
higher
> standard for this patch, I think.

there's a plwTerminate(&plwriter) call below. it seems like they don't care
about error cases...

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1690
third_party/sqlite/ext/fts3/fts3.c:1690: break;
On 2010/04/30 18:29:45, shess wrote:
> In keeping with the rest of this code, I'd say:
>   if( rc!=SQLITE_OK ) break;
> so that readers don't see the code sometimes plrDestroy() and other times not
> and imagine that was intentional.

i think we should leave it as it is. it seems to me that whenever 2 pointers are
initialized, the pattern is usually:

rc = initP1();
if (rc != SQLITE_OK) return;
rc = initP2();
if (rc != SQLITE_OK) {
  destroyP1();
  return;
}

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1751
third_party/sqlite/ext/fts3/fts3.c:1751: dlrInit(&right, DL_DOCIDS, pRight,
nRight);
On 2010/04/30 18:29:45, shess wrote:
> Missing rc =

fixed.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1799
third_party/sqlite/ext/fts3/fts3.c:1799: return;
On 2010/04/30 18:29:45, shess wrote:
> return SQLITE_OK;

fixed.

> [Err, has this guy been compiled?]

yes, and didn't run into any problem... weird.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode3930
third_party/sqlite/ext/fts3/fts3.c:3930: dataBufferDestroy(pResult);
On 2010/04/30 18:29:45, shess wrote:
> pResult should have been an initialized DataBuffer before this call and should
> be in charge of destroying it.  Note that res "stole" the old one and replaced
> it with an empty.  You could empty it, here, but should not destroy it. 
> [Emptying is probably not warranted, the caller shouldn't be inspecting it in
> case of error.]

done. just returning 'rc' in case of error.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode4682
third_party/sqlite/ext/fts3/fts3.c:4682: if( !n || nTerm<0 || n
Term>pReader->nData-n) return SQLITE_CORRUPT_BKPT;
On 2010/04/30 18:29:45, shess wrote:
> s/n Term/nTerm/

fixed... hmm, now i REALLY wonder how this got compiled... i'm pretty sure i saw
VS compile fts3.c...

http://codereview.chromium.org/1638019/diff/11001/12002#newcode4729
third_party/sqlite/ext/fts3/fts3.c:4729: if( nPrefix<0 ||
nPrefix>pReader->termp.nData ) return SQLITE_CORRUPT_BKPT;
On 2010/04/30 18:29:45, shess wrote:
> s/termp/term/

fixed.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode5321
third_party/sqlite/ext/fts3/fts3.c:5321: dataBufferAppend(&pReader->term,
pReader->pData+n, nSuffix);
On 2010/04/30 19:23:17, shess wrote:
> I think you missed a line, here.  pData += n a few lines up, so no +n here.

it's a few lines above, but there's no "+n" indeed. fixed.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode5975
third_party/sqlite/ext/fts3/fts3.c:5975: interiorReaderStep(&reader);
On 2010/04/30 19:23:17, shess wrote:
> Missing the tests around this.

oops, added.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode6162
third_party/sqlite/ext/fts3/fts3.c:6162: dlrDestroy(&readers[1]);
On 2010/04/30 19:23:17, shess wrote:
> s/1/0/;

fixed.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode6633
third_party/sqlite/ext/fts3/fts3.c:6633: optLeavesReaderData(&readers[0]),
On 2010/04/30 19:23:17, shess wrote:
> In the fts2 patch this has pData.

done.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode6849
third_party/sqlite/ext/fts3/fts3.c:6849: /* Corrupt if we get back different
tyeps than we stored. */
On 2010/04/30 19:23:17, shess wrote:
> s/tyeps/types/;

fixed.

[Scott Hess - ex-Googler, 2010-05-13 21:51:51]

Sorry for the long delay.  Also sorry for the lack of temporal sense in my
comments - maybe read them twice :-).  The gist of it is that there are things
we can get away with not calling *Destroy() on, and other things we can't get
away without.

I can't quite wrap my head around giving this another thorough compare-the-diffs
review.  My head might explode.  So I'm just reviewing your comments and patch
#3 :-).

http://codereview.chromium.org/1638019/diff/11001/12002
File third_party/sqlite/ext/fts3/fts3.c (right):

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1661
third_party/sqlite/ext/fts3/fts3.c:1661: if( rc!=SQLITE_OK ) break;
On 2010/05/08 02:25:17, dumi wrote:
> On 2010/04/30 18:29:45, shess wrote:
> > I think this needs DataBufferDestroy(&one).  Additionally, once past this
point
> > you need to destroy both buffers, as at the end of the scope.
> 
> it seems like the sqlite authors don't care too much about freeing memory in
> case of errors. do we need to care about them? is there a way to make sure
these
> buffers are destroyed without adding dataBufferDestroy() calls before every
> single break/goto instruction in this function?

The SQLite authors are generally cognizant of this type of issue, as a large
portion of SQLite's usage is on constrained devices like cellphones.  But it's
hard to thoroughly test error cases, and I can see that someone's been writing
code in here where they have manually inspected what the destroy call would do
and decided it wasn't necessary at that point.  [Since they're generally tiny
static functions, I'd be inclined to let the compiler make that decision.]

Anyhow, I think we should do it right, if we can, and where it doesn't depart
from the code's conventions.  Unfortunately, you're in C code, now, you don't
get to spin up an instance of scoped_awesomeness... this is one of the reasons
that the SQLite code is using goto in the first place, to consolidate error
handling.

NOTE: DataBufferDestroy() _does_ free memory, if memory has been allocated. 
|one| can have contents if the posListPhraseMerge() against |dlwriter2|
succeeded.  So not freeing it here is a definite leak.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1690
third_party/sqlite/ext/fts3/fts3.c:1690: break;
On 2010/05/08 02:25:17, dumi wrote:
> On 2010/04/30 18:29:45, shess wrote:
> > In keeping with the rest of this code, I'd say:
> >   if( rc!=SQLITE_OK ) break;
> > so that readers don't see the code sometimes plrDestroy() and other times
not
> > and imagine that was intentional.
> 
> i think we should leave it as it is. it seems to me that whenever 2 pointers
are
> initialized, the pattern is usually:
> 
> rc = initP1();
> if (rc != SQLITE_OK) return;
> rc = initP2();
> if (rc != SQLITE_OK) {
>   destroyP1();
>   return;
> }

My point was that this code seems to elide the plrDestroy() calls, and I don't
think we should introduce a plrDestroy() in case of error when there is no
plrDestroy() in case of success!

[plrDestroy() just scrambles the memory in debug mode, so this won't cause a
leak.  I find omitting it distasteful, because maybe someday someone will change
PLReader in a way which will cause a leak.  But when in Rome and all that.  The
more we depart from SQLite's code, the more likely a future change will cause us
problems.  Though someone should probably upstream these changes.]

http://codereview.chromium.org/1638019/diff/11001/12002#newcode5321
third_party/sqlite/ext/fts3/fts3.c:5321: dataBufferAppend(&pReader->term,
pReader->pData+n, nSuffix);
On 2010/05/08 02:25:17, dumi wrote:
> On 2010/04/30 19:23:17, shess wrote:
> > I think you missed a line, here.  pData += n a few lines up, so no +n here.
> 
> it's a few lines above, but there's no "+n" indeed. fixed.

That's what I meant :-).

[Scott Hess - ex-Googler, 2010-05-13 21:53:24]

Also, I think it might be worthwhile to upgrade your commit comment, maybe by
linking to the original fts2 patch, and maybe even including the comment from
that patch.  Maybe even include that BUG, if there was one.  Just so that
someone doesn't end up doing forensics to figure what happened.

[dumi, 2010-05-14 03:01:21]

Changed the patch description to include links to the patch that added
fts2.patch and the patch that made the changes to fts2.c.

http://codereview.chromium.org/1638019/diff/11001/12002
File third_party/sqlite/ext/fts3/fts3.c (right):

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1661
third_party/sqlite/ext/fts3/fts3.c:1661: if( rc!=SQLITE_OK ) break;
On 2010/05/13 21:51:52, shess wrote:
> On 2010/05/08 02:25:17, dumi wrote:
> > On 2010/04/30 18:29:45, shess wrote:
> > > I think this needs DataBufferDestroy(&one).  Additionally, once past this
> point
> > > you need to destroy both buffers, as at the end of the scope.
> > 
> > it seems like the sqlite authors don't care too much about freeing memory in
> > case of errors. do we need to care about them? is there a way to make sure
> these
> > buffers are destroyed without adding dataBufferDestroy() calls before every
> > single break/goto instruction in this function?
> 
> The SQLite authors are generally cognizant of this type of issue, as a large
> portion of SQLite's usage is on constrained devices like cellphones.  But it's
> hard to thoroughly test error cases, and I can see that someone's been writing
> code in here where they have manually inspected what the destroy call would do
> and decided it wasn't necessary at that point.  [Since they're generally tiny
> static functions, I'd be inclined to let the compiler make that decision.]
> 
> Anyhow, I think we should do it right, if we can, and where it doesn't depart
> from the code's conventions.  Unfortunately, you're in C code, now, you don't
> get to spin up an instance of scoped_awesomeness... this is one of the reasons
> that the SQLite code is using goto in the first place, to consolidate error
> handling.
> 
> NOTE: DataBufferDestroy() _does_ free memory, if memory has been allocated. 
> |one| can have contents if the posListPhraseMerge() against |dlwriter2|
> succeeded.  So not freeing it here is a definite leak.

done. please check the correctness of my goto's.

http://codereview.chromium.org/1638019/diff/11001/12002#newcode1690
third_party/sqlite/ext/fts3/fts3.c:1690: break;
On 2010/05/13 21:51:52, shess wrote:
> On 2010/05/08 02:25:17, dumi wrote:
> > On 2010/04/30 18:29:45, shess wrote:
> > > In keeping with the rest of this code, I'd say:
> > >   if( rc!=SQLITE_OK ) break;
> > > so that readers don't see the code sometimes plrDestroy() and other times
> not
> > > and imagine that was intentional.
> > 
> > i think we should leave it as it is. it seems to me that whenever 2 pointers
> are
> > initialized, the pattern is usually:
> > 
> > rc = initP1();
> > if (rc != SQLITE_OK) return;
> > rc = initP2();
> > if (rc != SQLITE_OK) {
> >   destroyP1();
> >   return;
> > }
> 
> My point was that this code seems to elide the plrDestroy() calls, and I don't
> think we should introduce a plrDestroy() in case of error when there is no
> plrDestroy() in case of success!
> 
> [plrDestroy() just scrambles the memory in debug mode, so this won't cause a
> leak.  I find omitting it distasteful, because maybe someday someone will
change
> PLReader in a way which will cause a leak.  But when in Rome and all that. 
The
> more we depart from SQLite's code, the more likely a future change will cause
us
> problems.  Though someone should probably upstream these changes.]

done.

[Scott Hess - ex-Googler, 2010-05-14 21:34:56]

getting very close.  hope you're enjoying this code :-).

http://codereview.chromium.org/1638019/diff/34001/35002
File third_party/sqlite/ext/fts3/fts3.c (right):

http://codereview.chromium.org/1638019/diff/34001/35002#newcode1620
third_party/sqlite/ext/fts3/fts3.c:1620: DataBuffer two = {0, 0, 0};
I see what you did there ... that steps around a lot of the issues!  Suggest a
comment, though, along the lines of "These are used in the loop, but pulled out
here to simplify error-handling."

http://codereview.chromium.org/1638019/diff/34001/35002#newcode1715
third_party/sqlite/ext/fts3/fts3.c:1715: dataBufferDestroy(&two);
This won't work, you never re-initialize the buffers for the next pass through
the outer loop.  You could change to dataBufferReset(), I think.

http://codereview.chromium.org/1638019/diff/34001/35002#newcode1725
third_party/sqlite/ext/fts3/fts3.c:1725: goto err;
If you use dataBufferReset() above, then I think you can drop this goto and push
the dataBufferDestroy() calls to be under err:.  Then many lines can go back to
their earlier form, and your new calls can change to break or goto-err.

[dumi, 2010-05-14 22:49:16]

http://codereview.chromium.org/1638019/diff/34001/35002
File third_party/sqlite/ext/fts3/fts3.c (right):

http://codereview.chromium.org/1638019/diff/34001/35002#newcode1620
third_party/sqlite/ext/fts3/fts3.c:1620: DataBuffer two = {0, 0, 0};
On 2010/05/14 21:34:56, shess wrote:
> I see what you did there ... that steps around a lot of the issues!  Suggest a
> comment, though, along the lines of "These are used in the loop, but pulled
out
> here to simplify error-handling."

done.

http://codereview.chromium.org/1638019/diff/34001/35002#newcode1715
third_party/sqlite/ext/fts3/fts3.c:1715: dataBufferDestroy(&two);
On 2010/05/14 21:34:56, shess wrote:
> This won't work, you never re-initialize the buffers for the next pass through
> the outer loop.  You could change to dataBufferReset(), I think.

done.

http://codereview.chromium.org/1638019/diff/34001/35002#newcode1725
third_party/sqlite/ext/fts3/fts3.c:1725: goto err;
On 2010/05/14 21:34:56, shess wrote:
> If you use dataBufferReset() above, then I think you can drop this goto and
push
> the dataBufferDestroy() calls to be under err:.  Then many lines can go back
to
> their earlier form, and your new calls can change to break or goto-err.

done.

[dumi, 2010-06-01 22:21:15]

ping.

[Scott Hess - ex-Googler, 2010-06-02 19:55:53]

Durn, I thought we were done with this.

The first two comments are advisory - ignore them if you wish.  The last two
comments I found by comparing the fts2.patch and fts3.patch files, and I think
do require changes.

http://codereview.chromium.org/1638019/diff/37002/49002
File third_party/sqlite/ext/fts3/fts3.c (right):

http://codereview.chromium.org/1638019/diff/37002/49002#newcode1641
third_party/sqlite/ext/fts3/fts3.c:1641: if( rc!=SQLITE_OK ) break;
Suggest that you either use |goto err| everywhere, or |break| everywhere where
you are in a single level of while().  I'm fine either way, just think it should
be consistent.

The length of this section might argue for a slight bias towards |goto err|. 
With this many braces it's sometimes hard to see which loop is being broken out
of.

http://codereview.chromium.org/1638019/diff/37002/49002#newcode1673
third_party/sqlite/ext/fts3/fts3.c:1673: dlrDestroy(&dr1);
I think we earlier debated whether it was worth doing this (since the other code
in here doesn't).  If you really want this, then full pedantic mode requires:
  if( one.nData ) dlrDestroy(&dr1);

http://codereview.chromium.org/1638019/diff/37002/49002#newcode4723
third_party/sqlite/ext/fts3/fts3.c:4723: n +=
fts3GetVarint32Safe(pReader->pData+n, &nSuffix, pReader->nData);
I believe |pReader->pData+n| should be |pReader->pData|, because of the += n on
the previous line.

http://codereview.chromium.org/1638019/diff/37002/49002#newcode5974
third_party/sqlite/ext/fts3/fts3.c:5974: interiorReaderStep(&reader);
You're missing an |rc =|, here (should be like below).

[dumi, 2010-06-03 01:32:07]

http://codereview.chromium.org/1638019/diff/37002/49002
File third_party/sqlite/ext/fts3/fts3.c (right):

http://codereview.chromium.org/1638019/diff/37002/49002#newcode1641
third_party/sqlite/ext/fts3/fts3.c:1641: if( rc!=SQLITE_OK ) break;
On 2010/06/02 19:55:53, shess wrote:
> Suggest that you either use |goto err| everywhere, or |break| everywhere where
> you are in a single level of while().  I'm fine either way, just think it
should
> be consistent.
> 
> The length of this section might argue for a slight bias towards |goto err|. 
> With this many braces it's sometimes hard to see which loop is being broken
out
> of.

replaced all 'break's with 'goto err'.

http://codereview.chromium.org/1638019/diff/37002/49002#newcode1673
third_party/sqlite/ext/fts3/fts3.c:1673: dlrDestroy(&dr1);
On 2010/06/02 19:55:53, shess wrote:
> I think we earlier debated whether it was worth doing this (since the other
code
> in here doesn't).  If you really want this, then full pedantic mode requires:
>   if( one.nData ) dlrDestroy(&dr1);

oops, thought i changed this. done.

http://codereview.chromium.org/1638019/diff/37002/49002#newcode4723
third_party/sqlite/ext/fts3/fts3.c:4723: n +=
fts3GetVarint32Safe(pReader->pData+n, &nSuffix, pReader->nData);
On 2010/06/02 19:55:53, shess wrote:
> I believe |pReader->pData+n| should be |pReader->pData|, because of the += n
on
> the previous line.

fixed!

http://codereview.chromium.org/1638019/diff/37002/49002#newcode5974
third_party/sqlite/ext/fts3/fts3.c:5974: interiorReaderStep(&reader);
On 2010/06/02 19:55:53, shess wrote:
> You're missing an |rc =|, here (should be like below).

done.

[Scott Hess - ex-Googler, 2010-06-03 02:23:08]

LGTM!!!!

OK, so one last thing to verify before check-in (and check in once you're
satisfied).  Have you applied your fts3.patch file against a clean check-out and
verified that it results in the expected fts3.c?  I ask because it looks kind of
like svn diff output, and I do not recall whether patch accepts that directly or
not these days.

[dumi, 2010-06-03 04:46:13]

On 2010/06/03 02:23:08, shess wrote:
> LGTM!!!!
> 
> OK, so one last thing to verify before check-in (and check in once you're
> satisfied).  Have you applied your fts3.patch file against a clean check-out
and
> verified that it results in the expected fts3.c?  I ask because it looks kind
of
> like svn diff output, and I do not recall whether patch accepts that directly
or
> not these days.

'patch' seemed happy with fts3.patch, and after applying the other changes in
the patch to a clean client too, it built without any problems.