Git development
 help / color / mirror / Atom feed
* Fix potential hang in https handshake (v2).
From: szager @ 2012-10-18 23:27 UTC (permalink / raw)
  To: git; +Cc: gitster, sop

>From aa77ab3dd5b98a5786ac158528f45355fc0ddbc3 Mon Sep 17 00:00:00 2001
From: Stefan Zager <szager@google.com>
Date: Thu, 18 Oct 2012 16:23:53 -0700
Subject: [PATCH] Fix potential hang in https handshake.

It will sometimes happen that curl_multi_fdset() doesn't
return any file descriptors.  In that case, it's recommended
that the application sleep for a short time before running
curl_multi_perform() again.

http://curl.haxx.se/libcurl/c/curl_multi_fdset.html

Signed-off-by: Stefan Zager <szager@google.com>
---
 http.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/http.c b/http.c
index df9bb71..e8aba7f 100644
--- a/http.c
+++ b/http.c
@@ -630,6 +630,10 @@ void run_active_slot(struct active_request_slot *slot)
 			FD_ZERO(&writefds);
 			FD_ZERO(&excfds);
 			curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
+			if (max_fd < 0) {
+				select_timeout.tv_sec  = 0;
+				select_timeout.tv_usec = 50000;
+			}
 
 			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
 		}
-- 
1.7.7.3

^ permalink raw reply related

* Unexpected directories from read-tree
From: Uri Moszkowicz @ 2012-10-18 23:10 UTC (permalink / raw)
  To: git

I'm testing out the sparse checkout feature of Git on my large (14GB)
repository and am running into a problem. When I add "dir1/" to
sparse-checkout and then run "git read-tree -mu HEAD" I see dir1 as
expected. But when I add "dir2/" to sparse-checkout and read-tree
again I see dir2 and dir3 appear and they're not nested. If I replace
"dir2/" with "dir3/" in the sparse-checkout file, then I see dir1 and
dir3 but not dir2 as expected again. How can I debug this problem?

^ permalink raw reply

* git subtree error (just how do you expect me to merge 0 trees?)
From: Drew Crawford @ 2012-10-18 23:04 UTC (permalink / raw)
  To: git@vger.kernel.org

I noticed today that if you leave off the branch name from git subtree like so:

$ git subtree add --prefix somewhere -m "adding CDH as subtree" path/to/repo
warning: read-tree: emptying the index with no arguments is deprecated; use --empty
fatal: just how do you expect me to merge 0 trees?

The error message is not particularly helpful (and seems to actually be in read-subtree?)  The solution in my case was to add the branch name on the end of the command.

Ideally it would be better to emit an error-message from a script higher up the calling chain that would be more descriptive about the problem (such as suggesting no branch is specified).

^ permalink raw reply

* Re: Fix potential hang in https handshake.
From: Junio C Hamano @ 2012-10-18 22:59 UTC (permalink / raw)
  To: szager; +Cc: git, sop
In-Reply-To: <5080761e.IEDySUuQzlCwL6fM%szager@google.com>

szager@google.com writes:

> From 700b8075c578941c8f951711825c390ac68b190f Mon Sep 17 00:00:00 2001
> From: Stefan Zager <szager@google.com>
> Date: Thu, 18 Oct 2012 14:03:59 -0700
> Subject: [PATCH] Fix potential hang in https handshake.
>
> It will sometimes happen that curl_multi_fdset() doesn't
> return any file descriptors.  In that case, it's recommended
> that the application sleep for a short time before running
> curl_multi_perform() again.
>
> http://curl.haxx.se/libcurl/c/curl_multi_fdset.html
>
> Signed-off-by: Stefan Zager <szager@google.com>
> ---

Thanks.  Would it be a better idea to "patch up" in problematic
case, instead of making this logic too deeply nested, like this
instead, I have to wonder...


	... all the existing code above unchanged ...
	curl_multi_fdset(..., &max_fd);
+	if (max_fd < 0) {    
+		/* nothing actionable??? */
+		select_timeout.tv_sec = 0;
+		select_timeout.tv_usec = 50000;
+	}

	select(max_fd+1, ..., &select_timeout);



