[PATCH] bash completion: offer more options for gitk

[PATCH] bash completion: offer more options for gitk

[Thomas Rast]

Add some more git-log options to the gitk completion.  Not all of them
make sense, unfortunately.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

I was simply too lazy to keep typing --simplify-by-decoration.

I made two passes over the git-log manpage and simply selected all
options that seemed to make sense, _seemed_ to work, and that didn't
mangle the history view in horrible ways (such as disconnecting it
into little pieces).  It does handle --left-right correctly, very
nice!


 contrib/completion/git-completion.bash |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a7a10c0..cedca6d 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1808,7 +1808,15 @@ _gitk ()
 	fi
 	case "$cur" in
 	--*)
-		__gitcomp "--not --all $merge"
+		__gitcomp "
+			--not --all $merge
+			--branches --tags --remotes
+			--first-parent
+			--dense --sparse --full-history
+			--simplify-merges --simplify-by-decoration
+			--since= --after= --until= --before=
+			--left-right
+			"
 		return
 		;;
 	esac
-- 
1.6.2.rc0.293.g947001

Re: [PATCH] bash completion: offer more options for gitk

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> Add some more git-log options to the gitk completion.  Not all of them
> make sense, unfortunately.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>
> I was simply too lazy to keep typing --simplify-by-decoration.
>
> I made two passes over the git-log manpage and simply selected all
> options that seemed to make sense, _seemed_ to work, and that didn't
> mangle the history view in horrible ways (such as disconnecting it
> into little pieces).  It does handle --left-right correctly, very
> nice!

The --left-right support in gitk was done in tandem with the low-level
support.

> +		__gitcomp "
> +			--not --all $merge
> +			--branches --tags --remotes
> +			--first-parent
> +			--dense --sparse --full-history
> +			--simplify-merges --simplify-by-decoration
> +			--since= --after= --until= --before=
> +			--left-right
> +			"

Many options you add here are useful for git-log and not present in its
completion, but as you point out not all git-log options necessarily make
sense for gitk.  I think it would make sense to introduce an extra
variable $__git_log_basic_options that holds the basic ones that can be in
both, and add the ones that are specific to gitk or git-log in their own
completion functions.  I suspect gitk's addition will be nil, while
git-log would add --graph, --walk-reflogs and --no-merges to the basic
set.

>  		return
>  		;;
>  	esac
> -- 
> 1.6.2.rc0.293.g947001

Re: [PATCH] bash completion: offer more options for gitk

[Santi Béjar]

2009/2/15 Junio C Hamano <gitster@pobox.com>:
> Many options you add here are useful for git-log and not present in its
> completion, but as you point out not all git-log options necessarily make
> sense for gitk.  I think it would make sense to introduce an extra
> variable $__git_log_basic_options that holds the basic ones that can be in
> both, and add the ones that are specific to gitk or git-log in their own
> completion functions.  I suspect gitk's addition will be nil, while
> git-log would add --graph, --walk-reflogs and --no-merges to the basic
> set.

I sometimes use the --no-merges with gitk, normally within a range
(the last 'next' update or so).

Santi

[PATCH v2 0/2] bash completion: more options for gitk/log/shortlog

[Thomas Rast]

Santi Béjar wrote:
> 2009/2/15 Junio C Hamano <gitster@pobox.com>:
> > Many options you add here are useful for git-log and not present in its
> > completion, but as you point out not all git-log options necessarily make
> > sense for gitk.  I think it would make sense to introduce an extra
> > variable $__git_log_basic_options that holds the basic ones that can be in
> > both, and add the ones that are specific to gitk or git-log in their own
> > completion functions.  I suspect gitk's addition will be nil, while
> > git-log would add --graph, --walk-reflogs and --no-merges to the basic
> > set.

Right.  Somehow git patches have a tendency to grow in scope; while I
was trying to refactor them in good ways, I couldn't help notice that
shortlog falls in the same category.

(Probably there's another log-like command that I missed?)

> I sometimes use the --no-merges with gitk, normally within a range
> (the last 'next' update or so).

For me that falls in the "mangles history in horrible ways" category,
but since you use it, I put it in.

2/2 is new; I figured since gitk has this bit of code already, why not
have git-log do it the same way?


Thomas Rast (2):
  bash completion: refactor common log, shortlog and gitk options
  bash completion: only show 'log --merge' if merging

 contrib/completion/git-completion.bash |   56 ++++++++++++++++++++++---------
 1 files changed, 40 insertions(+), 16 deletions(-)

Re: [PATCH v2 0/2] bash completion: more options for gitk/log/shortlog

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 274 bytes --]

Unrelated note regarding --thread usage:

Wouldn't it be better if format-patch made it so that the cover
letter, if any, is the (only) reply to the --in-reply-to, and all
actual patches are replies to the cover letter?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[RFC PATCH] format-patch: thread as reply to cover letter even with in-reply-to

[Thomas Rast]

Currently, format-patch --thread --cover-letter --in-reply-to $parent
makes all mails, including the cover letter, a reply to $parent.
However, we would want the reader to consider the cover letter above
all the patches.

This changes the semantics so that only the cover letter is a reply to
$parent, while all the patches are formatted as replies to the cover
letter.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Thomas Rast wrote:
> Unrelated note regarding --thread usage:
> 
> Wouldn't it be better if format-patch made it so that the cover
> letter, if any, is the (only) reply to the --in-reply-to, and all
> actual patches are replies to the cover letter?

