Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Teach cherry-pick/revert to skip commits #1

Closed
wants to merge 3 commits into from
Closed

Conversation

@r1walz
Copy link
Owner

@r1walz r1walz commented May 16, 2019

We are trying to implement git cherry-pick --skip for that I've introduced sequencer_skip in the sequencer.c file which basically calls rollback_single_pick and then, if there are still instructions in todo file, sequencer_continue.

Copy link

@tgummerer tgummerer left a comment

Thanks for this! Just a couple of quick thoughts, as the rest of my week is going to be quite busy :)

sequencer.c Show resolved Hide resolved
sequencer.h Outdated Show resolved Hide resolved
@tgummerer
Copy link

@tgummerer tgummerer commented May 16, 2019

Also don't forget to set DEVELOPER=1 in your config.mak. I can share my full config.mak later if you want.

@r1walz
Copy link
Owner Author

@r1walz r1walz commented May 16, 2019

Also don't forget to set DEVELOPER=1 in your config.mak. I can share my full config.mak later if you want.

Please do share your config.mak file

Copy link

@newren newren left a comment

Also, more typical format for the first line of the commit message would be:

cherry-pick/revert: add --skip option, for consistency with am and rebase

builtin/revert.c Outdated Show resolved Hide resolved
sequencer.c Outdated Show resolved Hide resolved
@tgummerer
Copy link

@tgummerer tgummerer commented May 17, 2019

Here's my config.mak:

O = 0
CFLAGS = -O$(O) -g $(EXTRA_CFLAGS) -Wall
DEVELOPER = 1
DEVOPTS = pedantic
THREADED_DELTA_SEARCH = 1
NO_SVN_TESTS = 1
TMP := /tmp/git-tests-$(shell git rev-parse --show-toplevel | sha1sum | head -c10)
GIT_TEST_OPTS = --root=$(TMP) -q --tee
GIT_PROVE_OPTS = --timer -j13 --state=failed,slow,save
DEFAULT_TEST_TARGET = prove
GIT_PERF_LARGE_REPO = /home/tgummerer/dev/Webkit/.git
GIT_PERF_REPEAT_COUNT = 3
GIT_PERF_MAKE_OPTS = -j12

@r1walz
Copy link
Owner Author

@r1walz r1walz commented May 18, 2019

@tgummerer

GIT_PERF_LARGE_REPO = /home/tgummerer/dev/Webkit/.git

What is this? Any alternative that I may use on my machine?

@r1walz r1walz force-pushed the cherry-pick--skip branch from 65d9f7f to 26deccf May 18, 2019
@tgummerer
Copy link

@tgummerer tgummerer commented May 18, 2019

This is a large repository used for performance testing. It's actually unnecessary for your project, so you can just ignore all the GIT_PERF_* settings.

sequencer.c Outdated Show resolved Hide resolved
@r1walz r1walz force-pushed the cherry-pick--skip branch 3 times, most recently from 02a6136 to a1f413f May 19, 2019
Copy link

@tgummerer tgummerer left a comment

Couple more comments

advice.c Outdated Show resolved Hide resolved
sequencer.c Outdated Show resolved Hide resolved
@r1walz r1walz force-pushed the cherry-pick--skip branch 3 times, most recently from c13ee8b to 536b750 May 20, 2019
Copy link

@tgummerer tgummerer left a comment

A few notes about the commit organization here, even though GitHub doesn't have the best interface to comment on that.

  • Throughout a patch series, every commit should pass the test suite. This is important to keep the bisectability of the codebase. Imagine for example you were running git bisect run "make test", and it hits a patch where the tests don't succeed in the middle of a series, such as your "change hint messages". In that case bisect would be mislead, even though in reality that patch is not the problem at all. So the patch after that should be squashed into that patch.
  • Similarly, tests should be introduced in the same patch as a feature. That makes it easier to review them together, and check that the new feature is actually covered by tests. In addition if you cherry-pick that commit you also cherry-pick the corresponding tests (and similar if the commit needs to be reverted for any reason)

Another thing that should be in the same commit is some documentation, especially if a new feature is introduced.

