[sakai-core-team] [Building Sakai] Question about Pull requests

[Noah Botimer]

As with many things, I see this as an "it depends" situation.

There are plenty of cases where an approach turns out wrong or there are
just plain mistakes and corrections in the branch. In these, I prefer that
the branch be rebased into a clean diff without the course corrections.

But I'm not a strict adherent to one-commit-per-branch (or
one-branch-per-issue). If the work is divided across coherent, cohesive
commits with good messages, clarity is only harmed by combining them.

So, a rule of thumb that is simple and generally safe seems the way to go
to me. There is plenty of room for experts to make special case judgment
calls. If we feel that our average developer should reasonably be familiar
with interactive rebases, force pushes, and the quirks and risks, therein,
so be it. If we feel that the general recommendation should be for no
history changes anywhere, so be it.

Because git affords a new, safer local workflow of committing early and
often (without shared clutter), I see the interactive rebase of a local
branch before push as worth recommending. If local history changes only is
as far as we want to include in the rule of thumb, I'm fine with that, but
also fine with the more complicated model of history changes in fork
feature branches. These are transactions with our collective history, so
they should be the shape and size we want them to be.

Anyway, as I opened, I think it depends -- and we should just make sure to
use the simplest acceptable pattern as the rule of thumb and make room for
special cases.

Thanks,
-Noah

On Feb 10, 2015, at 5:12 PM, Matthew Jones <matthew at longsight.com> wrote:

I think it depends, there really isn't a huge crisis if there just a a few
commits in a PR and the commits do have good messages tracked back to the
jira that indicates what specifically has changed between each commit.

I think the problem comes if someone was working on a larger feature and if
they were doing a commit very frequently and not putting much into the
message. (Or tracking it as a separate new issue) An example like in the
Stack Exchange answer that makes sense to me.

http://programmers.stackexchange.com/a/263172

Like if I was going to single out a recent SAK it would be this one
https://jira.sakaiproject.org/browse/SAK-25773

There are like 50 commits on that and I'd probably be happier if that was
all just squashed into one or two in the git logs. Taken individually there
weren't any large changes, but a ton of small ones.


On Tue, Feb 10, 2015 at 4:24 AM, Steve Swinsburg <steve.swinsburg at gmail.com>
wrote:

> I'm not a fan of squashing commits. We didn't do it in subversion and it
> can break things majorly if you choose the wrong commit, which is easy to
> do. Many gui clients don't fully support it either. IMO it is unnecessary.
>
> What you did was fine, the commit in your branch updated the pr
> automatically,  then it was merged in the pr. A pr is just a link between
> the commits in a branch and the base.
>
> Cheers,
> Steve
>
> sent from my mobile device
> On 10/02/2015 7:19 AM, "Noah Botimer" <botimer at umich.edu> wrote:
>
>> It's worth mentioning: any time you are planning to use --force,
>> double-check your work. You could probably recover from a mistake, but it
>> can be a lot of work.
>>
>> Since we are recommending that feature branches are rebased to include
>> updates, it is good to get in the habit of being explicit. That is, include
>> the remote name and branch name as a safety check, and try once before
>> forcing. The various versions of the git clients have different defaults,
>> you may not have your complete config on whatever machine you're currently
>> using, or remote tracking might not be what you assumed for a particular
>> branch.
>>
>> Use this:
>>
>> git push remote branch-name
>>  ### Look at the big, nasty error message about divergent branches
>>  ### Make sure this force is really what you want
>> git push origin branch-name --force
>>
>> Instead of:
>>
>> git push --force
>>
>> The latter will cost you karma points and major fixup time eventually,
>> dwarfing the few seconds of typing you saved with the short form.
>>
>> Thanks,
>> -Noah
>>
>> On Feb 9, 2015, at 2:55 PM, Brian Jones wrote:
>>
>> > Hey Neal,
>> >
>> >> I made a pull request off a branch from my clone of Sakai project and
>> >> then had a suggestion to change my pull request. I made the update in
>> >> my branch and Git (or Github) seems to have automatically squashed my
>> >> commits together in the one pull request, which is a good thing.
>> >
>> > Not quite. It's automatically updated the PR, but you've made several
>> > commits (3 in total), that should be squashed into 1. You can refer to
>> > step 6c in
>> https://confluence.sakaiproject.org/display/SAKDEV/Git+Setup
>> > to walk you through squashing commits.
>> >
>> >> I don't need to merge my change back into my clone of Sakai project,
>> >> just after the PR is merged, I can switch my local branch back to
>> >> origin and git pull upstream master, and then I'm all synche'd up and
>> >> all is good?
>> >
>> > Correct, after the PR is merged, you can just switch back to your master
>> > branch and do the git pull upstream master && git push origin master to
>> > both update your fork on your local machine and your fork in github.
>> >
>> > Hope this helps :)
>> >
>> > Brian Jones
>> > Programmer/Analyst
>> > Information Technology Services
>> > Support Services Building, Suite 4300
>> > Western University
>> > (519) 661-2111 x86969
>> > bjones86 at uwo.ca
>> > _______________________________________________
>> > sakai-core-team mailing list
>> > sakai-core-team at collab.sakaiproject.org
>> > http://collab.sakaiproject.org/mailman/listinfo/sakai-core-team
>>
>> _______________________________________________
>> sakai-core-team mailing list
>> sakai-core-team at collab.sakaiproject.org
>> http://collab.sakaiproject.org/mailman/listinfo/sakai-core-team
>>
>
> _______________________________________________
> sakai-dev mailing list
> sakai-dev at collab.sakaiproject.org
> http://collab.sakaiproject.org/mailman/listinfo/sakai-dev
>
> TO UNSUBSCRIBE: send email to
> sakai-dev-unsubscribe at collab.sakaiproject.org with a subject of
> "unsubscribe"
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://collab.sakaiproject.org/pipermail/sakai-core-team/attachments/20150210/8e7e4fd1/attachment-0001.html