[PATCH] fmt-merge-msg: use branch.$name.description

This teaches "merge --log" and fmt-merge-msg to use branch description
information when merging a local topic branch into the mainline. The
description goes between the branch name label and the list of commit
titles.

The refactoring to share the common configuration parsing between
merge and fmt-merge-msg needs to be made into a separate patch.

Signed-off-by: Junio C Hamano <redacted>
---
 * This is a follow-up to the branch.$name.description series that has
   been queued in 'pu' (jc/request-pull-show-head-4). The two commands
   join the family of commands that are aware of the branch description,
   i.e. format-patch (cover letter), request-pull.

 Makefile                |    1 +
 builtin/fmt-merge-msg.c |   63 ++++++++++++++++++++++++++++++++++++++--------
 builtin/merge.c         |   16 +++++------
 fmt-merge-msg.h         |    6 ++++
 4 files changed, 66 insertions(+), 20 deletions(-)
 create mode 100644 fmt-merge-msg.h

diff --git a/Makefile b/Makefile
index 8d6d451..b499049 100644
--- a/Makefile
+++ b/Makefile

@@ -527,6 +527,7 @@ LIB_H += diffcore.h
 LIB_H += diff.h
 LIB_H += dir.h
 LIB_H += exec_cmd.h
+LIB_H += fmt-merge-msg.h
 LIB_H += fsck.h
 LIB_H += gettext.h
 LIB_H += git-compat-util.h

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 7581632..21efd25 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c

@@ -5,6 +5,8 @@
 #include "revision.h"
 #include "tag.h"
 #include "string-list.h"
+#include "branch.h"
+#include "fmt-merge-msg.h"
 
 static const char * const fmt_merge_msg_usage[] = {
 	"git fmt-merge-msg [-m <message>] [--log[=<n>]|--no-log] [--file <file>]",

@@ -12,8 +14,9 @@ static const char * const fmt_merge_msg_usage[] = {
 };
 
 static int shortlog_len;
+static int use_branch_desc;
 
-static int fmt_merge_msg_config(const char *key, const char *value, void *cb)
+int fmt_merge_msg_config(const char *key, const char *value, void *cb)
 {
 	if (!strcmp(key, "merge.log") || !strcmp(key, "merge.summary")) {
 		int is_bool;

@@ -22,6 +25,8 @@ static int fmt_merge_msg_config(const char *key, const char *value, void *cb)
 			return error("%s: negative length %s", key, value);
 		if (is_bool && shortlog_len)
 			shortlog_len = DEFAULT_MERGE_LOG_LEN;
+	} else if (!strcmp(key, "merge.branchdesc")) {
+		use_branch_desc = git_config_bool(key, value);
 	}
 	return 0;
 }

@@ -31,6 +36,11 @@ struct src_data {
 	int head_status;
 };
 
+struct origin_data {
+	unsigned char sha1[20];
+	int is_local_branch:1;
+};
+
 static void init_src_data(struct src_data *data)
 {
 	data->branch.strdup_strings = 1;

@@ -45,7 +55,7 @@ static struct string_list origins = STRING_LIST_INIT_DUP;
 static int handle_line(char *line)
 {
 	int i, len = strlen(line);
-	unsigned char *sha1;
+	struct origin_data *origin_data;
 	char *src, *origin;
 	struct src_data *src_data;
 	struct string_list_item *item;

@@ -61,11 +71,13 @@ static int handle_line(char *line)
 		return 2;
 
 	line[40] = 0;
-	sha1 = xmalloc(20);
-	i = get_sha1(line, sha1);
+	origin_data = xcalloc(1, sizeof(struct origin_data));
+	i = get_sha1(line, origin_data->sha1);
 	line[40] = '\t';
-	if (i)
+	if (i) {
+		free(origin_data);
 		return 3;
+	}
 
 	if (line[len - 1] == '\n')
 		line[len - 1] = 0;

@@ -93,6 +105,7 @@ static int handle_line(char *line)
 		origin = src;
 		src_data->head_status |= 1;
 	} else if (!prefixcmp(line, "branch ")) {
+		origin_data->is_local_branch = 1;
 		origin = line + 7;
 		string_list_append(&src_data->branch, origin);
 		src_data->head_status |= 2;

@@ -119,7 +132,9 @@ static int handle_line(char *line)
 		sprintf(new_origin, "%s of %s", origin, src);
 		origin = new_origin;
 	}
-	string_list_append(&origins, origin)->util = sha1;
+	if (strcmp(".", src))
+		origin_data->is_local_branch = 0;
+	string_list_append(&origins, origin)->util = origin_data;
 	return 0;
 }
 

@@ -140,9 +155,30 @@ static void print_joined(const char *singular, const char *plural,
 	}
 }
 
