Re: [Tails-project] back-and-forth amendments on a single is…

Delete this message

Reply to this message
Autore: sajolida
Data:  
To: Public mailing list about the Tails project
Oggetto: Re: [Tails-project] back-and-forth amendments on a single issue
intrigeri:
> sajolida (2020-06-03):
>> 1. syster created a MR for #17227 on:
>> 2. Cody amended their MR by creating a MR against syster's repo:
>> 3. Cody then opened another MR with the work of both:
>> 4. When reviewing Cody's MR I added some commits to the branch
>
> Wow, indeed that seems convoluted and painful. It took me a while to
> think about this, learn about some GitLab features, and sleep on it.
>
> I don't think we can make it entirely painless in the exact situation
> you 3 people were in, i.e.:
>
>  - 2 rounds of reviews done by 2 different people, including 1 person
>    who doesn't have the commit bit.

>
>  - 2 of the 3 involved people cannot push to branches in tails/tails:
>    they can only push to their own fork. So they can't collaborate on
>    shared branches in tails/tails (see below for a possible solution
>    to this problem).

>
> Below I'll point to a few features and workflow hints that may help.
>
>> 1. syster created a MR for #17227 on:
>>
>>    https://gitlab.tails.boum.org/tails/tails/-/merge_requests/8

>
> So far, so good :)
>
>> 2. Cody amended their MR by creating a MR against syster's repo:
>>
>>    https://gitlab.tails.boum.org/syster/tails/-/merge_requests/1

>>
>>    That sounds pretty convoluted to me already. Is this the proper way?

>
>> 3. Cody then opened another MR with the work of both:
>>
>>    https://gitlab.tails.boum.org/tails/tails/-/merge_requests/32

>
> I think doing both (2) and (3) is sending conflicting signals
> and creating confusion:
>
>  - (2) suggests that syster is still the "owner" of the task,
>    i.e. responsible to lead it to a state that can be merged

>
> - (3) suggests that Cody took over the task
>
> IMO one should pick only 1. Which one is better in a given situation
> is a social question rather than a technical one. Regarding how to
> implement each of them:
>
> - Either syster remains the "owner" of the task
>
>    Possible implementations:

>
>     - What Cody did with (2) could be OK'ish for obvious improvements,
>       but indeed there's a risk that the discussion gets split between
>       2 MRs.

>
>     - Cody could have suggested changes on syster's MR (!8), and then
>       syster could have accepted those suggestions into !8:
>       https://docs.gitlab.com/ee/user/discussions/index.html#suggest-changes

>
>     - Cody and syster could share a fork on which they both have write
>       access. So they could have collaborated on the original MR: Cody
>       could have pushed his changes directly on top of the initially
>       proposed ones.

>
> - Or Cody takes over the task
>
>    Possible implementations:

>
>     - Fork shared by Cody and syster (see above)

>
>     - Cody creates his own MR (!32) that supersedes !8,
>       and !8 must be closed (i.e. rejected as obsolete),
>       either by syster or by someone allowed to close arbitrary MRs.

>
>       It could be OK as long as the original MR (!8) did not have much
>       discussion, that would be lost by migrating future discussion
>       to !32.


Indeed, it's a social question.

I understand that this one would have been the best option since Cody
did a totally rewrite of syster's edit.

>> 4. When reviewing Cody's MR I added some commits to the branch:
>>    3d8f1a284f..f8a5cd8289.

>>
>>    I pushed these to doc/17227-letterboxing-tor-browser. They get
>>    automatically referenced on #17227 because I mention the issue in the
>>    commit message but they are not referenced on !32.

>>
>> My intention was to amend Cody's MR !32 with my commits and reassign to
>> him for review but it doesn't seem to be possible.
>
> Indeed, when creating !32, Cody did not tick the "Allow commits from
> members who can merge to the target branch" checkbox. I see that since
> then, Cody learned about this feature, e.g. on !58 he did tick that
> checkbox, so if you wanted to amend his branch, I believe you could
> do that \o/
>
> Here's the doc for this GitLab feature:
> https://docs.gitlab.com/ee/user/project/merge_requests/allow_collaboration.html
>
>> What am I supposed to do now? Shall I Close Cody's MR !32 anyway, hoping
>> that it won't get merged by magic and open a 4th MR for a single issue?
>
> I think you could have asked Cody to edit the MR and tick the "Allow
> commits from members who can merge to the target branch" checkbox.
>
>> Does this mean that when going back and forth with reviews and
>> amendments we have to create a new MR every time? Probably loosing
>> comments and context each time?
>
> I'm confident that the toolbox I've described above will avoid
> the need to do that :)


Yes! I didn't know about it.

Then we would have had 2 branches and 2 MRs only:

1. syster's branch in their fork
2. Cody's branch in their fork, where I would have pushed my changes

Thanks for the interesting GitLab lesson :)

--
sajolida
Tails — https://tails.boum.org/
UX · Fundraising · Technical Writing