Re: [RFC/PATCH 2/2] Teach commit to handle CHERRY_HEAD automatically
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Feb 15, 2011, 23:00 UTC
- Message-ID
- <20110215230015.GA17812@elie>
- In-Reply-To
- <1297805034-3512-3-git-send-email-jaysoffian@gmail.com>
Hi,
Some quick thoughts.
Jay Soffian wrote:
Show 7 quoted lines
> Now that cherry-pick records the original commit id in CHERRY_HEAD > following a conflict, we can teach commit to handle it automatically. > > The primary difference between this and 'commit -c CHERRY_HEAD' > (besides saving the user some typing) is that we take only the > authorship from CHERRY_HEAD, while taking the commit message > from MERGE_MSG
This answers my question from before.
[...]
> For example, it should probably be an error to use --squash/--fixup when > there's a CHERRY_HEAD, but then again, it should probably be an error to > use those options when there's a MERGE_HEAD (but it's not).
I see what you're saying about MERGE_HEAD --- it is not clear what is intended when a person asks "rebase --autosquash" to squash in a merge commit. With CHERRY_PICK_HEAD, it means "after dealing with the conflict, I understand this commit better, and it ought to have been squashed in with commit X", no?
There is some potential for confusion from another direction. The uncareful patch series maintainer can be tempted to try, after conflict resolution,
- "git commit --amend" to say "I'm done fixing the broken thing".
- "git commit --fixup=HEAD/--squash=HEAD" to say "done fixing; let's look at this again later and squash it when I am less confused".
Both are mistakes if HEAD is the previous and good commit rather than the broken one. Maybe there is some simple safety that could protect against them?
Show 11 quoted lines
> --- 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,
or when committing after a conflicted merge, no?
> declare that the authorship of the > + resulting commit now belongs of the committer. This also renews > + the author timestamp.
How does it interact with --author?
> +++ b/builtin/commit.c
[...]
Show 6 quoted lines
> @@ -609,7 +609,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> die_errno("could not read log file '%s'",
> logfile);
> hook_arg1 = "message";
> - } else if (use_message) {
> + } else if (use_message && !cherry_pick) {Wouldn't this make
git commit -C foo
after a failed cherry-pick use MERGE_MSG instead of the message the user requested?
Show 8 quoted lines
> @@ -704,6 +704,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> "#\n",
> git_path("MERGE_HEAD"));
>
> + if (cherry_pick)
> + fprintf(fp,
> + "#\n"
> + "# It looks like you may be committing a cherry-pick.\n"Aside: shouldn't we suggest some porcelain-ish command (git merge --clear? git commit --no-merge?) to remove .git/MERGE_HEAD instead of asking the committer to do it?
This section is used to give a preview and sanity check for the commit.
- if we are on the wrong branch, that might be a mistake. - if the author is someone else, that might be a mistake. - if there are multiple parents, that might be a mistake. - if there are changes included, some might be mistakes. - if there are changes excluded, some might be mistakes, - if there are untracked files, some might be mistakes.
Where does committing a cherry-pick fall into that picture? Maybe
# Author: A U Thor <author@example.com> # Date: Tue Beb 9 01:23:45 1911 -0500 # # If this is not correct, please try again using the # --author and --date or --reset-author options.
Show 5 quoted lines
> @@ -898,6 +907,7 @@ static void handle_untracked_files_arg(struct wt_status *s)
> die("Invalid untracked files mode '%s'", untracked_files_arg);
> }
>
> +Stray whitespace change?
Show 6 quoted lines
> @@ -929,6 +939,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
> die("You have nothing to amend.");
> if (amend && in_merge)
> die("You are in the middle of a merge -- cannot amend.");
> + if (amend && cherry_pick)
> + die("You are in the middle of a cherry-pick -- cannot amend.");Ah, this addresses the worry mentioned above.
How can I get out of this state if I really do want to amend?
Show 12 quoted lines
> @@ -943,11 +955,19 @@ static int parse_and_validate_options(int argc, const char *argv[],
> die("Only one of -c/-C/-F/--fixup can be used.");
> if (message.len && f > 0)
> die("Option -m cannot be combined with -c/-C/-F/--fixup.");
> + if (cherry_pick) {
> + /* Let message-specifying options override CHERRY_HEAD */
> + if (f > 0 || message.len)
> + cherry_pick = 0;
> + else
> + /* used for authorship side-effect only */
> + use_message = "CHERRY_HEAD";
> + }Hmm, what if I have commits F and F' and after trying to cherry-pick F I decide I want the message from F'?
Show 5 quoted lines
> @@ -1118,6 +1138,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
> gitmodules_config();
> git_config(git_status_config, &s);
> in_merge = file_exists(git_path("MERGE_HEAD"));
> + cherry_pick = file_exists(git_path("CHERRY_HEAD"));Is it possible to be both in_merge and cherry_pick?
Show 7 quoted lines
> @@ -1369,7 +1391,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> parents = reduce_heads(parents);
> } else {
> if (!reflog_msg)
> - reflog_msg = "commit";
> + reflog_msg = cherry_pick ? "commit (cherry-pick)"
> + : "commit";Nice. Probably worth mentioning in the commit message.
Show 8 quoted lines
> --- 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_HEAD" ]; then > + r="|CHERRY-PICKING"
Likewise.