Re: [RFC PATCH v2 1/2] git-merge: add option to format default message using multiple lines

[Junio C Hamano <gitster@pobox.com>]

"i.Dark_Templar" <darktemplar@dark-templar-archives.net> writes:

> +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.


> +	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.

> +		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).

> +	}
> +
> +	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.

> +	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.