[PATCH v2 0/5] nd/maint-branch-desc-doc reroll

[PATCH v2 0/5] nd/maint-branch-desc-doc reroll

[Nguyễn Thái Ngọc Duy]

 - move the comment in 5/5 into the commented code block
 - resolve branch name for "HEAD" in 4/5

Nguyễn Thái Ngọc Duy (5):
  config.txt: a few lines about branch.<name>.description
  branch: delete branch description if it's empty
  t4014: a few more tests on cover letter using branch description
  format-patch: pick up correct branch name from symbolic ref
  format-patch: pick up branch description when no ref is specified

 Documentation/config.txt |  6 ++++++
 builtin/branch.c         |  2 +-
 builtin/log.c            | 33 ++++++++++++++++++++-------------
 t/t4014-format-patch.sh  | 42 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 69 insertions(+), 14 deletions(-)

-- 
1.8.0.rc2.23.g1fb49df

[PATCH v2 1/5] config.txt: a few lines about branch.<name>.description

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bf8f911..ee64846 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -735,6 +735,12 @@ branch.<name>.rebase::
 it unless you understand the implications (see linkgit:git-rebase[1]
 for details).
 
+branch.<name>.description::
+	Branch description, can be edited with
+	`git branch --edit-description`. Branch description is
+	automatically added in the format-patch cover letter or
+	request-pull summary.
+
 browser.<tool>.cmd::
 	Specify the command to invoke the specified browser. The
 	specified command is evaluated in shell with the URLs passed
-- 
1.8.0.rc2.23.g1fb49df

[PATCH v2 2/5] branch: delete branch description if it's empty

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 1ec9c02..873f624 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -725,7 +725,7 @@ static int edit_branch_description(const char *branch_name)
 	stripspace(&buf, 1);
 
 	strbuf_addf(&name, "branch.%s.description", branch_name);
-	status = git_config_set(name.buf, buf.buf);
+	status = git_config_set(name.buf, buf.len ? buf.buf : NULL);
 	strbuf_release(&name);
 	strbuf_release(&buf);
 
-- 
1.8.0.rc2.23.g1fb49df

[PATCH v2 3/5] t4014: a few more tests on cover letter using branch description

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t4014-format-patch.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 16a4ca1..ec9ef9e 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -963,4 +963,25 @@ test_expect_success 'format patch ignores color.ui' '
 	test_cmp expect actual
 '
 
+test_expect_success 'cover letter using branch description (1)' '
+	git checkout rebuild-1 &&
+	test_config branch.rebuild-1.description hello &&
+	git format-patch --stdout --cover-letter master >actual &&
+	grep hello actual >/dev/null
+'
+
+test_expect_success 'cover letter using branch description (2)' '
+	git checkout rebuild-1 &&
+	test_config branch.rebuild-1.description hello &&
+	git format-patch --stdout --cover-letter rebuild-1~2..rebuild-1 >actual &&
+	grep hello actual >/dev/null
+'
+
+test_expect_success 'cover letter using branch description (3)' '
+	git checkout rebuild-1 &&
+	test_config branch.rebuild-1.description hello &&
+	git format-patch --stdout --cover-letter ^master rebuild-1 >actual &&
+	grep hello actual >/dev/null
+'
+
 test_done
-- 
1.8.0.rc2.23.g1fb49df

[PATCH v2 4/5] format-patch: pick up correct branch name from symbolic ref

[Nguyễn Thái Ngọc Duy]

find_branch_name() assumes to take refs/heads/<branch>. But we also
have symbolic refs, such as HEAD, that can point to a valid branch in
refs/heads and do not follow refs/heads/<branch> syntax. Remove the
assumption and apply normal ref resolution. After all it would be
confusing if rev machinery resolves a ref in one way and
find_branch_name() another.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/log.c           | 21 +++++++++------------
 t/t4014-format-patch.sh | 14 ++++++++++++++
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index e7b7db1..039bf67 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1016,8 +1016,8 @@ static char *find_branch_name(struct rev_info *rev)
 {
 	int i, positive = -1;
 	unsigned char branch_sha1[20];
-	struct strbuf buf = STRBUF_INIT;
-	const char *branch;
+	const char *ref;
+	char *full_ref, *branch = NULL;
 
 	for (i = 0; i < rev->cmdline.nr; i++) {
 		if (rev->cmdline.rev[i].flags & UNINTERESTING)
@@ -1029,16 +1029,13 @@ static char *find_branch_name(struct rev_info *rev)
 	}
 	if (positive < 0)
 		return NULL;
-	strbuf_addf(&buf, "refs/heads/%s", rev->cmdline.rev[positive].name);
-	branch = resolve_ref_unsafe(buf.buf, branch_sha1, 1, NULL);
-	if (!branch ||
-	    prefixcmp(branch, "refs/heads/") ||
-	    hashcmp(rev->cmdline.rev[positive].item->sha1, branch_sha1))
-		branch = NULL;
-	strbuf_release(&buf);
-	if (branch)
-		return xstrdup(rev->cmdline.rev[positive].name);
-	return NULL;
+	ref = rev->cmdline.rev[positive].name;
+	if (dwim_ref(ref, strlen(ref), branch_sha1, &full_ref) &&
+	    !prefixcmp(full_ref, "refs/heads/") &&
+	    !hashcmp(rev->cmdline.rev[positive].item->sha1, branch_sha1))
+		branch = xstrdup(full_ref + strlen("refs/heads/"));
+	free(full_ref);
+	return branch;
 }
 
 int cmd_format_patch(int argc, const char **argv, const char *prefix)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index ec9ef9e..903a797 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -984,4 +984,18 @@ test_expect_success 'cover letter using branch description (3)' '
 	grep hello actual >/dev/null
 '
 
