New committing policies

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

New committing policies

Nick Hall

Devs,

Back on 7 Dec 2015, John Ralls suggested the following:

1. Pull requests must be code-reviewed and tested by a developer.
2. Pull requests should *not* be merged from inside of github: They should be pulled to a new branch on the developer’s machine; that branch should be created on the same base commit as the PR was, then rebased to the target branch’s HEAD, then merged. That’s because the github merge button does a no-ff merge, and...
3. Most changes should be fast-forwarded. Only major new features should be merged no-ff to maintain their development identity. This applies both to PRs and work by developers who can push themselves.
4. PR authors should be assumed not to be git experts, so its up to the Gramps developer doing the merge to ensure that the PR is clean and won’t make a mess of history.
5. Bug fixes must be committed to the current stable branch and cherry-picked to master (though I’d prefer that we just periodically merge the maintenance branch into master. Cherry-picking loses the original context of the commit.). Only new features should be committed to master directly.

I agree with all of these points and think that we should now make them our policy.  I also suggest that we periodically merge the maintenance branch into master, rather than cherry-picking.

Does anyone have any objections?

Regards,


Nick.



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: New committing policies

prculley
As a relatively new developer who nearly always uses PRs, I am in favor of their use.  It has saved me multiple times between helping spot bad rebases and other git issues, the Travis tests and comments.  So yes to #1.

#2, I note that GitHub does allow a rebase and merge with ff https://help.github.com/articles/about-pull-request-merges/#rebase-and-merge-your-pull-request-commits.
Seems like if it works like advertised, it would save some steps...

#3 Good idea

#4 We sure have plenty of experience indicating that this is a good idea.

#5 I currently do most of my debugging and fixes in master branch first; often the bugs have interactions with other changes in master, and sometimes they have been fixed in master.  I also have been selectively cherry-picking from my bug fix branch based on master back to another bug fix branch based on gramps42, depending on if the bug is significant enough to be worth the work, using my judgement in the matter.

I note that cherry-picking in either direction is often quite difficult.  Between changes to master for pylint, other fixes and enhancements, some API name or deprecation changes etc. you have to be really careful when doing this.  It helps a lot to know what bugs you are fixing when trying to test the changes in both code branches.

I suspect that a 'periodic merge' would have the same issues, except that the person doing the merge would not be familiar with the specific changes, so would have an even more difficult time making sure the changes did not screw something up.

So my recommendation would be to NOT do #5 periodic merge, and instead request that each developer do the dual bug fixes, one for each branch, and submit both as separate PRs.

Paul Culley

On Sun, Apr 2, 2017 at 12:23 PM, Nick Hall <[hidden email]> wrote:

Devs,

Back on 7 Dec 2015, John Ralls suggested the following:

1. Pull requests must be code-reviewed and tested by a developer.
2. Pull requests should *not* be merged from inside of github: They should be pulled to a new branch on the developer’s machine; that branch should be created on the same base commit as the PR was, then rebased to the target branch’s HEAD, then merged. That’s because the github merge button does a no-ff merge, and...
3. Most changes should be fast-forwarded. Only major new features should be merged no-ff to maintain their development identity. This applies both to PRs and work by developers who can push themselves.
4. PR authors should be assumed not to be git experts, so its up to the Gramps developer doing the merge to ensure that the PR is clean and won’t make a mess of history.
5. Bug fixes must be committed to the current stable branch and cherry-picked to master (though I’d prefer that we just periodically merge the maintenance branch into master. Cherry-picking loses the original context of the commit.). Only new features should be committed to master directly.

I agree with all of these points and think that we should now make them our policy.  I also suggest that we periodically merge the maintenance branch into master, rather than cherry-picking.

Does anyone have any objections?

Regards,


Nick.



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: New committing policies

Luigi Toscano
Paul Culley ha scritto:

