revert: clarify label on conflict hunks

When reverting a commit, the commit being merged is not the commit
to revert itself but its parent.  Add “parent of” to the conflict
hunk label to make this more clear.

The conflict hunk labels are all pieces of a single string written in
the new get_message() function.  Avoid some complication by using
mempcpy to advance a pointer as the result is written.

Also free the corresponding temporary buffer (it was leaked before).
This is not important because it is a small one-time allocation.  It
would become a memory leak if unnoticed when libifying revert.

This patch uses calls to strlen() instead of integer constants in some
places.  GCC will compute the length at compile time; I am not sure
about other compilers, but this is not performance-critical anyway.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/revert.c b/builtin/revert.c
index eff52687a87b45b766e064eede7c2c35b6d133d2..5a5b72112c132440888e2f88c4ae0162c7b1078f 100644 (file)
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -45,6 +45,8 @@ static const char *me;
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 

+static char *get_encoding(const char *message);
+

 static void parse_args(int argc, const char **argv)
 {
        const char * const * usage_str =
 static void parse_args(int argc, const char **argv)
 {
        const char * const * usage_str =


@@ -73,33 +75,64 @@ static void parse_args(int argc, const char **argv)
                exit(1);
 }
 
                exit(1);
 }
 

-static char *get_oneline(const char *message)
+struct commit_message {
+       char *parent_label;
+       const char *label;
+       const char *subject;
+       char *reencoded_message;
+       const char *message;
+};
+
+static int get_message(const char *raw_message, struct commit_message *out)

 {
 {

-       char *result;
-       const char *p = message, *abbrev, *eol;
+       const char *encoding;
+       const char *p, *abbrev, *eol;
+       char *q;

        int abbrev_len, oneline_len;
 
        int abbrev_len, oneline_len;
 

-       if (!p)
-               die ("Could not read commit message of %s",
-                               sha1_to_hex(commit->object.sha1));
+       if (!raw_message)
+               return -1;
+       encoding = get_encoding(raw_message);
+       if (!encoding)
+               encoding = "UTF-8";
+       if (!git_commit_encoding)
+               git_commit_encoding = "UTF-8";
+       if ((out->reencoded_message = reencode_string(raw_message,
+                                       git_commit_encoding, encoding)))
+               out->message = out->reencoded_message;
+
+       abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
+       abbrev_len = strlen(abbrev);
+
+       /* Find beginning and end of commit subject. */
+       p = out->message;

        while (*p && (*p != '\n' || p[1] != '\n'))
                p++;
        while (*p && (*p != '\n' || p[1] != '\n'))
                p++;

-

        if (*p) {
                p += 2;
                for (eol = p + 1; *eol && *eol != '\n'; eol++)
                        ; /* do nothing */
        } else
                eol = p;
        if (*p) {
                p += 2;
                for (eol = p + 1; *eol && *eol != '\n'; eol++)
                        ; /* do nothing */
        } else
                eol = p;

-       abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
-       abbrev_len = strlen(abbrev);

        oneline_len = eol - p;
        oneline_len = eol - p;

-       result = xmalloc(abbrev_len + 5 + oneline_len);
-       memcpy(result, abbrev, abbrev_len);
-       memcpy(result + abbrev_len, "... ", 4);
-       memcpy(result + abbrev_len + 4, p, oneline_len);
-       result[abbrev_len + 4 + oneline_len] = '\0';
-       return result;
+
+       out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
+                             strlen("... ") + oneline_len + 1);
+       q = out->parent_label;
+       q = mempcpy(q, "parent of ", strlen("parent of "));
+       out->label = q;
+       q = mempcpy(q, abbrev, abbrev_len);
+       q = mempcpy(q, "... ", strlen("... "));
+       out->subject = q;
+       q = mempcpy(q, p, oneline_len);
+       *q = '\0';
+       return 0;
+}
+
+static void free_message(struct commit_message *msg)
+{
+       free(msg->parent_label);
+       free(msg->reencoded_message);

 }
 
 static char *get_encoding(const char *message)
 }
 
 static char *get_encoding(const char *message)


