[llvm-dev] Workflow to commit changes using git alone (?)

[Mehdi AMINI via llvm-dev]

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>