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
On Wednesday, June 3, 2015, Galan Rémi remi.galan-alfo...@ensimag.grenoble-inp.fr 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 EOF +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 2warning 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 EOF +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 2warning + test D = $(git cat-file commit HEAD | sed -ne \$p) + test_cmp warning expect +' + +cat expect EOF +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 2warning + 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
Eric Sunshine sunsh...@sunshineco.com 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
Eric Sunshine sunsh...@sunshineco.com 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 sunsh...@sunshineco.com 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 sunsh...@sunshineco.com 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
Matthieu Moy matthieu@grenoble-inp.fr writes: Galan Rémi remi.galan-alfo...@ensimag.grenoble-inp.fr 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 sunsh...@sunshineco.com 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 remi.galan-alfo...@ensimag.grenoble-inp.fr wrote: Eric Sunshine sunsh...@sunshineco.com 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 matthieu@grenoble-inp.fr writes: Galan Rémi remi.galan-alfo...@ensimag.grenoble-inp.fr 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
Galan Rémi remi.galan-alfo...@ensimag.grenoble-inp.fr 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:52 AM, Galan Rémi remi.galan-alfo...@ensimag.grenoble-inp.fr 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 remi.galan-alfo...@ensimag.grenoble-inp.fr --- 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
Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits
On Mon, Jun 1, 2015 at 7:16 PM, Eric Sunshine sunsh...@sunshineco.com wrote: On Mon, Jun 1, 2015 at 7:52 AM, Galan Rémi remi.galan-alfo...@ensimag.grenoble-inp.fr 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
