Re: Enabling symmetric merge, and UI details

[Julian Foad]

Neels J Hofmeyr wrote:

> On 2012-08-03 21:25, Julian Foad wrote:
>>  It's time to lose the temporary '--symmetric' option and make 
>> it the default mode in 'svn'.
> 
> sounds fantastic :)
> 
>> .
> 
> A good read, but I can't find that "[5]" reference:
> "...we now have a better model of what merge tracking means [5]."

Ah...  In my head I have a better model of what merge tracking means.  I don't 
really have a good write-up to refer to.  This section 

 and this section 
 are the 
closest I have.

- Julian


> ~Neels
> 
> 
>

Re: Enabling symmetric merge, and UI details

[Neels J Hofmeyr]

On 2012-08-03 21:25, Julian Foad wrote:
> It's time to lose the temporary '--symmetric' option and make it the default 
> mode in 'svn'.

sounds fantastic :)

> .

A good read, but I can't find that "[5]" reference:
"...we now have a better model of what merge tracking means [5]."

~Neels





signature.asc
Description: OpenPGP digital signature

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Johan Corveleyn]

On Tue, Aug 21, 2012 at 11:22 PM, Branko Čibej  wrote:
> On 21.08.2012 22:28, Johan Corveleyn wrote:
>> On Tue, Aug 21, 2012 at 9:52 PM, Mark Phippard  wrote:
>>> On Tue, Aug 21, 2012 at 3:46 PM, Branko Čibej  wrote:
 On 21.08.2012 21:37, Mark Phippard wrote:
>> ...
> People could do a lot with that, and if
> it is easier to attach the client version to the txn properties, then
> that is another good reason.
>
> IMO, we have had a LOT of users@ requests for the client version in
> hook scripts.
 Yeah, well, if these requests can also define what the client version
 actually is, we can start thinking about a solution. In the meantime,
 I'd prefer this information to not be mixed with revision properties;
 not least because I can't think of a way of preventing older servers
 from just writing such props into the repository.
>>> Mike would have to answer that, but ISTR that the client/server
>>> negotiation kept the client from sending this information.  So older
>>> servers would not get the props.
>> Actually, as an svn admin, I wouldn't mind having the version revprops
>> written to the repository. That can be very interesting information,
>> especially if you're diagnosing some kind of problem, trying to find a
>> pattern, pinpointing it to particular clients, ...
>>
>> Last year I had a couple of situations where I really wanted to find
>> out what client (and client-version) had performed a particular
>> commit, or trying to find a common pattern between various occurrences
>> of strangeness (mostly related to issue #4065 [1], which was first
>> "broken" by some git-svn clients, and then later on by some versions
>> of svnkit).
>>
>> But that might be a rather exceptional use case, so not sure if it's
>> worth it ...
>
> That sort of thing belongs in server logs, not in the repository.

Ah, true, that would be even better.

-- 
Johan

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Branko Čibej]

On 21.08.2012 22:28, Johan Corveleyn wrote:
> On Tue, Aug 21, 2012 at 9:52 PM, Mark Phippard  wrote:
>> On Tue, Aug 21, 2012 at 3:46 PM, Branko Čibej  wrote:
>>> On 21.08.2012 21:37, Mark Phippard wrote:
> ...
 People could do a lot with that, and if
 it is easier to attach the client version to the txn properties, then
 that is another good reason.

 IMO, we have had a LOT of users@ requests for the client version in
 hook scripts.
>>> Yeah, well, if these requests can also define what the client version
>>> actually is, we can start thinking about a solution. In the meantime,
>>> I'd prefer this information to not be mixed with revision properties;
>>> not least because I can't think of a way of preventing older servers
>>> from just writing such props into the repository.
>> Mike would have to answer that, but ISTR that the client/server
>> negotiation kept the client from sending this information.  So older
>> servers would not get the props.
> Actually, as an svn admin, I wouldn't mind having the version revprops
> written to the repository. That can be very interesting information,
> especially if you're diagnosing some kind of problem, trying to find a
> pattern, pinpointing it to particular clients, ...
>
> Last year I had a couple of situations where I really wanted to find
> out what client (and client-version) had performed a particular
> commit, or trying to find a common pattern between various occurrences
> of strangeness (mostly related to issue #4065 [1], which was first
> "broken" by some git-svn clients, and then later on by some versions
> of svnkit).
>
> But that might be a rather exceptional use case, so not sure if it's
> worth it ...

That sort of thing belongs in server logs, not in the repository.

-- Brane

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: FS vtable common_pool and pack_fs/recover

[Daniel Shahaf]

Philip Martin wrote on Tue, Aug 21, 2012 at 20:42:55 +0100:
> Daniel Shahaf  writes:
> 
> > Philip Martin wrote on Tue, Aug 21, 2012 at 18:00:23 +0100:
> >> Functions like fs_library_vtable_t.pack_fs and
> >> fs_library_vtable_t.recover that don't take the common_pool parameter
> >> pass the pool parameter to fs_serialized_init.  I think that means these
> >> functions should only be used from a single threaded process.
> >> 
> >> Is that correct?  If so we should document it or change the functions to
> >> handle the common_pool.
> >> 
> >
> > I think the correct fix is to propagate common_pool to have fs_pack()
> > and pass it as to the appropriate parameter of fs_serialized_init().
> 
> Yes, I think that would be correct.  It's probably possible to backport
> to 1.7 since these functions are all private.
> 

+1

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Johan Corveleyn]

On Tue, Aug 21, 2012 at 9:52 PM, Mark Phippard  wrote:
> On Tue, Aug 21, 2012 at 3:46 PM, Branko Čibej  wrote:
>> On 21.08.2012 21:37, Mark Phippard wrote:
...
>>> People could do a lot with that, and if
>>> it is easier to attach the client version to the txn properties, then
>>> that is another good reason.
>>>
>>> IMO, we have had a LOT of users@ requests for the client version in
>>> hook scripts.
>>
>> Yeah, well, if these requests can also define what the client version
>> actually is, we can start thinking about a solution. In the meantime,
>> I'd prefer this information to not be mixed with revision properties;
>> not least because I can't think of a way of preventing older servers
>> from just writing such props into the repository.
>
> Mike would have to answer that, but ISTR that the client/server
> negotiation kept the client from sending this information.  So older
> servers would not get the props.

Actually, as an svn admin, I wouldn't mind having the version revprops
written to the repository. That can be very interesting information,
especially if you're diagnosing some kind of problem, trying to find a
pattern, pinpointing it to particular clients, ...

Last year I had a couple of situations where I really wanted to find
out what client (and client-version) had performed a particular
commit, or trying to find a common pattern between various occurrences
of strangeness (mostly related to issue #4065 [1], which was first
"broken" by some git-svn clients, and then later on by some versions
of svnkit).

