You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2012/05/29 23:11:54 UTC
Re: svn commit: r1343456 - in
/subversion/branches/javahl-ra/subversion/bindings/javahl/native:
RevpropTable.cpp RevpropTable.h
Greg Stein wrote on Tue, May 29, 2012 at 16:28:19 -0400:
> I would suggest any logical change that can apply to trunk should be
> submitted to dev@. If it helps trunk, or is at least neutral, then I'd
> support it.
>
> We couldn't digest your initial 27 patches :-), but some minor ones showing
> up should be just fine. I would guess you'll be looking for a +1 from Mark
> or Hyrum. Most others don't really know JavaHL :-(
>
Why don't we ask Vladimir to commit patches to the branch and then ask
for a +1 to cherry-pick them to trunk?
He could even maintain a STATUS file in the branch of revisions
nominated for cherry picking...
</peanut gallery>
Daniel
> Cheers,
> -g
> On May 28, 2012 11:35 PM, "Vladimir Berezniker" <vl...@berezniker.com>
> wrote:
>
> > Hi Hyrum,
> >
> > I committed JavaHL re-factoring changes in r1343452 and r1343456 thinking
> > that
> > while they are generally applicable to JavaHL code, they won't be used by
> > any
> > other JavaHL code, so they should go on the branch. But on a second thought
> > they are not tied to the new RA code, it just happens to be the only user
> > at
> > the moment. Should I submit changes like these as patches against trunk to
> > @dev or continue committing them to javahl-ra branch?
> >
> > Thank you in advance,
> >
> > Vladimir
> >
> >
> > On Mon, May 28, 2012 at 11:22 PM, <co...@subversion.apache.org>wrote:
> >
> >>
> >> Author: vmpn
> >> Date: Tue May 29 02:57:05 2012
> >> New Revision: 1343456
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1343456&view=rev
> >> Log:
> >> On the javahl-ra branch:
> >>
> >> JavaHL: Support returning non const, empty rather than NULL hash as
> >> required
> >> by (svn_ra_get_commit_editor3) apr_hash_t *revprop_table parameter
> >>
> >> [ in subversion/bindings/javahl/native ]
> >>
> >> * RevpropTable.cpp,
> >> RevpropTable.h
> >> (hash): Removed const qualifier and added bool nullIfEmpty parameter to
> >> specify whether empty hash or NULL should be returned
> >>
> >> Modified:
> >>
> >> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.cpp
> >>
> >> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.h
> >>
> >> Modified:
> >> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.cpp
> >> URL:
> >> http://svn.apache.org/viewvc/subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.cpp?rev=1343456&r1=1343455&r2=1343456&view=diff
> >>
> >> ==============================================================================
> >> ---
> >> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.cpp
> >> (original)
> >> +++
> >> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.cpp
> >> Tue May 29 02:57:05 2012
> >> @@ -41,9 +41,9 @@ RevpropTable::~RevpropTable()
> >> JNIUtil::getEnv()->DeleteLocalRef(m_revpropTable);
> >> }
> >>
> >> -const apr_hash_t *RevpropTable::hash(const SVN::Pool &pool)
> >> +apr_hash_t *RevpropTable::hash(const SVN::Pool &pool, bool nullIfEmpty)
> >> {
> >> - if (m_revprops.size() == 0)
> >> + if (m_revprops.size() == 0 && nullIfEmpty)
> >> return NULL;
> >>
> >> apr_hash_t *revprop_table = apr_hash_make(pool.getPool());
> >>
> >> Modified:
> >> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.h
> >> URL:
> >> http://svn.apache.org/viewvc/subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.h?rev=1343456&r1=1343455&r2=1343456&view=diff
> >>
> >> ==============================================================================
> >> ---
> >> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.h
> >> (original)
> >> +++
> >> subversion/branches/javahl-ra/subversion/bindings/javahl/native/RevpropTable.h
> >> Tue May 29 02:57:05 2012
> >> @@ -44,7 +44,7 @@ class RevpropTable
> >> public:
> >> RevpropTable(jobject jrevpropTable);
> >> ~RevpropTable();
> >> - const apr_hash_t *hash(const SVN::Pool &pool);
> >> + apr_hash_t *hash(const SVN::Pool &pool, bool nullIfEmpty = true);
> >> };
> >>
> >> #endif // REVPROPTABLE_H
> >>
> >>
> >>
> >>
> >>
> >
Re: svn commit: r1343456 - in /subversion/branches/javahl-ra/subversion/bindings/javahl/native:
RevpropTable.cpp RevpropTable.h
Posted by Vladimir Berezniker <vm...@hitechman.com>.
I will do as @dev decides.
But to give context, at the moment there are 14 patches with first two
already committed to the branch (mea culpa). 3 of the patches require
manual svn copy command to apply since svn patch does not support conveying
copy operation in the patch. Please don't be alarmed by the number of
patches, there are mostly small and each can be reviewed on their own.
From the perspective of the trunk they will be neutral. However about 3rd
of them can be used to reduce amount of code used in current JavaHL
implementation by factoring out commonly used code into macros. Rest are
about making code more generic so it can be used with the svn_ra_*
functions.
Hope this helps,
Vladimir
On Wed, May 30, 2012 at 12:01 AM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> Hyrum K Wright wrote on Tue, May 29, 2012 at 20:04:24 -0500:
> > On Tue, May 29, 2012 at 4:11 PM, Daniel Shahaf <d....@daniel.shahaf.name>
> wrote:
> > > Greg Stein wrote on Tue, May 29, 2012 at 16:28:19 -0400:
> > >> I would suggest any logical change that can apply to trunk should be
> > >> submitted to dev@. If it helps trunk, or is at least neutral, then
> I'd
> > >> support it.
> > >>
> > >> We couldn't digest your initial 27 patches :-), but some minor ones
> showing
> > >> up should be just fine. I would guess you'll be looking for a +1 from
> Mark
> > >> or Hyrum. Most others don't really know JavaHL :-(
> > >>
> > >
> > > Why don't we ask Vladimir to commit patches to the branch and then ask
> > > for a +1 to cherry-pick them to trunk?
> > >
> > > He could even maintain a STATUS file in the branch of revisions
> > > nominated for cherry picking...
> >
> > So you're suggesting a change in policy from the traditional method of
> > committing stuff directly to trunk (after the requisite +1 from a full
> > committer, if required)?
> >
>
> I'm just trying to get out of Vladimir's way. Via the "branch STATUS
> file" approach he can parallelize (a) his work, (b) merging it ---
> having N merge threads open would be harder to track.
>
> He'll still have to work with us, just differently.
>
> Daniel
>
> > I think the dev@ + review technique we've historically employed is
> > better for a couple of reasons. Firstly, it ensures stuff gets to
> > trunk by the quickest route possible, and encourages the contributor
> > to work with the community. Second, it also ensures that the code
> > will be released, even if the branch doesn't pan out. Thirdly, I
> > think it gives the contributor more visibility, and hopefully
> > encourages reviewers to offer the relevant commit access more rapidly.
> >
> > In light of those reasons, I still prefer Vladimir to submit generally
> > relevant patches to dev@ for commit to trunk, rather than commit to
> > the branch and pull them from there. (And this isn't specific to just
> > this instance of course.)
> >
> > -Hyrum
> >
> >
> > --
> >
> > uberSVN: Apache Subversion Made Easy
> > http://www.uberSVN.com/
>
Re: svn commit: r1343456 - in
/subversion/branches/javahl-ra/subversion/bindings/javahl/native:
RevpropTable.cpp RevpropTable.h
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Hyrum K Wright wrote on Tue, May 29, 2012 at 20:04:24 -0500:
> On Tue, May 29, 2012 at 4:11 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Greg Stein wrote on Tue, May 29, 2012 at 16:28:19 -0400:
> >> I would suggest any logical change that can apply to trunk should be
> >> submitted to dev@. If it helps trunk, or is at least neutral, then I'd
> >> support it.
> >>
> >> We couldn't digest your initial 27 patches :-), but some minor ones showing
> >> up should be just fine. I would guess you'll be looking for a +1 from Mark
> >> or Hyrum. Most others don't really know JavaHL :-(
> >>
> >
> > Why don't we ask Vladimir to commit patches to the branch and then ask
> > for a +1 to cherry-pick them to trunk?
> >
> > He could even maintain a STATUS file in the branch of revisions
> > nominated for cherry picking...
>
> So you're suggesting a change in policy from the traditional method of
> committing stuff directly to trunk (after the requisite +1 from a full
> committer, if required)?
>
I'm just trying to get out of Vladimir's way. Via the "branch STATUS
file" approach he can parallelize (a) his work, (b) merging it ---
having N merge threads open would be harder to track.
He'll still have to work with us, just differently.
Daniel
> I think the dev@ + review technique we've historically employed is
> better for a couple of reasons. Firstly, it ensures stuff gets to
> trunk by the quickest route possible, and encourages the contributor
> to work with the community. Second, it also ensures that the code
> will be released, even if the branch doesn't pan out. Thirdly, I
> think it gives the contributor more visibility, and hopefully
> encourages reviewers to offer the relevant commit access more rapidly.
>
> In light of those reasons, I still prefer Vladimir to submit generally
> relevant patches to dev@ for commit to trunk, rather than commit to
> the branch and pull them from there. (And this isn't specific to just
> this instance of course.)
>
> -Hyrum
>
>
> --
>
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com/
Re: svn commit: r1343456 - in /subversion/branches/javahl-ra/subversion/bindings/javahl/native:
RevpropTable.cpp RevpropTable.h
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Tue, May 29, 2012 at 4:11 PM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Greg Stein wrote on Tue, May 29, 2012 at 16:28:19 -0400:
>> I would suggest any logical change that can apply to trunk should be
>> submitted to dev@. If it helps trunk, or is at least neutral, then I'd
>> support it.
>>
>> We couldn't digest your initial 27 patches :-), but some minor ones showing
>> up should be just fine. I would guess you'll be looking for a +1 from Mark
>> or Hyrum. Most others don't really know JavaHL :-(
>>
>
> Why don't we ask Vladimir to commit patches to the branch and then ask
> for a +1 to cherry-pick them to trunk?
>
> He could even maintain a STATUS file in the branch of revisions
> nominated for cherry picking...
So you're suggesting a change in policy from the traditional method of
committing stuff directly to trunk (after the requisite +1 from a full
committer, if required)?
I think the dev@ + review technique we've historically employed is
better for a couple of reasons. Firstly, it ensures stuff gets to
trunk by the quickest route possible, and encourages the contributor
to work with the community. Second, it also ensures that the code
will be released, even if the branch doesn't pan out. Thirdly, I
think it gives the contributor more visibility, and hopefully
encourages reviewers to offer the relevant commit access more rapidly.
In light of those reasons, I still prefer Vladimir to submit generally
relevant patches to dev@ for commit to trunk, rather than commit to
the branch and pull them from there. (And this isn't specific to just
this instance of course.)
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/