> #5 I currently do most of my debugging and fixes in master branch first; often
> the bugs have interactions with other changes in master, and sometimes they
> have been fixed in master.  I also have been selectively cherry-picking from
> my bug fix branch based on master back to another bug fix branch based on
> gramps42, depending on if the bug is significant enough to be worth the work,
> using my judgement in the matter.
>
> I note that cherry-picking in either direction is often quite difficult.
> Between changes to master for pylint, other fixes and enhancements, some API
> name or deprecation changes etc. you have to be really careful when doing
> this.  It helps a lot to know what bugs you are fixing when trying to test the
> changes in both code branches.
>
> I suspect that a 'periodic merge' would have the same issues, except that the
> person doing the merge would not be familiar with the specific changes, so
> would have an even more difficult time making sure the changes did not screw
> something up.
>
> So my recommendation would be to NOT do #5 periodic merge, and instead request
> that each developer do the dual bug fixes, one for each branch, and submit
> both as separate PRs.

I would advise to use the periodic merge instead. The more you merge, the less
problems you hit when you merge again. And even if you are not familiar with a
specific change, if you are a regular developer, it's not difficult to
understand the changes from git log. Of course, chicken and egg: the less you
do it, the more it looks like something difficult and you won't do it.

Ciao
--
Luigi


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: New committing policies

Paul Franklin-5
In reply to this post by prculley
My online access is somewhat limited right now, so I
apologize for this tardy reply to Nick's request for
comments.

Most of this proposed policy seems to be concerned with pull
requests.

Since I have never made any pull request and do not plan to
do so, whatever is changed would not seem to affect my
behavior in that regard.

I have committed a few (less than five) pull requests by
others, from inside github, but I understand that is not
desired and will not do so again, if that is the new policy.

The one time I tried downloading a pull request, and then
merging it locally, and then uploading the result, it did
not turn out so well -- so I do not plan to do that again
either.  So if this new policy is implemented I assume that
others will take care of pull requests, presumably people
who understand such things better than I do.  So that takes
care of one and two, and four.

It seems to me that the last time the topic of number three
was discussed I was told that however I was committing
things and then uploading them to github, that the result
was correct.  I think I recall that the "no-ff" way required
a specific flag to be set, whereas the default was to
fast-forward it, so that was apparently what I am doing.  So
number three won't impact me either.

As far as number five is concerned, certainly "bug fixes
must be committed to the current stable branch."  That has
always been our policy, to the best of my knowledge.

Although obviously if the bug is only in the development
branch ("master"), it only gets fixed there.  Otherwise, it
has likewise always been our policy that the bug fix should
be put into the development branch also.  And that "new
features should be committed to master."

So I certainly agree those should continue.  I just object
to the proposed methods of doing so.  I think the method
should be left up to the developer.  I don't think he should
be forced to "cherry-pick" or "merge the maintenance branch
into master" (whatever that means, since I don't know).

I have fixed a lot of bugs over the years and have noticed
it is often the case that the area of the fix is slightly
different in master, compared to the maintenance branch.

Paul pointed this out also, and it could certainly be argued
that people doing a lot of bug fixes might have a somewhat
deeper realm of experience to base an opinion on.

As far as gramps42 and master are concerned, all trailing
spaces were removed from master for instance.  Likewise
there have been changes made to master for pylint reasons,
which I don't think anybody is proposing be done in gramps42
also.  And of course the usual feature addition and
refactoring has taken place in master, and presumably always
will, so that's yet another way they drift apart.

So I think some human intelligence and judgment is required,
to make whatever the change is, between gramps42 and master,
rather than requiring some mechanical, rigid protocol.

