@matheusbsbs / matheus-bagliano / commits / 6ce7afe163

rebase --skip: fix commit message clean up when skipping squash

During a series of "fixup" and/or "squash" commands, the interactive rebase accumulates a commit message from all the commits that are being squashed together. If one of the commits has conflicts when it is picked and the user chooses to skip that commit then we need to remove that commit's message from accumulated messages. To do this 15ef69314d5 (rebase --skip: clean up commit message after a failed fixup/squash, 2018-04-27) updated commit_staged_changes() to reset the accumulated message to the commit message of HEAD (which does not contain the message from the skipped commit) when the last command was "fixup" or "squash" and there are no staged changes. Unfortunately the code to do this contains two bugs. (1) If parse_head() fails we pass an invalid pointer to unuse_commit_buffer(). (2) The reconstructed message uses the entire commit buffer from HEAD including the headers, rather than just the commit message. The first issue is fixed by splitting up the "if" condition into several statements each with its own error handling. The second issue is fixed by finding the start of the commit message within the commit buffer using find_commit_subject(). The existing test added by 15ef69314d5 is modified to show the effect of this bug. The bug is triggered when skipping the first command in the chain (as the test does before this commit) but the effect is hidden because opts->current_fixup_count is set to zero which leads update_squash_messages() to recreate the squash message file from scratch overwriting the bad message created by commit_staged_changes(). The test is also updated to explicitly check the commit messages rather than relying on grep to ensure they do not contain any stray commit headers. To check the commit message the function test_commit_message() is moved from t3437-rebase-fixup-options.sh to test-lib.sh. As the function is now publicly available it is updated to provide better error detection and avoid overwriting the commonly used files "actual" and "expect". Support for reading the expected commit message from stdin is also added. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Phillip Wood committed Aug 3, 2023 at 13:09 UTC 6ce7afe16384b741f1ee4c5f310fa4a9f66348ba
4 files changed +93 -39
sequencer.c
+19 -7
@@ -5038,19 +5038,31 @@ static int commit_staged_changes(struct repository *r,
5038 * We need to update the squash message to skip
5039 * the latest commit message.
5040 */
5041 + int res = 0;
5042 struct commit *commit;
5043 + const char *msg;
5044 const char *path = rebase_path_squash_msg();
5045 const char *encoding = get_commit_output_encoding();
5046
5045 - if (parse_head(r, &commit) ||
5046 - !(p = repo_logmsg_reencode(r, commit, NULL, encoding)) ||
5047 - write_message(p, strlen(p), path, 0)) {
5048 - repo_unuse_commit_buffer(r, commit, p);
5049 - return error(_("could not write file: "
5047 + if (parse_head(r, &commit))
5048 + return error(_("could not parse HEAD"));
5049 +
5050 + p = repo_logmsg_reencode(r, commit, NULL, encoding);
5051 + if (!p) {
5052 + res = error(_("could not parse commit %s"),
5053 + oid_to_hex(&commit->object.oid));
5054 + goto unuse_commit_buffer;
5055 + }
5056 + find_commit_subject(p, &msg);
5057 + if (write_message(msg, strlen(msg), path, 0)) {
5058 + res = error(_("could not write file: "
5059 "'%s'"), path);
5060 + goto unuse_commit_buffer;
5061 }
5052 - repo_unuse_commit_buffer(r,
5053 - commit, p);
5062 + unuse_commit_buffer:
5063 + repo_unuse_commit_buffer(r, commit, p);
5064 + if (res)
5065 + return res;
5066 }
5067 }
5068
t/t3418-rebase-continue.sh
+41 -17
@@ -115,15 +115,23 @@ test_expect_success '--skip after failed fixup cleans commit message' '
115 test_when_finished "test_might_fail git rebase --abort" &&
116 git checkout -b with-conflicting-fixup &&
117 test_commit wants-fixup &&
118 - test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 &&
119 - test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
120 - test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
118 + test_commit "fixup 1" wants-fixup.t 1 wants-fixup-1 &&
119 + test_commit "fixup 2" wants-fixup.t 2 wants-fixup-2 &&
120 + test_commit "fixup 3" wants-fixup.t 3 wants-fixup-3 &&
121 test_must_fail env FAKE_LINES="1 fixup 2 squash 4" \
122 git rebase -i HEAD~4 &&
123
124 : now there is a conflict, and comments in the commit message &&
125 - git show HEAD >out &&
126 - grep "fixup! wants-fixup" out &&
125 + test_commit_message HEAD <<-\EOF &&
126 + # This is a combination of 2 commits.
127 + # This is the 1st commit message:
128 +
129 + wants-fixup
130 +
131 + # The commit message #2 will be skipped:
132 +
133 + # fixup 1
134 + EOF
135
136 : skip and continue &&
137 echo "cp \"\$1\" .git/copy.txt" | write_script copy-editor.sh &&
@@ -133,33 +141,49 @@ test_expect_success '--skip after failed fixup cleans commit message' '
141 test_path_is_missing .git/copy.txt &&
142
143 : now the comments in the commit message should have been cleaned up &&
136 - git show HEAD >out &&
137 - ! grep "fixup! wants-fixup" out &&
144 + test_commit_message HEAD -m wants-fixup &&
145
146 : now, let us ensure that "squash" is handled correctly &&
147 git reset --hard wants-fixup-3 &&
141 - test_must_fail env FAKE_LINES="1 squash 4 squash 2 squash 4" \
148 + test_must_fail env FAKE_LINES="1 squash 2 squash 1 squash 3 squash 1" \
149 git rebase -i HEAD~4 &&
150
144 - : the first squash failed, but there are two more in the chain &&
151 + : the second squash failed, but there are two more in the chain &&
152 (test_set_editor "$PWD/copy-editor.sh" &&
153 test_must_fail git rebase --skip) &&
154
155 : not the final squash, no need to edit the commit message &&
156 test_path_is_missing .git/copy.txt &&
157
151 - : The first squash was skipped, therefore: &&
152 - git show HEAD >out &&
153 - test_i18ngrep "# This is a combination of 2 commits" out &&
154 - test_i18ngrep "# This is the commit message #2:" out &&
158 + : The first and third squashes succeeded, therefore: &&
159 + test_commit_message HEAD <<-\EOF &&
160 + # This is a combination of 3 commits.
161 + # This is the 1st commit message:
162 +
163 + wants-fixup
164 +
165 + # This is the commit message #2:
166 +
167 + fixup 1
168 +
169 + # This is the commit message #3:
170 +
171 + fixup 2
172 + EOF
173
174 (test_set_editor "$PWD/copy-editor.sh" && git rebase --skip) &&
157 - git show HEAD >out &&
158 - test_i18ngrep ! "# This is a combination" out &&
175 + test_commit_message HEAD <<-\EOF &&
176 + wants-fixup
177 +
178 + fixup 1
179 +
180 + fixup 2
181 + EOF
182
183 : Final squash failed, but there was still a squash &&
161 - test_i18ngrep "# This is a combination of 2 commits" .git/copy.txt &&
162 - test_i18ngrep "# This is the commit message #2:" .git/copy.txt
184 + head -n1 .git/copy.txt >first-line &&
185 + test_i18ngrep "# This is a combination of 3 commits" first-line &&
186 + test_i18ngrep "# This is the commit message #3:" .git/copy.txt
187 '
188
189 test_expect_success 'setup rerere database' '
t/t3437-rebase-fixup-options.sh
-15
@@ -21,21 +21,6 @@ TEST_PASSES_SANITIZE_LEAK=true
21
22 EMPTY=""
23
24 -# test_commit_message <rev> -m <msg>
25 -# test_commit_message <rev> <path>
26 -# Verify that the commit message of <rev> matches
27 -# <msg> or the content of <path>.
28 -test_commit_message () {
29 - git show --no-patch --pretty=format:%B "$1" >actual &&
30 - case "$2" in
31 - -m)
32 - echo "$3" >expect &&
33 - test_cmp expect actual ;;
34 - *)
35 - test_cmp "$2" actual ;;
36 - esac
37 -}
38 -
24 get_author () {
25 rev="$1" &&
26 git log -1 --pretty=format:"%an %ae %at" "$rev"
t/test-lib-functions.sh
+33
@@ -1273,6 +1273,39 @@ test_cmp_rev () {
1273 fi
1274 }
1275
1276 +# Tests that a commit message matches the expected text
1277 +#
1278 +# Usage: test_commit_message <rev> [-m <msg> | <file>]
1279 +#
1280 +# When using "-m" <msg> will have a line feed appended. If the second
1281 +# argument is omitted then the expected message is read from stdin.
1282 +
1283 +test_commit_message () {
1284 + local msg_file=expect.msg
1285 +
1286 + case $# in
1287 + 3)
1288 + if test "$2" = "-m"
1289 + then
1290 + printf "%s\n" "$3" >"$msg_file"
1291 + else
1292 + BUG "Usage: test_commit_message <rev> [-m <message> | <file>]"
1293 + fi
1294 + ;;
1295 + 2)
1296 + msg_file="$2"
1297 + ;;
1298 + 1)
1299 + cat >"$msg_file"
1300 + ;;
1301 + *)
1302 + BUG "Usage: test_commit_message <rev> [-m <message> | <file>]"
1303 + ;;
1304 + esac
1305 + git show --no-patch --pretty=format:%B "$1" -- >actual.msg &&
1306 + test_cmp "$msg_file" actual.msg
1307 +}
1308 +
1309 # Compare paths respecting core.ignoreCase
1310 test_cmp_fspath () {
1311 if test "x$1" = "x$2"