Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits
I'm going to try to change the die_abort in this patch by a die, so that the user can use rebase --edit-todo afterward. This way, adding the checking on the SHA-1 for the 'drop' command (discussed in 1/2) (and also maybe the other commands requiring a correct SHA-1 corresponding to a commit) to the 2/2 part would make a bit more sense. Though I still see some other issues with this, I agree that it makes more sense in 2/2 rather than in 1/2 (some more checking in a future patch would be a good idea). (So far I've tried rather quickly, but it doesn't seem as easy as I originally though, working on it though) Rémi -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits
Eric Sunshine writes: > > +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=ignore' > > ' > > + test_config rebase.missingCommitsCheck ignore && > > + test_when_finished "git checkout master && > > + git branch -D tmp2" && > > Strange indentation. Considering that 'git branch -D tmp2' is a part of test_when_finished, I wasn't sure of how it was supposed to be indented, so I did it this way to show that it was still within test_when_finished and not a separate command. > test_when_finished "git checkout master && > git branch -D tmp2" && Doesn't seem as clear, especially if you quickly read the lines. For now, I have removed the tab. Also the other points have been corrected. Thank you, Rémi -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits
On Wednesday, June 3, 2015, Galan Rémi wrote: > Check if commits were removed (i.e. a line was deleted) and print > warnings or abort git rebase depending on the value of the > configuration variable rebase.missingCommits. A few comments below in addition to those already made by Matthieu... > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 8960083..f369d2c 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1112,4 +1112,67 @@ test_expect_success 'drop' ' > test A = $(git cat-file commit HEAD^^ | sed -ne \$p) > ' > > +cat >expect < +Successfully rebased and updated refs/heads/tmp2. > +EOF > + > +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=ignore' ' > + test_config rebase.missingCommitsCheck ignore && > + test_when_finished "git checkout master && > + git branch -D tmp2" && Strange indentation. > + git checkout -b tmp2 master && > + set_fake_editor && > + FAKE_LINES="1 2 3 4" \ > + git rebase -i --root 2>warning && The file containing the actual output is usually spelled "actual". > + test D = $(git cat-file commit HEAD | sed -ne \$p) && > + test_cmp warning expect The arguments to test_cmp are usually reversed so that 'expect' comes before 'actual', which results in a more natural-feeling diff when test_cmp detects that the files differ. These comments apply to remaining new tests, as well. > +' > + > +cat >expect < +Warning: some commits may have been dropped accidentally. > +Dropped commits (newer to older): > + - $(git rev-list --pretty=oneline --abbrev-commit -1 master) > + > +To avoid this message, use "drop" to explicitly remove a commit. > +Use git --config rebase.missingCommitsCheck to change the level of warnings > (ignore, warn, error). > + > +Successfully rebased and updated refs/heads/tmp2. > +EOF > + > +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=warn' ' > + test_config rebase.missingCommitsCheck warn && > + test_when_finished "git checkout master && > + git branch -D tmp2" && > + git checkout -b tmp2 master && > + set_fake_editor && > + FAKE_LINES="1 2 3 4" \ > + git rebase -i --root 2>warning && > + test D = $(git cat-file commit HEAD | sed -ne \$p) && > + test_cmp warning expect > +' > + > +cat >expect < +Warning: some commits may have been dropped accidentally. > +Dropped commits (newer to older): > + - $(git rev-list --pretty=oneline --abbrev-commit -1 master) > + - $(git rev-list --pretty=oneline --abbrev-commit -1 master~2) > + > +To avoid this message, use "drop" to explicitly remove a commit. > +Use git --config rebase.missingCommitsCheck to change the level of warnings > (ignore, warn, error). > + > +Rebase aborted due to dropped commits. > +EOF > + > +test_expect_success 'rebase -i respects rebase.missingCommitsCheck=error' ' > + test_config rebase.missingCommitsCheck error && > + test_when_finished "git checkout master && > + git branch -D tmp2" && > + git checkout -b tmp2 master && > + set_fake_editor && > + test_must_fail env FAKE_LINES="1 2 4" \ > + git rebase -i --root 2>warning && > + test E = $(git cat-file commit HEAD | sed -ne \$p) && > + test_cmp warning expect > +' > + > test_done > -- > 2.4.2.389.geaf7ccf -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits
Matthieu Moy writes: > Galan Rémi writes: > >> Check if commits were removed (i.e. a line was deleted) and print >> warnings or abort git rebase according to the value of the >> configuration variable rebase.checkLevel. > > checkLevel does not seem to be a good name, because it doesn't say > _what_ is checked. I don't have really good proposal in mind, but maybe > > rebase.todoListCheckLevel > rebase.missinLinesCheckLevel Yeah, with s/sin/sing/, but the above makes more sense. It may be that we probably do not even need "Level" there in the name. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits
Eric Sunshine writes: > Although the underlying behavior of "ignore" may be the same as the > default, you also want to check that higher-level machinery for > recognizing the "ignore" option is working correctly, which is why > checking "ignore" explicitly is a reasonable thing to do. Noted, I'm adding a test for "ignore". Thank you, Rémi -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits
On Tue, Jun 2, 2015 at 3:42 AM, Remi Galan Alfonso wrote: > Eric Sunshine writes: >> > +test_expect_success 'rebase -i respects rebase.checkLevel' ' >> > + test_config rebase.checkLevel error && >> > + test_when_finished "git checkout master" && >> > + git checkout -b tmp2 master && >> > + set_fake_editor && >> > + test_must_fail env FAKE_LINES="1 2 3 4" git rebase -i --root && >> > + test E = $(git cat-file commit HEAD | sed -ne \$p) >> > +' >> >> Shouldn't you also explicitly test "warn" and "ignore" modes? > > I don't think testing "ignore" is really necessary since it > corresponds to the default behaviour, it is thus silently tested by > the other tests. Although the underlying behavior of "ignore" may be the same as the default, you also want to check that higher-level machinery for recognizing the "ignore" option is working correctly, which is why checking "ignore" explicitly is a reasonable thing to do. > Either way, I will add a test for "warn". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits
Matthieu Moy writes: > Galan Rémi writes: > > > Check if commits were removed (i.e. a line was deleted) and print > > warnings or abort git rebase according to the value of the > > configuration variable rebase.checkLevel. > > checkLevel does not seem to be a good name, because it doesn't say > _what_ is checked. I don't have really good proposal in mind, but maybe > > rebase.todoListCheckLevel > rebase.missinLinesCheckLevel > > ? I agree that we could use a better name. I'm thinking about > rebase.missingLinesCheckLevel or rebase.missingCommitsCheckLevel but I am worried that these names are too long. Maybe rebase.dropCheckLevel I don't really have better propositions right now. Rémi -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits
Eric Sunshine writes: > > +rebase.checkLevel:: > > + If set to "warn", git rebase -i will print a warning if some > > + commits are removed (i.e. a line was deleted) or if some > > + commits appear more than one time (e.g. the same commit is > > + picked twice), however the rebase will still proceed. If set > > The cover letter says that v2 no longer checks for a duplicate, > however, this documentation still mentions it. > > > +rebase.checkLevel:: > > + If set to "warn" print warnings about removed commits and > > + duplicated commits in interactive mode. If set to "error" > > Same here. I forgot to modify the documentation, really sorry about that. Corrected here. Eric Sunshine writes: > > test_expect_success 'drop' ' > > - git checkout master && > > test_when_finished "git checkout master" && > > git checkout -b dropBranchTest master && > > This "cleanup" change might deserve its own patch (or at least a > mention in the commit message). I will rather move the this cleanup to the first part of this patch, where the test was added (it should cause no problem, since the patch is still in discussion), it makes more sense. I actually thought that was what I did but seems like I was wrong. I forgot to double check my patch this time, and it is showing, my bad. Eric Sunshine writes: > > +test_expect_success 'rebase -i respects rebase.checkLevel' ' > > + test_config rebase.checkLevel error && > > + test_when_finished "git checkout master" && > > + git checkout -b tmp2 master && > > + set_fake_editor && > > + test_must_fail env FAKE_LINES="1 2 3 4" git rebase -i --root && > > + test E = $(git cat-file commit HEAD | sed -ne \$p) > > +' > > Shouldn't you also explicitly test "warn" and "ignore" modes? I don't think testing "ignore" is really necessary since it corresponds to the default behaviour, it is thus silently tested by the other tests. Either way, I will add a test for "warn". Rémi -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits
Galan Rémi writes: > Check if commits were removed (i.e. a line was deleted) and print > warnings or abort git rebase according to the value of the > configuration variable rebase.checkLevel. checkLevel does not seem to be a good name, because it doesn't say _what_ is checked. I don't have really good proposal in mind, but maybe rebase.todoListCheckLevel rebase.missinLinesCheckLevel ? -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits
On Mon, Jun 1, 2015 at 7:16 PM, Eric Sunshine wrote: > On Mon, Jun 1, 2015 at 7:52 AM, Galan Rémi > wrote: >> test_expect_success 'drop' ' >> - git checkout master && >> test_when_finished "git checkout master" && >> git checkout -b dropBranchTest master && > > This "cleanup" change might deserve its own test (or at least a > mention in the commit message). "...might deserve its own _patch_..." -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits
On Mon, Jun 1, 2015 at 7:52 AM, Galan Rémi wrote: > Check if commits were removed (i.e. a line was deleted) and print > warnings or abort git rebase according to the value of the > configuration variable rebase.checkLevel. > > Add the configuration variable rebase.checkLevel. > - When unset or set to "ignore", no checking is done. > - When set to "warn", the commits are checked, warnings are > displayed but git rebase still proceeds. > - When set to "error", the commits are checked, warnings are > displayed and the rebase is aborted. > > rebase.checkLevel defaults to "ignore". > > Signed-off-by: Galan Rémi > --- > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 5f76e8c..e2e5554 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -2160,6 +2160,15 @@ rebase.autoStash:: > successful rebase might result in non-trivial conflicts. > Defaults to false. > > +rebase.checkLevel:: > + If set to "warn", git rebase -i will print a warning if some > + commits are removed (i.e. a line was deleted) or if some > + commits appear more than one time (e.g. the same commit is > + picked twice), however the rebase will still proceed. If set The cover letter says that v2 no longer checks for a duplicate, however, this documentation still mentions it. > + to "error", it will print the previous warnings and abort the > + rebase. If set to "ignore", no checking is done. Defaults to > + "ignore". > + > receive.advertiseAtomic:: > By default, git-receive-pack will advertise the atomic push > capability to its clients. If you don't want to this capability > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 9cf3760..d348ca2 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -213,6 +213,12 @@ rebase.autoSquash:: > rebase.autoStash:: > If set to true enable '--autostash' option by default. > > +rebase.checkLevel:: > + If set to "warn" print warnings about removed commits and > + duplicated commits in interactive mode. If set to "error" Same here. > + print the warnings and abort the rebase. If set to "ignore" no > + checking is done. "ignore" by default. > + > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 1bad068..d3a9ed5 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1103,7 +1103,6 @@ test_expect_success 'rebase -i commits that overwrite > untracked files (no ff)' ' > ' > > test_expect_success 'drop' ' > - git checkout master && > test_when_finished "git checkout master" && > git checkout -b dropBranchTest master && This "cleanup" change might deserve its own test (or at least a mention in the commit message). > set_fake_editor && > @@ -1113,4 +1112,13 @@ test_expect_success 'drop' ' > test A = $(git cat-file commit HEAD^^ | sed -ne \$p) > ' > > +test_expect_success 'rebase -i respects rebase.checkLevel' ' > + test_config rebase.checkLevel error && > + test_when_finished "git checkout master" && > + git checkout -b tmp2 master && > + set_fake_editor && > + test_must_fail env FAKE_LINES="1 2 3 4" git rebase -i --root && > + test E = $(git cat-file commit HEAD | sed -ne \$p) > +' Shouldn't you also explicitly test "warn" and "ignore" modes? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