advice.c Outdated Show resolved Hide resolved
sequencer.c Outdated Show resolved Hide resolved
t/t3514-revert-cherry-pick-skip.sh Outdated Show resolved Hide resolved
t/t3514-revert-cherry-pick-skip.sh Outdated Show resolved Hide resolved
@r1walz
Copy link
Owner Author

@r1walz r1walz commented May 21, 2019

@tgummerer wrote:

A few notes about the commit organization here, even though GitHub doesn't have the best interface to comment on that.
[...]
Another thing that should be in the same commit is some documentation, especially if a new feature is introduced.

Basically, you are saying that I should squash everything to one commit and that commit should also contain all the documentation?

@tgummerer
Copy link

@tgummerer tgummerer commented May 21, 2019

Basically, you are saying that I should squash everything to one commit and that commit should also contain all the documentation?

I think there could be two commits (or possibly three).

  1. (maybe) introduce a new sequencer_skip function, that is not used anywhere. This may or may not want to be its own commit, saying in the commit message that you're going to be using this in the next commit, or could be squashed into what I'm going to name 2) here. The way you're implementing it here still seems somewhat specific to cherry-pick/revert, so maybe squashing it into 2) is actually the better choice here.
  2. A commit introducing the new feature, including documentation and the new tests
  3. A commit changing the advice strings, including potentially adjusting tests/maybe adding new ones.

@r1walz
Copy link
Owner Author

@r1walz r1walz commented May 21, 2019

[...] The way you're implementing it here still seems somewhat specific to cherry-pick/revert, so maybe squashing it into 2) is actually the better choice here.
2. A commit introducing the new feature, including documentation and the new tests
3. A commit changing the advice strings, including potentially adjusting tests/maybe adding new ones.

Yes, I agree. Two commits should be enough to accommodate these changes.

@r1walz r1walz force-pushed the cherry-pick--skip branch from 536b750 to 7ffaeb1 May 25, 2019
@sivaraam
Copy link

@sivaraam sivaraam commented May 25, 2019

Throughout a patch series, every commit should pass the test suite.

@mhagger's git-test tool might help you with this. You can use it to ensure the test suites passes for every commit (thus ensuring bisectability). So, you don't have to take the burden of ensuring that the test suite passes after each commit. git test does that for you. 🙂

@r1walz
Copy link
Owner Author

@r1walz r1walz commented May 25, 2019

Thanks! @sivaraam, I'll surely check that out!

Copy link

@newren newren left a comment

I'd reword the commit message as:

cherry-pick/revert: update hints to make use of new --skip option

followed by your Signed-off-by. This seems small enough that I'm wondering if we want to separate from the first commit now. Hmm...

builtin/commit.c Show resolved Hide resolved
sequencer.c Show resolved Hide resolved
Copy link

@newren newren left a comment

The changes look good. Your commit message is heavy on the 'what' and light on the 'why'; we should reverse that. Something more like:

cherry-pick/revert: add --skip flag

rebase and am both have a --skip flag. cherry-pick and revert also have a concept of skipping a commit, but confusingly this functionality is accessed via 'git reset' (or in the case of a patch which had conflicts, 'git reset --merge'). Add a --skip option to make these commands more consistent.

@r1walz
Copy link
Owner Author

@r1walz r1walz commented May 27, 2019

@newren wrote:

The changes look good. Your commit message is heavy on the 'what' and light on the 'why'; we should reverse that [...]

Yes, I always tend to forget this since here (at college) we usually say "what" this commit will do instead of "how" or "why". Thanks, I'll keep that in mind and try to get rid of it ASAP.

Copy link

@tgummerer tgummerer left a comment

A couple of things in addition to what Elijah already said.

followed by your Signed-off-by. This seems small enough that I'm wondering if we want to separate from the first commit now. Hmm...

Yeah I feel like the second commit doesn't need to be separate at this point.

Documentation/git-revert.txt Outdated Show resolved Hide resolved
t/t3510-cherry-pick-sequence.sh Show resolved Hide resolved
t/t3510-cherry-pick-sequence.sh Show resolved Hide resolved
@r1walz r1walz force-pushed the cherry-pick--skip branch from 7ffaeb1 to 1186637 May 27, 2019
Copy link