-static void shortlog(const char *name, unsigned char *sha1,
-		struct commit *head, struct rev_info *rev, int limit,
-		struct strbuf *out)
+static void add_branch_desc(struct strbuf *out, const char *name)
+{
+	struct strbuf desc = STRBUF_INIT;
+
+	if (!read_branch_desc(&desc, name)) {
+		const char *bp = desc.buf;
+		while (*bp) {
+			const char *ep = strchrnul(bp, '\n');
+			if (*ep)
+				ep++;
+			strbuf_addf(out, "  : %.*s", (int)(ep - bp), bp);
+			bp = ep;
+		}
+		if (out->buf[out->len - 1] != '\n')
+			strbuf_addch(out, '\n');
+	}
+	strbuf_release(&desc);
+}
+
+static void shortlog(const char *name,
+		     struct origin_data *origin_data,
+		     struct commit *head,
+		     struct rev_info *rev, int limit,
+		     struct strbuf *out)
 {
 	int i, count = 0;
 	struct commit *commit;

@@ -150,6 +186,7 @@ static void shortlog(const char *name, unsigned char *sha1,
 	struct string_list subjects = STRING_LIST_INIT_DUP;
 	int flags = UNINTERESTING | TREESAME | SEEN | SHOWN | ADDED;
 	struct strbuf sb = STRBUF_INIT;
+	const unsigned char *sha1 = origin_data->sha1;
 
 	branch = deref_tag(parse_object(sha1), sha1_to_hex(sha1), 40);
 	if (!branch || branch->type != OBJ_COMMIT)

@@ -188,6 +225,9 @@ static void shortlog(const char *name, unsigned char *sha1,
 	else
 		strbuf_addf(out, "\n* %s:\n", name);
 
+	if (origin_data->is_local_branch && use_branch_desc)
+		add_branch_desc(out, name);
+
 	for (i = 0; i < subjects.nr; i++)
 		if (i >= limit)
 			strbuf_addf(out, "  ...\n");

@@ -303,8 +343,9 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
 			strbuf_addch(out, '\n');
 
 		for (i = 0; i < origins.nr; i++)
-			shortlog(origins.items[i].string, origins.items[i].util,
-					head, &rev, shortlog_len, out);
+			shortlog(origins.items[i].string,
+				 origins.items[i].util,
+				 head, &rev, shortlog_len, out);
 	}
 	return 0;
 }

diff --git a/builtin/merge.c b/builtin/merge.c
index ab4077f..fe56aad 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -26,6 +26,7 @@
 #include "merge-recursive.h"
 #include "resolve-undo.h"
 #include "remote.h"
+#include "fmt-merge-msg.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)

@@ -525,6 +526,8 @@ static void parse_branch_merge_options(char *bmo)
 
 static int git_merge_config(const char *k, const char *v, void *cb)
 {
+	int status;
+
 	if (branch && !prefixcmp(k, "branch.") &&
 		!prefixcmp(k + 7, branch) &&
 		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {

@@ -541,15 +544,7 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		return git_config_string(&pull_octopus, k, v);
 	else if (!strcmp(k, "merge.renormalize"))
 		option_renormalize = git_config_bool(k, v);
-	else if (!strcmp(k, "merge.log") || !strcmp(k, "merge.summary")) {
-		int is_bool;
-		shortlog_len = git_config_bool_or_int(k, v, &is_bool);
-		if (!is_bool && shortlog_len < 0)
-			return error(_("%s: negative length %s"), k, v);
-		if (is_bool && shortlog_len)
-			shortlog_len = DEFAULT_MERGE_LOG_LEN;
-		return 0;
-	} else if (!strcmp(k, "merge.ff")) {
+	else if (!strcmp(k, "merge.ff")) {
 		int boolval = git_config_maybe_bool(k, v);
 		if (0 <= boolval) {
 			allow_fast_forward = boolval;

@@ -562,6 +557,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		default_to_upstream = git_config_bool(k, v);
 		return 0;
 	}
+	status = fmt_merge_msg_config(k, v, cb);
+	if (status)
+		return status;
 	return git_diff_ui_config(k, v, cb);
 }
 

diff --git a/fmt-merge-msg.h b/fmt-merge-msg.h
new file mode 100644
index 0000000..9217fdb
--- /dev/null
+++ b/fmt-merge-msg.h

@@ -0,0 +1,6 @@
+#ifndef FMT_MERGE_MSG_H
+#define FMT_MERGE_MSG_H
+
+extern int fmt_merge_msg_config(const char *key, const char *value, void *cb);
+
+#endif /* FMT_MERGE_MSG_H */

-- 
1.7.7.138.g7f41b6

Junio C Hamano venit, vidit, dixit 07.10.2011 08:57:

This teaches "merge --log" and fmt-merge-msg to use branch description
information when merging a local topic branch into the mainline. The
description goes between the branch name label and the list of commit
titles.

The refactoring to share the common configuration parsing between
merge and fmt-merge-msg needs to be made into a separate patch.

Signed-off-by: Junio C Hamano <redacted>
---
 * This is a follow-up to the branch.$name.description series that has
   been queued in 'pu' (jc/request-pull-show-head-4). The two commands
   join the family of commands that are aware of the branch description,
   i.e. format-patch (cover letter), request-pull.

Uhm, I tried to start a discussion there about whether we really want
config based branch descriptions, together with suggestions and actual
series for notes based descriptions, notes based format-patch
cover-letter, notes based branch output. Making fmt-merge-msg use that
is a no-brainer.

Do we want an implementation race, or could we please reach some
consensus about the direction first? (Not many have spoken up yet.)

config based is so non-distributed, local in nature.

Michael J Gruber wrote:

config based is so non-distributed, local in nature.

I don't follow.  Wouldn't a protocol change be enough to fix that, by
sharing branch descriptions analagously to how refs themselves are
shared?

Jonathan Nieder venit, vidit, dixit 07.10.2011 11:16:

Michael J Gruber wrote:

quoted

config based is so non-distributed, local in nature.

I don't follow.  Wouldn't a protocol change be enough to fix that, by
sharing branch descriptions analagously to how refs themselves are
shared?

I'd be surprised if we changed the protocol just to be able to share
some descriptions, when we have everything we need for sharing refs.
Also note that config is non-versioned etc.

But my main point here is that we should discuss the pros and cons of
each approach in context (the context of the original thread), and I
haven't heard many pros and cons for either.

Michael

Michael J Gruber wrote:

But my main point here is that we should discuss the pros and cons of
each approach in context (the context of the original thread), and I
haven't heard many pros and cons for either.

Ok, here are some thoughts (you can file them under the "pro" or "con"
column according to your taste):

Branch names are local.

The same tip commit can belong to multiple branches, which would make
using the tip commit as a key for the branch description problematic.
I don't think git notes are a good fit for this purpose.

I personally would prefer my branch descriptions to be non-versioned,
though I realize that is a matter of taste.

Some other information about a branch (such as which upstream branch
it pulls from) is stored in the [branch "<branchname>"] section of the
git configuration, so it makes a certain kind of sense to put the
branch description there, too.  On the other hand, the possibility of
a [branch "<branchname>"] section in ~/.gitconfig has always been
strange, and this is no exception.

I'd be surprised if we changed the protocol just to be able to share
some descriptions, when we have everything we need for sharing refs.

The impact of such a protocol change would not have to be as dramatic
as you seem to be suggesting.  Virtual hosts and peeled ref values
were added as backward-compatible extensions to the reference
discovery protocol (see Documentation/technical/pack-protocol.txt)
which can be taken as examples for inspiration.

Here's some discussion of the implementation based on branchname-keyed
notes that you mentioned [1].  It is nice to have multiple designs
available for trying out, and I hope experience using each reveals
potential improvements in the other.

[1] http://thread.gmane.org/gmane.comp.version-control.git/181953/focus=182000

Jonathan Nieder venit, vidit, dixit 07.10.2011 12:06:

Michael J Gruber wrote:

quoted

But my main point here is that we should discuss the pros and cons of
each approach in context (the context of the original thread), and I
haven't heard many pros and cons for either.

Ok, here are some thoughts (you can file them under the "pro" or "con"
column according to your taste):

Branch names are local.

You can pull a branch only if you know its name, or using a wildcard
refspec. In both cases you know the translation from what is local on
the other side to your side. In the vast majority it will be the simple
refs/heads/foo -> refs/remotes/bar/foo mapping.

The same tip commit can belong to multiple branches, which would make
using the tip commit as a key for the branch description problematic.

Sure, tip commit is a no-go.

That's why I propose notes tacked onto refnames.

I don't think git notes are a good fit for this purpose.

Why not?

I really haven't seen any convincing argument against yet. The details
(note attached to refname object, or literal refanmes in the notes tree
as per Jeff) should be discussed further, of course, but if branch
descriptions aren't "notesy" then I don't know what is.

Alternatively, one could store the description in a blob and refer to
that directly, of course. I.e., have

refs/description/foo

point to a blob whose content is the description of the ref

ref/foo

That would be unversioned, and one could decide more easily which
descriptions to share. (A notes tree you either push or don't.)

But it still has the advantage of being easily pushable and shareable.
Heck, config is config and not metadata ;)

I personally would prefer my branch descriptions to be non-versioned,
though I realize that is a matter of taste.

Do you prefer you commit notes to be non-versioned? Probably, but still
they are, and you don't need to care. Just don't run log on your notes ref.

Some other information about a branch (such as which upstream branch
it pulls from) is stored in the [branch "<branchname>"] section of the
git configuration, so it makes a certain kind of sense to put the
branch description there, too.  On the other hand, the possibility of
a [branch "<branchname>"] section in ~/.gitconfig has always been
strange, and this is no exception.

Note that most use cases for branch descriptions that have been
described so far (format-patch cover-letter, merge-msg, pull-request
message) are about distributing that information. Not very local.

quoted

I'd be surprised if we changed the protocol just to be able to share
some descriptions, when we have everything we need for sharing refs.

The impact of such a protocol change would not have to be as dramatic
as you seem to be suggesting.  Virtual hosts and peeled ref values
were added as backward-compatible extensions to the reference
discovery protocol (see Documentation/technical/pack-protocol.txt)
which can be taken as examples for inspiration.

I don't see how a protocol change could solve the perceived issue with
localality of name, and indeed what it could solve that can't be solved
with existing methods.

Here's some discussion of the implementation based on branchname-keyed
notes that you mentioned [1].  It is nice to have multiple designs
available for trying out, and I hope experience using each reveals
potential improvements in the other.

[1] http://thread.gmane.org/gmane.comp.version-control.git/181953/focus=182000

Funny to point me at a thread I participated in, when I'm saying this
thread should have continued there ;)

Michael

Michael J Gruber wrote:

Jonathan Nieder venit, vidit, dixit 07.10.2011 12:06:

quoted

[1] http://thread.gmane.org/gmane.comp.version-control.git/181953/focus=182000

Funny to point me at a thread I participated in, when I'm saying this
thread should have continued there ;)

Ah, sorry, I wasn't trying to initiate a debate.  I provided the
pointer to save time for others who haven't looked up the thread
you were referring to yet.

I am surprised that you seem to have missed what I meant by "branch
names are local".  I meant that what I call "master" is likely to be
very different from what you call "master".  Yes, we share commits and
fetch them from each other, and I might even _decide_ that what I call
"master" will be similar to what you call "master".  Luckily, in
practice people don't seem to feel constrained to decide so, and there
are many workflows where my "master" is not your "master" and does not
even contain commits that would be acceptable for your "master".

This matters, a lot, because there is no easy way to partition a
namespace of names descriptive for humans without a central authority
to negotiate conflicts.

Hi again,

Michael J Gruber wrote:

I really haven't seen any convincing argument against yet. The details
(note attached to refname object, or literal refanmes in the notes tree
as per Jeff) should be discussed further, of course, but if branch
descriptions aren't "notesy" then I don't know what is.

As mentioned before, I don't want to debate how Junio should spend his
time (better for each person to provide relevant information and
improvements to help out or to spend time on the alternatives one is
interested in), but as a general question, this statement looked
interesting to me.

"git notes" has a funny name, but deep down, as you know, it's about
attaching additional versioned text to commit objects without changing
their names.  Branch descriptions are not per commit object (and as a
mapping, the _keys_ are not shared), and personally I don't think they
should be versioned any more than the branch names are.

I wanted to emphasize this because

	"git notes" is not the best tool for all annotations!

This ends this public service announcement.

Alternatively, one could store the description in a blob and refer to
that directly, of course. I.e., have

refs/description/foo

point to a blob whose content is the description of the ref

ref/foo

Sure, that would be a sane alternative design.  It has the advantage
of having the pumbing for fetching and pulling already set up, as you
mention.  The only disadvantages I know of are

 - "git branch -m" and "git remote rename" don't know about it yet
 - there's not one flat file you can edit to run a search/replace on
   all branch descriptions

and those aren't very serious problems.

quoted

I personally would prefer my branch descriptions to be non-versioned,
though I realize that is a matter of taste.

Do you prefer you commit notes to be non-versioned?

No, I like them versioned.  If I didn't, why wouldn't I have sent a
patch to change that?  Maybe some day there will be a "git notes log"
tool to track the history of a note, taking changes in fanout into
account.

Hope that clarifies a little.