[llvm-dev] How to deal with multiple patches to the same file

[Robinson, Paul via llvm-dev]

> -----Original Message-----
> From: Paul C. Anagnostopoulos <paul at windfall.com>
> Sent: Monday, August 10, 2020 2:13 PM
> To: Robinson, Paul <paul.robinson at sony.com>
> Cc: 'llvm-dev at lists.llvm.org' <llvm-dev at lists.llvm.org>
> Subject: RE: [llvm-dev] How to deal with multiple patches to the same file
> 
> I owe you a gala dinner at your favorite restaurant. Really.
> 
> A few questions:
> 
> Why did you 'git pull --rebase' when the branch was up-to-date? Is this
> just a safety habit?

Yes.  Frequently I put my patches on my local master branch, rather 
than create a separate patch branch, and always rebasing keeps my 
commits at the HEAD of the branch.  It's harmless when you have no 
local commits, so it's a good habit to form.

> I don't understand the pushing upstream. Since we use Phabricator, isn't
> that the job of the person who commits the patch?

Soon enough (if you keep contributing) you'll get commit access and 
be doing that yourself.  LLVM is a lot more liberal about commit 
privileges than many open-source projects.

> Does git keep all my branches up-to-date with the origin? Say I come in
> tomorrow and start working on a branch. Should I make sure I do a checkout
> on it again?

Git is designed to run "disconnected" and does nothing automatically 
to keep your clone up-to-date with its origin.  You need to decide for 
yourself often to update your local master branch from origin.  I 
have a nightly cron job on Linux, and the Windows equivalent.  Some 
people do it weekly, some don't automate it and just pull manually 
whenever it's convenient.  Partly this depends on how long your build 
takes and whether you leave your computers on overnight.

Any branch you create locally will stay until you explicitly delete it.
This project does not want you pushing your branches to origin; that
will change if/when we move to pull requests, but for now, keep them
local.

> Is it sufficient to do a 'git show' and submit with Arcanist, or is there
> an intermediate step to format the patch file?

I've never used Arcanist so I don't really know, but I expect it's fine.
'git show' works for uploading directly to Phab, as would 'git diff' and
'git format-patch'.  There might be an 'arc diff' I don't know.

Note that 'git show' by default only shows you the HEAD commit; if you
add commits to your patch branch, you'll need to specify the range of
commits to diff/show/whatever.  Phab doesn't keep track, the revisions
aren't cumulative.

I should say that we want "full context" diffs in Phab, meaning for any
of these commands you should use `-U999999` so people can see all the
context directly in the GUI.

--paulr


