You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by hw...@apache.org on 2011/04/12 02:31:00 UTC
svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Author: hwright
Date: Tue Apr 12 00:31:00 2011
New Revision: 1091262
URL: http://svn.apache.org/viewvc?rev=1091262&view=rev
Log:
* subversion/libsvn_wc/wc_db.c
(set_changelist_txn): Special case the with-changelist case, to avoid some
confusion by interleaving if-statements.
Modified:
subversion/trunk/subversion/libsvn_wc/wc_db.c
Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=1091262&r1=1091261&r2=1091262&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
+++ subversion/trunk/subversion/libsvn_wc/wc_db.c Tue Apr 12 00:31:00 2011
@@ -3460,37 +3460,34 @@ set_changelist_txn(void *baton,
svn_relpath_dirname(local_relpath,
scratch_pool)));
}
- else
+ else if (scb->changelists && scb->changelists->nelts)
{
- const char *stmt_text = statements[STMT_UPDATE_ACTUAL_CHANGELIST];
- const char *filter = construct_filter("changelist",
- scb->changelists,
- scratch_pool);
-
- if (*filter)
- stmt_text = apr_pstrcat(scratch_pool, stmt_text, " AND ", filter,
- NULL);
+ int i;
+ const char *stmt_text = apr_pstrcat(scratch_pool,
+ statements[STMT_UPDATE_ACTUAL_CHANGELIST],
+ " AND ",
+ construct_filter("changelist",
+ scb->changelists,
+ scratch_pool),
+ NULL);
SVN_ERR(svn_sqlite__prepare(&stmt, wcroot->sdb, stmt_text,
scratch_pool));
- /* If we have a filter, it means we need to bind the changelist
- params. */
- if (*filter)
+ for (i = 0; i < scb->changelists->nelts; i++)
{
- int i;
+ const char *cl = APR_ARRAY_IDX(scb->changelists, i, const char *);
- for (i = 0; i < scb->changelists->nelts; i++)
- {
- const char *cl = APR_ARRAY_IDX(scb->changelists, i,
- const char *);
-
- /* The magic number '4' here is the number of existing params,
- plus 1, in the statement, which will be bound below. */
- SVN_ERR(svn_sqlite__bind_text(stmt, i+4, cl));
- }
+ /* The magic number '4' here is the number of existing params,
+ plus 1, in the statement, which will be bound below. */
+ SVN_ERR(svn_sqlite__bind_text(stmt, i+4, cl));
}
}
+ else
+ {
+ SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
+ STMT_UPDATE_ACTUAL_CHANGELIST));
+ }
/* Run the update or insert query */
SVN_ERR(svn_sqlite__bindf(stmt, "iss", wcroot->wc_id, local_relpath,
Re: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Greg Stein <gs...@gmail.com>.
When it comes down to it, a single voice *can* veto a technical choice. We
strive very hard to avoid that because of the many anti-social a specs, but
the point still holds true.
I have not seen anything yet that makes me go "oh, that should work great".
Instead, I see a direction in our code that will encourage similar
following, and is being done to remove some code which isn't great, but can
be left for 1.7. I'll take the node walker over 'prepare' any day.
Cheers,
-g
On Apr 12, 2011 9:38 AM, "Mark Phippard" <ma...@gmail.com> wrote:
> On Tue, Apr 12, 2011 at 9:27 AM, Hyrum K Wright <hy...@hyrumwright.org>
wrote:
>
>>> Not sure. Maybe we can work through some ideas. But "we have no other
>>> choice" is not a good enough reason to keep this. That is an even worse
>>> slope to slide down. Doing things simply because they are "convenient".
>>
>> The general consensus seems to be to revert the whole lot of these
>> changes, and I'll bow to those wishes and comply.
>
> I do not think you should just back the changes right out immediately.
> Greg is not the only vote or voice in the project. Just because he
> has come out with a strong opinion on the matter does not mean he is
> right or it is the only answer.
>
> I am not saying Greg is wrong here either, I do not really know. You
> seem like you have made some fairly reasonable counter points to his
> concerns about SQL injection. It seems worthy of a broader discussion
> before changes are made.
>
> Greg has raised some objections. It seems like you have responded to
> them. I would like to see the discussion continue.
>
> --
> Thanks
>
> Mark Phippard
> http://markphip.blogspot.com/
Re: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Mark Phippard <ma...@gmail.com>.
On Tue, Apr 12, 2011 at 9:27 AM, Hyrum K Wright <hy...@hyrumwright.org> wrote:
>> Not sure. Maybe we can work through some ideas. But "we have no other
>> choice" is not a good enough reason to keep this. That is an even worse
>> slope to slide down. Doing things simply because they are "convenient".
>
> The general consensus seems to be to revert the whole lot of these
> changes, and I'll bow to those wishes and comply.
I do not think you should just back the changes right out immediately.
Greg is not the only vote or voice in the project. Just because he
has come out with a strong opinion on the matter does not mean he is
right or it is the only answer.
I am not saying Greg is wrong here either, I do not really know. You
seem like you have made some fairly reasonable counter points to his
concerns about SQL injection. It seems worthy of a broader discussion
before changes are made.
Greg has raised some objections. It seems like you have responded to
them. I would like to see the discussion continue.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Re: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Tue, Apr 12, 2011 at 2:12 AM, Greg Stein <gs...@gmail.com> wrote:
>
> On Apr 11, 2011 10:58 PM, "Hyrum K Wright" <hy...@hyrumwright.org> wrote:
>>
>> On Mon, Apr 11, 2011 at 9:41 PM, Greg Stein <gs...@gmail.com> wrote:
>> > Woah. When did svn_sqlite__prepare arrive?
>>
>> $ svnd blame subversion/libsvn_subr/sqlite.c | grep svn_sqlite__prepare
>> 875453 hwright
>> SVN_ERR(svn_sqlite__prepare(&db->prepared_stmts[stmt_idx], db,
>> 873188 gstein svn_sqlite__prepare(svn_sqlite__stmt_t **stmt,
>> svn_sqlite__db_t *db,
>> 875770 hwright SVN_ERR(svn_sqlite__prepare(&stmt, db, "PRAGMA
>> user_version;", scratch_pool));
>>
>> It looks like you introduced (or resurrected it) in r873188.
>
> Really, Hyrum? Is that the argument here? That I can't disagree with this
> function because I'm in the blame list?
I made no such argument. You asked "When did svn_sqlite__prepare
arrive?" and I answered the question using readily available methods.
> You use the term "resurrect" because you've probably see the log for
> r873188. That function came from an earlier implementation. It got
> (re)introduced by bringing that rev back. Then discussions between yourself
> and me determined that we wanted static text, rather than arbitrary
> preparation, so the current (static) system was devised, and we built it.
>
> So yeah: despite the implication that I should be OK with this... no. I'm
> not.
>
>>
>> > I'm basically -1 on that.
>> >
>> > The whole idea behind static statements was to avoid SQL injection
>> > attacks.
>> > Allowing the *code* to construct statements opens us up.
>> >
>> > This is Not Good.
>>
>> This is still a prepared statement, arguments are still bound using
>> the standard SQLite binding mechanism, so they will be properly
>> escaped.
>
> Arguments, yes. But this edits the statement itself, outside of binding.
>
>> Yes, one can put arbitrary text in the
>> statement-to-be-prepared, but the preparation step helps prevent
>> injection attacks. (You can't prepare multiple statements, which
>> vastly decreases the attack space.)
>
> No. You may be referring to *this* statement. What about the next one which
> is an INSERT or UPDATE? Slippery slope that I disagree with. We spoke about
> this, and I haven't changed my mind. Static text only.
>
>>
>> Plus, we're talking about local data here. While sql injection is a
>> Bad Thing, I suspect somebody wanting to hose a working copy will do
>> something much simpler: rm .svn/wc.db. :)
>
> Thanks for the strawman.
>
> Are you sure that *nothing* will cause an injection? Trojans? Bad server
> input? Inappropriate clicks in a UI?
Bugs in the sqlite library? A compromised compiler? A corporate
lackey intentionally sabotaging their compiled Subversion client? ...
> The static text design was to disable
> all such attacks. I did't even realize we had left the prepare function in
> there, since we had designed all callers to avoid it.
You know as well as I do that we can not possibly prevent all attacks.
We do our best through review and testing. Even in a manually
prepared statement, binding all parameters removes the possibility of
allowing non-escaped values into the query, just as with static text.
We still need to review all queries, even the static ones in
wc-queries.sql. Those can fall victim to programmer error just as
well as dynamically generated queries. (And since all external input
is parameterized and bound, both are protected from injection
attacks.)
>> On a higher-level, if we *don't* use this API, what are your
>> suggestions on how to create a IN clause with arbitrary numbers of
>> values?
>
> Not sure. Maybe we can work through some ideas. But "we have no other
> choice" is not a good enough reason to keep this. That is an even worse
> slope to slide down. Doing things simply because they are "convenient".
The general consensus seems to be to revert the whole lot of these
changes, and I'll bow to those wishes and comply.
On a more general note, this is the second time in as many weeks I've
read "don't do that" in reviews. It's a good learning experience to
know that others don't consider an approach feasible. It would be
much more productive, however, if alternate ideas were presented
instead of playing "bring me a rock".
-Hyrum
Re: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Greg Stein <gs...@gmail.com>.
On Apr 11, 2011 10:58 PM, "Hyrum K Wright" <hy...@hyrumwright.org> wrote:
>
> On Mon, Apr 11, 2011 at 9:41 PM, Greg Stein <gs...@gmail.com> wrote:
> > Woah. When did svn_sqlite__prepare arrive?
>
> $ svnd blame subversion/libsvn_subr/sqlite.c | grep svn_sqlite__prepare
> 875453 hwright
> SVN_ERR(svn_sqlite__prepare(&db->prepared_stmts[stmt_idx], db,
> 873188 gstein svn_sqlite__prepare(svn_sqlite__stmt_t **stmt,
> svn_sqlite__db_t *db,
> 875770 hwright SVN_ERR(svn_sqlite__prepare(&stmt, db, "PRAGMA
> user_version;", scratch_pool));
>
> It looks like you introduced (or resurrected it) in r873188.
Really, Hyrum? Is that the argument here? That I can't disagree with this
function because I'm in the blame list?
You use the term "resurrect" because you've probably see the log for
r873188. That function came from an earlier implementation. It got
(re)introduced by bringing that rev back. Then discussions between yourself
and me determined that we wanted static text, rather than arbitrary
preparation, so the current (static) system was devised, and we built it.
So yeah: despite the implication that I should be OK with this... no. I'm
not.
>
> > I'm basically -1 on that.
> >
> > The whole idea behind static statements was to avoid SQL injection
attacks.
> > Allowing the *code* to construct statements opens us up.
> >
> > This is Not Good.
>
> This is still a prepared statement, arguments are still bound using
> the standard SQLite binding mechanism, so they will be properly
> escaped.
Arguments, yes. But this edits the statement itself, outside of binding.
> Yes, one can put arbitrary text in the
> statement-to-be-prepared, but the preparation step helps prevent
> injection attacks. (You can't prepare multiple statements, which
> vastly decreases the attack space.)
No. You may be referring to *this* statement. What about the next one which
is an INSERT or UPDATE? Slippery slope that I disagree with. We spoke about
this, and I haven't changed my mind. Static text only.
>
> Plus, we're talking about local data here. While sql injection is a
> Bad Thing, I suspect somebody wanting to hose a working copy will do
> something much simpler: rm .svn/wc.db. :)
Thanks for the strawman.
Are you sure that *nothing* will cause an injection? Trojans? Bad server
input? Inappropriate clicks in a UI? The static text design was to disable
all such attacks. I did't even realize we had left the prepare function in
there, since we had designed all callers to avoid it.
>
> On a higher-level, if we *don't* use this API, what are your
> suggestions on how to create a IN clause with arbitrary numbers of
> values?
Not sure. Maybe we can work through some ideas. But "we have no other
choice" is not a good enough reason to keep this. That is an even worse
slope to slide down. Doing things simply because they are "convenient".
Cheers,
-g
Re: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Mon, Apr 11, 2011 at 9:41 PM, Greg Stein <gs...@gmail.com> wrote:
> Woah. When did svn_sqlite__prepare arrive?
$ svnd blame subversion/libsvn_subr/sqlite.c | grep svn_sqlite__prepare
875453 hwright
SVN_ERR(svn_sqlite__prepare(&db->prepared_stmts[stmt_idx], db,
873188 gstein svn_sqlite__prepare(svn_sqlite__stmt_t **stmt,
svn_sqlite__db_t *db,
875770 hwright SVN_ERR(svn_sqlite__prepare(&stmt, db, "PRAGMA
user_version;", scratch_pool));
It looks like you introduced (or resurrected it) in r873188.
> I'm basically -1 on that.
>
> The whole idea behind static statements was to avoid SQL injection attacks.
> Allowing the *code* to construct statements opens us up.
>
> This is Not Good.
This is still a prepared statement, arguments are still bound using
the standard SQLite binding mechanism, so they will be properly
escaped. Yes, one can put arbitrary text in the
statement-to-be-prepared, but the preparation step helps prevent
injection attacks. (You can't prepare multiple statements, which
vastly decreases the attack space.)
Plus, we're talking about local data here. While sql injection is a
Bad Thing, I suspect somebody wanting to hose a working copy will do
something much simpler: rm .svn/wc.db. :)
On a higher-level, if we *don't* use this API, what are your
suggestions on how to create a IN clause with arbitrary numbers of
values?
-Hyrum
Re: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 04/12/2011 09:14 AM, Hyrum Wright wrote:
> I'll revert this work sometime today.
Just in case it wasn't clear: I'm not suggesting that you unconditionally
revert. Others in the thread are able to evaluate the technical merits of
the approach you took in ways that currently I cannot. I'm really just
trying to argue for your freedom to defer altogether if those others agree
on "don't do it this way" and we all agree that it's okay for the node
walker -- and this specific use thereof -- to survive the 1.7.0 cut.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Johan Corveleyn <jc...@gmail.com>.
On Tue, Apr 12, 2011 at 3:40 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Johan Corveleyn [mailto:jcorvel@gmail.com]
>> Sent: dinsdag 12 april 2011 15:27
>> To: Hyrum Wright
>> Cc: C. Michael Pilato; Bert Huijben; Greg Stein; dev@subversion.apache.org
>> Subject: Re: svn commit: r1091262 -
>> /subversion/trunk/subversion/libsvn_wc/wc_db.c
>
>
>> Changelists in IDE's are used a lot, for separating work. If you do a
>> major refactoring, touching say 2000 files, and you want those to be
>> part of some changelist ... Also, when we do a sync-merge of a feature
>> branch, we usually let those merge-changes appear in a changelist
>> (makes it easy to commit with a nicely prepared commit message). If
>> those syncs are more than a week apart, the amount of changed files
>> can easily go to 2000-3000.
>>
>> I can't comment on the technical merits/problems of adapting
>> changelists for more optimal wc-ng use (I get the impression that the
>> effort currently outweighs the gains), but just wanted to add my 0.02
>> €: adding thousands of files to a changelist is not unusual when
>> working from an IDE.
>
> I use the same system in AnkhSVN, but then again when I change the changelists in Subversion I don't use any of the recursive operations: I apply the changelist changes as a batch of paths.
> (It is very expensive to calculate whether I could use a recursive set on a path and everything below).
>
> For my use case optimizing for batch processing is more important then for depth processing. (The current code optimizes for depth processing)
Ah ok, maybe IDEA (or rather its svn plugin, which uses svnkit) does
the same. I really have no clue what it does under the covers :-).
--
Johan
RE: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Johan Corveleyn [mailto:jcorvel@gmail.com]
> Sent: dinsdag 12 april 2011 15:27
> To: Hyrum Wright
> Cc: C. Michael Pilato; Bert Huijben; Greg Stein; dev@subversion.apache.org
> Subject: Re: svn commit: r1091262 -
> /subversion/trunk/subversion/libsvn_wc/wc_db.c
> Changelists in IDE's are used a lot, for separating work. If you do a
> major refactoring, touching say 2000 files, and you want those to be
> part of some changelist ... Also, when we do a sync-merge of a feature
> branch, we usually let those merge-changes appear in a changelist
> (makes it easy to commit with a nicely prepared commit message). If
> those syncs are more than a week apart, the amount of changed files
> can easily go to 2000-3000.
>
> I can't comment on the technical merits/problems of adapting
> changelists for more optimal wc-ng use (I get the impression that the
> effort currently outweighs the gains), but just wanted to add my 0.02
> €: adding thousands of files to a changelist is not unusual when
> working from an IDE.
I use the same system in AnkhSVN, but then again when I change the changelists in Subversion I don't use any of the recursive operations: I apply the changelist changes as a batch of paths.
(It is very expensive to calculate whether I could use a recursive set on a path and everything below).
For my use case optimizing for batch processing is more important then for depth processing. (The current code optimizes for depth processing)
And as we don't have proper changelist support on directories yet, some operations ignore directories when you use changelists (Diff & revert work this way if I remember correctly), while others apply the same change to all directories within the original filter (commit as a notable example).
Bert
Re: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Johan Corveleyn <jc...@gmail.com>.
On Tue, Apr 12, 2011 at 3:14 PM, Hyrum Wright <hw...@apache.org> wrote:
> On Tue, Apr 12, 2011 at 8:00 AM, C. Michael Pilato <cm...@collab.net> wrote:
>>> You are looking at changelists as a way to learn how to move operations into
>>> wc_db properly, but just like that temp table for notifications I don't see
>>> this as the way to go forward.
>>>
>>> I really don't see why users want to add thousands of nodes to changelists
>>> while we still don't support changelists on directories. And if it is just a
>>> handful of nodes the old code worked fine.
>>
>> This was one of the wrestling matches that I had with myself when I started
>> looking at this very bit of code that Hyrum has changed. As I *understood*
>> it, we had an internal goal of losing the svn_wc__node_walk_children().
>> "It's slow." But in some cases -- namely this one -- it just seemed like
>> doing so would require adding obnoxious or otherwise unpleasant code.
>>
>> Changelist operations are, I would suspect, pretty rare, so if folks don't
>> like the approach Hyrum has taken, I would suggest that he just revert the
>> whole of his effort in this space, delete notes/wc_node_walkers.txt, add a
>> note to the svn_wc__node_walk_children() docstring encouraging developers to
>> consider using a more batch-based approach if possible when considering
>> additional uses of the function, and then move on. If we're going to spin
>> our wheels somewhere, let's not do it on our arguably lesser-used features,
>> please.
>
> The point of this entire exercise was not to make setting changelists
> faster (though that is a nifty side effect), but rather to get more
> insight into how we are going to deal with them when doing this type
> of thing for use cases that do matter, such as recursive propset.
> We've got changelists all over the code, and since the database can do
> changelist filtering for us, I presumed that learning how to use that
> capability would be a fruitful use of time. I guess the insight is:
> "we can't do it using current methods."
>
> I'll revert this work sometime today.
I agree it's not the most important operation that needs optimization,
but I'd like to add some weight to the other side of the balance: I
think it's more than just an exercise. I think it can provide
performance benifits (or avoid performance regressions) for real
use-cases. Maybe not for command-line usage, but definitely for
integration with IDE's or other UI-integrations.
The IDE we use at work, IntelliJ IDEA, has it's own changelist concept
in the UI, but synchronizes that with "real changelists" of svn. It
does this in the background: you can see (modified) files becoming
part in the UI pretty quickly, but if you run 'svn st' on the command
line, only part of them are in the svn-changelist. After a while, the
svn-changelist has caught up.
Changelists in IDE's are used a lot, for separating work. If you do a
major refactoring, touching say 2000 files, and you want those to be
part of some changelist ... Also, when we do a sync-merge of a feature
branch, we usually let those merge-changes appear in a changelist
(makes it easy to commit with a nicely prepared commit message). If
those syncs are more than a week apart, the amount of changed files
can easily go to 2000-3000.
I can't comment on the technical merits/problems of adapting
changelists for more optimal wc-ng use (I get the impression that the
effort currently outweighs the gains), but just wanted to add my 0.02
€: adding thousands of files to a changelist is not unusual when
working from an IDE.
Cheers,
--
Johan
Re: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Hyrum Wright <hw...@apache.org>.
On Tue, Apr 12, 2011 at 8:00 AM, C. Michael Pilato <cm...@collab.net> wrote:
>> You are looking at changelists as a way to learn how to move operations into
>> wc_db properly, but just like that temp table for notifications I don't see
>> this as the way to go forward.
>>
>> I really don't see why users want to add thousands of nodes to changelists
>> while we still don't support changelists on directories. And if it is just a
>> handful of nodes the old code worked fine.
>
> This was one of the wrestling matches that I had with myself when I started
> looking at this very bit of code that Hyrum has changed. As I *understood*
> it, we had an internal goal of losing the svn_wc__node_walk_children().
> "It's slow." But in some cases -- namely this one -- it just seemed like
> doing so would require adding obnoxious or otherwise unpleasant code.
>
> Changelist operations are, I would suspect, pretty rare, so if folks don't
> like the approach Hyrum has taken, I would suggest that he just revert the
> whole of his effort in this space, delete notes/wc_node_walkers.txt, add a
> note to the svn_wc__node_walk_children() docstring encouraging developers to
> consider using a more batch-based approach if possible when considering
> additional uses of the function, and then move on. If we're going to spin
> our wheels somewhere, let's not do it on our arguably lesser-used features,
> please.
The point of this entire exercise was not to make setting changelists
faster (though that is a nifty side effect), but rather to get more
insight into how we are going to deal with them when doing this type
of thing for use cases that do matter, such as recursive propset.
We've got changelists all over the code, and since the database can do
changelist filtering for us, I presumed that learning how to use that
capability would be a fruitful use of time. I guess the insight is:
"we can't do it using current methods."
I'll revert this work sometime today.
-Hyrum
Re: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
> You are looking at changelists as a way to learn how to move operations into
> wc_db properly, but just like that temp table for notifications I don't see
> this as the way to go forward.
>
> I really don't see why users want to add thousands of nodes to changelists
> while we still don't support changelists on directories. And if it is just a
> handful of nodes the old code worked fine.
This was one of the wrestling matches that I had with myself when I started
looking at this very bit of code that Hyrum has changed. As I *understood*
it, we had an internal goal of losing the svn_wc__node_walk_children().
"It's slow." But in some cases -- namely this one -- it just seemed like
doing so would require adding obnoxious or otherwise unpleasant code.
Changelist operations are, I would suspect, pretty rare, so if folks don't
like the approach Hyrum has taken, I would suggest that he just revert the
whole of his effort in this space, delete notes/wc_node_walkers.txt, add a
note to the svn_wc__node_walk_children() docstring encouraging developers to
consider using a more batch-based approach if possible when considering
additional uses of the function, and then move on. If we're going to spin
our wheels somewhere, let's not do it on our arguably lesser-used features,
please.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
RE: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Greg Stein [mailto:gstein@gmail.com]
> Sent: dinsdag 12 april 2011 4:41
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1091262 -
> /subversion/trunk/subversion/libsvn_wc/wc_db.c
>
> Woah. When did svn_sqlite__prepare arrive?
>
> I'm basically -1 on that.
>
> The whole idea behind static statements was to avoid SQL injection
attacks.
> Allowing the *code* to construct statements opens us up.
+1 on that '-1'
You are looking at changelists as a way to learn how to move operations into
wc_db properly, but just like that temp table for notifications I don't see
this as the way to go forward.
I really don't see why users want to add thousands of nodes to changelists
while we still don't support changelists on directories. And if it is just a
handful of nodes the old code worked fine.
Once we start building svn stash, etc. things will be different... But that
is way out of scope for 1.7.
I think this time can be spend in a better way by documenting how
changelists should behave on directories and then enabling that. That would
be a truly useful feature.
(And it should be a lot easier than in 1.5 when we didn't have an op_root
concept..)
Bert
Re: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Greg Stein <gs...@gmail.com>.
The function has existed for a long time, but remained unused, AFAIK. I
don't think it should be, and it should be swutched to file-private.
On Apr 11, 2011 10:41 PM, "Greg Stein" <gs...@gmail.com> wrote:
> Woah. When did svn_sqlite__prepare arrive?
>
> I'm basically -1 on that.
>
> The whole idea behind static statements was to avoid SQL injection
attacks.
> Allowing the *code* to construct statements opens us up.
>
> This is Not Good.
> On Apr 11, 2011 8:31 PM, <hw...@apache.org> wrote:
>> Author: hwright
>> Date: Tue Apr 12 00:31:00 2011
>> New Revision: 1091262
>>
>> URL: http://svn.apache.org/viewvc?rev=1091262&view=rev
>> Log:
>> * subversion/libsvn_wc/wc_db.c
>> (set_changelist_txn): Special case the with-changelist case, to avoid
some
>> confusion by interleaving if-statements.
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_wc/wc_db.c
>>
>> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
>> URL:
>
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=1091262&r1=1091261&r2=1091262&view=diff
>>
>
==============================================================================
>> --- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
>> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Tue Apr 12 00:31:00
2011
>> @@ -3460,37 +3460,34 @@ set_changelist_txn(void *baton,
>> svn_relpath_dirname(local_relpath,
>> scratch_pool)));
>> }
>> - else
>> + else if (scb->changelists && scb->changelists->nelts)
>> {
>> - const char *stmt_text = statements[STMT_UPDATE_ACTUAL_CHANGELIST];
>> - const char *filter = construct_filter("changelist",
>> - scb->changelists,
>> - scratch_pool);
>> -
>> - if (*filter)
>> - stmt_text = apr_pstrcat(scratch_pool, stmt_text, " AND ", filter,
>> - NULL);
>> + int i;
>> + const char *stmt_text = apr_pstrcat(scratch_pool,
>> + statements[STMT_UPDATE_ACTUAL_CHANGELIST],
>> + " AND ",
>> + construct_filter("changelist",
>> + scb->changelists,
>> + scratch_pool),
>> + NULL);
>>
>> SVN_ERR(svn_sqlite__prepare(&stmt, wcroot->sdb, stmt_text,
>> scratch_pool));
>>
>> - /* If we have a filter, it means we need to bind the changelist
>> - params. */
>> - if (*filter)
>> + for (i = 0; i < scb->changelists->nelts; i++)
>> {
>> - int i;
>> + const char *cl = APR_ARRAY_IDX(scb->changelists, i, const char *);
>>
>> - for (i = 0; i < scb->changelists->nelts; i++)
>> - {
>> - const char *cl = APR_ARRAY_IDX(scb->changelists, i,
>> - const char *);
>> -
>> - /* The magic number '4' here is the number of existing params,
>> - plus 1, in the statement, which will be bound below. */
>> - SVN_ERR(svn_sqlite__bind_text(stmt, i+4, cl));
>> - }
>> + /* The magic number '4' here is the number of existing params,
>> + plus 1, in the statement, which will be bound below. */
>> + SVN_ERR(svn_sqlite__bind_text(stmt, i+4, cl));
>> }
>> }
>> + else
>> + {
>> + SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
>> + STMT_UPDATE_ACTUAL_CHANGELIST));
>> + }
>>
>> /* Run the update or insert query */
>> SVN_ERR(svn_sqlite__bindf(stmt, "iss", wcroot->wc_id, local_relpath,
>>
>>
Re: svn commit: r1091262 - /subversion/trunk/subversion/libsvn_wc/wc_db.c
Posted by Greg Stein <gs...@gmail.com>.
Woah. When did svn_sqlite__prepare arrive?
I'm basically -1 on that.
The whole idea behind static statements was to avoid SQL injection attacks.
Allowing the *code* to construct statements opens us up.
This is Not Good.
On Apr 11, 2011 8:31 PM, <hw...@apache.org> wrote:
> Author: hwright
> Date: Tue Apr 12 00:31:00 2011
> New Revision: 1091262
>
> URL: http://svn.apache.org/viewvc?rev=1091262&view=rev
> Log:
> * subversion/libsvn_wc/wc_db.c
> (set_changelist_txn): Special case the with-changelist case, to avoid some
> confusion by interleaving if-statements.
>
> Modified:
> subversion/trunk/subversion/libsvn_wc/wc_db.c
>
> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
> URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=1091262&r1=1091261&r2=1091262&view=diff
>
==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Tue Apr 12 00:31:00 2011
> @@ -3460,37 +3460,34 @@ set_changelist_txn(void *baton,
> svn_relpath_dirname(local_relpath,
> scratch_pool)));
> }
> - else
> + else if (scb->changelists && scb->changelists->nelts)
> {
> - const char *stmt_text = statements[STMT_UPDATE_ACTUAL_CHANGELIST];
> - const char *filter = construct_filter("changelist",
> - scb->changelists,
> - scratch_pool);
> -
> - if (*filter)
> - stmt_text = apr_pstrcat(scratch_pool, stmt_text, " AND ", filter,
> - NULL);
> + int i;
> + const char *stmt_text = apr_pstrcat(scratch_pool,
> + statements[STMT_UPDATE_ACTUAL_CHANGELIST],
> + " AND ",
> + construct_filter("changelist",
> + scb->changelists,
> + scratch_pool),
> + NULL);
>
> SVN_ERR(svn_sqlite__prepare(&stmt, wcroot->sdb, stmt_text,
> scratch_pool));
>
> - /* If we have a filter, it means we need to bind the changelist
> - params. */
> - if (*filter)
> + for (i = 0; i < scb->changelists->nelts; i++)
> {
> - int i;
> + const char *cl = APR_ARRAY_IDX(scb->changelists, i, const char *);
>
> - for (i = 0; i < scb->changelists->nelts; i++)
> - {
> - const char *cl = APR_ARRAY_IDX(scb->changelists, i,
> - const char *);
> -
> - /* The magic number '4' here is the number of existing params,
> - plus 1, in the statement, which will be bound below. */
> - SVN_ERR(svn_sqlite__bind_text(stmt, i+4, cl));
> - }
> + /* The magic number '4' here is the number of existing params,
> + plus 1, in the statement, which will be bound below. */
> + SVN_ERR(svn_sqlite__bind_text(stmt, i+4, cl));
> }
> }
> + else
> + {
> + SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
> + STMT_UPDATE_ACTUAL_CHANGELIST));
> + }
>
> /* Run the update or insert query */
> SVN_ERR(svn_sqlite__bindf(stmt, "iss", wcroot->wc_id, local_relpath,
>
>