+test_expect_success 'cover letter using branch description (4)' '
+	git checkout rebuild-1 &&
+	test_config branch.rebuild-1.description hello &&
+	git format-patch --stdout --cover-letter master.. >actual &&
+	grep hello actual >/dev/null
+'
+
+test_expect_success 'cover letter using branch description (5)' '
+	git checkout rebuild-1 &&
+	test_config branch.rebuild-1.description hello &&
+	git format-patch --stdout --cover-letter -2 HEAD >actual &&
+	grep hello actual >/dev/null
+'
+
 test_done
-- 
1.8.0.rc2.23.g1fb49df

[PATCH v2 5/5] format-patch: pick up branch description when no ref is specified

[Nguyễn Thái Ngọc Duy]

We only try to get branch name in "format-patch origin" case or
similar and not "format-patch -22" where HEAD is automatically
added. Without correct branch name, branch description cannot be
added. Make sure we always get branch name.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/log.c           | 16 +++++++++++++---
 t/t4014-format-patch.sh |  7 +++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 039bf67..81683f6 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1027,12 +1027,22 @@ static char *find_branch_name(struct rev_info *rev)
 		else
 			return NULL;
 	}
-	if (positive < 0)
+	if (positive >= 0)
+		ref = rev->cmdline.rev[positive].name;
+	else if (!rev->cmdline.nr && rev->pending.nr == 1 &&
+		 !strcmp(rev->pending.objects[0].name, "HEAD"))
+		/*
+		 * No actual ref from command line, but "HEAD" from
+		 * rev->def was added in setup_revisions()
+		 * e.g. format-patch --cover-letter -12
+		 */
+		ref = "HEAD";
+	else
 		return NULL;
-	ref = rev->cmdline.rev[positive].name;
 	if (dwim_ref(ref, strlen(ref), branch_sha1, &full_ref) &&
 	    !prefixcmp(full_ref, "refs/heads/") &&
-	    !hashcmp(rev->cmdline.rev[positive].item->sha1, branch_sha1))
+	    (positive < 0 ||
+	     !hashcmp(rev->cmdline.rev[positive].item->sha1, branch_sha1)))
 		branch = xstrdup(full_ref + strlen("refs/heads/"));
 	free(full_ref);
 	return branch;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 903a797..7e52941 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -998,4 +998,11 @@ test_expect_success 'cover letter using branch description (5)' '
 	grep hello actual >/dev/null
 '
 
+test_expect_success 'cover letter using branch description (6)' '
+	git checkout rebuild-1 &&
+	test_config branch.rebuild-1.description hello &&
+	git format-patch --stdout --cover-letter -2 >actual &&
+	grep hello actual >/dev/null
+'
+
 test_done
-- 
1.8.0.rc2.23.g1fb49df

Re: [PATCH v2 5/5] format-patch: pick up branch description when no ref is specified

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> We only try to get branch name in "format-patch origin" case or
> similar and not "format-patch -22" where HEAD is automatically
> added. Without correct branch name, branch description cannot be
> added. Make sure we always get branch name.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/log.c           | 16 +++++++++++++---
>  t/t4014-format-patch.sh |  7 +++++++
>  2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 039bf67..81683f6 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1027,12 +1027,22 @@ static char *find_branch_name(struct rev_info *rev)
>  		else
>  			return NULL;
>  	}
> -	if (positive < 0)
> +	if (positive >= 0)
> +		ref = rev->cmdline.rev[positive].name;
> +	else if (!rev->cmdline.nr && rev->pending.nr == 1 &&
> +		 !strcmp(rev->pending.objects[0].name, "HEAD"))
> +		/*
> +		 * No actual ref from command line, but "HEAD" from
> +		 * rev->def was added in setup_revisions()
> +		 * e.g. format-patch --cover-letter -12
> +		 */
> +		ref = "HEAD";
> +	else
>  		return NULL;
> -	ref = rev->cmdline.rev[positive].name;
>  	if (dwim_ref(ref, strlen(ref), branch_sha1, &full_ref) &&
>  	    !prefixcmp(full_ref, "refs/heads/") &&
> -	    !hashcmp(rev->cmdline.rev[positive].item->sha1, branch_sha1))
> +	    (positive < 0 ||
> +	     !hashcmp(rev->cmdline.rev[positive].item->sha1, branch_sha1)))

