From bf3de2b373d4fa55b6040c7dc6f7f8668ef45c19 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Wed, 14 Dec 2011 22:24:28 +0530 Subject: revert: free msg in format_todo() Memory allocated to the fields of msg by get_message() isn't freed. This is potentially a big leak, because fresh memory is allocated to store the commit message for each commit. Fix this using free_message(). Reported-by: Jonathan Nieder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano diff --git a/builtin/revert.c b/builtin/revert.c index 028bcbc..76a1633 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -709,6 +709,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list, if (get_message(cur->item, &msg)) return error(_("Cannot get commit message for %s"), sha1_abbrev); strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject); + free_message(&msg); } return 0; } -- cgit v0.10.2-6-g49f6 From 6bc1a235b1260a8395045261f8004ba9f12677c7 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Wed, 14 Dec 2011 22:24:29 +0530 Subject: revert: make commit subjects in insn sheet optional Change the instruction sheet format subtly so that the subject of the commit message that follows the object name is optional. As a result, an instruction sheet like this is now perfectly valid: pick 35b0426 pick fbd5bbcbc2e pick 7362160f While at it, also fix a bug introduced by 5a5d80f4 (revert: Introduce --continue to continue the operation, 2011-08-04) that failed to read lines that are too long to fit on the commit-id-shaped buffer we currently use; eliminate the need for the buffer altogether. In addition to literal SHA-1 hexes, you can now safely use expressions like the following in the instruction sheet: featurebranch~4 rr/revert-cherry-pick-continue^2~12@{12 days ago} [jc: simplify parsing] Suggested-by: Jonathan Nieder Helped-by: Junio C Hamano Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano diff --git a/builtin/revert.c b/builtin/revert.c index 76a1633..6d520ae 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -714,31 +714,27 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list, return 0; } -static struct commit *parse_insn_line(char *start, struct replay_opts *opts) +static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts) { unsigned char commit_sha1[20]; - char sha1_abbrev[40]; enum replay_action action; - int insn_len = 0; - char *p, *q; + char *end_of_object_name; + int saved, status; - if (!prefixcmp(start, "pick ")) { + if (!prefixcmp(bol, "pick ")) { action = CHERRY_PICK; - insn_len = strlen("pick"); - p = start + insn_len + 1; - } else if (!prefixcmp(start, "revert ")) { + bol += strlen("pick "); + } else if (!prefixcmp(bol, "revert ")) { action = REVERT; - insn_len = strlen("revert"); - p = start + insn_len + 1; + bol += strlen("revert "); } else return NULL; - q = strchr(p, ' '); - if (!q) - return NULL; - q++; - - strlcpy(sha1_abbrev, p, q - p); + end_of_object_name = bol + strcspn(bol, " \n"); + saved = *end_of_object_name; + *end_of_object_name = '\0'; + status = get_sha1(bol, commit_sha1); + *end_of_object_name = saved; /* * Verify that the action matches up with the one in @@ -751,7 +747,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts) return NULL; } - if (get_sha1(sha1_abbrev, commit_sha1) < 0) + if (status < 0) return NULL; return lookup_commit_reference(commit_sha1); @@ -766,13 +762,12 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list, int i; for (i = 1; *p; i++) { - commit = parse_insn_line(p, opts); + char *eol = strchrnul(p, '\n'); + commit = parse_insn_line(p, eol, opts); if (!commit) return error(_("Could not parse line %d."), i); next = commit_list_append(commit, next); - p = strchrnul(p, '\n'); - if (*p) - p++; + p = *eol ? eol + 1 : eol; } if (!*todo_list) return error(_("No commits parsed.")); diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index e80050e..1820a42 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -14,6 +14,9 @@ test_description='Test cherry-pick continuation features . ./test-lib.sh +# Repeat first match 10 times +_r10='\1\1\1\1\1\1\1\1\1\1' + pristine_detach () { git cherry-pick --quit && git checkout -f "$1^0" && @@ -478,4 +481,29 @@ test_expect_success 'empty commit set' ' test_expect_code 128 git cherry-pick base..base ' +test_expect_success 'malformed instruction sheet 3' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + echo "resolved" >foo && + git add foo && + git commit && + sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet && + cp new_sheet .git/sequencer/todo && + test_must_fail git cherry-pick --continue +' + +test_expect_success 'commit descriptions in insn sheet are optional' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + echo "c" >foo && + git add foo && + git commit && + cut -d" " -f1,2 .git/sequencer/todo >new_sheet && + cp new_sheet .git/sequencer/todo && + git cherry-pick --continue && + test_path_is_missing .git/sequencer && + git rev-list HEAD >commits && + test_line_count = 4 commits +' + test_done -- cgit v0.10.2-6-g49f6 From 0db76962d156c196a23a98014c1585246f561a5d Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Wed, 14 Dec 2011 22:24:30 +0530 Subject: revert: tolerate extra spaces, tabs in insn sheet Tolerate extra spaces and tabs as part of the the field separator in '.git/sequencer/todo', for people with fat fingers. Suggested-by: Junio C Hamano Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano diff --git a/builtin/revert.c b/builtin/revert.c index 6d520ae..164552e 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -719,18 +719,24 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts * unsigned char commit_sha1[20]; enum replay_action action; char *end_of_object_name; - int saved, status; + int saved, status, padding; - if (!prefixcmp(bol, "pick ")) { + if (!prefixcmp(bol, "pick")) { action = CHERRY_PICK; - bol += strlen("pick "); - } else if (!prefixcmp(bol, "revert ")) { + bol += strlen("pick"); + } else if (!prefixcmp(bol, "revert")) { action = REVERT; - bol += strlen("revert "); + bol += strlen("revert"); } else return NULL; - end_of_object_name = bol + strcspn(bol, " \n"); + /* Eat up extra spaces/ tabs before object name */ + padding = strspn(bol, " \t"); + if (!padding) + return NULL; + bol += padding; + + end_of_object_name = bol + strcspn(bol, " \t\n"); saved = *end_of_object_name; *end_of_object_name = '\0'; status = get_sha1(bol, commit_sha1); diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 1820a42..1069857 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -492,6 +492,17 @@ test_expect_success 'malformed instruction sheet 3' ' test_must_fail git cherry-pick --continue ' +test_expect_success 'instruction sheet, fat-fingers version' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + echo "c" >foo && + git add foo && + git commit && + sed "s/pick \([0-9a-f]*\)/pick \1 /" .git/sequencer/todo >new_sheet && + cp new_sheet .git/sequencer/todo && + git cherry-pick --continue +' + test_expect_success 'commit descriptions in insn sheet are optional' ' pristine_detach initial && test_must_fail git cherry-pick base..anotherpick && -- cgit v0.10.2-6-g49f6 From 9e1313648d4c0ee1bab0ee3d7ed22553bd5bf87c Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Wed, 14 Dec 2011 22:24:31 +0530 Subject: revert: simplify getting commit subject in format_todo() format_todo() calls get_message(), but uses only the subject line of the commit message. As a minor optimization, save work and unnecessary memory allocations by using find_commit_subject() instead. Also, remove the unnecessary check on cur->item->buffer: the lookup_commit_reference() call in parse_insn_line() has already made sure of this. Suggested-by: Jonathan Nieder Helped-by: Junio C Hamano Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano diff --git a/builtin/revert.c b/builtin/revert.c index 164552e..0a86fec 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -700,16 +700,16 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list, struct replay_opts *opts) { struct commit_list *cur = NULL; - struct commit_message msg = { NULL, NULL, NULL, NULL, NULL }; const char *sha1_abbrev = NULL; const char *action_str = opts->action == REVERT ? "revert" : "pick"; + const char *subject; + int subject_len; for (cur = todo_list; cur; cur = cur->next) { sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV); - if (get_message(cur->item, &msg)) - return error(_("Cannot get commit message for %s"), sha1_abbrev); - strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject); - free_message(&msg); + subject_len = find_commit_subject(cur->item->buffer, &subject); + strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev, + subject_len, subject); } return 0; } -- cgit v0.10.2-6-g49f6 From c6b7c7f3050977577af49e5d81340fae3b5bf2e7 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Wed, 14 Dec 2011 22:24:32 +0530 Subject: t3510 (cherry-pick-sequencer): use exit status All the tests asserting failure use 'test_must_fail', which simply checks for a non-zero exit status, potentially hiding underlying bugs. So, replace instances of 'test_must_fail' with 'test_expect_code' to check the exit status explicitly, where appropriate. Reviewed-by: Jonathan Nieder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 1069857..f13a194 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -46,7 +46,7 @@ test_expect_success setup ' test_expect_success 'cherry-pick persists data on failure' ' pristine_detach initial && - test_must_fail git cherry-pick -s base..anotherpick && + test_expect_code 1 git cherry-pick -s base..anotherpick && test_path_is_dir .git/sequencer && test_path_is_file .git/sequencer/head && test_path_is_file .git/sequencer/todo && @@ -67,7 +67,7 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' ' test_expect_success 'cherry-pick persists opts correctly' ' pristine_detach initial && - test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick && + test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick && test_path_is_dir .git/sequencer && test_path_is_file .git/sequencer/head && test_path_is_file .git/sequencer/todo && @@ -107,7 +107,7 @@ test_expect_success '--abort requires cherry-pick in progress' ' test_expect_success '--quit cleans up sequencer state' ' pristine_detach initial && - test_must_fail git cherry-pick base..picked && + test_expect_code 1 git cherry-pick base..picked && git cherry-pick --quit && test_path_is_missing .git/sequencer ' @@ -121,7 +121,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' ' :000000 100644 OBJID OBJID A foo :000000 100644 OBJID OBJID A unrelated EOF - test_must_fail git cherry-pick base..picked && + test_expect_code 1 git cherry-pick base..picked && git cherry-pick --quit && test_path_is_missing .git/sequencer && test_must_fail git update-index --refresh && @@ -135,7 +135,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' ' test_expect_success '--abort to cancel multiple cherry-pick' ' pristine_detach initial && - test_must_fail git cherry-pick base..anotherpick && + test_expect_code 1 git cherry-pick base..anotherpick && git cherry-pick --abort && test_path_is_missing .git/sequencer && test_cmp_rev initial HEAD && @@ -145,7 +145,7 @@ test_expect_success '--abort to cancel multiple cherry-pick' ' test_expect_success '--abort to cancel single cherry-pick' ' pristine_detach initial && - test_must_fail git cherry-pick picked && + test_expect_code 1 git cherry-pick picked && git cherry-pick --abort && test_path_is_missing .git/sequencer && test_cmp_rev initial HEAD && @@ -155,7 +155,7 @@ test_expect_success '--abort to cancel single cherry-pick' ' test_expect_success 'cherry-pick --abort to cancel multiple revert' ' pristine_detach anotherpick && - test_must_fail git revert base..picked && + test_expect_code 1 git revert base..picked && git cherry-pick --abort && test_path_is_missing .git/sequencer && test_cmp_rev anotherpick HEAD && @@ -165,7 +165,7 @@ test_expect_success 'cherry-pick --abort to cancel multiple revert' ' test_expect_success 'revert --abort works, too' ' pristine_detach anotherpick && - test_must_fail git revert base..picked && + test_expect_code 1 git revert base..picked && git revert --abort && test_path_is_missing .git/sequencer && test_cmp_rev anotherpick HEAD @@ -173,7 +173,7 @@ test_expect_success 'revert --abort works, too' ' test_expect_success '--abort to cancel single revert' ' pristine_detach anotherpick && - test_must_fail git revert picked && + test_expect_code 1 git revert picked && git revert --abort && test_path_is_missing .git/sequencer && test_cmp_rev anotherpick HEAD && @@ -184,7 +184,7 @@ test_expect_success '--abort to cancel single revert' ' test_expect_success '--abort keeps unrelated change, easy case' ' pristine_detach unrelatedpick && echo changed >expect && - test_must_fail git cherry-pick picked..yetanotherpick && + test_expect_code 1 git cherry-pick picked..yetanotherpick && echo changed >unrelated && git cherry-pick --abort && test_cmp expect unrelated @@ -193,7 +193,7 @@ test_expect_success '--abort keeps unrelated change, easy case' ' test_expect_success '--abort refuses to clobber unrelated change, harder case' ' pristine_detach initial && echo changed >expect && - test_must_fail git cherry-pick base..anotherpick && + test_expect_code 1 git cherry-pick base..anotherpick && echo changed >unrelated && test_must_fail git cherry-pick --abort && test_cmp expect unrelated && @@ -208,7 +208,7 @@ test_expect_success '--abort refuses to clobber unrelated change, harder case' ' test_expect_success 'cherry-pick still writes sequencer state when one commit is left' ' pristine_detach initial && - test_must_fail git cherry-pick base..picked && + test_expect_code 1 git cherry-pick base..picked && test_path_is_dir .git/sequencer && echo "resolved" >foo && git add foo && @@ -232,7 +232,7 @@ test_expect_success 'cherry-pick still writes sequencer state when one commit is test_expect_success '--abort after last commit in sequence' ' pristine_detach initial && - test_must_fail git cherry-pick base..picked && + test_expect_code 1 git cherry-pick base..picked && git cherry-pick --abort && test_path_is_missing .git/sequencer && test_cmp_rev initial HEAD && @@ -242,22 +242,22 @@ test_expect_success '--abort after last commit in sequence' ' test_expect_success 'cherry-pick does not implicitly stomp an existing operation' ' pristine_detach initial && - test_must_fail git cherry-pick base..anotherpick && + test_expect_code 1 git cherry-pick base..anotherpick && test-chmtime -v +0 .git/sequencer >expect && - test_must_fail git cherry-pick unrelatedpick && + test_expect_code 128 git cherry-pick unrelatedpick && test-chmtime -v +0 .git/sequencer >actual && test_cmp expect actual ' test_expect_success '--continue complains when no cherry-pick is in progress' ' pristine_detach initial && - test_must_fail git cherry-pick --continue + test_expect_code 128 git cherry-pick --continue ' test_expect_success '--continue complains when there are unresolved conflicts' ' pristine_detach initial && - test_must_fail git cherry-pick base..anotherpick && - test_must_fail git cherry-pick --continue + test_expect_code 1 git cherry-pick base..anotherpick && + test_expect_code 128 git cherry-pick --continue ' test_expect_success '--continue of single cherry-pick' ' @@ -321,7 +321,7 @@ test_expect_success '--continue after resolving conflicts' ' test_expect_success '--continue after resolving conflicts and committing' ' pristine_detach initial && - test_must_fail git cherry-pick base..anotherpick && + test_expect_code 1 git cherry-pick base..anotherpick && echo "c" >foo && git add foo && git commit && @@ -371,7 +371,7 @@ test_expect_success 'follow advice and skip nil patch' ' test_expect_success '--continue respects opts' ' pristine_detach initial && - test_must_fail git cherry-pick -x base..anotherpick && + test_expect_code 1 git cherry-pick -x base..anotherpick && echo "c" >foo && git add foo && git commit && @@ -412,7 +412,7 @@ test_expect_success '--continue respects -x in first commit in multi-pick' ' test_expect_success '--signoff is not automatically propagated to resolved conflict' ' pristine_detach initial && - test_must_fail git cherry-pick --signoff base..anotherpick && + test_expect_code 1 git cherry-pick --signoff base..anotherpick && echo "c" >foo && git add foo && git commit && @@ -456,24 +456,24 @@ test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, test_expect_success 'malformed instruction sheet 1' ' pristine_detach initial && - test_must_fail git cherry-pick base..anotherpick && + test_expect_code 1 git cherry-pick base..anotherpick && echo "resolved" >foo && git add foo && git commit && sed "s/pick /pick/" .git/sequencer/todo >new_sheet && cp new_sheet .git/sequencer/todo && - test_must_fail git cherry-pick --continue + test_expect_code 128 git cherry-pick --continue ' test_expect_success 'malformed instruction sheet 2' ' pristine_detach initial && - test_must_fail git cherry-pick base..anotherpick && + test_expect_code 1 git cherry-pick base..anotherpick && echo "resolved" >foo && git add foo && git commit && sed "s/pick/revert/" .git/sequencer/todo >new_sheet && cp new_sheet .git/sequencer/todo && - test_must_fail git cherry-pick --continue + test_expect_code 128 git cherry-pick --continue ' test_expect_success 'empty commit set' ' @@ -483,18 +483,18 @@ test_expect_success 'empty commit set' ' test_expect_success 'malformed instruction sheet 3' ' pristine_detach initial && - test_must_fail git cherry-pick base..anotherpick && + test_expect_code 1 git cherry-pick base..anotherpick && echo "resolved" >foo && git add foo && git commit && sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet && cp new_sheet .git/sequencer/todo && - test_must_fail git cherry-pick --continue + test_expect_code 128 git cherry-pick --continue ' test_expect_success 'instruction sheet, fat-fingers version' ' pristine_detach initial && - test_must_fail git cherry-pick base..anotherpick && + test_expect_code 1 git cherry-pick base..anotherpick && echo "c" >foo && git add foo && git commit && @@ -505,7 +505,7 @@ test_expect_success 'instruction sheet, fat-fingers version' ' test_expect_success 'commit descriptions in insn sheet are optional' ' pristine_detach initial && - test_must_fail git cherry-pick base..anotherpick && + test_expect_code 1 git cherry-pick base..anotherpick && echo "c" >foo && git add foo && git commit && -- cgit v0.10.2-6-g49f6 From bf71009e5389aa2f3187ae9c0352ba3c70780f62 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Wed, 14 Dec 2011 22:24:33 +0530 Subject: t3502, t3510: clarify cherry-pick -m failure The "cherry-pick persists opts correctly" test in t3510 (cherry-pick-sequence) can cause some confusion, because the command actually has two points of failure: 1. "-m 1" is specified on the command-line despite the base commit "initial" not being a merge-commit. 2. The revision range indicates that there will be a conflict that needs to be resolved. Although the former error is trapped, and cherry-pick die()s with the exit status 128, the reader may be distracted by the latter. Fix this by changing the revision range to something that wouldn't cause a conflict. Additionally, explicitly check the exit code in "cherry-pick a non-merge with -m should fail" in t3502 (cherry-pick-merge) to reassure the reader that this failure has nothing to do with the sequencer itself. Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh index 0ab52da..e37547f 100755 --- a/t/t3502-cherry-pick-merge.sh +++ b/t/t3502-cherry-pick-merge.sh @@ -35,7 +35,7 @@ test_expect_success 'cherry-pick a non-merge with -m should fail' ' git reset --hard && git checkout a^0 && - test_must_fail git cherry-pick -m 1 b && + test_expect_code 128 git cherry-pick -m 1 b && git diff --exit-code a -- ' diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index f13a194..97f3710 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -67,7 +67,7 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' ' test_expect_success 'cherry-pick persists opts correctly' ' pristine_detach initial && - test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick && + test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick && test_path_is_dir .git/sequencer && test_path_is_file .git/sequencer/head && test_path_is_file .git/sequencer/todo && -- cgit v0.10.2-6-g49f6