@tgummerer tgummerer left a comment

Minus my minor nit on the style change in the message, I think we should send this out to the mailing list, and get further reviews there, unless @newren has more thoughts on this?

Note also that we're currently in the -rc phase of the release cycle, so new features tend to be less likely to get reviews, so we might have to send it again after 2.22 is released. Not sure if it's worth waiting until that.

builtin/commit.c Outdated Show resolved Hide resolved
t/t3510-cherry-pick-sequence.sh Show resolved Hide resolved
@r1walz r1walz force-pushed the cherry-pick--skip branch from 1186637 to 2b4b3e9 May 28, 2019
Copy link

@dscho dscho left a comment

Just commenting, as I am not a mentor this year :-)

builtin/commit.c Outdated Show resolved Hide resolved
sequencer.c Outdated Show resolved Hide resolved
sequencer.c Show resolved Hide resolved
t/t3510-cherry-pick-sequence.sh Show resolved Hide resolved
@r1walz r1walz force-pushed the cherry-pick--skip branch from 2b4b3e9 to 80a63c8 May 29, 2019
Copy link

@tgummerer tgummerer left a comment

One more minor nit, that I didn't notice before, sorry.

sequencer.c Show resolved Hide resolved
@r1walz r1walz force-pushed the cherry-pick--skip branch from 80a63c8 to 5a17f2b May 30, 2019
Copy link

@tgummerer tgummerer left a comment

I just had another look over this, with fresh eyes, and only found a couple of nits at this point.

A couple of points about the commit message:

In case of merge conflict while performing a revert

s/conflict/conflicts/ would be more correct here I think.

So, we introduce a separate advice message for git revert.

Prefer the imperative mood in the commit message: "Introduce a separate advice message for git revert." "Us" doing it is not necessary.

I also notice one of the lines is longer than 72 characters in the first commit. Commit messages should always be wrapped at 72 characters or less. I couldn't actually find this anywhere in our docs, but as Git indents the commit message in places e.g. in git log, keeping it to 72 characters or less makes sure it can always fit in 80 characters even when Git indents it.

git am or rebase advise user

s/advice/& the/ is grammatically more correct I think. Though I'm not a native speaker either so I might be off. Same two lines below.

sequencer.c Outdated Show resolved Hide resolved
sequencer.c Outdated Show resolved Hide resolved
builtin/commit.c Outdated Show resolved Hide resolved
@r1walz r1walz force-pushed the cherry-pick--skip branch from 5a17f2b to 5255128 Jun 5, 2019
@r1walz r1walz changed the title add git-cherry-pick --skip Teach cherry-pick/revert to skip commits Jun 6, 2019
r1walz pushed a commit that referenced this issue Jun 6, 2019
Phillip found out that 'git checkout -f <branch>' does not restore
conflict/unmerged files correctly. All tracked files should be taken
from <branch> and all non-zero stages removed. Most of this is true,
except that the final file could be in stage one instead of zero.

"checkout -f" (among other commands) does this with one-way merge, which
is supposed to take stat info from the index and everything else from
the given tree. The add_entry(.., old, ...) call in oneway_merge()
though will keep stage index from the index.