@@ -248,9 +281,10 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 {
        unsigned char head[20];
        struct commit *base, *next, *parent;
 {
        unsigned char head[20];
        struct commit *base, *next, *parent;

+       const char *next_label;

        int i, index_fd, clean;
        int i, index_fd, clean;

-       char *oneline, *reencoded_message = NULL;
-       const char *message, *encoding;
+       struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+

        char *defmsg = git_pathdup("MERGE_MSG");
        struct merge_options o;
        struct tree *result, *next_tree, *base_tree, *head_tree;
        char *defmsg = git_pathdup("MERGE_MSG");
        struct merge_options o;
        struct tree *result, *next_tree, *base_tree, *head_tree;


@@ -314,14 +348,14 @@ static int revert_or_cherry_pick(int argc, const char **argv)
        else
                parent = commit->parents->item;
 
        else
                parent = commit->parents->item;
 

-       if (!(message = commit->buffer))
-               die ("Cannot get commit message for %s",
-                               sha1_to_hex(commit->object.sha1));
-

        if (parent && parse_commit(parent) < 0)
                die("%s: cannot parse parent commit %s",
                    me, sha1_to_hex(parent->object.sha1));
 
        if (parent && parse_commit(parent) < 0)
                die("%s: cannot parse parent commit %s",
                    me, sha1_to_hex(parent->object.sha1));
 

+       if (get_message(commit->buffer, &msg) != 0)
+               die("Cannot get commit message for %s",
+                               sha1_to_hex(commit->object.sha1));
+

        /*
         * "commit" is an existing commit.  We would want to apply
         * the difference it introduces since its first parent "prev"
        /*
         * "commit" is an existing commit.  We would want to apply
         * the difference it introduces since its first parent "prev"


@@ -332,24 +366,12 @@ static int revert_or_cherry_pick(int argc, const char **argv)
        msg_fd = hold_lock_file_for_update(&msg_file, defmsg,
                                           LOCK_DIE_ON_ERROR);
 
        msg_fd = hold_lock_file_for_update(&msg_file, defmsg,
                                           LOCK_DIE_ON_ERROR);
 

-       encoding = get_encoding(message);
-       if (!encoding)
-               encoding = "UTF-8";
-       if (!git_commit_encoding)
-               git_commit_encoding = "UTF-8";
-       if ((reencoded_message = reencode_string(message,
-                                       git_commit_encoding, encoding)))
-               message = reencoded_message;
-
-       oneline = get_oneline(message);
-

        if (action == REVERT) {
        if (action == REVERT) {

-               char *oneline_body = strchr(oneline, ' ');
-

                base = commit;
                next = parent;
                base = commit;
                next = parent;

+               next_label = msg.parent_label;

                add_to_msg("Revert \"");
                add_to_msg("Revert \"");

-               add_to_msg(oneline_body + 1);
+               add_to_msg(msg.subject);

                add_to_msg("\"\n\nThis reverts commit ");
                add_to_msg(sha1_to_hex(commit->object.sha1));
 
                add_to_msg("\"\n\nThis reverts commit ");
                add_to_msg(sha1_to_hex(commit->object.sha1));
 


@@ -361,8 +383,9 @@ static int revert_or_cherry_pick(int argc, const char **argv)
        } else {
                base = parent;
                next = commit;
        } else {
                base = parent;
                next = commit;

-               set_author_ident_env(message);
-               add_message_to_msg(message);
+               next_label = msg.label;
+               set_author_ident_env(msg.message);
+               add_message_to_msg(msg.message);

                if (no_replay) {
                        add_to_msg("(cherry picked from commit ");
                        add_to_msg(sha1_to_hex(commit->object.sha1));
                if (no_replay) {
                        add_to_msg("(cherry picked from commit ");
                        add_to_msg(sha1_to_hex(commit->object.sha1));


@@ -373,7 +396,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
        read_cache();
        init_merge_options(&o);
        o.branch1 = "HEAD";
        read_cache();
        init_merge_options(&o);
        o.branch1 = "HEAD";

-       o.branch2 = oneline;
+       o.branch2 = next ? next_label : "(empty tree)";

 
        head_tree = parse_tree_indirect(head);
        next_tree = next ? next->tree : empty_tree();
 
        head_tree = parse_tree_indirect(head);
        next_tree = next ? next->tree : empty_tree();


@@ -437,7 +460,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
                args[i] = NULL;
                return execv_git_cmd(args);
        }
                args[i] = NULL;
                return execv_git_cmd(args);
        }

-       free(reencoded_message);
+       free_message(&msg);

        free(defmsg);
 
        return 0;
        free(defmsg);
 
        return 0;

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index e8563560cbb861a38523772ffcf71103deaba64e..6a208173779f744f6b350ee12dd710be157ace6e 100644 (file)
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -138,7 +138,7 @@ test_expect_success 'revert also handles conflicts sanely' '
        a
        =======
        b
        a
        =======
        b

-       >>>>>>> objid picked
+       >>>>>>> parent of objid picked

        EOF
        {
                git checkout picked -- foo &&
        EOF
        {
                git checkout picked -- foo &&


@@ -183,7 +183,7 @@ test_expect_success 'revert conflict, diff3 -m style' '
        c
        =======
        b
        c
        =======
        b

-       >>>>>>> objid picked
+       >>>>>>> parent of objid picked

        EOF
 
        git update-index --refresh &&
        EOF
 
        git update-index --refresh &&