[apparmor] [administrivia] git conversion complete; gitlab projects set up

[Tyler Hicks]

On 11/02/2017 02:07 PM, Christian Boltz wrote:
> Hello,
> 
> Am Mittwoch, 1. November 2017, 21:46:17 CET schrieb Tyler Hicks:
>> On 11/01/2017 02:41 PM, Christian Boltz wrote:
> 
>>> Another question is if we want to continue sending patches to the
>>> mailinglist, or if we'll switch over to using branches (prefixed
>>> with the username, for example "cboltz-utils-foo") and then send
>>> merge requests.
>>
>> What's the reason for the username prefix?
> 
> I should probably explain or just show the full workflow we use for 
> managing the salt code ;-)
> 
>     git checkout -b cboltz-whatever  # create a new branch
>     vi $whatever
>     git add $whatever
>     git commit
>     git push  # fails because the branch doesn't have an origin set yet, 
>               # but gives copy&paste-ready instructions with the correct 
>               # push command
>     git push --options-shown-above  # this prints a link for opening a
>               # merge request
>     # open the given link to create the merge request, and make sure to
>     # enable the option to delete the source branch after merging to 
>     # avoid having lots of superfluous branches around.
> 
> Note that we do all this in the *main repo* (for AppArmor, that would be 
> gitlab.com/apparmor/apparmor), not in a user repo like 
> gitlab.com/cboltz/apparmor.
> 
> That should also explain the reason for the username prefix in the branch 
> name - it avoids naming conflicts. (If we decide to do all merge requests 
> from gitlab.com/$user/apparmor, the name prefix is superfluous.)
> 
> Advantages of having those temporary branches in the main repo are:
> - you don't need to refresh your $user repo regularly (by pulling from
>   the gitlab.com/apparmor/apparmor repo), just run   git pull
> - it's easy to checkout a branch someone created for review
> 
> The only disadvantage I see is that having everything as branches in the 
> main repo might be a bit noisy, but that's not a real problem.

Thanks. I see why you're using the prefix now.

My preference would be to create merge requests from
gitlab.com/$user/apparmor. One example of why is that I don't do many
reviews of the Python utilities so I'd rather not spend the time pulling
down all of the pending merge requests for the Python utilities when I
simply want to fetch the latest version of the master branch or maybe a
stable branch. In the case that I do a review of a merge request for the
Python utilities, I'd add a new 'cboltz' remote and fetch a copy of your
tree at that time.

>  
>> I really enjoyed the web-based merge request workflow and think it can
>> be an improvement over the mailing list patchset based flow. However,
>> I'd strongly recommend that we require contributors to:
>>
>> 1) Create a merge request
>> 2) Receive feedback from maintainers
>> 3) If changes are required, fold changes necessary to address feedback
>> into the existing patches, rebase, and force push to their merge
>> request branch.
>>
>> #3 is necessary to avoid a bunch of fixup patches that shouldn't be
>> standalone. It also makes for an bisect-able tree since there's no
>> broken commits being merged (with separate fixup commits).
> 
> Agreed, force push makes sense in this case.

We walked through this yesterday with the following merge request:

  https://gitlab.com/apparmor/apparmor/merge_requests/2

The "Compare with previous version" feature is a killer feature. AFAIK,
GitHub is sorely missing that.

> 
>>> I also wonder how to handle the Acked-by messages in case we use
>>> merge requests - while it's possible with git rebase + using the
>>> "reword" keyword, it means we'll have to force-push to those
>>> branches before merging them.
>>
>> What the maintainer did for the GitHub contribution that I mentioned
>> above was to merge my pull request into a local branch, interactive
>> rebase to add his Signed-off-by, and then push the resulting branch to
>> to the master branch on GitHub.
> 
> As Steve already mentioned, that sounds like lots of work.
> 
> IMHO having the Acked-by only in the merge commit would be enough, at 
> least if it's the same for all patches/commits in the to-be-merged 
> branch.
> 
> Things are more interesting[tm] if I ack the first 3 patches of a series, 
> and you ack the other 4 - in this (probably rare) case, adjusting the 
> commit messages might indeed make sense.

We walked through a merge yesterday with this merge request:

  https://gitlab.com/apparmor/apparmor/merge_requests/1

The audit trail of who merged the code is implicitly present in the
merge commit. By default, there's no information about who reviewed the
changes but the merge commit contains a mention of where to find the
merge request and that page will contain much more info about who
reviewed which parts of the merge request.

I'm fine with the default merge commit message. I think Steve had an
issue with the subject line of the default merge commit message. I'll
let him voice his opposition to it and maybe he'll have a better suggestion.

Tyler

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20171102/fc8799d7/attachment.sig>