My workflow produces a file of changes (typically from "git
diff"), which I then transfer from one machine to another,
before I commit it there.  But almost always one file is
never enough.  I almost always have to prepare two such
files, one for the maintenance branch and one for the
development branch, slightly different from each other.

So I counter-propose that as far as number five is concerned
that it be left up to the developer, how to make sure that
the bug fix gets applied to the development branch also.

I have no objections if we want to force anybody submitting
a pull request for the maintenance branch to also submit a
second one for the development branch.  Or we can leave that
up to the developer who will be reviewing and committing it.

But it certainly crosses my mind that if we make the
requirements for people submitting pull requests to be too
onerous, we may reduce the number of such things we get.  I
have no opinion as to whether we should want that or not.

I suppose there is a tradeoff, since enthusiastic people
trying to help might not understand some of the subtleties
involved in whatever they are trying to do.  I would guess
that is inherent with accepting pull requests from others.

But as I recall getting such pull requests was one of the
arguments used when it was proposed that we move to github,
to broaden the pool of people improving gramps.  So I assume
the people who were in favor of the move to github (not me)
are willing to handle such pull requests, from such people.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: New committing policies

Luigi Toscano
Paul Franklin ha scritto:

>
> As far as number five is concerned, certainly "bug fixes
> must be committed to the current stable branch."  That has
> always been our policy, to the best of my knowledge.
>
> Although obviously if the bug is only in the development
> branch ("master"), it only gets fixed there.  Otherwise, it
> has likewise always been our policy that the bug fix should
> be put into the development branch also.  And that "new
> features should be committed to master."
>
> So I certainly agree those should continue.  I just object
> to the proposed methods of doing so.  I think the method
> should be left up to the developer.  I don't think he should
> be forced to "cherry-pick" or "merge the maintenance branch
> into master" (whatever that means, since I don't know).

But this is exactly what would be better to do in a consistent way. I fear
that there is a bit much of concerns about using this workflow which are not
reflected by the reality of using it.

>
> I have fixed a lot of bugs over the years and have noticed
> it is often the case that the area of the fix is slightly
> different in master, compared to the maintenance branch.
>
> Paul pointed this out also, and it could certainly be argued
> that people doing a lot of bug fixes might have a somewhat
> deeper realm of experience to base an opinion on.
>
> As far as gramps42 and master are concerned, all trailing
> spaces were removed from master for instance.  Likewise
> there have been changes made to master for pylint reasons,
> which I don't think anybody is proposing be done in gramps42
> also.  And of course the usual feature addition and
> refactoring has taken place in master, and presumably always
> will, so that's yet another way they drift apart.
>
> So I think some human intelligence and judgment is required,
> to make whatever the change is, between gramps42 and master,
> rather than requiring some mechanical, rigid protocol.

This is true, but constantly merging does not mean that all features goes

Also, after the first time you solve a specific conflict with a merge, that
conflict does not appear anymore in the following merges. So you will have to
handle only the fix that you pushed to the stable branch. The git merge
process provides you the mean to solve the conflict, and adapt the fix to the
destination branch. This is exactly the human judgement that you advocated,
with the added bit that git knows about the previous merges and can help
finding obvious changes (like a bit of code which moved few lines, or so).

>
> My workflow produces a file of changes (typically from "git
> diff"), which I then transfer from one machine to another,
> before I commit it there.  But almost always one file is
> never enough.  I almost always have to prepare two such
> files, one for the maintenance branch and one for the
> development branch, slightly different from each other.

Did you ever try to produce the changes for the two branches by using the help
of git?

>
> So I counter-propose that as far as number five is concerned
> that it be left up to the developer, how to make sure that
> the bug fix gets applied to the development branch also.

As long as there is someone doing periodic merges of stable into master, it's
not a big trouble, but:
- it would be better if everyone get familiar with this; it's really less
complicated than it appears;
- the history will still be messy with duplicated commits which are really the
same.

--
Luigi

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: New committing policies

Paul Franklin-5
On 4/5/17, Luigi Toscano <[hidden email]> wrote:
> As long as there is someone doing periodic merges of stable into master,
> it's
> not a big trouble, but:
> - it would be better if everyone get familiar with this; it's really less
> complicated than it appears;
> - the history will still be messy with duplicated commits which are really
> the
> same.

Well, you certainly know more about git than I do.

But I think I'm as entitled to my opinion as you are of yours.

It's just that our opinions are different, based on our personal
different histories and knowledge.  That's fine.  it's a big world.

I am not interested in carrying on a long discussion of the
various features and advantages of git.  As far as I can tell
that discussion has already happened, back when we all
had to decide whether to remain with svn on SourceForge
or to change to git.  And then again when we discussed if
we should move to github from SourceForge.

I will offer my personal opinion that I think it is a mistake to
conflate the "history" of gramps42 with the "history" of the
master branch.  I don't see that it matters much what the
"history" of gramps42 is, since by definition it has been forked
(or whatever) from the main branch.  The main branch is
what contains the "history" of gramps -- IMHO.

I see no reason or advantage in keeping them closely linked,
since after the fork things are done to master which will never be
done to gramps42.  And we already have a policy which says
that bug fixes going into gramps42 should also be done to
master, and that no features or enhancements should go into
gramps42.

But as I said, I don't intend to try to convert you to the way I
think.  You are certainly entitled to think whatever you want.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: New committing policies

Luigi Toscano
Paul Franklin ha scritto:

> On 4/5/17, Luigi Toscano <[hidden email]> wrote:
>> As long as there is someone doing periodic merges of stable into master,
>> it's
>> not a big trouble, but:
>> - it would be better if everyone get familiar with this; it's really less
>> complicated than it appears;
>> - the history will still be messy with duplicated commits which are really
>> the
>> same.
>
> Well, you certainly know more about git than I do.

I certainly can't say.

> But I think I'm as entitled to my opinion as you are of yours.
>
> It's just that our opinions are different, based on our personal
> different histories and knowledge.  That's fine.  it's a big world.

Of course. This does not mean that I agree on all policies that I follow when
I contribute to a project, even when I follow it. Sometime I even start
understanding them after a while.

> I am not interested in carrying on a long discussion of the
> various features and advantages of git.  As far as I can tell
> that discussion has already happened, back when we all
> had to decide whether to remain with svn on SourceForge
> or to change to git.  And then again when we discussed if
> we should move to github from SourceForge.

Different time, different knowledge of the tools.

>
> I will offer my personal opinion that I think it is a mistake to
> conflate the "history" of gramps42 with the "history" of the
> master branch.  I don't see that it matters much what the
> "history" of gramps42 is, since by definition it has been forked
> (or whatever) from the main branch.  The main branch is
> what contains the "history" of gramps -- IMHO.

On the other side, a bugfix is the same entity which is propagated through the
branches. With a policy of merging upwards you maintain this link.


>
> I see no reason or advantage in keeping them closely linked,
> since after the fork things are done to master which will never be
> done to gramps42.  And we already have a policy which says
> that bug fixes going into gramps42 should also be done to
> master, and that no features or enhancements should go into
> gramps42.

So let's move to the technical point here: if some developers here would end
up implementing this policy, would it fine for you? Your commits will just
show as duplicate in the two branches.

--
Luigi


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: New committing policies

Paul Franklin-5
On 4/7/17, Luigi Toscano <[hidden email]> wrote:
> So let's move to the technical point here: if some developers here would
> end
> up implementing this policy, would it fine for you? Your commits will just
> show as duplicate in the two branches.

I suppose it depends on what you mean by "implementing
this policy" (since as I hope I have made clear, I do not claim
to understand what you are arguing for).

If "this policy" doesn't impact me, does not require me to
change anything I do, does not force me into doing an action
I am unfamiliar with and which thus may lead to errors, lets
me go on committing one change to gramps42 and another
(similar but not identical) change to master, then I would
guess it would not affect me.

However, I'm certainly not willing to say that something will
be "fine" with me when I don't understand what it entails.

If it will cause me to have to change what I do, for some
marginal claimed-positive gain which I can't get excited about,
even if I could understand it, then no, it wouldn't be "fine" with
me.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: New committing policies

Luigi Toscano
Paul Franklin ha scritto:
> On 4/7/17, Luigi Toscano <[hidden email]> wrote:
>> So let's move to the technical point here: if some developers here would
>> end
>> up implementing this policy, would it fine for you? Your commits will just
>> show as duplicate in the two branches.
>
> I suppose it depends on what you mean by "implementing
> this policy" (since as I hope I have made clear, I do not claim
> to understand what you are arguing for).


