* Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations
From: Jonathan Tan @ 2016-09-16 22:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff
In-Reply-To: <xmqqshszimrh.fsf@gitster.mtv.corp.google.com>
On 09/16/2016 01:59 PM, Junio C Hamano wrote:
> if (mi->in_line_header->len) {
> /* we have read the beginning of one in-line header */
> if (line->len && isspace(*line->buf) &&
> !(mi->use_scissors && is_scissors_line(line))) {
Minor note: this means that the scissors check appears twice in the
code, once here and once below (for the non-header case).
> append to mi->in_line_header strbuf;
> return 0;
> }
> /* otherwise we know mi->in_line_header is now complete */
> check_header(mi, mi->in_line_header, ...);
(Sorry - should have also noticed this in your original e-mail.)
I'm concerned about what happens if check_header fails - we would then
have some lines which need to be treated as log messages. (At least,
they are currently treated that way.)
To treat them as log messages, we would need to convert them into UTF-8,
which may possibly fail, so we would have to figure out how to clean up
(we have to clean up because we cannot `die` immediately, at least to
preserve the current behavior). Also, we are likely to detect such a
failure only while processing a subsequent line - this non-"fail fast"
currently is fine, but I'm concerned that it will hinder future
development (especially when debugging).
Minor note: the buffer would also need to be more complicated (instead
of the current single buffer), either:
o store newlines in that buffer (and we would need to remove all
newlines before passing to check_header), or
o 2 buffers: one with newlines (for log messages) and one without (for
check_header).
In light of the above (multiple scissors checks, late detection of
failure, more complicated buffer), it seems clearer to me to just change
the order of the checks (as in RFC/PATCH 1/3). This necessitates holding
on to the old un-decoded buf and len, but this seems easier to me than
the above.
> strbuf_reset(&mi->in_line_header);
> }
> ...
^ permalink raw reply
* [wishlist] disable boring messages
From: Alexander Inyukhin @ 2016-09-16 22:17 UTC (permalink / raw)
To: git
Hi,
is it possible to make git silent, when nothing interesting
is happening?
I have a lot of repos and a batch script to update them all,
and I want to get rid of 'Fetching origin' and 'Already up-to-date.'
messages leaving only new refs and tags.
^ permalink raw reply
* Re: [PATCH] mailinfo: unescape quoted-pair in header fields
From: Jeff King @ 2016-09-16 22:22 UTC (permalink / raw)
To: Kevin Daudt; +Cc: git, Swift Geek, Junio C Hamano
In-Reply-To: <20160916210204.31282-1-me@ikke.info>
On Fri, Sep 16, 2016 at 11:02:04PM +0200, Kevin Daudt wrote:
> rfc2822 has provisions for quoted strings in structured header fields,
> but also allows for escaping these with so-called quoted-pairs.
>
> The only thing git currently does is removing exterior quotes, but
> quotes within are left alone.
>
> Tell mailinfo to remove exterior quotes and remove escape characters from the
> author so that they don't show up in the commits author field.
>
> Signed-off-by: Kevin Daudt <me@ikke.info>
> ---
> The only thing I could not easily fix is the prevent git am from
> removing any quotes around the author. This is done in fmt_ident,
> which calls `strbuf_addstr_without_crud`.
Ah, OK. I was wondering where that stripping was being done. That makes
sense, and makes me doubly confident this is the right place to be doing
it, since the other quote-stripping was not even intentional, but just a
side effect of the low-level routines.
I think it is OK to leave it in place. If you really want your name to
be:
"My Name is Always in Quotes"
then tough luck. Git does not support it via git-am, but nor does it via
git-commit, etc.
> mailinfo.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++
> t/t5100-mailinfo.sh | 6 ++++++
> t/t5100/quoted-pair.expect | 5 +++++
> t/t5100/quoted-pair.in | 9 ++++++++
> 4 files changed, 74 insertions(+)
> create mode 100644 t/t5100/quoted-pair.expect
> create mode 100644 t/t5100/quoted-pair.in
>
> diff --git a/mailinfo.c b/mailinfo.c
> index e19abe3..04036f3 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -54,15 +54,69 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)
> get_sane_name(&mi->name, &mi->name, &mi->email);
> }
>
> +static int unquote_quoted_string(struct strbuf *line)
> +{
> + struct strbuf outbuf;
> + const char *in = line->buf;
> + int c, take_next_literally = 0;
> + int found_error = 0;
> + char escape_context=0;
Style: whitespace around "=".
I had to wonder why we needed both escape_context and
take_next_literally; shouldn't we just need a single state bit. But
escape_context is not "escape the next character", it is "we are
currently in a mode where we should be escaping".
Could we give it a more descriptive name? I guess it is more than just
"we are in a mode", but rather "here is the character that will end the
escaped mode". Maybe a comment would be more appropriate.
> + while ((c = *in++) != 0) {
> + if (take_next_literally) {
> + take_next_literally = 0;
> + } else {
OK, so that means the previous one was backslash-quoted, and we don't do
any other cleverness. Good.
> + switch (c) {
> + case '"':
> + if (!escape_context)
> + escape_context = '"';
> + else if (escape_context == '"')
> + escape_context = 0;
> + continue;
And here we open or close the quoted portion, depending. Makes sense.
> + case '\\':
> + if (escape_context) {
> + take_next_literally = 1;
> + continue;
> + }
> + break;
I didn't look in the RFC. Is:
From: my \"name\" <foo@example.com>
really the same as:
From: "my \\\"name\\\"" <foo@example.com>
? That seems weird, but I think it may be that the former is simply
bogus (you are not supposed to use backslashes outside of the quoted
section at all).
> + case '(':
> + if (!escape_context)
> + escape_context = '(';
> + else if (escape_context == '(')
> + found_error = 1;
> + break;
Hmm. Is:
From: Name (Comment with (another comment))
really disallowed? RFC2822 seems to say that "comment" can contain
"ccontent", which can itself be a comment.
This is obviously getting pretty silly, but if we are going to follow
the RFC, I think you actually have to do a recursive parse, and keep
track of an arbitrary depth of context.
I dunno. This method probably covers most cases in practice, and it's
easy to reason about.
> + case ')':
> + if (escape_context == '(')
> + escape_context = 0;
> + break;
> + }
> + }
> +
> + strbuf_addch(&outbuf, c);
> + }
> +
> + strbuf_reset(line);
> + strbuf_addbuf(line, &outbuf);
> + strbuf_release(&outbuf);
I think you can use strbuf_swap() here to avoid copying the line an
extra time, like:
strbuf_swap(line, &outbuf);
strbuf_release(&outbuf);
Another option would be to just:
in = strbuf_detach(&line);
at the beginning, and then output back into "line".
> + return found_error;
What happens when we get here and take_next_literally is set? I.e., a
backslash at the end of the string. We'll silently print nothing, which
seems reasonable to me (the other option is to print a literal
backslash).
Ditto, what if escape_context is non-zero? We're in the middle of an
unterminated quoted string (or comment).
I'm fine with silently continuing, but it seems weird that we notice
embedded comments (and return an error), but not these other conditions.
> static void handle_from(struct mailinfo *mi, const struct strbuf *from)
> {
> char *at;
> size_t el;
> struct strbuf f;
>
> +
> strbuf_init(&f, from->len);
> strbuf_addbuf(&f, from);
Funny extra line?
> +test_expect_success 'mailinfo unescapes rfc2822 quoted-string' '
> + mkdir quoted-pair &&
> + git mailinfo /dev/null /dev/null <"$TEST_DIRECTORY"/t5100/quoted-pair.in >quoted-pair/info &&
> + test_cmp "$TEST_DIRECTORY"/t5100/quoted-pair.expect quoted-pair/info
> +'
We usually break long lines with backslash-escapes. Like:
git mailinfo /dev/null /dev/null \
<"$TEST_DIRECTORY"/t5100/quoted-pair.in \
>quoted-pair/info
I'd also wonder if things might be made much more readable by putting
"$TEST_DIRECTORY/t5100" into a shorter variable like $data or something.
That would be best done as a preparatory patch which updates all of the
tests.
> --- /dev/null
> +++ b/t/t5100/quoted-pair.in
> @@ -0,0 +1,9 @@
> +From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001
> +From: "Author \"The Author\" Name" <somebody@example.com>
> +Date: Sun, 25 May 2008 00:38:18 -0700
> +Subject: [PATCH] testing quoted-pair
I do not care that much about the "()" comment behavior myself, but if
we are going to implement it, it probably makes sense to protect it from
regression with a test.
-Peff
^ permalink raw reply
* Re: [PATCH 03/11] pkt-line: create gentle packet_read_line functions
From: Junio C Hamano @ 2016-09-16 22:17 UTC (permalink / raw)
To: Kevin Wern; +Cc: git
In-Reply-To: <1473984742-12516-4-git-send-email-kevin.m.wern@gmail.com>
Kevin Wern <kevin.m.wern@gmail.com> writes:
> /* And complain if we didn't get enough bytes to satisfy the read. */
> if (ret < size) {
> - if (options & PACKET_READ_GENTLE_ON_EOF)
> + if (options & (PACKET_READ_GENTLE_ON_EOF | PACKET_READ_GENTLE_ALL))
> return -1;
The name _ALL suggested to me that there may be multiple "under this
condition, be gentle", "under that condition, be gentle", and _ALL
is used as a catch-all "under any condition, be gentle". If you
defined _ALL symbol to have all GENTLE bits on, this line could have
become
if (options & PACKET_READ_GENTLE_ALL)
> @@ -205,15 +209,23 @@ int packet_read(int fd, char **src_buf, size_t *src_len,
> if (ret < 0)
> return ret;
> len = packet_length(linelen);
> - if (len < 0)
> + if (len < 0) {
> + if (options & PACKET_READ_GENTLE_ALL)
> + return -1;
On the other hand, however, you do want to die here when only
GENTLE_ON_EOF is set.
Taking the above two observations together, I'd have to say that
_ALL is probably a misnomer. I agree with a need for a flag with
the behaviour you defined in this patch, though.
> die("protocol error: bad line length character: %.4s", linelen);
> static char *packet_read_line_generic(int fd,
> char **src, size_t *src_len,
> - int *dst_len)
> + int *dst_len, int flags)
The original one is called options, not flags, and it would be
easier to follow if it is consistently called options, instead of
requiring the reader to keep track of "ah, it is called flags here
but the callee renames it to options".
> +/*
> + * Same as packet_read_line, but does not die on any errors (uses
> + * PACKET_READ_GENTLE_ALL).
> + */
> +char *packet_read_line_gentle(int fd, int *len_p);
> +
> +/*
> + * Same as packet_read_line_buf, but does not die on any errors (uses
> + * PACKET_READ_GENTLE_ALL).
> + */
> +char *packet_read_line_buf_gentle(char **src_buf, size_t *src_len, int *size);
I think most if not all "do the same thing as do_something() but
report errors instead of dying" variant of functions are named
do_something_gently(), not do_something_gentle().
^ permalink raw reply
* Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations
From: Jeff King @ 2016-09-16 21:51 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <0152df30db0972d61ff45b2b099ad1242aacd431.1474047135.git.jonathantanmy@google.com>
On Fri, Sep 16, 2016 at 10:37:24AM -0700, Jonathan Tan wrote:
> Mailinfo currently handles multi-line headers, but it does not handle
> multi-line in-body headers. Teach it to handle such headers, for
> example, for this input:
>
> Subject: a very long
> broken line
>
> Subject: another very long
> broken line
>
> interpret the in-body subject to be "another very long broken line"
> instead of "another very long".
This puzzled me; we should stop parsing in-body headers after the first
blank line. But then I realized you probably meant the first "Subject"
to be the real mail header.
I wonder if it would be more obvious with an example like:
From: ...
Date: ...
Subject: the actual mail subject
Subject: a very long
broken line
Or something.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 1/3] mailinfo: refactor commit message processing
From: Jeff King @ 2016-09-16 21:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, git
In-Reply-To: <xmqqoa3nk6a5.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 16, 2016 at 12:12:50PM -0700, Junio C Hamano wrote:
> > +static int check_header_raw(struct mailinfo *mi,
> > + char *buf, size_t len,
> > + struct strbuf *hdr_data[], int overwrite) {
> > + const struct strbuf sb = {0, len, buf};
> > + return check_header(mi, &sb, hdr_data, overwrite);
> > +}
>
> IIUC, this is a helper for callers that do not have a strbuf but
> instead have <buf, len> pair to perform the same check_header() the
> callers that have strbuf can do.
>
> As check_header() uses the strbuf as a read-only entity, wrapping
> the <buf, len> pair in a temporary strbuf like this is safe.
>
> The incoming <buf> should conceptually be "const char *", but it's
> OK.
I think the "right" way to do this would be to continue taking a "char
*", and then strbuf_attach() it. That saves us from unexpectedly
violating any strbuf invariants.
If our assumption that check_header() does not touch the
contents turns out to be wrong, neither strategy would inform our
caller, though. I think you'd want something like:
assert(sb.buf == buf);
after check_header() returns (though I guess we are in theory protected
by the "const").
That being said...
> If check_header() didn't call any helper function that gets passed
> &sb as a strbuf, or if convertiong the helper function to take a
> <buf, len> pair instead, I would actually suggest refactoring this
> the other way around, though. That is, move the implementation of
> check_header() to this function, updating its reference to line->buf
> and line->len to reference to <buf> and <len>, and then make
> check_header() a thin wrapper that does
>
> check_header(mi, const struct strbuf *line,
> struct strbuf *hdr_data[], int overwrite)
> {
> return check_header_raw(mi, line->buf, line->len,
> hdr_data, overwrite);
> }
This is _way_ better, and it looks like check_header() could handle it
easily. Looking at it, I also suspect the cascading if in that function
could be made more pleasant by modeling cmp_header()'s interface after
skip_prefix_mem(), but that is totally orthogonal and optional.
-Peff
^ permalink raw reply
* [PATCH] mailinfo: unescape quoted-pair in header fields
From: Kevin Daudt @ 2016-09-16 21:02 UTC (permalink / raw)
To: git; +Cc: Kevin Daudt, Swift Geek, Jeff King, Junio C Hamano
rfc2822 has provisions for quoted strings in structured header fields,
but also allows for escaping these with so-called quoted-pairs.
The only thing git currently does is removing exterior quotes, but
quotes within are left alone.
Tell mailinfo to remove exterior quotes and remove escape characters from the
author so that they don't show up in the commits author field.
Signed-off-by: Kevin Daudt <me@ikke.info>
---
The only thing I could not easily fix is the prevent git am from removing any quotes around the author. This is done in fmt_ident, which calls `strbuf_addstr_without_crud`.
mailinfo.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++
t/t5100-mailinfo.sh | 6 ++++++
t/t5100/quoted-pair.expect | 5 +++++
t/t5100/quoted-pair.in | 9 ++++++++
4 files changed, 74 insertions(+)
create mode 100644 t/t5100/quoted-pair.expect
create mode 100644 t/t5100/quoted-pair.in
diff --git a/mailinfo.c b/mailinfo.c
index e19abe3..04036f3 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -54,15 +54,69 @@ static void parse_bogus_from(struct mailinfo *mi, const struct strbuf *line)
get_sane_name(&mi->name, &mi->name, &mi->email);
}
+static int unquote_quoted_string(struct strbuf *line)
+{
+ struct strbuf outbuf;
+ const char *in = line->buf;
+ int c, take_next_literally = 0;
+ int found_error = 0;
+ char escape_context=0;
+
+ strbuf_init(&outbuf, line->len);
+
+ while ((c = *in++) != 0) {
+ if (take_next_literally) {
+ take_next_literally = 0;
+ } else {
+ switch (c) {
+ case '"':
+ if (!escape_context)
+ escape_context = '"';
+ else if (escape_context == '"')
+ escape_context = 0;
+ continue;
+ case '\\':
+ if (escape_context) {
+ take_next_literally = 1;
+ continue;
+ }
+ break;
+ case '(':
+ if (!escape_context)
+ escape_context = '(';
+ else if (escape_context == '(')
+ found_error = 1;
+ break;
+ case ')':
+ if (escape_context == '(')
+ escape_context = 0;
+ break;
+ }
+ }
+
+ strbuf_addch(&outbuf, c);
+ }
+
+ strbuf_reset(line);
+ strbuf_addbuf(line, &outbuf);
+ strbuf_release(&outbuf);
+
+ return found_error;
+
+}
+
static void handle_from(struct mailinfo *mi, const struct strbuf *from)
{
char *at;
size_t el;
struct strbuf f;
+
strbuf_init(&f, from->len);
strbuf_addbuf(&f, from);
+ unquote_quoted_string(&f);
+
at = strchr(f.buf, '@');
if (!at) {
parse_bogus_from(mi, from);
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 1a5a546..d0c21fc 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -142,4 +142,10 @@ test_expect_success 'mailinfo unescapes with --mboxrd' '
test_cmp expect mboxrd/msg
'
+test_expect_success 'mailinfo unescapes rfc2822 quoted-string' '
+ mkdir quoted-pair &&
+ git mailinfo /dev/null /dev/null <"$TEST_DIRECTORY"/t5100/quoted-pair.in >quoted-pair/info &&
+ test_cmp "$TEST_DIRECTORY"/t5100/quoted-pair.expect quoted-pair/info
+'
+
test_done
diff --git a/t/t5100/quoted-pair.expect b/t/t5100/quoted-pair.expect
new file mode 100644
index 0000000..cab1bce
--- /dev/null
+++ b/t/t5100/quoted-pair.expect
@@ -0,0 +1,5 @@
+Author: Author "The Author" Name
+Email: somebody@example.com
+Subject: testing quoted-pair
+Date: Sun, 25 May 2008 00:38:18 -0700
+
diff --git a/t/t5100/quoted-pair.in b/t/t5100/quoted-pair.in
new file mode 100644
index 0000000..e2e627a
--- /dev/null
+++ b/t/t5100/quoted-pair.in
@@ -0,0 +1,9 @@
+From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001
+From: "Author \"The Author\" Name" <somebody@example.com>
+Date: Sun, 25 May 2008 00:38:18 -0700
+Subject: [PATCH] testing quoted-pair
+
+
+
+---
+patch
--
2.10.0.86.g6ffa4f1.dirty
^ permalink raw reply related
* Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations
From: Junio C Hamano @ 2016-09-16 20:59 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, peff
In-Reply-To: <1b392241-461e-3b87-400d-70d66903e3d7@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
>> handle_commit_msg(...)
>> {
>> if (mi->in_line_header->len) {
>> /* we have read the beginning of one in-line header */
>> if (line->len && isspace(*line->buf))
>
> This would mean that a message like the following:
>
> From: Me <me@example.com>
> -- 8< -- this scissors line will be treated as part of "From"
>
> would have its scissors line treated as a header.
>
> The main reason why I reordered the checks (in RFC/PATCH 1/3) is to
> avoid this (treating a scissors line with an initial space immediately
> following an in-body header as part of a header).
>
> (If this is not a concern then yes, I agree that the way you described
> is simpler and better.)
Ahh, OK. I do not think anybody sane would do the "From:" thing,
but with the "does it look like 2822 header" check to decide if the
first header-looking line should be queued, another failure mode may
be:
any-random-alpha-and-dash-string:
-- >8 -- cut here -- >8 --
Subject: real subject
The first line of the real message
I personally do not think it matters that much, but if we wanted to
protect us from it we could easily do
if (mi->in_line_header->len) {
/* we have read the beginning of one in-line header */
if (line->len && isspace(*line->buf) &&
!(mi->use_scissors && is_scissors_line(line))) {
append to mi->in_line_header strbuf;
return 0;
}
/* otherwise we know mi->in_line_header is now complete */
check_header(mi, mi->in_line_header, ...);
strbuf_reset(&mi->in_line_header);
}
...
instead, I think.
^ permalink raw reply
* Re: [PATCH 01/11] Resumable clone: create service git-prime-clone
From: Junio C Hamano @ 2016-09-16 20:53 UTC (permalink / raw)
To: Kevin Wern; +Cc: git
In-Reply-To: <1473984742-12516-2-git-send-email-kevin.m.wern@gmail.com>
Kevin Wern <kevin.m.wern@gmail.com> writes:
> Create git-prime-clone, a program to be executed on the server that
> returns the location and type of static resource to download before
> performing the rest of a clone.
>
> Additionally, as this executable's location will be configurable (see:
> upload-pack and receive-pack), add the program to
> BINDIR_PROGRAMS_NEED_X, in addition to the usual builtin places. Add
> git-prime-clone executable to gitignore, as well
>
> Signed-off-by: Kevin Wern <kevin.m.wern@gmail.com>
> ---
I wonder if we even need a separate service like this.
Wouldn't a new protocol capability that is advertised from
upload-pack sufficient to tell the "git clone" that it can
and should consider priming from this static resource?
> +static void prime_clone(void)
> +{
> + if (!enabled) {
> + fprintf(stderr, _("prime-clone not enabled\n"));
> + }
> + else if (url && filetype){
> + packet_write(1, "%s %s\n", filetype, url);
> + }
> + else if (url || filetype) {
> + if (filetype)
> + fprintf(stderr, _("prime-clone not properly "
> + "configured: missing url\n"));
> + else if (url)
> + fprintf(stderr, _("prime-clone not properly "
> + "configured: missing filetype\n"));
> + }
> + packet_flush(1);
> +}
Two minor comments:
- For whom are you going to localize these strings? This program
is running on the server side and we do not know the locale
preferred by the end-user who is sitting on the other end of the
connection, no?
- Turn "}\n\s+else " into "} else ", please.
^ permalink raw reply
* Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations
From: Jonathan Tan @ 2016-09-16 20:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff
In-Reply-To: <xmqq8turk3aw.fsf@gitster.mtv.corp.google.com>
On 09/16/2016 01:17 PM, Junio C Hamano wrote:
> In other words, wouldn't something like the illustration at the end
> of this message sufficient? If the body consists solely of in-body
> header without any patch or patchbreak, we may reach EOF with
> something in mi->in_line_header buffer and nothing in
> mi->log_message and without this function getting any chance to
> return 1, so a careful caller may want to flush in_line_header, but
> the overall result of the mailinfo subsystem in such a case would be
> an error ("you didn't have any patch or a message?"), so it may not
> matter too much.
Noted. (This was one of my concerns - that the caller should, but did
not, flush.)
> What am I missing?
>
> handle_commit_msg(...)
> {
> if (mi->in_line_header->len) {
> /* we have read the beginning of one in-line header */
> if (line->len && isspace(*line->buf))
This would mean that a message like the following:
From: Me <me@example.com>
-- 8< -- this scissors line will be treated as part of "From"
would have its scissors line treated as a header.
The main reason why I reordered the checks (in RFC/PATCH 1/3) is to
avoid this (treating a scissors line with an initial space immediately
following an in-body header as part of a header).
(If this is not a concern then yes, I agree that the way you described
is simpler and better.)
> append to mi->in_line_header strbuf;
> return 0;
> /* otherwise we know mi->in_line_header is now complete */
> check_header(mi, mi->in_line_header, ...);
> strbuf_reset(&mi->in_line_header);
> }
>
> if (mi->header_stage && (it is a blank line))
> return 0;
>
> if (mi->use_inbody_headers && mi->header_stage &&
> (the line looks like beginning of 2822 header)) {
> strbuf_addbuf(&mi->in_line_header, line);
> return 0;
> }
> /* otherwise we are no longer looking at headers */
> mi->header_stage = 0;
>
> /* normalize the log message to UTF-8. */
> if (convert_to_utf8(mi, line, mi->charset.buf))
> return 0; /* mi->input_error already set */
>
> if (mi->use_scissors && is_scissors_line(line)) {
> int i;
>
> strbuf_setlen(&mi->log_message, 0);
> mi->header_stage = 1;
>
> /*
> * We may have already read "secondary headers"; purge
> * them to give ourselves a clean restart.
> */
> for (i = 0; header[i]; i++) {
> if (mi->s_hdr_data[i])
> strbuf_release(mi->s_hdr_data[i]);
> mi->s_hdr_data[i] = NULL;
> }
> return 0;
> }
>
> if (patchbreak(line)) {
> if (mi->message_id)
> strbuf_addf(&mi->log_message,
> "Message-Id: %s\n", mi->message_id);
> return 1;
> }
>
> strbuf_addbuf(&mi->log_message, line);
> return 0;
> }
>
>
^ permalink raw reply
* Re: [PATCH 00/11] Resumable clone
From: Junio C Hamano @ 2016-09-16 20:47 UTC (permalink / raw)
To: Kevin Wern; +Cc: git
In-Reply-To: <1473984742-12516-1-git-send-email-kevin.m.wern@gmail.com>
Kevin Wern <kevin.m.wern@gmail.com> writes:
> It's been a while (sent a very short patch in May), but I've
> still been working on the resumable clone feature and checking up on
> the mailing list for any updates. After submitting the prime-clone
> service alone, I figured implementing the whole thing would be the best
> way to understand the full scope of the problem (this is my first real
> contribution here, and learning while working on such an involved
> feature has not been easy).
It may not have been easy but I hope it has been a fun journey for
you ;-)
> On the client side, the transport_prime_clone and
> transport_download_primer APIs are built to be more robust (i.e. read
> messages without dying due to protocol errors), so that git clone can
> always try them without being dependent on the capability output of
> git-upload-pack. transport_download_primer is dependent on the success
> of transport_prime_clone, but transport_prime_clone is always run on an
> initial clone. Part of achieving this robustness involves adding
> *_gentle functions to pkt_line, so that prime_clone can fail silently
> without dying.
OK.
> Right now, a manually resumable directory is left behind only if the
> *client* is interrupted while a new junk mode, JUNK_LEAVE_RESUMABLE,
> is set (right before the download). For an initial clone, if the
> connection fails after automatic resuming, the client erases the
> partial resources and falls through to a normal clone. However, once a
> resumable directory is left behind by the program, it is NEVER
> deleted/abandoned after it is continued with --resume.
Sounds like you made a sensible design decision here.
> - When running with ssh and a password, the credentials are
> prompted for twice. I don't know if there is a way to
> preserve credentials between executions. I couldn't find any
> examples in git's source.
We leave credentail reuse to keyring services like ssh-agent.
^ permalink raw reply
* Re: [RFC/PATCH 3/3] mailinfo: handle in-body header continuations
From: Junio C Hamano @ 2016-09-16 20:17 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, peff
In-Reply-To: <0152df30db0972d61ff45b2b099ad1242aacd431.1474047135.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> Instead of repeatedly calling "check_header" (as in this patch), one
> alternate method to accomplish this would be to have a buffer of
> potential header text in struct mailinfo to be flushed whenever a header
> is known to end (for example, if we detect the start of a new header),
> but this makes the logic more complicated - for example, the flushing
> would not only invoke check_header but would also need to reconstruct
> the original lines, possibly decode them into UTF-8, and store them in
> log_message, and any failures would be noticed a few "lines" away from
> the original failure point. Also, care would need to be taken to flush
> the buffer at all appropriate places.
I am not sure how much the UTF-8 decoding argument above matters.
The current way handle_commit_msg() is structured (before any of
your patches) is for it to take one raw line at a time and:
- If we haven't seen a non-header line (i.e. at the beginning,
or we were reading in-body headers), return without doing
anything.
- If we are told to honor in-body headers and if we haven't seen
a non-header line, see if the line itself looks like a header
and if so, handle it as an in-body header and return. If that
line is not an in-body header, continue processing.
- If the processing reaches at this point, we are done with the
headers (i.e. mi->header_stage is set to 0).
- Make sure the line is in utf8.
- If it is a scissors line and we are told to honor scissors
lines, ignore what we have read so far and go back to "we
haven't seen a non-header line" state and return.
- If it is a patch break, return and signal the caller we are
done with the log message.
- Otherwise accumulate the line as part of the log message.
The bug we want to address is in the second step. We only look at
the first line of folded in-body header line, because we are fed one
line at a time.
If we keep the location of UTF8 conversion, and buffered the in-body
header in "struct mailinfo *mi" (like you seem to do in this patch),
what we will queue there will be _before_ conversion. We'd call
check_header() on it once we know one logical line of a header is
accumulated, and check_header() would do the right conversion via
decode_header() etc., so I do not see why you need to worry about
the encoding issues at all.
I wonder if the simplest would be to introduce another state in the
state machine that is "we know we are processing in-body header, and
we have read early part of an in-body header line that may not be
complete".
In other words, wouldn't something like the illustration at the end
of this message sufficient? If the body consists solely of in-body
header without any patch or patchbreak, we may reach EOF with
something in mi->in_line_header buffer and nothing in
mi->log_message and without this function getting any chance to
return 1, so a careful caller may want to flush in_line_header, but
the overall result of the mailinfo subsystem in such a case would be
an error ("you didn't have any patch or a message?"), so it may not
matter too much.
What am I missing?
handle_commit_msg(...)
{
if (mi->in_line_header->len) {
/* we have read the beginning of one in-line header */
if (line->len && isspace(*line->buf))
append to mi->in_line_header strbuf;
return 0;
/* otherwise we know mi->in_line_header is now complete */
check_header(mi, mi->in_line_header, ...);
strbuf_reset(&mi->in_line_header);
}
if (mi->header_stage && (it is a blank line))
return 0;
if (mi->use_inbody_headers && mi->header_stage &&
(the line looks like beginning of 2822 header)) {
strbuf_addbuf(&mi->in_line_header, line);
return 0;
}
/* otherwise we are no longer looking at headers */
mi->header_stage = 0;
/* normalize the log message to UTF-8. */
if (convert_to_utf8(mi, line, mi->charset.buf))
return 0; /* mi->input_error already set */
if (mi->use_scissors && is_scissors_line(line)) {
int i;
strbuf_setlen(&mi->log_message, 0);
mi->header_stage = 1;
/*
* We may have already read "secondary headers"; purge
* them to give ourselves a clean restart.
*/
for (i = 0; header[i]; i++) {
if (mi->s_hdr_data[i])
strbuf_release(mi->s_hdr_data[i]);
mi->s_hdr_data[i] = NULL;
}
return 0;
}
if (patchbreak(line)) {
if (mi->message_id)
strbuf_addf(&mi->log_message,
"Message-Id: %s\n", mi->message_id);
return 1;
}
strbuf_addbuf(&mi->log_message, line);
return 0;
}
^ permalink raw reply
* Re: [RFC/PATCH 2/3] mailinfo: correct malformed test example
From: Junio C Hamano @ 2016-09-16 19:19 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, peff
In-Reply-To: <5dbb0b0f64906fd18c217908cd2c04e74d80fa68.1474047135.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> An existing sample message (0015) in the tests for mailinfo contains an
> indented line immediately after an in-body header (without any
> intervening blank line).
This comes from d25e5159 ("git am/mailinfo: Don't look at in-body
headers when rebasing", 2009-11-20), where we want to make sure that
a "From: bogosity" that isn't meant to be an in-body header is not
identified as such, even when it is immediately followed by a
non-blank line. "From: bogosity" is for msg0015 but the same
applies to the header-looking block for msg0008.
Adding a blank line there will defeat the whole point of the test,
which is to make sure we don't do anything funky when --no-inbody-headers
is asked for, no?
> diff --git a/t/t5100/info0008--no-inbody-headers b/t/t5100/info0008--no-inbody-headers
> new file mode 100644
> index 0000000..e8a2951
> --- /dev/null
> +++ b/t/t5100/info0008--no-inbody-headers
> @@ -0,0 +1,5 @@
> +Author: Junio C Hamano
> +Email: junio@kernel.org
> +Subject: another patch
> +Date: Fri, 9 Jun 2006 00:44:16 -0700
> +
> diff --git a/t/t5100/msg0008--no-inbody-headers b/t/t5100/msg0008--no-inbody-headers
> new file mode 100644
> index 0000000..d6e950e
> --- /dev/null
> +++ b/t/t5100/msg0008--no-inbody-headers
> @@ -0,0 +1,6 @@
> +From: A U Thor <a.u.thor@example.com>
> +Subject: [PATCH] another patch
> +>Here is an empty patch from A U Thor.
> +
> +Hey you forgot the patch!
> +
> diff --git a/t/t5100/msg0015--no-inbody-headers b/t/t5100/msg0015--no-inbody-headers
> index be5115b..44a6ce7 100644
> --- a/t/t5100/msg0015--no-inbody-headers
> +++ b/t/t5100/msg0015--no-inbody-headers
> @@ -1,3 +1,4 @@
> From: bogosity
> +
> - a list
> - of stuff
> diff --git a/t/t5100/patch0008--no-inbody-headers b/t/t5100/patch0008--no-inbody-headers
> new file mode 100644
> index 0000000..e69de29
> diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
> index 8b2ae06..ba8b208 100644
> --- a/t/t5100/sample.mbox
> +++ b/t/t5100/sample.mbox
> @@ -656,6 +656,7 @@ Subject: check bogus body header (from)
> Date: Fri, 9 Jun 2006 00:44:16 -0700
>
> From: bogosity
> +
> - a list
> - of stuff
> ---
^ permalink raw reply
* Re: [RFC/PATCH 1/3] mailinfo: refactor commit message processing
From: Junio C Hamano @ 2016-09-16 19:12 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, peff
In-Reply-To: <7dbb4bc0659056211b27f0033c73f0d558efdb54.1474047135.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> Within the processing of the commit message, check for a scissors line
> or a patchbreak line first (before checking for in-body headers) so that
> a subsequent patch modifying the processing of in-body headers would not
> cause a scissors line or patchbreak line to be misidentified.
>
> If a line could be both an in-body header and a scissors line (for
> example, "From: -- >8 --"), this is considered a fatal error
> (previously, it would be interpreted as an in-body header).
The scissors line is designed to allow garbage other than scissors
and perforation marks to be on the same line, i.e.
/*
* The mark must be at least 8 bytes long (e.g. "-- >8 --").
* Even though there can be arbitrary cruft on the same line
* (e.g. "cut here"), in order to avoid misidentification, the
* perforation must occupy more than a third of the visible
* width of the line, and dashes and scissors must occupy more
* than half of the perforation.
*/
Even though it is not likely for people to do so, it would probably
be nicer if we can treat
From: -- >8 -- cut -- >8 -- >8 -- here -- >8 --
as a scissors line instead of making it a fatal error, by treating
that "From:" as just a random garbage.
But this is a minor point. It is not worth to make it work like so
if the resulting code will become messier.
> The following enumeration shows that processing is the same except (as
> described above) the in-body header + scissors line case.
>
> o in-body header (check_header OK)
> o passes UTF-8 conversion
> o [described above] is scissors line
> o [not possible] is patchbreak line
> o [not possible] is blank line
> o is none of the above - processed as header
> o fails UTF-8 conversion - processed as header
> o not in-body header
> o passes UTF-8 conversion
> o is scissors line - processed as scissors
> o is patchbreak line - processed as patchbreak
> o is blank line - ignored if in header_stage
> o is none of the above - log message
> o fails UTF-8 conversion - input error
>
> As for the result left in "line" (after the invocation of
> handle_commit_msg), it is unused (by its caller, handle_filter, and by
> handle_filter's callers, handle_boundary and handle_body) unless this
> line is a patchbreak line, in which case handle_patch is subsequently
> called (in handle_filter) on "line". In this case, "line" must have
> passed UTF-8 conversion both before and after this patch, so the result
> is still the same overall.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> mailinfo.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 115 insertions(+), 30 deletions(-)
>
> diff --git a/mailinfo.c b/mailinfo.c
> index e19abe3..23a56c2 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -340,23 +340,56 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg)
> return out;
> }
>
> -static int convert_to_utf8(struct mailinfo *mi,
> - struct strbuf *line, const char *charset)
> +/*
> + * Attempts to convert line into UTF-8, storing the result in line.
> + *
> + * This differs from convert_to_utf8 in that conversion non-success is not
> + * considered an error case - mi->input_error is not set, and no error message
> + * is printed.
> + *
> + * If the conversion is unnecessary, returns 0 and stores NULL in old_buf (if
> + * old_buf is not NULL).
> + *
> + * If the conversion is successful, returns 0 and stores the unconverted string
> + * in old_buf and old_len (if they are respectively not NULL).
> + *
> + * If the conversion is unsuccessful, returns -1.
> + */
> +static int try_convert_to_utf8(const struct mailinfo *mi, struct strbuf *line,
> + const char *charset, char **old_buf,
> + size_t *old_len)
> {
> - char *out;
> + char *utf8;
>
> - if (!mi->metainfo_charset || !charset || !*charset)
> + if (!mi->metainfo_charset || !charset || !*charset ||
> + same_encoding(mi->metainfo_charset, charset)) {
> + if (old_buf)
> + *old_buf = NULL;
> return 0;
> + }
>
> - if (same_encoding(mi->metainfo_charset, charset))
> + utf8 = reencode_string(line->buf, mi->metainfo_charset, charset);
> + if (utf8) {
> + char *temp = strbuf_detach(line, old_len);
> + if (old_buf)
> + *old_buf = temp;
> + strbuf_attach(line, utf8, strlen(utf8), strlen(utf8));
> return 0;
> - out = reencode_string(line->buf, mi->metainfo_charset, charset);
> - if (!out) {
> + }
> + return -1;
> +}
> +
> +/*
> + * Converts line into UTF-8, setting mi->input_error to -1 upon failure.
> + */
> +static int convert_to_utf8(struct mailinfo *mi,
> + struct strbuf *line, const char *charset)
> +{
> + if (try_convert_to_utf8(mi, line, charset, NULL, NULL)) {
> mi->input_error = -1;
> return error("cannot convert from %s to %s",
> charset, mi->metainfo_charset);
> }
> - strbuf_attach(line, out, strlen(out), strlen(out));
> return 0;
> }
Please split this part into its own patch. IIUC, it moves the meat
of convert_to_utf8() to a more silent try_convert_to_utf8() and then
makes the former a thin wrapper of the latter. Which by itself is a
good change but does not have anything to do with "fix handling of
the in-body headers", other than that the main fix wants to have
such a more silent helper for its own use.
> @@ -515,6 +548,13 @@ static int check_header(struct mailinfo *mi,
> return ret;
> }
>
> +static int check_header_raw(struct mailinfo *mi,
> + char *buf, size_t len,
> + struct strbuf *hdr_data[], int overwrite) {
> + const struct strbuf sb = {0, len, buf};
> + return check_header(mi, &sb, hdr_data, overwrite);
> +}
IIUC, this is a helper for callers that do not have a strbuf but
instead have <buf, len> pair to perform the same check_header() the
callers that have strbuf can do.
As check_header() uses the strbuf as a read-only entity, wrapping
the <buf, len> pair in a temporary strbuf like this is safe.
The incoming <buf> should conceptually be "const char *", but it's
OK.
If check_header() didn't call any helper function that gets passed
&sb as a strbuf, or if convertiong the helper function to take a
<buf, len> pair instead, I would actually suggest refactoring this
the other way around, though. That is, move the implementation of
check_header() to this function, updating its reference to line->buf
and line->len to reference to <buf> and <len>, and then make
check_header() a thin wrapper that does
check_header(mi, const struct strbuf *line,
struct strbuf *hdr_data[], int overwrite)
{
return check_header_raw(mi, line->buf, line->len,
hdr_data, overwrite);
}
I didn't check how involved to update cmp_header() to take <buf,len>
pair. If it does not look too bad, then I think I would prefer to
do it that way, and as before, make that conversion a separate
preparatory patch.
> @@ -623,32 +663,48 @@ static int is_scissors_line(const struct strbuf *line)
> gap * 2 < perforation);
> }
>
> -static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
> +static int resembles_rfc2822_header(const struct strbuf *line)
> {
> - assert(!mi->filter_stage);
> + char *c;
>
> - if (mi->header_stage) {
> - if (!line->len || (line->len == 1 && line->buf[0] == '\n'))
> + if (!isalpha(line->buf[0]))
> + return 0;
> +
> + for (c = line->buf + 1; *c != 0; c++) {
> + if (*c == ':')
> + return 1;
> + else if (*c != '-' && !isalpha(*c))
> return 0;
> }
> + return 0;
> +}
Is this helper supposed to handle any rfc2822 looking header, or
only the ones we expect to see as in-body header?
> - if (mi->use_inbody_headers && mi->header_stage) {
> - mi->header_stage = check_header(mi, line, mi->s_hdr_data, 0);
> - if (mi->header_stage)
> - return 0;
> - } else
> - /* Only trim the first (blank) line of the commit message
> - * when ignoring in-body headers.
> - */
> - mi->header_stage = 0;
> +static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
> +{
> + int ret = 0;
> + int utf8_result;
> + char *old_buf;
> + size_t old_len;
> +
> + assert(!mi->filter_stage);
>
> - /* normalize the log message to UTF-8. */
> - if (convert_to_utf8(mi, line, mi->charset.buf))
> - return 0; /* mi->input_error already set */
> + /*
> + * Obtain UTF8 for scissors line and patchbreak checks, but retain the
> + * undecoded line in case we need to process it as an in-body header.
> + */
> + utf8_result = try_convert_to_utf8(mi, line, mi->charset.buf, &old_buf,
> + &old_len);
Just a minor style suggestion. As <old_buf, old_len> come in a
pair, fold the line before them, so that the readers can easily
see the association between them. I.e.
utf8_result = try_convert_to_utf8(mi, line, mi->charset.buf,
&old_buf, &old_len);
> - if (mi->use_scissors && is_scissors_line(line)) {
> + if (!utf8_result && mi->use_scissors && is_scissors_line(line)) {
> int i;
>
> + if (resembles_rfc2822_header(line))
> + /*
> + * Explicitly reject scissor lines that resemble a RFC
> + * 2822 header, to avoid being prone to error.
> + */
> + die("scissors line resembles RFC 2822 header");
> +
I guess "disambiguate to favor scissors" is not that difficult ;-)
> strbuf_setlen(&mi->log_message, 0);
> mi->header_stage = 1;
>
> @@ -661,18 +717,47 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
> strbuf_release(mi->s_hdr_data[i]);
> mi->s_hdr_data[i] = NULL;
> }
> - return 0;
> + goto handle_commit_msg_out;
> }
> -
> - if (patchbreak(line)) {
> + if (!utf8_result && patchbreak(line)) {
> if (mi->message_id)
> strbuf_addf(&mi->log_message,
> "Message-Id: %s\n", mi->message_id);
> - return 1;
> + ret = 1;
> + goto handle_commit_msg_out;
> }
>
> + if (mi->header_stage) {
> + char *buf = old_buf ? old_buf : line->buf;
> + if (buf[0] == 0 || (buf[0] == '\n' && buf[1] == 0))
> + goto handle_commit_msg_out;
> + }
> +
> + if (mi->use_inbody_headers && mi->header_stage) {
> + char *buf = old_buf ? old_buf : line->buf;
> + size_t len = old_buf ? old_len : line->len;
> + mi->header_stage = check_header_raw(mi, buf, len,
> + mi->s_hdr_data, 0);
Just a minor comment, but I guess check_header_raw() refactoring is
not strictly needed after all, as this callsite can wrap <buf,len>
into a temporary strbuf.
Unlike the real header that is read in read_one_header_line() inside
a loop to implement line folding before check_header() is called, we
call check_header() before possibly-foldable header lines is fully
assembled into one header. Probably it comes in later patches, I
guess.
It is not immediately obvious to me how this step helps further work
done by later patches in the series until I read them, but so far
what this patch did looks understandable to me ;-)
Thanks.
> + if (mi->header_stage)
> + goto handle_commit_msg_out;
> + } else
> + /* Only trim the first (blank) line of the commit message
> + * when ignoring in-body headers.
> + */
> + mi->header_stage = 0;
> +
> + /* If adding as a log message, conversion to UTF-8 is required. */
> + if (utf8_result) {
> + mi->input_error = -1;
> + error("cannot convert from %s to %s",
> + mi->charset.buf, mi->metainfo_charset);
> + goto handle_commit_msg_out;
> + }
> strbuf_addbuf(&mi->log_message, line);
> - return 0;
> +
> +handle_commit_msg_out:
> + free(old_buf);
> + return ret;
> }
>
> static void handle_patch(struct mailinfo *mi, const struct strbuf *line)
^ permalink raw reply
* Re: [RFC] extending pathspec support to submodules
From: Brandon Williams @ 2016-09-16 18:40 UTC (permalink / raw)
To: Heiko Voigt
Cc: Stefan Beller, Junio C Hamano, git@vger.kernel.org, Duy Nguyen,
Jens Lehmann
In-Reply-To: <20160916093456.GA1488@book.hvoigt.net>
On Thu, Sep 15, 2016 at 3:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> * Your program that runs in the top-level superproject still needs
> to be able to say "this pathspec from the top cannot possibly
> match anything in the submodule, so let's not even bother
> descending into it".
>
Yes, we would need to first check if the submodule is a prefix match to the
pathspec. ie a submodule 'sub' would need to match the pathspec 'sub/somedir'
or '*.txt' but not the pathspecs 'subdirectory' or 'otherdir'
> > > So we may have to rethink what this option name should be. "You
> > > are running in a repository that is used as a submodule in a
> > > larger context, which has the submodule at this path" is what the
> > > option tells the command; if any existing command already has
> > > such an option, we should use it. If we are inventing one,
> > > perhaps "--submodule-path" (I didn't check if there are existing
> > > options that sound similar to it and mean completely different
> > > things, in which case that name is not usable)?
> >
> > Would it make sense to add the '--submodule-path' to a more generic
> > part of the code? It's not just ls-files/grep that have to solve exactly this
> > problem. Up to now we just did not go for those commands, though.
>
> Yes I think so, since it should also handle starting from a submodule
> with a pathspec to the superproject or other submodule. In case we
> go with my above suggestion I would suggest a more generic name since
> the option could also be passed to processes handling the superproject.
> E.g. something like --module-prefix or --repository-prefix comes to my
> mind, not checked though.
Yeah we may want to come up with a more descriptive option name now which can
be generally applied, especially if we are going to continue adding submodule
support for other commands.
-Brandon
^ permalink raw reply
* Re: [RFC/PATCH 0/3] handle multiline in-body headers
From: Junio C Hamano @ 2016-09-16 18:29 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, peff
In-Reply-To: <cover.1474047135.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> Thanks, Peff, for the explanation and the method to reproduce the issue.
>
> The issue seems to be in mailinfo.c - this patch set addresses that, and I have
> also included a test for "git am" in t/t4150-am.sh to show the effect of this
> patch set on that command.
Thanks, will take a look.
> Jonathan Tan (3):
> mailinfo: refactor commit message processing
> mailinfo: correct malformed test example
> mailinfo: handle in-body header continuations
>
> mailinfo.c | 165 ++++++++++++++++++++++++++++-------
> mailinfo.h | 1 +
> t/t4150-am.sh | 23 +++++
> t/t5100-mailinfo.sh | 4 +-
> t/t5100/info0008--no-inbody-headers | 5 ++
> t/t5100/info0018 | 5 ++
> t/t5100/msg0008--no-inbody-headers | 6 ++
> t/t5100/msg0015--no-inbody-headers | 1 +
> t/t5100/msg0018 | 2 +
> t/t5100/patch0008--no-inbody-headers | 0
> t/t5100/patch0018 | 6 ++
> t/t5100/sample.mbox | 20 +++++
> 12 files changed, 206 insertions(+), 32 deletions(-)
> create mode 100644 t/t5100/info0008--no-inbody-headers
> create mode 100644 t/t5100/info0018
> create mode 100644 t/t5100/msg0008--no-inbody-headers
> create mode 100644 t/t5100/msg0018
> create mode 100644 t/t5100/patch0008--no-inbody-headers
> create mode 100644 t/t5100/patch0018
^ permalink raw reply
* Re: [wishlist?] make submodule commands robust to having non-submodule Subprojects
From: Junio C Hamano @ 2016-09-16 18:28 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Stefan Beller, Yaroslav Halchenko, git@vger.kernel.org
In-Reply-To: <20160916141143.GA47240@book.hvoigt.net>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> On Thu, Sep 15, 2016 at 11:27:54AM -0700, Junio C Hamano wrote:
>
>> If the trend in Git community collectively these days is to make
>> usage of submodules easier and smoother, I'd imagine that you would
>> want to teach "git add" that was given a submodule to "git submodule
>> add" instead by default, with an option "git add --no-gitmodules
>> sub" to disable it, or something like that.
>>
>> > $ git submodule add --fixup-modules-file ./sub sub
>> > Adding .gitmodule entry only for `sub` to use `git -C remote
>> > show origin` as URL.
>>
>> I agree that a feature like this is needed regardless of what
>> happens at "git add" time.
>
> How about just
>
> git submodule add <submodulepath>
When I said "a feature like this is needed", I didn't care about
exact syntax. I am not sure how often people need the "fixup", what
kind of causes there are that they need the "fixup", and what the
distribution of vaious causes would be like. If the _ONLY_ kind of
fixup necessary is "I meant to say 'git submodule add ./path path'
but I said 'git add path' instead", then I think it makes sense to
teach "submodule add" that the form "git submodule add <path>" is a
short-pand for "git submodule add ./<path> <path>". I am not sure
if we want to _ignore_ a gitlink that is already in the index
unconditionally, i.e. if it is a good idea to let the second one
override the first one
git submodule add $URL sub &&
git submodule add sub
in this sequence, though.
> ? I remember back in the days when I started with submodules thats the
> way I imagined submodules would work:
>
> 1. clone the submodule into a directory
> 2. git submodule add it
> 3. git commit everything
>
> Because that how you basically work with files. So instead of adding
> another option I would rather like to autodetect that:
>
> * its a relative path inside this repo that is passed to
> 'git submodule add'
> * there is no .gitmodules entry
> * and no .git/config
> ==> create those from a remote in the submodule
In other words, I agree with the general direction but I'd add
another condition to the above three, i.e.
* and there is no gitlink for that path in the index yet.
^ permalink raw reply
* Re: [PATCH 3/2] batch check whether submodule needs pushing into one call
From: Junio C Hamano @ 2016-09-16 18:13 UTC (permalink / raw)
To: Heiko Voigt
Cc: Jeff King, Stefan Beller, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <20160916123155.GA40725@book.hvoigt.net>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> On Fri, Sep 16, 2016 at 11:40:19AM +0200, Heiko Voigt wrote:
>> > By the way, with the two new patches, 'pu' seems to start failing
>> > some tests, e.g. 5533 5404 5405.
>>
>> Ah ok I did only test on master, will look into those.
>
> Ok I had a look into these and the reason t5533 fails is because on pu
> --recurse-submodules is enabled by default and I missed the case when
> overwriting a ref. In that case we get the sha1 from the remote side as
> old. So we could catch that and fall back to all revisions there, but...
>
> ... tl;dr: The solution to use the old revisions from the remote side
> was too simple and does not make matters better but actually worse for
> some typical usecases. Its only in the last patch.
You may not even have the old one in your copy of the remote
repository if you haven't fetched from them and you are forcing your
push. "rev-list <new ones> --not <old ones>" may fail in such a case,
not producing the list of new commits. You'd need to exclude old ones
you learned over the wire that you do not yet have locally.
> The most exact solution would be to use all actual remote refs available
> (not sure if we have them at this point in the process?) another
> solution would be to still append the --remotes=<remotename> option as a
> fallback to reduce the revisions.
I'd say --remotes=<remotename> is the least problematic thing to do.
^ permalink raw reply
* Re: [PATCH 3/2] batch check whether submodule needs pushing into one call
From: Junio C Hamano @ 2016-09-16 17:59 UTC (permalink / raw)
To: Heiko Voigt
Cc: Jeff King, Stefan Beller, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <20160915121044.GA96648@book.hvoigt.net>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> +static void append_hash_to_argv(const unsigned char sha1[20], void *data)
> {
> - if (add_submodule_odb(path) || !lookup_commit_reference(sha1))
> + struct argv_array *argv = (struct argv_array *) data;
> + argv_array_push(argv, sha1_to_hex(sha1));
> +}
Hmph, why do I think I've seen this before in the previous patch?
... scans through this patch and finds that a similar one is
removed ;-)
OK. This makes sense.
> +static void check_has_hash(const unsigned char sha1[20], void *data)
> +{
> + int *has_hash = (int *) data;
> +
> + if (!lookup_commit_reference(sha1))
> + *has_hash = 0;
> +}
> +
> +static int submodule_has_hashes(const char *path, struct sha1_array *hashes)
> +{
> + int has_hash = 1;
> +
> + if (add_submodule_odb(path))
> + return 0;
> +
> + sha1_array_for_each_unique(hashes, check_has_hash, &has_hash);
> + return has_hash;
> +}
> +
> +static int submodule_needs_pushing(const char *path, struct sha1_array *hashes)
> +{
> + if (!submodule_has_hashes(path, hashes))
> return 0;
I think you meant well, but this optimization is wrong. A mere
presence of an object does not mean that the current tip can reach
that object. Imagine you pushed commit A earlier to them at the
tip, then pushed commit A~ to them at the tip, which is the current
state of the remote of the submodule, and since them they may have
GC'ed. They no longer have the commit A.
For that matter, because you are doing this check by pretending as
if all the submodule objects are in the object store of the current
superproject you are working in, and saying "it exists there in the
submodule repository" when the only thing you know is it exists in
an object store of either the submodule repository, the superproject
repository, or any of the other submodule repositories, you really
cannot tell much from a mere presence of an object. Not just the
remote of the submodule repository you are interested in, but the
submodule repository you are interested in itself, may not have that
object.
Drop the previous two helper functions and this short-cut.
> if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
> struct child_process cp = CHILD_PROCESS_INIT;
> - const char *argv[] = {"rev-list", NULL, "--not", "--remotes", "-n", "1" , NULL};
> +
> + argv_array_push(&cp.args, "rev-list");
> + sha1_array_for_each_unique(hashes, append_hash_to_argv, &cp.args);
> + argv_array_pushl(&cp.args, "--not", "--remotes", "-n", "1" , NULL);
> +
> struct strbuf buf = STRBUF_INIT;
> int needs_pushing = 0;
>
> - argv[1] = sha1_to_hex(sha1);
> - cp.argv = argv;
> prepare_submodule_repo_env(&cp.env_array);
> cp.git_cmd = 1;
> cp.no_stdin = 1;
> cp.out = -1;
> cp.dir = path;
> if (start_command(&cp))
> - die("Could not run 'git rev-list %s --not --remotes -n 1' command in submodule %s",
> - sha1_to_hex(sha1), path);
> + die("Could not run 'git rev-list <hashes> --not --remotes -n 1' command in submodule %s",
> + path);
> if (strbuf_read(&buf, cp.out, 41))
> needs_pushing = 1;
> finish_command(&cp);
> @@ -601,21 +628,6 @@ static void find_unpushed_submodule_commits(struct commit *commit,
> diff_tree_combined_merge(commit, 1, &rev);
> }
Good. This is the optimization I alluded to in the review of the
first one in the series.
^ permalink raw reply
* Re: [PATCH 2/2] serialize collection of refs that contain submodule changes
From: Junio C Hamano @ 2016-09-16 17:47 UTC (permalink / raw)
To: Heiko Voigt
Cc: Jeff King, Stefan Beller, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <20160914175130.GB7613@sandbox>
Heiko Voigt <hvoigt@hvoigt.net> writes:
> diff --git a/submodule.c b/submodule.c
> index b04c066..a15e346 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -627,24 +627,31 @@ static void free_submodules_sha1s(struct string_list *submodules)
> string_list_clear(submodules, 1);
> }
>
> -int find_unpushed_submodules(unsigned char new_sha1[20],
> +static void append_hash_to_argv(const unsigned char sha1[20],
> + void *data)
> +{
> + struct argv_array *argv = (struct argv_array *) data;
> + argv_array_push(argv, sha1_to_hex(sha1));
> +}
> +
> +int find_unpushed_submodules(struct sha1_array *hashes,
> const char *remotes_name, struct string_list *needs_pushing)
> {
> struct rev_info rev;
> struct commit *commit;
> - const char *argv[] = {NULL, NULL, "--not", "NULL", NULL};
> - int argc = ARRAY_SIZE(argv) - 1, i;
> - char *sha1_copy;
> + int i;
> struct string_list submodules = STRING_LIST_INIT_DUP;
> + struct argv_array argv = ARGV_ARRAY_INIT;
>
> - struct strbuf remotes_arg = STRBUF_INIT;
> -
> - strbuf_addf(&remotes_arg, "--remotes=%s", remotes_name);
> init_revisions(&rev, NULL);
> - sha1_copy = xstrdup(sha1_to_hex(new_sha1));
> - argv[1] = sha1_copy;
> - argv[3] = remotes_arg.buf;
> - setup_revisions(argc, argv, &rev, NULL);
> +
> + /* argv.argv[0] will be ignored by setup_revisions */
> + argv_array_push(&argv, "find_unpushed_submodules");
> + sha1_array_for_each_unique(hashes, append_hash_to_argv, &argv);
> + argv_array_push(&argv, "--not");
> + argv_array_pushf(&argv, "--remotes=%s", remotes_name);
> +
> + setup_revisions(argv.argc, argv.argv, &rev, NULL);
Yes, its about time to for us to lose that fixed-size argv[] and
replace it with an argv-array ;-).
> if (prepare_revision_walk(&rev))
> die("revision walk setup failed");
So this one used to get a single commit at the tip of what we pushed
in the superproject and was asked "Look at the history we just
pushed leading to the tip commit, and tell me if any of the ones new
to the remote requires submodule commits the remote does not yet
have". Now the caller collects all the tip commits and asks us
once: "Here are the new tips we just pushed; in the history leading
to them, is there a commit that the remote did not have that requires
submodule history the remote does not yet have?".
Makes sort-of sense.
I speculated that you would be doing the same kind of optimization
to feed all positive commits to rev-list at once in each submodule
repository in the review of the previous one, but you didn't do it
here. You did the same for superproject in this step. Perhaps 3 or
4 would do so in the submodule repository.
One thing that makes me worried is how the ref cache layer interacts
with this. I see you first call push_unpushed_submodules() when
ON_DEMAND is set, which would result in pushes in submodule
repositories, updating their remote tracking branches. At that
point, before you make another call to find_unpushed_submodules(),
is our cached ref layer knows that the remote tracking branches
are now up to date (otherwise, we would incorrectly judge that these
submodules need pushing based on stale information)?
> diff --git a/transport.c b/transport.c
> index 94d6dc3..76e1daf 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -903,23 +903,29 @@ int transport_push(struct transport *transport,
>
> if ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) && !is_bare_repository()) {
> struct ref *ref = remote_refs;
> + struct sha1_array hashes = SHA1_ARRAY_INIT;
> +
> for (; ref; ref = ref->next)
> - if (!is_null_oid(&ref->new_oid) &&
> - !push_unpushed_submodules(ref->new_oid.hash,
> - transport->remote->name))
> - die ("Failed to push all needed submodules!");
> + if (!is_null_oid(&ref->new_oid))
> + sha1_array_append(&hashes, ref->new_oid.hash);
> +
> + if (!push_unpushed_submodules(&hashes, transport->remote->name))
> + die ("Failed to push all needed submodules!");
Do we leak the contents of hashes here?
> }
>
> if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
> TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {
> struct ref *ref = remote_refs;
> struct string_list needs_pushing = STRING_LIST_INIT_DUP;
> + struct sha1_array hashes = SHA1_ARRAY_INIT;
>
> for (; ref; ref = ref->next)
> - if (!is_null_oid(&ref->new_oid) &&
> - find_unpushed_submodules(ref->new_oid.hash,
> - transport->remote->name, &needs_pushing))
> - die_with_unpushed_submodules(&needs_pushing);
> + if (!is_null_oid(&ref->new_oid))
> + sha1_array_append(&hashes, ref->new_oid.hash);
> +
> + if (find_unpushed_submodules(&hashes, transport->remote->name,
> + &needs_pushing))
> + die_with_unpushed_submodules(&needs_pushing);
Do we leak the contents of hashes here? I do not think we need to
worry about needs_pushing leaking, as we will always die if it is
not empty, but it might be a better code hygiene to clear it as
well.
> }
>
> push_ret = transport->push_refs(transport, remote_refs, flags);
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/1] git-p4: Add --checkpoint-period option to sync/clone
From: Ori Rawlings @ 2016-09-16 17:43 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, Vitor Antunes, Luke Diamand, Pete Wyckoff
In-Reply-To: <9A490197-3220-4AF9-95DA-89B726A91F92@gmail.com>
On Fri, Sep 16, 2016 at 11:19 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
>
> This looks interesting! I ran into the same issue and added a parameter to the p4 commands to retry (patch not yet proposed to the mailing list):
> https://github.com/autodesk-forks/git/commit/fcfc96a7814935ee6cefb9d69e44def30a90eabb
I was unaware of the retry flag to the p4 command, that seems like a
useful trick too. I think both approaches might pair nicely together
(p4 optimistically retrying, but still falling back to the latest git
checkpoint if we exhaust our N retry attempts).
> Would it make sense to print the "git-p4 resume command" in case an error happens and checkpoints are written?
I was thinking something like this would be a good idea and would
certainly aide in usability. Resuming a sync is fairly
straight-forward (just re-execute the same command). Resuming a clone
is a bit more problematic, today if a depot path argument is provided
to the sync or clone command (and it is always required for clone), no
attempt is made to examine the existing git branches and limit to only
Perforce changes missing from git.
There is a lingering TODO in the script where we check the presence of
the depot path argument, with a suggestion that we should always make
an attempt to continue building upon existing history when it is
available. I think there might be a few edge cases around this
behavior that I'd need to think through. But, if I'm able to address
the TODO, then printing the command to resume the import should be
pretty straight-forward. I'll continue working on that next week.
^ permalink raw reply
* [RFC/PATCH 3/3] mailinfo: handle in-body header continuations
From: Jonathan Tan @ 2016-09-16 17:37 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, peff, gitster
In-Reply-To: <cover.1474047135.git.jonathantanmy@google.com>
Mailinfo currently handles multi-line headers, but it does not handle
multi-line in-body headers. Teach it to handle such headers, for
example, for this input:
Subject: a very long
broken line
Subject: another very long
broken line
interpret the in-body subject to be "another very long broken line"
instead of "another very long".
Instead of repeatedly calling "check_header" (as in this patch), one
alternate method to accomplish this would be to have a buffer of
potential header text in struct mailinfo to be flushed whenever a header
is known to end (for example, if we detect the start of a new header),
but this makes the logic more complicated - for example, the flushing
would not only invoke check_header but would also need to reconstruct
the original lines, possibly decode them into UTF-8, and store them in
log_message, and any failures would be noticed a few "lines" away from
the original failure point. Also, care would need to be taken to flush
the buffer at all appropriate places.
Another alternate would be to modify "read_one_header_line" to accept
strings of lines instead of reading its own from a FILE pointer, but
this would also require a buffer, with the same issues.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
mailinfo.c | 24 ++++++++++++++++++++++--
mailinfo.h | 1 +
t/t4150-am.sh | 23 +++++++++++++++++++++++
t/t5100-mailinfo.sh | 4 ++--
t/t5100/info0018 | 5 +++++
t/t5100/msg0018 | 2 ++
t/t5100/patch0018 | 6 ++++++
t/t5100/sample.mbox | 19 +++++++++++++++++++
8 files changed, 80 insertions(+), 4 deletions(-)
create mode 100644 t/t5100/info0018
create mode 100644 t/t5100/msg0018
create mode 100644 t/t5100/patch0018
diff --git a/mailinfo.c b/mailinfo.c
index 23a56c2..3bbdf74 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -729,8 +729,10 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
if (mi->header_stage) {
char *buf = old_buf ? old_buf : line->buf;
- if (buf[0] == 0 || (buf[0] == '\n' && buf[1] == 0))
+ if (buf[0] == 0 || (buf[0] == '\n' && buf[1] == 0)) {
+ strbuf_reset(&mi->last_inbody_header);
goto handle_commit_msg_out;
+ }
}
if (mi->use_inbody_headers && mi->header_stage) {
@@ -738,8 +740,24 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
size_t len = old_buf ? old_len : line->len;
mi->header_stage = check_header_raw(mi, buf, len,
mi->s_hdr_data, 0);
- if (mi->header_stage)
+ if (mi->header_stage) {
+ strbuf_reset(&mi->last_inbody_header);
+ strbuf_add(&mi->last_inbody_header, buf, len);
goto handle_commit_msg_out;
+ }
+
+ if (mi->last_inbody_header.len &&
+ (buf[0] == ' ' || buf[0] == '\t')) {
+ strbuf_strip_suffix(&mi->last_inbody_header, "\n");
+ strbuf_add(&mi->last_inbody_header, buf, len);
+ mi->header_stage = check_header(mi,
+ &mi->last_inbody_header,
+ mi->s_hdr_data, 1);
+ if (mi->header_stage)
+ goto handle_commit_msg_out;
+ }
+
+ mi->header_stage = 0;
} else
/* Only trim the first (blank) line of the commit message
* when ignoring in-body headers.
@@ -1086,6 +1104,7 @@ void setup_mailinfo(struct mailinfo *mi)
strbuf_init(&mi->email, 0);
strbuf_init(&mi->charset, 0);
strbuf_init(&mi->log_message, 0);
+ strbuf_init(&mi->last_inbody_header, 0);
mi->header_stage = 1;
mi->use_inbody_headers = 1;
mi->content_top = mi->content;
@@ -1099,6 +1118,7 @@ void clear_mailinfo(struct mailinfo *mi)
strbuf_release(&mi->name);
strbuf_release(&mi->email);
strbuf_release(&mi->charset);
+ strbuf_release(&mi->last_inbody_header);
free(mi->message_id);
for (i = 0; mi->p_hdr_data[i]; i++)
diff --git a/mailinfo.h b/mailinfo.h
index 93776a7..ab2d0dd 100644
--- a/mailinfo.h
+++ b/mailinfo.h
@@ -27,6 +27,7 @@ struct mailinfo {
int patch_lines;
int filter_stage; /* still reading log or are we copying patch? */
int header_stage; /* still checking in-body headers? */
+ struct strbuf last_inbody_header;
struct strbuf **p_hdr_data;
struct strbuf **s_hdr_data;
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 9ce9424..89a5bac 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -977,4 +977,27 @@ test_expect_success 'am --patch-format=mboxrd handles mboxrd' '
test_cmp msg out
'
+test_expect_success 'am works with multi-line in-body headers' '
+ FORTY="String that has a length of more than forty characters" &&
+ LONG="$FORTY $FORTY" &&
+ rm -fr .git/rebase-apply &&
+ git checkout -f first &&
+ echo one >> file &&
+ git commit -am "$LONG" --author="$LONG <long@example.com>" &&
+ git format-patch --stdout -1 >patch &&
+ # bump from, date, and subject down to in-body header
+ perl -lpe "
+ if (/^From:/) {
+ print \"From: x <x\@example.com>\";
+ print \"Date: Sat, 1 Jan 2000 00:00:00 +0000\";
+ print \"Subject: x\n\";
+ }
+ " patch >msg &&
+ git checkout HEAD^ &&
+ git am msg &&
+ # Ensure that the author and full message are present
+ git cat-file commit HEAD | grep "^author.*long@example.com" &&
+ git cat-file commit HEAD | grep "^$LONG"
+'
+
test_done
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 1a5a546..99e8722 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \
'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last &&
last=$(cat last) &&
echo total is $last &&
- test $(cat last) = 17'
+ test $(cat last) = 18'
check_mailinfo () {
mail=$1 opt=$2
@@ -51,7 +51,7 @@ test_expect_success 'split box with rfc2047 samples' \
echo total is $last &&
test $(cat rfc2047/last) = 11'
-for mail in rfc2047/00*
+for mail in rfc2047/0001
do
test_expect_success "mailinfo $mail" '
git mailinfo -u $mail-msg $mail-patch <$mail >$mail-info &&
diff --git a/t/t5100/info0018 b/t/t5100/info0018
new file mode 100644
index 0000000..d53e749
--- /dev/null
+++ b/t/t5100/info0018
@@ -0,0 +1,5 @@
+Author: Another Thor
+Email: a.thor@example.com
+Subject: This one contains a tab and a space
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
diff --git a/t/t5100/msg0018 b/t/t5100/msg0018
new file mode 100644
index 0000000..56de83d
--- /dev/null
+++ b/t/t5100/msg0018
@@ -0,0 +1,2 @@
+a commit message
+
diff --git a/t/t5100/patch0018 b/t/t5100/patch0018
new file mode 100644
index 0000000..789df6d
--- /dev/null
+++ b/t/t5100/patch0018
@@ -0,0 +1,6 @@
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index ba8b208..ae61497 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -700,3 +700,22 @@ index e69de29..d95f3ad 100644
+++ b/foo
@@ -0,0 +1 @@
+New content
+From nobody Mon Sep 17 00:00:00 2001
+From: A U Thor <a.u.thor@example.com>
+Subject: check multiline inbody headers
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
+From: Another Thor
+ <a.thor@example.com>
+Subject: This one contains
+ a tab
+ and a space
+
+a commit message
+
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++content
--
2.10.0.rc2.20.g5b18e70
^ permalink raw reply related
* [RFC/PATCH 2/3] mailinfo: correct malformed test example
From: Jonathan Tan @ 2016-09-16 17:37 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, peff, gitster
In-Reply-To: <cover.1474047135.git.jonathantanmy@google.com>
An existing sample message (0015) in the tests for mailinfo contains an
indented line immediately after an in-body header (without any
intervening blank line). Correct this by adding the blank line, in
preparation for a subsequent patch that will treat such indented lines
as RFC 2822 continuation lines (instead of as part of the commit
message).
To ensure that non-indented lines immediately after an in-body header
are still treated correctly (now and in the future), 0008 has been
updated to test both the case when in-body headers are used and the case
when they are not used.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
t/t5100/info0008--no-inbody-headers | 5 +++++
t/t5100/msg0008--no-inbody-headers | 6 ++++++
t/t5100/msg0015--no-inbody-headers | 1 +
t/t5100/patch0008--no-inbody-headers | 0
t/t5100/sample.mbox | 1 +
5 files changed, 13 insertions(+)
create mode 100644 t/t5100/info0008--no-inbody-headers
create mode 100644 t/t5100/msg0008--no-inbody-headers
create mode 100644 t/t5100/patch0008--no-inbody-headers
diff --git a/t/t5100/info0008--no-inbody-headers b/t/t5100/info0008--no-inbody-headers
new file mode 100644
index 0000000..e8a2951
--- /dev/null
+++ b/t/t5100/info0008--no-inbody-headers
@@ -0,0 +1,5 @@
+Author: Junio C Hamano
+Email: junio@kernel.org
+Subject: another patch
+Date: Fri, 9 Jun 2006 00:44:16 -0700
+
diff --git a/t/t5100/msg0008--no-inbody-headers b/t/t5100/msg0008--no-inbody-headers
new file mode 100644
index 0000000..d6e950e
--- /dev/null
+++ b/t/t5100/msg0008--no-inbody-headers
@@ -0,0 +1,6 @@
+From: A U Thor <a.u.thor@example.com>
+Subject: [PATCH] another patch
+>Here is an empty patch from A U Thor.
+
+Hey you forgot the patch!
+
diff --git a/t/t5100/msg0015--no-inbody-headers b/t/t5100/msg0015--no-inbody-headers
index be5115b..44a6ce7 100644
--- a/t/t5100/msg0015--no-inbody-headers
+++ b/t/t5100/msg0015--no-inbody-headers
@@ -1,3 +1,4 @@
From: bogosity
+
- a list
- of stuff
diff --git a/t/t5100/patch0008--no-inbody-headers b/t/t5100/patch0008--no-inbody-headers
new file mode 100644
index 0000000..e69de29
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index 8b2ae06..ba8b208 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -656,6 +656,7 @@ Subject: check bogus body header (from)
Date: Fri, 9 Jun 2006 00:44:16 -0700
From: bogosity
+
- a list
- of stuff
---
--
2.10.0.rc2.20.g5b18e70
^ permalink raw reply related
* [RFC/PATCH 1/3] mailinfo: refactor commit message processing
From: Jonathan Tan @ 2016-09-16 17:37 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, peff, gitster
In-Reply-To: <cover.1474047135.git.jonathantanmy@google.com>
Within the processing of the commit message, check for a scissors line
or a patchbreak line first (before checking for in-body headers) so that
a subsequent patch modifying the processing of in-body headers would not
cause a scissors line or patchbreak line to be misidentified.
If a line could be both an in-body header and a scissors line (for
example, "From: -- >8 --"), this is considered a fatal error
(previously, it would be interpreted as an in-body header). (It is not
possible for a line to be both an in-body header and a patchbreak line,
since both require different prefixes.)
The following enumeration shows that processing is the same except (as
described above) the in-body header + scissors line case.
o in-body header (check_header OK)
o passes UTF-8 conversion
o [described above] is scissors line
o [not possible] is patchbreak line
o [not possible] is blank line
o is none of the above - processed as header
o fails UTF-8 conversion - processed as header
o not in-body header
o passes UTF-8 conversion
o is scissors line - processed as scissors
o is patchbreak line - processed as patchbreak
o is blank line - ignored if in header_stage
o is none of the above - log message
o fails UTF-8 conversion - input error
As for the result left in "line" (after the invocation of
handle_commit_msg), it is unused (by its caller, handle_filter, and by
handle_filter's callers, handle_boundary and handle_body) unless this
line is a patchbreak line, in which case handle_patch is subsequently
called (in handle_filter) on "line". In this case, "line" must have
passed UTF-8 conversion both before and after this patch, so the result
is still the same overall.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
mailinfo.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 115 insertions(+), 30 deletions(-)
diff --git a/mailinfo.c b/mailinfo.c
index e19abe3..23a56c2 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -340,23 +340,56 @@ static struct strbuf *decode_b_segment(const struct strbuf *b_seg)
return out;
}
-static int convert_to_utf8(struct mailinfo *mi,
- struct strbuf *line, const char *charset)
+/*
+ * Attempts to convert line into UTF-8, storing the result in line.
+ *
+ * This differs from convert_to_utf8 in that conversion non-success is not
+ * considered an error case - mi->input_error is not set, and no error message
+ * is printed.
+ *
+ * If the conversion is unnecessary, returns 0 and stores NULL in old_buf (if
+ * old_buf is not NULL).
+ *
+ * If the conversion is successful, returns 0 and stores the unconverted string
+ * in old_buf and old_len (if they are respectively not NULL).
+ *
+ * If the conversion is unsuccessful, returns -1.
+ */
+static int try_convert_to_utf8(const struct mailinfo *mi, struct strbuf *line,
+ const char *charset, char **old_buf,
+ size_t *old_len)
{
- char *out;
+ char *utf8;
- if (!mi->metainfo_charset || !charset || !*charset)
+ if (!mi->metainfo_charset || !charset || !*charset ||
+ same_encoding(mi->metainfo_charset, charset)) {
+ if (old_buf)
+ *old_buf = NULL;
return 0;
+ }
- if (same_encoding(mi->metainfo_charset, charset))
+ utf8 = reencode_string(line->buf, mi->metainfo_charset, charset);
+ if (utf8) {
+ char *temp = strbuf_detach(line, old_len);
+ if (old_buf)
+ *old_buf = temp;
+ strbuf_attach(line, utf8, strlen(utf8), strlen(utf8));
return 0;
- out = reencode_string(line->buf, mi->metainfo_charset, charset);
- if (!out) {
+ }
+ return -1;
+}
+
+/*
+ * Converts line into UTF-8, setting mi->input_error to -1 upon failure.
+ */
+static int convert_to_utf8(struct mailinfo *mi,
+ struct strbuf *line, const char *charset)
+{
+ if (try_convert_to_utf8(mi, line, charset, NULL, NULL)) {
mi->input_error = -1;
return error("cannot convert from %s to %s",
charset, mi->metainfo_charset);
}
- strbuf_attach(line, out, strlen(out), strlen(out));
return 0;
}
@@ -515,6 +548,13 @@ static int check_header(struct mailinfo *mi,
return ret;
}
+static int check_header_raw(struct mailinfo *mi,
+ char *buf, size_t len,
+ struct strbuf *hdr_data[], int overwrite) {
+ const struct strbuf sb = {0, len, buf};
+ return check_header(mi, &sb, hdr_data, overwrite);
+}
+
static void decode_transfer_encoding(struct mailinfo *mi, struct strbuf *line)
{
struct strbuf *ret;
@@ -623,32 +663,48 @@ static int is_scissors_line(const struct strbuf *line)
gap * 2 < perforation);
}
-static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
+static int resembles_rfc2822_header(const struct strbuf *line)
{
- assert(!mi->filter_stage);
+ char *c;
- if (mi->header_stage) {
- if (!line->len || (line->len == 1 && line->buf[0] == '\n'))
+ if (!isalpha(line->buf[0]))
+ return 0;
+
+ for (c = line->buf + 1; *c != 0; c++) {
+ if (*c == ':')
+ return 1;
+ else if (*c != '-' && !isalpha(*c))
return 0;
}
+ return 0;
+}
- if (mi->use_inbody_headers && mi->header_stage) {
- mi->header_stage = check_header(mi, line, mi->s_hdr_data, 0);
- if (mi->header_stage)
- return 0;
- } else
- /* Only trim the first (blank) line of the commit message
- * when ignoring in-body headers.
- */
- mi->header_stage = 0;
+static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
+{
+ int ret = 0;
+ int utf8_result;
+ char *old_buf;
+ size_t old_len;
+
+ assert(!mi->filter_stage);
- /* normalize the log message to UTF-8. */
- if (convert_to_utf8(mi, line, mi->charset.buf))
- return 0; /* mi->input_error already set */
+ /*
+ * Obtain UTF8 for scissors line and patchbreak checks, but retain the
+ * undecoded line in case we need to process it as an in-body header.
+ */
+ utf8_result = try_convert_to_utf8(mi, line, mi->charset.buf, &old_buf,
+ &old_len);
- if (mi->use_scissors && is_scissors_line(line)) {
+ if (!utf8_result && mi->use_scissors && is_scissors_line(line)) {
int i;
+ if (resembles_rfc2822_header(line))
+ /*
+ * Explicitly reject scissor lines that resemble a RFC
+ * 2822 header, to avoid being prone to error.
+ */
+ die("scissors line resembles RFC 2822 header");
+
strbuf_setlen(&mi->log_message, 0);
mi->header_stage = 1;
@@ -661,18 +717,47 @@ static int handle_commit_msg(struct mailinfo *mi, struct strbuf *line)
strbuf_release(mi->s_hdr_data[i]);
mi->s_hdr_data[i] = NULL;
}
- return 0;
+ goto handle_commit_msg_out;
}
-
- if (patchbreak(line)) {
+ if (!utf8_result && patchbreak(line)) {
if (mi->message_id)
strbuf_addf(&mi->log_message,
"Message-Id: %s\n", mi->message_id);
- return 1;
+ ret = 1;
+ goto handle_commit_msg_out;
}
+ if (mi->header_stage) {
+ char *buf = old_buf ? old_buf : line->buf;
+ if (buf[0] == 0 || (buf[0] == '\n' && buf[1] == 0))
+ goto handle_commit_msg_out;
+ }
+
+ if (mi->use_inbody_headers && mi->header_stage) {
+ char *buf = old_buf ? old_buf : line->buf;
+ size_t len = old_buf ? old_len : line->len;
+ mi->header_stage = check_header_raw(mi, buf, len,
+ mi->s_hdr_data, 0);
+ if (mi->header_stage)
+ goto handle_commit_msg_out;
+ } else
+ /* Only trim the first (blank) line of the commit message
+ * when ignoring in-body headers.
+ */
+ mi->header_stage = 0;
+
+ /* If adding as a log message, conversion to UTF-8 is required. */
+ if (utf8_result) {
+ mi->input_error = -1;
+ error("cannot convert from %s to %s",
+ mi->charset.buf, mi->metainfo_charset);
+ goto handle_commit_msg_out;
+ }
strbuf_addbuf(&mi->log_message, line);
- return 0;
+
+handle_commit_msg_out:
+ free(old_buf);
+ return ret;
}
static void handle_patch(struct mailinfo *mi, const struct strbuf *line)
--
2.10.0.rc2.20.g5b18e70
^ permalink raw reply related
* [RFC/PATCH 0/3] handle multiline in-body headers
From: Jonathan Tan @ 2016-09-16 17:37 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, peff, gitster
In-Reply-To: <20160907063819.dd7aulnlsytcuyqj@sigill.intra.peff.net>
Thanks, Peff, for the explanation and the method to reproduce the issue.
The issue seems to be in mailinfo.c - this patch set addresses that, and I have
also included a test for "git am" in t/t4150-am.sh to show the effect of this
patch set on that command.
Jonathan Tan (3):
mailinfo: refactor commit message processing
mailinfo: correct malformed test example
mailinfo: handle in-body header continuations
mailinfo.c | 165 ++++++++++++++++++++++++++++-------
mailinfo.h | 1 +
t/t4150-am.sh | 23 +++++
t/t5100-mailinfo.sh | 4 +-
t/t5100/info0008--no-inbody-headers | 5 ++
t/t5100/info0018 | 5 ++
t/t5100/msg0008--no-inbody-headers | 6 ++
t/t5100/msg0015--no-inbody-headers | 1 +
t/t5100/msg0018 | 2 +
t/t5100/patch0008--no-inbody-headers | 0
t/t5100/patch0018 | 6 ++
t/t5100/sample.mbox | 20 +++++
12 files changed, 206 insertions(+), 32 deletions(-)
create mode 100644 t/t5100/info0008--no-inbody-headers
create mode 100644 t/t5100/info0018
create mode 100644 t/t5100/msg0008--no-inbody-headers
create mode 100644 t/t5100/msg0018
create mode 100644 t/t5100/patch0008--no-inbody-headers
create mode 100644 t/t5100/patch0018
--
2.10.0.rc2.20.g5b18e70
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).