Move the MFC doc from my freebsd-git-docs repo to handbook
Integreate MFC procedures into the committer's handbook. This was originally in my freebsd-git-docs repo where it was collaberatively created. The changes here are light edits and a conversion from markdown to asciidoc. Contributions from: jhb@, lwhsu@, delphij@, Ceri Davies, Alexandre
This commit is contained in:
parent
3a52e436d3
commit
965da1835e
1 changed files with 494 additions and 0 deletions
|
@ -241,6 +241,500 @@ Committers are encouraged to seek review for their work as part of the normal de
|
|||
this section is a work in progress...
|
||||
====
|
||||
|
||||
[[mfc-with-git]]
|
||||
=== MFC (Merge From Current) Procedures
|
||||
==== Summary
|
||||
|
||||
MFC workflow can be summarized as `git cherry-pick -x` plus git commit
|
||||
--amend to adjust the commit message. For multiple commits, use `git rebase -i`
|
||||
to squash them together and edit the commit message.
|
||||
|
||||
==== Single commit MFC
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git checkout stable/X
|
||||
% git cherry-pick -x $HASH --edit
|
||||
....
|
||||
|
||||
For MFC commits, for example a vendor import, you would need to specify one parent for cherry-pick
|
||||
purposes. Normally, that would be the "first parent" of the branch you are cherry-picking from, so:
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git checkout stable/X
|
||||
% git cherry-pick -x $HASH -m 1 --edit
|
||||
....
|
||||
|
||||
If things go wrong, you'll either need to abort the cherry-pick with `git cherry-pick --abort` or fix it
|
||||
up and do a `git cherry-pick --continue`.
|
||||
|
||||
Once the cherry-pick is finished, push with `git push`. If you get an error due to losing the commit race,
|
||||
use `git pull --rebase` and try to push again.
|
||||
|
||||
==== Multiple commit MFC
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git checkout -b tmp-branch stable/X
|
||||
% for h in $HASH_LIST; do git cherry-pick -x $h; done
|
||||
% git rebase -i stable/X
|
||||
# mark each of the commits after the first as 'squash'
|
||||
# edit the commit message to be sane
|
||||
% git push freebsd HEAD:stable/X
|
||||
....
|
||||
|
||||
If the push fails due to losing the commit race, rebase and try again:
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git checkout stable/X
|
||||
% git pull
|
||||
% git checkout tmp-branch
|
||||
% git rebase stable/X
|
||||
% git push freebsd HEAD:stable/X
|
||||
....
|
||||
|
||||
Once the MFC is complete, you can delete the temporary branch:
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git checkout stable/X
|
||||
% git branch -d tmp-branch
|
||||
....
|
||||
|
||||
==== MFC a vendor import
|
||||
|
||||
Vendor imports are the only thing in the tree that creates a merge
|
||||
commit in the main line. Cherry picking merge commits into stable/XX
|
||||
presents an additional difficulty because there are two parents for a
|
||||
merge commit. Generally, you'll want the first parent's diff since
|
||||
that's the diff to mainline (though there may be some exceptions).
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git cherry-pick -x -m 1 $HASH
|
||||
....
|
||||
is typically what you want. This will tell cherry-pick to apply the correct diff.
|
||||
|
||||
There are some, hopefully, rare cases where it's possible that the
|
||||
mainline was merged backwards by the conversion script. Should that be
|
||||
the case (and we've not found any yet), you'd change the above to '-m 2'
|
||||
to pickup the proper parent. Just do
|
||||
[source,shell]
|
||||
....
|
||||
% git cherry-pick --abort
|
||||
% git cherry-pick -x -m 2 $HASH
|
||||
....
|
||||
to do that. The `--aboort` will cleanup the failed first attempt.
|
||||
|
||||
==== Redoing a MFC
|
||||
|
||||
If you do a MFC, and it goes horribly wrong and you want to start over,
|
||||
then the easiest way is to use `git reset --hard` like so:
|
||||
[source,shell]
|
||||
....
|
||||
% git reset --hard freebsd/stable/12
|
||||
....
|
||||
though if you have some revs you want to keep, and others you don't,
|
||||
using 'git rebase -i' is better.
|
||||
|
||||
==== Considerations when MFCing
|
||||
|
||||
When committing source commits to stable and releng branches, we have
|
||||
the following goals:
|
||||
|
||||
1. Clearly mark direct commits distinct from commits that land a
|
||||
change from another branch
|
||||
2. Avoid introducing known breakage into stable and releng branches
|
||||
3. Allow developers to determine which changes have or have not been
|
||||
landed from one branch to another
|
||||
|
||||
With subversion, we used the following practices to achieve these goals:
|
||||
|
||||
1. Using 'MFC' and 'MFS' tags to mark commits that merged changes from
|
||||
another branch
|
||||
2. Squashing fixup commits into the main commit when merging a change
|
||||
3. Recording mergeinfo so that `svn mergeinfo --show-revs` worked
|
||||
|
||||
With Git, we will need to use different strategies to achieve the same
|
||||
goals. This document aims to define best practices when merging
|
||||
source commits using git that achieve these goals. In general, we aim
|
||||
to use git's native support to achieve these goals rather than
|
||||
enforcing practices built on subversion's model.
|
||||
|
||||
One general note: due to technical differences with Git, we will not
|
||||
be using git "merge commits" (created via `git merge`) in stable or
|
||||
releng branches. Instead, when this document refers to "merge
|
||||
commits", it means a commit originally made to `main` that is
|
||||
replicated or "landed" to a stable branch, or a commit from a stable
|
||||
branch that is replicated to a releng branch with some varation of
|
||||
`git cherry-pick`.
|
||||
|
||||
==== Commit message standards
|
||||
===== Marking MFCs
|
||||
|
||||
There are two main options for marking MFCs as distinct from direct
|
||||
commits:
|
||||
|
||||
1. One option that matches our existing practice (the wisdom of which
|
||||
I'm not commenting on) would mark MFCs like this in the commit
|
||||
message: `MFC: 12def6789a3a,ac32ee4a5c2e`
|
||||
|
||||
where the first 12 digits of the hash is used to mark the commit message.
|
||||
This "abbreviated hash" can be retrieved by: `git show --format=%p --no-patch $full_hash`
|
||||
|
||||
This preserves the information, but isn't 'git standard'. It also
|
||||
requires committers to manually edit commit messages to include
|
||||
this information when merging.
|
||||
|
||||
2. Use the `-x` flag with `git cherry-pick`. This adds a line to the
|
||||
commit message that includes the hash of the original commit when
|
||||
merging. Since it is added by Git directly, committers do not have
|
||||
to manually edit the commit log when merging.
|
||||
|
||||
We feel that the second option is simpler going forward.
|
||||
|
||||
===== Finding Eligible Hashes to MFC
|
||||
|
||||
Git provides some built-in support for this via the `git cherry` and
|
||||
`git log --cherry` commands. These commands compare the raw diffs of
|
||||
commits (but not other metadata such as log messages) to determine if
|
||||
two commits are identical. This works well when each commit from head
|
||||
is landed as a single commit to a stable branch, but it falls over if
|
||||
multiple commits from main are squashed together as a single commit to
|
||||
a stable branch.
|
||||
|
||||
There are a few options for resolving this:
|
||||
|
||||
1. We could ban squashing of commits and instead require that committers
|
||||
stage all of the fixup / follow-up commits to stable into a single
|
||||
push. This would still achieve the goal of stability in stable and
|
||||
releng branches since pushes are atomic and users doing a simple pull
|
||||
will never end up with a tree that has the main commit without the
|
||||
fixup(s). `git bisect` is also able to cope with this model via
|
||||
`git bisect skip`.
|
||||
|
||||
2. We could adopt a consistent style for describing MFCs and write
|
||||
our own tooling to wrap around `git cherry` to determine the list
|
||||
of eligible commits. A simple approach here might be to use the
|
||||
syntax from `git cherry-pick -x`, but require that a squashed
|
||||
commit list all of the hashes (one line per hash) at the end of
|
||||
the commit message. Developers could do this by using
|
||||
`git cherry-pick -x` of each individual commit into a branch and
|
||||
then use `git rebase` to squash the commits down into a single
|
||||
commit, but collecting the `-x` annotations at the end of the
|
||||
landed commit log.
|
||||
|
||||
===== Trim Metadata?
|
||||
|
||||
One area that was not clearly documented with subversion (or even CVS)
|
||||
is how to format metadata in log messages for MFC commits. Should
|
||||
it include the metadata from the original commit unchanged, or should
|
||||
it be altered to reflect information about the MFC commit itself?
|
||||
|
||||
Historical practice has varied, though some of the variance is by
|
||||
field. For example, MFCs that are relevant to a PR generally
|
||||
include the PR field in the MFC so that MFC commits are included
|
||||
in the bug tracker's audit trail. Other fields are less clear. For
|
||||
example, Phabricator shows the diff of the last commit tagged to a
|
||||
review, so including Phabricator URLs replaces the `main` commit with
|
||||
the landed commits. The list of reviewers is also not clear. If a
|
||||
reviewer has approved a change to `main`, does that mean they have
|
||||
approved the MFC commit? Is that true if it's identical code only,
|
||||
or with merely trivial reworkes? It's clearly not true for more
|
||||
extensive reworks. Even for identical code what if the commit doesn't
|
||||
conflict but introduces an ABI change? A reviewer may have ok'd a
|
||||
commit for `main` due to the ABI breakage but may not approve of
|
||||
merging the same commit as-is. One will have to use one's best
|
||||
judgement until clear guidelines can be agreed upon.
|
||||
|
||||
For MFCs regulated by re@, new metadata fields are added, such as
|
||||
the Approved by tag for approved commits. This new metadata will have
|
||||
to be added via `git commit --amend` or similar after the original
|
||||
commit has been reviewed and approved. We may also want to reserve
|
||||
some metadata fields in MFC commits such as Phabricator URLs for use
|
||||
by re@ in the future.
|
||||
|
||||
Preserving existing metadata provides a very simple workflow.
|
||||
Developers can just use `git cherry-pick -x` without having to edit
|
||||
the log message.
|
||||
|
||||
If instead we choose to adjust metadata in MFCs, developers will
|
||||
have to edit log messages explicitly via the use of `git cherry-pick
|
||||
--edit` or `git commit --amend`. However, as compared to svn, at
|
||||
least the existing commit message can be pre-populated and metadata
|
||||
fields can be added or removed without having to re-enter the entire
|
||||
commit message.
|
||||
|
||||
The bottom line is that developers will likely need to curate their
|
||||
commit message for MFCs that are non-trivial.
|
||||
|
||||
==== Examples
|
||||
|
||||
===== Merging a Single Subversion Commit
|
||||
|
||||
This walks through the process of merging a commit to stable/12 that
|
||||
was originally committed to head in Subversion. In this case, the
|
||||
original commit is r368685.
|
||||
|
||||
The first step is to map the Subversion commit to a Git hash. Once
|
||||
you have fetched refs/notes/commits, you can pass the revision number
|
||||
to `git log --grep`:
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git log main --grep 368685
|
||||
commit ce8395ecfda2c8e332a2adf9a9432c2e7f35ea81
|
||||
Author: John Baldwin <jhb@FreeBSD.org>
|
||||
Date: Wed Dec 16 00:11:30 2020 +0000
|
||||
|
||||
Use the 't' modifier to print a ptrdiff_t.
|
||||
|
||||
Reviewed by: imp
|
||||
Obtained from: CheriBSD
|
||||
Sponsored by: DARPA
|
||||
Differential Revision: https://reviews.freebsd.org/D27576
|
||||
|
||||
Notes:
|
||||
svn path=/head/; revision=368685
|
||||
....
|
||||
|
||||
Next, MFC the commit to a `stable/12` checkout:
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
git checkout stable/12
|
||||
git cherry-pick -x ce8395ecfda2c8e332a2adf9a9432c2e7f35ea81 --edit
|
||||
....
|
||||
|
||||
Git will invoke the editor. Use this to remove the metadata that only
|
||||
applied to the original commit (Phabricator URL and Reviewed by).
|
||||
After the editor saves the updated log message, Git completes the
|
||||
commit:
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
[stable/12 3e3a548c4874] Use the 't' modifier to print a ptrdiff_t.
|
||||
Date: Wed Dec 16 00:11:30 2020 +0000
|
||||
1 file changed, 1 insertion(+), 1 deletion(-)
|
||||
....
|
||||
|
||||
The contents of the MFCd commit can be examined via `git show`:
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git show
|
||||
commit 3e3a548c487450825679e4bd63d8d1a67fd8bd2d (HEAD -> stable/12)
|
||||
Author: John Baldwin <jhb@FreeBSD.org>
|
||||
Date: Wed Dec 16 00:11:30 2020 +0000
|
||||
|
||||
Use the 't' modifier to print a ptrdiff_t.
|
||||
|
||||
Obtained from: CheriBSD
|
||||
Sponsored by: DARPA
|
||||
|
||||
(cherry picked from commit ce8395ecfda2c8e332a2adf9a9432c2e7f35ea81)
|
||||
|
||||
diff --git a/sys/compat/linuxkpi/common/include/linux/printk.h b/sys/compat/linuxkpi/common/include/linux/printk.h
|
||||
index 31802bdd2c99..e6510e9e9834 100644
|
||||
--- a/sys/compat/linuxkpi/common/include/linux/printk.h
|
||||
+++ b/sys/compat/linuxkpi/common/include/linux/printk.h
|
||||
@@ -68,7 +68,7 @@ print_hex_dump(const char *level, const char *prefix_str,
|
||||
printf("[%p] ", buf);
|
||||
break;
|
||||
case DUMP_PREFIX_OFFSET:
|
||||
- printf("[%p] ", (const char *)((const char *)buf -
|
||||
+ printf("[%#tx] ", ((const char *)buf -
|
||||
(const char *)buf_old));
|
||||
break;
|
||||
default:
|
||||
....
|
||||
|
||||
The MFC commit can now be published via `git push`
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git push freebsd
|
||||
Enumerating objects: 17, done.
|
||||
Counting objects: 100% (17/17), done.
|
||||
Delta compression using up to 4 threads
|
||||
Compressing objects: 100% (7/7), done.
|
||||
Writing objects: 100% (9/9), 817 bytes | 204.00 KiB/s, done.
|
||||
Total 9 (delta 5), reused 1 (delta 1), pack-reused 0
|
||||
To gitrepo-dev.FreeBSD.org:src.git
|
||||
525bd9c9dda7..3e3a548c4874 stable/12 -> stable/12
|
||||
....
|
||||
|
||||
===== Merging a Single Subversion Commit with a Conflict
|
||||
|
||||
This example is similar to the previous example except that the
|
||||
commit in question encounters a merge conflict. In this case, the
|
||||
original commit is r368314.
|
||||
|
||||
As above, the first step is to map the Subversion commit to a Git
|
||||
hash:
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git log main --grep 368314
|
||||
commit 99963f5343a017e934e4d8ea2371a86789a46ff9
|
||||
Author: John Baldwin <jhb@FreeBSD.org>
|
||||
Date: Thu Dec 3 22:01:13 2020 +0000
|
||||
|
||||
Don't transmit mbufs that aren't yet ready on TOE sockets.
|
||||
|
||||
This includes mbufs waiting for data from sendfile() I/O requests, or
|
||||
mbufs awaiting encryption for KTLS.
|
||||
|
||||
Reviewed by: np
|
||||
MFC after: 2 weeks
|
||||
Sponsored by: Chelsio Communications
|
||||
Differential Revision: https://reviews.freebsd.org/D27469
|
||||
|
||||
Notes:
|
||||
svn path=/head/; revision=368314
|
||||
....
|
||||
|
||||
Next, MFC the commit to a `stable/12` checkout:
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git checkout stable/12
|
||||
% git cherry-pick -x 99963f5343a017e934e4d8ea2371a86789a46ff9 --edit
|
||||
Auto-merging sys/dev/cxgbe/tom/t4_cpl_io.c
|
||||
CONFLICT (content): Merge conflict in sys/dev/cxgbe/tom/t4_cpl_io.c
|
||||
warning: inexact rename detection was skipped due to too many files.
|
||||
warning: you may want to set your merge.renamelimit variable to at least 7123 and retry the command.
|
||||
error: could not apply 99963f5343a0... Don't transmit mbufs that aren't yet ready on TOE sockets.
|
||||
hint: after resolving the conflicts, mark the corrected paths
|
||||
hint: with 'git add <paths>' or 'git rm <paths>'
|
||||
hint: and commit the result with 'git commit'
|
||||
....
|
||||
|
||||
In this case, the commit encountered a merge conflict in
|
||||
sys/dev/cxge/tom/t4_cpl_io.c as kernel TLS is not present in
|
||||
stable/12. Note that Git does not invoke an editor to adjust the
|
||||
commit message due to the conflict. `git status` confirms that this
|
||||
file has merge conflicts:
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git status
|
||||
On branch stable/12
|
||||
Your branch is up to date with 'upstream/stable/12'.
|
||||
|
||||
You are currently cherry-picking commit 99963f5343a0.
|
||||
(fix conflicts and run "git cherry-pick --continue")
|
||||
(use "git cherry-pick --skip" to skip this patch)
|
||||
(use "git cherry-pick --abort" to cancel the cherry-pick operation)
|
||||
|
||||
Unmerged paths:
|
||||
(use "git add <file>..." to mark resolution)
|
||||
both modified: sys/dev/cxgbe/tom/t4_cpl_io.c
|
||||
|
||||
no changes added to commit (use "git add" and/or "git commit -a")
|
||||
....
|
||||
|
||||
After editing the file to resolve the conflict, `git status` shows the
|
||||
conflict as resolved:
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git status
|
||||
On branch stable/12
|
||||
Your branch is up to date with 'upstream/stable/12'.
|
||||
|
||||
You are currently cherry-picking commit 99963f5343a0.
|
||||
(all conflicts fixed: run "git cherry-pick --continue")
|
||||
(use "git cherry-pick --skip" to skip this patch)
|
||||
(use "git cherry-pick --abort" to cancel the cherry-pick operation)
|
||||
|
||||
Changes to be committed:
|
||||
modified: sys/dev/cxgbe/tom/t4_cpl_io.c
|
||||
....
|
||||
|
||||
The cherry-pick can now be completed:
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git cherry-pick --continue
|
||||
....
|
||||
|
||||
Since there was a merge conflict, Git invokes the editor to
|
||||
adjust the commit message. Trim the metadata fields from the
|
||||
commit log from the original commit to head and save the
|
||||
updated log message.
|
||||
|
||||
The contents of the MFC commit can be examined via `git show`:
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
% git show
|
||||
commit 525bd9c9dda7e7c7efad2d4570c7fd8e1a8ffabc (HEAD -> stable/12)
|
||||
Author: John Baldwin <jhb@FreeBSD.org>
|
||||
Date: Thu Dec 3 22:01:13 2020 +0000
|
||||
|
||||
Don't transmit mbufs that aren't yet ready on TOE sockets.
|
||||
|
||||
This includes mbufs waiting for data from sendfile() I/O requests, or
|
||||
mbufs awaiting encryption for KTLS.
|
||||
|
||||
Sponsored by: Chelsio Communications
|
||||
|
||||
(cherry picked from commit 99963f5343a017e934e4d8ea2371a86789a46ff9)
|
||||
|
||||
diff --git a/sys/dev/cxgbe/tom/t4_cpl_io.c b/sys/dev/cxgbe/tom/t4_cpl_io.c
|
||||
index 8e8c2b8639e6..43861f10b689 100644
|
||||
--- a/sys/dev/cxgbe/tom/t4_cpl_io.c
|
||||
+++ b/sys/dev/cxgbe/tom/t4_cpl_io.c
|
||||
@@ -746,6 +746,8 @@ t4_push_frames(struct adapter *sc, struct toepcb *toep, int drop)
|
||||
for (m = sndptr; m != NULL; m = m->m_next) {
|
||||
int n;
|
||||
|
||||
+ if ((m->m_flags & M_NOTAVAIL) != 0)
|
||||
+ break;
|
||||
if (IS_AIOTX_MBUF(m))
|
||||
n = sglist_count_vmpages(aiotx_mbuf_pages(m),
|
||||
aiotx_mbuf_pgoff(m), m->m_len);
|
||||
@@ -821,8 +823,9 @@ t4_push_frames(struct adapter *sc, struct toepcb *toep, int drop)
|
||||
|
||||
/* nothing to send */
|
||||
if (plen == 0) {
|
||||
- KASSERT(m == NULL,
|
||||
- ("%s: nothing to send, but m != NULL", __func__));
|
||||
+ KASSERT(m == NULL || (m->m_flags & M_NOTAVAIL) != 0,
|
||||
+ ("%s: nothing to send, but m != NULL is ready",
|
||||
+ __func__));
|
||||
break;
|
||||
}
|
||||
|
||||
@@ -910,7 +913,7 @@ t4_push_frames(struct adapter *sc, struct toepcb *toep, int drop)
|
||||
toep->txsd_avail--;
|
||||
|
||||
t4_l2t_send(sc, wr, toep->l2te);
|
||||
- } while (m != NULL);
|
||||
+ } while (m != NULL && (m->m_flags & M_NOTAVAIL) == 0);
|
||||
|
||||
/* Send a FIN if requested, but only if there's no more data to send */
|
||||
if (m == NULL && toep->flags & TPF_SEND_FIN)
|
||||
....
|
||||
|
||||
The MFC commit can now be published via `git push`
|
||||
|
||||
[source,shell]
|
||||
....
|
||||
git push freebsd
|
||||
Enumerating objects: 13, done.
|
||||
Counting objects: 100% (13/13), done.
|
||||
Delta compression using up to 4 threads
|
||||
Compressing objects: 100% (7/7), done.
|
||||
Writing objects: 100% (7/7), 819 bytes | 117.00 KiB/s, done.
|
||||
Total 7 (delta 6), reused 0 (delta 0), pack-reused 0
|
||||
To gitrepo.FreeBSD.org:src.git
|
||||
f4d0bc6aa6b9..525bd9c9dda7 stable/12 -> stable/12
|
||||
....
|
||||
|
||||
[[vendor-import-git]]
|
||||
=== Vendor Imports with Git
|
||||
|
||||
|
|
Loading…
Reference in a new issue