> 
> At 8/10/2020 12:19 PM, Robinson, Paul wrote:
> >Hi Paul,
> >Here we go with a full example.  I'm using git from the command line,
> >because that's my comfort zone; you should be able to do equivalent
> things
> >from a GUI.
> >
> >Let's start with a test tweak one of my colleagues made recently.
> >We'll pretend I'm still experimenting with it.
> >------------------------------
> >D:\upstream\llvm-project>git status
> >On branch master
> >Your branch is up to date with 'origin/master'.
> >
> >Changes not staged for commit:
> >  (use "git add <file>..." to update what will be committed)
> >  (use "git restore <file>..." to discard changes in working directory)
> >        modified:   llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-
> ops.ll
> >
> >no changes added to commit (use "git add" and/or "git commit -a")
> >
> >D:\upstream\llvm-project>git diff
> >diff --git a/llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-ops.ll
> b/llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-ops.ll
> >index ffe5d8eedf4..ed3e2e6b481 100644
> >--- a/llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-ops.ll
> >+++ b/llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-ops.ll
> >@@ -1,6 +1,6 @@
> > ; REQUIRES: asserts
> >
> >-; RUN: opt -newgvn -S %s | FileChecks %s
> >+; RUN: opt -newgvn -S %s | FileCheck %s
> >
> > ; XFAIL: *
> >
> >
> >D:\upstream\llvm-project>git add llvm/test/Transforms
> >
> >D:\upstream\llvm-project>git status
> >On branch master
> >Your branch is up to date with 'origin/master'.
> >
> >Changes to be committed:
> >  (use "git restore --staged <file>..." to unstage)
> >        modified:   llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-
> ops.ll
> >
> >
> >D:\upstream\llvm-project>git checkout -b mypatch
> >Switched to a new branch 'mypatch'
> >
> >D:\upstream\llvm-project>git status
> >On branch mypatch
> >Changes to be committed:
> >  (use "git restore --staged <file>..." to unstage)
> >        modified:   llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-
> ops.ll
> >
> >
> >D:\upstream\llvm-project>git commit -m "fix bad command name"
> >[mypatch 1768f568884] fix bad command name
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >D:\upstream\llvm-project>git show
> >commit 1768f56888452f2d068eb1ee51f2bab39042808c (HEAD -> mypatch)
> >Author: Paul Robinson <paul.robinson at sony.com>
> >Date:   Mon Aug 10 08:43:58 2020 -0700
> >
> >    fix bad command name
> >
> >diff --git a/llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-ops.ll
> b/llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-ops.ll
> >index ffe5d8eedf4..ed3e2e6b481 100644
> >--- a/llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-ops.ll
> >+++ b/llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-ops.ll
> >@@ -1,6 +1,6 @@
> > ; REQUIRES: asserts
> >
> >-; RUN: opt -newgvn -S %s | FileChecks %s
> >+; RUN: opt -newgvn -S %s | FileCheck %s
> >
> > ; XFAIL: *
> >
> >
> >D:\upstream\llvm-project>
> >------------------------------
> >Notice that it was okay to create the branch even though I had changes
> >staged; they stayed staged.  Other git operations might not like having
> >staged changes present, but you can use `git stash` to push/pop changes
> >(an exercise left to the reader).
> >You can redirect the output of `git show` to a file and upload that to
> >Phabricator as a diff; it doesn't care about branches.
> >
> >Now let's make a second patch dependent on the first one, using a
> >separate branch. This is a foolish irrelevant change just for example.
> >------------------------------
> >D:\upstream\llvm-project>git checkout -b mypatch2
> >Switched to a new branch 'mypatch2'
> >
> >D:\upstream\llvm-project>notepad llvm\test\Transforms\NewGVN\todo-
> pr42422-phi-of-ops.ll
> >
> >D:\upstream\llvm-project>git commit -a -m "a dependent patch"
> >[mypatch2 46eb5487e15] a dependent patch
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >D:\upstream\llvm-project>git show
> >commit 46eb5487e15027d83dda027ad02823dfb4cf09b6 (HEAD -> mypatch2)
> >Author: Paul Robinson <paul.robinson at sony.com>
> >Date:   Mon Aug 10 08:47:15 2020 -0700
> >
> >    a dependent patch
> >
> >diff --git a/llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-ops.ll
> b/llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-ops.ll
> >index ed3e2e6b481..9b9ce0ae82b 100644
> >--- a/llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-ops.ll
> >+++ b/llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-ops.ll
> >@@ -1,6 +1,6 @@
> > ; REQUIRES: asserts
> >
> >-; RUN: opt -newgvn -S %s | FileCheck %s
> >+; RUN: opt -newgvn %s -S | FileCheck %s
> >
> > ; XFAIL: *
> >
> >------------------------------
> >This patch depends on the first one because it changed the same line,
> >and preserves the change made by the first patch.
> >
> >Again, redirect the output of `git show` to generate a diff for Phab.
> >As I mentioned before, Phab has a way to mark it as dependent on the
> >previous patch, but offhand I don't know what that is.
> >
> >Now time has passed, 'master' has been updated, and I should update
> >all my branches.
> >Start with 'master' and work your way through the dependent branches.
> >The branch dependencies are master -> mypatch -> mypatch2,
> >so you update each branch in order by rebasing on the previous.
> >'master' is a special case because it needs to be rebased on the
> >remote branch; mypatch rebased on master, mypatch2 rebased on
> >mypatch.
> >------------------------------
> >D:\upstream\llvm-project>git checkout master
> >Switched to branch 'master'
> >Your branch is up to date with 'origin/master'.
> >
> >D:\upstream\llvm-project>git pull --rebase
> ># lots of git output omitted for brevity
> >
> >D:\upstream\llvm-project>git checkout mypatch
> >Updating files: 100% (1730/1730), done.
> >Switched to branch 'mypatch'
> >
> >D:\upstream\llvm-project>git rebase master
> >Successfully rebased and updated refs/heads/mypatch.
> >
> >D:\upstream\llvm-project>git checkout mypatch2
> >Updating files: 100% (1731/1731), done.
> >Switched to branch 'mypatch2'
> >
> >D:\upstream\llvm-project>git rebase mypatch
> >Successfully rebased and updated refs/heads/mypatch2.
> >------------------------------
> >You'll note I used `git pull --rebase` to update master.
> >The `--rebase` would be mandatory if I had committed to master
> >(otherwise my local master branch history would be unusable for
> >pushing to upstream).  It's harmless otherwise, so I've trained
> >myself always to use `--rebase` when updating from a remote.
> >
> >When it comes time to push my patch upstream, I need to push it
> >on master, not on my local branch, so I need to move the patch
> >from mypatch to master.
> >------------------------------
> >D:\upstream\llvm-project>git checkout master
> >Switched to branch 'master'
> >Your branch is up to date with 'origin/master'.
> >
> >D:\upstream\llvm-project>git merge --ff-only mypatch
> >Updating 68330ee0a97..543870614f6
> >Fast-forward
> > llvm/test/Transforms/NewGVN/todo-pr42422-phi-of-ops.ll | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >------------------------------
> >The `--ff-only` is LLVM project policy, we don't allow using
> >merge commits; new patches must be made directly on master.
> >At this point you could do `git push` of the first patch.
> >
> >Once that patch is pushed, the mypatch branch can be deleted,
> >and mypatch2 can be rebased directly onto master.  Repeat as
> >needed for each patch.
> >
> >HTH,
> >--paulr