This hashcmp() is to make sure that the tip we _guessed_ with
dwim_ref() that the user meant matches what we are going to use, so
I do not think disabling the check is a good idea.

You could (and I think you should) do something like this:

	if (0 <= positive) {
        	ref = rev->cmdline.rev[positive].name;
                tip_sha1 = rev->cmdline.rev[positive].item->sha1;
	} else if (... defaulted to implied HEAD? ...) {
		ref = "HEAD";
                tip_sha1 = rev->pending.objects[0].item->sha1;
	} else {
		return NULL;
	}

        if (dwim_ref(...) && !prefixcmp(full_ref, "refs/heads/") &&
            !hashcmp(tip_sha1, branch_sha1))

to preserve that safety instead.

[PATCH v3 5/5] format-patch: pick up branch description when no ref is specified

[Nguyễn Thái Ngọc Duy]

We only try to get branch name in "format-patch origin" case or
similar and not "format-patch -22" where HEAD is automatically
added. Without correct branch name, branch description cannot be
added. Make sure we always get branch name.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Thu, Jan 3, 2013 at 10:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
 > You could (and I think you should) do something like this:
 >
 >         if (0 <= positive) {
 >                 ref = rev->cmdline.rev[positive].name;
 >                 tip_sha1 = rev->cmdline.rev[positive].item->sha1;
 >         } else if (... defaulted to implied HEAD? ...) {
 >                 ref = "HEAD";
 >                 tip_sha1 = rev->pending.objects[0].item->sha1;
 >         } else {
 >                 return NULL;
 >         }
 >
 >         if (dwim_ref(...) && !prefixcmp(full_ref, "refs/heads/") &&
 >             !hashcmp(tip_sha1, branch_sha1))
 >
 > to preserve that safety instead.

 Good idea. Done.

 builtin/log.c           | 18 +++++++++++++++---
 t/t4014-format-patch.sh |  7 +++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 039bf67..72a368a 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1016,6 +1016,7 @@ static char *find_branch_name(struct rev_info *rev)
 {
 	int i, positive = -1;
 	unsigned char branch_sha1[20];
+	const unsigned char *tip_sha1;
 	const char *ref;
 	char *full_ref, *branch = NULL;
 
@@ -1027,12 +1028,23 @@ static char *find_branch_name(struct rev_info *rev)
 		else
 			return NULL;
 	}
-	if (positive < 0)
+	if (0 <= positive) {
+		ref = rev->cmdline.rev[positive].name;
+		tip_sha1 = rev->cmdline.rev[positive].item->sha1;
+	} else if (!rev->cmdline.nr && rev->pending.nr == 1 &&
+		   !strcmp(rev->pending.objects[0].name, "HEAD")) {
+		/*
+		 * No actual ref from command line, but "HEAD" from
+		 * rev->def was added in setup_revisions()
+		 * e.g. format-patch --cover-letter -12
+		 */
+		ref = "HEAD";
+		tip_sha1 = rev->pending.objects[0].item->sha1;
+	} else
 		return NULL;
-	ref = rev->cmdline.rev[positive].name;
 	if (dwim_ref(ref, strlen(ref), branch_sha1, &full_ref) &&
 	    !prefixcmp(full_ref, "refs/heads/") &&
-	    !hashcmp(rev->cmdline.rev[positive].item->sha1, branch_sha1))
+	    !hashcmp(tip_sha1, branch_sha1))
 		branch = xstrdup(full_ref + strlen("refs/heads/"));
 	free(full_ref);
 	return branch;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 903a797..7e52941 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -998,4 +998,11 @@ test_expect_success 'cover letter using branch description (5)' '
 	grep hello actual >/dev/null
 '
 
+test_expect_success 'cover letter using branch description (6)' '
+	git checkout rebuild-1 &&
+	test_config branch.rebuild-1.description hello &&
+	git format-patch --stdout --cover-letter -2 >actual &&
+	grep hello actual >/dev/null
+'
+
 test_done
-- 
1.8.0.rc2.23.g1fb49df