But that might be a rather exceptional use case, so not sure if it's
worth it ...

[1] http://subversion.tigris.org/issues/show_bug.cgi?id=4065 (server
should enforce LF normalization for svn:eol-style=native files)

-- 
Johan

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[C. Michael Pilato]

On 08/21/2012 03:41 PM, Branko Čibej wrote:
> Seriously though: How will the presence of revprops affect the decision
> to reject a commit? If based on svn:author, we already have a better way
> in place. What kind of other questions are relevant?

The one that seems most common is to reject commits whose log messages don't
conform to some ruleset.  Since we lack a log message template feature, we
advise folks to simply bounce non-conforming commits with the log template
embedded in the bounce message.  Subversion is also integrated into workflow
systems that may demand that an issue tracker ID be associated with every
commit -- again, something that can't be checked today until the whole
commit crosses the wire.  Some releases ago (1.5?) we made extra effort to
allow folks to attach custom revprops to their commits as part of the
process (rather than as a follow-up, post-commit thing): any validation of
those things must necessarily wait until pre-commit today as well.

We have all kinds of just-in-time checks that are part of our commit
process:  just-in-time out-of-dateness checks (this is why we do postfix
text deltas), just-in-time lock token checks, etc.  It seems like a
deficiency that we have no just-in-time metadata checks, too.  And even
though this is isn't part of the codified roadmap yet, it seems reasonable
to look to the future where we've discussed having clients pre-announce the
to-be-modified files before sending even the first bit of real commit data
across the wire and realize that we'd want a hook in place that could do
something with that information to, again, prevent unnecessary additional
wire transfer.

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development



signature.asc
Description: OpenPGP digital signature

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Mark Phippard]

On Tue, Aug 21, 2012 at 3:46 PM, Branko Čibej  wrote:
> On 21.08.2012 21:37, Mark Phippard wrote:
>> On Tue, Aug 21, 2012 at 3:29 PM, C. Michael Pilato  
>> wrote:

>> FWIW, I agree it would be nice to have access to svn:log before the
>> data is sent from the client.
>
> I expect this would mean checking for log-file format conformance?

Yes.  Users use the log file for lots of things that they might
validate in the pre-commit hook today.  Could be conforming to some
kind of template or format, but it often is also something like
requiring a link to a bug ID, or some kind of "signed-off" by line
etc.

>> People could do a lot with that, and if
>> it is easier to attach the client version to the txn properties, then
>> that is another good reason.
>>
>> IMO, we have had a LOT of users@ requests for the client version in
>> hook scripts.
>
> Yeah, well, if these requests can also define what the client version
> actually is, we can start thinking about a solution. In the meantime,
> I'd prefer this information to not be mixed with revision properties;
> not least because I can't think of a way of preventing older servers
> from just writing such props into the repository.

Mike would have to answer that, but ISTR that the client/server
negotiation kept the client from sending this information.  So older
servers would not get the props.

-- 
Thanks

Mark Phippard
http://markphip.blogspot.com/

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Mark Phippard]

On Tue, Aug 21, 2012 at 3:41 PM, Branko Čibej  wrote:

> Seriously though: How will the presence of revprops affect the decision
> to reject a commit? If based on svn:author, we already have a better way
> in place. What kind of other questions are relevant?

My understanding is that Mike proposed properties because he could not
figure out a good way to jam the client version into the capabilities
strings.  I think this led him to remember that people have often
wished they could reject a commit based on svn:log BEFORE the client
wasted time sending all the content to the server.

FWIW, I basically agree with Mike that these are both high-value,
low-cost features that users have consistently asked for.

-- 
Thanks

Mark Phippard
http://markphip.blogspot.com/

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Branko Čibej]

On 21.08.2012 21:37, Mark Phippard wrote:
> On Tue, Aug 21, 2012 at 3:29 PM, C. Michael Pilato  
> wrote:
>
>> If the community decides to simply modify start-commit to run *after* the
>> txn gets created and populated with txnprops, that's fine with me.  I
>> assumed that that change was undesirable due to the fact mentioned
>> elsethread about how start-commit would no longer be useful for keeping a
>> would-be read-only repository read-only.
> start-commit does not currently receive the txn-id, so I do not see
> how we can use/move it for svn:log access anyway.  How about a single
> new hook called something like start-commit-txn or something like
> that?
>
> FWIW, I agree it would be nice to have access to svn:log before the
> data is sent from the client.

I expect this would mean checking for log-file format conformance?

> People could do a lot with that, and if
> it is easier to attach the client version to the txn properties, then
> that is another good reason.
>
> IMO, we have had a LOT of users@ requests for the client version in
> hook scripts.

Yeah, well, if these requests can also define what the client version
actually is, we can start thinking about a solution. In the meantime,
I'd prefer this information to not be mixed with revision properties;
not least because I can't think of a way of preventing older servers
from just writing such props into the repository.

-- Brane

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[C. Michael Pilato]

On 08/21/2012 03:37 PM, Mark Phippard wrote:
> On Tue, Aug 21, 2012 at 3:29 PM, C. Michael Pilato  
> wrote:
> 
>> If the community decides to simply modify start-commit to run *after* the
>> txn gets created and populated with txnprops, that's fine with me.  I
>> assumed that that change was undesirable due to the fact mentioned
>> elsethread about how start-commit would no longer be useful for keeping a
>> would-be read-only repository read-only.
> 
> start-commit does not currently receive the txn-id, so I do not see
> how we can use/move it for svn:log access anyway.  How about a single
> new hook called something like start-commit-txn or something like
> that?

We've added parameters to the end of a hook before (ACTION was added to
post-revprop-change in Subversion 1.2.0), so I figured we'd just tack txn-id
onto the end of the current start-txn argument list.  Existing scripts won't
care about the extra arg; new or updated scripts can use it to do the
svn:log checks.

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development



signature.asc
Description: OpenPGP digital signature

Re: FS vtable common_pool and pack_fs/recover

[Philip Martin]

Daniel Shahaf  writes:

> Philip Martin wrote on Tue, Aug 21, 2012 at 18:00:23 +0100:
>> Functions like fs_library_vtable_t.pack_fs and
>> fs_library_vtable_t.recover that don't take the common_pool parameter
>> pass the pool parameter to fs_serialized_init.  I think that means these
>> functions should only be used from a single threaded process.
>> 
>> Is that correct?  If so we should document it or change the functions to
>> handle the common_pool.
>> 
>
> I think the correct fix is to propagate common_pool to have fs_pack()
> and pass it as to the appropriate parameter of fs_serialized_init().

