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

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

[Junio C Hamano]

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 <gitster@pobox.com>
---
 * 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

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

[Michael J Gruber]

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 <gitster@pobox.com>
> ---
>  * 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.

>  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 */

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

[Jonathan Nieder]

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?

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

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 07.10.2011 11:16:
> 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?

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

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

[Jonathan Nieder]

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

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

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 07.10.2011 12:06:
> 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.

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.

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

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

[Jonathan Nieder]

Michael J Gruber wrote:
> Jonathan Nieder venit, vidit, dixit 07.10.2011 12:06:

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

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I am surprised that you seem to have missed what I meant by "branch
> names are local"....
> 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.

I think we are in total agreement.  The branch names are local, but the
users may want to annotate to describe _the history_ they intend to build
on it.

Various ways to convey the description when the end product (i.e. the
history built on it) is shiped were outlined in the series, so that the
shipper can help the receiver understand the history. The information in
the annotation (i.e. the _value_ of branch.$name.description) is something
the shipper wants to share with the receiver, but the mapping between the
local name of the branch the shipper used to build that history (i.e. the
key "$name" in branch.$name.description) is immaterial in the end result.

I do not think there is much more for me to add to this topic, as I think
you covered all the important bases already.

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

[Jonathan Nieder]

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.

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

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 07.10.2011 21:59:
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
>> I am surprised that you seem to have missed what I meant by "branch
>> names are local"....
>> 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.
> 
> I think we are in total agreement.  The branch names are local, but the
> users may want to annotate to describe _the history_ they intend to build
> on it.
> 
> Various ways to convey the description when the end product (i.e. the
> history built on it) is shiped were outlined in the series, so that the
> shipper can help the receiver understand the history. The information in
> the annotation (i.e. the _value_ of branch.$name.description) is something
> the shipper wants to share with the receiver, but the mapping between the
> local name of the branch the shipper used to build that history (i.e. the
> key "$name" in branch.$name.description) is immaterial in the end result.

It just seems we judge the "local" aspect completely differently. The
point that I don't get is: How to share a branch without sharing a
branch name? You cannot. Of course you may "change the name" during the
push, or during a fetch, and at that point you know both and can map the
description.

Note that I'm not tied to the notes implementation. But versioned branch
descriptions would be nice for several purposes, for example series
cover letters, or descriptions on long lived feature branches. And I
don't see how else to have versioned descriptions.

Also, for transporting descriptions via config, you have to have an
updated git on the server side, whereas anything object/ref based would
work now, right?

Michael

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

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

If refs/descriptions/foo were to point at a commit object and its commit
log message is used to store the description you could make it versioned.

A history that records forest of descriptions where its commit contains a
tree whose leaves are branch names is slightly better than notes based
approach in that it does not have to violate "notes are for objects"
design principle, but it shares the same "branch names are local" issue as
your "lone refs/description/foo describes 'foo' and 'foo' only".

In addition, it shares with the notes based approach that exchanging a
description on a single branch will inevitably pull in descriptions of all
the other branches you happen to have in the forest, which was one of the
reasons that recording "push -s" certificates in notes tree failed whether
the v2 or v3 approach was used.

You should be able to make a few changes to jc/request-pull-show-head-4 to
move the description to such a "refs/desc/$name" from completely local
"branch.$name.description". The machinery to edit the description is
localized to edit_branch_description() introduced in b7200e8 (branch:
teach --edit-description option, 2011-09-20), and the machinery to read
the description is localized to read_branch_desc() in 6f9a332 (branch: add
read_branch_desc() helper function, 2011-09-21); the remainder of the
series could be used unmodified.

But it remains that any of these approaches assume branch names are
universal. Unlike other systems, what we call branches do not have their
own identity, so if you really want to go that route (and we _might_ need
to in the longer term, but I am not convinced at this point yet), you
would first need to define how that local namespace would look like, how
people interact with it, etc. It might be just the matter of declaring a
convention e.g. "Among people who meet at this central repository,
everybody must map the branches identically to their local branch
namespace, and all sharing must go through the central repository", and
calling a tuple <central repository URL, branch name in that repository>
with a name that cannot be confused with "branch" (so "remote branch" is
out), such as "(development) track".

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

[Jakub Narebski]

Junio C Hamano <gitster@pobox.com> writes:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
> > 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 remains that any of these approaches assume branch names are
> universal. Unlike other systems, what we call branches do not have their
> own identity, so if you really want to go that route (and we _might_ need
> to in the longer term, but I am not convinced at this point yet), you
> would first need to define how that local namespace would look like, how
> people interact with it, etc. It might be just the matter of declaring a
> convention e.g. "Among people who meet at this central repository,
> everybody must map the branches identically to their local branch
> namespace, and all sharing must go through the central repository", and
> calling a tuple <central repository URL, branch name in that repository>
> with a name that cannot be confused with "branch" (so "remote branch" is
> out), such as "(development) track".

Well, git could by default imply that 'refs/heads/*:refs/remotes/foo/*'
implies 'refs/description/*:refs/remote-descriptions/foo/*'...

...one more argument for hierarchical remote-tracking refs namespace,
i.e. 'refs/remotes/foo/refs/heads/*', and not current 'refs/remotes/foo/*'

Just my 3 eurocents^W groszy.
-- 
Jakub Narębski