Maybe like this?


 builtin-log.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 2ae39af..ec11f95 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -1031,12 +1031,22 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			/* Have we already had a message ID? */
 			if (rev.message_id) {
 				/*
-				 * If we've got the ID to be a reply
-				 * to, discard the current ID;
-				 * otherwise, make everything a reply
-				 * to that.
+				 * Without --cover-letter and
+				 * --in-reply-to, make every mail a
+				 * reply to the one before.
+				 *
+				 * With --in-reply-to but no
+				 * --cover-letter, make every mail a
+				 * reply to the <reply-to>.
+				 *
+				 * With --cover-letter, make every
+				 * mail but the cover letter a reply
+				 * to the cover letter.  The cover
+				 * letter is a reply to the
+				 * --in-reply-to, if specified.
 				 */
-				if (rev.ref_message_id)
+				if (rev.ref_message_id
+				    && (!cover_letter || rev.nr > 1))
 					free(rev.message_id);
 				else
 					rev.ref_message_id = rev.message_id;
-- 
1.6.2.rc0.328.g2c702

Re: [RFC PATCH] format-patch: thread as reply to cover letter even with in-reply-to

[Jay Soffian]

On Mon, Feb 16, 2009 at 2:00 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> Currently, format-patch --thread --cover-letter --in-reply-to $parent
> makes all mails, including the cover letter, a reply to $parent.
> However, we would want the reader to consider the cover letter above
> all the patches.
>
> This changes the semantics so that only the cover letter is a reply to
> $parent, while all the patches are formatted as replies to the cover
> letter.

I think this change may be okay, but I think to go with it the
cover-letter and all the patches should have a "References:" header
with the message-id given by --in-reply-to.

RFC 2822 says:

   The "In-Reply-To:" and "References:" fields are used when creating a
   reply to a message.  They hold the message identifier of the original
   message and the message identifiers of other messages (for example,
   in the case of a reply to a message which was itself a reply).  The
   "In-Reply-To:" field may be used to identify the message (or
   messages) to which the new message is a reply, while the
   "References:" field may be used to identify a "thread" of
   conversation.

j.

Re: [RFC PATCH] format-patch: thread as reply to cover letter even with in-reply-to

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

Jay Soffian wrote:
> I think this change may be okay, but I think to go with it the
> cover-letter and all the patches should have a "References:" header
> with the message-id given by --in-reply-to.

That's a completely separate issue.  I'm only proposing to change
what is formatted as a reply to what, the In-Reply-To and References
formatting is handled by the existing code.  That being said...

> RFC 2822 says:
> 
>    The "In-Reply-To:" and "References:" fields are used when creating a
>    reply to a message.  They hold the message identifier of the original
>    message and the message identifiers of other messages (for example,
>    in the case of a reply to a message which was itself a reply).  The
>    "In-Reply-To:" field may be used to identify the message (or
>    messages) to which the new message is a reply, while the
>    "References:" field may be used to identify a "thread" of
>    conversation.

The References formatting can't satisfy the following requirement, two
paragraphs further down in the RFC,

   The "References:" field will contain the contents of the parent's
   "References:" field (if any) followed by the contents of the parent's
   "Message-ID:" field (if any).

because it doesn't have access to the mail being replied to.  It
merely sets References to the same as In-Reply-To.

(Note that I'm just guessing this from behaviour I can observe, I
haven't actually read the code for that part.)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [RFC PATCH] format-patch: thread as reply to cover letter even with in-reply-to

[Jakub Narebski]

Thomas Rast <trast@student.ethz.ch> writes:
> Jay Soffian wrote:

> > I think this change may be okay, but I think to go with it the
> > cover-letter and all the patches should have a "References:" header
> > with the message-id given by --in-reply-to.
> 
> That's a completely separate issue.  I'm only proposing to change
> what is formatted as a reply to what, the In-Reply-To and References
> formatting is handled by the existing code.

Perhaps a follow-up patch then?

>  That being said...
> 
> > RFC 2822 says:
> > 
> >    The "In-Reply-To:" and "References:" fields are used when creating a
> >    reply to a message.  They hold the message identifier of the original
> >    message and the message identifiers of other messages (for example,
> >    in the case of a reply to a message which was itself a reply).  The
> >    "In-Reply-To:" field may be used to identify the message (or
> >    messages) to which the new message is a reply, while the
> >    "References:" field may be used to identify a "thread" of
> >    conversation.
> 
> The References formatting can't satisfy the following requirement, two
> paragraphs further down in the RFC,
> 
>    The "References:" field will contain the contents of the parent's
>    "References:" field (if any) followed by the contents of the parent's
>    "Message-ID:" field (if any).
> 
> because it doesn't have access to the mail being replied to.  It
> merely sets References to the same as In-Reply-To.

I think "References:" field can be shortened, and although IIRC it is
recommended to leave first ancestor, two separators, and three last
ancestors in shortened "References:" header, it would be better if
patches had both '--in-reply-to' message id AND cover letter id in
"References:" header.  That would make for safer threading...

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [RFC PATCH] format-patch: thread as reply to cover letter even with in-reply-to

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

Jakub Narebski wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
> > Jay Soffian wrote:
> 
> > > I think this change may be okay, but I think to go with it the
> > > cover-letter and all the patches should have a "References:" header
> > > with the message-id given by --in-reply-to.
> > 
> > That's a completely separate issue.  I'm only proposing to change
> > what is formatted as a reply to what, the In-Reply-To and References
> > formatting is handled by the existing code.
[...]
> patches had both '--in-reply-to' message id AND cover letter id in
> "References:" header.  That would make for safer threading...

Oh, now I see what you meant.

It seems that right now, send-email appends the right references for
--thread mode, and format-patch only does the one-reference case.
I'll try and make a patch for that.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 0/4] format-patch --cover-letter --in-reply-to

[Thomas Rast]

Lemma 1: Any task, no matter how trivial, expands to at least three
         patches.

Turns out there were tests, but they never worked.  In any case it
should now know how to properly format References, and also knows
about deep and shallow threading.

I formatted this series with

  git format-patch origin/master..t/thread-reply-to-cover \
    --cover-letter --thread \
    --in-reply-to='<200902170027.09406.trast@student.ethz.ch>'


Thomas Rast (4):
  format-patch: threading test reactivation
  format-patch: track several references
  format-patch: thread as reply to cover letter even with in-reply-to
  format-patch: support deep threading

 Documentation/config.txt           |   10 ++
 Documentation/git-format-patch.txt |   10 ++-
 builtin-log.c                      |   65 ++++++++--
 log-tree.c                         |   11 +-
 revision.h                         |    2 +-
 t/t4014-format-patch.sh            |  265 ++++++++++++++++++++++++++++++------
 6 files changed, 308 insertions(+), 55 deletions(-)

[PATCH 1/4] format-patch: threading test reactivation

[Thomas Rast]

t4014 tests format-patch --thread since 7d812145, but the tests were
ineffective right from the start at least for bash and dash.  The
loops of the form

  for ...; do something || break; done

introduced by 7d812145 and 5d02294 always exit with status 0, even if
'something' failed, because 'break' returns 0 unless there was no loop
to break.

We take a rather different approach that uses an admittedly heinous
inline Perl script to mangle all interesting information into a format
that is invariant between runs.  We can then test the full patch
sequence in one go (with --stdout), doing away with the loop problem.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/t4014-format-patch.sh |  144 ++++++++++++++++++++++++++++++++++-------------
 1 files changed, 104 insertions(+), 40 deletions(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index f045898..345e6de 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -138,56 +138,120 @@ test_expect_success 'multiple files' '
 	ls patches/0001-Side-changes-1.patch patches/0002-Side-changes-2.patch patches/0003-Side-changes-3-with-n-backslash-n-in-it.patch
 '
 
-test_expect_success 'thread' '
+check_threading () {
+	expect="$1" &&
+	shift &&
+	(git format-patch --stdout "$@"; echo $? > status.out) |
+	# Prints everything between the Message-ID and In-Reply-To,
+	# and replaces all Message-ID-lookalikes by a sequence number
+	perl -ne '
+		if (/^(message-id|references|in-reply-to)/i) {
+			$printing = 1;
+		} elsif (/^\S/) {
+			$printing = 0;
+		}
+		if ($printing) {
+			$h{$1}=$i++ if (/<([^>]+)>/ and !exists $h{$1});
+			for $k (keys %h) {s/$k/$h{$k}/};
+			print;
+		}
+		print "---\n" if /^From /i;
+	' > actual &&
+	test 0 = "$(cat status.out)" &&
+	test_cmp "$expect" actual
+}
+
+cat >> expect.no-threading <<EOF
+---
+---
+---
+EOF
 
-	rm -rf patches/ &&
+test_expect_success 'no threading' '
 	git checkout side &&
-	git format-patch --thread -o patches/ master &&
-	FIRST_MID=$(grep "Message-Id:" patches/0001-* | sed "s/^[^<]*\(<[^>]*>\).*$/\1/") &&
-	for i in patches/0002-* patches/0003-*
-	do
-	  grep "References: $FIRST_MID" $i &&
-	  grep "In-Reply-To: $FIRST_MID" $i || break
-	done
+	check_threading expect.no-threading master
 '
 
-test_expect_success 'thread in-reply-to' '
+cat > expect.thread <<EOF
+---
+Message-Id: <0>
+---
+Message-Id: <1>
+In-Reply-To: <0>
+References: <0>
+---
+Message-Id: <2>
+In-Reply-To: <0>
+References: <0>
+EOF
 
-	rm -rf patches/ &&
-	git checkout side &&
-	git format-patch --in-reply-to="<test.message>" --thread -o patches/ master &&
-	FIRST_MID="<test.message>" &&
-	for i in patches/*
-	do
-	  grep "References: $FIRST_MID" $i &&
-	  grep "In-Reply-To: $FIRST_MID" $i || break
-	done
+test_expect_success 'thread' '
+	check_threading expect.thread --thread master
 '
 
-test_expect_success 'thread cover-letter' '
+cat > expect.in-reply-to <<EOF
+---
+Message-Id: <0>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <2>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <3>
+In-Reply-To: <1>
+References: <1>
+EOF
 
-	rm -rf patches/ &&
-	git checkout side &&
-	git format-patch --cover-letter --thread -o patches/ master &&
-	FIRST_MID=$(grep "Message-Id:" patches/0000-* | sed "s/^[^<]*\(<[^>]*>\).*$/\1/") &&
-	for i in patches/0001-* patches/0002-* patches/0003-*
-	do
-	  grep "References: $FIRST_MID" $i &&
-	  grep "In-Reply-To: $FIRST_MID" $i || break
-	done
+test_expect_success 'thread in-reply-to' '
+	check_threading expect.in-reply-to --in-reply-to="<test.message>" \
+		--thread master
 '
 
-test_expect_success 'thread cover-letter in-reply-to' '
+cat > expect.cover-letter <<EOF
+---
+Message-Id: <0>
+---
+Message-Id: <1>
+In-Reply-To: <0>
+References: <0>
+---
+Message-Id: <2>
+In-Reply-To: <0>
+References: <0>
+---
+Message-Id: <3>
+In-Reply-To: <0>
+References: <0>
+EOF
 
-	rm -rf patches/ &&
-	git checkout side &&
-	git format-patch --cover-letter --in-reply-to="<test.message>" --thread -o patches/ master &&
-	FIRST_MID="<test.message>" &&
-	for i in patches/*
-	do
-	  grep "References: $FIRST_MID" $i &&
-	  grep "In-Reply-To: $FIRST_MID" $i || break
-	done
+test_expect_success 'thread cover-letter' '
+	check_threading expect.cover-letter --cover-letter --thread master
+'
+
+cat > expect.cl-irt <<EOF
+---
+Message-Id: <0>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <2>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <3>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <4>
+In-Reply-To: <1>
+References: <1>
+EOF
+
+test_expect_success 'thread cover-letter in-reply-to' '
+	check_threading expect.cl-irt --cover-letter \
+		--in-reply-to="<test.message>" --thread master
 '
 
 test_expect_success 'excessive subject' '
-- 
1.6.2.rc1.295.g1e29

[PATCH 2/4] format-patch: track several references

[Thomas Rast]

Currently, format-patch can only track a single reference (the
In-Reply-To:) for each mail.  To ensure proper threading, we should
list all known references for every mail.

Change the rev_info.ref_message_id field to a string_list, so that we
can append references at will, and change the output formatting
routines to print all of them in the References: header.  The last
entry in the list is implicitly assumed to be the In-Reply-To:, which
gives output consistent with RFC 2822:

   The "References:" field will contain the contents of the parent's
   "References:" field (if any) followed by the contents of the
   parent's "Message-ID:" field (if any).

Note that this is just preparatory work; nothing uses it yet, so all
"References:" fields in the output are still only one deep.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 builtin-log.c |   14 ++++++++++----
 log-tree.c    |   11 ++++++++---
 revision.h    |    2 +-
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 2ae39af..b188575 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -17,6 +17,7 @@
 #include "run-command.h"
 #include "shortlog.h"
 #include "remote.h"
+#include "string-list.h"
 
 /* Set a default date-time format for git log ("log.date" config variable) */
 static const char *default_date_mode = NULL;
@@ -1011,8 +1012,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		numbered = 1;
 	if (numbered)
 		rev.total = total + start_number - 1;
-	if (in_reply_to)
-		rev.ref_message_id = clean_message_id(in_reply_to);
+	if (in_reply_to || thread || cover_letter)
+		rev.ref_message_id = xcalloc(1, sizeof(struct string_list));
+	if (in_reply_to) {
+		const char *msgid = clean_message_id(in_reply_to);
+		string_list_append(msgid, rev.ref_message_id);
+	}
 	if (cover_letter) {
 		if (thread)
 			gen_message_id(&rev, "cover");
@@ -1036,10 +1041,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 				 * otherwise, make everything a reply
 				 * to that.
 				 */
-				if (rev.ref_message_id)
+				if (rev.ref_message_id->nr > 0)
 					free(rev.message_id);
 				else
-					rev.ref_message_id = rev.message_id;
+					string_list_append(rev.message_id,
+							   rev.ref_message_id);
 			}
 			gen_message_id(&rev, sha1_to_hex(commit->object.sha1));
 		}
diff --git a/log-tree.c b/log-tree.c
index 84a74e5..72d97b8 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -6,6 +6,7 @@
 #include "log-tree.h"
 #include "reflog-walk.h"
 #include "refs.h"
+#include "string-list.h"
 
 struct decoration name_decoration = { "object names" };
 
@@ -211,9 +212,13 @@ void log_write_email_headers(struct rev_info *opt, const char *name,
 		printf("Message-Id: <%s>\n", opt->message_id);
 		graph_show_oneline(opt->graph);
 	}
-	if (opt->ref_message_id) {
-		printf("In-Reply-To: <%s>\nReferences: <%s>\n",
-		       opt->ref_message_id, opt->ref_message_id);
+	if (opt->ref_message_id && opt->ref_message_id->nr > 0) {
+		int i, n;
+		n = opt->ref_message_id->nr;
+		printf("In-Reply-To: <%s>\n", opt->ref_message_id->items[n-1].string);
+		for (i = 0; i < n; i++)
+			printf("%s<%s>\n", (i > 0 ? "\t" : "References: "),
+			       opt->ref_message_id->items[i].string);
 		graph_show_oneline(opt->graph);
 	}
 	if (opt->mime_boundary) {
diff --git a/revision.h b/revision.h
index 7cf8487..01dc74e 100644
--- a/revision.h
+++ b/revision.h
@@ -89,7 +89,7 @@ struct rev_info {
 	int		nr, total;
 	const char	*mime_boundary;
 	char		*message_id;
-	const char	*ref_message_id;
+	struct string_list *ref_message_id;
 	const char	*add_signoff;
 	const char	*extra_headers;
 	const char	*log_reencode;
-- 
1.6.2.rc1.295.g1e29

Re: [PATCH 2/4] format-patch: track several references

[Daniel Barkalow]

On Thu, 19 Feb 2009, Thomas Rast wrote:

> Currently, format-patch can only track a single reference (the
> In-Reply-To:) for each mail.  To ensure proper threading, we should
> list all known references for every mail.
> 
> Change the rev_info.ref_message_id field to a string_list

It would be nice to change the field name to "ref_message_ids", which is 
more likely to suggest that it's a list of ids, rather than a single id 
stored strangely.

Aside from that, parts 2-4 look good to me (I'm not up on reviewing shell 
code).

	-Daniel
*This .sig left intentionally blank*

[PATCH v2 next 0/4] format-patch --cover-letter --in-reply-to

[Thomas Rast]

Daniel Barkalow wrote:
> On Thu, 19 Feb 2009, Thomas Rast wrote:
> 
> > Currently, format-patch can only track a single reference (the
> > In-Reply-To:) for each mail.  To ensure proper threading, we should
> > list all known references for every mail.
> > 
> > Change the rev_info.ref_message_id field to a string_list
> 
> It would be nice to change the field name to "ref_message_ids", which is 
> more likely to suggest that it's a list of ids, rather than a single id 
> stored strangely.

Good point, I changed that.

> Aside from that, parts 2-4 look good to me (I'm not up on reviewing shell 
> code).

Thanks.

I also noticed that there is a textual conflict with 0db5260 (Enable
setting attach as the default in .gitconfig for git-format-patch.,
2009-02-12), which is already in 'next'.  So I rebased the series to
'next'.



Thomas Rast (4):
  format-patch: threading test reactivation
  format-patch: track several references
  format-patch: thread as reply to cover letter even with in-reply-to
  format-patch: support deep threading

 Documentation/config.txt           |   10 ++
 Documentation/git-format-patch.txt |   10 ++-
 builtin-log.c                      |   66 ++++++++--
 log-tree.c                         |   11 +-
 revision.h                         |    2 +-
 t/t4014-format-patch.sh            |  265 ++++++++++++++++++++++++++++++------
 6 files changed, 308 insertions(+), 56 deletions(-)

[PATCH v2 next 1/4] format-patch: threading test reactivation

[Thomas Rast]

t4014 tests format-patch --thread since 7d812145, but the tests were
ineffective right from the start at least for bash and dash.  The
loops of the form

  for ...; do something || break; done

introduced by 7d812145 and 5d02294 always exit with status 0, even if
'something' failed, because 'break' returns 0 unless there was no loop
to break.

We take a rather different approach that uses an admittedly heinous
inline Perl script to mangle all interesting information into a format
that is invariant between runs.  We can then test the full patch
sequence in one go (with --stdout), doing away with the loop problem.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Same as v1.


 t/t4014-format-patch.sh |  144 ++++++++++++++++++++++++++++++++++-------------
 1 files changed, 104 insertions(+), 40 deletions(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index f045898..345e6de 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -138,56 +138,120 @@ test_expect_success 'multiple files' '
 	ls patches/0001-Side-changes-1.patch patches/0002-Side-changes-2.patch patches/0003-Side-changes-3-with-n-backslash-n-in-it.patch
 '
 
-test_expect_success 'thread' '
+check_threading () {
+	expect="$1" &&
+	shift &&
+	(git format-patch --stdout "$@"; echo $? > status.out) |
+	# Prints everything between the Message-ID and In-Reply-To,
+	# and replaces all Message-ID-lookalikes by a sequence number
+	perl -ne '
+		if (/^(message-id|references|in-reply-to)/i) {
+			$printing = 1;
+		} elsif (/^\S/) {
+			$printing = 0;
+		}
+		if ($printing) {
+			$h{$1}=$i++ if (/<([^>]+)>/ and !exists $h{$1});
+			for $k (keys %h) {s/$k/$h{$k}/};
+			print;
+		}
+		print "---\n" if /^From /i;
+	' > actual &&
+	test 0 = "$(cat status.out)" &&
+	test_cmp "$expect" actual
+}
+
+cat >> expect.no-threading <<EOF
+---
+---
+---
+EOF
 
-	rm -rf patches/ &&
+test_expect_success 'no threading' '
 	git checkout side &&
-	git format-patch --thread -o patches/ master &&
-	FIRST_MID=$(grep "Message-Id:" patches/0001-* | sed "s/^[^<]*\(<[^>]*>\).*$/\1/") &&
-	for i in patches/0002-* patches/0003-*
-	do
-	  grep "References: $FIRST_MID" $i &&
-	  grep "In-Reply-To: $FIRST_MID" $i || break
-	done
+	check_threading expect.no-threading master
 '
 
-test_expect_success 'thread in-reply-to' '
+cat > expect.thread <<EOF
+---
+Message-Id: <0>
+---
+Message-Id: <1>
+In-Reply-To: <0>
+References: <0>
+---
+Message-Id: <2>
+In-Reply-To: <0>
+References: <0>
+EOF
 
-	rm -rf patches/ &&
-	git checkout side &&
-	git format-patch --in-reply-to="<test.message>" --thread -o patches/ master &&
-	FIRST_MID="<test.message>" &&
-	for i in patches/*
-	do
-	  grep "References: $FIRST_MID" $i &&
-	  grep "In-Reply-To: $FIRST_MID" $i || break
-	done
+test_expect_success 'thread' '
+	check_threading expect.thread --thread master
 '
 
-test_expect_success 'thread cover-letter' '
+cat > expect.in-reply-to <<EOF
+---
+Message-Id: <0>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <2>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <3>
+In-Reply-To: <1>
+References: <1>
+EOF
 
-	rm -rf patches/ &&
-	git checkout side &&
-	git format-patch --cover-letter --thread -o patches/ master &&
-	FIRST_MID=$(grep "Message-Id:" patches/0000-* | sed "s/^[^<]*\(<[^>]*>\).*$/\1/") &&
-	for i in patches/0001-* patches/0002-* patches/0003-*
-	do
-	  grep "References: $FIRST_MID" $i &&
-	  grep "In-Reply-To: $FIRST_MID" $i || break
-	done
+test_expect_success 'thread in-reply-to' '
+	check_threading expect.in-reply-to --in-reply-to="<test.message>" \
+		--thread master
 '
 
-test_expect_success 'thread cover-letter in-reply-to' '
+cat > expect.cover-letter <<EOF
+---
+Message-Id: <0>
+---
+Message-Id: <1>
+In-Reply-To: <0>
+References: <0>
+---
+Message-Id: <2>
+In-Reply-To: <0>
+References: <0>
+---
+Message-Id: <3>
+In-Reply-To: <0>
+References: <0>
+EOF
 
-	rm -rf patches/ &&
-	git checkout side &&
-	git format-patch --cover-letter --in-reply-to="<test.message>" --thread -o patches/ master &&
-	FIRST_MID="<test.message>" &&
-	for i in patches/*
-	do
-	  grep "References: $FIRST_MID" $i &&
-	  grep "In-Reply-To: $FIRST_MID" $i || break
-	done
+test_expect_success 'thread cover-letter' '
+	check_threading expect.cover-letter --cover-letter --thread master
+'
+
+cat > expect.cl-irt <<EOF
+---
+Message-Id: <0>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <2>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <3>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <4>
+In-Reply-To: <1>
+References: <1>
+EOF
+
+test_expect_success 'thread cover-letter in-reply-to' '
+	check_threading expect.cl-irt --cover-letter \
+		--in-reply-to="<test.message>" --thread master
 '
 
 test_expect_success 'excessive subject' '
-- 
1.6.2.rc1.310.ga3b4a

[PATCH v2 next 2/4] format-patch: track several references

[Thomas Rast]

Currently, format-patch can only track a single reference (the
In-Reply-To:) for each mail.  To ensure proper threading, we should
list all known references for every mail.

Replace the rev_info.ref_message_id field by a string_list
ref_message_ids, so that we can append references at will, and change
the output formatting routines to print all of them in the References:
header.  The last entry in the list is implicitly assumed to be the
In-Reply-To:, which gives output consistent with RFC 2822:

   The "References:" field will contain the contents of the parent's
   "References:" field (if any) followed by the contents of the
   parent's "Message-ID:" field (if any).

Note that this is just preparatory work; nothing uses it yet, so all
"References:" fields in the output are still only one deep.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Changed 'ref_message_id' to 'ref_message_ids' as per Daniel's
suggestion.  Adjusted the commit message accordingly.

 builtin-log.c |   14 ++++++++++----
 log-tree.c    |   11 ++++++++---
 revision.h    |    2 +-
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 8549028..1d7cf2f 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -17,6 +17,7 @@
 #include "run-command.h"
 #include "shortlog.h"
 #include "remote.h"
+#include "string-list.h"
 
 /* Set a default date-time format for git log ("log.date" config variable) */
 static const char *default_date_mode = NULL;
@@ -1030,8 +1031,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		numbered = 1;
 	if (numbered)
 		rev.total = total + start_number - 1;
-	if (in_reply_to)
-		rev.ref_message_id = clean_message_id(in_reply_to);
+	if (in_reply_to || thread || cover_letter)
+		rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
+	if (in_reply_to) {
+		const char *msgid = clean_message_id(in_reply_to);
+		string_list_append(msgid, rev.ref_message_ids);
+	}
 	if (cover_letter) {
 		if (thread)
 			gen_message_id(&rev, "cover");
@@ -1055,10 +1060,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 				 * otherwise, make everything a reply
 				 * to that.
 				 */
-				if (rev.ref_message_id)
+				if (rev.ref_message_ids->nr > 0)
 					free(rev.message_id);
 				else
-					rev.ref_message_id = rev.message_id;
+					string_list_append(rev.message_id,
+							   rev.ref_message_ids);
 			}
 			gen_message_id(&rev, sha1_to_hex(commit->object.sha1));
 		}
diff --git a/log-tree.c b/log-tree.c
index 84a74e5..a315ebb 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -6,6 +6,7 @@
 #include "log-tree.h"
 #include "reflog-walk.h"
 #include "refs.h"
+#include "string-list.h"
 
 struct decoration name_decoration = { "object names" };
 
@@ -211,9 +212,13 @@ void log_write_email_headers(struct rev_info *opt, const char *name,
 		printf("Message-Id: <%s>\n", opt->message_id);
 		graph_show_oneline(opt->graph);
 	}
-	if (opt->ref_message_id) {
-		printf("In-Reply-To: <%s>\nReferences: <%s>\n",
-		       opt->ref_message_id, opt->ref_message_id);
+	if (opt->ref_message_ids && opt->ref_message_ids->nr > 0) {
+		int i, n;
+		n = opt->ref_message_ids->nr;
+		printf("In-Reply-To: <%s>\n", opt->ref_message_ids->items[n-1].string);
+		for (i = 0; i < n; i++)
+			printf("%s<%s>\n", (i > 0 ? "\t" : "References: "),
+			       opt->ref_message_ids->items[i].string);
 		graph_show_oneline(opt->graph);
 	}
 	if (opt->mime_boundary) {
diff --git a/revision.h b/revision.h
index 7cf8487..8c0a417 100644
--- a/revision.h
+++ b/revision.h
@@ -89,7 +89,7 @@ struct rev_info {
 	int		nr, total;
 	const char	*mime_boundary;
 	char		*message_id;
-	const char	*ref_message_id;
+	struct string_list *ref_message_ids;
 	const char	*add_signoff;
 	const char	*extra_headers;
 	const char	*log_reencode;
-- 
1.6.2.rc1.310.ga3b4a

[PATCH v2 next 3/4] format-patch: thread as reply to cover letter even with in-reply-to

[Thomas Rast]

Currently, format-patch --thread --cover-letter --in-reply-to $parent
makes all mails, including the cover letter, a reply to $parent.
However, we would want the reader to consider the cover letter above
all the patches.

This changes the semantics so that only the cover letter is a reply to
$parent, while all the patches are formatted as replies to the cover
letter.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Same as v1.

 builtin-log.c           |   20 +++++++++++++++-----
 t/t4014-format-patch.sh |    9 ++++++---
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 1d7cf2f..2dc6c48 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -1055,12 +1055,22 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			/* Have we already had a message ID? */
 			if (rev.message_id) {
 				/*
-				 * If we've got the ID to be a reply
-				 * to, discard the current ID;
-				 * otherwise, make everything a reply
-				 * to that.
+				 * Without --cover-letter and
+				 * --in-reply-to, make every mail a
+				 * reply to the one before.
+				 *
+				 * With --in-reply-to but no
+				 * --cover-letter, make every mail a
+				 * reply to the <reply-to>.
+				 *
+				 * With --cover-letter, make every
+				 * mail but the cover letter a reply
+				 * to the cover letter.  The cover
+				 * letter is a reply to the
+				 * --in-reply-to, if specified.
 				 */
-				if (rev.ref_message_ids->nr > 0)
+				if (rev.ref_message_ids->nr > 0
+				    && (!cover_letter || rev.nr > 1))
 					free(rev.message_id);
 				else
 					string_list_append(rev.message_id,
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 345e6de..8b970c3 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -237,16 +237,19 @@ In-Reply-To: <1>
 References: <1>
 ---
 Message-Id: <2>
-In-Reply-To: <1>
+In-Reply-To: <0>
 References: <1>
+	<0>
 ---
 Message-Id: <3>
-In-Reply-To: <1>
+In-Reply-To: <0>
 References: <1>
+	<0>
 ---
 Message-Id: <4>
-In-Reply-To: <1>
+In-Reply-To: <0>
 References: <1>
+	<0>
 EOF
 
 test_expect_success 'thread cover-letter in-reply-to' '
-- 
1.6.2.rc1.310.ga3b4a

[PATCH v2 next 4/4] format-patch: support deep threading

[Thomas Rast]

For deep threading mode, i.e., the mode that gives a thread structured
like

  + [PATCH 0/n] Cover letter
   `-+ [PATCH 1/n] First patch
      `-+ [PATCH 2/n] Second patch
         `-+ ...

we currently have to use 'git send-email --thread' (the default).  On
the other hand, format-patch also has a --thread option which gives
shallow mode, i.e.,

  + [PATCH 0/n] Cover letter
  |-+ [PATCH 1/n] First patch
  |-+ [PATCH 2/n] Second patch
  ...

To reduce the confusion resulting from having two indentically named
features in different tools giving different results, let format-patch
take an optional argument '--thread=deep' that gives the same output
as 'send-mail --thread'.  With no argument, or 'shallow', behave as
before.  Also add a configuration variable format.thread with the same
semantics.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Same as v1.

 Documentation/config.txt           |   10 +++
 Documentation/git-format-patch.txt |   10 +++-
 builtin-log.c                      |   36 +++++++++--
 t/t4014-format-patch.sh            |  120 ++++++++++++++++++++++++++++++++++++
 4 files changed, 170 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3117d65..cf835c4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -690,6 +690,16 @@ format.pretty::
 	See linkgit:git-log[1], linkgit:git-show[1],
 	linkgit:git-whatchanged[1].
 
+format.thread::
+	The default threading style for 'git-format-patch'.  Can be
+	either a boolean value, `shallow` or `deep`.  'Shallow'
+	threading makes every mail a reply to the head of the series,
+	where the head is chosen from the cover letter, the
+	`\--in-reply-to`, and the first patch mail, in this order.
+	'Deep' threading makes every mail a reply to the previous one.
+	A true boolean value is the same as `shallow`, and a false
+	value disables threading.
+
 gc.aggressiveWindow::
 	The window size parameter used in the delta compression
 	algorithm used by 'git-gc --aggressive'.  This defaults
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index e7ae8cf..c14e3ee 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -127,10 +127,18 @@ include::diff-options.txt[]
 	which is the commit message and the patch itself in the
 	second part, with "Content-Disposition: inline".
 
---thread::
+--thread[=<style>]::
 	Add In-Reply-To and References headers to make the second and
 	subsequent mails appear as replies to the first.  Also generates
 	the Message-Id header to reference.
++
+The optional <style> argument can be either `shallow` or `deep`.
+'Shallow' threading makes every mail a reply to the head of the
+series, where the head is chosen from the cover letter, the
+`\--in-reply-to`, and the first patch mail, in this order.  'Deep'
+threading makes every mail a reply to the previous one.  If not
+specified, defaults to the 'format.thread' configuration, or `shallow`
+if that is not set.
 
 --in-reply-to=Message-Id::
 	Make the first mail (or all the mails with --no-thread) appear as a
diff --git a/builtin-log.c b/builtin-log.c
index 2dc6c48..8684fcd 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -462,6 +462,10 @@ static void add_header(const char *value)
 	extra_hdr[extra_hdr_nr++] = xstrndup(value, len);
 }
 
+#define THREAD_SHALLOW 1
+#define THREAD_DEEP 2
+static int thread = 0;
+
 static int git_format_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "format.headers")) {
@@ -498,7 +502,18 @@ static int git_format_config(const char *var, const char *value, void *cb)
 			default_attach = xstrdup(git_version_string);
 		return 0;
 	}
-
+	if (!strcmp(var, "format.thread")) {
+		if (value && !strcasecmp(value, "deep")) {
+			thread = THREAD_DEEP;
+			return 0;
+		}
+		if (value && !strcasecmp(value, "shallow")) {
+			thread = THREAD_SHALLOW;
+			return 0;
+		}
+		thread = git_config_bool(var, value) && THREAD_SHALLOW;
+		return 0;
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -777,7 +792,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int numbered_files = 0;		/* _just_ numbers */
 	int subject_prefix = 0;
 	int ignore_if_in_upstream = 0;
-	int thread = 0;
 	int cover_letter = 0;
 	int boundary_count = 0;
 	int no_binary_diff = 0;
@@ -879,8 +893,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		}
 		else if (!strcmp(argv[i], "--ignore-if-in-upstream"))
 			ignore_if_in_upstream = 1;
-		else if (!strcmp(argv[i], "--thread"))
-			thread = 1;
+		else if (!strcmp(argv[i], "--thread")
+			|| !strcmp(argv[i], "--thread=shallow"))
+			thread = THREAD_SHALLOW;
+		else if (!strcmp(argv[i], "--thread=deep"))
+			thread = THREAD_DEEP;
+		else if (!strcmp(argv[i], "--no-thread"))
+			thread = 0;
 		else if (!prefixcmp(argv[i], "--in-reply-to="))
 			in_reply_to = argv[i] + 14;
 		else if (!strcmp(argv[i], "--in-reply-to")) {
@@ -1055,6 +1074,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			/* Have we already had a message ID? */
 			if (rev.message_id) {
 				/*
+				 * For deep threading: make every mail
+				 * a reply to the previous one, no
+				 * matter what other options are set.
+				 *
+				 * For shallow threading:
+				 *
 				 * Without --cover-letter and
 				 * --in-reply-to, make every mail a
 				 * reply to the one before.
@@ -1069,7 +1094,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 				 * letter is a reply to the
 				 * --in-reply-to, if specified.
 				 */
-				if (rev.ref_message_ids->nr > 0
+				if (thread == THREAD_SHALLOW
+				    && rev.ref_message_ids->nr > 0
 				    && (!cover_letter || rev.nr > 1))
 					free(rev.message_id);
 				else
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 8b970c3..ebfc4a6 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -257,6 +257,126 @@ test_expect_success 'thread cover-letter in-reply-to' '
 		--in-reply-to="<test.message>" --thread master
 '
 
+test_expect_success 'thread explicit shallow' '
+	check_threading expect.cl-irt --cover-letter \
+		--in-reply-to="<test.message>" --thread=shallow master
+'
+
+cat > expect.deep <<EOF
+---
+Message-Id: <0>
+---
+Message-Id: <1>
+In-Reply-To: <0>
+References: <0>
+---
+Message-Id: <2>
+In-Reply-To: <1>
+References: <0>
+	<1>
+EOF
+
+test_expect_success 'thread deep' '
+	check_threading expect.deep --thread=deep master
+'
+
+cat > expect.deep-irt <<EOF
+---
+Message-Id: <0>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <2>
+In-Reply-To: <0>
+References: <1>
+	<0>
+---
+Message-Id: <3>
+In-Reply-To: <2>
+References: <1>
+	<0>
+	<2>
+EOF
+
+test_expect_success 'thread deep in-reply-to' '
+	check_threading expect.deep-irt  --thread=deep \
+		--in-reply-to="<test.message>" master
+'
+
+cat > expect.deep-cl <<EOF
+---
+Message-Id: <0>
+---
+Message-Id: <1>
+In-Reply-To: <0>
+References: <0>
+---
+Message-Id: <2>
+In-Reply-To: <1>
+References: <0>
+	<1>
+---
+Message-Id: <3>
+In-Reply-To: <2>
+References: <0>
+	<1>
+	<2>
+EOF
+
+test_expect_success 'thread deep cover-letter' '
+	check_threading expect.deep-cl --cover-letter --thread=deep master
+'
+
+cat > expect.deep-cl-irt <<EOF
+---
+Message-Id: <0>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <2>
+In-Reply-To: <0>
+References: <1>
+	<0>
+---
+Message-Id: <3>
+In-Reply-To: <2>
+References: <1>
+	<0>
+	<2>
+---
+Message-Id: <4>
+In-Reply-To: <3>
+References: <1>
+	<0>
+	<2>
+	<3>
+EOF
+
+test_expect_success 'thread deep cover-letter in-reply-to' '
+	check_threading expect.deep-cl-irt --cover-letter \
+		--in-reply-to="<test.message>" --thread=deep master
+'
+
+test_expect_success 'thread via config' '
+	git config format.thread true &&
+	check_threading expect.thread master
+'
+
+test_expect_success 'thread deep via config' '
+	git config format.thread deep &&
+	check_threading expect.deep master
+'
+
+test_expect_success 'thread config + override' '
+	git config format.thread deep &&
+	check_threading expect.thread --thread master
+'
+
+test_expect_success 'thread config + --no-thread' '
+	git config format.thread deep &&
+	check_threading expect.no-threading --no-thread master
+'
+
 test_expect_success 'excessive subject' '
 
 	rm -rf patches/ &&
-- 
1.6.2.rc1.310.ga3b4a

Re: [PATCH v2 next 0/4] format-patch --cover-letter --in-reply-to

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> I also noticed that there is a textual conflict with 0db5260 (Enable
> setting attach as the default in .gitconfig for git-format-patch.,
> 2009-02-12), which is already in 'next'.  So I rebased the series to
> 'next'.

In general I'd rather not to see a series based on 'next' merely for a
textual dependency like this (it is a different matter if your patch
semantically depends on somebody else's work in 'next').  Your own topic
will fork from 'master' and it will not want to be taken hostage of
"attach as default" series.

[PATCH 3/4] format-patch: thread as reply to cover letter even with in-reply-to

[Thomas Rast]

Currently, format-patch --thread --cover-letter --in-reply-to $parent
makes all mails, including the cover letter, a reply to $parent.
However, we would want the reader to consider the cover letter above
all the patches.

This changes the semantics so that only the cover letter is a reply to
$parent, while all the patches are formatted as replies to the cover
letter.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 builtin-log.c           |   20 +++++++++++++++-----
 t/t4014-format-patch.sh |    9 ++++++---
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index b188575..89bfa70 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -1036,12 +1036,22 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			/* Have we already had a message ID? */
 			if (rev.message_id) {
 				/*
-				 * If we've got the ID to be a reply
-				 * to, discard the current ID;
-				 * otherwise, make everything a reply
-				 * to that.
+				 * Without --cover-letter and
+				 * --in-reply-to, make every mail a
+				 * reply to the one before.
+				 *
+				 * With --in-reply-to but no
+				 * --cover-letter, make every mail a
+				 * reply to the <reply-to>.
+				 *
+				 * With --cover-letter, make every
+				 * mail but the cover letter a reply
+				 * to the cover letter.  The cover
+				 * letter is a reply to the
+				 * --in-reply-to, if specified.
 				 */
-				if (rev.ref_message_id->nr > 0)
+				if (rev.ref_message_id->nr > 0
+				    && (!cover_letter || rev.nr > 1))
 					free(rev.message_id);
 				else
 					string_list_append(rev.message_id,
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 345e6de..8b970c3 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -237,16 +237,19 @@ In-Reply-To: <1>
 References: <1>
 ---
 Message-Id: <2>
-In-Reply-To: <1>
+In-Reply-To: <0>
 References: <1>
+	<0>
 ---
 Message-Id: <3>
-In-Reply-To: <1>
+In-Reply-To: <0>
 References: <1>
+	<0>
 ---
 Message-Id: <4>
-In-Reply-To: <1>
+In-Reply-To: <0>
 References: <1>
+	<0>
 EOF
 
 test_expect_success 'thread cover-letter in-reply-to' '
-- 
1.6.2.rc1.295.g1e29

[PATCH 4/4] format-patch: support deep threading

[Thomas Rast]

For deep threading mode, i.e., the mode that gives a thread structured
like

  + [PATCH 0/n] Cover letter
   `-+ [PATCH 1/n] First patch
      `-+ [PATCH 2/n] Second patch
         `-+ ...

we currently have to use 'git send-email --thread' (the default).  On
the other hand, format-patch also has a --thread option which gives
shallow mode, i.e.,

  + [PATCH 0/n] Cover letter
  |-+ [PATCH 1/n] First patch
  |-+ [PATCH 2/n] Second patch
  ...

To reduce the confusion resulting from having two indentically named
features in different tools giving different results, let format-patch
take an optional argument '--thread=deep' that gives the same output
as 'send-mail --thread'.  With no argument, or 'shallow', behave as
before.  Also add a configuration variable format.thread with the same
semantics.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 Documentation/config.txt           |   10 +++
 Documentation/git-format-patch.txt |   10 +++-
 builtin-log.c                      |   35 +++++++++-
 t/t4014-format-patch.sh            |  120 ++++++++++++++++++++++++++++++++++++
 4 files changed, 170 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f5152c5..300ab25 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -677,6 +677,16 @@ format.pretty::
 	See linkgit:git-log[1], linkgit:git-show[1],
 	linkgit:git-whatchanged[1].
 
+format.thread::
+	The default threading style for 'git-format-patch'.  Can be
+	either a boolean value, `shallow` or `deep`.  'Shallow'
+	threading makes every mail a reply to the head of the series,
+	where the head is chosen from the cover letter, the
+	`\--in-reply-to`, and the first patch mail, in this order.
+	'Deep' threading makes every mail a reply to the previous one.
+	A true boolean value is the same as `shallow`, and a false
+	value disables threading.
+
 gc.aggressiveWindow::
 	The window size parameter used in the delta compression
 	algorithm used by 'git-gc --aggressive'.  This defaults
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 11a7d77..4302b14 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -122,10 +122,18 @@ include::diff-options.txt[]
 	which is the commit message and the patch itself in the
 	second part, with "Content-Disposition: inline".
 
---thread::
+--thread[=<style>]::
 	Add In-Reply-To and References headers to make the second and
 	subsequent mails appear as replies to the first.  Also generates
 	the Message-Id header to reference.
++
+The optional <style> argument can be either `shallow` or `deep`.
+'Shallow' threading makes every mail a reply to the head of the
+series, where the head is chosen from the cover letter, the
+`\--in-reply-to`, and the first patch mail, in this order.  'Deep'
+threading makes every mail a reply to the previous one.  If not
+specified, defaults to the 'format.thread' configuration, or `shallow`
+if that is not set.
 
 --in-reply-to=Message-Id::
 	Make the first mail (or all the mails with --no-thread) appear as a
diff --git a/builtin-log.c b/builtin-log.c
index 89bfa70..101343b 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -460,6 +460,10 @@ static void add_header(const char *value)
 	extra_hdr[extra_hdr_nr++] = xstrndup(value, len);
 }
 
+#define THREAD_SHALLOW 1
+#define THREAD_DEEP 2
+static int thread = 0;
+
 static int git_format_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "format.headers")) {
@@ -489,6 +493,18 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		auto_number = auto_number && numbered;
 		return 0;
 	}
+	if (!strcmp(var, "format.thread")) {
+		if (value && !strcasecmp(value, "deep")) {
+			thread = THREAD_DEEP;
+			return 0;
+		}
+		if (value && !strcasecmp(value, "shallow")) {
+			thread = THREAD_SHALLOW;
+			return 0;
+		}
+		thread = git_config_bool(var, value) && THREAD_SHALLOW;
+		return 0;
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -767,7 +783,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int numbered_files = 0;		/* _just_ numbers */
 	int subject_prefix = 0;
 	int ignore_if_in_upstream = 0;
-	int thread = 0;
 	int cover_letter = 0;
 	int boundary_count = 0;
 	int no_binary_diff = 0;
@@ -860,8 +875,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		}
 		else if (!strcmp(argv[i], "--ignore-if-in-upstream"))
 			ignore_if_in_upstream = 1;
-		else if (!strcmp(argv[i], "--thread"))
-			thread = 1;
+		else if (!strcmp(argv[i], "--thread")
+			|| !strcmp(argv[i], "--thread=shallow"))
+			thread = THREAD_SHALLOW;
+		else if (!strcmp(argv[i], "--thread=deep"))
+			thread = THREAD_DEEP;
+		else if (!strcmp(argv[i], "--no-thread"))
+			thread = 0;
 		else if (!prefixcmp(argv[i], "--in-reply-to="))
 			in_reply_to = argv[i] + 14;
 		else if (!strcmp(argv[i], "--in-reply-to")) {
@@ -1036,6 +1056,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			/* Have we already had a message ID? */
 			if (rev.message_id) {
 				/*
+				 * For deep threading: make every mail
+				 * a reply to the previous one, no
+				 * matter what other options are set.
+				 *
+				 * For shallow threading:
+				 *
 				 * Without --cover-letter and
 				 * --in-reply-to, make every mail a
 				 * reply to the one before.
@@ -1050,7 +1076,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 				 * letter is a reply to the
 				 * --in-reply-to, if specified.
 				 */
-				if (rev.ref_message_id->nr > 0
+				if (thread == THREAD_SHALLOW
+				    && rev.ref_message_id->nr > 0
 				    && (!cover_letter || rev.nr > 1))
 					free(rev.message_id);
 				else
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 8b970c3..ebfc4a6 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -257,6 +257,126 @@ test_expect_success 'thread cover-letter in-reply-to' '
 		--in-reply-to="<test.message>" --thread master
 '
 
+test_expect_success 'thread explicit shallow' '
+	check_threading expect.cl-irt --cover-letter \
+		--in-reply-to="<test.message>" --thread=shallow master
+'
+
+cat > expect.deep <<EOF
+---
+Message-Id: <0>
+---
+Message-Id: <1>
+In-Reply-To: <0>
+References: <0>
+---
+Message-Id: <2>
+In-Reply-To: <1>
+References: <0>
+	<1>
+EOF
+
+test_expect_success 'thread deep' '
+	check_threading expect.deep --thread=deep master
+'
+
+cat > expect.deep-irt <<EOF
+---
+Message-Id: <0>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <2>
+In-Reply-To: <0>
+References: <1>
+	<0>
+---
+Message-Id: <3>
+In-Reply-To: <2>
+References: <1>
+	<0>
+	<2>
+EOF
+
+test_expect_success 'thread deep in-reply-to' '
+	check_threading expect.deep-irt  --thread=deep \
+		--in-reply-to="<test.message>" master
+'
+
+cat > expect.deep-cl <<EOF
+---
+Message-Id: <0>
+---
+Message-Id: <1>
+In-Reply-To: <0>
+References: <0>
+---
+Message-Id: <2>
+In-Reply-To: <1>
+References: <0>
+	<1>
+---
+Message-Id: <3>
+In-Reply-To: <2>
+References: <0>
+	<1>
+	<2>
+EOF
+
+test_expect_success 'thread deep cover-letter' '
+	check_threading expect.deep-cl --cover-letter --thread=deep master
+'
+
+cat > expect.deep-cl-irt <<EOF
+---
+Message-Id: <0>
+In-Reply-To: <1>
+References: <1>
+---
+Message-Id: <2>
+In-Reply-To: <0>
+References: <1>
+	<0>
+---
+Message-Id: <3>
+In-Reply-To: <2>
+References: <1>
+	<0>
+	<2>
+---
+Message-Id: <4>
+In-Reply-To: <3>
+References: <1>
+	<0>
+	<2>
+	<3>
+EOF
+
+test_expect_success 'thread deep cover-letter in-reply-to' '
+	check_threading expect.deep-cl-irt --cover-letter \
+		--in-reply-to="<test.message>" --thread=deep master
+'
+
+test_expect_success 'thread via config' '
+	git config format.thread true &&
+	check_threading expect.thread master
+'
+
+test_expect_success 'thread deep via config' '
+	git config format.thread deep &&
+	check_threading expect.deep master
+'
+
+test_expect_success 'thread config + override' '
+	git config format.thread deep &&
+	check_threading expect.thread --thread master
+'
+
+test_expect_success 'thread config + --no-thread' '
+	git config format.thread deep &&
+	check_threading expect.no-threading --no-thread master
+'
+
 test_expect_success 'excessive subject' '
 
 	rm -rf patches/ &&
-- 
1.6.2.rc1.295.g1e29

[PATCH v2 1/2] bash completion: refactor common log, shortlog and gitk options

[Thomas Rast]

Refactor options that are useful for more than one of them into a
variable used by the relevant completions.  This has the effect of
adding the following options to git-log:

  --branches --tags --remotes --first-parent --dense --sparse
  --simplify-merges --simplify-by-decoration --first-parent
  --no-merges

The following to git-shortlog:

  --branches --tags --remotes --first-parent

And the following to gitk:

  --branches --tags --remotes --first-parent --no-merges --max-count=
  --max-age= --since= --after= --min-age= --until= --before= --dense
  --sparse --full-history --simplify-merges --simplify-by-decoration
  --left-right

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 contrib/completion/git-completion.bash |   49 ++++++++++++++++++++++----------
 1 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 412d2c0..a5b3bf3 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -975,6 +975,27 @@ _git_ls_tree ()
 	__git_complete_file
 }
 
+# Options that go well for log, shortlog and gitk
+__git_log_common_options="
+	--not --all
+	--branches --tags --remotes
+	--first-parent --no-merges
+	--max-count=
+	--max-age= --since= --after=
+	--min-age= --until= --before=
+"
+# Options that go well for log and gitk (not shortlog)
+__git_log_gitk_options="
+	--dense --sparse --full-history
+	--simplify-merges --simplify-by-decoration
+	--left-right
+"
+# Options that go well for log and shortlog (not gitk)
+__git_log_shortlog_options="
+	--author= --committer= --grep=
+	--all-match
+"
+
 __git_log_pretty_formats="oneline short medium full fuller email raw format:"
 
 _git_log ()
@@ -996,21 +1017,19 @@ _git_log ()
 		;;
 	--*)
 		__gitcomp "
-			--max-count= --max-age= --since= --after=
-			--min-age= --before= --until=
+			$__git_log_common_options
+			$__git_log_shortlog_options
+			$__git_log_gitk_options
 			--root --topo-order --date-order --reverse
-			--no-merges --follow
+			--follow
 			--abbrev-commit --abbrev=
 			--relative-date --date=
-			--author= --committer= --grep=
-			--all-match
 			--pretty=
-			--not --all
-			--left-right --cherry-pick
+			--cherry-pick
 			--graph
 			--decorate
 			--walk-reflogs
-			--parents --children --full-history
+			--parents --children
 			--merge
 			$__git_diff_common_options
 			--pickaxe-all --pickaxe-regex
@@ -1495,12 +1514,8 @@ _git_shortlog ()
 	case "$cur" in
 	--*)
 		__gitcomp "
-			--max-count= --max-age= --since= --after=
-			--min-age= --before= --until=
-			--no-merges
-			--author= --committer= --grep=
-			--all-match
-			--not --all
+			$__git_log_common_options
+			$__git_log_shortlog_options
 			--numbered --summary
 			"
 		return
@@ -1808,7 +1823,11 @@ _gitk ()
 	fi
 	case "$cur" in
 	--*)
-		__gitcomp "--not --all $merge"
+		__gitcomp "
+			$__git_log_common_options
+			$__git_log_gitk_options
+			$merge
+			"
 		return
 		;;
 	esac
-- 
1.6.2.rc0.328.g2c702

[PATCH v2 2/2] bash completion: only show 'log --merge' if merging

[Thomas Rast]

The gitk completion only shows --merge if MERGE_HEAD is present.
Do it the same way for git-log completion.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 contrib/completion/git-completion.bash |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a5b3bf3..489951b 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1003,6 +1003,11 @@ _git_log ()
 	__git_has_doubledash && return
 
 	local cur="${COMP_WORDS[COMP_CWORD]}"
+	local g="$(git rev-parse --git-dir 2>/dev/null)"
+	local merge=""
+	if [ -f $g/MERGE_HEAD ]; then
+		merge="--merge"
+	fi
 	case "$cur" in
 	--pretty=*)
 		__gitcomp "$__git_log_pretty_formats
@@ -1030,7 +1035,7 @@ _git_log ()
 			--decorate
 			--walk-reflogs
 			--parents --children
-			--merge
+			$merge
 			$__git_diff_common_options
 			--pickaxe-all --pickaxe-regex
 			"
-- 
1.6.2.rc0.328.g2c702

Re: [PATCH v2 2/2] bash completion: only show 'log --merge' if merging

[Shawn O. Pearce]

Thomas Rast <trast@student.ethz.ch> wrote:
> The gitk completion only shows --merge if MERGE_HEAD is present.
> Do it the same way for git-log completion.
> 
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>  contrib/completion/git-completion.bash |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)

Both patches,

Acked-by: Shawn O. Pearce <spearce@spearce.org>
 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index a5b3bf3..489951b 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1003,6 +1003,11 @@ _git_log ()
>  	__git_has_doubledash && return
>  
>  	local cur="${COMP_WORDS[COMP_CWORD]}"
> +	local g="$(git rev-parse --git-dir 2>/dev/null)"
> +	local merge=""
> +	if [ -f $g/MERGE_HEAD ]; then
> +		merge="--merge"
> +	fi
>  	case "$cur" in
>  	--pretty=*)
>  		__gitcomp "$__git_log_pretty_formats
> @@ -1030,7 +1035,7 @@ _git_log ()
>  			--decorate
>  			--walk-reflogs
>  			--parents --children
> -			--merge
> +			$merge
>  			$__git_diff_common_options
>  			--pickaxe-all --pickaxe-regex
>  			"
> -- 
> 1.6.2.rc0.328.g2c702
> 

-- 
Shawn.

Re: [PATCH v2 2/2] bash completion: only show 'log --merge' if merging

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Thomas Rast <trast@student.ethz.ch> wrote:
>> The gitk completion only shows --merge if MERGE_HEAD is present.
>> Do it the same way for git-log completion.
>> 
>> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
>> ---
>>  contrib/completion/git-completion.bash |    7 ++++++-
>>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> Both patches,
>
> Acked-by: Shawn O. Pearce <spearce@spearce.org>

Thanks.