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@xxxxxxxxxxxxxxxx &&
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@xxxxxxxxxxx
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@xxxxxxxxxxx>'
...
# 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@xxxxxxxxx>
---
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@xxxxxxxxxxx> */
+ *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@xxxxxxxxxxx> */
- 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html