This is normally not a problem if the entry from the index is
normal (stage #0). But if there is a conflict, stage #0 does not exist
and we'll get stage #1 entry as "old" variable, which gets recorded in
the final index. Fix it by clearing stage mask.

This bug probably comes from b5b4250 (git-read-tree: make one-way
merge also honor the "update" flag, 2005-06-07). Before this commit, we
may create the final ("dst") index entry from the one in index, but we
do clear CE_STAGEMASK.

I briefly checked two- and three-way merge functions. I think we don't
have the same problem in those.

Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@r1walz r1walz force-pushed the cherry-pick--skip branch 2 times, most recently from f3a40ec to 034122b Jun 7, 2019
@r1walz
Copy link
Owner Author

@r1walz r1walz commented Jun 8, 2019

@tgummerer @newren Now that v2.22.0 is out, I think I'll send this PR to the mailing list. Any final thoughts?

In the case of merge conflicts, while performing a revert, we are
currently advised to use `git cherry-pick --<sequencer-options>`
of which --continue is incompatible for continuing the revert.
Introduce a separate advice message for `git revert`.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
@r1walz r1walz force-pushed the cherry-pick--skip branch from 034122b to 9b4d89a Jun 8, 2019
Copy link

@newren newren left a comment

A couple minor comments that you could improve, but yeah, I'd say send it off to the list soon.

t/t3510-cherry-pick-sequence.sh Outdated Show resolved Hide resolved
t/t3510-cherry-pick-sequence.sh Outdated Show resolved Hide resolved
Documentation/git-cherry-pick.txt Outdated Show resolved Hide resolved
Documentation/git-revert.txt Outdated Show resolved Hide resolved
r1walz added 2 commits Jun 8, 2019
git am or rebase advise the user to use `git (am | rebase) --skip` to
skip the commit. cherry-pick and revert also have this concept of
skipping commits but they advise the user to use `git reset` (or in
case of a patch which had conflicts, `git reset --merge`) which on the
user's part is annoying and sometimes confusing. Add a `--skip` option
to make these commands more consistent.

In the next commit, we will change the advice messages hence finishing
the process of teaching revert and cherry-pick "how to skip commits."

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
@r1walz r1walz force-pushed the cherry-pick--skip branch from 9b4d89a to 1965b9b Jun 8, 2019
@r1walz
Copy link
Owner Author

@r1walz r1walz commented Jun 8, 2019

--- sent to the Mailing List ---

@r1walz r1walz closed this Jun 8, 2019
r1walz pushed a commit that referenced this issue Aug 10, 2019
Some tests need to create a string of commits. Doing this with
test_commit is very heavy-weight, as it needs at least one process per
commit (and in fact, uses several).

For bulk creation, we can do much better by using fast-import, but it's
often a pain to generate the input. Let's provide a helper to do so.

We'll use t5310 as a guinea pig, as it has three 10-commit loops. Here
are hyperfine results before and after:

  [before]
  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests
    Time (mean ± σ):      2.846 s ±  0.305 s    [User: 3.042 s, System: 0.919 s]
    Range (min … max):    2.250 s …  3.210 s    10 runs

  [after]
  Benchmark #1: ./t5310-pack-bitmaps.sh --root=/var/ram/git-tests
    Time (mean ± σ):      2.210 s ±  0.174 s    [User: 2.570 s, System: 0.604 s]
    Range (min … max):    1.999 s …  2.590 s    10 runs

So we're over 20% faster, while making the callers slightly shorter. We
added a lot more lines in test-lib-function.sh, of course, and the
helper is way more featureful than we need here. But my hope is that it
will be flexible enough to use in more places.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
r1walz pushed a commit that referenced this issue Aug 10, 2019
One of the tests in t3311 creates 300 commits by running "test_commit"
in a loop. This requires 900 processes. Instead, we can use
test_commit_bulk to do it with only four. This improves the runtime of
the script from:

  Benchmark #1: ./t3311-notes-merge-fanout.sh --root=/var/ram/git-tests
    Time (mean ± σ):      5.821 s ±  0.691 s    [User: 3.146 s, System: 2.782 s]
    Range (min … max):    4.783 s …  6.841 s    10 runs

to:

  Benchmark #1: ./t3311-notes-merge-fanout.sh --root=/var/ram/git-tests
    Time (mean ± σ):      1.743 s ±  0.116 s    [User: 1.144 s, System: 0.691 s]
    Range (min … max):    1.629 s …  1.994 s    10 runs

for an average speedup of over 70%.

Unfortunately we still have to run 300 instances of "git notes add",
since the point is to test the fanout that comes from adding notes one
by one.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
r1walz pushed a commit that referenced this issue Aug 10, 2019
There are two loops that create 32 commits each using test_commit. Using
test_commit_bulk speeds this up from:

  Benchmark #1: ./t5702-protocol-v2.sh --root=/var/ram/git-tests
    Time (mean ± σ):      5.409 s ±  0.513 s    [User: 2.382 s, System: 2.466 s]
    Range (min … max):    4.633 s …  5.927 s    10 runs

to:

  Benchmark #1: ./t5702-protocol-v2.sh --root=/var/ram/git-tests
    Time (mean ± σ):      3.956 s ±  0.242 s    [User: 1.775 s, System: 1.627 s]
    Range (min … max):    3.449 s …  4.239 s    10 runs

for an average savings of over 25%.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
r1walz pushed a commit that referenced this issue Aug 10, 2019
There are two loops that create 33 commits each using test_commit. Using
test_commit_bulk speeds this up from:

  Benchmark #1: ./t5703-upload-pack-ref-in-want.sh --root=/var/ram/git-tests
    Time (mean ± σ):      2.142 s ±  0.161 s    [User: 1.136 s, System: 0.974 s]
    Range (min … max):    1.903 s …  2.401 s    10 runs

to:

  Benchmark #1: ./t5703-upload-pack-ref-in-want.sh --root=/var/ram/git-tests
    Time (mean ± σ):      1.440 s ±  0.114 s    [User: 737.7 ms, System: 615.4 ms]
    Range (min … max):    1.230 s …  1.604 s    10 runs

for an average savings of almost 33%.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
r1walz pushed a commit that referenced this issue Aug 10, 2019
There's a loop that creates 30 commits using test_commit. Using
test_commit_bulk speeds this up from:

  Benchmark #1: ./t6200-fmt-merge-msg.sh --root=/var/ram/git-tests
    Time (mean ± σ):      1.926 s ±  0.240 s    [User: 1.055 s, System: 0.963 s]
    Range (min … max):    1.431 s …  2.166 s    10 runs

to:

  Benchmark #1: ./t6200-fmt-merge-msg.sh --root=/var/ram/git-tests
    Time (mean ± σ):      1.343 s ±  0.179 s    [User: 766.5 ms, System: 662.9 ms]
    Range (min … max):    1.032 s …  1.664 s    10 runs

for an average savings of over 30%.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
r1walz pushed a commit that referenced this issue Dec 28, 2019
…ev()

In 'builtin/name-rev.c' in the name_rev() function there is a loop
iterating over all parents of the given commit, and the loop body
looks like this:

  if (parent_number > 1) {
      if (generation > 0)
          // branch #1
          new_name = ...
      else
          // branch #2
          new_name = ...
      name_rev(parent, new_name, ...);
  } else {
      // branch #3
      name_rev(...);
  }

These conditions are not covered properly in the test suite.  As far
as purely test coverage goes, they are all executed several times over
in 't6120-describe.sh'.  However, they don't directly influence the
command's output, because the repository used in that test script
contains several branches and tags pointing somewhere into the middle
of the commit DAG, and thus result in a better name for the
to-be-named commit.  This can hide bugs: e.g. by replacing the
'new_name' parameter of the first recursive name_rev() call with
'tip_name' (effectively making both branch #1 and #2 a noop) 'git
name-rev --all' shows thousands of bogus names in the Git repository,
but the whole test suite still passes successfully.  In an early
version of a later patch in this series I managed to mess up all three
branches (at once!), but the test suite still passed.

So add a new test case that operates on the following history:

  A--------------master
   \            /
    \----------M2
     \        /
      \---M1-C
       \ /
        B

and names the commit 'B' to make sure that all three branches are
crucial to determine 'B's name:

  - There is only a single ref, so all names are based on 'master',
    without any undesired interference from other refs.

  - Each time name_rev() follows the second parent of a merge commit,
    it appends "^2" to the name.  Following 'master's second parent
    right at the start ensures that all commits on the ancestry path
    from 'master' to 'B' have a different base name from the original
    'tip_name' of the very first name_rev() invocation.  Currently,
    while name_rev() is recursive, it doesn't matter, but it will be
    necessary to properly cover all three branches after the recursion
    is eliminated later in this series.

  - Following 'M2's second parent makes sure that branch #2 (i.e. when
    'generation = 0') affects 'B's name.

  - Following the only parent of the non-merge commit 'C' ensures that
    branch #3 affects 'B's name, and that it increments 'generation'.

  - Coming from 'C' 'generation' is 1, thus following 'M1's second
    parent makes sure that branch #1 affects 'B's name.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants