| Message ID | efdd3736a96f90a4ab52acaf2e5efbe3435bcb89.1542065154.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Assorted fixes revolving around rebase and merges | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > When calling `merge` on a branch that has already been merged, that > `merge` is skipped quietly, but currently a MERGE_HEAD file is being > left behind and will then be grabbed by the next `pick` (that did > not want to create a *merge* commit). > > Demonstrate this. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > t/t3430-rebase-merges.sh | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) For a trivially small change/fix like this, it is OK and even preferrable to make 1+2 a single step, as applying t/ part only to try to see the breakage (or "am"ing everything and then "diff | apply -R" the part outside t/ for the same purpose) is easy enough. Because the patch 2 with your method ends up showing only the test set-up part in the context by changing _failure to _success, without showing what end-user visible breakage the step fixed, which usually comes near the end of the added test piece. A single patch that gives tests that ought to succeed would not force the readers to switch between patches 1 and 2 while reading the fix. Of course, the above would not apply for a more involved case where the actual fix to the code needs to span multiple patches. Thanks. > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > index aa7bfc88ec..1f08a33687 100755 > --- a/t/t3430-rebase-merges.sh > +++ b/t/t3430-rebase-merges.sh > @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' ' > grep "G: +G" actual > ' > > +test_expect_failure '--continue after resolving conflicts after a merge' ' > + git checkout -b already-has-g E && > + git cherry-pick E..G && > + test_commit H2 && > + > + git checkout -b conflicts-in-merge H && > + test_commit H2 H2.t conflicts H2-conflict && > + test_must_fail git rebase -r already-has-g && > + grep conflicts H2.t && Is this making sure that the above test_must_fail succeeded because of a conflict and not due to any other failure? I would have used "ls-files -u H2.t" to see if the index is unmerged, which probably is a more direct way to test what this is trying to test, but if we are in the conflicted state, the one side of << == >> has this string (the other has "H2" in it, presumably?), so in practice this should be good enough. > + echo resolved >H2.t && > + git add -u && and we resolve to continue. > + git rebase --continue && > + test_must_fail git rev-parse --verify HEAD^2 && Even if we made an octopus by mistake, the above will catch it, which is good. > + test_path_is_missing .git/MERGE_HEAD > +' > + > test_done And from the proposed log message, I am reading that the last two things (i.e. resulting tip is a child with a single parent and there is no leftover MERGE_HEAD file) fail without the fix. This is enough material to convince me or anybody that the bug is worth fixing. Thanks for being careful noticing a glitch during your real (and otherwise unrelated to the bug) work and following through.
Hi Junio, On Tue, 13 Nov 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > When calling `merge` on a branch that has already been merged, that > > `merge` is skipped quietly, but currently a MERGE_HEAD file is being > > left behind and will then be grabbed by the next `pick` (that did > > not want to create a *merge* commit). > > > > Demonstrate this. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > t/t3430-rebase-merges.sh | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > For a trivially small change/fix like this, it is OK and even > preferrable to make 1+2 a single step, as applying t/ part only to > try to see the breakage (or "am"ing everything and then "diff | > apply -R" the part outside t/ for the same purpose) is easy enough. I disagree. It helps both development and porting to different branches to be able to cherry-pick the regression test individually. Please do not ask me to violate this hard-learned principle. > Because the patch 2 with your method ends up showing only the test > set-up part in the context by changing _failure to _success, without > showing what end-user visible breakage the step fixed, which usually > comes near the end of the added test piece. A single patch that > gives tests that ought to succeed would not force the readers to > switch between patches 1 and 2 while reading the fix. That is why I put in a verbose commit message, so that you do not have to guess. And even the test title talks about this. Seriously, I am very much opposed to changing the patches in the direction you suggested. In my mind, they would make the story substantially worse. Thank you for your review, Dscho > > Of course, the above would not apply for a more involved case where > the actual fix to the code needs to span multiple patches. > > Thanks. > > > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > > index aa7bfc88ec..1f08a33687 100755 > > --- a/t/t3430-rebase-merges.sh > > +++ b/t/t3430-rebase-merges.sh > > @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' ' > > grep "G: +G" actual > > ' > > > > +test_expect_failure '--continue after resolving conflicts after a merge' ' > > + git checkout -b already-has-g E && > > + git cherry-pick E..G && > > + test_commit H2 && > > + > > + git checkout -b conflicts-in-merge H && > > + test_commit H2 H2.t conflicts H2-conflict && > > + test_must_fail git rebase -r already-has-g && > > + grep conflicts H2.t && > > Is this making sure that the above test_must_fail succeeded because > of a conflict and not due to any other failure? I would have used > "ls-files -u H2.t" to see if the index is unmerged, which probably > is a more direct way to test what this is trying to test, but if we > are in the conflicted state, the one side of << == >> has this > string (the other has "H2" in it, presumably?), so in practice this > should be good enough. > > > + echo resolved >H2.t && > > + git add -u && > > and we resolve to continue. > > > + git rebase --continue && > > + test_must_fail git rev-parse --verify HEAD^2 && > > Even if we made an octopus by mistake, the above will catch it, > which is good. > > > + test_path_is_missing .git/MERGE_HEAD > > +' > > + > > test_done > > And from the proposed log message, I am reading that the last two > things (i.e. resulting tip is a child with a single parent and there > is no leftover MERGE_HEAD file) fail without the fix. > > This is enough material to convince me or anybody that the bug is > worth fixing. Thanks for being careful noticing a glitch during > your real (and otherwise unrelated to the bug) work and following > through. >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> For a trivially small change/fix like this, it is OK and even >> preferrable to make 1+2 a single step, as applying t/ part only to >> try to see the breakage (or "am"ing everything and then "diff | >> apply -R" the part outside t/ for the same purpose) is easy enough. > > I disagree. It helps both development and porting to different branches to > be able to cherry-pick the regression test individually. Please do not ask > me to violate this hard-learned principle. A trivially small change/fix like this, by definition (of "trivial" and "small" ness), it is trivial to develop and port to different branches a single patch, and test with just one half (either the test part or the code-change part) of the change reversed, to ensure that the codebase is broken without the code-change and to demonstrate that the code-change does fix the problem revealed by the test change. And "porting" by cherry-picking a single patch is always easier than two patch series. So you may disagree all you want in your project, but do not make reviewer's lives unnecessarily harder in this project. Thanks.
Hi Junio, On Tue, 13 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > >> For a trivially small change/fix like this, it is OK and even > >> preferrable to make 1+2 a single step, as applying t/ part only to > >> try to see the breakage (or "am"ing everything and then "diff | > >> apply -R" the part outside t/ for the same purpose) is easy enough. > > > > I disagree. It helps both development and porting to different branches to > > be able to cherry-pick the regression test individually. Please do not ask > > me to violate this hard-learned principle. > > A trivially small change/fix like this, by definition (of "trivial" > and "small" ness), it is trivial to develop and port to different > branches a single patch, and test with just one half (either the > test part or the code-change part) of the change reversed, to ensure > that the codebase is broken without the code-change and to > demonstrate that the code-change does fix the problem revealed by > the test change. And "porting" by cherry-picking a single patch is > always easier than two patch series. > > So you may disagree all you want in your project, but do not make > reviewer's lives unnecessarily harder in this project. You misunderstand. In this case it is crucial to read the regression test first. The fix does not make much sense before one understands the condition under which the order of the code statements matters. By trying to force me to smoosh them together, you are trying to force me to combine them in one patch where you would read the (now seemingly non-sensical) fix first, and only then the test. That's just really unhelpful. If I were a reviewer, I would want it presented in the way it *was* presented. I firmly believe most reviewers would. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > You misunderstand. In this case it is crucial to read the regression test > first. The fix does not make much sense before one understands the > condition under which the order of the code statements matters. I am not sure what you mean. It sounds as if you want to use diff-orderfile to present change for t/ before changes to other files are presented in format-patch output to help readers, and I think that may make sense for certain cases. It may even include this case. But that is not incompatible with "avoid showing the patch that updates the code to fix breakages separately, ending up with showing the changes to t/ that are mostly about s/_failure/_success/ and readers are forced to go back to the previous patch to remind themselves what the broken scenario was about; by keeping it in a single patch, the readers can get the tests in one piece".
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index aa7bfc88ec..1f08a33687 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' ' grep "G: +G" actual ' +test_expect_failure '--continue after resolving conflicts after a merge' ' + git checkout -b already-has-g E && + git cherry-pick E..G && + test_commit H2 && + + git checkout -b conflicts-in-merge H && + test_commit H2 H2.t conflicts H2-conflict && + test_must_fail git rebase -r already-has-g && + grep conflicts H2.t && + echo resolved >H2.t && + git add -u && + git rebase --continue && + test_must_fail git rev-parse --verify HEAD^2 && + test_path_is_missing .git/MERGE_HEAD +' + test_done