RE: [PATCH v2] doc: add triangular workflow

[ALBERTIN TIMOTHEE p1514771]

>>> +This workflow is commonly used on different platforms like BitBucket,
>>> +GitHub or GitLab which provide a dedicated mechanism for requesting merges.
>>
>> As a user, I find it terribly frustrating when reading documentation
>> telling me that "there's a dedicated mechanism" for something without
>> telling me actually how to do it.

>Also I think the description is backwards from end-user's point of
>view.  It's not that it is common to use the workflow on these
>hosting sites.  It's more like people use the workflow and adopt use
>of these hosting sites as one building block of the workflow.

I'm wondering if this sentence is really useful. It's not essential
and it will take lot of time and space to talk about it properly.
So, if you agree, we'll erase it.

>>> +If **PUBLISH** doesn't exist, a contributor can publish his own repository.
>>> +**PUBLISH** contains modifications before integration.

>I am not sure what this really means.  Isn't it the norm to create a
>place to publish your work (and only your work) for your own use?
>IOW, the above two lines solicit questions like "So... what happens
>when publish does already exist??? and where does that publish
>repository come from???"

In the case of a triangular workflow, if the project already exists,
PUBLISH will already exist too. However, if the project doesn't exist
it is at the creator charge to create it. In fact, we just explain
how doing it if the project already exist. We'll add it for the 
second case.
BTW, the line : * `git push`   is useless.


Thank you for the review

Timothée Albertin

RE: [PATCH v2] doc: add triangular workflow

[ALBERTIN TIMOTHEE p1514771]

>> +
>> +
>> +--   -
>> +| UPSTREAM   |  maintainer   | PUBLISH   |
>> +||- - - - - - - -|   |
>> +--  <-   -
>> +  \ /
>> +   \   /
>> +fetch | \ / ^ push
>> +  v  \   /  |
>> +  \ /
>> +   -
>> +   |   LOCAL   |
>> +   -