Yes, I think that would be correct.  It's probably possible to backport
to 1.7 since these functions are all private.

>> I'm not sure how the absence of common_pool affects the BDB
>> implementations.
>
> Is it even used?  A quick grep in libsvn_fs_base suggests that only fs.c
> has variables by that name; therein, only svn_fs_base__init() uses them;
> and that function seems to have no callers(!?).

It's called by the FS dynamic loader via a function pointer when
get_library_vtable_direct calls initfunc.

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Branko Čibej]

On 21.08.2012 21:29, C. Michael Pilato wrote:
> On 08/21/2012 03:14 PM, Branko Čibej wrote:
>> On 21.08.2012 21:07, Mark Phippard wrote:
>>> I am also not crazy about introducing a bunch of new hooks that are
>>> all more or less what the original start-commit hook was doing.
>> Have to agree here. I'm a bit worried that we're trying to introduce
>> features too specific to what some vendor's client requests. This
>> started off with finding ways to communicate the client version to the
>> server (doubtful, but marginally acceptable) and suddenly we're talking
>> about a new hook which, among other things, would set in stone the
>> process that the server must follow during a commit.
>>
>> I'm not at all sure that enough thought has been put into the idea.
> While it is the case that client-version stuff stirred my creativity towards
> this change, I want to be clear that I didn't actually decide to make the
> change for that reason.  I made the change to introduce the hook because
> when I mentioned the possibility of blocking a commit destined to fail
> before a gig of data was transmitted across the wire, I got feedback from
> several folks who were singing the praises of the idea.

Ah well, people are going to sing praises to all sorts of things.

> for the record, I
> still am not sure that the client-version stuff is best approached using
> txnprops.  (But thanks anyway for the public implication that I'm merely a
> hive-mind hacker.)

Heh. I know I've been guilty of that kind of thinking every so often, so
welcome to the club. :)

Seriously though: How will the presence of revprops affect the decision
to reject a commit? If based on svn:author, we already have a better way
in place. What kind of other questions are relevant?

-- Brane

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Mark Phippard]

On Tue, Aug 21, 2012 at 3:29 PM, C. Michael Pilato  wrote:

> If the community decides to simply modify start-commit to run *after* the
> txn gets created and populated with txnprops, that's fine with me.  I
> assumed that that change was undesirable due to the fact mentioned
> elsethread about how start-commit would no longer be useful for keeping a
> would-be read-only repository read-only.

start-commit does not currently receive the txn-id, so I do not see
how we can use/move it for svn:log access anyway.  How about a single
new hook called something like start-commit-txn or something like
that?

FWIW, I agree it would be nice to have access to svn:log before the
data is sent from the client.  People could do a lot with that, and if
it is easier to attach the client version to the txn properties, then
that is another good reason.

IMO, we have had a LOT of users@ requests for the client version in
hook scripts.

-- 
Thanks

Mark Phippard
http://markphip.blogspot.com/

Re: FS vtable common_pool and pack_fs/recover

[Daniel Shahaf]

Philip Martin wrote on Tue, Aug 21, 2012 at 18:00:23 +0100:
> The FS loader uses a vtable fs_library_vtable_t defined in fs-loader.h.
> Some of the functions defined in the vtable have a common_pool
> parameter:
> 
>   /* The open_fs/create/open_fs_for_recovery/upgrade_fs functions are
>  serialized so that they may use the common_pool parameter to
>  allocate fs-global objects such as the bdb env cache. */
>   svn_error_t *(*create)(svn_fs_t *fs, const char *path, apr_pool_t *pool,
>  apr_pool_t *common_pool);
>   svn_error_t *(*open_fs)(svn_fs_t *fs, const char *path, apr_pool_t *pool,
>   apr_pool_t *common_pool);
> 
> 
> In FSFS this common_pool gets passed to fs_serialized_init in fs.c where
> it is used to setup setup mutexes:
> 
>   status = apr_pool_userdata_get(&val, key, common_pool);
>   if (status)
> return svn_error_wrap_apr(status, _("Can't fetch FSFS shared data"));
>   ffsd = val;
> 
>   if (!ffsd)
> {
>   ffsd = apr_pcalloc(common_pool, sizeof(*ffsd));
>   ffsd->common_pool = common_pool;
> 
>   /* POSIX fcntl locks are per-process, so we need a mutex for
>  intra-process synchronization when grabbing the repository write
>  lock. */
>   SVN_ERR(svn_mutex__init(&ffsd->fs_write_lock,
>   SVN_FS_FS__USE_LOCK_MUTEX, common_pool));
> 
> 
> Functions like fs_library_vtable_t.pack_fs and
> fs_library_vtable_t.recover that don't take the common_pool parameter
> pass the pool parameter to fs_serialized_init.  I think that means these
> functions should only be used from a single threaded process.
> 
> Is that correct?  If so we should document it or change the functions to
> handle the common_pool.
> 

I think the correct fix is to propagate common_pool to have fs_pack()
and pass it as to the appropriate parameter of fs_serialized_init().

> I'm not sure how the absence of common_pool affects the BDB
> implementations.

Is it even used?  A quick grep in libsvn_fs_base suggests that only fs.c
has variables by that name; therein, only svn_fs_base__init() uses them;
and that function seems to have no callers(!?).

(These results are so odd that I'm pretty sure I typoed the argument to
grep at some point...)

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[C. Michael Pilato]

On 08/21/2012 03:14 PM, Branko Čibej wrote:
> On 21.08.2012 21:07, Mark Phippard wrote:
>> I am also not crazy about introducing a bunch of new hooks that are
>> all more or less what the original start-commit hook was doing.
> 
> Have to agree here. I'm a bit worried that we're trying to introduce
> features too specific to what some vendor's client requests. This
> started off with finding ways to communicate the client version to the
> server (doubtful, but marginally acceptable) and suddenly we're talking
> about a new hook which, among other things, would set in stone the
> process that the server must follow during a commit.
> 
> I'm not at all sure that enough thought has been put into the idea.

While it is the case that client-version stuff stirred my creativity towards
this change, I want to be clear that I didn't actually decide to make the
change for that reason.  I made the change to introduce the hook because
when I mentioned the possibility of blocking a commit destined to fail
before a gig of data was transmitted across the wire, I got feedback from
several folks who were singing the praises of the idea.  for the record, I
still am not sure that the client-version stuff is best approached using
txnprops.  (But thanks anyway for the public implication that I'm merely a
hive-mind hacker.)

If the community decides to simply modify start-commit to run *after* the
txn gets created and populated with txnprops, that's fine with me.  I
assumed that that change was undesirable due to the fact mentioned
elsethread about how start-commit would no longer be useful for keeping a
would-be read-only repository read-only.

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development



signature.asc
Description: OpenPGP digital signature

Re: svn commit: r1374357 - in /subversion/trunk: Makefile.in build.conf build/ac-macros/apache.m4 build/generator/gen_base.py build/generator/gen_make.py build/generator/templates/makefile.ezt

[Branko Čibej]

On 21.08.2012 21:25, Branko Čibej wrote:
> On 21.08.2012 18:57, Daniel Shahaf wrote:
>> Suggest:
>>
>>   if [if-any target.when]$([target.when])[else]true[endif] ; then ...
>>
>> This avoids the need for another if-any at the end of the line.
> Good idea.

On second thoughts, this would make the build-outputs.mk less
readable/more confusing. So I'll leave it this way.

-- Brane

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: svn commit: r1374357 - in /subversion/trunk: Makefile.in build.conf build/ac-macros/apache.m4 build/generator/gen_base.py build/generator/gen_make.py build/generator/templates/makefile.ezt

[Branko Čibej]

On 21.08.2012 18:57, Daniel Shahaf wrote:
> A few comments on read-through (but without checking out-of-mail
> context):
[...]
> Could you document 'when' somewhere in the tree itself, please?  In
> build.conf comments, or build/generator/, I don't care, but not just in
> the log message please. :-)

