Phillip Wood <phillip.w...@talktalk.net> writes:

> From: Phillip Wood <phillip.w...@dunelm.org.uk>
>
> Signed-off-by: Phillip Wood <phillip.w...@dunelm.org.uk>
> ---

This seems to do a lot more than just moving code, most notably, it
uses setenv() to affect what happens in any subprocesses we may
spawn, and it is unclear if it was verified that this patch is free
of unwanted consequences due to that change (and any others I may
have missed while reading this patch, if any).

I suspect that it would be sufficient to make update_head() helper
function take the reflog action message as another parameter
instead to fix the above, but there may be other reasons why you
chose to do it this way---I cannot read it in your empty log
message, though.

I will not give line-by-line style nitpick but in general we do not
leave a SP between function name and the open parenthesis that
starts its argument list.  New code in this patch seems to use
mixture of styles.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 
> 0b8c1ef6f57cfed328d12255e6834adb4bda4137..497778ba2c02afdd4a337969a27ca781e8389040
>  100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1578,13 +1578,11 @@ int cmd_commit(int argc, const char **argv, const 
> char *prefix)
>       struct strbuf sb = STRBUF_INIT;
>       struct strbuf author_ident = STRBUF_INIT;
>       const char *index_file, *reflog_msg;
> -     char *nl;
>       struct object_id oid;
>       struct commit_list *parents = NULL;
>       struct stat statbuf;
>       struct commit *current_head = NULL;
>       struct commit_extra_header *extra = NULL;
> -     struct ref_transaction *transaction;
>       struct strbuf err = STRBUF_INIT;
>  
>       if (argc == 2 && !strcmp(argv[1], "-h"))
> @@ -1625,10 +1623,10 @@ int cmd_commit(int argc, const char **argv, const 
> char *prefix)
>       reflog_msg = getenv("GIT_REFLOG_ACTION");
>       if (!current_head) {
>               if (!reflog_msg)
> -                     reflog_msg = "commit (initial)";
> +                     setenv ("GIT_REFLOG_ACTION", "commit (initial)", 1);
>       } else if (amend) {
>               if (!reflog_msg)
> -                     reflog_msg = "commit (amend)";
> +                     setenv("GIT_REFLOG_ACTION", "commit (amend)", 1);
>               parents = copy_commit_list(current_head->parents);
>       } else if (whence == FROM_MERGE) {
>               struct strbuf m = STRBUF_INIT;
> @@ -1637,7 +1635,7 @@ int cmd_commit(int argc, const char **argv, const char 
> *prefix)
>               struct commit_list **pptr = &parents;
>  
>               if (!reflog_msg)
> -                     reflog_msg = "commit (merge)";
> +                     setenv("GIT_REFLOG_ACTION", "commit (merge)", 1);
>               pptr = commit_list_append(current_head, pptr);
>               fp = xfopen(git_path_merge_head(), "r");
>               while (strbuf_getline_lf(&m, fp) != EOF) {
> @@ -1660,9 +1658,9 @@ int cmd_commit(int argc, const char **argv, const char 
> *prefix)
>                       parents = reduce_heads(parents);
>       } else {
>               if (!reflog_msg)
> -                     reflog_msg = (whence == FROM_CHERRY_PICK)
> -                                     ? "commit (cherry-pick)"
> -                                     : "commit";
> +                     setenv("GIT_REFLOG_ACTION", (whence == FROM_CHERRY_PICK)
> +                                             ? "commit (cherry-pick)"
> +                                             : "commit", 1);
>               commit_list_insert(current_head, &parents);
>       }
>  
> @@ -1707,25 +1705,10 @@ int cmd_commit(int argc, const char **argv, const 
> char *prefix)
>       strbuf_release(&author_ident);
>       free_commit_extra_headers(extra);
>  
> -     nl = strchr(sb.buf, '\n');
> -     if (nl)
> -             strbuf_setlen(&sb, nl + 1 - sb.buf);
> -     else
> -             strbuf_addch(&sb, '\n');
> -     strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
> -     strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
> -
> -     transaction = ref_transaction_begin(&err);
> -     if (!transaction ||
> -         ref_transaction_update(transaction, "HEAD", oid.hash,
> -                                current_head
> -                                ? current_head->object.oid.hash : null_sha1,
> -                                0, sb.buf, &err) ||
> -         ref_transaction_commit(transaction, &err)) {
> +     if (update_head (current_head, &oid, &sb, &err)) {
>               rollback_index_files();
>               die("%s", err.buf);
>       }
> -     ref_transaction_free(transaction);
>  
>       unlink(git_path_cherry_pick_head());
>       unlink(git_path_revert_head());
> diff --git a/sequencer.c b/sequencer.c
> index 
> 319208afb3de36c97b6c62d4ecf6e641245e7a54..917ad4a16216b30adb2c2c9650217926d8db8ba7
>  100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1,10 +1,10 @@
>  #include "cache.h"
>  #include "config.h"
>  #include "lockfile.h"
> -#include "sequencer.h"
>  #include "dir.h"
>  #include "object.h"
>  #include "commit.h"
> +#include "sequencer.h"
>  #include "tag.h"
>  #include "run-command.h"
>  #include "exec_cmd.h"
> @@ -750,6 +750,43 @@ int template_untouched(const struct strbuf *sb, const 
> char *template_file,
>       return rest_is_empty(sb, start - sb->buf);
>  }
>  
> +int update_head(const struct commit *old_head, const struct object_id 
> *new_head,
> +             const struct strbuf *msg, struct strbuf *err)
> +{
> +     struct ref_transaction *transaction;
> +     struct strbuf sb = STRBUF_INIT;
> +     const char *nl, *reflog_msg;
> +     int ret = 0;
> +
> +     reflog_msg = getenv("GIT_REFLOG_ACTION");
> +     if (!reflog_msg)
> +             reflog_msg="";
> +
> +     nl = strchr(msg->buf, '\n');
> +     if (nl) {
> +             strbuf_add(&sb, msg->buf, nl + 1 - msg->buf);
> +     } else {
> +             strbuf_addbuf(&sb, msg);
> +             strbuf_addch(&sb, '\n');
> +     }
> +     strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
> +     strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
> +
> +     transaction = ref_transaction_begin(err);
> +     if (!transaction ||
> +         ref_transaction_update(transaction, "HEAD", new_head->hash,
> +                                old_head
> +                                ? old_head->object.oid.hash : null_sha1,
> +                                0, sb.buf, err) ||
> +         ref_transaction_commit(transaction, err)) {
> +             ret = -1;
> +     }
> +     ref_transaction_free(transaction);
> +     strbuf_release(&sb);
> +
> +     return ret;
> +}
> +
>  static int is_original_commit_empty(struct commit *commit)
>  {
>       const struct object_id *ptree_oid;
> diff --git a/sequencer.h b/sequencer.h
> index 
> dd071cfcd82d165bd23726814b74cbf3384e1a17..87edf40e5274d59f48d5af57678100ea220d2c8a
>  100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -60,4 +60,6 @@ enum cleanup_mode {
>  int message_is_empty(const struct strbuf *sb, enum cleanup_mode 
> cleanup_mode);
>  int template_untouched(const struct strbuf *sb, const char *template_file,
>                      enum cleanup_mode cleanup_mode);
> +int update_head(const struct commit *old_head, const struct object_id 
> *new_head,
> +             const struct strbuf *msg, struct strbuf *err);
>  #endif

Reply via email to