* [RFC 0/1] de-quote quoted-strings in mailinfo
From: Kevin Daudt @ 2016-09-13 23:46 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt
In-Reply-To: <20160913152622.2xtyn6mki6p6afsg@sigill.intra.peff.net>
This is my first 'big' C patch, so first an RFC.
This patch implements RFC2822 dequoting of quoted-pairs in quoted
strings, which was not done yet. This means removing the "\" as escape
character from header fields, but only quoted strings, and comments
(text between braces).
According to the RFC, comments can also appear in square brackets in the
e-mail domain, but that has not been implemented. In fact, just like
other functions, it just looks at the whole header line.
Please let me know what you think.
Kevin Daudt (1):
mailinfo: de-quote quoted-pair in header fields
mailinfo.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
t/t5100-mailinfo.sh | 5 +++++
t/t5100/quoted-pair.expect | 5 +++++
t/t5100/quoted-pair.in | 9 +++++++++
t/t5100/quoted-pair.info | 5 +++++
5 files changed, 70 insertions(+)
create mode 100644 t/t5100/quoted-pair.expect
create mode 100644 t/t5100/quoted-pair.in
create mode 100644 t/t5100/quoted-pair.info
--
2.10.0.rc2
^ permalink raw reply
* [RFC 0/1] mailinfo: de-quote quoted-pair in header fields
From: Kevin Daudt @ 2016-09-13 23:46 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kevin Daudt
In-Reply-To: <20160913234612.22806-1-me@ikke.info>
rfc2822 has provisions for quoted strings in structured header fields,
but also allows for escaping these with so-called quoted-pairs.
git currently does not do anything with this at all, and verbatim takes
over the field body.
Make sure to properly dequote these quoted-strings and comments.
Signed-off-by: Kevin Daudt <me@ikke.info>
---
mailinfo.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
t/t5100-mailinfo.sh | 5 +++++
t/t5100/quoted-pair.expect | 5 +++++
t/t5100/quoted-pair.in | 9 +++++++++
t/t5100/quoted-pair.info | 5 +++++
5 files changed, 70 insertions(+)
create mode 100644 t/t5100/quoted-pair.expect
create mode 100644 t/t5100/quoted-pair.in
create mode 100644 t/t5100/quoted-pair.info
diff --git a/mailinfo.c b/mailinfo.c
index e19abe3..3b7ae8a 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -445,6 +445,51 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)
mi->input_error = -1;
}
+static int unescape_quoted_pair(struct mailinfo *mi, struct strbuf *line)
+{
+ struct strbuf outbuf = STRBUF_INIT;
+ const char *in = line->buf;
+ int c, skip=0;
+ char escape_context=0;
+
+ while ((c = *in++) != 0) {
+ if (!skip) {
+ switch (c) {
+ case '"':
+ if (!escape_context)
+ escape_context = '"';
+ else if (escape_context == '"')
+ escape_context = 0;
+ break;
+ case '\\':
+ if (escape_context) {
+ skip = 1;
+ continue;
+ }
+ break;
+ case '(':
+ if (!escape_context)
+ escape_context = '(';
+ break;
+ case ')':
+ if (escape_context == '(')
+ escape_context = 0;
+ break;
+ }
+ } else {
+ skip = 0;
+ }
+
+ strbuf_addch(&outbuf, c);
+ }
+
+ strbuf_reset(line);
+ strbuf_addbuf(line, &outbuf);
+
+ return 0;
+
+}
+
static int check_header(struct mailinfo *mi,
const struct strbuf *line,
struct strbuf *hdr_data[], int overwrite)
@@ -461,6 +506,7 @@ static int check_header(struct mailinfo *mi,
*/
strbuf_add(&sb, line->buf + len + 2, line->len - len - 2);
decode_header(mi, &sb);
+ unescape_quoted_pair(mi, &sb);
handle_header(&hdr_data[i], &sb);
ret = 1;
goto check_header_out;
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 1a5a546..2be61bf 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -142,4 +142,9 @@ test_expect_success 'mailinfo unescapes with --mboxrd' '
test_cmp expect mboxrd/msg
'
+test_expect_success 'mailinfo unescapes rfc2822 quoted-pair' '
+ git mailinfo /dev/null /dev/null <"$TEST_DIRECTORY"/t5100/quoted-pair.in >"$TEST_DIRECTORY"/t5100/quoted-pair.info &&
+ test_cmp "$TEST_DIRECTORY"/t5100/quoted-pair.expect "$TEST_DIRECTORY"/t5100/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..9fe72e9
--- /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
diff --git a/t/t5100/quoted-pair.info b/t/t5100/quoted-pair.info
new file mode 100644
index 0000000..9fe72e9
--- /dev/null
+++ b/t/t5100/quoted-pair.info
@@ -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
+
--
2.10.0.rc2
^ permalink raw reply related
* Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields
From: Junio C Hamano @ 2016-09-14 0:04 UTC (permalink / raw)
To: Kevin Daudt; +Cc: git
In-Reply-To: <20160913234612.22806-2-me@ikke.info>
Kevin Daudt <me@ikke.info> writes:
> +static int unescape_quoted_pair(struct mailinfo *mi, struct strbuf *line)
> +{
> + struct strbuf outbuf = STRBUF_INIT;
> + const char *in = line->buf;
> + int c, skip=0;
> + char escape_context=0;
Have SP around '=', i.e.
int c, skip = 0;
char escape_context = 0;
> + while ((c = *in++) != 0) {
> + if (!skip) {
> + switch (c) {
> + case '"':
> +...
> + break;
> + }
> + } else {
> + skip = 0;
> + }
> +
> + strbuf_addch(&outbuf, c);
> + }
It often is easier to read if smaller of the two are in the if part
and the larger in else part. Also your switch/case is indented one
level too deep. I.e.
while (...) {
if (skip) {
skip = 0;
} else {
switch (c) {
case '"':
do this;
...
}
}
strbuf_addch(...);
}
I found the variable name "skip" a bit hard to reason about. What
it does is to signal the next round of the processing that we have
seen a single-byte quote and it should keep the byte it will get, no
matter what its value is. It is "skipping" the conditional
processing, but I'd imagine most people would consider it is
"keeping the byte".
> @@ -461,6 +506,7 @@ static int check_header(struct mailinfo *mi,
> */
> strbuf_add(&sb, line->buf + len + 2, line->len - len - 2);
> decode_header(mi, &sb);
> + unescape_quoted_pair(mi, &sb);
> handle_header(&hdr_data[i], &sb);
> ret = 1;
> goto check_header_out;
I wonder why this call is only in here, not on other headers that
all call decode_header(). For that matter, I wonder if the call (or
the logic of the helper function itself) should go at the end of
decode_header(). After all, this is different kind of decoding; the
current one knows how to do b/q encoding but forgot about the more
traditional quoting done with backslash, and you are teaching the
code that the current decoding it does is insufficient and how to
handle the one that the original implementors forgot about.
Thanks.
^ permalink raw reply
* Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields
From: Kevin Daudt @ 2016-09-14 4:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqr38ns5wi.fsf@gitster.mtv.corp.google.com>
On Tue, Sep 13, 2016 at 05:04:45PM -0700, Junio C Hamano wrote:
> Kevin Daudt <me@ikke.info> writes:
>
>
> It often is easier to read if smaller of the two are in the if part
> and the larger in else part. Also your switch/case is indented one
> level too deep. I.e.
>
Thanks, I've switched the order and fixed indentation.
>
> I found the variable name "skip" a bit hard to reason about. What
> it does is to signal the next round of the processing that we have
> seen a single-byte quote and it should keep the byte it will get, no
> matter what its value is. It is "skipping" the conditional
> processing, but I'd imagine most people would consider it is
> "keeping the byte".
Yes, I agree and was trying to find a better name. I have renamed it to
"take_next_literally", which indicates better what it means.
>
> > @@ -461,6 +506,7 @@ static int check_header(struct mailinfo *mi,
> > */
> > strbuf_add(&sb, line->buf + len + 2, line->len - len - 2);
> > decode_header(mi, &sb);
> > + unescape_quoted_pair(mi, &sb);
> > handle_header(&hdr_data[i], &sb);
> > ret = 1;
> > goto check_header_out;
>
> I wonder why this call is only in here, not on other headers that
> all call decode_header(). For that matter, I wonder if the call (or
> the logic of the helper function itself) should go at the end of
> decode_header(). After all, this is different kind of decoding; the
> current one knows how to do b/q encoding but forgot about the more
> traditional quoting done with backslash, and you are teaching the
> code that the current decoding it does is insufficient and how to
> handle the one that the original implementors forgot about.
Makes sense, it should be applied to all headers (I missed the other
decode_header calls).
I will send a new version later.
^ permalink raw reply
* Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields
From: Jeff King @ 2016-09-14 5:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kevin Daudt, git
In-Reply-To: <xmqqr38ns5wi.fsf@gitster.mtv.corp.google.com>
On Tue, Sep 13, 2016 at 05:04:45PM -0700, Junio C Hamano wrote:
> > @@ -461,6 +506,7 @@ static int check_header(struct mailinfo *mi,
> > */
> > strbuf_add(&sb, line->buf + len + 2, line->len - len - 2);
> > decode_header(mi, &sb);
> > + unescape_quoted_pair(mi, &sb);
> > handle_header(&hdr_data[i], &sb);
> > ret = 1;
> > goto check_header_out;
>
> I wonder why this call is only in here, not on other headers that
> all call decode_header(). For that matter, I wonder if the call (or
> the logic of the helper function itself) should go at the end of
> decode_header(). After all, this is different kind of decoding; the
> current one knows how to do b/q encoding but forgot about the more
> traditional quoting done with backslash, and you are teaching the
> code that the current decoding it does is insufficient and how to
> handle the one that the original implementors forgot about.
It has been a while since I looked at rfc2822, but aren't the quoting
and syntax rules different for addresses versus other headers? We would
not want to dequote a Subject header, I think.
-Peff
^ permalink raw reply
* Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields
From: Jeff King @ 2016-09-14 5:13 UTC (permalink / raw)
To: kevin; +Cc: git, Junio C Hamano, Kevin Daudt
In-Reply-To: <20160913234612.22806-2-me@ikke.info>
On Wed, Sep 14, 2016 at 01:46:12AM +0200, Kevin Daudt wrote:
> diff --git a/t/t5100/quoted-pair.expect b/t/t5100/quoted-pair.expect
> new file mode 100644
> index 0000000..9fe72e9
> --- /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
So obviously this is much better than including the backslashed quotes.
But I have to wonder why the first line is not:
Author: Author "The Author" Name
Who is responsible for stripping out the other quotes? I know that they
_do_ get stripped out even in the current code, but it is not clear to
me if that is intentional or an accident.
In Git's world-view (e.g., in commit headers), an ident name continues
until we get to the "<" of the email (or a "\n" terminates the header
line completely). So if mailinfo is converting rfc2822 headers into Git
ident, I'd expect it to fully remove any quotes that are not intended to
be in the name, and everything after "Author: " up to the newline would
become the name.
It's entirely possible I'm missing something subtle about the design of
mailinfo, though.
-Peff
^ permalink raw reply
* Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields
From: Junio C Hamano @ 2016-09-14 5:54 UTC (permalink / raw)
To: Jeff King; +Cc: Kevin Daudt, git
In-Reply-To: <20160914050919.qhv2gxzjyj5ydpub@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> It has been a while since I looked at rfc2822, but aren't the quoting
> and syntax rules different for addresses versus other headers? We would
> not want to dequote a Subject header, I think.
You're absolutely right. RFC2822 does not quite _want_ to dequote
anything. As you pointed out in a separate message, we are the one
who want to strip out "" quoting when mailinfo says
Author: "Jeff King"
to its standard output (aka "info"), and turn it into
GIT_AUTHOR_NAME='Jeff King'
and do so ONLY for the author name.
So I would think it is the responsibility of the one that reads the
"info" file that is produced by mailinfo to dequote the backslash
thing if the mailinfo gave us
Author: "Jeff \"Peff\" King"
^ permalink raw reply
* Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout
From: Oleg Taranenko @ 2016-09-14 6:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ben Peart, git, pclouds, Ben Peart
In-Reply-To: <xmqq7fafv376.fsf@gitster.mtv.corp.google.com>
Sorry for bothering, why not introduce a brand new option like git
checkout -b foo --skip-worktree-merge for such rare optimization use
case?
On Wed, Sep 14, 2016 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ben Peart <peartben@gmail.com> writes:
>
>> +static int needs_working_tree_merge(const struct checkout_opts *opts,
>> + const struct branch_info *old,
>> + const struct branch_info *new)
>> +{
>> +...
>> +}
>
> I do not think I need to repeat the same remarks on the conditions
> in this helper, which hasn't changed since v2. Many "comments" in
> the code do not explain why skipping is justified, or what they
> claim to check looks to me just plain wrong.
>
> For example, there is
>
> /*
> * If we're not creating a new branch, by definition we're changing
> * the existing one so need to do the merge
> */
> if (!opts->new_branch)
> return 1;
>
> but "git checkout" (no other argument) hits this condition. It
> disables the most trivial optimization opportunity, because we are
> not "creating".
>
> "By definition, we're changing"? Really? Not quite.
>
> If you disable this bogus check, "git checkout" (no other argument)
> would be allowed to skip the merge_working_tree(), and that in turn
> reveals another case that the helper is not checking when
> unpack_trees() MUST be called.
>
> Note: namely, when sparse checkout is in effect, switching from
> HEAD to HEAD can nuke existing working tree files outside the
> sparse pattern -- YUCK! See penultimate test in t1011 for
> an example.
>
> This yuckiness is not your fault, but needs_working_tree_merge()
> logic you added needs to refrain from skipping unpack_trees() call
> when sparse thing is in effect. I'd expect "git checkout -b foo"
> instead of "git checkout" (no other argument) would fail to honor
> the sparse thing and reveal this bug, because the above bogus
> "!opts->new_branch" check will not protect you for that case.
>
> In other words, these random series of "if (...) return 1" are bugs
> hiding other real bugs and we need to reason about which ones are
> bugs that are hiding what other bugs that are not covered by this
> function. As Peff said earlier for v1, this is still an unreadable
> mess. We need to figure out a way to make sure we are skipping on
> the right condition and not accidentally hiding a bug of failing to
> check the right condition. I offhand do not have a good suggestion
> on this; sorry.
>
>> static int merge_working_tree(const struct checkout_opts *opts,
>> struct branch_info *old,
>> struct branch_info *new,
>> int *writeout_error)
>> {
>> + /*
>> + * Optimize the performance of "git checkout -b foo" by avoiding
>> + * the expensive merge, index and working directory updates if they
>> + * are not needed.
>> + */
>> + if (!needs_working_tree_merge(opts, old, new))
>> + return 0;
>> +
>> int ret;
>> struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
>
> With the change you made at the beginning of this function, it no
> longer compiles with -Wdecl-after-stmt, but that is the smallest of
> the problems.
>
> It is a small step in the right direction to move the call to the
> helper from the caller to this function, but it is a bit too small.
>
> Notice that the lines after the above context look like this:
>
> hold_locked_index(lock_file, 1);
> if (read_cache_preload(NULL) < 0)
> return error(_("index file corrupt"));
>
> resolve_undo_clear();
> if (opts->force) {
> ret = reset_tree(new->commit->tree, opts, 1, writeout_error);
> if (ret)
> return ret;
> } else {
> struct tree_desc trees[2];
> ...
>
> I would have expected that the check goes inside the "else" thing
> that actually does a two-tree merge, and the helper loses the check
> with opts->force, at least. That would still be a change smaller
> than desired, but at least a meaningful improvement compared to the
> previous one. As I have already pointed out, in the "else" clause
> there is a check "is the index free of conflicted entries? if so
> error out", and that must be honored in !opt->force case, no matter
> what your needs_working_tree_merge() says. I also was hoping that
> you would notice, when you were told about the unmerged check, by
> reading the remainder of the merge_working_tree(), that we need to
> call show_local_changes() when we are not doing force and when we
> are not quiet---returning early like the above patch will never be
> able to call that one downstream in the function.
>
> Regardless of what the actual checks end up to be, the right place
> to do this "optimization" would look more like:
>
> builtin/checkout.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 2b50a49..a6b9e17 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -508,14 +508,19 @@ static int merge_working_tree(const struct checkout_opts *opts,
> topts.dir->flags |= DIR_SHOW_IGNORED;
> setup_standard_excludes(topts.dir);
> }
> +
> + if ( we know we can skip the unpack ) {
> + ret = 0;
> + } else {
> tree = parse_tree_indirect(old->commit ?
> old->commit->object.oid.hash :
> EMPTY_TREE_SHA1_BIN);
> init_tree_desc(&trees[0], tree->buffer, tree->size);
> tree = parse_tree_indirect(new->commit->object.oid.hash);
> init_tree_desc(&trees[1], tree->buffer, tree->size);
> -
> ret = unpack_trees(2, trees, &topts);
> + }
> +
> if (ret == -1) {
> /*
> * Unpack couldn't do a trivial merge; either
>
> I'd think. Note that the determination of "we can skip" would
> involve knowing the object names of the two trees involved, so for
> performance reasons, some of the parse-tree calls may have to come
> before the call to "do we know we can skip?", but that does not
> fundamentally change the basic code structure.
>
> Thanks.
^ permalink raw reply
* [PATCH] vcs-svn/fast_export: fix timestamp fmt specifiers
From: Mike Ralphson @ 2016-09-14 6:40 UTC (permalink / raw)
To: git
Two instances of %ld being used for unsigned longs
Signed-off-by: Mike Ralphson <mike.ralphson@gmail.com>
---
vcs-svn/fast_export.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index bd0f2c2..97cba39 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -73,7 +73,7 @@ void fast_export_begin_note(uint32_t revision, const char *author,
static int firstnote = 1;
size_t loglen = strlen(log);
printf("commit %s\n", note_ref);
- printf("committer %s <%s@%s> %ld +0000\n", author, author, "local", timestamp);
+ printf("committer %s <%s@%s> %lu +0000\n", author, author, "local", timestamp);
printf("data %"PRIuMAX"\n", (uintmax_t)loglen);
fwrite(log, loglen, 1, stdout);
if (firstnote) {
@@ -107,7 +107,7 @@ void fast_export_begin_commit(uint32_t revision, const char *author,
}
printf("commit %s\n", local_ref);
printf("mark :%"PRIu32"\n", revision);
- printf("committer %s <%s@%s> %ld +0000\n",
+ printf("committer %s <%s@%s> %lu +0000\n",
*author ? author : "nobody",
*author ? author : "nobody",
*uuid ? uuid : "local", timestamp);
--
https://github.com/git/git/pull/293
^ permalink raw reply related
* Re: [PATCH 0/16] fix config-reading in non-repos
From: Dennis Kaarsemaker @ 2016-09-14 10:55 UTC (permalink / raw)
To: Jeff King, git; +Cc: Nguyễn Thái Ngọc Duy
In-Reply-To: <20160913032242.coyuhyhn6uklewuk@sigill.intra.peff.net>
On ma, 2016-09-12 at 23:22 -0400, Jeff King wrote:
> The motivation for this series is to fix the regression in v2.9 where
> core.logallrefupdates is sometimes not set properly in a newly
> initialized repository, as described in this thread:
>
> http://public-inbox.org/git/c46d36ef-3c2e-374f-0f2e-ffe31104e023@gmx.de/T/#u
>
> The root of the problem is that we are overly eager to read and use
> config from ".git/config", even when we have not established that it is
> part of a repository. This is especially bad for git-init, which would
> not want to read anything until we've created the new repo.
>
> So the two interesting parts of the fix are:
>
> 1. We stop blindly reading ".git/config" when we don't know there's an
> actual git directory. This is in patch 14, and is actually enough
> to fix the v2.9 regression.
>
> 2. We are more thorough about dropping any cached config values when
> we move into the new repository in git-init (patch 16).
>
> I didn't dig into when this was broken, but it was probably when we
> switched git_config() to use cached values in the v2.2.0
> time-frame.
>
> Doing (1) required fixing up some builtins that depended on the blind
> .git/config thing, as the tests demonstrated. But I think this is a sign
> that we are moving in the right direction, because each one of those
> programs could easily be demonstrated to be broken in scenarios only
> slightly more exotic than the test scripts (e.g., see patch 3 for one of
> the simplest cases).
>
> So I think notwithstanding their use as prep for patch 14, patches 1-13
> fix useful bugs.
>
> I won't be surprised if there are other fallouts that were not caught by
> the test suite (i.e., programs that expect to read config, don't do
> RUN_SETUP, but aren't covered well by tests). I poked around the list of
> builtins in git.c that do not use RUN_SETUP, and they seem to correctly
> end up in setup_git_directory_gently() before reading config. But it's
> possible I missed a case.
>
> So this is definitely a bit larger than I'd hope for a regression-fix to
> maint. But anything that doesn't address this issue at the config layer
> is going to end up as a bit of a hack, and I'd rather not pile up hacks
> if we can avoid it.
Agreed with all of the above, this is much better than just fixing the
symptom on the mailinglist thread that started this.
> I've cc'd Dennis, who helped investigate solutions in the thread
> mentioned above, and Duy, because historically he has been the one most
> willing and able to battle the dragon of our setup code. :)
>
> [01/16]: t1007: factor out repeated setup
> [02/16]: hash-object: always try to set up the git repository
> [03/16]: patch-id: use RUN_SETUP_GENTLY
> [04/16]: diff: skip implicit no-index check when given --no-index
> [05/16]: diff: handle --no-index prefixes consistently
> [06/16]: diff: always try to set up the repository
> [07/16]: pager: remove obsolete comment
> [08/16]: pager: stop loading git_default_config()
> [09/16]: pager: make pager_program a file-local static
> [10/16]: pager: use callbacks instead of configset
> [11/16]: pager: handle early config
> [12/16]: t1302: use "git -C"
> [13/16]: test-config: setup git directory
> [14/16]: config: only read .git/config from configured repos
> [15/16]: init: expand comments explaining config trickery
> [16/16]: init: reset cached config when entering new repo
Couldn't find anything to comment on, and I've tested that this does
indeed fix the symptoms we saw.
Reviewed-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
--
Dennis Kaarsemaker
http://www.kaarsemaker.net
^ permalink raw reply
* Re: Git Ignore Exception bug
From: Dennis Kaarsemaker @ 2016-09-14 11:26 UTC (permalink / raw)
To: Nathan Williams, git; +Cc: Eric Severtson
In-Reply-To: <CAC5iUd2mS1n1=sRB=Bs6tn3L3raAXrZaEcs765UFtQZB9XZuYg@mail.gmail.com>
On vr, 2016-09-09 at 15:39 -0600, Nathan Williams wrote:
> it ignore doesn't seem to be working properly when adding exceptions.
>8 -- snip testcase
> Expected results
> % git st
> On branch master
> Untracked files:
> (use "git add <file>..." to include in what will be committed)
>
> foo/bar/
That expectation is wrong, it should show foo/. And indeed it does
(tested with 2.9.0 and 2.10.0-rc1)
$ sh -x testscript
+ rm -rf repo
+ mkdir repo
+ cd repo
+ git init
Initialized empty Git repository in /home/dennis/code/git/repo/.git/
+ echo foo/*
+ echo !foo/bar
+ git add .gitignore
+ git commit -m Ignore file with exceptions
[master (root-commit) 7e1b82a] Ignore file with exceptions
1 file changed, 2 insertions(+)
create mode 100644 .gitignore
+ mkdir foo
+ mkdir foo/bar
+ touch foo/1
+ touch foo/2
+ touch foo/bar/a
+ touch foo/bar/b
+ git status
On branch master
Untracked files:
(use "git add <file>..." to include in what will be committed)
foo/
nothing added to commit but untracked files present (use "git add" to track)
--
Dennis Kaarsemaker
http://www.kaarsemaker.net
^ permalink raw reply
* Re: [RFC 0/3] http: avoid repeatedly adding curl easy to curlm
From: Yaroslav Halchenko @ 2016-09-14 11:59 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Jeff King, Junio C Hamano
In-Reply-To: <20160913002557.10671-1-e@80x24.org>
On Tue, 13 Sep 2016, Eric Wong wrote:
> What is unclear to me is how only Yaroslav's repository seems to
> trigger this bug after all these years...
Thank you Eric very much for tracking down this issue! Since issue is
intermittent, I guess people just didn't bother going through reporting
if they got an error once in a while, and/or could have attributed
to somehow misran update-server-info.
> However, I am fairly sure this fixes the bug Yaroslav
> encountered. This patch series is also needed for 2.9.3 and
> perhaps older maintenance tracks for distros.
FWIW I have tested your branch locally - and do not observe that bug any
longer. Thanks again!
--
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik
^ permalink raw reply
* [ANNOUNCE] Git Rev News edition 19
From: Christian Couder @ 2016-09-14 12:52 UTC (permalink / raw)
To: git
Cc: Thomas Ferris Nicolaisen, Nicola Paolucci, Junio C Hamano,
Jeff King, Jakub Narębski, Josh Triplett, Lars Schneider,
brian m. carlson, Matthieu Moy, Kevin Willford,
Johannes Schindelin, Pranit Bauva, lwn
Hi everyone,
I'm happy announce that the 19th edition of Git Rev News is now published:
https://git.github.io/rev_news/2016/09/14/edition-19/
Thanks a lot to all the contributors and helpers, especially Brian,
Jakub, Lars and Josh!
Enjoy,
Christian and Thomas.
^ permalink raw reply
* [PATCH] pkt-line: mark a file-local symbol static
From: Ramsay Jones @ 2016-09-14 13:31 UTC (permalink / raw)
To: Lars Schneider; +Cc: Junio C Hamano, GIT Mailing-list
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Lars,
If you need to re-roll your 'ls/filter-process' branch, could you
please squash this into the relevant patch; commit 2afd9b22
("pkt-line: add packet_write_gently()", 08-09-2016).
[If you think the symbol should be public (I don't), then add a
suitable declaration to pkt-line.h instead.]
Thanks!
ATB,
Ramsay Jones
pkt-line.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pkt-line.c b/pkt-line.c
index 538e35f..4900fc0 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -181,7 +181,7 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
return status;
}
-int packet_write_gently(const int fd_out, const char *buf, size_t size)
+static int packet_write_gently(const int fd_out, const char *buf, size_t size)
{
static char packet_write_buffer[LARGE_PACKET_MAX];
--
2.10.0
^ permalink raw reply related
* git submodule add spits unrelated to actual problem error msg about .gitignore
From: Yaroslav Halchenko @ 2016-09-14 14:03 UTC (permalink / raw)
To: git
In-Reply-To: <20160909221942.GS9830@onerussian.com>
I have spent some time chasing the wild goose (well - the .gitignore
file) after getting:
$> git-submodule add --name fcx-1 ./fcx-1/ ./fcx-1/
The following path is ignored by one of your .gitignore files:
fcx-1
Use -f if you really want to add it.
long story short -- the culprit is this piece of code in git-submodule:
if test -z "$force" && ! git add --dry-run --ignore-missing "$sm_path" > /dev/null 2>&1
then
eval_gettextln "The following path is ignored by one of your .gitignore files:
\$sm_path
Use -f if you really want to add it." >&2
exit 1
fi
so if anything goes wrong in git add, it just reports this error
message.
FTR -- actual problem in my case was:
$> git add --dry-run --ignore-missing fcx-1
fatal: Unable to create '/mnt/datasets/datalad/crawl/crcns/.git/index.lock': File exists.
Another git process seems to be running in this repository, e.g.
an editor opened by 'git commit'. Please make sure all processes
are terminated then try again. If it still fails, a git process
may have crashed in this repository earlier:
remove the file manually to continue.
;-)
Cheers!
P.S. Please CC me in replies
--
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik
^ permalink raw reply
* Re: [PATCH 0/16] fix config-reading in non-repos
From: Jeff King @ 2016-09-14 15:31 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: git, Nguyễn Thái Ngọc Duy
In-Reply-To: <1473850541.30531.18.camel@kaarsemaker.net>
On Wed, Sep 14, 2016 at 12:55:41PM +0200, Dennis Kaarsemaker wrote:
> > [14/16]: config: only read .git/config from configured repos
> > [15/16]: init: expand comments explaining config trickery
> > [16/16]: init: reset cached config when entering new repo
>
> Couldn't find anything to comment on, and I've tested that this does
> indeed fix the symptoms we saw.
>
> Reviewed-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Thanks. Based on our conversations earlier, I tried to dig around for
other cases that might be broken, especially with the "reinit" cases.
The result is the tests in patch 16. But let me know if you can think of
anything else that might be broken.
-Peff
^ permalink raw reply
* Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout
From: Junio C Hamano @ 2016-09-14 15:48 UTC (permalink / raw)
To: Oleg Taranenko; +Cc: Ben Peart, git, pclouds, Ben Peart
In-Reply-To: <CABEd3j_FrWhOe_jXcc+VJWiyy80SG1JfbZC9woRU2cqdzjkGyA@mail.gmail.com>
Oleg Taranenko <olegtaranenko@gmail.com> writes:
> Sorry for bothering, why not introduce a brand new option like git
> checkout -b foo --skip-worktree-merge for such rare optimization use
> case?
I am not sure what problem such a new option solves. How would you
describe and explain what "--skip-worktree-merge" option to the end
user?
^ permalink raw reply
* Re: [PATCH] vcs-svn/fast_export: fix timestamp fmt specifiers
From: Junio C Hamano @ 2016-09-14 15:56 UTC (permalink / raw)
To: Mike Ralphson; +Cc: git
In-Reply-To: <01020157276d4d1f-9c995462-4aea-4949-8d29-3dbdbec77dd7-000000@eu-west-1.amazonses.com>
Mike Ralphson <mike.ralphson@gmail.com> writes:
> Two instances of %ld being used for unsigned longs.
>
> Signed-off-by: Mike Ralphson <mike.ralphson@gmail.com>
> ---
Good eyes. Thanks for spotting.
> vcs-svn/fast_export.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
> index bd0f2c2..97cba39 100644
> --- a/vcs-svn/fast_export.c
> +++ b/vcs-svn/fast_export.c
> @@ -73,7 +73,7 @@ void fast_export_begin_note(uint32_t revision, const char *author,
> static int firstnote = 1;
> size_t loglen = strlen(log);
> printf("commit %s\n", note_ref);
> - printf("committer %s <%s@%s> %ld +0000\n", author, author, "local", timestamp);
> + printf("committer %s <%s@%s> %lu +0000\n", author, author, "local", timestamp);
> printf("data %"PRIuMAX"\n", (uintmax_t)loglen);
> fwrite(log, loglen, 1, stdout);
> if (firstnote) {
> @@ -107,7 +107,7 @@ void fast_export_begin_commit(uint32_t revision, const char *author,
> }
> printf("commit %s\n", local_ref);
> printf("mark :%"PRIu32"\n", revision);
> - printf("committer %s <%s@%s> %ld +0000\n",
> + printf("committer %s <%s@%s> %lu +0000\n",
> *author ? author : "nobody",
> *author ? author : "nobody",
> *uuid ? uuid : "local", timestamp);
>
> --
> https://github.com/git/git/pull/293
^ permalink raw reply
* Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields
From: Kevin Daudt @ 2016-09-14 16:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqmvjbrpp4.fsf@gitster.mtv.corp.google.com>
On Tue, Sep 13, 2016 at 10:54:47PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > It has been a while since I looked at rfc2822, but aren't the quoting
> > and syntax rules different for addresses versus other headers? We would
> > not want to dequote a Subject header, I think.
>
> You're absolutely right. RFC2822 does not quite _want_ to dequote
> anything. As you pointed out in a separate message, we are the one
> who want to strip out "" quoting when mailinfo says
>
> Author: "Jeff King"
>
> to its standard output (aka "info"), and turn it into
>
> GIT_AUTHOR_NAME='Jeff King'
>
> and do so ONLY for the author name.
>
> So I would think it is the responsibility of the one that reads the
> "info" file that is produced by mailinfo to dequote the backslash
> thing if the mailinfo gave us
>
> Author: "Jeff \"Peff\" King"
>
The RFC makes a distinction between structured fields and unstructured
fields. Quoting would not even be necessary for unstructured fields
(like Subject), so yes, that those fields should be left alone.
Unstructures fields are subject, comments, keywords and optional fields,
the rest is considered structured.
Because the only field where this is a problem is the From field, I
think it would be safe to limit the unquoting just to that field.
My reasoning to do the unquoting here is because it's the RFC requires
the quoting in the first place.
I already noticed a bug in the current unquoting of the author when
adding a comment to the From: field.
From: "A U Thor" <au@thor.com> (test)
When applied the the author of this patch shows up as:
Author: A U Thor" (test) <au@thor.com>
So I agree with Jeff[1] where he states that the surrounding quotes
should be removed, if that's not a problem for git.
[1]:https://public-inbox.org/git/20160914051305.vphknpsikyxi3hg3@sigill.intra.peff.net/
^ permalink raw reply
* [PATCH] xdiff: fix merging of hunks with -W context and -u context
From: René Scharfe @ 2016-09-14 16:05 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
If the function context for a hunk (with -W) reaches the beginning of
the next hunk then we need to merge these two -- otherwise we'd show
some lines twice, which looks strange and even confuses git apply. We
already do this checking and merging in xdl_emit_diff(), but forget to
consider regular context (with -u or -U).
Fix that by merging hunks already if function context of the first one
touches or overlaps regular context of the second one.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
t/t4051-diff-function-context.sh | 25 +++++++++++++++++++++++++
xdiff/xemit.c | 2 +-
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index b79b877..6154acb 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -67,6 +67,15 @@ test_expect_success 'setup' '
commit_and_tag long_common_tail file.c &&
git checkout initial &&
+ cat "$dir/hello.c" "$dir/dummy.c" >file.c &&
+ commit_and_tag hello_dummy file.c &&
+
+ # overlap function context of 1st change and -u context of 2nd change
+ grep -v "delete me from hello" <"$dir/hello.c" >file.c &&
+ sed 2p <"$dir/dummy.c" >>file.c &&
+ commit_and_tag changed_hello_dummy file.c &&
+
+ git checkout initial &&
grep -v "delete me from hello" <file.c >file.c.new &&
mv file.c.new file.c &&
cat "$dir/appended1.c" >>file.c &&
@@ -179,4 +188,20 @@ test_expect_success ' context does not include other functions' '
test $(grep -c "^[ +-].*Begin" changed_hello_appended.diff) -le 2
'
+check_diff changed_hello_dummy 'changed two consecutive functions'
+
+test_expect_success ' context includes begin' '
+ grep "^ .*Begin of hello" changed_hello_dummy.diff &&
+ grep "^ .*Begin of dummy" changed_hello_dummy.diff
+'
+
+test_expect_success ' context includes end' '
+ grep "^ .*End of hello" changed_hello_dummy.diff &&
+ grep "^ .*End of dummy" changed_hello_dummy.diff
+'
+
+test_expect_success ' overlapping hunks are merged' '
+ test $(grep -c "^@@" changed_hello_dummy.diff) -eq 1
+'
+
test_done
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index b52b4b9..7389ce4 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -239,7 +239,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
if (xche->next) {
long l = XDL_MIN(xche->next->i1,
xe->xdf1.nrec - 1);
- if (l <= e1 ||
+ if (l - xecfg->ctxlen <= e1 ||
get_func_line(xe, xecfg, NULL, l, e1) < 0) {
xche = xche->next;
goto post_context_calculation;
--
2.10.0
^ permalink raw reply related
* Re: Git Miniconference at Plumbers
From: Christian Couder @ 2016-09-14 16:27 UTC (permalink / raw)
To: Lars Schneider
Cc: Junio C Hamano, Jon Loeliger, David Bainbridge, Jeff King,
git@vger.kernel.org
In-Reply-To: <1019E7FD-0AC0-4BCE-B810-BE20968DFEE9@gmail.com>
On Tue, Sep 13, 2016 at 1:14 AM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
>> On 12 Sep 2016, at 21:11, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> [..]
>> properly; supporting "huge objects" better in the object layer,
>> without having to resort to ugly hacks like GitLFS that will never
>> be part of the core Git. [...]
>
> I agree with you that GitLFS is an ugly hack.
>
> Some applications have test data, image assets, and other data sets that
> need to be versioned along with the source code.
>
> How would you deal with these kind of "huge objects" _today_?
I think that Junio was saying that this problem and other problems
like this one are indeed itches for some people, but maybe not for
kernel community.
About this specific problem, as you probably know, I started working
on adding support for external object databases, on top of some
previous work that Peff had started some years ago:
https://public-inbox.org/git/20160628181933.24620-1-chriscool@tuxfamily.org/
So if you want to better deal with huge objects in the near future,
you are welcome to help on this.
^ permalink raw reply
* Re: Git Miniconference at Plumbers
From: Junio C Hamano @ 2016-09-14 17:26 UTC (permalink / raw)
To: Lars Schneider
Cc: Jon Loeliger, David Bainbridge, Jeff King, git@vger.kernel.org
In-Reply-To: <1019E7FD-0AC0-4BCE-B810-BE20968DFEE9@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
> Some applications have test data, image assets, and other data sets that
> need to be versioned along with the source code.
>
> How would you deal with these kind of "huge objects" _today_?
When you know that you'd find the answer to that question totally
uninteresting, why do you even bother to ask? ;-)
I don't, and if I had to, I would deal with them just like any other
objects.
A more interesting pair of questions to ask would be what the
fundamental requirement for an acceptable solution is, and what
solution within the constraint I would envision, if I were given a
group competent Git hackers and enough time to realize it.
The most important constraint is that any acceptable solution should
preserve the object identity.
And starting from a "I don't but if I had to..." repository that is
created in a dumb way, a solution that satisifies the constraint may
work like this, requiring enhancements to various parts of the
system:
- The "upload-pack" protocol would allow the owner of a such
repository and the party that "git clone"'s from there to
negotiate:
. what it means for a object to be "huge" (e.g. the owner may
implicitly show the preference by marking a packfile as
containing such "huge" objects, may configure that blobs that
appear at paths that match certain glob pattern are "huge", or
the sender and the receiver may say objects that are larger
than X MB are "huge", etc.); and
. what to do with "huge" objects (e.g. the receiver may ask for
a full clone, or the receiver may ask to omit "huge" ones from
the initial transfer)
- The "upload-pack" protocol would give, in addition to the normal
pack stream that conveys only non-"huge" objects, for each of
"huge" objects that are not transferred, what its object name is
and how it can later be retrieved.
- Just like packing objects in packfiles was added as a different
implementation to store objects in the object database that is
better than storing them individually as loose object files,
there will be a third way to store such "huge" object _in_ the
object database, which may actually not _store_ them locally at
all. The local object store may merely have placeholders for
them, in which instructions for how it can be acquired when
necessary are stored. The extra information sent over the
"upload-pack" protocol for "huge" objects with the previous
bullet-point are used to store these objects in this "third" way.
- A new mechanism would allow such objects that are stored in this
"third" way to be retrieved lazily or on-demand.
There are other enhancements whose necessity will fall naturally out
of such a lazy scheme outlined above. E.g. "fsck" needs to learn
that the objects stored in the third way are considered to "exist"
but their actual contents is not expected to be verifiable until
they are retrieved. "send-pack" (i.e. running "git push" from a
repository cloned with the procedure outlined above) needs to treat
the objects stored in the third way differently (most likely, it
will fail a request for full-clone and send "not here, but you can
get it this way" for them). Local operations that need more than
object names need to learn reasonable fallback behaviours to work
when the actual object contents are not yet available (e.g. all of
them may offer "this is not yet available; do you want to get
on-demand?" or there may even be "object.ondemand" configuration
option to skip the end-user interaction. When on-demand retrieving
is not done, "git archive" may place a placeholder file in its
output that says "no data (yet) here", "git log --raw" may show the
object name but "git log -p" may say "insufficient data to produce a
patch", etc.) [*1*].
Because we start from the "object identity should not change", you
do not have to make a decision upfront when preparing the ultimate
source of the truth. When you take a clone-network of a single
project as a whole, somebody needs to hold the entire set of objects
somewhere, and many of the repository in the clone-network may have
"huge" objects in the third "not here yet, here is how to get it"
form. As the system improves, and as the networking and storage
technology changes, the definition of "huge" WILL change over time
and those repositories can turn the ones that used to be "huge" into
normal objects.
If you use approaches taken by various clean/smudge based current
crop of solutions [*2*], on the other hand, once you decide a blob
object is "huge" and needs to be replaced with a surrogate (to be
instantiated via the "clean" filter), the "huge" object _has_ to
stay in the surrogate form in the containing tree and you cannot
change the division between "huge" and "normal" ever without
rewriting the history.
[Footnote]
*1* Astute readers would realize that the utility of such a "third
way" object storage mechanism is not limited to "keep and
transfer huge objects lazily". The same mechanism can say "not
yet here, and there is no way for _you_ to retrieve the
contents", which is an effective way to "obliterate" an object.
*2* I called them "hacks" because they are practical compromise that
can be done with today's Git, while sidestepping harder problems
that are needed to be solved to realize the solution outlined
above.
^ permalink raw reply
* [PATCH 1/2] serialize collection of changed submodules
From: Heiko Voigt @ 2016-09-14 17:31 UTC (permalink / raw)
To: Jeff King
Cc: Stefan Beller, Junio C Hamano, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <20160824230115.jhmcr4r7wobj5ejb@sigill.intra.peff.net>
To check whether a submodule needs to be pushed we need to collect all
changed submodules. Lets collect them first and then execute the
possibly expensive test whether certain revisions are already pushed
only once per submodule.
There is further potential for optimization since we can assemble one
command and only issued that instead of one call for each remote ref in
the submodule.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
Sorry about the late reply. I was not able to process emails until now.
Here are two patches that should help to improve the situation and batch
up some processing. This one is for repositories with submodules, so
that they do not iterate over the same submodule twice with the same
hash.
The second one will be the one people without submodules are interested
in.
Cheers Heiko
submodule.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 62 insertions(+), 5 deletions(-)
diff --git a/submodule.c b/submodule.c
index 0ef2ff4..b04c066 100644
--- a/submodule.c
+++ b/submodule.c
@@ -554,19 +554,38 @@ static int submodule_needs_pushing(const char *path, const unsigned char sha1[20
return 0;
}
+static struct sha1_array *get_sha1s_from_list(struct string_list *submodules,
+ const char *path)
+{
+ struct string_list_item *item;
+ struct sha1_array *hashes;
+
+ item = string_list_insert(submodules, path);
+ if (item->util)
+ return (struct sha1_array *) item->util;
+
+ hashes = (struct sha1_array *) xmalloc(sizeof(struct sha1_array));
+ /* NEEDSWORK: should we add an initializer function for
+ * sha1_array ? */
+ memset(hashes, 0, sizeof(struct sha1_array));
+ item->util = hashes;
+ return hashes;
+}
+
static void collect_submodules_from_diff(struct diff_queue_struct *q,
struct diff_options *options,
void *data)
{
int i;
- struct string_list *needs_pushing = data;
+ struct string_list *submodules = data;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
+ struct sha1_array *hashes;
if (!S_ISGITLINK(p->two->mode))
continue;
- if (submodule_needs_pushing(p->two->path, p->two->oid.hash))
- string_list_insert(needs_pushing, p->two->path);
+ hashes = get_sha1s_from_list(submodules, p->two->path);
+ sha1_array_append(hashes, p->two->oid.hash);
}
}
@@ -582,14 +601,41 @@ static void find_unpushed_submodule_commits(struct commit *commit,
diff_tree_combined_merge(commit, 1, &rev);
}
+struct collect_submodule_from_sha1s_data {
+ char *submodule_path;
+ struct string_list *needs_pushing;
+};
+
+static void collect_submodules_from_sha1s(const unsigned char sha1[20],
+ void *data)
+{
+ struct collect_submodule_from_sha1s_data *me =
+ (struct collect_submodule_from_sha1s_data *) data;
+
+ if (submodule_needs_pushing(me->submodule_path, sha1))
+ string_list_insert(me->needs_pushing, me->submodule_path);
+}
+
+static void free_submodules_sha1s(struct string_list *submodules)
+{
+ int i;
+ for (i = 0; i < submodules->nr; i++) {
+ struct string_list_item *item = &submodules->items[i];
+ struct sha1_array *hashes = (struct sha1_array *) item->util;
+ sha1_array_clear(hashes);
+ }
+ string_list_clear(submodules, 1);
+}
+
int find_unpushed_submodules(unsigned char new_sha1[20],
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;
+ int argc = ARRAY_SIZE(argv) - 1, i;
char *sha1_copy;
+ struct string_list submodules = STRING_LIST_INIT_DUP;
struct strbuf remotes_arg = STRBUF_INIT;
@@ -603,12 +649,23 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
die("revision walk setup failed");
while ((commit = get_revision(&rev)) != NULL)
- find_unpushed_submodule_commits(commit, needs_pushing);
+ find_unpushed_submodule_commits(commit, &submodules);
reset_revision_walk();
free(sha1_copy);
strbuf_release(&remotes_arg);
+ for (i = 0; i < submodules.nr; i++) {
+ struct string_list_item *item = &submodules.items[i];
+ struct collect_submodule_from_sha1s_data data;
+ data.submodule_path = item->string;
+ data.needs_pushing = needs_pushing;
+ sha1_array_for_each_unique((struct sha1_array *) item->util,
+ collect_submodules_from_sha1s,
+ &data);
+ }
+ free_submodules_sha1s(&submodules);
+
return needs_pushing->nr;
}
--
2.0.2.832.g083c931
^ permalink raw reply related
* Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields
From: Junio C Hamano @ 2016-09-14 17:43 UTC (permalink / raw)
To: Kevin Daudt; +Cc: Jeff King, git
In-Reply-To: <20160914160308.GB26893@ikke.info>
Kevin Daudt <me@ikke.info> writes:
> When applied the the author of this patch shows up as:
>
> Author: A U Thor" (test) <au@thor.com>
>
> So I agree with Jeff[1] where he states that the surrounding quotes
> should be removed, if that's not a problem for git.
>
> [1]:https://public-inbox.org/git/20160914051305.vphknpsikyxi3hg3@sigill.intra.peff.net/
I think we can go either way and it does not matter all that much if
"mailinfo" changes its output or the reader of "mailinfo" output
changes its input--we will either be munging data read from "From:"
when producing the "Author:" line, or taking the "Author:" output by
mailinfo and removing the quotes.
As an output from mailinfo that looks like this:
Author: "A U Thor"
Email: au@thor.com
is made into a commit object that has this:
author A U Thor <au@thor.com>
we know that the reader of mailinfo output _already_ has some logic
to strip the surrounding double quotes. That is the only reason why
I think it is a better approach to not dequote in the "mailinfo" but
in the reader to turn
Author: "A \"U\" Thor"
Email: au@thor.com
into a commit object that has this:
author A "U" Thor <au@thor.com>
than updating mailinfo to produce
Author: A "U" Thor
Email: au@thor.com
and then create the same result.
^ permalink raw reply
* [PATCH 2/2] serialize collection of refs that contain submodule changes
From: Heiko Voigt @ 2016-09-14 17:51 UTC (permalink / raw)
To: Jeff King
Cc: Stefan Beller, Junio C Hamano, git@vger.kernel.org, Jens Lehmann,
Fredrik Gustafsson, Leandro Lucarella
In-Reply-To: <20160824230115.jhmcr4r7wobj5ejb@sigill.intra.peff.net>
We are iterating over each pushed ref and want to check whether it
contains changes to submodules. Instead of immediately checking each ref
lets first collect them and then do the check for all of them in one
revision walk.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
Sorry this was not catched earlier. This was implemented as part of
summer of code and it seems we never tested with --mirror.
This is the one which does only one revision walk instead of one for
each ref. Here are some numbers (using the my development clone of git
itself) from my local machine:
rm -rf <test-git> && mkdir <test-git> &&
(cd <test-git> && git init) &&
time git push --mirror <test-git>
real 0m16.056s
user 0m24.424s
sys 0m1.380s
real 0m15.885s
user 0m24.204s
sys 0m1.296s
real 0m16.731s
user 0m24.176s
sys 0m1.244s
rm -rf <test-git> && mkdir <test-git> &&
(cd <test-git> && git init) &&
time git push --mirror --recurse-submodules=check <test-git>
real 0m21.441s
user 0m29.560s
sys 0m1.480s
real 0m21.319s
user 0m29.484s
sys 0m1.464s
real 0m21.261s
user 0m29.252s
sys 0m1.592s
Without my patches and --recurse-submodules=check the numbers are
basically the same. I stopped the test with --recurse-submodules=check
after ~ 9 minutes.
Cheers Heiko
submodule.c | 36 +++++++++++++++++++++---------------
submodule.h | 5 +++--
transport.c | 22 ++++++++++++++--------
3 files changed, 38 insertions(+), 25 deletions(-)
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);
if (prepare_revision_walk(&rev))
die("revision walk setup failed");
@@ -652,8 +659,7 @@ int find_unpushed_submodules(unsigned char new_sha1[20],
find_unpushed_submodule_commits(commit, &submodules);
reset_revision_walk();
- free(sha1_copy);
- strbuf_release(&remotes_arg);
+ argv_array_clear(&argv);
for (i = 0; i < submodules.nr; i++) {
struct string_list_item *item = &submodules.items[i];
@@ -691,12 +697,12 @@ static int push_submodule(const char *path)
return 1;
}
-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name)
+int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name)
{
int i, ret = 1;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
- if (!find_unpushed_submodules(new_sha1, remotes_name, &needs_pushing))
+ if (!find_unpushed_submodules(hashes, remotes_name, &needs_pushing))
return 1;
for (i = 0; i < needs_pushing.nr; i++) {
diff --git a/submodule.h b/submodule.h
index d9e197a..065b2f0 100644
--- a/submodule.h
+++ b/submodule.h
@@ -3,6 +3,7 @@
struct diff_options;
struct argv_array;
+struct sha1_array;
enum {
RECURSE_SUBMODULES_CHECK = -4,
@@ -62,9 +63,9 @@ int submodule_uses_gitfile(const char *path);
int ok_to_remove_submodule(const char *path);
int merge_submodule(unsigned char result[20], const char *path, const unsigned char base[20],
const unsigned char a[20], const unsigned char b[20], int search);
-int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,
+int find_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name,
struct string_list *needs_pushing);
-int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
+int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name);
void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
int parallel_submodules(void);
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!");
}
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);
}
push_ret = transport->push_refs(transport, remote_refs, flags);
--
2.0.2.832.g083c931
^ permalink raw reply related
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