llvm dev - Nov 2019 - Workflow to commit changes using git alone (?)

> On 10 Nov 2019, at 07:00, Mehdi AMINI <joker.eph at gmail.com> wrote:
> 
>  recipe is not correct in the absolute: the delta from master does not mean
it contains exactly what you want, you seem to assume that master didn't
evolve between the time "patchbranch" was created.
> 
Hi Mehdi,

I’m doing it this way to make sure that master /actually/ contains “exactly what
I want” (!). Of course the remote master could have evolved slightly during such
steps, but that’s a cat-and-mouse game, that I don’t think that can be easily
avoided (at least to my knowledge): We use Phabricator to get patches reviewed
and this implies that new commits should be based on what we post there. If
master has evolved significantly since the review, then a patch update should be
sent to Phabricator anyway for further review. Also tests must be run again
while master continues evolving. Ultimately, the commit that gets sent to Github
must be strictly based on what we got reviewed on Phabricator. My procedure only
attempts to enforce that the ‘delta’ commit that I push is predictable, and
exactly the one that I previously posted on Phabricator.

The procedure stated on the docs  
https://llvm.org/docs/Phabricator.html#committing-a-change
<https://llvm.org/docs/Phabricator.html#committing-a-change> suggests
using ‘arcanist’ to create a ‘diferential’ branch based on master and the
Phabricator revision number. The 'arc patch’ command is used for that. To my
understanding, the problem is the same: creating that branch may take a few
seconds, and while doing so, master can evolve too. The docs even suggest
re-running the tests before pushing, which gives even more time for master to
have changed.

(Said that, maybe I’m not fully getting ‘git', because I only used it in the
past with small teams and no need for cross-reviews, so everything was pretty
straightforward: pulling what others did, merging working branches with master,
eventually resolving conflicts, and then pushing)

Since you suggest that my “recipe” is not correct, I would appreciate that you
elaborate on the correct one, which is why I opened this subject to begin with.

Thank you very much!

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191110/355be17d/attachment.html>

On Sat, Nov 9, 2019 at 11:50 PM Joan Lluch <joan.lluch at icloud.com>
wrote:
>
> On 10 Nov 2019, at 07:00, Mehdi AMINI <joker.eph at gmail.com> wrote:
>
>  recipe is not correct in the absolute: the delta from master does not
> mean it contains exactly what you want, you seem to assume that master
> didn't evolve between the time "patchbranch" was created.
>
>
> Hi Mehdi,
>
> I’m doing it this way to make sure that master /actually/ contains
> “exactly what I want” (!). Of course the remote master could have evolved
> slightly during such steps
>
I meant: your list of instructions assume that master didn't move *locally*
since you branched "patchbranch".
If you're juggling with multiple patch branches at the same time, you need
to make sure they are all rebased correctly on the current local master.
Your workflow may work very well for you, but I was pointing this out
because someone trying to adapt it can mess up their commit fairly easily
if they just follow this recipe.
Interactive rebase is much safer from this point of view.


> , but that’s a cat-and-mouse game, that I don’t think that can be easily
> avoided (at least to my knowledge): We use Phabricator to get patches
> reviewed and this implies that new commits should be based on what we post
> there. If master has evolved significantly since the review, then a patch
> update should be sent to Phabricator anyway for further review. Also tests
> must be run again while master continues evolving. Ultimately, the commit
> that gets sent to Github must be strictly based on what we got reviewed on
> Phabricator. My procedure only attempts to enforce that the ‘delta’ commit
> that I push is predictable, and exactly the one that I previously posted on
> Phabricator.
>
> The procedure stated on the docs
> https://llvm.org/docs/Phabricator.html#committing-a-change suggests using
> ‘arcanist’ to create a ‘diferential’ branch based on master and the
> Phabricator revision number. The 'arc patch’ command is used for that.
To
> my understanding, the problem is the same: creating that branch may take a
> few seconds, and while doing so, master can evolve too. The docs even
> suggest re-running the tests before pushing, which gives even more time for
> master to have changed.
>
No: the arcanist command does not suffer from the problem I was raising.
The issue I was referring to is that your reset command will lead to
*undoing* changes from master (unrelated to your branch) when you commit in
the end (all the changes that are in master but not in "patchbranch").
(just try to add `git checkout master && git pull && git
checkout tmp`
before your `git reset `, and then look at the resulting commit).

>
> (Said that, maybe I’m not fully getting ‘git', because I only used it
in
> the past with small teams and no need for cross-reviews, so everything was
> pretty straightforward: pulling what others did, merging working branches
> with master, eventually resolving conflicts, and then pushing)
>
> Since you suggest that my “recipe” is not correct, I would appreciate that
> you elaborate on the correct one, which is why I opened this subject to
> begin with.
>
Avoid `git reset` unless you are really sure that this is what you need at
a given time: I would not advise `git reset` to any beginner for a
"normal"
workflow.
Christopher mentioned a more robust set of steps for instance, and in
general `git rebase -i` is the tool: you likely want to invest in knowing
it (you mentioned you had to address conflicts, but such situation will
lead to resolve conflicts regardless of the workflow, yours included).

Best,

-- 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191110/97b6848a/attachment-0001.html>

Hi Mehdi,
> On 10 Nov 2019, at 20:27, Mehdi AMINI <joker.eph at gmail.com> wrote:
> 
> No: the arcanist command does not suffer from the problem I was raising.
> The issue I was referring to is that your reset command will lead to
*undoing* changes from master (unrelated to your branch) when you commit in the
end (all the changes that are in master but not in "patchbranch").
> (just try to add `git checkout master && git pull && git
checkout tmp` before your `git reset `, and then look at the resulting commit).
>  
But I would never do that!. The commands "git checkout master &&
git pull” are only run before the whole procedure, never in the middle. And the
patchbranch is always merged with master before starting the procedure. The
Phabricator diff is created by comparing both branches after they have merged,
and the point of reseting ’tmp' to ‘master’ is to obtain a fresh commit
containing exactly the same diff.

In any case, I would want to understand why the archaist command does not suffer
from an “evolving” master, which is the problem that you raised first. The “arc
patch” command creates a new branch from the local master, so the remote repo
can still have changed even before the actual command is completed (!) or at any
time after that. So what makes executing that command different?, or maybe I
should ask, what is the archaist command actually doing in terms of plain git
commands?

Thanks 

John
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191110/900d7116/attachment-0001.html>