>This kind of diagram deserves a bit of text to explain the situation.
>For example, LOCAL is local only for the contributor (the maintainer
>doesn't need to know about it for example). I'd add a sentence to
>explain that this gives the overall view on the flow, from the point
>of view of a contributor.

Ok, we'll do that

>> +* `git push`

>This will push to UPSTREAM, right?

Yes, we will specify it.

>> +Adding **UPSTREAM** remote:
>> +
>> +===
>> +`git remote add upstream `
>> +===

>In which circumstance shall one write this? If you don't say it, the
>reader will probably assume that this is to be done after the commands
>you specified right above. But then: it doesn't make sense. You've
>just cloned from UPSTREAM, you already have the UPSTREAM remote.

Indeed, we just remove it.

>> +For each branch requiring a triangular workflow, set
>> +`branch..remote` and `branch..pushRemote` to set up
>> +the **UPSTREAM** and **PUBLISH** repositories.

>This neither tells me how to set the variables, nor what the effect
>will be ("set up"?).

We'll fix that in the next patch.

>> +Example with master as :
>> +===
>> +* `git config branch.master.remote upstream`
>> +* `git config branch.master.pushRemote origin`
>> +===

>origin is the remote you've cloned from. From the text above, I guess
>you meant it to point to PUBLISH. But all the examples "git clone" you
>gave are from UPSTREAM.

>You're mixing the case where one "git clone"s from UPSTREAM and "git
>remode add"s PUBLISH, and the converse. Both are possible, but the
>"origin" remote will be different depending on which one you chose.

I think I don't really get it. IMHO UPSTREAM is name from the repository
you pull from and PUBLISH from the repositiry you push to.

>> +Making your work available
>> +~~
>> +
>> +The `git push` command sends commits to the **PUBLISH** repository and not 
>> to
>> +the **UPSTREAM** thanks to the configuration you did earlier with the
>> +`git config remote.pushdefault origin` command.

>This explanation should be next to the place where you recommend
>setting remote.pushdefault.

Done.

>> +When a contributor pushes something, the `git config push.default
>> +current` command can be used to specify that the name of the
>> +**PUBLISH** branch is the same as the name of the **LOCAL** one.

>I already said it multiple times, but I don't think it's a good idea
>to recommend changing push.default. The default, "simple", was
>specifically designed to be appropriate for triangular workflow:

  
>http://git.661346.n2.nabble.com/PATCH-0-6-push-default-in-the-triangular-world-td7589907.html
  >(PATCH 3/6 in particular)

>You may disagree with me, but then please explain your motivation (by
>replying to my messages and/or by explaining the rationale in the
>commit message).

I read this discussion and so I understand the point here. I agree we
shouldn't recommend this.

>> +=
>> +`git rev-parse --abbrev-ref @{push}`
>> +=
>> +
>> +.Display the fetch remote's name:
>> +[caption="Recipe: "]
>> +
>> +===
>> +`git rev-parse --abbrev-ref @{upstream}`
>> +===

>I don't think "rev-parse" is the best example to give.

>I use @{upstream} all the time to see what commits I have which aren't
>in upstream yet:

  >git log @{upstream}..

git log seems a better exemple.

We are ok we the rest of the review


Thank you for your time

Timothée Albertin

RE: [PATCH 1/2] Documentation about triangular workflow

[ALBERTIN TIMOTHEE p1514771]

Daniel Bensoussan  writes:

>> +TRIANGULAR WORKFLOW
>> +---
>> +
>> +Introduction
>> +
>> +
>> +In some projects, contributors cannot push directly to the project but
>> +have to suggest their commits to the maintainer (e.g. pull requests).
>> +For these projects, it's common to use what's called a *triangular
>> +workflow*:
>> + ...
>> +Motivations
>> +~~~
>> +
>> +* Allows contributors to work with Git even if they don't have
>> +write access to **UPSTREAM**.
>> +
>> +Indeed, in a centralized workflow, a contributor without write access
>> +could write some code but could not send it by itself.  The contributor
>> +was forced to create a mail which shows the difference between the
>> +new and the old code, and then send it to a maintainer to commit
>> +and push it.  This isn't convenient at all, neither for the
>> +contributor, neither for the maintainer. With the triangular
>> +workflow, the contributors have the write access on **PUBLISH**
>> +so they don't have to pass upon maintainer(s).  And only the
>> +maintainer(s) can push from **PUBLISH** to **UPSTREAM**.
>> +This is called a distributed workflow (See "DISTRIBUTED WORKFLOWS"
>> +above).

>I probably should not be judging if these additions to
>gitworkflows.txt is a good idea in the first place without seeing
>any explanation as to why this patch is here, but I think it misses
>the place where "triangular" sits in a larger picture.

There already have been a discussion about this documentation:
https://public-inbox.org/git/e83a9439-54c8-4925-8ee3-6aeedd941...@grenoble-inp.org/
We forgot to add it to the commit message, it will be in the next
commit message.

>The workflow to contrast against to illustrate the motivation is a
>centralized workflow, where everybody pushes their updates to a
>single place.  It does have problems inherent to its structure
>(e.g. "review before integration" is much harder, if possible), and
>also has its merits (e.g. it is simpler to explain and reason
>about).

>If you want to wean a project off of the centralized model, you'd
>need to use the "distributed workflow".  The workflow to review and
>apply mailed patches in public, and the workflow to have the project
>pull from many publish repositories individual contributor has, are
>two that allows the project to go distributed.  These two are
>complementary choices with pros and cons, and it is not like one is
>an improvement of the other.  Projects like the kernel even uses
>hybrid of the two---the patches are reviewed in public at central
>places (i.e. subsystem mailing lists) in an e-mail form and go
>through iterations getting polished, and the polished results are
>collected by (sub)maintainers and sent upwards, either as a request
>to pull from publish repositories maintained by (sub)maintainers, or
>relayed again in e-mail form (the last mile being e-mail primarily
>serves as a transport vehicle for changes proven to be good, not as
>material to be further reviewed).

>The reason why projects make these choices is because there are pros
>and cons.  A large collection of changes is far easier to integrate
>with one command (i.e. "git pull") and with a need to resolve merge
>conflicts just once, than applying many small changes as e-mailed
>patches, having to resolve many conflicts along the way.  In order
>to ensure quality of the individual changes, however, the changes
>need to be reviewed and polished, and the reality of the life is
>that there are far fewer people who are qualified to adequately
>review and help polishing the changes than those who make changes.
>Asking reviewers to go to different repositories (whose number
>scales with the number of contributors) and leave comments in the
>webforms is much less efficient and more costly for the project
>overall, than asking them to subscribe to relevant mailing lists
>(whose number scales only with the number of areas of interest) and
>conduct reviews there.  Other factors like "offline access" also
>count when considering the two models as "choices".

>As long as the document uses phrases like "forced to", "isn't
>convenient at all", etc., it is clear that it starts from a wrong
>premise, "one is an improvement over the other".

We will take this into account.
We didn't know there were hybrid workflows.

Thank you for your time

Timothée Albertin

RE: [PATCH 1/2] Documentation about triangular workflow

[ALBERTIN TIMOTHEE p1514771]

>On 17 November 2017 at 17:07, Daniel Bensoussan
> wrote:
>> +- If the maintainer accepts the changes, he merges them into the
>> +  **UPSTREAM** repository.

>Personally, I would prefer "they" and "their" over "he" and "his". I'm
>not sure how universal this preference is, but see also 715a51bcaf (am:
>counteract gender bias, 2016-07-08). I realize that "he" is already used
>in this document...

This could be a good thing in order to be neutral.  We can change this in
the whole file.

>> + ... The contributor
>> +was forced to create a mail which shows the difference between the
>> +new and the old code, and then send it to a maintainer to commit
>> +and push it.  This isn't convenient at all, neither for the
>> +contributor, neither for the maintainer.

>"neither ... nor". That said, I find the tone of this paragraph a bit
>value-loaded ("forced ... isn't convenient at all"). It does sort of
>contradict or at least compare interestingly with how git.git itself is
>maintained. ;-) Maybe this could be framed in a more neutral way?

Junio C Hamano told us the same thing about the motivation
section, we'll change it the next patch.

>> +The goal of the triangular workflow is also that the rest of the
>> +community or the company can review the code before it's in production.
>> +Everyone can read on **PUBLISH** so everyone can review code
>> +before the maintainer(s) merge it to **UPSTREAM**.  It also means

>I think you can drop the "(s)". Your example workflow could have a
>single maintainer. In a multi-maintainer workflow, the workflow would
>still be the same. So no need to cover all bases by sprinkling "(s)" on
>the text. (IMHO.)

We'll fix that.


Thank you for your review.

Timothée Albertin