Eh. Good point. Not that the rest of the generator is all that well
documented.

[...]

>> @@ -396,7 +397,8 @@ class Generator(gen_base.GeneratorBase):
>>  
>>  # ### TODO: This is a hack.  See discussion here:
>>  # ### http://mid.gmane.org/20120316191639.GA28451@daniel3.local
> Is this comment still applicable?

I have no idea.
[...]

> Suggest:
>
>   if [if-any target.when]$([target.when])[else]true[endif] ; then ...
>
> This avoids the need for another if-any at the end of the line.

Good idea.

> + [if-any areas.files.when]if $([areas.files.when]) ; then [else][end]
> + cd [areas.files.dirname] ; $(MKDIR) "$(APACHE_LIBEXECDIR)" ; 
> $(INSTALL_MOD_SHARED) -n [areas.files.name] [areas.files.filename]
> + [if-any areas.files.when] ; fi[else][end][end]
> Should the line above be:
>
>   +   [end][if-any areas.files.when] ; fi[else][end]
>
> ?

Nope, the last [end] closes the [for areas.files].

-- Brane

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Branko Čibej]

On 21.08.2012 21:07, Mark Phippard wrote:
> I am also not crazy about introducing a bunch of new hooks that are
> all more or less what the original start-commit hook was doing.

Have to agree here. I'm a bit worried that we're trying to introduce
features too specific to what some vendor's client requests. This
started off with finding ways to communicate the client version to the
server (doubtful, but marginally acceptable) and suddenly we're talking
about a new hook which, among other things, would set in stone the
process that the server must follow during a commit.

I'm not at all sure that enough thought has been put into the idea.

-- Brane

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Branko Čibej]

On 21.08.2012 20:45, Philip Martin wrote:
> Blair Zajac  writes:
>
>> On 08/21/2012 11:09 AM, C. Michael Pilato wrote:
>>> I actually considered using "post-create-txn" and renaming "start-commit" to
>>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>>> hook exists, for compat purposes).
>> +1.  I always have to remember which comes first, start-commit or
>> pre-commit, so this renaming helps.
> Suppose both pre-create-txn and start-commit exist.  Is it an error?
> If not which one is run?
>
> We have already bumped the FSFS format in 1.8 but we have not yet bumped
> the repos format.  Perhaps we could bump and have an upgrade that
> renames the hook?

How will that conform to our compatibility guarantees? The hooks are
part of the Subversion repository API. You can't rename them.

-- Brane

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Mark Phippard]

On Tue, Aug 21, 2012 at 3:00 PM, C. Michael Pilato  wrote:
> On 08/21/2012 02:55 PM, Mark Phippard wrote:
>> On Tue, Aug 21, 2012 at 2:38 PM, C. Michael Pilato  
>> wrote:
>>> On 08/21/2012 02:29 PM, Blair Zajac wrote:
> I actually considered using "post-create-txn" and renaming "start-commit" 
> to
> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
> hook exists, for compat purposes).

 +1.  I always have to remember which comes first, start-commit or
 pre-commit, so this renaming helps.
>>>
>>> Cool.  Will make that my next set of changes, then.  Thanks for the 
>>> feedback.
>>
>> Who besides a SVN developer would know that pre/post create-txn comes
>> before pre-commit?  I do not see how these names are an improvement.
>> I also do not think start-commit, pre-commit were all that confusing
>> to end users.  The confusing part is always just understanding what
>> you can do in the hooks.
>
> Hook authorship falls to admins, not mere users.  And I think it is fair to
> say that we expect admins to understand the notion of commit transactions as
> "revisions-to-be".  In fact, we expose this nomenclature readily through the
> 'svnlook -t TXN_NAME ...' interfaces.  You pretty much can't write a
> pre-commit hook script without "getting" the concept of these 'txn' things.

I am simply pointing out that anyone who believes these new names
makes things crystal clear are too close to the underlying code.  In
particular as an admin and hook author I would have no reason to
expect that a post-create-txn hook only contains a small subset of the
data that will ultimately go into the transaction.  I imagine this is
why you initially were leaning towards "init-txn".  I think of
creating the txn as also populating things like the list of files and
likely the data as well.  AFAIK, you are proposing to call the hook
after only the basics of the txn have been established.

I am not proposing another hook with this additional data either.
Just trying to inject some skepticism into how clear these names
really make things.  In an ideal world, I would have a better name to
propose.  I realize that.

I am also not crazy about introducing a bunch of new hooks that are
all more or less what the original start-commit hook was doing.

> One downside of changing the behavior of the start-commit hook is that it
> will no longer be useful for making a repository effectively read-only in
> the way that it is today (in concert with restrictive pre-revprop-change,
> pre-lock, and pre-unlock hooks).

Fair enough.

-- 
Thanks

Mark Phippard
http://markphip.blogspot.com/

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Blair Zajac]

On 08/21/2012 11:56 AM, C. Michael Pilato wrote:

On 08/21/2012 02:45 PM, Philip Martin wrote:

Blair Zajac  writes:


On 08/21/2012 11:09 AM, C. Michael Pilato wrote:


I actually considered using "post-create-txn" and renaming "start-commit" to
"pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
hook exists, for compat purposes).


+1.  I always have to remember which comes first, start-commit or
pre-commit, so this renaming helps.


Suppose both pre-create-txn and start-commit exist.  Is it an error?
If not which one is run?