>  http.c |   40 ++++++++++++++++++++++++++--------------
>  1 files changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/http.c b/http.c
> index df9bb71..a6f66c0 100644
> --- a/http.c
> +++ b/http.c
> @@ -602,35 +602,47 @@ void run_active_slot(struct active_request_slot *slot)
>  	int max_fd;
>  	struct timeval select_timeout;
>  	int finished = 0;
> +	long curl_timeout;
>  
>  	slot->finished = &finished;
>  	while (!finished) {
>  		step_active_slots();
>  
>  		if (slot->in_use) {
> +			max_fd = -1;
> +			FD_ZERO(&readfds);
> +			FD_ZERO(&writefds);
> +			FD_ZERO(&excfds);
> +			curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
> +
>  #if LIBCURL_VERSION_NUM >= 0x070f04
> -			long curl_timeout;
> -			curl_multi_timeout(curlm, &curl_timeout);
> -			if (curl_timeout == 0) {
> -				continue;
> -			} else if (curl_timeout == -1) {
> +			/* It will sometimes happen that curl_multi_fdset() doesn't
> +			   return any file descriptors.  In that case, it's recommended
> +			   that the application sleep for a short time before running
> +			   curl_multi_perform() again.
> +
> +			   http://curl.haxx.se/libcurl/c/curl_multi_fdset.html
> +			*/
> +			if (max_fd == -1) {
>  				select_timeout.tv_sec  = 0;
>  				select_timeout.tv_usec = 50000;
>  			} else {
> -				select_timeout.tv_sec  =  curl_timeout / 1000;
> -				select_timeout.tv_usec = (curl_timeout % 1000) * 1000;
> +				curl_timeout = 0;
> +				curl_multi_timeout(curlm, &curl_timeout);
> +				if (curl_timeout == 0) {
> +					continue;
> +				} else if (curl_timeout == -1) {
> +					select_timeout.tv_sec  = 0;
> +					select_timeout.tv_usec = 50000;
> +				} else {
> +					select_timeout.tv_sec  =  curl_timeout / 1000;
> +					select_timeout.tv_usec = (curl_timeout % 1000) * 1000;
> +				}
>  			}
>  #else
>  			select_timeout.tv_sec  = 0;
>  			select_timeout.tv_usec = 50000;
>  #endif
> -
> -			max_fd = -1;
> -			FD_ZERO(&readfds);
> -			FD_ZERO(&writefds);
> -			FD_ZERO(&excfds);
> -			curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
> -
>  			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
>  		}
>  	}

^ permalink raw reply

* [PATCH] Documentation/fetch-options.txt: Clarify --prune with option --tags
From: jari.aalto @ 2012-10-18 22:41 UTC (permalink / raw)
  To: git; +Cc: Jari Aalto

From: Jari Aalto <jari.aalto@cante.net>


Signed-off-by: Jari Aalto <jari.aalto@cante.net>
---
 Documentation/fetch-options.txt |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index b4d6476..90d50fb 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -38,7 +38,8 @@ ifndef::git-pull[]
 -p::
 --prune::
 	After fetching, remove any remote-tracking branches which
-	no longer exist	on the remote.
+	no longer exist	on the remote. If used with option `--tags`,
+	remove tags in a similar manner.
 endif::git-pull[]
 
 ifdef::git-pull[]
-- 
1.7.10.4

^ permalink raw reply related

* Re: Git for Windows and line endings
From: Erik Faye-Lund @ 2012-10-18 22:40 UTC (permalink / raw)
  To: Chris B; +Cc: git, msysgit
In-Reply-To: <CADKp0pyy=Nnv29LyhzAOX4B5wJNYnZ0h5d7zxNRyZxV2OGUSjg@mail.gmail.com>

On Fri, Oct 19, 2012 at 12:13 AM, Chris B <chris.blaszczynski@gmail.com> wrote:
> Hi.. it is such a crime to have that default option of MSysGit mess
> around with the line endings.

No it's not.

> There is no thought to the fact that it's possible the Git users are
> not using Git the exact way the authors thought it would be used.

Suggesting this is just insulting. A lot of thought was laid to ground
for the decision.

> We have both Windows and Linux systems that have parts of their files
> stored in Git repositories. And in addition to that, some Linux and
> Windows Git clients. If everyone leaves the line endings alone,
> everything works out just fine!

No, it will not. Notepad, which is the default text editor on Windows,
barfs on LF line-endings, and many vi installations does the same
thing on Unix-systems bards on CRLF line-endings. And so does a huge
amount of custom, less tested tools.

Thinking that "this works for me, so it must work for everyone" is
exactly the reason why this whole situation is a big mess. You are
only making it worse by not realizing the issues.

> But messing with the line endings broke some things in production.

I'm not even sure what you're trying to say with this e-mail other
than to blame us for your own problems. Stop making broken commits.
Clean up after you when you did by accident. And for the love of God,
stop blaming other people when you messed up.

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply

* Git for Windows and line endings
From: Chris B @ 2012-10-18 22:13 UTC (permalink / raw)
  To: git, msysgit

Hi.. it is such a crime to have that default option of MSysGit mess
around with the line endings.

Caused us a lot of trouble.

There is no thought to the fact that it's possible the Git users are
not using Git the exact way the authors thought it would be used.
We have both Windows and Linux systems that have parts of their files
stored in Git repositories. And in addition to that, some Linux and
Windows Git clients. If everyone leaves the line endings alone,
everything works out just fine!

But messing with the line endings broke some things in production.

^ permalink raw reply

* Re: [PATCH 6/6] format-patch --notes: show notes after three-dashes
From: Junio C Hamano @ 2012-10-18 22:08 UTC (permalink / raw)
  To: Philip Oakley; +Cc: git
In-Reply-To: <C2D3CE0879B345FFAC2A6487760B5378@PhilipOakley>

"Philip Oakley" <philipoakley@iee.org> writes:

> From: "Junio C Hamano" <gitster@pobox.com>
>> When inserting the note after the commit log message to format-patch
>> output, add three dashes before the note.  Record the fact that we
>> did so in the rev_info and omit showing duplicated three dashes in
>> the usual codepath that is used when notes are not being shown.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Should this also include a documentation update to make this
> substantive benefit visible, whether that be in the format-patch man
> pages, the SubmittingPatches guide, in the git-notes description of 'A
> typical use...', or even in the user-manual?

Eric Blake (http://mid.gmane.org/507EB310.8020904@redhat.com) was
already working on a documentation updates already, I thought.

As long as what it does is explained in format-patch, that is fine.

I do not think this deserves to be in the SubmittingPatches.  We do
tell people to hide "here is the context of the change" additional
explanation after three dashes, but how the submitters prepare that
text is entirely up to them (and I personally do not think notes is
not necessarily the right tool to do so).

^ permalink raw reply

* Fix potential hang in https handshake.
From: szager @ 2012-10-18 21:35 UTC (permalink / raw)
  To: git; +Cc: gitster, sop

>From 700b8075c578941c8f951711825c390ac68b190f Mon Sep 17 00:00:00 2001
From: Stefan Zager <szager@google.com>
Date: Thu, 18 Oct 2012 14:03:59 -0700
Subject: [PATCH] Fix potential hang in https handshake.

It will sometimes happen that curl_multi_fdset() doesn't
return any file descriptors.  In that case, it's recommended
that the application sleep for a short time before running
curl_multi_perform() again.

http://curl.haxx.se/libcurl/c/curl_multi_fdset.html

Signed-off-by: Stefan Zager <szager@google.com>
---
 http.c |   40 ++++++++++++++++++++++++++--------------
 1 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/http.c b/http.c
index df9bb71..a6f66c0 100644
--- a/http.c
+++ b/http.c
@@ -602,35 +602,47 @@ void run_active_slot(struct active_request_slot *slot)
 	int max_fd;
 	struct timeval select_timeout;
 	int finished = 0;
+	long curl_timeout;
 
 	slot->finished = &finished;
 	while (!finished) {
 		step_active_slots();
 
 		if (slot->in_use) {
+			max_fd = -1;
+			FD_ZERO(&readfds);
+			FD_ZERO(&writefds);
+			FD_ZERO(&excfds);
+			curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
+
 #if LIBCURL_VERSION_NUM >= 0x070f04
-			long curl_timeout;
-			curl_multi_timeout(curlm, &curl_timeout);
-			if (curl_timeout == 0) {
-				continue;
-			} else if (curl_timeout == -1) {
+			/* It will sometimes happen that curl_multi_fdset() doesn't
+			   return any file descriptors.  In that case, it's recommended
+			   that the application sleep for a short time before running
+			   curl_multi_perform() again.
+
+			   http://curl.haxx.se/libcurl/c/curl_multi_fdset.html
+			*/
+			if (max_fd == -1) {
 				select_timeout.tv_sec  = 0;
 				select_timeout.tv_usec = 50000;
 			} else {
-				select_timeout.tv_sec  =  curl_timeout / 1000;
-				select_timeout.tv_usec = (curl_timeout % 1000) * 1000;
+				curl_timeout = 0;
+				curl_multi_timeout(curlm, &curl_timeout);
+				if (curl_timeout == 0) {
+					continue;
+				} else if (curl_timeout == -1) {
+					select_timeout.tv_sec  = 0;
+					select_timeout.tv_usec = 50000;
+				} else {
+					select_timeout.tv_sec  =  curl_timeout / 1000;
+					select_timeout.tv_usec = (curl_timeout % 1000) * 1000;
+				}
 			}
 #else
 			select_timeout.tv_sec  = 0;
 			select_timeout.tv_usec = 50000;
 #endif
-
-			max_fd = -1;
-			FD_ZERO(&readfds);
-			FD_ZERO(&writefds);
-			FD_ZERO(&excfds);
-			curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
-
 			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
 		}
 	}
-- 
1.7.7.3

^ permalink raw reply related

* Re: [PATCH 6/6] format-patch --notes: show notes after three-dashes
From: Philip Oakley @ 2012-10-18 21:35 UTC (permalink / raw)
  To: Junio C Hamano, git
In-Reply-To: <1350539128-21577-7-git-send-email-gitster@pobox.com>

From: "Junio C Hamano" <gitster@pobox.com>
> When inserting the note after the commit log message to format-patch
> output, add three dashes before the note.  Record the fact that we
> did so in the rev_info and omit showing duplicated three dashes in
> the usual codepath that is used when notes are not being shown.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Should this also include a documentation update to make this substantive 
benefit visible, whether that be in the format-patch man pages, the 
SubmittingPatches guide, in the git-notes description of 'A typical 
use...', or even in the user-manual?

Do you have a preference?

> ---
> log-tree.c              | 15 +++++++++++----
> revision.h              |  1 +
> t/t4014-format-patch.sh |  7 +++++--
> 3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/log-tree.c b/log-tree.c
> index 4390b11..712a22b 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -677,8 +677,13 @@ void show_log(struct rev_info *opt)
>  append_signoff(&msgbuf, opt->add_signoff);
>
>  if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
> -     ctx.notes_message && *ctx.notes_message)
> +     ctx.notes_message && *ctx.notes_message) {
> + if (ctx.fmt == CMIT_FMT_EMAIL) {
> + strbuf_addstr(&msgbuf, "---\n");
> + opt->shown_dashes = 1;
> + }
>  strbuf_addstr(&msgbuf, ctx.notes_message);
> + }
>
>  if (opt->show_log_size) {
>  printf("log size %i\n", (int)msgbuf.len);
> @@ -710,6 +715,7 @@ void show_log(struct rev_info *opt)
>
> int log_tree_diff_flush(struct rev_info *opt)
> {
> + opt->shown_dashes = 0;
>  diffcore_std(&opt->diffopt);
>
>  if (diff_queue_is_empty()) {
> @@ -737,10 +743,11 @@ int log_tree_diff_flush(struct rev_info *opt)
>  opt->diffopt.output_prefix_data);
>  fwrite(msg->buf, msg->len, 1, stdout);
>  }
> - if ((pch & opt->diffopt.output_format) == pch) {
> - printf("---");
> + if (!opt->shown_dashes) {
> + if ((pch & opt->diffopt.output_format) == pch)
> + printf("---");
> + putchar('\n');
>  }
> - putchar('\n');
>  }
>  }
>  diff_flush(&opt->diffopt);
> diff --git a/revision.h b/revision.h
> index a95bd0b..059bfff 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -111,6 +111,7 @@ struct rev_info {
>
>  /* Format info */
>  unsigned int shown_one:1,
> + shown_dashes:1,
>  show_merge:1,
>  show_notes:1,
>  show_notes_given:1,
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index bea6381..9750ba6 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -623,9 +623,12 @@ test_expect_success 'format-patch --signoff' '
> test_expect_success 'format-patch --notes --signoff' '
>  git notes --ref test add -m "test message" HEAD &&
>  git format-patch -1 --signoff --stdout --notes=test >out &&
> - # Notes message must come after S-o-b
> + # Three dashes must come after S-o-b
>  ! sed "/^Signed-off-by: /q" out | grep "test message" &&
> - sed "1,/^Signed-off-by: /d" out | grep "test message"
> + sed "1,/^Signed-off-by: /d" out | grep "test message" &&
> + # Notes message must come after three dashes
> + ! sed "/^---$/q" out | grep "test message" &&
> + sed "1,/^---$/d" out | grep "test message"
> '
>
> echo "fatal: --name-only does not make sense" > expect.name-only
> -- 
> 1.8.0.rc3.112.gdb88a5e
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> -----
> No virus found in this message.
> Checked by AVG - www.avg.com
> Version: 2013.0.2741 / Virus Database: 2614/5837 - Release Date: 
> 10/17/12
> 

^ permalink raw reply

* Re: [PATCH 4/5] branch: skip commit checks when deleting symref branches
From: Junio C Hamano @ 2012-10-18 21:34 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git mailing list, Johan Herland, Miklos Vajna
In-Reply-To: <507FF0EF.3070702@lsrfire.ath.cx>

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Before a branch is deleted, we check that it points to a valid
> commit.  With -d we also check that the commit is a merged; this
> check is not done with -D.
>
> The reason for that is that commits pointed to by branches should
> never go missing; if they do then something broke and it's better
> to stop instead of adding to the mess.  And a non-merged commit
> may contain changes that are worth preserving, so we require the
> stronger option -D instead of -d to get rid of them.
>
> If a branch consists of a symref, these concerns don't apply.
> Deleting such a branch can't make a commit become unreferenced,
> so we don't need to check if it is merged, or even if it is
> actually a valid commit.  Skip them in that case.  This allows
> us to delete dangling symref branches.

Purist in me tells me that we should be using "symbolic-ref -d" to
correct from such a misconfiguration, but ignoring that, the above
logic makes perfect sense to me, especially together with the next
change to tell what branch the symref was pointing at, which can be
used by the user when the user removes such a symbolic ref by
mistake.

Thanks.  Will queue all five.

^ permalink raw reply

* Re: [PATCH v3] status: refactor output format to represent "default" and add --long
From: Jeff King @ 2012-10-18 21:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <7vtxtrlbvo.fsf@alter.siamese.dyndns.org>

On Thu, Oct 18, 2012 at 02:16:11PM -0700, Junio C Hamano wrote:

> I guess combining both is fine, but then the commit is no longer "in
> preparation for adding" the option, but it already adds "--long", I
> would think.

Maybe a better commit message is:

-- >8 --
Subject: status: add --long output format option

You can currently set the output format to --short or
--porcelain. There is no --long, because we default to it
already. However, you may want to override an alias that
uses "--short" to get back to the default.

This requires a little bit of refactoring, because currently
we use STATUS_FORMAT_LONG internally to mean the same as
"the user did not specify anything". By expanding the enum
to include STATUS_FORMAT_NONE, we can distinguish between
the implicit and explicit cases. This effects these
conditions:

  1. The user has asked for NUL termination. With NONE, we
     currently default to turning on the porcelain mode.
     With an explicit --long, we would in theory use NUL
     termination with the long mode, but it does not support
     it. So we can just complain and die.

  2. When an output format is given to "git commit", we
     default to "--dry-run". This behavior would now kick in
     when "--long" is given, too.

^ permalink raw reply

* Re: [PATCH v3] status: refactor output format to represent "default" and add --long
From: Junio C Hamano @ 2012-10-18 21:16 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King
In-Reply-To: <1350569750-26304-1-git-send-email-pclouds@gmail.com>

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

> From: Jeff King <peff@peff.net>
>
> When deciding which output format to use, we default an internal enum
> to STATUS_FORMAT_LONG and modify it if "--porcelain" or "--short" is
> given. If this enum is set to LONG, then we know the user has not
> specified any format, and we can kick in default behaviors. This works
> because there is no "--long" which they could use to explicitly
> specify LONG.
>
> Let's expand the enum to have an explicit STATUS_FORMAT_NONE, in
> preparation for adding "--long", which can be used to override --short
> or --porcelain. Then we can distinguish between LONG and NONE when
> setting other defaults. There are two such cases:
>
>   1. The user has asked for NUL termination. With NONE, we
>      currently default to turning on the porcelain mode.
>      With an explicit --long, we would in theory use NUL
>      termination with the long mode, but it does not support
>      it. So we can just complain and die.
>
>   2. When an output format is given to "git commit", we
>      default to "--dry-run". This behavior would now kick in
>      when "--long" is given, too.
>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  On Thu, Oct 18, 2012 at 9:03 AM, Jeff King <peff@peff.net> wrote:
>  > I think that is fine to split it like this, but you would want to update
>  > the commit message above. Probably just remove those two cases and say
>  > something like:
>  >
>  >   Note that you cannot actually trigger STATUS_FORMAT_LONG, as we do
>  >   not yet have "--long"; that will come in a follow-on patch.
>  >
>  > And then move the reasoning for how "--long" works with each case into
>  > the commit message of the next patch.
>
>  Nope, it's hard to split the explanation in two (at least to me),
>  which may mean that the split does not make sense.

I guess combining both is fine, but then the commit is no longer "in
preparation for adding" the option, but it already adds "--long", I
would think.

Will queue.

Thanks.

^ permalink raw reply

* Re: Aw: Re: Aw: Re: [Patch v3 0/8] Create single PDF for all HTML files
From: Junio C Hamano @ 2012-10-18 20:40 UTC (permalink / raw)
  To: Thomas Ackermann; +Cc: git, philipoakley, peff, git
In-Reply-To: <634888125.421937.1350580792693.JavaMail.ngmail@webmail11.arcor-online.net>

Thomas Ackermann <th.acker66@arcor.de> writes:

> Yes; in e2399e9 the following hunks where left out from the patch to update-hook-example.txt:
>
> @@ -111,12 +114,12 @@ then
>  
>        info "Found matching head pattern: '$head_pattern'"
>        for user_pattern in $user_patterns; do
> -	info "Checking user: '$username' against pattern: '$user_pattern'"
> -	matchlen=$(expr "$username" : "$user_pattern")
> -	if test "$matchlen" = "${#username}"
> -	then
> -	  grant "Allowing user: '$username' with pattern: '$user_pattern'"
> -	fi
> +        info "Checking user: '$username' against pattern: '$user_pattern'"
> +        matchlen=$(expr "$username" : "$user_pattern")
> +        if test "$matchlen" = "${#username}"
> +        then
> +          grant "Allowing user: '$username' with pattern: '$user_pattern'"
> +        fi
>        done
>        deny "The user is not in the access list for this branch"
>      done
> @@ -149,13 +152,13 @@ then
>  
>        info "Found matching head pattern: '$head_pattern'"
>        for group_pattern in $group_patterns; do
> -	for groupname in $groups; do
> -	  info "Checking group: '$groupname' against pattern: '$group_pattern'"
> -	  matchlen=$(expr "$groupname" : "$group_pattern")
> -	  if test "$matchlen" = "${#groupname}"
> -	  then
> -	    grant "Allowing group: '$groupname' with pattern: '$group_pattern'"
> -	  fi
> +        for groupname in $groups; do
> +          info "Checking group: '$groupname' against pattern: '$group_pattern'"
> +          matchlen=$(expr "$groupname" : "$group_pattern")
> +          if test "$matchlen" = "${#groupname}"
> +          then
> +            grant "Allowing group: '$groupname' with pattern: '$group_pattern'"
> +          fi
>          done
>        done
>        deny "None of the user's groups are in the access list for this branch"

Yuck.  That is because I almost always apply patches with whitespace
breakage fix.  The above two hunks, if taken as patches to shell
script, does nothing but adding whitespace breakages, turning tab
indent into expanded runs of spaces, and that was why the tool
dropped them.

Resurrected; will queue the result later.

Thanks.

^ permalink raw reply

* Re: [PATCH v3] status: refactor output format to represent "default" and add --long
From: Jeff King @ 2012-10-18 20:26 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <1350569750-26304-1-git-send-email-pclouds@gmail.com>

On Thu, Oct 18, 2012 at 09:15:50PM +0700, Nguyen Thai Ngoc Duy wrote:

>  On Thu, Oct 18, 2012 at 9:03 AM, Jeff King <peff@peff.net> wrote:
>  > I think that is fine to split it like this, but you would want to update
>  > the commit message above. Probably just remove those two cases and say
>  > something like:
>  >
>  >   Note that you cannot actually trigger STATUS_FORMAT_LONG, as we do
>  >   not yet have "--long"; that will come in a follow-on patch.
>  >
>  > And then move the reasoning for how "--long" works with each case into
>  > the commit message of the next patch.
> 
>  Nope, it's hard to split the explanation in two (at least to me),
>  which may mean that the split does not make sense.

Yeah, that was the same place that I had difficulty when writing the
original. This version looks OK to me.

-Peff

^ permalink raw reply

* Aw: Re: Aw: Re: [Patch v3 0/8] Create single PDF for all HTML files
From: Thomas Ackermann @ 2012-10-18 17:19 UTC (permalink / raw)
  To: gitster, th.acker66; +Cc: git, philipoakley, peff, git

 
>
> That means that for the patch [6/8], which adds content-type to the
> text files, to be complete, it needs to update Makefile to produce
> html files from them.
> 

IMHO also for the new files in ./technical html should be created because
now as we have asciidoc markup why not also use it.

>
> > BTW2: The 'pretty-print shell script in update-hook-example.txt'
> > part of my changes was left out from the merge to pu ...
> 
> Do you mean e2399e9 (Documentation/howto: convert plain text files
> to asciidoc, 2012-10-16), or something else?

Yes; in e2399e9 the following hunks where left out from the patch to update-hook-example.txt:

@@ -111,12 +114,12 @@ then
 
       info "Found matching head pattern: '$head_pattern'"
       for user_pattern in $user_patterns; do
-	info "Checking user: '$username' against pattern: '$user_pattern'"
-	matchlen=$(expr "$username" : "$user_pattern")
-	if test "$matchlen" = "${#username}"
-	then
-	  grant "Allowing user: '$username' with pattern: '$user_pattern'"
-	fi
+        info "Checking user: '$username' against pattern: '$user_pattern'"
+        matchlen=$(expr "$username" : "$user_pattern")
+        if test "$matchlen" = "${#username}"
+        then
+          grant "Allowing user: '$username' with pattern: '$user_pattern'"
+        fi
       done
       deny "The user is not in the access list for this branch"
     done
@@ -149,13 +152,13 @@ then
 
       info "Found matching head pattern: '$head_pattern'"
       for group_pattern in $group_patterns; do
-	for groupname in $groups; do
-	  info "Checking group: '$groupname' against pattern: '$group_pattern'"
-	  matchlen=$(expr "$groupname" : "$group_pattern")
-	  if test "$matchlen" = "${#groupname}"
-	  then
-	    grant "Allowing group: '$groupname' with pattern: '$group_pattern'"
-	  fi
+        for groupname in $groups; do
+          info "Checking group: '$groupname' against pattern: '$group_pattern'"
+          matchlen=$(expr "$groupname" : "$group_pattern")
+          if test "$matchlen" = "${#groupname}"
+          then
+            grant "Allowing group: '$groupname' with pattern: '$group_pattern'"
+          fi
         done
       done
       deny "None of the user's groups are in the access list for this branch"



---
Thomas

^ permalink raw reply

* Re: [PATCH 0/6] Bring "format-patch --notes" closer to a real feature
From: Junio C Hamano @ 2012-10-18 17:04 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <CACsJy8Dxx6JtRXP_HWNBrobNems+4UsaOGSRef08cf8--b5dOw@mail.gmail.com>

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Thu, Oct 18, 2012 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> This replaces the earlier "wip" with a real thing.
>>
>> We never advertised the "--notes" option to format-patch (or
>> anything related to the pretty format options for that matter)
>> because the behaviour of these options was whatever they happened to
>> do, not what they were designed to do.
>
> Stupid question: does "git am" recreate notes from "format-patch
> --notes" output? If it does not, should it? I think it could be a nice
> way of copying notes from one machine to another, but not enabled by
> default (iow "am --notes").

Thinking about what the "notes" are, I do not think it should, at
least by default; the model is broken at the conceptual level.

The notes are comments on a commit you make after the fact, because
you do not want to (or cannot afford to) amend the commit to include
the comment.  When you are sending it out over e-mailto be applied
with "am", as opposed to asking the other to pull, you are by
definition willing to see the commit replayed with modification.

I think it is sensible for format-patch/am pipeline when asked to
use --notes to add the notes section after "---" as additional
material to help the recipient understand the context of the patch
better, which is done with this series.  If the submitter (or the
recipient) wants to incorporate the description from the notes to
update the proposed log message, it can easily be done by editing
the output of "format-patch --notes" before sending it out (or
before applying it with "am").

That does not mean that the recipient should not use notes for his
own purpose on the resulting commit, by the way.  It would be a
convenient feature to prime the contents of such a new note the
recipient creates on the resulting commit from the comment after
"---" before the diffstat or "diff --git" line.  Note that (no pun
intended) that additional comment does not have to originate from
any notes in the submitter's repository.  If saving the additional
comments the submitter attached from the notes to the patch is
useful, it would equally be useful to save typed-in comments on the
patch that came from the submitter's fingers, not from the notes.

It is something you can do by inspecting $dotest/patch file in your
post-applypatch hook with today's git.  If many people use and find
such a feature desirable, we could add inbuilt support for it, but I
do not think we are there yet.

^ permalink raw reply

* [PATCH v2 7/7] format-patch tests: check quoting/encoding in To: and Cc: headers
From: Jan H. Schönherr @ 2012-10-18 14:43 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, Jan H. Schönherr
In-Reply-To: <1350571414-17907-1-git-send-email-schnhrr@cs.tu-berlin.de>

From: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>

git-format-patch does currently not parse user supplied extra header
values (e. g., --cc, --add-header) and just replays them. That forces
users to add them RFC 2822/2047 conform in encoded form, e. g.

--cc '=?UTF-8?q?Jan=20H=2E=20Sch=C3=B6nherr?= <...>'

which is inconvenient. We would want to update git-format-patch to
accept human-readable input

--cc 'Jan H. Schönherr <...>'

and handle the encoding, wrapping and quoting internally in the future,
similar to what is already done in git-send-email. The necessary code
should mostly exist in the code paths that handle the From: and Subject:
headers.

Whether we want to do this only for the git-format-patch options
--to and --cc (and the corresponding config options) or also for
user supplied headers via --add-header, is open for discussion.

For now, add test_expect_failure tests for To: and Cc: headers as a
reminder and fix tests that would otherwise fail should this get
implemented.

Signed-off-by: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
---
This patch is RFC material. There are a few reasons, why this
is a good idea and also a few, why it is bad:

Pro:
- current git-format-patch behavior differs from git-send-email
- we should be able to use the address format that git uses
  elsewhere (e. g., author and committer info)
- necessary code mostly exists

Con:
- changes current behavior
- make code more complex

(Feel free to add more.)

The first drawback can be mitigated by checking whether the
input is already properly encoded, so that we do not accidentally
double-encode things. git-send-email does that, but that's written
in Perl, so we would need even more code.

For now, this is only about _addresses_ supplied to git-format-patch,
not _headers_. We could also validate/encode/wrap user supplied headers.
RFC 2822/2047 is specific enough to allow that. But there is no point
thinking about that without the intention of encoding addresses.

v2:
- updated commit message as suggested by Junio C Hamano
---
 t/t4014-format-patch.sh | 98 +++++++++++++++++++++++++++++++++----------------
 1 Datei geändert, 66 Zeilen hinzugefügt(+), 32 Zeilen entfernt(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index e024eb8..ad9f69e 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -110,73 +110,107 @@ test_expect_success 'replay did not screw up the log message' '
 
 test_expect_success 'extra headers' '
 
-	git config format.headers "To: R. E. Cipient <rcipient@example.com>
+	git config format.headers "To: R E Cipient <rcipient@example.com>
 " &&
-	git config --add format.headers "Cc: S. E. Cipient <scipient@example.com>
+	git config --add format.headers "Cc: S E Cipient <scipient@example.com>
 " &&
 	git format-patch --stdout master..side > patch2 &&
 	sed -e "/^\$/q" patch2 > hdrs2 &&
-	grep "^To: R. E. Cipient <rcipient@example.com>\$" hdrs2 &&
-	grep "^Cc: S. E. Cipient <scipient@example.com>\$" hdrs2
+	grep "^To: R E Cipient <rcipient@example.com>\$" hdrs2 &&
+	grep "^Cc: S E Cipient <scipient@example.com>\$" hdrs2
 
 '
 
 test_expect_success 'extra headers without newlines' '
 
-	git config --replace-all format.headers "To: R. E. Cipient <rcipient@example.com>" &&
-	git config --add format.headers "Cc: S. E. Cipient <scipient@example.com>" &&
+	git config --replace-all format.headers "To: R E Cipient <rcipient@example.com>" &&
+	git config --add format.headers "Cc: S E Cipient <scipient@example.com>" &&
 	git format-patch --stdout master..side >patch3 &&
 	sed -e "/^\$/q" patch3 > hdrs3 &&
-	grep "^To: R. E. Cipient <rcipient@example.com>\$" hdrs3 &&
-	grep "^Cc: S. E. Cipient <scipient@example.com>\$" hdrs3
+	grep "^To: R E Cipient <rcipient@example.com>\$" hdrs3 &&
+	grep "^Cc: S E Cipient <scipient@example.com>\$" hdrs3
 
 '
 
 test_expect_success 'extra headers with multiple To:s' '
 
-	git config --replace-all format.headers "To: R. E. Cipient <rcipient@example.com>" &&
-	git config --add format.headers "To: S. E. Cipient <scipient@example.com>" &&
+	git config --replace-all format.headers "To: R E Cipient <rcipient@example.com>" &&
+	git config --add format.headers "To: S E Cipient <scipient@example.com>" &&
 	git format-patch --stdout master..side > patch4 &&
 	sed -e "/^\$/q" patch4 > hdrs4 &&
-	grep "^To: R. E. Cipient <rcipient@example.com>,\$" hdrs4 &&
-	grep "^ *S. E. Cipient <scipient@example.com>\$" hdrs4
+	grep "^To: R E Cipient <rcipient@example.com>,\$" hdrs4 &&
+	grep "^ *S E Cipient <scipient@example.com>\$" hdrs4
 '
 
-test_expect_success 'additional command line cc' '
+test_expect_success 'additional command line cc (ascii)' '
 
-	git config --replace-all format.headers "Cc: R. E. Cipient <rcipient@example.com>" &&
+	git config --replace-all format.headers "Cc: R E Cipient <rcipient@example.com>" &&
+	git format-patch --cc="S E Cipient <scipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch5 &&
+	grep "^Cc: R E Cipient <rcipient@example.com>,\$" patch5 &&
+	grep "^ *S E Cipient <scipient@example.com>\$" patch5
+'
+
+test_expect_failure 'additional command line cc (rfc822)' '
+
+	git config --replace-all format.headers "Cc: R E Cipient <rcipient@example.com>" &&
 	git format-patch --cc="S. E. Cipient <scipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch5 &&
-	grep "^Cc: R. E. Cipient <rcipient@example.com>,\$" patch5 &&
-	grep "^ *S. E. Cipient <scipient@example.com>\$" patch5
+	grep "^Cc: R E Cipient <rcipient@example.com>,\$" patch5 &&
+	grep "^ *"S. E. Cipient" <scipient@example.com>\$" patch5
 '
 
 test_expect_success 'command line headers' '
 
 	git config --unset-all format.headers &&
-	git format-patch --add-header="Cc: R. E. Cipient <rcipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch6 &&
-	grep "^Cc: R. E. Cipient <rcipient@example.com>\$" patch6
+	git format-patch --add-header="Cc: R E Cipient <rcipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch6 &&
+	grep "^Cc: R E Cipient <rcipient@example.com>\$" patch6
 '
 
 test_expect_success 'configuration headers and command line headers' '
 
-	git config --replace-all format.headers "Cc: R. E. Cipient <rcipient@example.com>" &&
-	git format-patch --add-header="Cc: S. E. Cipient <scipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch7 &&
-	grep "^Cc: R. E. Cipient <rcipient@example.com>,\$" patch7 &&
-	grep "^ *S. E. Cipient <scipient@example.com>\$" patch7
+	git config --replace-all format.headers "Cc: R E Cipient <rcipient@example.com>" &&
+	git format-patch --add-header="Cc: S E Cipient <scipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch7 &&
+	grep "^Cc: R E Cipient <rcipient@example.com>,\$" patch7 &&
+	grep "^ *S E Cipient <scipient@example.com>\$" patch7
 '
 
-test_expect_success 'command line To: header' '
+test_expect_success 'command line To: header (ascii)' '
 
 	git config --unset-all format.headers &&
+	git format-patch --to="R E Cipient <rcipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch8 &&
+	grep "^To: R E Cipient <rcipient@example.com>\$" patch8
+'
+
+test_expect_failure 'command line To: header (rfc822)' '
+
 	git format-patch --to="R. E. Cipient <rcipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch8 &&
-	grep "^To: R. E. Cipient <rcipient@example.com>\$" patch8
+	grep "^To: "R. E. Cipient" <rcipient@example.com>\$" patch8
+'
+
+test_expect_failure 'command line To: header (rfc2047)' '
+
+	git format-patch --to="R Ä Cipient <rcipient@example.com>" --stdout master..side | sed -e "/^\$/q" >patch8 &&
+	grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" patch8
 '
 
-test_expect_success 'configuration To: header' '
+test_expect_success 'configuration To: header (ascii)' '
+
+	git config format.to "R E Cipient <rcipient@example.com>" &&
+	git format-patch --stdout master..side | sed -e "/^\$/q" >patch9 &&
+	grep "^To: R E Cipient <rcipient@example.com>\$" patch9
+'
+
+test_expect_failure 'configuration To: header (rfc822)' '
 
 	git config format.to "R. E. Cipient <rcipient@example.com>" &&
 	git format-patch --stdout master..side | sed -e "/^\$/q" >patch9 &&
-	grep "^To: R. E. Cipient <rcipient@example.com>\$" patch9
+	grep "^To: "R. E. Cipient" <rcipient@example.com>\$" patch9
+'
+
+test_expect_failure 'configuration To: header (rfc2047)' '
+
+	git config format.to "R Ä Cipient <rcipient@example.com>" &&
+	git format-patch --stdout master..side | sed -e "/^\$/q" >patch9 &&
+	grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" patch9
 '
 
 # check_patch <patch>: Verify that <patch> looks like a half-sane
@@ -190,11 +224,11 @@ check_patch () {
 test_expect_success '--no-to overrides config.to' '
 
 	git config --replace-all format.to \
-		"R. E. Cipient <rcipient@example.com>" &&
+		"R E Cipient <rcipient@example.com>" &&
 	git format-patch --no-to --stdout master..side |
 	sed -e "/^\$/q" >patch10 &&
 	check_patch patch10 &&
-	! grep "^To: R. E. Cipient <rcipient@example.com>\$" patch10
+	! grep "^To: R E Cipient <rcipient@example.com>\$" patch10
 '
 
 test_expect_success '--no-to and --to replaces config.to' '
@@ -212,21 +246,21 @@ test_expect_success '--no-to and --to replaces config.to' '
 test_expect_success '--no-cc overrides config.cc' '
 
 	git config --replace-all format.cc \
-		"C. E. Cipient <rcipient@example.com>" &&
+		"C E Cipient <rcipient@example.com>" &&
 	git format-patch --no-cc --stdout master..side |
 	sed -e "/^\$/q" >patch12 &&
 	check_patch patch12 &&
-	! grep "^Cc: C. E. Cipient <rcipient@example.com>\$" patch12
+	! grep "^Cc: C E Cipient <rcipient@example.com>\$" patch12
 '
 
 test_expect_success '--no-add-header overrides config.headers' '
 
 	git config --replace-all format.headers \
-		"Header1: B. E. Cipient <rcipient@example.com>" &&
+		"Header1: B E Cipient <rcipient@example.com>" &&
 	git format-patch --no-add-header --stdout master..side |
 	sed -e "/^\$/q" >patch13 &&
 	check_patch patch13 &&
-	! grep "^Header1: B. E. Cipient <rcipient@example.com>\$" patch13
+	! grep "^Header1: B E Cipient <rcipient@example.com>\$" patch13
 '
 
 test_expect_success 'multiple files' '
-- 
1.7.12

^ permalink raw reply related

* [PATCH v2 6/7] format-patch: fix rfc2047 address encoding with respect to rfc822 specials
From: Jan H. Schönherr @ 2012-10-18 14:43 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, Jan H. Schönherr
In-Reply-To: <1350571414-17907-1-git-send-email-schnhrr@cs.tu-berlin.de>

From: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>

According to RFC 2047 and RFC 822, rfc2047 encoded words and and rfc822
quoted strings do not mix. Since add_rfc2047() no longer leaves RFC 822
specials behind, the quoting is also no longer necessary to create a
standard-conform mail.

Remove the quoting, when RFC 2047 encoding takes place. This actually
requires to refactor add_rfc2047() a bit, so that the different cases
can be distinguished.

With this patch, my own name gets correctly decoded as Jan H. Schönherr
(without quotes) and not as "Jan H. Schönherr" (with quotes).

Signed-off-by: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
---
v2:
- part of restructured patch 4 of v1
- use constants for both, the 76 and 78 char limit
- select correct maximum length for possible final folding
- removed off-by-one correction now handled by first patch
---
 pretty.c                | 49 ++++++++++++++++++++++++++++++++-----------------
 t/t4014-format-patch.sh |  2 +-
 2 Dateien geändert, 33 Zeilen hinzugefügt(+), 18 Zeilen entfernt(-)

diff --git a/pretty.c b/pretty.c
index 613e4ea..413e758 100644
--- a/pretty.c
+++ b/pretty.c
@@ -231,7 +231,7 @@ static int is_rfc822_special(char ch)
 	}
 }
 
-static int has_rfc822_specials(const char *s, int len)
+static int needs_rfc822_quoting(const char *s, int len)
 {
 	int i;
 	for (i = 0; i < len; i++)
@@ -329,25 +329,29 @@ static int is_rfc2047_special(char ch, enum rfc2047_type type)
 	return !(isalnum(ch) || ch == '!' || ch == '*' || ch == '+' || ch == '-' || ch == '/');
 }
 
-static void add_rfc2047(struct strbuf *sb, const char *line, int len,
-		       const char *encoding, enum rfc2047_type type)
+static int needs_rfc2047_encoding(const char *line, int len,
+				  enum rfc2047_type type)
 {
-	static const int max_length = 78; /* per rfc2822 */
-	static const int max_encoded_length = 76; /* per rfc2047 */
 	int i;
-	int line_len = last_line_length(sb);
 
 	for (i = 0; i < len; i++) {
 		int ch = line[i];
 		if (non_ascii(ch) || ch == '\n')
-			goto needquote;
+			return 1;
 		if ((i + 1 < len) && (ch == '=' && line[i+1] == '?'))
-			goto needquote;
+			return 1;
 	}
-	strbuf_add_wrapped_bytes(sb, line, len, -line_len, 1, max_length);
-	return;
 
-needquote:
+	return 0;
+}
+
+static void add_rfc2047(struct strbuf *sb, const char *line, int len,
+		       const char *encoding, enum rfc2047_type type)
+{
+	static const int max_encoded_length = 76; /* per rfc2047 */
+	int i;
+	int line_len = last_line_length(sb);
+
 	strbuf_grow(sb, len * 3 + strlen(encoding) + 100);
 	strbuf_addf(sb, "=?%s?q?", encoding);
 	line_len += strlen(encoding) + 5; /* 5 for =??q? */
@@ -383,6 +387,7 @@ void pp_user_info(const struct pretty_print_context *pp,
 		  const char *what, struct strbuf *sb,
 		  const char *line, const char *encoding)
 {
+	int max_length = 78; /* per rfc2822 */
 	char *date;
 	int namelen;
 	unsigned long time;
@@ -406,17 +411,21 @@ void pp_user_info(const struct pretty_print_context *pp,
 			name_tail--;
 		display_name_length = name_tail - line;
 		strbuf_addstr(sb, "From: ");
-		if (!has_rfc822_specials(line, display_name_length)) {
+		if (needs_rfc2047_encoding(line, display_name_length, RFC2047_ADDRESS)) {
 			add_rfc2047(sb, line, display_name_length,
 						encoding, RFC2047_ADDRESS);
-		} else {
+			max_length = 76; /* per rfc2047 */
+		} else if (needs_rfc822_quoting(line, display_name_length)) {
 			struct strbuf quoted = STRBUF_INIT;
 			add_rfc822_quoted(&quoted, line, display_name_length);
-			add_rfc2047(sb, quoted.buf, quoted.len,
-						encoding, RFC2047_ADDRESS);
+			strbuf_add_wrapped_bytes(sb, quoted.buf, quoted.len,
+							-6, 1, max_length);
 			strbuf_release(&quoted);
+		} else {
+			strbuf_add_wrapped_bytes(sb, line, display_name_length,
+							-6, 1, max_length);
 		}
-		if (namelen - display_name_length + last_line_length(sb) > 78) {
+		if (namelen - display_name_length + last_line_length(sb) > max_length) {
 			strbuf_addch(sb, '\n');
 			if (!isspace(name_tail[0]))
 				strbuf_addch(sb, ' ');
@@ -1336,6 +1345,7 @@ void pp_title_line(const struct pretty_print_context *pp,
 		   const char *encoding,
 		   int need_8bit_cte)
 {
+	static const int max_length = 78; /* per rfc2047 */
 	struct strbuf title;
 
 	strbuf_init(&title, 80);
@@ -1345,7 +1355,12 @@ void pp_title_line(const struct pretty_print_context *pp,
 	strbuf_grow(sb, title.len + 1024);
 	if (pp->subject) {
 		strbuf_addstr(sb, pp->subject);
-		add_rfc2047(sb, title.buf, title.len, encoding, RFC2047_SUBJECT);
+		if (needs_rfc2047_encoding(title.buf, title.len, RFC2047_SUBJECT))
+			add_rfc2047(sb, title.buf, title.len,
+						encoding, RFC2047_SUBJECT);
+		else
+			strbuf_add_wrapped_bytes(sb, title.buf, title.len,
+					 -last_line_length(sb), 1, max_length);
 	} else {
 		strbuf_addbuf(sb, &title);
 	}
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 727d606..e024eb8 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -839,7 +839,7 @@ test_expect_success 'format-patch uses rfc2047-encoded from-headers when necessa
 cat >expect <<'EOF'
 From: =?UTF-8?q?F=C3=B6o=20B=2E=20Bar?= <author@example.com>
 EOF
-test_expect_failure 'rfc2047-encoded from-headers leave no rfc822 specials' '
+test_expect_success 'rfc2047-encoded from-headers leave no rfc822 specials' '
 	check_author "Föo B. Bar"
 '
 
-- 
1.7.12

^ permalink raw reply related

* [PATCH v2 5/7] format-patch: make rfc2047 encoding more strict
From: Jan H. Schönherr @ 2012-10-18 14:43 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, Jan H. Schönherr
In-Reply-To: <1350571414-17907-1-git-send-email-schnhrr@cs.tu-berlin.de>

From: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>

RFC 2047 requires more characters to be encoded than it is currently done.
Especially, RFC 2047 distinguishes between allowed remaining characters
in encoded words in addresses (From, To, etc.) and other headers, such
as Subject.

Make add_rfc2047() and is_rfc2047_special() location dependent and include
all non-allowed characters to hopefully be RFC 2047 conform.

This especially fixes a problem, where RFC 822 specials (e. g. ".") were
left unencoded in addresses, which was solved with a non-standard-conform
workaround in the past (which is going to be removed in a follow-up patch).

Signed-off-by: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
---
v2:
- part of restructured patch 4 of v1
- disallow even more characters in is_rfc2047_special()

The implementation of isprint() should later probably be substituted by
the one from Nguyen:
http://article.gmane.org/gmane.comp.version-control.git/207666
---
 git-compat-util.h       |  2 ++
 pretty.c                | 67 +++++++++++++++++++++++++++++++++++++++++++------
 t/t4014-format-patch.sh | 15 ++++++++---
 3 Dateien geändert, 72 Zeilen hinzugefügt(+), 12 Zeilen entfernt(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 000042d..d4ea446 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -475,6 +475,7 @@ extern const char tolower_trans_tbl[256];
 #undef isdigit
 #undef isalpha
 #undef isalnum
+#undef isprint
 #undef islower
 #undef isupper
 #undef tolower
@@ -492,6 +493,7 @@ extern unsigned char sane_ctype[256];
 #define isdigit(x) sane_istest(x,GIT_DIGIT)
 #define isalpha(x) sane_istest(x,GIT_ALPHA)
 #define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
+#define isprint(x) ((x) >= 0x20 && (x) <= 0x7e)
 #define islower(x) sane_iscase(x, 1)
 #define isupper(x) sane_iscase(x, 0)
 #define is_glob_special(x) sane_istest(x,GIT_GLOB_SPECIAL)
diff --git a/pretty.c b/pretty.c
index 482402d..613e4ea 100644
--- a/pretty.c
+++ b/pretty.c
@@ -272,16 +272,65 @@ static void add_rfc822_quoted(struct strbuf *out, const char *s, int len)
 	strbuf_addch(out, '"');
 }
 
-static int is_rfc2047_special(char ch)
+enum rfc2047_type {
+	RFC2047_SUBJECT,
+	RFC2047_ADDRESS,
+};
+
+static int is_rfc2047_special(char ch, enum rfc2047_type type)
 {
-	if (ch == ' ' || ch == '\n')
+	/*
+	 * rfc2047, section 4.2:
+	 *
+	 *    8-bit values which correspond to printable ASCII characters other
+	 *    than "=", "?", and "_" (underscore), MAY be represented as those
+	 *    characters.  (But see section 5 for restrictions.)  In
+	 *    particular, SPACE and TAB MUST NOT be represented as themselves
+	 *    within encoded words.
+	 */
+
+	/*
+	 * rule out non-ASCII characters and non-printable characters (the
+	 * non-ASCII check should be redundant as isprint() is not localized
+	 * and only knows about ASCII, but be defensive about that)
+	 */
+	if (non_ascii(ch) || !isprint(ch))
+		return 1;
+
+	/*
+	 * rule out special printable characters (' ' should be the only
+	 * whitespace character considered printable, but be defensive and use
+	 * isspace())
+	 */
+	if (isspace(ch) || ch == '=' || ch == '?' || ch == '_')
 		return 1;
 
-	return (non_ascii(ch) || (ch == '=') || (ch == '?') || (ch == '_'));
+	/*
+	 * rfc2047, section 5.3:
+	 *
+	 *    As a replacement for a 'word' entity within a 'phrase', for example,
+	 *    one that precedes an address in a From, To, or Cc header.  The ABNF
+	 *    definition for 'phrase' from RFC 822 thus becomes:
+	 *
+	 *    phrase = 1*( encoded-word / word )
+	 *
+	 *    In this case the set of characters that may be used in a "Q"-encoded
+	 *    'encoded-word' is restricted to: <upper and lower case ASCII
+	 *    letters, decimal digits, "!", "*", "+", "-", "/", "=", and "_"
+	 *    (underscore, ASCII 95.)>.  An 'encoded-word' that appears within a
+	 *    'phrase' MUST be separated from any adjacent 'word', 'text' or
+	 *    'special' by 'linear-white-space'.
+	 */
+
+	if (type != RFC2047_ADDRESS)
+		return 0;
+
+	/* '=' and '_' are special cases and have been checked above */
+	return !(isalnum(ch) || ch == '!' || ch == '*' || ch == '+' || ch == '-' || ch == '/');
 }
 
 static void add_rfc2047(struct strbuf *sb, const char *line, int len,
-		       const char *encoding)
+		       const char *encoding, enum rfc2047_type type)
 {
 	static const int max_length = 78; /* per rfc2822 */
 	static const int max_encoded_length = 76; /* per rfc2047 */
@@ -304,7 +353,7 @@ needquote:
 	line_len += strlen(encoding) + 5; /* 5 for =??q? */
 	for (i = 0; i < len; i++) {
 		unsigned ch = line[i] & 0xFF;
-		int is_special = is_rfc2047_special(ch);
+		int is_special = is_rfc2047_special(ch, type);
 
 		/*
 		 * According to RFC 2047, we could encode the special character
@@ -358,11 +407,13 @@ void pp_user_info(const struct pretty_print_context *pp,
 		display_name_length = name_tail - line;
 		strbuf_addstr(sb, "From: ");
 		if (!has_rfc822_specials(line, display_name_length)) {
-			add_rfc2047(sb, line, display_name_length, encoding);
+			add_rfc2047(sb, line, display_name_length,
+						encoding, RFC2047_ADDRESS);
 		} else {
 			struct strbuf quoted = STRBUF_INIT;
 			add_rfc822_quoted(&quoted, line, display_name_length);
-			add_rfc2047(sb, quoted.buf, quoted.len, encoding);
+			add_rfc2047(sb, quoted.buf, quoted.len,
+						encoding, RFC2047_ADDRESS);
 			strbuf_release(&quoted);
 		}
 		if (namelen - display_name_length + last_line_length(sb) > 78) {
@@ -1294,7 +1345,7 @@ void pp_title_line(const struct pretty_print_context *pp,
 	strbuf_grow(sb, title.len + 1024);
 	if (pp->subject) {
 		strbuf_addstr(sb, pp->subject);
-		add_rfc2047(sb, title.buf, title.len, encoding);
+		add_rfc2047(sb, title.buf, title.len, encoding, RFC2047_SUBJECT);
 	} else {
 		strbuf_addbuf(sb, &title);
 	}
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 1d5636d..727d606 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -818,21 +818,28 @@ check_author() {
 cat >expect <<'EOF'
 From: "Foo B. Bar" <author@example.com>
 EOF
-test_expect_success 'format-patch quotes dot in headers' '
+test_expect_success 'format-patch quotes dot in from-headers' '
 	check_author "Foo B. Bar"
 '
 
 cat >expect <<'EOF'
 From: "Foo \"The Baz\" Bar" <author@example.com>
 EOF
-test_expect_success 'format-patch quotes double-quote in headers' '
+test_expect_success 'format-patch quotes double-quote in from-headers' '
 	check_author "Foo \"The Baz\" Bar"
 '
 
 cat >expect <<'EOF'
-From: =?UTF-8?q?"F=C3=B6o=20B.=20Bar"?= <author@example.com>
+From: =?UTF-8?q?F=C3=B6o=20Bar?= <author@example.com>
 EOF
-test_expect_success 'rfc2047-encoded headers also double-quote 822 specials' '
+test_expect_success 'format-patch uses rfc2047-encoded from-headers when necessary' '
+	check_author "Föo Bar"
+'
+
+cat >expect <<'EOF'
+From: =?UTF-8?q?F=C3=B6o=20B=2E=20Bar?= <author@example.com>
+EOF
+test_expect_failure 'rfc2047-encoded from-headers leave no rfc822 specials' '
 	check_author "Föo B. Bar"
 '
 
-- 
1.7.12

^ permalink raw reply related

* [PATCH v2 4/7] format-patch: introduce helper function last_line_length()
From: Jan H. Schönherr @ 2012-10-18 14:43 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, Jan H. Schönherr
In-Reply-To: <1350571414-17907-1-git-send-email-schnhrr@cs.tu-berlin.de>

From: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>

Currently, an open-coded loop to calculate the length of the last
line of a string buffer is used in multiple places.

Move that code into a function of its own.

Signed-off-by: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
---
 pretty.c | 25 +++++++++++++------------
 1 Datei geändert, 13 Zeilen hinzugefügt(+), 12 Zeilen entfernt(-)

diff --git a/pretty.c b/pretty.c
index da75879..482402d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -240,6 +240,17 @@ static int has_rfc822_specials(const char *s, int len)
 	return 0;
 }
 
+static int last_line_length(struct strbuf *sb)
+{
+	int i;
+
+	/* How many bytes are already used on the last line? */
+	for (i = sb->len - 1; i >= 0; i--)
+		if (sb->buf[i] == '\n')
+			break;
+	return sb->len - (i + 1);
+}
+
 static void add_rfc822_quoted(struct strbuf *out, const char *s, int len)
 {
 	int i;
@@ -275,13 +286,7 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 	static const int max_length = 78; /* per rfc2822 */
 	static const int max_encoded_length = 76; /* per rfc2047 */
 	int i;
-	int line_len;
-
-	/* How many bytes are already used on the current line? */
-	for (i = sb->len - 1; i >= 0; i--)
-		if (sb->buf[i] == '\n')
-			break;
-	line_len = sb->len - (i+1);
+	int line_len = last_line_length(sb);
 
 	for (i = 0; i < len; i++) {
 		int ch = line[i];
@@ -346,7 +351,6 @@ void pp_user_info(const struct pretty_print_context *pp,
 	if (pp->fmt == CMIT_FMT_EMAIL) {
 		char *name_tail = strchr(line, '<');
 		int display_name_length;
-		int final_line;
 		if (!name_tail)
 			return;
 		while (line < name_tail && isspace(name_tail[-1]))
@@ -361,10 +365,7 @@ void pp_user_info(const struct pretty_print_context *pp,
 			add_rfc2047(sb, quoted.buf, quoted.len, encoding);
 			strbuf_release(&quoted);
 		}
-		for (final_line = 0; final_line < sb->len; final_line++)
-			if (sb->buf[sb->len - final_line - 1] == '\n')
-				break;
-		if (namelen - display_name_length + final_line > 78) {
+		if (namelen - display_name_length + last_line_length(sb) > 78) {
 			strbuf_addch(sb, '\n');
 			if (!isspace(name_tail[0]))
 				strbuf_addch(sb, ' ');
-- 
1.7.12

^ permalink raw reply related

* [PATCH v2 3/7] format-patch: do not wrap rfc2047 encoded headers too late
From: Jan H. Schönherr @ 2012-10-18 14:43 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, Jan H. Schönherr
In-Reply-To: <1350571414-17907-1-git-send-email-schnhrr@cs.tu-berlin.de>

From: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>

Encoded characters add more than one character at once to an encoded
header. Include all characters that are about to be added in the length
calculation for wrapping.

Additionally, RFC 2047 imposes a maximum line length of 76 characters
if that line contains an rfc2047 encoded word.

Signed-off-by: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
---
v2:
- use constants for both, the 76 and 78 char limit
- rephrase comment
---
 pretty.c                | 26 +++++++++++++---------
 t/t4014-format-patch.sh | 58 +++++++++++++++++++++++++++++--------------------
 2 Dateien geändert, 51 Zeilen hinzugefügt(+), 33 Zeilen entfernt(-)

diff --git a/pretty.c b/pretty.c
index 71e4024..da75879 100644
--- a/pretty.c
+++ b/pretty.c
@@ -263,6 +263,9 @@ static void add_rfc822_quoted(struct strbuf *out, const char *s, int len)
 
 static int is_rfc2047_special(char ch)
 {
+	if (ch == ' ' || ch == '\n')
+		return 1;
+
 	return (non_ascii(ch) || (ch == '=') || (ch == '?') || (ch == '_'));
 }
 
@@ -270,6 +273,7 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 		       const char *encoding)
 {
 	static const int max_length = 78; /* per rfc2822 */
+	static const int max_encoded_length = 76; /* per rfc2047 */
 	int i;
 	int line_len;
 
@@ -295,23 +299,25 @@ needquote:
 	line_len += strlen(encoding) + 5; /* 5 for =??q? */
 	for (i = 0; i < len; i++) {
 		unsigned ch = line[i] & 0xFF;
+		int is_special = is_rfc2047_special(ch);
+
+		/*
+		 * According to RFC 2047, we could encode the special character
+		 * ' ' (space) with '_' (underscore) for readability. But many
+		 * programs do not understand this and just leave the
+		 * underscore in place. Thus, we do nothing special here, which
+		 * causes ' ' to be encoded as '=20', avoiding this problem.
+		 */
 
-		if (line_len >= max_length - 2) {
+		if (line_len + 2 + (is_special ? 3 : 1) > max_encoded_length) {
 			strbuf_addf(sb, "?=\n =?%s?q?", encoding);
 			line_len = strlen(encoding) + 5 + 1; /* =??q? plus SP */
 		}
 
-		/*
-		 * We encode ' ' using '=20' even though rfc2047
-		 * allows using '_' for readability.  Unfortunately,
-		 * many programs do not understand this and just
-		 * leave the underscore in place.
-		 */
-		if (is_rfc2047_special(ch) || ch == ' ' || ch == '\n') {
+		if (is_special) {
 			strbuf_addf(sb, "=%02X", ch);
 			line_len += 3;
-		}
-		else {
+		} else {
 			strbuf_addch(sb, ch);
 			line_len++;
 		}
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index d66e358..1d5636d 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -772,30 +772,31 @@ M8="föö bar "
 M64=$M8$M8$M8$M8$M8$M8$M8$M8
 M512=$M64$M64$M64$M64$M64$M64$M64$M64
 cat >expect <<'EOF'
-Subject: [PATCH] =?UTF-8?q?f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
- =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+Subject: [PATCH] =?UTF-8?q?f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
+ =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
+ =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
+ =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
+ =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
+ =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
+ =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
+ =?UTF-8?q?=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20?=
+ =?UTF-8?q?bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6?=
+ =?UTF-8?q?=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3?=
+ =?UTF-8?q?=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6?=
+ =?UTF-8?q?=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3?=
+ =?UTF-8?q?=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar=20f?=
+ =?UTF-8?q?=C3=B6=C3=B6=20bar=20f=C3=B6=C3=B6=20bar?=
 EOF
-test_expect_success 'format-patch wraps extremely long headers (rfc2047)' '
+test_expect_success 'format-patch wraps extremely long subject (rfc2047)' '
 	rm -rf patches/ &&
 	echo content >>file &&
 	git add file &&
@@ -862,6 +863,17 @@ test_expect_success 'format-patch wraps extremely long from-header (rfc822)' '
 '
 
 cat >expect <<'EOF'
+From: =?UTF-8?q?Fo=C3=B6=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo?=
+ =?UTF-8?q?=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20?=
+ =?UTF-8?q?Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar?=
+ =?UTF-8?q?=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20Foo=20Bar=20?=
+ =?UTF-8?q?Foo=20Bar=20Foo=20Bar?= <author@example.com>
+EOF
+test_expect_success 'format-patch wraps extremely long from-header (rfc2047)' '
+	check_author "Foö Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar"
+'
+
+cat >expect <<'EOF'
 Subject: header with . in it
 EOF
 test_expect_success 'subject lines do not have 822 atom-quoting' '
-- 
1.7.12

^ permalink raw reply related

* [PATCH v2 2/7] format-patch: do not wrap non-rfc2047 headers too early
From: Jan H. Schönherr @ 2012-10-18 14:43 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, Jan H. Schönherr
In-Reply-To: <1350571414-17907-1-git-send-email-schnhrr@cs.tu-berlin.de>

From: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>

Do not wrap the second and later lines of non-rfc2047-encoded headers
substantially before the 78 character limit.

Instead of passing the remaining length of the first line as wrapping
width, use the correct maximum length and tell strbuf_add_wrapped_bytes()
how many characters of the first line are already used.

Signed-off-by: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
---
v2:
- removed off-by-one correction now handled by first patch
- commit message clarifications
---
 pretty.c                |  2 +-
 t/t4014-format-patch.sh | 60 ++++++++++++++++++++++++++++---------------------
 2 Dateien geändert, 35 Zeilen hinzugefügt(+), 27 Zeilen entfernt(-)

diff --git a/pretty.c b/pretty.c
index 8b1ea9f..71e4024 100644
--- a/pretty.c
+++ b/pretty.c
@@ -286,7 +286,7 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
 		if ((i + 1 < len) && (ch == '=' && line[i+1] == '?'))
 			goto needquote;
 	}
-	strbuf_add_wrapped_bytes(sb, line, len, 0, 1, max_length - line_len);
+	strbuf_add_wrapped_bytes(sb, line, len, -line_len, 1, max_length);
 	return;
 
 needquote:
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 959aa26..d66e358 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -752,16 +752,14 @@ M64=$M8$M8$M8$M8$M8$M8$M8$M8
 M512=$M64$M64$M64$M64$M64$M64$M64$M64
 cat >expect <<'EOF'
 Subject: [PATCH] foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo
- bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
- foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo
- bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
- foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo
- bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
- foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo
- bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
- foo bar foo bar foo bar foo bar
+ bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
+ foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo
+ bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
+ foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo
+ bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
+ foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar foo bar
 EOF
-test_expect_success 'format-patch wraps extremely long headers (ascii)' '
+test_expect_success 'format-patch wraps extremely long subject (ascii)' '
 	echo content >>file &&
 	git add file &&
 	git commit -m "$M512" &&
@@ -807,28 +805,12 @@ test_expect_success 'format-patch wraps extremely long headers (rfc2047)' '
 	test_cmp expect subject
 '
 
-M8="foo_bar_"
-M64=$M8$M8$M8$M8$M8$M8$M8$M8
-cat >expect <<EOF
-From: $M64
- <foobar@foo.bar>
-EOF
-test_expect_success 'format-patch wraps non-quotable headers' '
-	rm -rf patches/ &&
-	echo content >>file &&
-	git add file &&
-	git commit -mfoo --author "$M64 <foobar@foo.bar>" &&
-	git format-patch --stdout -1 >patch &&
-	sed -n "/^From: /p; /^ /p; /^$/q" <patch >from &&
-	test_cmp expect from
-'
-
 check_author() {
 	echo content >>file &&
 	git add file &&
 	GIT_AUTHOR_NAME=$1 git commit -m author-check &&
 	git format-patch --stdout -1 >patch &&
-	grep ^From: patch >actual &&
+	sed -n "/^From: /p; /^ /p; /^$/q" <patch >actual &&
 	test_cmp expect actual
 }
 
@@ -853,6 +835,32 @@ test_expect_success 'rfc2047-encoded headers also double-quote 822 specials' '
 	check_author "Föo B. Bar"
 '
 
+cat >expect <<EOF
+From: foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_
+ <author@example.com>
+EOF
+test_expect_success 'format-patch wraps moderately long from-header (ascii)' '
+	check_author "foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_"
+'
+
+cat >expect <<'EOF'
+From: Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar
+ Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo
+ Bar Foo Bar Foo Bar Foo Bar <author@example.com>
+EOF
+test_expect_success 'format-patch wraps extremely long from-header (ascii)' '
+	check_author "Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar"
+'
+
+cat >expect <<'EOF'
+From: "Foo.Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar
+ Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo
+ Bar Foo Bar Foo Bar Foo Bar" <author@example.com>
+EOF
+test_expect_success 'format-patch wraps extremely long from-header (rfc822)' '
+	check_author "Foo.Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar Foo Bar"
+'
+
 cat >expect <<'EOF'
 Subject: header with . in it
 EOF
-- 
1.7.12

^ permalink raw reply related

* [PATCH v2 1/7] utf8: fix off-by-one wrapping of text
From: Jan H. Schönherr @ 2012-10-18 14:43 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, Jan H. Schönherr
In-Reply-To: <1350571414-17907-1-git-send-email-schnhrr@cs.tu-berlin.de>

From: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>

The wrapping logic in strbuf_add_wrapped_text() does currently not allow
lines that entirely fill the allowed width, instead it wraps the line one
character too early.

For example, the text "This is the sixth commit." formatted via
"%w(11,1,2)" (wrap at 11 characters, 1 char indent of first line, 2 char
indent of following lines) results in four lines: " This is", "  the",
"  sixth", "  commit." This is wrong, because "  the sixth" is exactly
11 characters long, and thus allowed.

Fix this by allowing the (width+1) character of a line to be a valid
wrapping point if it is a whitespace character.

Signed-off-by: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
---
v2: new patch, result of v1 discussion
---
 t/t4202-log.sh | 4 ++--
 utf8.c         | 2 +-
 2 Dateien geändert, 3 Zeilen hinzugefügt(+), 3 Zeilen entfernt(-)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index b3ac6be..584e3d8 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -72,9 +72,9 @@ cat > expect << EOF
   commit.
 EOF
 
-test_expect_success 'format %w(12,1,2)' '
+test_expect_success 'format %w(11,1,2)' '
 
-	git log -2 --format="%w(12,1,2)This is the %s commit." > actual &&
+	git log -2 --format="%w(11,1,2)This is the %s commit." > actual &&
 	test_cmp expect actual
 '
 
diff --git a/utf8.c b/utf8.c
index a544f15..28791a7 100644
--- a/utf8.c
+++ b/utf8.c
@@ -353,7 +353,7 @@ retry:
 
 		c = *text;
 		if (!c || isspace(c)) {
-			if (w < width || !space) {
+			if (w <= width || !space) {
 				const char *start = bol;
 				if (!c && text == start)
 					return w;
-- 
1.7.12

^ permalink raw reply related

* [PATCH v2 0/7] Cure some format-patch wrapping and encoding issues
From: Jan H. Schönherr @ 2012-10-18 14:43 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Junio C Hamano, Jan H. Schönherr

Hi all.

[This is the second version of this series. If you still remember
the first version, you might want to jump directly to the summary
of changes below.]

The main point of this series is to teach git to encode my name
correctly, see patches 5+6, so that the decoded version is actually
my name, so that send-email does not insist on adding a wrong
superfluous From: line to the mail body.

The other patches more mostly by-products that fix other issues
I came across.

Patch 1 fixes an old off-by-one error, so that wrapped text may
now use all available columns.

Patches 2 and 3 make the wrapping of header lines more correct,
i. e., neither too early nor too late.

Patch 4 does some refactoring, which is too unrelated to be included
in one of the later patches.

Patch 5 improves RFC 2047 encoding; patch 6 removes an old non-RFC
conform workaround.

Patch 7 is more an RFC, which seems to be a good idea from my point
of view. Indeed, I thought the current implementation is erroneous,
until Junio C Hamano pointed out, that this might be desired behavior.
Thus, make up your mind about this one.


The series is currently based on the maint branch, but it applies
to master as well. It does also apply to next, but then my
implementation of isprint() has to be dropped from patch 5.


Changes in v2:
- patch 1 is new and is a result of the v1 discussion
- patch 5+6 split the old patch 4 into two patches
- use of constants for maximum line lengths
- even better adherence to RFC 2047 than v1
- updated commit messages/comments


Regards
Jan

Jan H. Schönherr (7):
  utf8: fix off-by-one wrapping of text
  format-patch: do not wrap non-rfc2047 headers too early
  format-patch: do not wrap rfc2047 encoded headers too late
  format-patch: introduce helper function last_line_length()
  format-patch: make rfc2047 encoding more strict
  format-patch: fix rfc2047 address encoding with respect to rfc822
    specials
  format-patch tests: check quoting/encoding in To: and Cc: headers

 git-compat-util.h       |   2 +
 pretty.c                | 149 +++++++++++++++++++++++--------
 t/t4014-format-patch.sh | 231 ++++++++++++++++++++++++++++++------------------
 t/t4202-log.sh          |   4 +-
 utf8.c                  |   2 +-
 5 Dateien geändert, 262 Zeilen hinzugefügt(+), 126 Zeilen entfernt(-)

-- 
1.7.12

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox