Re: [PATCH v5 1/5] fmt_merge_msg: Change fmt_merge_msg API to accept shortlog_len
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:22
Subsystem:
the rest · Maintainer:
Linus Torvalds
Ramkumar Ramachandra wrote:
Update the API of the fmt_merge_msg function to replace the the merge_summary argument with a new shortlog_len argument.
Even more, actually:
quoted hunk ↗ jump to hunk
+++ b/builtin.h@@ -14,9 +14,8 @@ extern const char git_more_info_string[]; extern void list_common_cmds_help(void); extern const char *help_unknown_cmd(const char *cmd); extern void prune_packed_objects(int); -extern int fmt_merge_msg(int merge_summary, struct strbuf *in, - struct strbuf *out); -extern int fmt_merge_msg_shortlog(struct strbuf *in, struct strbuf *out); +extern int fmt_merge_msg(struct strbuf *in, struct strbuf *out, + int merge_title, int shortlog_len);
This combines the public functions. Nice, but probably worth a mention in the log message. The API does not make the sense of the merge_title argument clear: should it be 1 when the caller provides a title or when fmt_merge_msg should provide one? There is not much I can see to do about that; I'd just suggest making it clear in the commit message (so it can be mentioned in the API docs, once they're written :)).
quoted hunk ↗ jump to hunk
@@ -357,9 +354,10 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix) die_errno("could not read input file"); if (message) { strbuf_addstr(&output, message); - ret = fmt_merge_msg_shortlog(&input, &output); + ret = fmt_merge_msg(&input, &output, 0, 20); } else { - ret = fmt_merge_msg(merge_summary, &input, &output); + ret = fmt_merge_msg(&input, &output, 1, + merge_summary ? 20 : 0);
Could be further simplified: if (message) strbuf_addstr(&output, message); ret = fmt_merge_msg(&input, &output, message ? 0 : 1, merge_summary ? 20 : 0); Not sure if that helps or not. A part of me wishes that the constant 20 wouldn't be duplicated so much. Would something like #define DEFAULT_MERGE_SHORTLOG_LEN 20 make sense?
quoted hunk ↗ jump to hunk
+++ b/builtin/merge.c@@ -1014,9 +1014,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix) merge_name(argv[i], &merge_names); if (have_message && option_log) - fmt_merge_msg_shortlog(&merge_names, &merge_msg); + fmt_merge_msg(&merge_names, &merge_msg, 0, 20); else if (!have_message) - fmt_merge_msg(option_log, &merge_names, &merge_msg); + fmt_merge_msg(&merge_names, &merge_msg, 1, + option_log ? 20: 0);
Likewise. The rest looks good. So except for the commit message, Reviewed-by: Jonathan Nieder <redacted> Thanks. Here are the cleanups I mentioned. Except for the #define, they probably don't belong in this patch. ---
diff --git a/builtin.h b/builtin.h
index 7d18106..09b94ea 100644
--- a/builtin.h
+++ b/builtin.h@@ -7,6 +7,8 @@ #include "commit.h" #include "notes.h" +#define DEFAULT_MERGE_LOG_LEN 20 + extern const char git_version_string[]; extern const char git_usage_string[]; extern const char git_more_info_string[];
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 4ed728a..7f06b95 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c@@ -352,13 +352,13 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix) if (strbuf_read(&input, fileno(in), 0) < 0) die_errno("could not read input file"); - if (message) { + + if (message) strbuf_addstr(&output, message); - ret = fmt_merge_msg(&input, &output, 0, 20); - } else { - ret = fmt_merge_msg(&input, &output, 1, - merge_summary ? 20 : 0); - } + ret = fmt_merge_msg(&input, &output, + message ? 0 : 1, + merge_summary ? DEFAULT_MERGE_LOG_LEN : 0); + if (ret) return ret; write_in_full(STDOUT_FILENO, output.buf, output.len);
diff --git a/builtin/merge.c b/builtin/merge.c
index 70ee412..d797853 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c@@ -1000,15 +1000,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix) for (i = 0; i < argc; i++) merge_name(argv[i], &merge_names); - if (have_message && option_log) - fmt_merge_msg(&merge_names, &merge_msg, 0, 20); - else if (!have_message) - fmt_merge_msg(&merge_names, &merge_msg, 1, - option_log ? 20: 0); - - - if (!(have_message && !option_log) && merge_msg.len) - strbuf_setlen(&merge_msg, merge_msg.len-1); + if (!have_message || option_log) { + fmt_merge_msg(&merge_names, &merge_msg, !have_message, + option_log ? DEFAULT_MERGE_LOG_LEN : 0); + if (merge_msg.len) + strbuf_setlen(&merge_msg, merge_msg.len - 1); + } } if (head_invalid || !argc)
--