We have already bumped the FSFS format in 1.8 but we have not yet bumped
the repos format.  Perhaps we could bump and have an upgrade that
renames the hook?


Are we comfortable with renaming the hook, which -- strictly speaking -- is
user-managed data, not Subversion managed data?  What if the hook itself is
kept under version control (which is pretty common)?  I lean against doing
so.  And because no change of administrative behavior is required (we'll
still happily run "start-commit" if there's no "pre-txn-create"), I see no
need for the format bump.


True, good point.  Moving anything there isn't save.  In my setup, we 
have symlinks to a common area, which wouldn't break with a rename, but 
I would be surprised.



As to whether to flag an error if both "start-commit" and "pre-txn-create"
exist:  this makes sense.  I see value in warning *someone* that the
repository configuration is non-ideal, similarly to the error we return from
a missing pre-revprop-change hook script ("ask the administrator to [fix
this problem]").


A missing pre-revprop-change script will warn to users doing a propedit, 
which is infrequent.  But for a non-ideal hooks, do we warn all 
committers?  That would be annoying, but probably result in a prompt fix 
by the admin ;)


Blair

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[C. Michael Pilato]

On 08/21/2012 02:55 PM, Mark Phippard wrote:
> On Tue, Aug 21, 2012 at 2:38 PM, C. Michael Pilato  
> wrote:
>> On 08/21/2012 02:29 PM, Blair Zajac wrote:
 I actually considered using "post-create-txn" and renaming "start-commit" 
 to
 "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
 hook exists, for compat purposes).
>>>
>>> +1.  I always have to remember which comes first, start-commit or
>>> pre-commit, so this renaming helps.
>>
>> Cool.  Will make that my next set of changes, then.  Thanks for the feedback.
> 
> Who besides a SVN developer would know that pre/post create-txn comes
> before pre-commit?  I do not see how these names are an improvement.
> I also do not think start-commit, pre-commit were all that confusing
> to end users.  The confusing part is always just understanding what
> you can do in the hooks.

Hook authorship falls to admins, not mere users.  And I think it is fair to
say that we expect admins to understand the notion of commit transactions as
"revisions-to-be".  In fact, we expose this nomenclature readily through the
'svnlook -t TXN_NAME ...' interfaces.  You pretty much can't write a
pre-commit hook script without "getting" the concept of these 'txn' things.

> Did you ever say why you ruled out your earlier idea of simply moving
> where the start-commit hook was called to later in the process where
> more information would be available?
> 
> If we have a new hook, I do not have an obvious idea as to better new
> name.  Maybe init-commit just so that the link to the commit process
> is being maintained.  Then we would have to document the order is
> start > init > pre > post
> 
> It seems like moving the start-commit hook would be easiest option and
> I do not immediately see how it would break existing hooks.  So I do
> not see the downside to that option.

One downside of changing the behavior of the start-commit hook is that it
will no longer be useful for making a repository effectively read-only in
the way that it is today (in concert with restrictive pre-revprop-change,
pre-lock, and pre-unlock hooks).

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development



signature.asc
Description: OpenPGP digital signature

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Philip Martin]

Blair Zajac  writes:

> On 08/21/2012 11:45 AM, Philip Martin wrote:
>> Blair Zajac  writes:
>
>> We have already bumped the FSFS format in 1.8 but we have not yet bumped
>> the repos format.  Perhaps we could bump and have an upgrade that
>> renames the hook?
>
> That would be make repos maintenance easier in the long run.

It also means that the pre-create-txn cannot be bypassed by using older
Subversion.

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[C. Michael Pilato]

On 08/21/2012 02:45 PM, Philip Martin wrote:
> Blair Zajac  writes:
> 
>> On 08/21/2012 11:09 AM, C. Michael Pilato wrote:
>>>
>>> I actually considered using "post-create-txn" and renaming "start-commit" to
>>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>>> hook exists, for compat purposes).
>>
>> +1.  I always have to remember which comes first, start-commit or
>> pre-commit, so this renaming helps.
> 
> Suppose both pre-create-txn and start-commit exist.  Is it an error?
> If not which one is run?
> 
> We have already bumped the FSFS format in 1.8 but we have not yet bumped
> the repos format.  Perhaps we could bump and have an upgrade that
> renames the hook?

Are we comfortable with renaming the hook, which -- strictly speaking -- is
user-managed data, not Subversion managed data?  What if the hook itself is
kept under version control (which is pretty common)?  I lean against doing
so.  And because no change of administrative behavior is required (we'll
still happily run "start-commit" if there's no "pre-txn-create"), I see no
need for the format bump.

As to whether to flag an error if both "start-commit" and "pre-txn-create"
exist:  this makes sense.  I see value in warning *someone* that the
repository configuration is non-ideal, similarly to the error we return from
a missing pre-revprop-change hook script ("ask the administrator to [fix
this problem]").

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development



signature.asc
Description: OpenPGP digital signature

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Mark Phippard]

On Tue, Aug 21, 2012 at 2:38 PM, C. Michael Pilato  wrote:
> On 08/21/2012 02:29 PM, Blair Zajac wrote:
>>> I actually considered using "post-create-txn" and renaming "start-commit" to
>>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>>> hook exists, for compat purposes).
>>
>> +1.  I always have to remember which comes first, start-commit or
>> pre-commit, so this renaming helps.
>
> Cool.  Will make that my next set of changes, then.  Thanks for the feedback.

Who besides a SVN developer would know that pre/post create-txn comes
before pre-commit?  I do not see how these names are an improvement.
I also do not think start-commit, pre-commit were all that confusing
to end users.  The confusing part is always just understanding what
you can do in the hooks.

Did you ever say why you ruled out your earlier idea of simply moving
where the start-commit hook was called to later in the process where
more information would be available?

If we have a new hook, I do not have an obvious idea as to better new
name.  Maybe init-commit just so that the link to the commit process
is being maintained.  Then we would have to document the order is
start > init > pre > post

It seems like moving the start-commit hook would be easiest option and
I do not immediately see how it would break existing hooks.  So I do
not see the downside to that option.

-- 
Thanks

Mark Phippard
http://markphip.blogspot.com/

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Blair Zajac]

On 08/21/2012 11:45 AM, Philip Martin wrote:

Blair Zajac  writes:


On 08/21/2012 11:09 AM, C. Michael Pilato wrote:


I actually considered using "post-create-txn" and renaming "start-commit" to
"pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
hook exists, for compat purposes).


+1.  I always have to remember which comes first, start-commit or
pre-commit, so this renaming helps.


Suppose both pre-create-txn and start-commit exist.  Is it an error?
If not which one is run?


I don't think it's an error, run both of them.  I don't have any 
thoughts on the ordering, perhaps start-commit first, since it's always 
been there?