It means no change to your commit policy.

--
Luigi

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: New committing policies

Paul Franklin-5
On 4/9/17, Luigi Toscano <[hidden email]> wrote:
> It means no change to your commit policy.

Well, if it won't affect me, and since I proposed days ago
that IMHO the right thing was to let every developer do what
he wants, it's a little hard for me to understand why you had
this long discussion with me.  Especially since I said I was
not interested in having a long discussion.

Still, I'm glad that it's over.  (I hope it's over.)  I woke up in
the night this morning and wasn't able to get back to sleep
for two hours, thinking about you and this discussion.

Still, for the record, while I /don't/ think whatever you want done
will be something drastic -- for instance some developer doing
a delete of every commit I make and then doing them over "the
right way" -- let me say for the record that if something like that
happens I will still be very upset, even if "it means no change to"
my "commit policy."

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: New committing policies

Luigi Toscano
Paul Franklin ha scritto:
> On 4/9/17, Luigi Toscano <[hidden email]> wrote:
>> It means no change to your commit policy.
>
> Well, if it won't affect me, and since I proposed days ago
> that IMHO the right thing was to let every developer do what
> he wants, it's a little hard for me to understand why you had
> this long discussion with me.  Especially since I said I was
> not interested in having a long discussion.

I only answered trying to answer question raised in the discussion, probably
more for the other readers of the thread who had less knowledge of the context
and could think that the proposed change is more complicated that it really is.


> Still, I'm glad that it's over.  (I hope it's over.)  I woke up in
> the night this morning and wasn't able to get back to sleep
> for two hours, thinking about you and this discussion.

I'm really sorry about that, this is certainly not what I wanted to communicate.
If you really don't want to go to this workflow, that's probably fine, but
still I didn't hear the answer from who proposed this change.
There is more work to do for the other people who are going to do implement it
and it would be better if it is an exception more than rule, and this is all
what I wanted as I tried to explain previously.



>
> Still, for the record, while I /don't/ think whatever you want done
> will be something drastic -- for instance some developer doing
> a delete of every commit I make and then doing them over "the
> right way" -- let me say for the record that if something like that
> happens I will still be very upset, even if "it means no change to"
> my "commit policy."

That would be a worse cure than the disease, and it should be clear from the
goal (commit once, forward port the change). Please follow this: you don't
remove commits in git, so if this revert would happen, the history would show
both your commit and the revert commit in the destination branch. It would be
definitely more messy.

--
Luigi


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: New committing policies

Nick Hall
In reply to this post by Luigi Toscano
On 02/04/17 20:20, Luigi Toscano wrote:
> I would advise to use the periodic merge instead. The more you merge, the less
> problems you hit when you merge again. And even if you are not familiar with a
> specific change, if you are a regular developer, it's not difficult to
> understand the changes from git log. Of course, chicken and egg: the less you
> do it, the more it looks like something difficult and you won't do it.

Given the other comments in this thread, perhaps we should wait until
the gramps50 branch is created before introducing a policy of periodic
merges into the master branch.  The initial merge will be easier when
the maintenance branch is almost identical to the master branch.  Until
then, it will remain the responsibility of the developer to ensure that
bug fixes in the maintenance branch are also committed to the master branch.

I have updated the committing policies:

https://gramps-project.org/wiki/index.php?title=Committing_policies

Let me know if you have any further comments or suggestions.


Nick.



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Gramps-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gramps-devel
Loading...