|
Packit |
01d647 |
# Guidelines for using git
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
## Commit messages
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
The first line of a commit message should be < 80 characters long and briefly
|
|
Packit |
01d647 |
describe the whole commit. Optionally, you can prefix the summary with a
|
|
Packit |
01d647 |
tag/module in square brackets (e.g. travis if your commit changed something for
|
|
Packit |
01d647 |
Travis, or testsuite for the testsuite, or tiff-parser, etc.). If the commit
|
|
Packit |
01d647 |
requires additional explanation, a blank line can be put below the summary
|
|
Packit |
01d647 |
followed by a more thorough explanation.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
A commit message can look like this:
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
[travis] Fix mac osx jobs
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- Specify concrete ubuntu and mac versions
|
|
Packit |
01d647 |
- Use latest conan version
|
|
Packit |
01d647 |
- Fix the profiles for linux and mac
|
|
Packit |
01d647 |
- Use new version of expat (avilable in conan-center)
|
|
Packit |
01d647 |
- Install urllib3 as suggested in python guidelines
|
|
Packit |
01d647 |
- Use virtualenv with python3
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
The advantage of this approach is that we always see the brief summary via `git
|
|
Packit |
01d647 |
log --oneline` and on GitHub. The 80 characters limit ensures that the message
|
|
Packit |
01d647 |
does not wrap.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
Please avoid overly generic commit messages like "fixed a bug", instead write
|
|
Packit |
01d647 |
e.g. "fixed an overflow in the TIFF parser". If your commit fixes a specific
|
|
Packit |
01d647 |
issue on GitHub then provide its number in the commit message. A message of the
|
|
Packit |
01d647 |
form "fixes #aNumber" result in GitHub automatically closing issue #aNumber once
|
|
Packit |
01d647 |
the issue got merged (please write that in the detailed description below the
|
|
Packit |
01d647 |
summary). If the commit fixes an issue that got a CVE assigned, then you must
|
|
Packit |
01d647 |
mention the CVE number in the commit message. Please also mention it in commit
|
|
Packit |
01d647 |
messages for accompanying commits (like adding regression tests), so that
|
|
Packit |
01d647 |
downstream package maintainers can cherry-pick the respective commits easily.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
If you have trouble finding a brief summary that fits into 80 characters, then
|
|
Packit |
01d647 |
you should probably split your commit.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
## When to commit
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
Commits should be atomic, i.e. they should make one self-contained
|
|
Packit |
01d647 |
change. Consider the following example: you want to fix an issue, which requires
|
|
Packit |
01d647 |
you to perform two changes in two separate files to fix the issue. Then you also
|
|
Packit |
01d647 |
want to reformat both files using clang-format and add a regression test or a
|
|
Packit |
01d647 |
unit test.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
This would result in the following commits:
|
|
Packit |
01d647 |
1. the fix for the issue in the two source files
|
|
Packit |
01d647 |
2. addition of a unit test or regression test (provided that it does not require
|
|
Packit |
01d647 |
additional changes to other logical units)
|
|
Packit |
01d647 |
3. Application of clang format to the first source file
|
|
Packit |
01d647 |
4. Application of clang format to the second source file
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
We can summarize this in the following guidelines:
|
|
Packit |
01d647 |
- Large formatting changes should be separate commits for each source file (that
|
|
Packit |
01d647 |
way changes can be reviewed easily, as formatting changes result in very noisy
|
|
Packit |
01d647 |
diffs)
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- Changes made in different source which do not make sense without each other
|
|
Packit |
01d647 |
should be committed together
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- Changes made in the same file which do not belong together should not be
|
|
Packit |
01d647 |
committed together
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- If changes are requested during the code review, then they should be either
|
|
Packit |
01d647 |
included in the previous already created commits, if that is applicable.
|
|
Packit |
01d647 |
For example if a variable's name should be changed, then that should be
|
|
Packit |
01d647 |
included into the already created commit. A bigger change, like a new function
|
|
Packit |
01d647 |
or class will probably require a separate commit.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- Please keep in mind that your commits might be cherry-picked into an older
|
|
Packit |
01d647 |
branch. Therefore split your commits accordingly, so that changes into
|
|
Packit |
01d647 |
separate modules go into separate commits.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- Every commit should keep the code base in a buildable state. The test suite
|
|
Packit |
01d647 |
needn't pass on every commit, but must pass before being merged into
|
|
Packit |
01d647 |
`master`.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
These are however not strict rules and it always depends on the case. If in
|
|
Packit |
01d647 |
doubt: ask.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
## Keeping the history linear
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
We prefer to keep the git log nearly linear with the individual pull requests
|
|
Packit |
01d647 |
still visible, since they usually form one logical unit. It should look roughly
|
|
Packit |
01d647 |
like this:
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
* 9f74f247 Merge pull request #227 from frli8848/master
|
|
Packit |
01d647 |
|\
|
|
Packit |
01d647 |
| * 73ac02d7 Added test for Sigma lenses
|
|
Packit |
01d647 |
| * fc8b45dd Added the Sigma 120-300mm F2.8 DG OS HSM | S for Nikon mount.
|
|
Packit |
01d647 |
| * 34a3be02 Added Sigma 50mm F1.4 DG HSM | A mount/UPC code (for Nikon mount).
|
|
Packit |
01d647 |
| * 21522702 Added Sigma 20mm F1.4 DG HSM | A mount/UPC code (for Nikon mount).
|
|
Packit |
01d647 |
|/
|
|
Packit |
01d647 |
* f9d421b1 Merge pull request #109 from D4N/error_codes_enum
|
|
Packit |
01d647 |
|\
|
|
Packit |
01d647 |
| * 3965a44d Replace error variable names in test suite with enum error codes
|
|
Packit |
01d647 |
| * a15f090f Modified test suite so that case sensitive keys are possible
|
|
Packit |
01d647 |
| * efe2ccdc Replaced all hardcoded error codes with ker... constants
|
|
Packit |
01d647 |
| * d897997b Force error code usage to construct a Exiv2::BasicError
|
|
Packit |
01d647 |
| * d3c3c036 Incorporated error codes into errList
|
|
Packit |
01d647 |
| * b80fa1b4 Added error codes from src/error.cpp into an enumeration
|
|
Packit |
01d647 |
|/
|
|
Packit |
01d647 |
* efee9a2b Merge pull request #205 from D4N/CVE-2017-1000127_reproducer
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
As can be seen, the two pull requests are still distinguishable but the history
|
|
Packit |
01d647 |
is still nearly linear. This ensures that cherry-picking and bisecting works
|
|
Packit |
01d647 |
without issues.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
To ensure such a linear history, do **not** use GitHub's `Update Branch` button!
|
|
Packit |
01d647 |
This creates a merge commit in your pull request's branch and can results in
|
|
Packit |
01d647 |
rather complicated logs, like this:
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
* |
|
|
Packit |
01d647 |
|\ \
|
|
Packit |
01d647 |
| * |
|
|
Packit |
01d647 |
* | |
|
|
Packit |
01d647 |
|\ \ \
|
|
Packit |
01d647 |
| |/ /
|
|
Packit |
01d647 |
|/| |
|
|
Packit |
01d647 |
| * |
|
|
Packit |
01d647 |
| * |
|
|
Packit |
01d647 |
| * |
|
|
Packit |
01d647 |
| * |
|
|
Packit |
01d647 |
| * |
|
|
Packit |
01d647 |
|/ /
|
|
Packit |
01d647 |
* |
|
|
Packit |
01d647 |
|\ \
|
|
Packit |
01d647 |
| |/
|
|
Packit |
01d647 |
|/|
|
|
Packit |
01d647 |
| *
|
|
Packit |
01d647 |
| *
|
|
Packit |
01d647 |
| *
|
|
Packit |
01d647 |
|/
|
|
Packit |
01d647 |
*
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
Instead of using the `Update Branch` button use `git pull --rebase`. For the
|
|
Packit |
01d647 |
following example, we'll assume that we are working in a branch called
|
|
Packit |
01d647 |
`feature_xyz` that should be merged into the branch `master`. Furthermore the
|
|
Packit |
01d647 |
remote `origin` is a fork of exiv2 and the remote `upstream` is the "official"
|
|
Packit |
01d647 |
exiv2 repository.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
Before we start working, the `master` branch looks like this:
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
$ git log master --oneline --graph
|
|
Packit |
01d647 |
* efee9a2b (master) Merge pull request #something
|
|
Packit |
01d647 |
|\
|
|
Packit |
01d647 |
| * ead7f309 A commit on master
|
|
Packit |
01d647 |
|/
|
|
Packit |
01d647 |
* 55001c8d Merge pull request #something else
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
We create a new branch `feature_xyz` based on `master`, create two new commits
|
|
Packit |
01d647 |
`My commit 1` and `My commit 2` and submit a pull request into master. The log
|
|
Packit |
01d647 |
of the branch `feature_xyz` now looks like this:
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
$ git log feature_xyz --oneline --graph
|
|
Packit |
01d647 |
* 893fffa5 (HEAD -> feature_xyz) My commit 2
|
|
Packit |
01d647 |
* a2a22fb9 My commit 1
|
|
Packit |
01d647 |
* efee9a2b (master) Merge pull request #something
|
|
Packit |
01d647 |
|\
|
|
Packit |
01d647 |
| * ead7f309 A commit on master
|
|
Packit |
01d647 |
|/
|
|
Packit |
01d647 |
* 55001c8d Merge pull request #something else
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
If now new commits are pushed to `master`, resulting in this log:
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
$ git log master --oneline --graph
|
|
Packit |
01d647 |
* 0d636cc9 (HEAD -> master) Hotfix for issue #something completely different
|
|
Packit |
01d647 |
* efee9a2b Merge pull request #something
|
|
Packit |
01d647 |
|\
|
|
Packit |
01d647 |
| * ead7f309 A commit on master
|
|
Packit |
01d647 |
|/
|
|
Packit |
01d647 |
* 55001c8d Merge pull request #something else
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
then the branch `feature_xyz` is out of date with `master`, because it lacks the
|
|
Packit |
01d647 |
commit `0d636cc9`. We could now merge both branches (via the cli or GitHub's
|
|
Packit |
01d647 |
`Update Branch` button), but that will result in a messy history. Thus **don't**
|
|
Packit |
01d647 |
do it! If you do it, you'll have to remove the merge commits manually.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
Instead run: `git pull --rebase upstream master` in the `feature_xyz`
|
|
Packit |
01d647 |
branch. Git will pull the new commit `0d636cc9` from master into your branch
|
|
Packit |
01d647 |
`feature_xyz` and apply the two commits `My commit 1` and `My commit 2` on top
|
|
Packit |
01d647 |
of it:
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
$ git log feature_xyz --oneline --graph
|
|
Packit |
01d647 |
* 22a7a8c2 (HEAD -> feature_xyz) My commit 2
|
|
Packit |
01d647 |
* efe2ccdc My commit 1
|
|
Packit |
01d647 |
* 0d636cc9 (master) Hotfix for issue #something completely different
|
|
Packit |
01d647 |
* efee9a2b Merge pull request #something
|
|
Packit |
01d647 |
|\
|
|
Packit |
01d647 |
| * ead7f309 A commit on master
|
|
Packit |
01d647 |
|/
|
|
Packit |
01d647 |
* 55001c8d Merge pull request #something else
|
|
Packit |
01d647 |
```
|
|
Packit |
01d647 |
Please note, that the hash of `My commit 1` and `My commit 2` changed! That
|
|
Packit |
01d647 |
happened because their parent changed. Therefore you have to force push your
|
|
Packit |
01d647 |
changes via `git push --force` next time you push your changes upstream.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
## Merging pull requests
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
Most pull requests should be merged by creating a merge commit (the default on
|
|
Packit |
01d647 |
GitHub). Small pull requests (= only one can commit) can be rebased on top of
|
|
Packit |
01d647 |
master.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
## Branches and tags
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- The `master` branch is the current "main" development branch. It is protected
|
|
Packit |
01d647 |
so that changes can be only included via reviewed pull requests. New releases
|
|
Packit |
01d647 |
are made by tagging a specific commit on `master`.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- Releases are tagged with a tag of the form `v$major.$minor`. The tag is not
|
|
Packit |
01d647 |
changed when changes are backported.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- For each release a branch of the form `$major.$minor` should be created to
|
|
Packit |
01d647 |
store backported changes. It should be branched of from `master` at the commit
|
|
Packit |
01d647 |
which was tagged with `v$major.$minor`.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- All other branches are development branches for pull requests, experiments,
|
|
Packit |
01d647 |
etc. They should be deleted once the pull request got merged or the branch is
|
|
Packit |
01d647 |
no longer useful.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- Exiv2 team members can create branches for pull requests in the main
|
|
Packit |
01d647 |
repository if they want to collaborate with others (e.g. for big changes that
|
|
Packit |
01d647 |
require a lot of work). No one should not `push --force` in these branches
|
|
Packit |
01d647 |
without coordinating with others and should only `push --force-with-lease`.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
When only one person will work on a pull request, then the branch can be
|
|
Packit |
01d647 |
created in their personal fork or in the main repository (note that branches
|
|
Packit |
01d647 |
in the main repository provide an automatic continuous integration).
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
## Backporting changes
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
We try to backport critical bugfixes to the latest released version on a best
|
|
Packit |
01d647 |
effort basis. We lack the man power to support older releases, but accept
|
|
Packit |
01d647 |
patches for these.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
Bugfixes for crashes, memory corruptions, overflows and other potentially
|
|
Packit |
01d647 |
dangerous bugs **must** be backported. The same applies to bugfixes for issues
|
|
Packit |
01d647 |
that got a CVE assigned.
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
## Final remarks
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
Since git is a fully distributed version control system, all changes stay on
|
|
Packit |
01d647 |
your machine until you push them. Thus, if you are in doubt whether a trickier
|
|
Packit |
01d647 |
step with git might screw up your repository, you can simply create a backup of
|
|
Packit |
01d647 |
your whole exiv2 folder. In case the tricky step went downhill, you can restore
|
|
Packit |
01d647 |
your working copy of exiv2 and no one will ever know (unless you did a `git
|
|
Packit |
01d647 |
push`)!
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
## Additional material
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- [The git book](https://git-scm.com/book/en/v2/)
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- `man git` and `man git $command`
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- [amending and interactive
|
|
Packit |
01d647 |
rebase](https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History)
|
|
Packit |
01d647 |
|
|
Packit |
01d647 |
- [interactive
|
|
Packit |
01d647 |
staging](https://git-scm.com/book/en/v2/Git-Tools-Interactive-Staging) (for
|
|
Packit |
01d647 |
Emacs users: consider using [magit](https://magit.vc/) for interactive
|
|
Packit |
01d647 |
staging)
|