We have already bumped the FSFS format in 1.8 but we have not yet bumped
the repos format.  Perhaps we could bump and have an upgrade that
renames the hook?


That would be make repos maintenance easier in the long run.

Blair

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Philip Martin]

Blair Zajac  writes:

> On 08/21/2012 11:09 AM, C. Michael Pilato wrote:
>>
>> I actually considered using "post-create-txn" and renaming "start-commit" to
>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>> hook exists, for compat purposes).
>
> +1.  I always have to remember which comes first, start-commit or
> pre-commit, so this renaming helps.

Suppose both pre-create-txn and start-commit exist.  Is it an error?
If not which one is run?

We have already bumped the FSFS format in 1.8 but we have not yet bumped
the repos format.  Perhaps we could bump and have an upgrade that
renames the hook?

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[C. Michael Pilato]

On 08/21/2012 02:29 PM, Blair Zajac wrote:
>> I actually considered using "post-create-txn" and renaming "start-commit" to
>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>> hook exists, for compat purposes).
> 
> +1.  I always have to remember which comes first, start-commit or
> pre-commit, so this renaming helps.

Cool.  Will make that my next set of changes, then.  Thanks for the feedback.

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development



signature.asc
Description: OpenPGP digital signature

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Blair Zajac]

On 08/21/2012 11:09 AM, C. Michael Pilato wrote:

On 08/21/2012 01:54 PM, Blair Zajac wrote:

On 08/21/2012 10:29 AM, cmpil...@apache.org wrote:

Author: cmpilato
Date: Tue Aug 21 17:29:40 2012
New Revision: 1375675

URL: http://svn.apache.org/viewvc?rev=1375675&view=rev
Log:
Introduce a new 'init-commit' hook script which runs immediately after
the commit txn is created and populated with initial txnprops.


I'm curious where this is used.  This is just after the txn is begun but
before any modifications are made in it?


That's correct.  Well, not before *any* modifications are made.  The
libsvn_repos-level code which creates commit transactions also sets a bunch
of txnprops (author, date, plus any user-supplied ones such as log messages
or custom revprops) on the newly created transaction.  This hook runs
immediately after that initial batch of txnprop changes is made.


Is the use case it to check the txn props that aren't available in 
start-commit?



Calling this "init" isn't self documenting, it's not clear where in the
commit steps it occurs.  It doesn't fit in the "pre-" or "post-" convention
where it's clear where it runs in the order.


I agree that the name isn't ideal.


Calling this "post-create", "post-txn-create" would be a clearer name. Given
this hook is infrequently used, I would prefer the later.


I actually considered using "post-create-txn" and renaming "start-commit" to
"pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
hook exists, for compat purposes).


+1.  I always have to remember which comes first, start-commit or 
pre-commit, so this renaming helps.


Blair




Any thoughts on that?

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[C. Michael Pilato]

On 08/21/2012 02:02 PM, Bert Huijben wrote:
> When is this script run when committing with a non http-v2 dav client?
>
> Are the txnprops available then? Or just with the other ra layers and
> new dav clients?

The script is still run after the txn is created and the initial txnprops
set.  So, you'll get svn:date and svn:author, but not svn:log or the
user-provided custom revprops.  (If this is a point of concern, the hook
author can simply bounce commits on the grounds that the client isn't new
enough for its standards.)

>> +svn_error_t *
>> +svn_repos__hooks_init_commit(svn_repos_t *repos,
>> + const char *txn_name,
>> + apr_pool_t *pool)

[...]

> Should we also pass the user here?
> 
> We pass it for at least some of the other hooks and I'm not sure if it is
> guaranteed always to be the same as the one stored in the transaction
> properties.

It would have *just* been set as the svn:author txnprop, but I strictly
speaking there is a race condition between the time the property is set and
the time the hook executes.  If a program was able to guess the name of the
newly created transaction and change svn:author before the hook examined it,
that might be a point of concern?

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development



signature.asc
Description: OpenPGP digital signature

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[C. Michael Pilato]

On 08/21/2012 01:54 PM, Blair Zajac wrote:
> On 08/21/2012 10:29 AM, cmpil...@apache.org wrote:
>> Author: cmpilato
>> Date: Tue Aug 21 17:29:40 2012
>> New Revision: 1375675
>>
>> URL: http://svn.apache.org/viewvc?rev=1375675&view=rev
>> Log:
>> Introduce a new 'init-commit' hook script which runs immediately after
>> the commit txn is created and populated with initial txnprops.
> 
> I'm curious where this is used.  This is just after the txn is begun but
> before any modifications are made in it?

That's correct.  Well, not before *any* modifications are made.  The
libsvn_repos-level code which creates commit transactions also sets a bunch
of txnprops (author, date, plus any user-supplied ones such as log messages
or custom revprops) on the newly created transaction.  This hook runs
immediately after that initial batch of txnprop changes is made.

> Calling this "init" isn't self documenting, it's not clear where in the
> commit steps it occurs.  It doesn't fit in the "pre-" or "post-" convention
> where it's clear where it runs in the order.

I agree that the name isn't ideal.

> Calling this "post-create", "post-txn-create" would be a clearer name. Given
> this hook is infrequently used, I would prefer the later.

I actually considered using "post-create-txn" and renaming "start-commit" to
"pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
hook exists, for compat purposes).

Any thoughts on that?

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development



signature.asc
Description: OpenPGP digital signature

RE: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Bert Huijben]

> -Original Message-
> From: cmpil...@apache.org [mailto:cmpil...@apache.org]
> Sent: dinsdag 21 augustus 2012 19:30
> To: comm...@subversion.apache.org
> Subject: svn commit: r1375675 - in /subversion/trunk/subversion:
> include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c
> libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py
> tests/cmdline/svntest/actions.py
> 
> Author: cmpilato
> Date: Tue Aug 21 17:29:40 2012
> New Revision: 1375675
> 
> URL: http://svn.apache.org/viewvc?rev=1375675&view=rev
> Log:
> Introduce a new 'init-commit' hook script which runs immediately after
> the commit txn is created and populated with initial txnprops.

This is more about your previous patches:

When is this script run when committing with a non http-v2 dav client?

Are the txnprops available then? Or just with the other ra layers and new dav 
clients?



