RE: [PATCH v2] doc: add triangular workflow

From: ALBERTIN TIMOTHEE p1514771 <timothee.albertin@etu.univ-lyon1.fr>
Date: Dec 15, 2017, 15:18 UTC
Message-ID: <1513354712419.77557@etu.univ-lyon1.fr>
In-Reply-To: <1547311095.1194033.1513263844281.JavaMail.zimbra@inria.fr>

Show 14 quoted lines

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

Show 5 quoted lines

>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.

Show 5 quoted lines

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

>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.<branch>.remote` and `branch.<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.

Show 5 quoted lines

>> +Example with master as <branch>:
>> +===================================
>> +* `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.

Show 6 quoted lines

>> +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.

Show 10 quoted lines

>> +=================================
>> +`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