* [PATCH v3 0/4] CHERRY_PICK_HEAD
@ 2011-02-17 4:18 Jay Soffian
2011-02-17 4:18 ` [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD Jay Soffian
` (3 more replies)
0 siblings, 4 replies; 17+ messages in thread
From: Jay Soffian @ 2011-02-17 4:18 UTC (permalink / raw)
To: git
Cc: Jay Soffian, Jonathan Nieder, Junio C Hamano,
Ævar Arnfjörð Bjarmason
Incorporating feedback from http://thread.gmane.org/gmane.comp.version-control.git/166945
Jay Soffian (4):
Introduce CHERRY_PICK_HEAD
bash: teach __git_ps1 about CHERRY_PICK_HEAD
commit.c: replace some literal strings with constants
Teach commit about CHERRY_PICK_HEAD
Documentation/git-cherry-pick.txt | 19 +++
Documentation/git-commit.txt | 7 +-
Documentation/revisions.txt | 5 +-
branch.c | 1 +
builtin/commit.c | 196 ++++++++++++++++++++++----------
builtin/merge.c | 7 +
builtin/revert.c | 74 ++++---------
contrib/completion/git-completion.bash | 2 +
t/t3507-cherry-pick-conflict.sh | 138 ++++++++++++++++++++++-
t/t7509-commit.sh | 29 +++++
wt-status.c | 4 +-
wt-status.h | 9 ++-
12 files changed, 371 insertions(+), 120 deletions(-)
--
1.7.4.1.30.g7fe09
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD 2011-02-17 4:18 [PATCH v3 0/4] CHERRY_PICK_HEAD Jay Soffian @ 2011-02-17 4:18 ` Jay Soffian 2011-02-17 20:01 ` Junio C Hamano 2011-02-17 21:56 ` Junio C Hamano 2011-02-17 4:18 ` [PATCH v3 2/4] bash: teach __git_ps1 about CHERRY_PICK_HEAD Jay Soffian ` (2 subsequent siblings) 3 siblings, 2 replies; 17+ messages in thread From: Jay Soffian @ 2011-02-17 4:18 UTC (permalink / raw) To: git Cc: Jay Soffian, Jonathan Nieder, Junio C Hamano, Ævar Arnfjörð Bjarmason When a cherry-pick conflicts git advises to use: $ git commit -c <original commit id> to preserve the original commit message and authorship. Instead, let's record the original commit id in CHERRY_PICK_HEAD and advise to use: $ git commit -c CHERRY_PICK_HEAD A later patch teaches git to handle the '-c CHERRY_PICK_HEAD' part. Note that we record CHERRY_PICK_HEAD even in the case where there are no conflicts so that we may use it to communicate authorship to commit; this will then allow us to remove set_author_ident_env from revert.c. Tests and documentation contributed by Jonathan Nieder. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Jay Soffian <jaysoffian@gmail.com> --- Incorporated feedback and additional tests from http://article.gmane.org/gmane.comp.version-control.git/167007 Documentation/git-cherry-pick.txt | 19 +++++ Documentation/revisions.txt | 5 +- branch.c | 1 + builtin/commit.c | 1 + builtin/merge.c | 7 ++ builtin/revert.c | 26 +++++++- t/t3507-cherry-pick-conflict.sh | 138 ++++++++++++++++++++++++++++++++++++- 7 files changed, 193 insertions(+), 4 deletions(-) diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index 749d68a..5d85daa 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -16,6 +16,25 @@ Given one or more existing commits, apply the change each one introduces, recording a new commit for each. This requires your working tree to be clean (no modifications from the HEAD commit). +When it is not obvious how to apply a change, the following +happens: + +1. The current branch and `HEAD` pointer stay at the last commit + successfully made. +2. The `CHERRY_PICK_HEAD` ref is set to point at the commit that + introduced the change that is difficult to apply. +3. Paths in which the change applied cleanly are updated both + in the index file and in your working tree. +4. For conflicting paths, the index file records up to three + versions, as described in the "TRUE MERGE" section of + linkgit:git-merge[1]. The working tree files will include + a description of the conflict bracketed by the usual + conflict markers `<<<<<<<` and `>>>>>>>`. +5. No other modifications are made. + +See linkgit:git-merge[1] for some hints on resolving such +conflicts. + OPTIONS ------- <commit>...:: diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index 9e92734..04fceee 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -25,7 +25,8 @@ blobs contained in a commit. first match in the following rules: . if `$GIT_DIR/<name>` exists, that is what you mean (this is usually - useful only for `HEAD`, `FETCH_HEAD`, `ORIG_HEAD` and `MERGE_HEAD`); + useful only for `HEAD`, `FETCH_HEAD`, `ORIG_HEAD`, `MERGE_HEAD` + and `CHERRY_PICK_HEAD`); . otherwise, `refs/<name>` if exists; @@ -46,6 +47,8 @@ you can change the tip of the branch back to the state before you ran them easily. MERGE_HEAD records the commit(s) you are merging into your branch when you run 'git merge'. +CHERRY_PICK_HEAD records the commit you are cherry-picking +when you run 'git cherry-pick'. + Note that any of the `refs/*` cases above may come either from the `$GIT_DIR/refs` directory or from the `$GIT_DIR/packed-refs` file. diff --git a/branch.c b/branch.c index 93dc866..dc23e95 100644 --- a/branch.c +++ b/branch.c @@ -217,6 +217,7 @@ void create_branch(const char *head, void remove_branch_state(void) { + unlink(git_path("CHERRY_PICK_HEAD")); unlink(git_path("MERGE_HEAD")); unlink(git_path("MERGE_RR")); unlink(git_path("MERGE_MSG")); diff --git a/builtin/commit.c b/builtin/commit.c index 03cff5a..0def540 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1424,6 +1424,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) die("cannot update HEAD ref"); } + unlink(git_path("CHERRY_PICK_HEAD")); unlink(git_path("MERGE_HEAD")); unlink(git_path("MERGE_MSG")); unlink(git_path("MERGE_MODE")); diff --git a/builtin/merge.c b/builtin/merge.c index 9403747..454dad2 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -971,6 +971,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix) else die("You have not concluded your merge (MERGE_HEAD exists)."); } + if (file_exists(git_path("CHERRY_PICK_HEAD"))) { + if (advice_resolve_conflict) + die("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\n" + "Please, commit your changes before you can merge."); + else + die("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists)."); + } resolve_undo_clear(); if (verbosity < 0) diff --git a/builtin/revert.c b/builtin/revert.c index dc1b702..fbb465a 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -248,6 +248,22 @@ static void set_author_ident_env(const char *message) sha1_to_hex(commit->object.sha1)); } +static void write_cherry_pick_head(void) +{ + int fd; + struct strbuf buf = STRBUF_INIT; + + strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1)); + + fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666); + if (fd < 0) + die_errno("Could not open '%s' for writing", + git_path("CHERRY_PICK_HEAD")); + if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd)) + die_errno("Could not write to '%s'", git_path("CHERRY_PICK_HEAD")); + strbuf_release(&buf); +} + static void advise(const char *advice, ...) { va_list params; @@ -263,6 +279,12 @@ static void print_advice(void) if (msg) { fprintf(stderr, "%s\n", msg); + /* + * A conflict has occured but the porcelain + * (typically rebase --interactive) wants to take care + * of the commit itself so remove CHERRY_PICK_HEAD + */ + unlink(git_path("CHERRY_PICK_HEAD")); return; } @@ -270,8 +292,7 @@ static void print_advice(void) advise("with 'git add <paths>' or 'git rm <paths>'"); if (action == CHERRY_PICK) - advise("and commit the result with 'git commit -c %s'", - find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV)); + advise("and commit the result with 'git commit -c CHERRY_PICK_HEAD'"); } static void write_message(struct strbuf *msgbuf, const char *filename) @@ -504,6 +525,7 @@ static int do_pick_commit(void) strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1)); strbuf_addstr(&msgbuf, ")\n"); } + write_cherry_pick_head(); } if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) { diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index 607bf25..ea52720 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -11,6 +11,12 @@ test_description='test cherry-pick and revert with conflicts . ./test-lib.sh +test_cmp_rev () { + git rev-parse --verify "$1" >expect.rev && + git rev-parse --verify "$2" >actual.rev && + test_cmp expect.rev actual.rev +} + test_expect_success setup ' echo unrelated >unrelated && @@ -51,13 +57,143 @@ test_expect_success 'advice from failed cherry-pick' " error: could not apply \$picked... picked hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' - hint: and commit the result with 'git commit -c \$picked' + hint: and commit the result with 'git commit -c CHERRY_PICK_HEAD' EOF test_must_fail git cherry-pick picked 2>actual && test_cmp expected actual " +test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' ' + + git checkout -f initial^0 && + git read-tree -u --reset HEAD && + git clean -d -f -f -q -x && + + git update-index --refresh && + git diff-index --exit-code HEAD && + + test_must_fail git cherry-pick picked && + + test_cmp_rev picked CHERRY_PICK_HEAD +' + +test_expect_success 'successful cherry-pick does not set CHERRY_PICK_HEAD' ' + + git checkout -f initial^0 && + git read-tree -u --reset HEAD && + git clean -d -f -f -q -x && + + git update-index --refresh && + git diff-index --exit-code HEAD && + + git cherry-pick base && + + test_must_fail git rev-parse --verify CHERRY_PICK_HEAD +' + +test_expect_success 'cherry-pick --no-commit sets CHERRY_PICK_HEAD' ' + + git checkout -f initial^0 && + git read-tree -u --reset HEAD && + git clean -d -f -f -q -x && + + git update-index --refresh && + git diff-index --exit-code HEAD && + + git cherry-pick --no-commit base && + + test_cmp_rev base CHERRY_PICK_HEAD +' + +test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' ' + + git checkout -f initial^0 && + git read-tree -u --reset HEAD && + git clean -d -f -f -q -x && + + git update-index --refresh && + git diff-index --exit-code HEAD && + + ( + GIT_CHERRY_PICK_HELP="and then do something else" && + export GIT_CHERRY_PICK_HELP && + test_must_fail git cherry-pick picked + ) && + + test_must_fail git rev-parse --verify CHERRY_PICK_HEAD +' + +test_expect_success 'git reset clears CHERRY_PICK_HEAD' ' + + git checkout -f initial^0 && + git read-tree -u --reset HEAD && + git clean -d -f -f -q -x && + + git update-index --refresh && + git diff-index --exit-code HEAD && + + test_must_fail git cherry-pick picked && + git reset && + + test_must_fail git rev-parse --verify CHERRY_PICK_HEAD +' + +test_expect_success 'failed commit does not clear CHERRY_PICK_HEAD' ' + + git checkout -f initial^0 && + git read-tree -u --reset HEAD && + git clean -d -f -f -q -x && + + git update-index --refresh && + git diff-index --exit-code HEAD && + + test_must_fail git cherry-pick picked && + test_must_fail git commit && + + test_cmp_rev picked CHERRY_PICK_HEAD +' + +test_expect_success 'cancelled commit does not clear CHERRY_PICK_HEAD' ' + + git checkout -f initial^0 && + git read-tree -u --reset HEAD && + git clean -d -f -f -q -x && + + git update-index --refresh && + git diff-index --exit-code HEAD && + + test_must_fail git cherry-pick picked && + echo resolved >foo && + git add foo && + git update-index --refresh -q && + test_must_fail git diff-index --exit-code HEAD && + ( + GIT_EDITOR=false && + export GIT_EDITOR && + test_must_fail git commit + ) && + + test_cmp_rev picked CHERRY_PICK_HEAD +' + +test_expect_success 'successful commit clears CHERRY_PICK_HEAD' ' + + git checkout -f initial^0 && + git read-tree -u --reset HEAD && + git clean -d -f -f -q -x && + + git update-index --refresh && + git diff-index --exit-code HEAD && + + test_must_fail git cherry-pick picked && + echo resolved >foo && + git add foo && + git commit && + + test_must_fail git rev-parse --verify CHERRY_PICK_HEAD +' + test_expect_success 'failed cherry-pick produces dirty index' ' git checkout -f initial^0 && -- 1.7.4.1.30.g7fe09 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD 2011-02-17 4:18 ` [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD Jay Soffian @ 2011-02-17 20:01 ` Junio C Hamano 2011-02-17 22:32 ` Jonathan Nieder 2011-02-20 2:29 ` Jay Soffian 2011-02-17 21:56 ` Junio C Hamano 1 sibling, 2 replies; 17+ messages in thread From: Junio C Hamano @ 2011-02-17 20:01 UTC (permalink / raw) To: Jay Soffian; +Cc: git, Jonathan Nieder, Ævar Arnfjörð Bjarmason Jay Soffian <jaysoffian@gmail.com> writes: > diff --git a/builtin/merge.c b/builtin/merge.c > index 9403747..454dad2 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -971,6 +971,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > else > die("You have not concluded your merge (MERGE_HEAD exists)."); > } > + if (file_exists(git_path("CHERRY_PICK_HEAD"))) { > + if (advice_resolve_conflict) > + die("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\n" > + "Please, commit your changes before you can merge."); > + else > + die("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists)."); > + } Micronit: "Please, commit your changes before you can merge." - We are not merging in this codepath to begin with; - I'd suggest rephrasing this (together with "MERGE_HEAD" codepath) to something like: "Commit your changes first before retrying." > +test_expect_success 'cherry-pick --no-commit sets CHERRY_PICK_HEAD' ' > + > + git checkout -f initial^0 && > + git read-tree -u --reset HEAD && > + git clean -d -f -f -q -x && > + > + git update-index --refresh && > + git diff-index --exit-code HEAD && Getting tired of seeing these five lines repeated over and over. Perhaps it is time to introduce: pristine_detach () { git checkout -f "$1^0" && git read-tree -u --reset HEAD && git clean -d -f -f -q -x } I don't think "diff-index --exit-code HEAD" is necessary as the point of this testsuite is not about testing "read-tree --reset", and the index refresh is just a prerequisite for diff-index that can go together with it. > + git cherry-pick --no-commit base && > + > + test_cmp_rev base CHERRY_PICK_HEAD If the next "git commit" would notice and use this information, that would introduce an unpleasant regression to one use case in my workflow, which is to pick and consolidate one or more small pieces made on a private "misc" branch, possibly with a bit of further work, into a new commit with a readable explanation that is unrelated to any of the original commits: git cherry-pick --no-commit $some_commit git cherry-pick --no-commit $another_commit ;# optional edit ;# optional git commit I'd prefer to see a way to tell cherry-pick not to leave CHERRY_PICK_HEAD behind when "cherry-pick --no-commit" results in a successful cherry-pick to avoid a backward incompatibility surprise. Otherwise people need to retrain their fingers to say --reset-author when they run "git commit". Other then the above three points, this patch looks good. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD 2011-02-17 20:01 ` Junio C Hamano @ 2011-02-17 22:32 ` Jonathan Nieder 2011-02-20 2:29 ` Jay Soffian 1 sibling, 0 replies; 17+ messages in thread From: Jonathan Nieder @ 2011-02-17 22:32 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jay Soffian, git, Ævar Arnfjörð Bjarmason Junio C Hamano wrote: > Jay Soffian <jaysoffian@gmail.com> writes: >> +test_expect_success 'cherry-pick --no-commit sets CHERRY_PICK_HEAD' ' >> + >> + git checkout -f initial^0 && >> + git read-tree -u --reset HEAD && >> + git clean -d -f -f -q -x && >> + >> + git update-index --refresh && >> + git diff-index --exit-code HEAD && > > Getting tired of seeing these five lines repeated over and over. Perhaps > it is time to introduce: > > pristine_detach () { The following patch (modulo naming) was sitting in my tree as a separate patch on top. > I don't think "diff-index --exit-code HEAD" is necessary as the point of > this testsuite is not about testing "read-tree --reset", and the index > refresh is just a prerequisite for diff-index that can go together with > it. Yep. Confusingly, the index refresh was just meant to check for a clean index. -- 8< -- Subject: [PATCH] tests: introduce pristine-detach helper All the tests in t3507 (cherry-pick with conflicts) begin with the same checkout + read-tree + clean incantation to ensure a predictable starting point. Factor out a function for that so the interesting part of the tests is easier to read. The "update-index --refresh" and "diff-index --exit-code HEAD" are not necessary as the point of this testsuite is not about testing "read-tree --reset". Improved-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Applies on top of the CHERRY_PICK_HEAD tests. t/t3507-cherry-pick-conflict.sh | 142 +++++++-------------------------------- 1 files changed, 24 insertions(+), 118 deletions(-) diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index ea52720..2d8437e 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -17,6 +17,12 @@ test_cmp_rev () { test_cmp expect.rev actual.rev } +pristine_detach () { + git checkout -f "$1^0" && + git read-tree -u --reset HEAD && + git clean -d -f -f -q -x +} + test_expect_success setup ' echo unrelated >unrelated && @@ -29,13 +35,7 @@ test_expect_success setup ' ' test_expect_success 'failed cherry-pick does not advance HEAD' ' - - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && - - git update-index --refresh && - git diff-index --exit-code HEAD && + pristine_detach initial && head=$(git rev-parse HEAD) && test_must_fail git cherry-pick picked && @@ -45,12 +45,7 @@ test_expect_success 'failed cherry-pick does not advance HEAD' ' ' test_expect_success 'advice from failed cherry-pick' " - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && - - git update-index --refresh && - git diff-index --exit-code HEAD && + pristine_detach initial && picked=\$(git rev-parse --short picked) && cat <<-EOF >expected && @@ -65,73 +60,35 @@ test_expect_success 'advice from failed cherry-pick' " " test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' ' - - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && - - git update-index --refresh && - git diff-index --exit-code HEAD && - + pristine_detach initial && test_must_fail git cherry-pick picked && - test_cmp_rev picked CHERRY_PICK_HEAD ' test_expect_success 'successful cherry-pick does not set CHERRY_PICK_HEAD' ' - - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && - - git update-index --refresh && - git diff-index --exit-code HEAD && - + pristine_detach initial && git cherry-pick base && - test_must_fail git rev-parse --verify CHERRY_PICK_HEAD ' test_expect_success 'cherry-pick --no-commit sets CHERRY_PICK_HEAD' ' - - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && - - git update-index --refresh && - git diff-index --exit-code HEAD && - + pristine_detach initial && git cherry-pick --no-commit base && - test_cmp_rev base CHERRY_PICK_HEAD ' test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' ' - - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && - - git update-index --refresh && - git diff-index --exit-code HEAD && - + pristine_detach initial && ( GIT_CHERRY_PICK_HELP="and then do something else" && export GIT_CHERRY_PICK_HELP && test_must_fail git cherry-pick picked ) && - test_must_fail git rev-parse --verify CHERRY_PICK_HEAD ' test_expect_success 'git reset clears CHERRY_PICK_HEAD' ' - - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && - - git update-index --refresh && - git diff-index --exit-code HEAD && + pristine_detach initial && test_must_fail git cherry-pick picked && git reset && @@ -140,13 +97,7 @@ test_expect_success 'git reset clears CHERRY_PICK_HEAD' ' ' test_expect_success 'failed commit does not clear CHERRY_PICK_HEAD' ' - - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && - - git update-index --refresh && - git diff-index --exit-code HEAD && + pristine_detach initial && test_must_fail git cherry-pick picked && test_must_fail git commit && @@ -155,13 +106,7 @@ test_expect_success 'failed commit does not clear CHERRY_PICK_HEAD' ' ' test_expect_success 'cancelled commit does not clear CHERRY_PICK_HEAD' ' - - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && - - git update-index --refresh && - git diff-index --exit-code HEAD && + pristine_detach initial && test_must_fail git cherry-pick picked && echo resolved >foo && @@ -178,13 +123,7 @@ test_expect_success 'cancelled commit does not clear CHERRY_PICK_HEAD' ' ' test_expect_success 'successful commit clears CHERRY_PICK_HEAD' ' - - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && - - git update-index --refresh && - git diff-index --exit-code HEAD && + pristine_detach initial && test_must_fail git cherry-pick picked && echo resolved >foo && @@ -195,13 +134,7 @@ test_expect_success 'successful commit clears CHERRY_PICK_HEAD' ' ' test_expect_success 'failed cherry-pick produces dirty index' ' - - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && - - git update-index --refresh && - git diff-index --exit-code HEAD && + pristine_detach initial && test_must_fail git cherry-pick picked && @@ -210,9 +143,7 @@ test_expect_success 'failed cherry-pick produces dirty index' ' ' test_expect_success 'failed cherry-pick registers participants in index' ' - - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && + pristine_detach initial && { git checkout base -- foo && git ls-files --stage foo && @@ -226,10 +157,7 @@ test_expect_success 'failed cherry-pick registers participants in index' ' 2 s/ 0 / 2 / 3 s/ 0 / 3 / " < stages > expected && - git checkout -f initial^0 && - - git update-index --refresh && - git diff-index --exit-code HEAD && + git read-tree -u --reset HEAD && test_must_fail git cherry-pick picked && git ls-files --stage --unmerged > actual && @@ -238,10 +166,7 @@ test_expect_success 'failed cherry-pick registers participants in index' ' ' test_expect_success 'failed cherry-pick describes conflict in work tree' ' - - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && + pristine_detach initial && cat <<-EOF > expected && <<<<<<< HEAD a @@ -250,9 +175,6 @@ test_expect_success 'failed cherry-pick describes conflict in work tree' ' >>>>>>> objid picked EOF - git update-index --refresh && - git diff-index --exit-code HEAD && - test_must_fail git cherry-pick picked && sed "s/[a-f0-9]*\.\.\./objid/" foo > actual && @@ -260,11 +182,8 @@ test_expect_success 'failed cherry-pick describes conflict in work tree' ' ' test_expect_success 'diff3 -m style' ' - + pristine_detach initial && git config merge.conflictstyle diff3 && - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && cat <<-EOF > expected && <<<<<<< HEAD a @@ -275,9 +194,6 @@ test_expect_success 'diff3 -m style' ' >>>>>>> objid picked EOF - git update-index --refresh && - git diff-index --exit-code HEAD && - test_must_fail git cherry-pick picked && sed "s/[a-f0-9]*\.\.\./objid/" foo > actual && @@ -285,10 +201,8 @@ test_expect_success 'diff3 -m style' ' ' test_expect_success 'revert also handles conflicts sanely' ' - git config --unset merge.conflictstyle && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && + pristine_detach initial && cat <<-EOF > expected && <<<<<<< HEAD a @@ -309,10 +223,7 @@ test_expect_success 'revert also handles conflicts sanely' ' 2 s/ 0 / 2 / 3 s/ 0 / 3 / " < stages > expected-stages && - git checkout -f initial^0 && - - git update-index --refresh && - git diff-index --exit-code HEAD && + git read-tree -u --reset HEAD && head=$(git rev-parse HEAD) && test_must_fail git revert picked && @@ -328,10 +239,8 @@ test_expect_success 'revert also handles conflicts sanely' ' ' test_expect_success 'revert conflict, diff3 -m style' ' + pristine_detach initial && git config merge.conflictstyle diff3 && - git checkout -f initial^0 && - git read-tree -u --reset HEAD && - git clean -d -f -f -q -x && cat <<-EOF > expected && <<<<<<< HEAD a @@ -342,9 +251,6 @@ test_expect_success 'revert conflict, diff3 -m style' ' >>>>>>> parent of objid picked EOF - git update-index --refresh && - git diff-index --exit-code HEAD && - test_must_fail git revert picked && sed "s/[a-f0-9]*\.\.\./objid/" foo > actual && -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD 2011-02-17 20:01 ` Junio C Hamano 2011-02-17 22:32 ` Jonathan Nieder @ 2011-02-20 2:29 ` Jay Soffian 2011-02-20 2:48 ` Jonathan Nieder 2011-02-20 6:43 ` Junio C Hamano 1 sibling, 2 replies; 17+ messages in thread From: Jay Soffian @ 2011-02-20 2:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder, Ævar Arnfjörð On Thu, Feb 17, 2011 at 3:01 PM, Junio C Hamano <gitster@pobox.com> wrote: > Jay Soffian <jaysoffian@gmail.com> writes: > >> diff --git a/builtin/merge.c b/builtin/merge.c >> index 9403747..454dad2 100644 >> --- a/builtin/merge.c >> +++ b/builtin/merge.c >> @@ -971,6 +971,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix) >> else >> die("You have not concluded your merge (MERGE_HEAD exists)."); >> } >> + if (file_exists(git_path("CHERRY_PICK_HEAD"))) { >> + if (advice_resolve_conflict) >> + die("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\n" >> + "Please, commit your changes before you can merge."); >> + else >> + die("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists)."); >> + } > > Micronit: "Please, commit your changes before you can merge." > > - We are not merging in this codepath to begin with; > > - I'd suggest rephrasing this (together with "MERGE_HEAD" codepath) to > something like: > > "Commit your changes first before retrying." This hunk is from Jonathan, so I'd like him to address your comment as well, but I think the current message is correct. This hunk is inside cmd_merge and the user is about to start a merge. IOW: $ git cherry-pick # fails, leave behinds CHERRY_PICK_HEAD [... time passes ...] $ git merge You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists). Please, commit your changes before you can merge. > Getting tired of seeing these five lines repeated over and over. Perhaps > it is time to introduce: Okay. (Will roll in from Jonathan's later email.) >> + git cherry-pick --no-commit base && >> + >> + test_cmp_rev base CHERRY_PICK_HEAD > > If the next "git commit" would notice and use this information, that would > introduce an unpleasant regression to one use case in my workflow, which > is to pick and consolidate one or more small pieces made on a private > "misc" branch, possibly with a bit of further work, into a new commit with > a readable explanation that is unrelated to any of the original commits: > > git cherry-pick --no-commit $some_commit > git cherry-pick --no-commit $another_commit ;# optional > edit ;# optional > git commit > > I'd prefer to see a way to tell cherry-pick not to leave CHERRY_PICK_HEAD > behind when "cherry-pick --no-commit" results in a successful cherry-pick > to avoid a backward incompatibility surprise. Otherwise people need to > retrain their fingers to say --reset-author when they run "git commit". Okay. In that case, I think when using --no-commit, we shouldn't write CHERRY_PICK_HEAD regardless of whether there is a conflict or not. j. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD 2011-02-20 2:29 ` Jay Soffian @ 2011-02-20 2:48 ` Jonathan Nieder 2011-02-20 6:43 ` Junio C Hamano 1 sibling, 0 replies; 17+ messages in thread From: Jonathan Nieder @ 2011-02-20 2:48 UTC (permalink / raw) To: Jay Soffian; +Cc: Junio C Hamano, git, Ævar Arnfjörð Hi, Jay Soffian wrote: > On Thu, Feb 17, 2011 at 3:01 PM, Junio C Hamano <gitster@pobox.com> wrote: >> Micronit: "Please, commit your changes before you can merge." >> >> - We are not merging in this codepath to begin with; >> >> - I'd suggest rephrasing this (together with "MERGE_HEAD" codepath) to >> something like: >> >> "Commit your changes first before retrying." > > This hunk is from Jonathan I agree with what Jay said --- this is in cmd_merge, so the current message is correct. I like your wording ("before retrying") more, but that seems orthogonal to the topic at hand. I can send a patch rephrasing it on top if someone reminds me. > Okay. In that case, I think when using --no-commit, we shouldn't write > CHERRY_PICK_HEAD regardless of whether there is a conflict or not. Thanks for your attention to detail. Jonathan ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD 2011-02-20 2:29 ` Jay Soffian 2011-02-20 2:48 ` Jonathan Nieder @ 2011-02-20 6:43 ` Junio C Hamano 1 sibling, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2011-02-20 6:43 UTC (permalink / raw) To: Jay Soffian Cc: Junio C Hamano, git, Jonathan Nieder, Ævar Arnfjörð Jay Soffian <jaysoffian@gmail.com> writes: > ..., but I think the current message is correct. This hunk is inside > cmd_merge and the user is about to start a merge. Yes, you are right; I misread the patch. Sorry about that. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD 2011-02-17 4:18 ` [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD Jay Soffian 2011-02-17 20:01 ` Junio C Hamano @ 2011-02-17 21:56 ` Junio C Hamano 2011-02-17 22:42 ` Jay Soffian 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2011-02-17 21:56 UTC (permalink / raw) To: Jay Soffian; +Cc: git, Jonathan Nieder, Ævar Arnfjörð Bjarmason This series in 'pu' seems to break t3404#16. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD 2011-02-17 21:56 ` Junio C Hamano @ 2011-02-17 22:42 ` Jay Soffian 2011-02-17 22:48 ` Jonathan Nieder 0 siblings, 1 reply; 17+ messages in thread From: Jay Soffian @ 2011-02-17 22:42 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jonathan Nieder, Ævar Arnfjörð On Thu, Feb 17, 2011 at 4:56 PM, Junio C Hamano <gitster@pobox.com> wrote: > This series in 'pu' seems to break t3404#16. Sorry for the breakage, I was running the test suite piecemeal and must've not run that test (which was dumb of me). I'll figure out what's going on and re-roll with Jonathan's t3507 improvements as well. Thank you for your patience. j. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD 2011-02-17 22:42 ` Jay Soffian @ 2011-02-17 22:48 ` Jonathan Nieder 0 siblings, 0 replies; 17+ messages in thread From: Jonathan Nieder @ 2011-02-17 22:48 UTC (permalink / raw) To: Jay Soffian; +Cc: Junio C Hamano, git, Ævar Arnfjörð Jay Soffian wrote: > On Thu, Feb 17, 2011 at 4:56 PM, Junio C Hamano <gitster@pobox.com> wrote: >> This series in 'pu' seems to break t3404#16. > > Sorry for the breakage, I was running the test suite piecemeal and > must've not run that test (which was dumb of me). I made the same mistake, too. Sorry for the trouble. > I'll figure out > what's going on and re-roll with Jonathan's t3507 improvements as > well. FWIW (probably you noticed, but just in case) I imagine the t3507 improvements should go as a separate patch, since their main effect is to clean up existing tests. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 2/4] bash: teach __git_ps1 about CHERRY_PICK_HEAD 2011-02-17 4:18 [PATCH v3 0/4] CHERRY_PICK_HEAD Jay Soffian 2011-02-17 4:18 ` [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD Jay Soffian @ 2011-02-17 4:18 ` Jay Soffian 2011-02-17 4:18 ` [PATCH v3 3/4] commit.c: replace some literal strings with constants Jay Soffian 2011-02-17 4:18 ` [PATCH v3 4/4] Teach commit about CHERRY_PICK_HEAD Jay Soffian 3 siblings, 0 replies; 17+ messages in thread From: Jay Soffian @ 2011-02-17 4:18 UTC (permalink / raw) To: git Cc: Jay Soffian, Jonathan Nieder, Junio C Hamano, Ævar Arnfjörð Bjarmason Make the git prompt (when enabled) show a CHERRY-PICKING indicator when we are in the middle of a conflicted cherry-pick, analogous to the existing MERGING and BISECTING flags. Signed-off-by: Jay Soffian <jaysoffian@gmail.com> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Split this out into its own patch per http://article.gmane.org/gmane.comp.version-control.git/167009 contrib/completion/git-completion.bash | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 893b771..0b0b913 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -246,6 +246,8 @@ __git_ps1 () fi elif [ -f "$g/MERGE_HEAD" ]; then r="|MERGING" + elif [ -f "$g/CHERRY_PICK_HEAD" ]; then + r="|CHERRY-PICKING" elif [ -f "$g/BISECT_LOG" ]; then r="|BISECTING" fi -- 1.7.4.1.30.g7fe09 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v3 3/4] commit.c: replace some literal strings with constants 2011-02-17 4:18 [PATCH v3 0/4] CHERRY_PICK_HEAD Jay Soffian 2011-02-17 4:18 ` [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD Jay Soffian 2011-02-17 4:18 ` [PATCH v3 2/4] bash: teach __git_ps1 about CHERRY_PICK_HEAD Jay Soffian @ 2011-02-17 4:18 ` Jay Soffian 2011-02-17 5:19 ` Jonathan Nieder 2011-02-17 5:49 ` [PATCH v3 3/4] commit.c: replace some literal strings with constants Junio C Hamano 2011-02-17 4:18 ` [PATCH v3 4/4] Teach commit about CHERRY_PICK_HEAD Jay Soffian 3 siblings, 2 replies; 17+ messages in thread From: Jay Soffian @ 2011-02-17 4:18 UTC (permalink / raw) To: git Cc: Jay Soffian, Jonathan Nieder, Junio C Hamano, Ævar Arnfjörð Bjarmason A typo in any of these would be bad, so let's use constants for them Signed-off-by: Jay Soffian <jaysoffian@gmail.com> --- I converted these per http://article.gmane.org/gmane.comp.version-control.git/167015 Maybe this should be the last patch in the series; it's questionable to me whether it's even worth doing. builtin/commit.c | 48 +++++++++++++++++++++++++++--------------------- 1 files changed, 27 insertions(+), 21 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 0def540..5b32743 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -54,10 +54,16 @@ static const char empty_amend_advice[] = "it empty. You can repeat your command with --allow-empty, or you can\n" "remove the commit entirely with \"git reset HEAD^\".\n"; +static const char commit_editmsg[] = "COMMIT_EDITMSG"; +static const char cherry_pick_head[] = "CHERRY_PICK_HEAD"; +static const char merge_head[] = "MERGE_HEAD"; +static const char merge_msg[] = "MERGE_MSG"; +static const char merge_mode[] = "MERGE_MODE"; +static const char squash_msg[] = "SQUASH_MSG"; + static unsigned char head_sha1[20]; static char *use_message_buffer; -static const char commit_editmsg[] = "COMMIT_EDITMSG"; static struct lock_file index_lock; /* real index */ static struct lock_file false_lock; /* used only for partial commits */ static enum { @@ -626,13 +632,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix, format_commit_message(commit, "fixup! %s\n\n", &sb, &ctx); hook_arg1 = "message"; - } else if (!stat(git_path("MERGE_MSG"), &statbuf)) { - if (strbuf_read_file(&sb, git_path("MERGE_MSG"), 0) < 0) - die_errno("could not read MERGE_MSG"); + } else if (!stat(git_path(merge_msg), &statbuf)) { + if (strbuf_read_file(&sb, git_path(merge_msg), 0) < 0) + die_errno("could not read %s", merge_msg); hook_arg1 = "merge"; - } else if (!stat(git_path("SQUASH_MSG"), &statbuf)) { - if (strbuf_read_file(&sb, git_path("SQUASH_MSG"), 0) < 0) - die_errno("could not read SQUASH_MSG"); + } else if (!stat(git_path(squash_msg), &statbuf)) { + if (strbuf_read_file(&sb, git_path(squash_msg), 0) < 0) + die_errno("could not read %s", squash_msg); hook_arg1 = "squash"; } else if (template_file && !stat(template_file, &statbuf)) { if (strbuf_read_file(&sb, template_file, 0) < 0) @@ -702,7 +708,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, "# %s\n" "# and try again.\n" "#\n", - git_path("MERGE_HEAD")); + git_path(merge_head)); fprintf(fp, "\n" @@ -1117,7 +1123,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) wt_status_prepare(&s); gitmodules_config(); git_config(git_status_config, &s); - in_merge = file_exists(git_path("MERGE_HEAD")); + in_merge = file_exists(git_path(merge_head)); argc = parse_options(argc, argv, prefix, builtin_status_options, builtin_status_usage, 0); @@ -1302,7 +1308,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) wt_status_prepare(&s); git_config(git_commit_config, &s); - in_merge = file_exists(git_path("MERGE_HEAD")); + in_merge = file_exists(git_path(merge_head)); s.in_merge = in_merge; if (s.use_color == -1) @@ -1347,21 +1353,21 @@ int cmd_commit(int argc, const char **argv, const char *prefix) if (!reflog_msg) reflog_msg = "commit (merge)"; pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next; - fp = fopen(git_path("MERGE_HEAD"), "r"); + fp = fopen(git_path(merge_head), "r"); if (fp == NULL) die_errno("could not open '%s' for reading", - git_path("MERGE_HEAD")); + git_path(merge_head)); while (strbuf_getline(&m, fp, '\n') != EOF) { unsigned char sha1[20]; if (get_sha1_hex(m.buf, sha1) < 0) - die("Corrupt MERGE_HEAD file (%s)", m.buf); + die("Corrupt %s file (%s)", merge_head, m.buf); pptr = &commit_list_insert(lookup_commit(sha1), pptr)->next; } fclose(fp); strbuf_release(&m); - if (!stat(git_path("MERGE_MODE"), &statbuf)) { - if (strbuf_read_file(&sb, git_path("MERGE_MODE"), 0) < 0) - die_errno("could not read MERGE_MODE"); + if (!stat(git_path(merge_mode), &statbuf)) { + if (strbuf_read_file(&sb, git_path(merge_mode), 0) < 0) + die_errno("could not read %s", merge_mode); if (!strcmp(sb.buf, "no-ff")) allow_fast_forward = 0; } @@ -1424,11 +1430,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix) die("cannot update HEAD ref"); } - unlink(git_path("CHERRY_PICK_HEAD")); - unlink(git_path("MERGE_HEAD")); - unlink(git_path("MERGE_MSG")); - unlink(git_path("MERGE_MODE")); - unlink(git_path("SQUASH_MSG")); + unlink(git_path(cherry_pick_head)); + unlink(git_path(merge_head)); + unlink(git_path(merge_msg)); + unlink(git_path(merge_mode)); + unlink(git_path(squash_msg)); if (commit_index_files()) die ("Repository has been updated, but unable to write\n" -- 1.7.4.1.30.g7fe09 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/4] commit.c: replace some literal strings with constants 2011-02-17 4:18 ` [PATCH v3 3/4] commit.c: replace some literal strings with constants Jay Soffian @ 2011-02-17 5:19 ` Jonathan Nieder 2011-02-17 5:50 ` [PATCH] Teach commit about CHERRY_PICK_HEAD Jay Soffian 2011-02-17 5:49 ` [PATCH v3 3/4] commit.c: replace some literal strings with constants Junio C Hamano 1 sibling, 1 reply; 17+ messages in thread From: Jonathan Nieder @ 2011-02-17 5:19 UTC (permalink / raw) To: Jay Soffian; +Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason Jay Soffian wrote: > I converted these per > http://article.gmane.org/gmane.comp.version-control.git/167015 > > Maybe this should be the last patch in the series; it's questionable to > me whether it's even worth doing. What have I wrought? I think this makes the code much less readable. > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -626,13 +632,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > format_commit_message(commit, "fixup! %s\n\n", > &sb, &ctx); > hook_arg1 = "message"; > - } else if (!stat(git_path("MERGE_MSG"), &statbuf)) { > - if (strbuf_read_file(&sb, git_path("MERGE_MSG"), 0) < 0) > - die_errno("could not read MERGE_MSG"); > + } else if (!stat(git_path(merge_msg), &statbuf)) { > + if (strbuf_read_file(&sb, git_path(merge_msg), 0) < 0) > + die_errno("could not read %s", merge_msg); So now a person needs to look earlier in the file to see that merge_msg means "MERGE_MSG" rather than being something that can vary. Yes, commit_editmsg has the same problem. If I get to choose[*] then cache.h would contain something like #define MERGE_HEAD_FILE "MERGE_HEAD" #define PREPARED_COMMIT_MESSAGE_FILE "MERGE_MSG" #define <description of SQUASH_MSG goes here> "SQUASH_MSG" and commit.c or cache.h would contain something like #define EDITABLE_COMMIT_MESSAGE_FILE "COMMIT_EDITMSG" and those symbolic names would be used throughout builtin code to name those files. So we would get (1) documentation of what filenames are special to git, collected in one place (2) protection against typos without losing (3) names that look like constants Jonathan [*] and I shouldn't! What I like should matter much less than what is right. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] Teach commit about CHERRY_PICK_HEAD 2011-02-17 5:19 ` Jonathan Nieder @ 2011-02-17 5:50 ` Jay Soffian 2011-02-18 0:29 ` Jonathan Nieder 0 siblings, 1 reply; 17+ messages in thread From: Jay Soffian @ 2011-02-17 5:50 UTC (permalink / raw) To: git Cc: Jay Soffian, Ævar Arnfjörð Bjarmason, Jonathan Nieder, Junio C Hamano Previously the user was advised to use commit -c CHERRY_PICK_HEAD after a conflicting cherry-pick. While this would preserve the original commit's authorship, it would sadly discard cherry-pick's carefully crafted MERGE_MSG (which contains the list of conflicts as well as the original commit-id in the case of cherry-pick -x). On the other hand, if a bare 'commit' were performed, it would preserve the MERGE_MSG while resetting the authorship. In other words, there was no way to simultaneously take the authorship from CHERRY_PICK_HEAD and the commit message from MERGE_MSG. This change fixes that situation. A bare 'commit' will now take the authorship from CHERRY_PICK_HEAD and the commit message from MERGE_MSG. If the user wishes to reset authorship, that must now be done explicitly via --reset-author. A side-benefit of passing commit authorship along this way is that we can eliminate redundant authorship parsing code from revert.c. (Also removed an unused include from revert.c) Signed-off-by: Jay Soffian <jaysoffian@gmail.com> --- On Thu, Feb 17, 2011 at 12:19 AM, Jonathan Nieder <jrnieder@gmail.com> wrote: > What have I wrought? I think this makes the code much less readable. I agree. Junio - this is a respin of "[PATCH v3 4/4] Teach commit about CHERRY_PICK_HEAD" with "[PATCH v3 3/4] commit.c: replace some literal strings with constants" dropped. Documentation/git-commit.txt | 7 +- builtin/commit.c | 155 ++++++++++++++++++++++++++++----------- builtin/revert.c | 56 +-------------- t/t3507-cherry-pick-conflict.sh | 2 +- t/t7509-commit.sh | 29 +++++++ wt-status.c | 4 +- wt-status.h | 9 ++- 7 files changed, 158 insertions(+), 104 deletions(-) diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index b586c0f..fd6a1f7 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -84,9 +84,10 @@ OPTIONS linkgit:git-rebase[1] for details. --reset-author:: - When used with -C/-c/--amend options, declare that the - authorship of the resulting commit now belongs of the committer. - This also renews the author timestamp. + When used with -C/-c/--amend options, or when committing after a + a conflicting cherry-pick, declare that the authorship of the + resulting commit now belongs of the committer. This also renews + the author timestamp. --short:: When doing a dry-run, give the output in the short-format. See diff --git a/builtin/commit.c b/builtin/commit.c index 0def540..0c7b439 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -54,9 +54,16 @@ static const char empty_amend_advice[] = "it empty. You can repeat your command with --allow-empty, or you can\n" "remove the commit entirely with \"git reset HEAD^\".\n"; +static const char empty_cherry_pick_advice[] = +"The most recent cherry-pick is empty. If you wish to commit it, use:\n" +"\n" +" git commit --allow-empty\n" +"\n" +"Otherwise, please remove the file %s\n"; + static unsigned char head_sha1[20]; -static char *use_message_buffer; +static const char *use_message_buffer; static const char commit_editmsg[] = "COMMIT_EDITMSG"; static struct lock_file index_lock; /* real index */ static struct lock_file false_lock; /* used only for partial commits */ @@ -68,6 +75,11 @@ static enum { static const char *logfile, *force_author; static const char *template_file; +/* + * The _message variables are commit names from which to take + * the commit message and/or authorship. + */ +static const char *author_message, *author_message_buffer; static char *edit_message, *use_message; static char *fixup_message, *squash_message; static int all, edit_flag, also, interactive, only, amend, signoff; @@ -88,7 +100,8 @@ static enum { } cleanup_mode; static char *cleanup_arg; -static int use_editor = 1, initial_commit, in_merge, include_status = 1; +static enum commit_whence whence; +static int use_editor = 1, initial_commit, include_status = 1; static int show_ignored_in_status; static const char *only_include_assumed; static struct strbuf message; @@ -163,6 +176,36 @@ static struct option builtin_commit_options[] = { OPT_END() }; +static void determine_whence(struct wt_status *s) +{ + if (file_exists(git_path("MERGE_HEAD"))) + whence = FROM_MERGE; + else if (file_exists(git_path("CHERRY_PICK_HEAD"))) + whence = FROM_CHERRY_PICK; + else + whence = FROM_COMMIT; + if (s) + s->whence = whence; +} + +static const char *whence_s(void) +{ + char *s = ""; + + switch (whence) { + case FROM_COMMIT: + break; + case FROM_MERGE: + s = "merge"; + break; + case FROM_CHERRY_PICK: + s = "cherry-pick"; + break; + } + + return s; +} + static void rollback_index_files(void) { switch (commit_style) { @@ -378,8 +421,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int */ commit_style = COMMIT_PARTIAL; - if (in_merge) - die("cannot do a partial commit during a merge."); + if (whence != FROM_COMMIT) + die("cannot do a partial commit during a %s.", whence_s()); memset(&partial, 0, sizeof(partial)); partial.strdup_strings = 1; @@ -469,18 +512,18 @@ static void determine_author_info(struct strbuf *author_ident) email = getenv("GIT_AUTHOR_EMAIL"); date = getenv("GIT_AUTHOR_DATE"); - if (use_message && !renew_authorship) { + if (author_message) { const char *a, *lb, *rb, *eol; - a = strstr(use_message_buffer, "\nauthor "); + a = strstr(author_message_buffer, "\nauthor "); if (!a) - die("invalid commit: %s", use_message); + die("invalid commit: %s", author_message); lb = strchrnul(a + strlen("\nauthor "), '<'); rb = strchrnul(lb, '>'); eol = strchrnul(rb, '\n'); if (!*lb || !*rb || !*eol) - die("invalid commit: %s", use_message); + die("invalid commit: %s", author_message); if (lb == a + strlen("\nauthor ")) /* \nauthor <foo@example.com> */ @@ -641,11 +684,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix, } /* - * This final case does not modify the template message, - * it just sets the argument to the prepare-commit-msg hook. + * The remaining cases don't modify the template message, but + * just set the argument(s) to the prepare-commit-msg hook. */ - else if (in_merge) + else if (whence == FROM_MERGE) hook_arg1 = "merge"; + else if (whence == FROM_CHERRY_PICK) { + hook_arg1 = "commit"; + hook_arg2 = "CHERRY_PICK_HEAD"; + } if (squash_message) { /* @@ -694,16 +741,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix, strbuf_addstr(&committer_ident, git_committer_info(0)); if (use_editor && include_status) { char *ai_tmp, *ci_tmp; - if (in_merge) + if (whence != FROM_COMMIT) fprintf(fp, "#\n" - "# It looks like you may be committing a MERGE.\n" + "# It looks like you may be committing a %s.\n" "# If this is not correct, please remove the file\n" "# %s\n" "# and try again.\n" "#\n", - git_path("MERGE_HEAD")); - + whence_s(), + git_path(whence == FROM_MERGE + ? "MERGE_HEAD" + : "CHERRY_PICK_HEAD")); fprintf(fp, "\n" "# Please enter the commit message for your changes."); @@ -766,11 +815,19 @@ static int prepare_to_commit(const char *index_file, const char *prefix, fclose(fp); - if (!commitable && !in_merge && !allow_empty && + /* + * Reject an attempt to record a non-merge empty commit without + * explicit --allow-empty. In the cherry-pick case, it may be + * empty due to conflict resolution, which the user should okay. + */ + if (!commitable && whence != FROM_MERGE && !allow_empty && !(amend && is_a_merge(head_sha1))) { run_status(stdout, index_file, prefix, 0, s); if (amend) fputs(empty_amend_advice, stderr); + else if (whence == FROM_CHERRY_PICK) + fprintf(stderr, empty_cherry_pick_advice, + git_path("CHERRY_PICK_HEAD")); return 0; } @@ -898,6 +955,28 @@ static void handle_untracked_files_arg(struct wt_status *s) die("Invalid untracked files mode '%s'", untracked_files_arg); } +static const char *read_commit_message(const char *name) +{ + const char *out_enc, *out; + struct commit *commit; + + commit = lookup_commit_reference_by_name(name); + if (!commit) + die("could not lookup commit %s", name); + out_enc = get_commit_output_encoding(); + out = logmsg_reencode(commit, out_enc); + + /* + * If we failed to reencode the buffer, just copy it + * byte for byte so the user can try to fix it up. + * This also handles the case where input and output + * encodings are identical. + */ + if (out == NULL) + out = xstrdup(commit->buffer); + return out; +} + static int parse_and_validate_options(int argc, const char *argv[], const char * const usage[], const char *prefix, @@ -927,8 +1006,8 @@ static int parse_and_validate_options(int argc, const char *argv[], /* Sanity check options */ if (amend && initial_commit) die("You have nothing to amend."); - if (amend && in_merge) - die("You are in the middle of a merge -- cannot amend."); + if (amend && whence != FROM_COMMIT) + die("You are in the middle of a %s -- cannot amend.", whence_s()); if (fixup_message && squash_message) die("Options --squash and --fixup cannot be used together"); if (use_message) @@ -947,26 +1026,18 @@ static int parse_and_validate_options(int argc, const char *argv[], use_message = edit_message; if (amend && !use_message && !fixup_message) use_message = "HEAD"; - if (!use_message && renew_authorship) + if (!use_message && whence != FROM_CHERRY_PICK && renew_authorship) die("--reset-author can be used only with -C, -c or --amend."); if (use_message) { - const char *out_enc; - struct commit *commit; - - commit = lookup_commit_reference_by_name(use_message); - if (!commit) - die("could not lookup commit %s", use_message); - out_enc = get_commit_output_encoding(); - use_message_buffer = logmsg_reencode(commit, out_enc); - - /* - * If we failed to reencode the buffer, just copy it - * byte for byte so the user can try to fix it up. - * This also handles the case where input and output - * encodings are identical. - */ - if (use_message_buffer == NULL) - use_message_buffer = xstrdup(commit->buffer); + use_message_buffer = read_commit_message(use_message); + if (!renew_authorship) { + author_message = use_message; + author_message_buffer = use_message_buffer; + } + } + if (whence == FROM_CHERRY_PICK && !renew_authorship) { + author_message = "CHERRY_PICK_HEAD"; + author_message_buffer = read_commit_message(author_message); } if (!!also + !!only + !!all + !!interactive > 1) @@ -1117,7 +1188,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) wt_status_prepare(&s); gitmodules_config(); git_config(git_status_config, &s); - in_merge = file_exists(git_path("MERGE_HEAD")); + determine_whence(&s); argc = parse_options(argc, argv, prefix, builtin_status_options, builtin_status_usage, 0); @@ -1140,7 +1211,6 @@ int cmd_status(int argc, const char **argv, const char *prefix) } s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0; - s.in_merge = in_merge; s.ignore_submodule_arg = ignore_submodule_arg; wt_status_collect(&s); @@ -1302,8 +1372,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) wt_status_prepare(&s); git_config(git_commit_config, &s); - in_merge = file_exists(git_path("MERGE_HEAD")); - s.in_merge = in_merge; + determine_whence(&s); if (s.use_color == -1) s.use_color = git_use_color_default; @@ -1340,7 +1409,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) for (c = commit->parents; c; c = c->next) pptr = &commit_list_insert(c->item, pptr)->next; - } else if (in_merge) { + } else if (whence == FROM_MERGE) { struct strbuf m = STRBUF_INIT; FILE *fp; @@ -1369,7 +1438,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix) parents = reduce_heads(parents); } else { if (!reflog_msg) - reflog_msg = "commit"; + reflog_msg = (whence == FROM_CHERRY_PICK) + ? "commit (cherry-pick)" + : "commit"; pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next; } diff --git a/builtin/revert.c b/builtin/revert.c index fbb465a..2ecc6e1 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -3,7 +3,6 @@ #include "object.h" #include "commit.h" #include "tag.h" -#include "wt-status.h" #include "run-command.h" #include "exec_cmd.h" #include "utf8.h" @@ -198,56 +197,6 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message) strbuf_addstr(msgbuf, p); } -static void set_author_ident_env(const char *message) -{ - const char *p = message; - if (!p) - die ("Could not read commit message of %s", - sha1_to_hex(commit->object.sha1)); - while (*p && *p != '\n') { - const char *eol; - - for (eol = p; *eol && *eol != '\n'; eol++) - ; /* do nothing */ - if (!prefixcmp(p, "author ")) { - char *line, *pend, *email, *timestamp; - - p += 7; - line = xmemdupz(p, eol - p); - email = strchr(line, '<'); - if (!email) - die ("Could not extract author email from %s", - sha1_to_hex(commit->object.sha1)); - if (email == line) - pend = line; - else - for (pend = email; pend != line + 1 && - isspace(pend[-1]); pend--); - ; /* do nothing */ - *pend = '\0'; - email++; - timestamp = strchr(email, '>'); - if (!timestamp) - die ("Could not extract author time from %s", - sha1_to_hex(commit->object.sha1)); - *timestamp = '\0'; - for (timestamp++; *timestamp && isspace(*timestamp); - timestamp++) - ; /* do nothing */ - setenv("GIT_AUTHOR_NAME", line, 1); - setenv("GIT_AUTHOR_EMAIL", email, 1); - setenv("GIT_AUTHOR_DATE", timestamp, 1); - free(line); - return; - } - p = eol; - if (*p == '\n') - p++; - } - die ("No author information found in %s", - sha1_to_hex(commit->object.sha1)); -} - static void write_cherry_pick_head(void) { int fd; @@ -290,9 +239,7 @@ static void print_advice(void) advise("after resolving the conflicts, mark the corrected paths"); advise("with 'git add <paths>' or 'git rm <paths>'"); - - if (action == CHERRY_PICK) - advise("and commit the result with 'git commit -c CHERRY_PICK_HEAD'"); + advise("and commit the result with 'git commit'"); } static void write_message(struct strbuf *msgbuf, const char *filename) @@ -518,7 +465,6 @@ static int do_pick_commit(void) base_label = msg.parent_label; next = commit; next_label = msg.label; - set_author_ident_env(msg.message); add_message_to_msg(&msgbuf, msg.message); if (no_replay) { strbuf_addstr(&msgbuf, "(cherry picked from commit "); diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index ea52720..3e8d177 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -57,7 +57,7 @@ test_expect_success 'advice from failed cherry-pick' " error: could not apply \$picked... picked hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' - hint: and commit the result with 'git commit -c CHERRY_PICK_HEAD' + hint: and commit the result with 'git commit' EOF test_must_fail git cherry-pick picked 2>actual && diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh index 77b6920..b61fd3c 100755 --- a/t/t7509-commit.sh +++ b/t/t7509-commit.sh @@ -157,4 +157,33 @@ test_expect_success '--reset-author should be rejected without -c/-C/--amend' ' test_must_fail git commit -a --reset-author -m done ' +test_expect_success 'commit respects CHERRY_PICK_HEAD and MERGE_MSG' ' + echo "cherry-pick 1a" >>foo && + test_tick && + git commit -am "cherry-pick 1" --author="Cherry <cherry@pick.er>" && + git tag cherry-pick-head && + git rev-parse cherry-pick-head >.git/CHERRY_PICK_HEAD && + echo "This is a MERGE_MSG" >.git/MERGE_MSG && + echo "cherry-pick 1b" >>foo && + test_tick && + git commit -a && + author_header cherry-pick-head >expect && + author_header HEAD >actual && + test_cmp expect actual && + + echo "This is a MERGE_MSG" >expect && + message_body HEAD >actual && + test_cmp expect actual +' + +test_expect_success '--reset-author with CHERRY_PICK_HEAD' ' + git rev-parse cherry-pick-head >.git/CHERRY_PICK_HEAD && + echo "cherry-pick 2" >>foo && + test_tick && + git commit -am "cherry-pick 2" --reset-author && + echo "author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE" >expect && + author_header HEAD >actual && + test_cmp expect actual +' + test_done diff --git a/wt-status.c b/wt-status.c index 123582b..fbaaf54 100644 --- a/wt-status.c +++ b/wt-status.c @@ -60,7 +60,7 @@ static void wt_status_print_unmerged_header(struct wt_status *s) color_fprintf_ln(s->fp, c, "# Unmerged paths:"); if (!advice_status_hints) return; - if (s->in_merge) + if (s->whence != FROM_COMMIT) ; else if (!s->is_initial) color_fprintf_ln(s->fp, c, "# (use \"git reset %s <file>...\" to unstage)", s->reference); @@ -77,7 +77,7 @@ static void wt_status_print_cached_header(struct wt_status *s) color_fprintf_ln(s->fp, c, "# Changes to be committed:"); if (!advice_status_hints) return; - if (s->in_merge) + if (s->whence != FROM_COMMIT) ; /* NEEDSWORK: use "git reset --unresolve"??? */ else if (!s->is_initial) color_fprintf_ln(s->fp, c, "# (use \"git reset %s <file>...\" to unstage)", s->reference); diff --git a/wt-status.h b/wt-status.h index 20b17cf..cec482a 100644 --- a/wt-status.h +++ b/wt-status.h @@ -24,6 +24,13 @@ enum untracked_status_type { SHOW_ALL_UNTRACKED_FILES }; +/* from where does this commit originate */ +enum commit_whence { + FROM_COMMIT, /* normal */ + FROM_MERGE, /* commit came from merge */ + FROM_CHERRY_PICK /* commit came from cherry-pick */ +}; + struct wt_status_change_data { int worktree_status; int index_status; @@ -40,7 +47,7 @@ struct wt_status { const char **pathspec; int verbose; int amend; - int in_merge; + enum commit_whence whence; int nowarn; int use_color; int relative_paths; -- 1.7.4.1.29.g21713 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] Teach commit about CHERRY_PICK_HEAD 2011-02-17 5:50 ` [PATCH] Teach commit about CHERRY_PICK_HEAD Jay Soffian @ 2011-02-18 0:29 ` Jonathan Nieder 0 siblings, 0 replies; 17+ messages in thread From: Jonathan Nieder @ 2011-02-18 0:29 UTC (permalink / raw) To: Jay Soffian; +Cc: git, Ævar Arnfjörð Bjarmason, Junio C Hamano Jay Soffian wrote: > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -54,9 +54,16 @@ static const char empty_amend_advice[] = > "it empty. You can repeat your command with --allow-empty, or you can\n" > "remove the commit entirely with \"git reset HEAD^\".\n"; > > +static const char empty_cherry_pick_advice[] = > +"The most recent cherry-pick is empty. If you wish to commit it, use:\n" > +"\n" > +" git commit --allow-empty\n" > +"\n" > +"Otherwise, please remove the file %s\n"; After scratching my head a little, this seems to mean After conflict resolution, your last cherry-pick does not introduce any changes. To commit it as an empty commit, use:\n \n git commit --allow-empty\n \n Alternatively, you can cancel the cherry-pick by removing\n the file %s\n It suspect it might be more intuitive to say Alternatively, you can cancel the cherry-pick by running\n "git reset".\n which works because the index is known to be clean at that point. > +/* > + * The _message variables are commit names from which to take > + * the commit message and/or authorship. > + */ Makes sense, thanks. I'll think more about an intuitive and consistent name for these and hopefully send a patch for it later as a separate topic. > + if (whence == FROM_CHERRY_PICK && !renew_authorship) { > + author_message = "CHERRY_PICK_HEAD"; > + author_message_buffer = read_commit_message(author_message); I mentioned in a side thread that script authors might be happier if their carefully prepared GIT_AUTHOR_NAME and GIT_AUTHOR_EMAIL do not get overridden in this case, but I was wrong[*]. Might make sense to add a test for that: GIT_AUTHOR_NAME="Someone else" && GIT_AUTHOR_EMAIL=someone@else.example.com && export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL && git cherry-pick something && git diff-tree -s --format="%an <%ae>" >actual && test_cmp expect actual (This is just a side note; the patch is good.) In the $GIT_CHERRY_PICK_HELP set case, won't the CHERRY_PICK_HEAD behavior be harmless? rebase --interactive --continue wants to set GIT_AUTHOR_NAME and GIT_AUTHOR_EMAIL but that is only in order to copy the author identity from the original commit. > --- a/wt-status.c > +++ b/wt-status.c > @@ -60,7 +60,7 @@ static void wt_status_print_unmerged_header(struct wt_status *s) > color_fprintf_ln(s->fp, c, "# Unmerged paths:"); > if (!advice_status_hints) > return; > - if (s->in_merge) > + if (s->whence != FROM_COMMIT) > ; > else if (!s->is_initial) > color_fprintf_ln(s->fp, c, "# (use \"git reset %s <file>...\" to unstage)", s->reference); The advice 'use "git reset HEAD -- <file>..." to unstage' makes perfect sense to me in the cherry-pick case, no? The operator is replaying an existing commit, and tweaking the result amounts to pretending she made the change herself and tweaking that. > @@ -77,7 +77,7 @@ static void wt_status_print_cached_header(struct wt_status *s) > color_fprintf_ln(s->fp, c, "# Changes to be committed:"); > if (!advice_status_hints) > return; > - if (s->in_merge) > + if (s->whence != FROM_COMMIT) > ; /* NEEDSWORK: use "git reset --unresolve"??? */ Likewise. > --- a/wt-status.h > +++ b/wt-status.h > @@ -24,6 +24,13 @@ enum untracked_status_type { > SHOW_ALL_UNTRACKED_FILES > }; > > +/* from where does this commit originate */ > +enum commit_whence { > + FROM_COMMIT, /* normal */ > + FROM_MERGE, /* commit came from merge */ > + FROM_CHERRY_PICK /* commit came from cherry-pick */ > +}; I think these comments are not in a place that will help a person understand the code, but I don't have the energy to go back and forth on it. Everything not mentioned above except the --no-commit case mentioned by Junio looks good to me, though I haven't tested. Thanks for your thoughtfulness. Jonathan [*] -- 8< -- Subject: [BAD IDEA] commit: let GIT_AUTHOR_NAME/EMAIL take effect when commiting a cherry-pick commit -c/-C/--amend to take the commit message and author from another message has always overridden the GIT_AUTHOR_NAME and GIT_AUTHOR_EMAIL variables. Letting the command line override the environment in this way is generally convenient and more intuitive if a person has forgotten what is in the environment: GIT_AUTHOR_NAME=me GIT_AUTHOR_EMAIL=email@example.com export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... git commit; # using identity from environment ... git commit; # another commit as me ... # commit by someone else git commit --author='Someone Else <other@example.com>' ... # commit by someone else git commit -c someone-elses-commit The new CHERRY_PICK_HEAD feature inherits the same semantics. Uncareful scripts that want to commit with a different author name and email when taking over after a failed cherry-pick use authorship from the cherry-picked commit instead of the environment: ! git cherry-pick complicated; # failed cherry-pick ... resolve the conflict ... git commit While that could theoretically break some scripts, it's worth it for consistency, the intuitiveness-when-user-forgets-environment issue mentioned above, and because cherry-pick itself, which internally does something like git cherry-pick --no-commit $1; # simple cherry-pick git commit has always ignored the GIT_AUTHOR_NAME/EMAIL environment settings. The patch below makes commit with CHERRY_PICK_HEAD respect GIT_AUTHOR_NAME/EMAIL anyway, to demonstrate how broken that is. It breaks tests t3404 and t3506. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- builtin/commit.c | 59 ++++++++++++++++++++++++++++++++--------------------- 1 files changed, 36 insertions(+), 23 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 35eb024..f398910 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -80,6 +80,7 @@ static const char *template_file; * the commit message and/or authorship. */ static const char *author_message, *author_message_buffer; +static int author_message_overrides_environ; static char *edit_message, *use_message; static char *fixup_message, *squash_message; static int all, edit_flag, also, interactive, only, amend, signoff; @@ -504,6 +505,38 @@ static int is_a_merge(const unsigned char *sha1) static const char sign_off_header[] = "Signed-off-by: "; +static void reuse_author_info(char **name, char **email, char **date) +{ + const char *a, *lb, *rb, *eol; + + if (author_message_overrides_environ) + *name = *email = *date = NULL; + + a = strstr(author_message_buffer, "\nauthor "); + if (!a) + die("invalid commit: %s", author_message); + + lb = strchrnul(a + strlen("\nauthor "), '<'); + rb = strchrnul(lb, '>'); + eol = strchrnul(rb, '\n'); + if (!*lb || !*rb || !*eol) + die("invalid commit: %s", author_message); + + if (!*name) { + if (lb == a + strlen("\nauthor ")) + /* \nauthor <foo@example.com> */ + *name = xcalloc(1, 1); + else + *name = xmemdupz(a + strlen("\nauthor "), + (lb - strlen(" ") - + (a + strlen("\nauthor ")))); + } + if (!*email) + *email = xmemdupz(lb + strlen("<"), rb - (lb + strlen("<"))); + if (!*date) + *date = xmemdupz(rb + strlen("> "), eol - (rb + strlen("> "))); +} + static void determine_author_info(struct strbuf *author_ident) { char *name, *email, *date; @@ -512,29 +545,8 @@ static void determine_author_info(struct strbuf *author_ident) email = getenv("GIT_AUTHOR_EMAIL"); date = getenv("GIT_AUTHOR_DATE"); - if (author_message) { - const char *a, *lb, *rb, *eol; - - a = strstr(author_message_buffer, "\nauthor "); - if (!a) - die("invalid commit: %s", author_message); - - lb = strchrnul(a + strlen("\nauthor "), '<'); - rb = strchrnul(lb, '>'); - eol = strchrnul(rb, '\n'); - if (!*lb || !*rb || !*eol) - die("invalid commit: %s", author_message); - - if (lb == a + strlen("\nauthor ")) - /* \nauthor <foo@example.com> */ - name = xcalloc(1, 1); - else - name = xmemdupz(a + strlen("\nauthor "), - (lb - strlen(" ") - - (a + strlen("\nauthor ")))); - email = xmemdupz(lb + strlen("<"), rb - (lb + strlen("<"))); - date = xmemdupz(rb + strlen("> "), eol - (rb + strlen("> "))); - } + if (author_message) + reuse_author_info(&name, &email, &date); if (force_author) { const char *lb = strstr(force_author, " <"); @@ -1034,6 +1046,7 @@ static int parse_and_validate_options(int argc, const char *argv[], author_message = use_message; author_message_buffer = use_message_buffer; } + author_message_overrides_environ = 1; } if (whence == FROM_CHERRY_PICK && !renew_authorship) { author_message = "CHERRY_PICK_HEAD"; -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 3/4] commit.c: replace some literal strings with constants 2011-02-17 4:18 ` [PATCH v3 3/4] commit.c: replace some literal strings with constants Jay Soffian 2011-02-17 5:19 ` Jonathan Nieder @ 2011-02-17 5:49 ` Junio C Hamano 1 sibling, 0 replies; 17+ messages in thread From: Junio C Hamano @ 2011-02-17 5:49 UTC (permalink / raw) To: Jay Soffian Cc: git, Jonathan Nieder, Junio C Hamano, Ævar Arnfjörð Bjarmason Jay Soffian <jaysoffian@gmail.com> writes: > A typo in any of these would be bad, so let's use > constants for them > > Signed-off-by: Jay Soffian <jaysoffian@gmail.com> > --- > I converted these per > http://article.gmane.org/gmane.comp.version-control.git/167015 > > Maybe this should be the last patch in the series; it's questionable to > me whether it's even worth doing. Yeah, it's horrible ;-) > +static const char commit_editmsg[] = "COMMIT_EDITMSG"; > +static const char cherry_pick_head[] = "CHERRY_PICK_HEAD"; > +static const char merge_head[] = "MERGE_HEAD"; > +static const char merge_msg[] = "MERGE_MSG"; > +static const char merge_mode[] = "MERGE_MODE"; > +static const char squash_msg[] = "SQUASH_MSG"; If they were like static const char COMMIT_EDITMSG[] = "..."; then that would give us typo-safety while keeping the similarity from original code. Uppercase variable names are a bit weird, though. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 4/4] Teach commit about CHERRY_PICK_HEAD 2011-02-17 4:18 [PATCH v3 0/4] CHERRY_PICK_HEAD Jay Soffian ` (2 preceding siblings ...) 2011-02-17 4:18 ` [PATCH v3 3/4] commit.c: replace some literal strings with constants Jay Soffian @ 2011-02-17 4:18 ` Jay Soffian 3 siblings, 0 replies; 17+ messages in thread From: Jay Soffian @ 2011-02-17 4:18 UTC (permalink / raw) To: git Cc: Jay Soffian, Jonathan Nieder, Junio C Hamano, Ævar Arnfjörð Bjarmason Previously the user was advised to use commit -c CHERRY_PICK_HEAD after a conflicting cherry-pick. While this would preserve the original commit's authorship, it would sadly discard cherry-pick's carefully crafted MERGE_MSG (which contains the list of conflicts as well as the original commit-id in the case of cherry-pick -x). On the other hand, if a bare 'commit' were performed, it would preserve the MERGE_MSG while resetting the authorship. In other words, there was no way to simultaneously take the authorship from CHERRY_PICK_HEAD and the commit message from MERGE_MSG. This change fixes that situation. A bare 'commit' will now take the authorship from CHERRY_PICK_HEAD and the commit message from MERGE_MSG. If the user wishes to reset authorship, that must now be done explicitly via --reset-author. A side-benefit of passing commit authorship along this way is that we can eliminate redundant authorship parsing code from revert.c. (Also removed an unused include from revert.c) Signed-off-by: Jay Soffian <jaysoffian@gmail.com> --- Incorporates feedback from: - http://article.gmane.org/gmane.comp.version-control.git/167000 - http://article.gmane.org/gmane.comp.version-control.git/167015 Documentation/git-commit.txt | 7 +- builtin/commit.c | 155 ++++++++++++++++++++++++++++----------- builtin/revert.c | 56 +-------------- t/t3507-cherry-pick-conflict.sh | 2 +- t/t7509-commit.sh | 29 +++++++ wt-status.c | 4 +- wt-status.h | 9 ++- 7 files changed, 158 insertions(+), 104 deletions(-) diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index b586c0f..fd6a1f7 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -84,9 +84,10 @@ OPTIONS linkgit:git-rebase[1] for details. --reset-author:: - When used with -C/-c/--amend options, declare that the - authorship of the resulting commit now belongs of the committer. - This also renews the author timestamp. + When used with -C/-c/--amend options, or when committing after a + a conflicting cherry-pick, declare that the authorship of the + resulting commit now belongs of the committer. This also renews + the author timestamp. --short:: When doing a dry-run, give the output in the short-format. See diff --git a/builtin/commit.c b/builtin/commit.c index 5b32743..af750df 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -54,6 +54,13 @@ static const char empty_amend_advice[] = "it empty. You can repeat your command with --allow-empty, or you can\n" "remove the commit entirely with \"git reset HEAD^\".\n"; +static const char empty_cherry_pick_advice[] = +"The most recent cherry-pick is empty. If you wish to commit it, use:\n" +"\n" +" git commit --allow-empty\n" +"\n" +"Otherwise, please remove the file %s\n"; + static const char commit_editmsg[] = "COMMIT_EDITMSG"; static const char cherry_pick_head[] = "CHERRY_PICK_HEAD"; static const char merge_head[] = "MERGE_HEAD"; @@ -63,7 +70,7 @@ static const char squash_msg[] = "SQUASH_MSG"; static unsigned char head_sha1[20]; -static char *use_message_buffer; +static const char *use_message_buffer; static struct lock_file index_lock; /* real index */ static struct lock_file false_lock; /* used only for partial commits */ static enum { @@ -74,6 +81,11 @@ static enum { static const char *logfile, *force_author; static const char *template_file; +/* + * The _message variables are commit names from which to take + * the commit message and/or authorship. + */ +static const char *author_message, *author_message_buffer; static char *edit_message, *use_message; static char *fixup_message, *squash_message; static int all, edit_flag, also, interactive, only, amend, signoff; @@ -94,7 +106,8 @@ static enum { } cleanup_mode; static char *cleanup_arg; -static int use_editor = 1, initial_commit, in_merge, include_status = 1; +static enum commit_whence whence; +static int use_editor = 1, initial_commit, include_status = 1; static int show_ignored_in_status; static const char *only_include_assumed; static struct strbuf message; @@ -169,6 +182,36 @@ static struct option builtin_commit_options[] = { OPT_END() }; +static void determine_whence(struct wt_status *s) +{ + if (file_exists(git_path(merge_head))) + whence = FROM_MERGE; + else if (file_exists(git_path(cherry_pick_head))) + whence = FROM_CHERRY_PICK; + else + whence = FROM_COMMIT; + if (s) + s->whence = whence; +} + +static const char *whence_s(void) +{ + char *s = ""; + + switch (whence) { + case FROM_COMMIT: + break; + case FROM_MERGE: + s = "merge"; + break; + case FROM_CHERRY_PICK: + s = "cherry-pick"; + break; + } + + return s; +} + static void rollback_index_files(void) { switch (commit_style) { @@ -384,8 +427,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int */ commit_style = COMMIT_PARTIAL; - if (in_merge) - die("cannot do a partial commit during a merge."); + if (whence != FROM_COMMIT) + die("cannot do a partial commit during a %s.", whence_s()); memset(&partial, 0, sizeof(partial)); partial.strdup_strings = 1; @@ -475,18 +518,18 @@ static void determine_author_info(struct strbuf *author_ident) email = getenv("GIT_AUTHOR_EMAIL"); date = getenv("GIT_AUTHOR_DATE"); - if (use_message && !renew_authorship) { + if (author_message) { const char *a, *lb, *rb, *eol; - a = strstr(use_message_buffer, "\nauthor "); + a = strstr(author_message_buffer, "\nauthor "); if (!a) - die("invalid commit: %s", use_message); + die("invalid commit: %s", author_message); lb = strchrnul(a + strlen("\nauthor "), '<'); rb = strchrnul(lb, '>'); eol = strchrnul(rb, '\n'); if (!*lb || !*rb || !*eol) - die("invalid commit: %s", use_message); + die("invalid commit: %s", author_message); if (lb == a + strlen("\nauthor ")) /* \nauthor <foo@example.com> */ @@ -647,11 +690,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix, } /* - * This final case does not modify the template message, - * it just sets the argument to the prepare-commit-msg hook. + * The remaining cases don't modify the template message, but + * just set the argument(s) to the prepare-commit-msg hook. */ - else if (in_merge) + else if (whence == FROM_MERGE) hook_arg1 = "merge"; + else if (whence == FROM_CHERRY_PICK) { + hook_arg1 = "commit"; + hook_arg2 = cherry_pick_head; + } if (squash_message) { /* @@ -700,16 +747,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix, strbuf_addstr(&committer_ident, git_committer_info(0)); if (use_editor && include_status) { char *ai_tmp, *ci_tmp; - if (in_merge) + if (whence != FROM_COMMIT) fprintf(fp, "#\n" - "# It looks like you may be committing a MERGE.\n" + "# It looks like you may be committing a %s.\n" "# If this is not correct, please remove the file\n" "# %s\n" "# and try again.\n" "#\n", - git_path(merge_head)); - + whence_s(), + git_path(whence == FROM_MERGE + ? merge_head + : cherry_pick_head)); fprintf(fp, "\n" "# Please enter the commit message for your changes."); @@ -772,11 +821,19 @@ static int prepare_to_commit(const char *index_file, const char *prefix, fclose(fp); - if (!commitable && !in_merge && !allow_empty && + /* + * Reject an attempt to record a non-merge empty commit without + * explicit --allow-empty. In the cherry-pick case, it may be + * empty due to conflict resolution, which the user should okay. + */ + if (!commitable && whence != FROM_MERGE && !allow_empty && !(amend && is_a_merge(head_sha1))) { run_status(stdout, index_file, prefix, 0, s); if (amend) fputs(empty_amend_advice, stderr); + else if (whence == FROM_CHERRY_PICK) + fprintf(stderr, empty_cherry_pick_advice, + git_path(cherry_pick_head)); return 0; } @@ -904,6 +961,28 @@ static void handle_untracked_files_arg(struct wt_status *s) die("Invalid untracked files mode '%s'", untracked_files_arg); } +static const char *read_commit_message(const char *name) +{ + const char *out_enc, *out; + struct commit *commit; + + commit = lookup_commit_reference_by_name(name); + if (!commit) + die("could not lookup commit %s", name); + out_enc = get_commit_output_encoding(); + out = logmsg_reencode(commit, out_enc); + + /* + * If we failed to reencode the buffer, just copy it + * byte for byte so the user can try to fix it up. + * This also handles the case where input and output + * encodings are identical. + */ + if (out == NULL) + out = xstrdup(commit->buffer); + return out; +} + static int parse_and_validate_options(int argc, const char *argv[], const char * const usage[], const char *prefix, @@ -933,8 +1012,8 @@ static int parse_and_validate_options(int argc, const char *argv[], /* Sanity check options */ if (amend && initial_commit) die("You have nothing to amend."); - if (amend && in_merge) - die("You are in the middle of a merge -- cannot amend."); + if (amend && whence != FROM_COMMIT) + die("You are in the middle of a %s -- cannot amend.", whence_s()); if (fixup_message && squash_message) die("Options --squash and --fixup cannot be used together"); if (use_message) @@ -953,26 +1032,18 @@ static int parse_and_validate_options(int argc, const char *argv[], use_message = edit_message; if (amend && !use_message && !fixup_message) use_message = "HEAD"; - if (!use_message && renew_authorship) + if (!use_message && whence != FROM_CHERRY_PICK && renew_authorship) die("--reset-author can be used only with -C, -c or --amend."); if (use_message) { - const char *out_enc; - struct commit *commit; - - commit = lookup_commit_reference_by_name(use_message); - if (!commit) - die("could not lookup commit %s", use_message); - out_enc = get_commit_output_encoding(); - use_message_buffer = logmsg_reencode(commit, out_enc); - - /* - * If we failed to reencode the buffer, just copy it - * byte for byte so the user can try to fix it up. - * This also handles the case where input and output - * encodings are identical. - */ - if (use_message_buffer == NULL) - use_message_buffer = xstrdup(commit->buffer); + use_message_buffer = read_commit_message(use_message); + if (!renew_authorship) { + author_message = use_message; + author_message_buffer = use_message_buffer; + } + } + if (whence == FROM_CHERRY_PICK && !renew_authorship) { + author_message = cherry_pick_head; + author_message_buffer = read_commit_message(author_message); } if (!!also + !!only + !!all + !!interactive > 1) @@ -1123,7 +1194,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) wt_status_prepare(&s); gitmodules_config(); git_config(git_status_config, &s); - in_merge = file_exists(git_path(merge_head)); + determine_whence(&s); argc = parse_options(argc, argv, prefix, builtin_status_options, builtin_status_usage, 0); @@ -1146,7 +1217,6 @@ int cmd_status(int argc, const char **argv, const char *prefix) } s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0; - s.in_merge = in_merge; s.ignore_submodule_arg = ignore_submodule_arg; wt_status_collect(&s); @@ -1308,8 +1378,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) wt_status_prepare(&s); git_config(git_commit_config, &s); - in_merge = file_exists(git_path(merge_head)); - s.in_merge = in_merge; + determine_whence(&s); if (s.use_color == -1) s.use_color = git_use_color_default; @@ -1346,7 +1415,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) for (c = commit->parents; c; c = c->next) pptr = &commit_list_insert(c->item, pptr)->next; - } else if (in_merge) { + } else if (whence == FROM_MERGE) { struct strbuf m = STRBUF_INIT; FILE *fp; @@ -1375,7 +1444,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix) parents = reduce_heads(parents); } else { if (!reflog_msg) - reflog_msg = "commit"; + reflog_msg = (whence == FROM_CHERRY_PICK) + ? "commit (cherry-pick)" + : "commit"; pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next; } diff --git a/builtin/revert.c b/builtin/revert.c index fbb465a..2ecc6e1 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -3,7 +3,6 @@ #include "object.h" #include "commit.h" #include "tag.h" -#include "wt-status.h" #include "run-command.h" #include "exec_cmd.h" #include "utf8.h" @@ -198,56 +197,6 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message) strbuf_addstr(msgbuf, p); } -static void set_author_ident_env(const char *message) -{ - const char *p = message; - if (!p) - die ("Could not read commit message of %s", - sha1_to_hex(commit->object.sha1)); - while (*p && *p != '\n') { - const char *eol; - - for (eol = p; *eol && *eol != '\n'; eol++) - ; /* do nothing */ - if (!prefixcmp(p, "author ")) { - char *line, *pend, *email, *timestamp; - - p += 7; - line = xmemdupz(p, eol - p); - email = strchr(line, '<'); - if (!email) - die ("Could not extract author email from %s", - sha1_to_hex(commit->object.sha1)); - if (email == line) - pend = line; - else - for (pend = email; pend != line + 1 && - isspace(pend[-1]); pend--); - ; /* do nothing */ - *pend = '\0'; - email++; - timestamp = strchr(email, '>'); - if (!timestamp) - die ("Could not extract author time from %s", - sha1_to_hex(commit->object.sha1)); - *timestamp = '\0'; - for (timestamp++; *timestamp && isspace(*timestamp); - timestamp++) - ; /* do nothing */ - setenv("GIT_AUTHOR_NAME", line, 1); - setenv("GIT_AUTHOR_EMAIL", email, 1); - setenv("GIT_AUTHOR_DATE", timestamp, 1); - free(line); - return; - } - p = eol; - if (*p == '\n') - p++; - } - die ("No author information found in %s", - sha1_to_hex(commit->object.sha1)); -} - static void write_cherry_pick_head(void) { int fd; @@ -290,9 +239,7 @@ static void print_advice(void) advise("after resolving the conflicts, mark the corrected paths"); advise("with 'git add <paths>' or 'git rm <paths>'"); - - if (action == CHERRY_PICK) - advise("and commit the result with 'git commit -c CHERRY_PICK_HEAD'"); + advise("and commit the result with 'git commit'"); } static void write_message(struct strbuf *msgbuf, const char *filename) @@ -518,7 +465,6 @@ static int do_pick_commit(void) base_label = msg.parent_label; next = commit; next_label = msg.label; - set_author_ident_env(msg.message); add_message_to_msg(&msgbuf, msg.message); if (no_replay) { strbuf_addstr(&msgbuf, "(cherry picked from commit "); diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index ea52720..3e8d177 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -57,7 +57,7 @@ test_expect_success 'advice from failed cherry-pick' " error: could not apply \$picked... picked hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' - hint: and commit the result with 'git commit -c CHERRY_PICK_HEAD' + hint: and commit the result with 'git commit' EOF test_must_fail git cherry-pick picked 2>actual && diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh index 77b6920..b61fd3c 100755 --- a/t/t7509-commit.sh +++ b/t/t7509-commit.sh @@ -157,4 +157,33 @@ test_expect_success '--reset-author should be rejected without -c/-C/--amend' ' test_must_fail git commit -a --reset-author -m done ' +test_expect_success 'commit respects CHERRY_PICK_HEAD and MERGE_MSG' ' + echo "cherry-pick 1a" >>foo && + test_tick && + git commit -am "cherry-pick 1" --author="Cherry <cherry@pick.er>" && + git tag cherry-pick-head && + git rev-parse cherry-pick-head >.git/CHERRY_PICK_HEAD && + echo "This is a MERGE_MSG" >.git/MERGE_MSG && + echo "cherry-pick 1b" >>foo && + test_tick && + git commit -a && + author_header cherry-pick-head >expect && + author_header HEAD >actual && + test_cmp expect actual && + + echo "This is a MERGE_MSG" >expect && + message_body HEAD >actual && + test_cmp expect actual +' + +test_expect_success '--reset-author with CHERRY_PICK_HEAD' ' + git rev-parse cherry-pick-head >.git/CHERRY_PICK_HEAD && + echo "cherry-pick 2" >>foo && + test_tick && + git commit -am "cherry-pick 2" --reset-author && + echo "author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE" >expect && + author_header HEAD >actual && + test_cmp expect actual +' + test_done diff --git a/wt-status.c b/wt-status.c index 123582b..fbaaf54 100644 --- a/wt-status.c +++ b/wt-status.c @@ -60,7 +60,7 @@ static void wt_status_print_unmerged_header(struct wt_status *s) color_fprintf_ln(s->fp, c, "# Unmerged paths:"); if (!advice_status_hints) return; - if (s->in_merge) + if (s->whence != FROM_COMMIT) ; else if (!s->is_initial) color_fprintf_ln(s->fp, c, "# (use \"git reset %s <file>...\" to unstage)", s->reference); @@ -77,7 +77,7 @@ static void wt_status_print_cached_header(struct wt_status *s) color_fprintf_ln(s->fp, c, "# Changes to be committed:"); if (!advice_status_hints) return; - if (s->in_merge) + if (s->whence != FROM_COMMIT) ; /* NEEDSWORK: use "git reset --unresolve"??? */ else if (!s->is_initial) color_fprintf_ln(s->fp, c, "# (use \"git reset %s <file>...\" to unstage)", s->reference); diff --git a/wt-status.h b/wt-status.h index 20b17cf..cec482a 100644 --- a/wt-status.h +++ b/wt-status.h @@ -24,6 +24,13 @@ enum untracked_status_type { SHOW_ALL_UNTRACKED_FILES }; +/* from where does this commit originate */ +enum commit_whence { + FROM_COMMIT, /* normal */ + FROM_MERGE, /* commit came from merge */ + FROM_CHERRY_PICK /* commit came from cherry-pick */ +}; + struct wt_status_change_data { int worktree_status; int index_status; @@ -40,7 +47,7 @@ struct wt_status { const char **pathspec; int verbose; int amend; - int in_merge; + enum commit_whence whence; int nowarn; int use_color; int relative_paths; -- 1.7.4.1.30.g7fe09 ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2011-02-20 6:44 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-02-17 4:18 [PATCH v3 0/4] CHERRY_PICK_HEAD Jay Soffian 2011-02-17 4:18 ` [PATCH v3 1/4] Introduce CHERRY_PICK_HEAD Jay Soffian 2011-02-17 20:01 ` Junio C Hamano 2011-02-17 22:32 ` Jonathan Nieder 2011-02-20 2:29 ` Jay Soffian 2011-02-20 2:48 ` Jonathan Nieder 2011-02-20 6:43 ` Junio C Hamano 2011-02-17 21:56 ` Junio C Hamano 2011-02-17 22:42 ` Jay Soffian 2011-02-17 22:48 ` Jonathan Nieder 2011-02-17 4:18 ` [PATCH v3 2/4] bash: teach __git_ps1 about CHERRY_PICK_HEAD Jay Soffian 2011-02-17 4:18 ` [PATCH v3 3/4] commit.c: replace some literal strings with constants Jay Soffian 2011-02-17 5:19 ` Jonathan Nieder 2011-02-17 5:50 ` [PATCH] Teach commit about CHERRY_PICK_HEAD Jay Soffian 2011-02-18 0:29 ` Jonathan Nieder 2011-02-17 5:49 ` [PATCH v3 3/4] commit.c: replace some literal strings with constants Junio C Hamano 2011-02-17 4:18 ` [PATCH v3 4/4] Teach commit about CHERRY_PICK_HEAD Jay Soffian
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).