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

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