[mantisbt-dev] Github / Pull request spam

[Damien Regad]

Paul,

Can we please have a SINGLE branch/pull request for a feature/fix, with
several commits in it, instead of a series of pull requests each having
a single commit ?

I'm referring to:
- "Following merge of #216 - update calls to db_get_table()"
==> 14 pull requests (not counting #260 which is more of the same)
- "Remove use of AS keyword from sql query"
==> 3 pull requests

This basically multiplies the effort for you to generate the individual
branches, and later on merge them, as well as for the reviewers: 17
distinct reviews and "+1" posts - assuming there are no issues - instead
of 2 in this case.

Please reword also the commit messages:

- it is useless to have each commit begin with "Following merge of #216"
(no added value in in terms of information on what the commit is about,
and "#216" will be interpreted as a Mantis bug ID when in fact it's a
Github pull request)
- please indicate *which table is being updated* - just think for a
second at how the git log will look like...

It would be much better instead, to have each commit's summary be
"update db_get_table() calls for table XXX"

Also, can we please have meaningful descriptions for the pull requests?
"Backport <SHA>" is useless really, particularly when the SHA is from
your own fork and does not exist in the mantisbt repository.

Finally, how can something be a "backport" when master is the latest
head (and has never been released) ?

Kindly fix the above, I'll review things then.

Thanks for your understanding.


---
This email is free from viruses and malware because avast! Antivirus protection is active.
http://www.avast.com

[P Richards]

Hi Damien,

As I said to Victor, it's deliberately split up by table to ensure ease of
updating if we choose not to merge early on.

I've been over the code base doing the db_get_table replacement 5 different
times now - each time basically taking 5-6 hours of effort on my part. So
that's about 30 hours combined of coding.

In each case up until now, for one reason or another, I've ended up redoing
that work.

By splitting the the db_table replacements by table, yes, I'll have to
manage 20 individual branches until we decide to merge them, however the
work to manage those will be less the time spent starting from scratch each
time.

In addition, given that when I've done this on previous occasions, we've
ended up with a table getting swapped incorrectly (as some of our variables
for table names used to be inconsistent in a couple of cases), it's actually
easier to review to ensure that all the table changes are the same in the
PR. i.e. they all reference bug_text and a {bug} doesn't creep in.

In terms of,
"Backport <SHA>" is useless really, particularly when the SHA is from your
own fork and does not exist in the mantisbt repository."

I don't have anything in my own repository that does not exist in the
mantisbt repository. I often reset the contents of my remote and reclone it,
so any code with the word backport is something that has been previously
pushed to the mantisbt repository, often reviewed by atrol but does not
exist in the current master.

I will update the naming of the commits to indicate the table in the initial
summary line as you request.

In terms of the AS keyword being 3 PR's, we generally do not make use of AS
for db compatibility. I spotted them individually at different times hence
multiple PR's. Those got generated to save time in the future, as opposed to
going looking for them.

Paul

-----Original Message-----
From: Damien Regad [mailto:***@mantisbt.org]
Sent: 11 August 2014 16:28
To: Mantisbt-***@lists.sourceforge.net
Subject: [mantisbt-dev] Github / Pull request spam

Paul,

Can we please have a SINGLE branch/pull request for a feature/fix, with
several commits in it, instead of a series of pull requests each having a
single commit ?

I'm referring to:
- "Following merge of #216 - update calls to db_get_table()"
==> 14 pull requests (not counting #260 which is more of the same)
- "Remove use of AS keyword from sql query"
==> 3 pull requests

This basically multiplies the effort for you to generate the individual
branches, and later on merge them, as well as for the reviewers: 17 distinct
reviews and "+1" posts - assuming there are no issues - instead of 2 in this
case.

Please reword also the commit messages:

- it is useless to have each commit begin with "Following merge of #216"
(no added value in in terms of information on what the commit is about, and
"#216" will be interpreted as a Mantis bug ID when in fact it's a Github
pull request)
- please indicate *which table is being updated* - just think for a second
at how the git log will look like...

It would be much better instead, to have each commit's summary be "update
db_get_table() calls for table XXX"

Also, can we please have meaningful descriptions for the pull requests?
"Backport <SHA>" is useless really, particularly when the SHA is from your
own fork and does not exist in the mantisbt repository.

Finally, how can something be a "backport" when master is the latest head
(and has never been released) ?

Kindly fix the above, I'll review things then.

Thanks for your understanding.


---
This email is free from viruses and malware because avast! Antivirus
protection is active.
http://www.avast.com



----------------------------------------------------------------------------
--

[Damien Regad]

Post by P Richards
By splitting the the db_table replacements by table, yes, I'll have to
manage 20 individual branches until we decide to merge them, however the
work to manage those will be less the time spent starting from scratch each
time.

This does not make any sense.

I am not asking you to collapse everything into a single commit. Just to
have a *single branch* + pull request *with one commit per table*,
(exactly as you have them now), in-line with git best practices, i.e.

--- * --- bug --- user --- project --- ...

instead of

--- * --- bug
|
\--- user
|
\--- project
|
\--- ...

That would greatly ease the maintenance of the patch (having only one
branch to maintain / rebase = less work for you), and conflicts
generated by future commits can be fixed just as easily as with separate
branches.

Also, it makes review and testing a lot easier (only a single checkout +
tests instead of doing the same for each individual branch, and having
to redo it again when you finally decide to merge everyting)

And back to my original complain, it would drastically cut down the
level of spam generated by what you've done (I received over 50 messages
for this now, counting your last update).

Anyway, thanks for updating the commits, it's much better already; you
should also remove the last paragraph in the commit (it's a commit, not
a pull request, and with the updated summary that does not add any
useful information)

Post by P Richards
In terms of,
"Backport <SHA>" is useless really, particularly when the SHA is from your
own fork and does not exist in the mantisbt repository."
I don't have anything in my own repository that does not exist in the
mantisbt repository. I often reset the contents of my remote and reclone it,
so any code with the word backport is something that has been previously
pushed to the mantisbt repository, often reviewed by atrol but does not
exist in the current master.

I remind you that your "mantis-2.0.x" branch as well as the "next"
branch are not official, and should be (have been) moved to your / dhx's
forks respectively.

If it's not in master or master-1.x.x, *it does not officially exist*,
therefore the term "backport" (or even "port") is inappropriate.

And even if that were not the case, the fact remains that "backport
XXXX" is a meaningless commit summary that must be changed, so please
fix. Suggest you just use the message from the original commit;

git commit -c <SHA>

will do this for you.

Post by P Richards
In terms of the AS keyword being 3 PR's, we generally do not make use of AS
for db compatibility. I spotted them individually at different times hence
multiple PR's. Those got generated to save time in the future, as opposed to
going looking for them.

I'm fine with the approach of fixing things as you notice them, but
again if they are related changes, you should group them into a single
branch/PR with multiple commits. That's easy enough to do with git
(cherry-pick, etc).




---
This email is free from viruses and malware because avast! Antivirus protection is active.
http://www.avast.com

[P Richards]

Damien,

Db_table patches refreshed to hopefully cover what you were after.
Github PR's pushed to update and reflect changes

I've also taken the step of manually going through and renaming the PR's to
match the first line of the commit message so it's hopefully cleaner now.

Paul

-----Original Message-----
From: Damien Regad [mailto:***@mantisbt.org]
Sent: 11 August 2014 16:28
To: Mantisbt-***@lists.sourceforge.net
Subject: [mantisbt-dev] Github / Pull request spam

Paul,

Can we please have a SINGLE branch/pull request for a feature/fix, with
several commits in it, instead of a series of pull requests each having a
single commit ?

I'm referring to:
- "Following merge of #216 - update calls to db_get_table()"
==> 14 pull requests (not counting #260 which is more of the same)
- "Remove use of AS keyword from sql query"
==> 3 pull requests

This basically multiplies the effort for you to generate the individual
branches, and later on merge them, as well as for the reviewers: 17 distinct
reviews and "+1" posts - assuming there are no issues - instead of 2 in this
case.

Please reword also the commit messages:

- it is useless to have each commit begin with "Following merge of #216"
(no added value in in terms of information on what the commit is about, and
"#216" will be interpreted as a Mantis bug ID when in fact it's a Github
pull request)
- please indicate *which table is being updated* - just think for a second
at how the git log will look like...

It would be much better instead, to have each commit's summary be "update
db_get_table() calls for table XXX"

Also, can we please have meaningful descriptions for the pull requests?
"Backport <SHA>" is useless really, particularly when the SHA is from your
own fork and does not exist in the mantisbt repository.

Finally, how can something be a "backport" when master is the latest head
(and has never been released) ?

Kindly fix the above, I'll review things then.

Thanks for your understanding.


---
This email is free from viruses and malware because avast! Antivirus
protection is active.
http://www.avast.com



----------------------------------------------------------------------------
--