> +svn_error_t *
> +svn_repos__hooks_init_commit(svn_repos_t *repos,
> + const char *txn_name,
> + apr_pool_t *pool)
> +{
> +  const char *hook = svn_repos_init_commit_hook(repos, pool);
> +  svn_boolean_t broken_link;
> +
> +  if ((hook = check_hook_cmd(hook, &broken_link, pool)) && broken_link)
> +{
> +  return hook_symlink_error(hook);
> +}
> +  else if (hook)
> +{
> +  const char *args[4];
> +
> +  args[0] = hook;
> +  args[1] = svn_dirent_local_style(svn_repos_path(repos, pool), pool);
> +  args[2] = txn_name;
> +  args[3] = NULL;

Should we also pass the user here?

We pass it for at least some of the other hooks and I'm not sure if it is 
guaranteed always to be the same as the one stored in the transaction 
properties.

Bert

Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py test

[Blair Zajac]

On 08/21/2012 10:29 AM, cmpil...@apache.org wrote:

Author: cmpilato
Date: Tue Aug 21 17:29:40 2012
New Revision: 1375675

URL: http://svn.apache.org/viewvc?rev=1375675&view=rev
Log:
Introduce a new 'init-commit' hook script which runs immediately after
the commit txn is created and populated with initial txnprops.


I'm curious where this is used.  This is just after the txn is begun but 
before any modifications are made in it?


Calling this "init" isn't self documenting, it's not clear where in the 
commit steps it occurs.  It doesn't fit in the "pre-" or "post-" 
convention where it's clear where it runs in the order.


Calling this "post-create", "post-txn-create" would be a clearer name. 
Given this hook is infrequently used, I would prefer the later.


Blair

Re: svn commit: r1374357 - in /subversion/trunk: Makefile.in build.conf build/ac-macros/apache.m4 build/generator/gen_base.py build/generator/gen_make.py build/generator/templates/makefile.ezt

[Daniel Shahaf]

br...@apache.org wrote on Fri, Aug 17, 2012 at 16:58:22 -:
> Author: brane
> Date: Fri Aug 17 16:58:22 2012
> New Revision: 1374357
> 
> URL: http://svn.apache.org/viewvc?rev=1374357&view=rev
> Log:
> Followup to r1374198: Introduce a new build.conf predicate "when", which names
> the post-configure substituted variable that controls the building and linking
> of a module. Since the makefiles are generated before configure, "when" cannot
> control the dependencies, but it can control the build/link commands.
> 
> With this change, we can now put mod_dontdothat back into the tools group.
> 
> Modified: subversion/trunk/build/generator/templates/makefile.ezt
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/build/generator/templates/makefile.ezt?rev=1374357&r1=1374356&r2=1374357&view=diff
> ==
> --- subversion/trunk/build/generator/templates/makefile.ezt (original)
> +++ subversion/trunk/build/generator/templates/makefile.ezt Fri Aug 17 
> 16:58:22 2012
> @@ -127,13 +127,13 @@ $([target.varname]_OBJECTS): $([target.v
>  
>  [for areas]
>  [is areas.type "apache-mod"]install-mods-shared:[for areas.files] 
> [areas.files.fullname][end][for areas.files]
> - cd [areas.files.dirname] ; $(MKDIR) "$(APACHE_LIBEXECDIR)" ; 
> $(INSTALL_MOD_SHARED) -n [areas.files.name] [areas.files.filename][end]
> + [if-any areas.files.when]if $([areas.files.when]) ; then [else][end]
> + cd [areas.files.dirname] ; $(MKDIR) "$(APACHE_LIBEXECDIR)" ; 
> $(INSTALL_MOD_SHARED) -n [areas.files.name] [areas.files.filename]
> + [if-any areas.files.when] ; fi[else][end][end]

Should the line above be:

  + [end][if-any areas.files.when] ; fi[else][end]

?

>  [else]install-[areas.type]: [for areas.files][if-index areas.files 
> first][else] [end][areas.files.fullname][end] [for areas.apache_files] 
> [areas.apache_files.fullname][end]
>   $(MKDIR) $(DESTDIR)$([areas.varname]dir)[for areas.files][is areas.type 
> "locale"]
>   $(MKDIR) [areas.files.installdir]
> - cd [areas.files.dirname] ; $(INSTALL_[areas.uppervar]) 
> [areas.files.filename] 
> [areas.files.installdir]/$(PACKAGE_NAME)[areas.files.objext][else]
> - cd [areas.files.dirname] ; $(INSTALL_[areas.uppervar]) 
> [areas.files.filename] $(DESTDIR)[areas.files.install_fname][end][end][for 
> areas.apache_files]
> - cd [areas.apache_files.dirname] ; $(MKDIR) "$(APACHE_LIBEXECDIR)" ; 
> $(INSTALL_MOD_SHARED) -n [areas.apache_files.name] 
> [areas.apache_files.filename][end]
> + [if-any areas.files.when]if $([areas.files.when]) ; then [else][end]cd 
> [areas.files.dirname] ; $(INSTALL_[areas.uppervar]) [areas.files.filename] 
> [areas.files.installdir]/$(PACKAGE_NAME)[areas.files.objext][if-any 
> areas.files.when] ; fi[else][end][else]
> + [if-any areas.files.when]if $([areas.files.when]) ; then [else][end]cd 
> [areas.files.dirname] ; $(INSTALL_[areas.uppervar]) [areas.files.filename] 
> $(DESTDIR)[areas.files.install_fname][if-any areas.files.when] ; 
> fi[else][end][end][end][for areas.apache_files]
> + [if-any areas.apache_files.when]if $([areas.apache_files.when]) ; then 
> [else][end]cd [areas.apache_files.dirname] ; $(MKDIR) "$(APACHE_LIBEXECDIR)" 
> ; $(INSTALL_MOD_SHARED) -n [areas.apache_files.name] 
> [areas.apache_files.filename][if-any areas.apache_files.when] ; 
> fi[else][end][end]

I didn't check whether the same issue applies here.

>  [if-any areas.extra_install] $(INSTALL_EXTRA_[areas.uppervar])
>  [end][end][end]
>  

FS vtable common_pool and pack_fs/recover

[Philip Martin]

The FS loader uses a vtable fs_library_vtable_t defined in fs-loader.h.
Some of the functions defined in the vtable have a common_pool
parameter:

  /* The open_fs/create/open_fs_for_recovery/upgrade_fs functions are
 serialized so that they may use the common_pool parameter to
 allocate fs-global objects such as the bdb env cache. */
  svn_error_t *(*create)(svn_fs_t *fs, const char *path, apr_pool_t *pool,
 apr_pool_t *common_pool);
  svn_error_t *(*open_fs)(svn_fs_t *fs, const char *path, apr_pool_t *pool,
  apr_pool_t *common_pool);


In FSFS this common_pool gets passed to fs_serialized_init in fs.c where
it is used to setup setup mutexes:

  status = apr_pool_userdata_get(&val, key, common_pool);
  if (status)
return svn_error_wrap_apr(status, _("Can't fetch FSFS shared data"));
  ffsd = val;

  if (!ffsd)
{
  ffsd = apr_pcalloc(common_pool, sizeof(*ffsd));
  ffsd->common_pool = common_pool;

  /* POSIX fcntl locks are per-process, so we need a mutex for
 intra-process synchronization when grabbing the repository write
 lock. */
  SVN_ERR(svn_mutex__init(&ffsd->fs_write_lock,
  SVN_FS_FS__USE_LOCK_MUTEX, common_pool));


Functions like fs_library_vtable_t.pack_fs and
fs_library_vtable_t.recover that don't take the common_pool parameter
pass the pool parameter to fs_serialized_init.  I think that means these
functions should only be used from a single threaded process.

Is that correct?  If so we should document it or change the functions to
handle the common_pool.

I'm not sure how the absence of common_pool affects the BDB
implementations.

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Re: svn commit: r1374357 - in /subversion/trunk: Makefile.in build.conf build/ac-macros/apache.m4 build/generator/gen_base.py build/generator/gen_make.py build/generator/templates/makefile.ezt

[Daniel Shahaf]

A few comments on read-through (but without checking out-of-mail
context):

br...@apache.org wrote on Fri, Aug 17, 2012 at 16:58:22 -:
> Author: brane
> Date: Fri Aug 17 16:58:22 2012
> New Revision: 1374357
> 
> URL: http://svn.apache.org/viewvc?rev=1374357&view=rev
> Log:
> Followup to r1374198: Introduce a new build.conf predicate "when", which names
> the post-configure substituted variable that controls the building and linking
> of a module. Since the makefiles are generated before configure, "when" cannot
> control the dependencies, but it can control the build/link commands.
> 

Could you document 'when' somewhere in the tree itself, please?  In
build.conf comments, or build/generator/, I don't care, but not just in
the log message please. :-)

> @@ -396,7 +397,8 @@ class Generator(gen_base.GeneratorBase):
>  
>  # ### TODO: This is a hack.  See discussion here:
>  # ### http://mid.gmane.org/20120316191639.GA28451@daniel3.local

Is this comment still applicable?

> -apache_files = [t.filename for t in inst_targets
> +apache_files = [gen_base.FileInfo(t.filename, t.when)
> +for t in inst_targets
>  if isinstance(t, gen_base.TargetApacheMod)]
>  
>  files = [f for f in files if f not in apache_files]
> +++ subversion/trunk/build/generator/templates/makefile.ezt Fri Aug 17 
> 16:58:22 2012
> @@ -112,7 +112,7 @@ $([target.varname]_OBJECTS): $([target.v
>  [else][target.varname]_DEPS = [target.add_deps][for target.objects] 
> [target.objects][end][for target.deps] [target.deps][end]
>  [target.varname]_OBJECTS =[for target.objnames] [target.objnames][end]
>  [target.filename]: $([target.varname]_DEPS)
> - cd [target.path] && [target.link_cmd] $([target.varname]_LDFLAGS) -o 
> [target.basename] [target.undefined_flag] $([target.varname]_OBJECTS)[for 
> target.libs] [target.libs][end] $(LIBS)
> + [if-any target.when]if $([target.when]) ; then [else][end]cd 
> [target.path] && [target.link_cmd] $([target.varname]_LDFLAGS) -o 
> [target.basename] [target.undefined_flag] $([target.varname]_OBJECTS)[for 
> target.libs] [target.libs][end] $(LIBS)[if-any target.when] ; else echo 
> "fake" > [target.filename] ; fi[else][end]

Suggest:

  if [if-any target.when]$([target.when])[else]true[endif] ; then ...

This avoids the need for another if-any at the end of the line.

Re: Vladimir's Status

[Greg Stein]

No worries, Vladimir! It happens to all of us :-) ... thanks for the
update, and we'll see you soon!

Cheers,
-g

On Tue, Aug 21, 2012 at 8:31 AM, Vladimir Berezniker  wrote:
> Hi All,
>
> Just wanted to let you know that I did not forget about the javahl-ra
> branch and most likely be able to get back to it towards the end of
> September. But at the moment $DAYJOB does not leave me with sufficient
> free time to work on other projects.
>
> Regards,
>
> Vladimir

Re: [Issue 4124] New - Give servers sufficient means to disallow commits from clients based on version numbers

[Philip Martin]

"C. Michael Pilato"  writes:

> Now, if we can find a way to expose the revprops to the start-commit hook,
> all three RA layers should be able to provide early detection of commits
> that would fail in pre-commit for revprop-related reasons.  (And we should
> consider this independently of the whole client-version capability stuff.)

Perhaps we need another hook that runs immediately after the transaction
is created?

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Vladimir's Status

[Vladimir Berezniker]

Hi All,

Just wanted to let you know that I did not forget about the javahl-ra
branch and most likely be able to get back to it towards the end of
September. But at the moment $DAYJOB does not leave me with sufficient
free time to work on other projects.

Regards,

Vladimir

RE: svn commit: r1371831 / The idea behind the workqueue

[Bert Huijben]

> -Original Message-
> From: pbu...@apache.org [mailto:pbu...@apache.org]
> Sent: zaterdag 11 augustus 2012 00:19
> To: comm...@subversion.apache.org
> Subject: svn commit: r1371831 - in /subversion/branches/inheritable-
> props/subversion: include/private/ libsvn_client/ libsvn_wc/
> tests/libsvn_wc/
> 
> Author: pburba
> Date: Fri Aug 10 22:18:59 2012
> New Revision: 1371831
> 
> URL: http://svn.apache.org/viewvc?rev=1371831&view=rev
> Log:
> On the inheritable-props branch: Atomically update cached iprops with
> work queue items during update/switch editor drives.  Remove the
> temporary
> svn_client__update_inheritable_props API.

Delayed review, as I didn't feel like typing this on my phone a few weeks ago, 
and I don't think this had to be solved soon.


As a temporary api I don't see a problem with this, but I would recommend *not* 
using workqueue items for just updating wc.db. Especially if you can do the 
same work in the same db transaction as the primary operation. The workqueue's 
task is just keeping the database and in-working copy data in sync.

Delaying the operation to a workqueue operation leaves the database in an 
'in-between' stable-states and running the workqueue for such an operation 
requires at least 3 additional database transactions before the database is 
stable again.
I just finished removing the last of such states before I left for vacation. 
(We still had a few of such operations left over from the old loggy behavior)


And not using a workqueue operation also makes it easier to change the way we 
handle this in the future. Once we release a Subversion version with some 
workqueue support 'all future versions' should support running this workqueue 
item.



For now I would recommend just going on the way you started... we can fix this 
before branching.



Bert