Re: [RFC PATCH v2 1/2] git-merge: add option to format default message using multiple lines
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 9, 2020, 16:20 UTC
- Message-ID
- <xmqqblp51owb.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20200309120722.4987-2-darktemplar@dark-templar-archives.net>
"i.Dark_Templar" <darktemplar@dark-templar-archives.net> writes:
Show 22 quoted lines
> +static int use_multiline = -1;
>
> int fmt_merge_msg_config(const char *key, const char *value, void *cb)
> {
> @@ -32,6 +33,8 @@ int fmt_merge_msg_config(const char *key, const char *value, void *cb)
> merge_log_config = DEFAULT_MERGE_LOG_LEN;
> } else if (!strcmp(key, "merge.branchdesc")) {
> use_branch_desc = git_config_bool(key, value);
> + } else if (!strcmp(key, "merge.usemultiline")) {
> + use_multiline = git_config_int(key, value);
> } else {
> return git_default_config(key, value, cb);
> }
> @@ -413,6 +416,39 @@ static void shortlog(const char *name,
> string_list_clear(&subjects, 0);
> }
>
> +static int fmt_merge_refs_count(void)
> +{
> + int i = 0;
> + int j = 0;
> + int objects_count = 0;Call it "object_count" and drop "j"; that's shorter.
Also, this is a "STATIC" function, an implementation detail of the fmt_merge_msg program. There is no need to repeat that fact by replicating fmt_merge_ in its name to differenciate with other things, unlike the functions that are visible from other places.
Just call it "count_srcs()" or something like that and do not use the word "ref" in its name, as "merging refs" is probably too ancient, limiting and misleading concept---we do not merge "refs". We merge commits (and more recently) tags, and "refs" is merely one of the ways to name these things that are merged.
Show 10 quoted lines
> + for (i = 0; i < srcs.nr; i++) {
> + struct src_data *src_data = srcs.items[i].util;
> +
> + if (src_data->head_status == 1) {
> + ++objects_count;
> + continue;
> + }
> + if (src_data->head_status == 3) {
> + ++objects_count;
> + }This part deserves commenting. The bit #0 of head_status is special in that it signals a pull of the default branch (HEAD) of the remote repository, and it is special because in that case (and only in that case) none of the string lists in src_data is updated, so the number of merged heads is one more than the case where the bit #0 is off.
Show 12 quoted lines
> + for (j = 0; j < src_data->branch.nr; j++) {
> + ++objects_count;
> + }
> + for (j = 0; j < src_data->r_branch.nr; j++) {
> + ++objects_count;
> + }
> + for (j = 0; j < src_data->tag.nr; j++) {
> + ++objects_count;
> + }
> + for (j = 0; j < src_data->generic.nr; j++) {
> + ++objects_count;
> + }Isn't it sufficient to add .nr to object_count? Why increment object_count by one as many times as j counts up from 0 to .nr?
If the merge involves only the remote HEAD, then the lists are all empty, isn't it? I do not see the point of treating the case where head_status is 3 == 1|2 any special, so the following is sufficient as the replacement for the above and should be far easier to follow, no? Or am I missing something subtle in your version?
if (src_data->head_status & 01)
/*
* the set of merged heads includes the
* default branch of the remote, aka HEAD,
* which is not counted in any of the lists
* in src_data.
*/
object_count++; object_count += (src->data->branch.nr +
src->data->r_branch.nr +
src->data->tag.nr +
src->data->generic.nr);By the way, if you read the original code, you may have noticed that our codebase prefers post-increment over pre-increment, when the difference does not matter (in other words, "a = ++b;" is perfectly legit; it is just that we do not say "++b;" when "b++;" would do).
Show 22 quoted lines
> + }
> +
> + return objects_count;
> +}
> +
> static void fmt_merge_msg_title(struct strbuf *out,
> const char *current_branch)
> {
> @@ -467,6 +503,68 @@ static void fmt_merge_msg_title(struct strbuf *out,
> strbuf_addf(out, " into %s\n", current_branch);
> }
>
> +static void fmt_merge_msg_title_multiline(struct strbuf *out,
> + const char *current_branch, int count)
> +{
> + int i = 0;
> + int j = 0;
> +
> + if (!strcmp("master", current_branch))
> + strbuf_addf(out, "Merge %d %s\n", count, (count == 1) ? "commit" : "commits");
> + else
> + strbuf_addf(out, "Merge %d %s into %s\n", count, (count == 1) ? "commit" : "commits", current_branch);Overlong lines.
Show 6 quoted lines
> + strbuf_addch(out, '\n'); > + > + if (count == 1) > + strbuf_addstr(out, "Following commit is merged:\n"); > + else > + strbuf_addstr(out, "Following commits are merged:\n");
The above makes me wonder if you are better off having "if there is only one, do these things, otherwise do these other things" at the top level, i.e.
if (count == 1) {
strbuf_addstr(out, "Merge 1 commit");
if (strcmp(current_branch, "master"))
strbuf_addstr(out, " into %s", current_branch);
strbuf_addstr(out, "\n\nFollowing commit is ...");
} else {
...similarly...
}But stepping back a bit, is there a point in handing count==1 case in this new format in the first place? Why waste the most precious information real estate, which is the title line, to say a lot less informative "merge 1 commit" instead of what topic is merged there?
Also, I am not sure if "Following commits are merged" is a good message for a few reasons.
- Obviously, we allow pulling and merging a signed tag and in that case, we are merging a tag, not a commit.
- When we pull a topic branch and nothing else, the above code would say "Merge 1 commit", but there may probably be more than one commit on the history leading to that commit we are merging into our history.
What actually is happening, instead of "merge 1 commit", is that we are merging one lineage of history, which may have one or more commits. The phrase suitable to call the lineage of history may be "a topic branch", but it may be "a release tag", in which case what you are merging would be the entire history built while developing towards that tag that you have been missing.