* [PATCH v3 3/4] remote-curl: tighten "version 2" check for smart-http
From: Josh Steadmon @ 2018-12-12 0:25 UTC (permalink / raw)
To: git; +Cc: peff, gitster, masayasuzuki
In-Reply-To: <cover.1544572142.git.steadmon@google.com>
From: Jeff King <peff@peff.net>
In a v2 smart-http conversation, the server should reply to our initial
request with a pkt-line saying "version 2" (this is the start of the
"capabilities advertisement"). We check for the string using
starts_with(), but that's overly permissive (it would match "version
20", for example).
Let's tighten this check to use strcmp(). Note that we don't need to
worry about a trailing newline here, because the ptk-line code will have
chomped it for us already.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
remote-curl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/remote-curl.c b/remote-curl.c
index 38f51dffb8..b77b173f33 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -375,7 +375,7 @@ static void check_smart_http(struct discovery *d, const char *service,
d->len = src_len;
d->proto_git = 1;
- } else if (starts_with(line, "version 2")) {
+ } else if (!strcmp(line, "version 2")) {
/*
* v2 smart http; do not consume version packet, which will
* be handled elsewhere.
--
2.20.0.rc2.403.gdbc3b29805-goog
^ permalink raw reply related
* [PATCH v3 2/4] remote-curl: refactor smart-http discovery
From: Josh Steadmon @ 2018-12-12 0:25 UTC (permalink / raw)
To: git; +Cc: peff, gitster, masayasuzuki
In-Reply-To: <cover.1544572142.git.steadmon@google.com>
From: Jeff King <peff@peff.net>
After making initial contact with an http server, we have to decide if
the server supports smart-http, and if so, which version. Our rules are
a bit inconsistent:
1. For v0, we require that the content-type indicates a smart-http
response. We also require the response to look vaguely like a
pkt-line starting with "#". If one of those does not match, we fall
back to dumb-http.
But according to our http protocol spec[1]:
Dumb servers MUST NOT return a return type starting with
`application/x-git-`.
If we see the expected content-type, we should consider it
smart-http. At that point we can parse the pkt-line for real, and
complain if it is not syntactically valid.
2. For v2, we do not actually check the content-type. Our v2 protocol
spec says[2]:
When using the http:// or https:// transport a client makes a
"smart" info/refs request as described in `http-protocol.txt`[...]
and the http spec is clear that for a smart-http[3]:
The Content-Type MUST be `application/x-$servicename-advertisement`.
So it is required according to the spec.
These inconsistencies were easy to miss because of the way the original
code was written as an inline conditional. Let's pull it out into its
own function for readability, and improve a few things:
- we now predicate the smart/dumb decision entirely on the presence of
the correct content-type
- we do a real pkt-line parse before deciding how to proceed (and die
if it isn't valid)
- use skip_prefix() for comparing service strings, instead of
constructing expected output in a strbuf; this avoids dealing with
memory cleanup
Note that this _is_ tightening what the client will allow. It's all
according to the spec, but it's possible that other implementations
might violate these. However, violating these particular rules seems
like an odd choice for a server to make.
[1] Documentation/technical/http-protocol.txt, l. 166-167
[2] Documentation/technical/protocol-v2.txt, l. 63-64
[3] Documentation/technical/http-protocol.txt, l. 247
Helped-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
remote-curl.c | 93 ++++++++++++++++++++++++++++++++-------------------
1 file changed, 59 insertions(+), 34 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 1220dffcdc..38f51dffb8 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -330,9 +330,65 @@ static int get_protocol_http_header(enum protocol_version version,
return 0;
}
+static void check_smart_http(struct discovery *d, const char *service,
+ struct strbuf *type)
+{
+ char *src_buf;
+ size_t src_len;
+ char *line;
+ const char *p;
+
+ /*
+ * If we don't see x-$service-advertisement, then it's not smart-http.
+ * But once we do, we commit to it and assume any other protocol
+ * violations are hard errors.
+ */
+ if (!skip_prefix(type->buf, "application/x-", &p) ||
+ !skip_prefix(p, service, &p) ||
+ strcmp(p, "-advertisement"))
+ return;
+
+ /*
+ * "Peek" at the first packet by using a separate buf/len pair; some
+ * cases below require us leaving the originals intact.
+ */
+ src_buf = d->buf;
+ src_len = d->len;
+ line = packet_read_line_buf(&src_buf, &src_len, NULL);
+ if (!line)
+ die("invalid server response; expected service, got flush packet");
+
+ if (skip_prefix(line, "# service=", &p) && !strcmp(p, service)) {
+ /*
+ * The header can include additional metadata lines, up
+ * until a packet flush marker. Ignore these now, but
+ * in the future we might start to scan them.
+ */
+ while (packet_read_line_buf(&src_buf, &src_len, NULL))
+ ;
+
+ /*
+ * v0 smart http; callers expect us to soak up the
+ * service and header packets
+ */
+ d->buf = src_buf;
+ d->len = src_len;
+ d->proto_git = 1;
+
+ } else if (starts_with(line, "version 2")) {
+ /*
+ * v2 smart http; do not consume version packet, which will
+ * be handled elsewhere.
+ */
+ d->proto_git = 1;
+
+ } else {
+ die("invalid server response; got '%s'", line);
+ }
+}
+
static struct discovery *discover_refs(const char *service, int for_push)
{
- struct strbuf exp = STRBUF_INIT;
struct strbuf type = STRBUF_INIT;
struct strbuf charset = STRBUF_INIT;
struct strbuf buffer = STRBUF_INIT;
@@ -405,38 +461,8 @@ static struct discovery *discover_refs(const char *service, int for_push)
last->buf_alloc = strbuf_detach(&buffer, &last->len);
last->buf = last->buf_alloc;
- strbuf_addf(&exp, "application/x-%s-advertisement", service);
- if (maybe_smart &&
- (5 <= last->len && last->buf[4] == '#') &&
- !strbuf_cmp(&exp, &type)) {
- char *line;
-
- /*
- * smart HTTP response; validate that the service
- * pkt-line matches our request.
- */
- line = packet_read_line_buf(&last->buf, &last->len, NULL);
- if (!line)
- die("invalid server response; expected service, got flush packet");
-
- strbuf_reset(&exp);
- strbuf_addf(&exp, "# service=%s", service);
- if (strcmp(line, exp.buf))
- die("invalid server response; got '%s'", line);
- strbuf_release(&exp);
-
- /* The header can include additional metadata lines, up
- * until a packet flush marker. Ignore these now, but
- * in the future we might start to scan them.
- */
- while (packet_read_line_buf(&last->buf, &last->len, NULL))
- ;
-
- last->proto_git = 1;
- } else if (maybe_smart &&
- last->len > 5 && starts_with(last->buf + 4, "version 2")) {
- last->proto_git = 1;
- }
+ if (maybe_smart)
+ check_smart_http(last, service, &type);
if (last->proto_git)
last->refs = parse_git_refs(last, for_push);
@@ -444,7 +470,6 @@ static struct discovery *discover_refs(const char *service, int for_push)
last->refs = parse_info_refs(last);
strbuf_release(&refs_url);
- strbuf_release(&exp);
strbuf_release(&type);
strbuf_release(&charset);
strbuf_release(&effective_url);
--
2.20.0.rc2.403.gdbc3b29805-goog
^ permalink raw reply related
* [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context
From: Josh Steadmon @ 2018-12-12 0:25 UTC (permalink / raw)
To: git; +Cc: peff, gitster, masayasuzuki
In-Reply-To: <cover.1544572142.git.steadmon@google.com>
From: Masaya Suzuki <masayasuzuki@google.com>
In the Git pack protocol definition, an error packet may appear only in
a certain context. However, servers can face a runtime error (e.g. I/O
error) at an arbitrary timing. This patch changes the protocol to allow
an error packet to be sent instead of any packet.
Following this protocol spec change, the error packet handling code is
moved to pkt-line.c.
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Documentation/technical/pack-protocol.txt | 20 +++++++++++---------
builtin/archive.c | 2 --
connect.c | 3 ---
fetch-pack.c | 2 --
pkt-line.c | 4 ++++
t/t5703-upload-pack-ref-in-want.sh | 4 ++--
6 files changed, 17 insertions(+), 18 deletions(-)
diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index 6ac774d5f6..7a2375a55d 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -22,6 +22,16 @@ protocol-common.txt. When the grammar indicate `PKT-LINE(...)`, unless
otherwise noted the usual pkt-line LF rules apply: the sender SHOULD
include a LF, but the receiver MUST NOT complain if it is not present.
+An error packet is a special pkt-line that contains an error string.
+
+----
+ error-line = PKT-LINE("ERR" SP explanation-text)
+----
+
+Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY
+be sent. Once this packet is sent by a client or a server, the data transfer
+process defined in this protocol is terminated.
+
Transports
----------
There are three transports over which the packfile protocol is
@@ -89,13 +99,6 @@ process on the server side over the Git protocol is this:
"0039git-upload-pack /schacon/gitbook.git\0host=example.com\0" |
nc -v example.com 9418
-If the server refuses the request for some reasons, it could abort
-gracefully with an error message.
-
-----
- error-line = PKT-LINE("ERR" SP explanation-text)
-----
-
SSH Transport
-------------
@@ -398,12 +401,11 @@ from the client).
Then the server will start sending its packfile data.
----
- server-response = *ack_multi ack / nak / error-line
+ server-response = *ack_multi ack / nak
ack_multi = PKT-LINE("ACK" SP obj-id ack_status)
ack_status = "continue" / "common" / "ready"
ack = PKT-LINE("ACK" SP obj-id)
nak = PKT-LINE("NAK")
- error-line = PKT-LINE("ERR" SP explanation-text)
----
A simple clone may look like this (with no 'have' lines):
diff --git a/builtin/archive.c b/builtin/archive.c
index d2455237ce..5d179bbd16 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -59,8 +59,6 @@ static int run_remote_archiver(int argc, const char **argv,
if (strcmp(buf, "ACK")) {
if (starts_with(buf, "NACK "))
die(_("git archive: NACK %s"), buf + 5);
- if (starts_with(buf, "ERR "))
- die(_("remote error: %s"), buf + 4);
die(_("git archive: protocol error"));
}
diff --git a/connect.c b/connect.c
index 24281b6082..4813f005ab 100644
--- a/connect.c
+++ b/connect.c
@@ -296,7 +296,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,
struct ref **orig_list = list;
int len = 0;
enum get_remote_heads_state state = EXPECTING_FIRST_REF;
- const char *arg;
*list = NULL;
@@ -306,8 +305,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,
die_initial_contact(1);
case PACKET_READ_NORMAL:
len = reader->pktlen;
- if (len > 4 && skip_prefix(reader->line, "ERR ", &arg))
- die(_("remote error: %s"), arg);
break;
case PACKET_READ_FLUSH:
state = EXPECTING_DONE;
diff --git a/fetch-pack.c b/fetch-pack.c
index 9691046e64..e66cd7b71b 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -178,8 +178,6 @@ static enum ack_type get_ack(int fd, struct object_id *result_oid)
return ACK;
}
}
- if (skip_prefix(line, "ERR ", &arg))
- die(_("remote error: %s"), arg);
die(_("git fetch-pack: expected ACK/NAK, got '%s'"), line);
}
diff --git a/pkt-line.c b/pkt-line.c
index 04d10bbd03..ce9e42d10e 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
return PACKET_READ_EOF;
}
+ if (starts_with(buffer, "ERR ")) {
+ die(_("remote error: %s"), buffer + 4);
+ }
+
if ((options & PACKET_READ_CHOMP_NEWLINE) &&
len && buffer[len-1] == '\n')
len--;
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index 3f58f05cbb..d2a9d0c127 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -208,7 +208,7 @@ test_expect_success 'server is initially ahead - no ref in want' '
cp -r "$LOCAL_PRISTINE" local &&
inconsistency master 1234567890123456789012345678901234567890 &&
test_must_fail git -C local fetch 2>err &&
- grep "ERR upload-pack: not our ref" err
+ grep "fatal: remote error: upload-pack: not our ref" err
'
test_expect_success 'server is initially ahead - ref in want' '
@@ -254,7 +254,7 @@ test_expect_success 'server loses a ref - ref in want' '
echo "s/master/raster/" >"$HTTPD_ROOT_PATH/one-time-sed" &&
test_must_fail git -C local fetch 2>err &&
- grep "ERR unknown ref refs/heads/raster" err
+ grep "fatal: remote error: unknown ref refs/heads/raster" err
'
stop_httpd
--
2.20.0.rc2.403.gdbc3b29805-goog
^ permalink raw reply related
* [PATCH v3 0/4] Unify pkt-line error handling and refactor smart-http
From: Josh Steadmon @ 2018-12-12 0:25 UTC (permalink / raw)
To: git; +Cc: peff, gitster, masayasuzuki
In-Reply-To: <20181116084427.GA31493@sigill.intra.peff.net>
This is a reroll of js/smart-http-detect-remote-error that also includes
a fixed version of ms/proto-err-packet-anywhere [1].
The first patch clarifies the use of ERR messages in the pkt-line
protocol and unifies error handling in pkt-line.c
The second patch refactors smart-http discovery in remote-curl.c. Among
other improvements, it makes more use of the pkt-line functions, which
allows us to catch remote errors that were previously ignored.
The third patch makes the version check in remote-curl more strict.
The final patch adds a test to verify that the fix in patch #2 does
actually catch remote HTTP errors.
[1]: https://public-inbox.org/git/20181127045301.103807-1-masayasuzuki@google.com/
Jeff King (2):
remote-curl: refactor smart-http discovery
remote-curl: tighten "version 2" check for smart-http
Josh Steadmon (1):
lib-httpd, t5551: check server-side HTTP errors
Masaya Suzuki (1):
pack-protocol.txt: accept error packets in any context
Documentation/technical/pack-protocol.txt | 20 ++---
builtin/archive.c | 2 -
connect.c | 3 -
fetch-pack.c | 2 -
pkt-line.c | 4 +
remote-curl.c | 93 ++++++++++++++---------
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf | 4 +
t/lib-httpd/error-smart-http.sh | 3 +
t/t5551-http-fetch-smart.sh | 5 ++
t/t5703-upload-pack-ref-in-want.sh | 4 +-
11 files changed, 89 insertions(+), 52 deletions(-)
create mode 100644 t/lib-httpd/error-smart-http.sh
--
2.20.0.rc2.403.gdbc3b29805-goog
^ permalink raw reply
* [PATCH 5/5] notes: Implement xref-cherry-picks hooks and tests
From: Tejun Heo @ 2018-12-11 23:49 UTC (permalink / raw)
To: git, Junio C Hamano, Jeff King; +Cc: kernel-team, Tejun Heo
In-Reply-To: <20181211234909.2855638-1-tj@kernel.org>
From: Tejun Heo <htejun@fb.com>
Add post-cherry-pick.sample and post-fetch.sample which, when enabled,
will keep refs/notes/xref-cherry-picks up-to-date as new cherry-picks
are created and fetched. Also, add tests to verify xref-cherry-picks.
Signed-off-by: Tejun Heo <htejun@fb.com>
---
Documentation/git-reverse-trailer-xrefs.txt | 9 ++
t/t3321-notes-xref-cherry-picks.sh | 124 ++++++++++++++++++++
templates/hooks--post-cherry-pick.sample | 8 ++
templates/hooks--post-fetch.sample | 30 +++++
4 files changed, 171 insertions(+)
create mode 100644 t/t3321-notes-xref-cherry-picks.sh
create mode 100644 templates/hooks--post-cherry-pick.sample
create mode 100644 templates/hooks--post-fetch.sample
diff --git a/Documentation/git-reverse-trailer-xrefs.txt b/Documentation/git-reverse-trailer-xrefs.txt
index 20d260486..651ecdce1 100644
--- a/Documentation/git-reverse-trailer-xrefs.txt
+++ b/Documentation/git-reverse-trailer-xrefs.txt
@@ -131,6 +131,15 @@ ddd1bf2 commit A
------------
+Keeping xref-cherry-picks up-to-date
+------------------------------------
+
+Reverse-maps can be kept-up incrementally with hooks. For example, to
+keep xref-cherry-picks up-to-date, `git-reverse-trailer-xrefs` should
+be invoked on new cherry-picks and fetched commits. See
+`hooks/post-cherry-pick.sample` and `hooks/post-fetch.sample`.
+
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/t/t3321-notes-xref-cherry-picks.sh b/t/t3321-notes-xref-cherry-picks.sh
new file mode 100644
index 000000000..96b6731c9
--- /dev/null
+++ b/t/t3321-notes-xref-cherry-picks.sh
@@ -0,0 +1,124 @@
+#!/bin/sh
+
+test_description='Verify xref-cherry-picks handling
+
+Assume the following git repository.
+
+ D*---E** release-B
+ /
+ C* E* release-D
+ / /
+ A---B---C---D---E master
+
+which contains the following cherry-picks.
+
+ C -> C*
+ D -> D*
+ E -> E* -> E**
+
+1. Build the above repository using `git-cherry-pick -x` with the
+ sample post-cherry-pick hook enabled. Verify that the
+ xref-cherry-picks notes are populated correctly.
+
+2. Clear the notes and rebuild them by directly running
+ git-reverse-xref-trailers and verify the output.
+
+3. Run it again and check the output still agrees to verify duplicate
+ handling.
+
+4. Build a cloned repository using per-branch fetches with the sample
+ post-fetch hook enabled. Verify that the xref-cherry-picks notes
+ are populatec correctly.
+'
+
+TEST_NO_CREATE_REPO=1
+
+. ./test-lib.sh
+
+GIT_AUTHOR_EMAIL=bogus_email_address
+export GIT_AUTHOR_EMAIL
+
+test_expect_success \
+ 'Build repo with cherry-picks and verify xref-cherry-picks' \
+ 'test_create_repo main &&
+ cd main &&
+ mkdir -p .git/hooks &&
+ mv .git/hooks-disabled/post-cherry-pick.sample .git/hooks/post-cherry-pick &&
+
+ test_tick &&
+ echo A >> testfile &&
+ git update-index --add testfile &&
+ git commit -am "A" &&
+ echo B >> testfile &&
+ git commit -am "B" &&
+ echo C >> testfile &&
+ git commit -am "C" &&
+ echo D >> testfile &&
+ git commit -am "D" &&
+ echo E >> testfile &&
+ git commit -am "E" &&
+
+ test_tick &&
+ git checkout -b release-D master^ &&
+ git cherry-pick -x master &&
+
+ test_tick &&
+ git checkout -b release-B master^^^ &&
+ git cherry-pick -x release-D^^ &&
+ git cherry-pick -x release-D^ &&
+ git cherry-pick -x release-D &&
+
+ cat > expect <<-EOF &&
+master E
+Notes (xref-cherry-picks):
+ Cherry-picked-to: release-D
+ Cherry-picked-to: release-B
+
+master~1 D
+Notes (xref-cherry-picks):
+ Cherry-picked-to: release-B~1
+
+master~2 C
+Notes (xref-cherry-picks):
+ Cherry-picked-to: release-B~2
+
+master~3 B
+master~4 A
+EOF
+
+ git log --pretty=oneline --notes=xref-cherry-picks master | git name-rev --name-only --stdin > actual &&
+ test_cmp expect actual
+'
+
+test_expect_success \
+ 'Clear, rebuild and verify xref-cherry-picks' \
+ 'git reverse-trailer-xrefs --xref-cherry-picks --all --clear &&
+ git reverse-trailer-xrefs --xref-cherry-picks --all &&
+ git log --pretty=oneline --notes=xref-cherry-picks master | git name-rev --name-only --stdin > actual &&
+ test_cmp expect actual
+'
+
+test_expect_success \
+ 'Build it again to verify duplicate handling' \
+ 'git reverse-trailer-xrefs --xref-cherry-picks --all &&
+ git log --pretty=oneline --notes=xref-cherry-picks master | git name-rev --name-only --stdin > actual &&
+ test_cmp expect actual
+'
+
+test_expect_success \
+ 'Build a clone through per-branch fetches and verify xref-cherry-picks' \
+ 'cd .. &&
+ test_create_repo clone &&
+ cd clone &&
+ mkdir -p .git/hooks &&
+ mv .git/hooks-disabled/post-fetch.sample .git/hooks/post-fetch &&
+
+ git fetch -fu ../main master:master &&
+ git fetch -fu ../main release-D:release-D &&
+ git fetch -fu ../main release-B:release-B &&
+
+ git log --pretty=oneline --notes=xref-cherry-picks master | git name-rev --name-only --stdin > actual &&
+ test_cmp ../main/expect actual
+'
+
+test_done
diff --git a/templates/hooks--post-cherry-pick.sample b/templates/hooks--post-cherry-pick.sample
new file mode 100644
index 000000000..3af8b5d23
--- /dev/null
+++ b/templates/hooks--post-cherry-pick.sample
@@ -0,0 +1,8 @@
+#!/bin/sh
+#
+# An example hook script to reverse map new cherry-picks. See
+# git-reverse-trailer-xrefs(1) for details.
+#
+# To enable this hook, rename this file to "post-cherry-pick".
+
+git reverse-trailer-xrefs --xref-cherry-picks $1..$2
diff --git a/templates/hooks--post-fetch.sample b/templates/hooks--post-fetch.sample
new file mode 100644
index 000000000..6b98a5c10
--- /dev/null
+++ b/templates/hooks--post-fetch.sample
@@ -0,0 +1,30 @@
+#!/bin/sh
+#
+# An example hook script to reverse map cherry-picks in newly fetched
+# commits. See git-reverse-trailer-xrefs(1) for details.
+#
+# To enable this hook, rename this file to "post-fetch".
+
+z40=0000000000000000000000000000000000000000
+
+while read ref old_sha remote new_sha
+do
+ case $ref in
+ refs/heads/*)
+ ;;
+ *)
+ continue
+ esac
+
+ if [ $new_sha == $z40 ]
+ then
+ continue
+ fi
+
+ if [ $old_sha != $z40 ]
+ then
+ git reverse-trailer-xrefs --xref-cherry-picks $old_sha..$new_sha
+ else
+ git reverse-trailer-xrefs --xref-cherry-picks $new_sha
+ fi
+done
--
2.17.1
^ permalink raw reply related
* [PATCH 4/5] githooks: Add post-cherry-pick and post-fetch hooks
From: Tejun Heo @ 2018-12-11 23:49 UTC (permalink / raw)
To: git, Junio C Hamano, Jeff King; +Cc: kernel-team, Tejun Heo
In-Reply-To: <20181211234909.2855638-1-tj@kernel.org>
From: Tejun Heo <htejun@fb.com>
* post-cherry-pick: Called after a cherry-pick and given parameters so
that it can tell which are the new cherry-picks.
* post-fetch: Called after a fetch. Each updated ref and sha1 are fed
on stdin.
These two hooks will be used to keep refs/notes/xref-cherry-picks
up-to-date.
Signed-off-by: Tejun Heo <htejun@fb.com>
---
Documentation/git-cherry-pick.txt | 5 +++
Documentation/git-fetch.txt | 5 +++
Documentation/githooks.txt | 23 ++++++++++
builtin/fetch.c | 72 ++++++++++++++++++++++++++++---
builtin/revert.c | 14 ++++++
5 files changed, 114 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index d35d771fc..527cb9fea 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -224,6 +224,11 @@ the working tree.
spending extra time to avoid mistakes based on incorrectly matching
context lines.
+HOOKS
+-----
+This command can run `post-cherry-pick` hook. See linkgit:githooks[5]
+for more information.
+
SEE ALSO
--------
linkgit:git-revert[1]
diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index e31993559..a04c6079a 100644
--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -290,6 +290,11 @@ fetched, making it impossible to check out that submodule later without
having to do a fetch again. This is expected to be fixed in a future Git
version.
+HOOKS
+-----
+This command can run `post-fetch` hook. See linkgit:githooks[5]
+for more information.
+
SEE ALSO
--------
linkgit:git-pull[1]
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 959044347..24c122343 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -149,6 +149,14 @@ invoked after a commit is made.
This hook is meant primarily for notification, and cannot affect
the outcome of `git commit`.
+post-cherry-pick
+~~~~~~~~~~~~~~~~
+
+This hook is invoked by linkgit:git-cherry-pick[1]. This hook is
+called with two parameters. The first is `<old sha1>` and the second
+`<new sha1>`, where `<old sha1>..<new sha1>` describes all new
+cherry-picked commits.
+
pre-rebase
~~~~~~~~~~
@@ -191,6 +199,21 @@ save and restore any form of metadata associated with the working tree
(e.g.: permissions/ownership, ACLS, etc). See contrib/hooks/setgitperms.perl
for an example of how to do this.
+post-fetch
+~~~~~~~~~~
+This hook is called by linkgit:git-fetch[1] and can be used to process
+newly fetched commits and tags.
+
+Information about what was fetched is provided on the hook's standard
+input with lines of the form:
+
+ <local ref> SP <old sha1> SP <remote ref> SP <new sha1> LF
+
+where `<local ref>` got updated from `<old sha1>` to `<new sha1>` as a
+result of fetching `<remote ref>`. If a branch or tag was created,
+`<old_sha1>` will be 40 `0`. If a tag was pruned, `<remote_ref>` will
+be `(delete)` and <new sha1> will be 40 `0`.
+
pre-push
~~~~~~~~
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e0140327a..eac792a33 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -66,6 +66,7 @@ static struct refspec refmap = REFSPEC_INIT_FETCH;
static struct list_objects_filter_options filter_options;
static struct string_list server_options = STRING_LIST_INIT_DUP;
static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
+static struct strbuf post_fetch_sb = STRBUF_INIT;
static int git_fetch_config(const char *k, const char *v, void *cb)
{
@@ -510,10 +511,24 @@ static struct ref *get_ref_map(struct remote *remote,
return ref_map;
}
+static void record_post_fetch(const char *name,
+ const struct object_id *old_oid,
+ const char *remote,
+ const struct object_id *new_oid)
+{
+ char old_hex[GIT_MAX_HEXSZ + 1], new_hex[GIT_MAX_HEXSZ + 1];
+
+ oid_to_hex_r(old_hex, old_oid);
+ oid_to_hex_r(new_hex, new_oid);
+ strbuf_addf(&post_fetch_sb, "%s %s %s %s\n",
+ name, old_hex, remote ?: "(delete)", new_hex);
+}
+
#define STORE_REF_ERROR_OTHER 1
#define STORE_REF_ERROR_DF_CONFLICT 2
static int s_update_ref(const char *action,
+ const char *remote,
struct ref *ref,
int check_old)
{
@@ -546,6 +561,7 @@ static int s_update_ref(const char *action,
ref_transaction_free(transaction);
strbuf_release(&err);
free(msg);
+ record_post_fetch(ref->name, &ref->old_oid, remote, &ref->new_oid);
return 0;
fail:
ref_transaction_free(transaction);
@@ -726,7 +742,7 @@ static int update_local_ref(struct ref *ref,
starts_with(ref->name, "refs/tags/")) {
if (force || ref->force) {
int r;
- r = s_update_ref("updating tag", ref, 0);
+ r = s_update_ref("updating tag", remote, ref, 0);
format_display(display, r ? '!' : 't', _("[tag update]"),
r ? _("unable to update local ref") : NULL,
remote, pretty_ref, summary_width);
@@ -766,7 +782,7 @@ static int update_local_ref(struct ref *ref,
if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
(recurse_submodules != RECURSE_SUBMODULES_ON))
check_for_new_submodule_commits(&ref->new_oid);
- r = s_update_ref(msg, ref, 0);
+ r = s_update_ref(msg, remote, ref, 0);
format_display(display, r ? '!' : '*', what,
r ? _("unable to update local ref") : NULL,
remote, pretty_ref, summary_width);
@@ -782,7 +798,7 @@ static int update_local_ref(struct ref *ref,
if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
(recurse_submodules != RECURSE_SUBMODULES_ON))
check_for_new_submodule_commits(&ref->new_oid);
- r = s_update_ref("fast-forward", ref, 1);
+ r = s_update_ref("fast-forward", remote, ref, 1);
format_display(display, r ? '!' : ' ', quickref.buf,
r ? _("unable to update local ref") : NULL,
remote, pretty_ref, summary_width);
@@ -797,7 +813,7 @@ static int update_local_ref(struct ref *ref,
if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
(recurse_submodules != RECURSE_SUBMODULES_ON))
check_for_new_submodule_commits(&ref->new_oid);
- r = s_update_ref("forced-update", ref, 1);
+ r = s_update_ref("forced-update", remote, ref, 1);
format_display(display, r ? '!' : '+', quickref.buf,
r ? _("unable to update local ref") : _("forced update"),
remote, pretty_ref, summary_width);
@@ -1071,8 +1087,11 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
if (!dry_run) {
struct string_list refnames = STRING_LIST_INIT_NODUP;
- for (ref = stale_refs; ref; ref = ref->next)
+ for (ref = stale_refs; ref; ref = ref->next) {
string_list_append(&refnames, ref->name);
+ record_post_fetch(ref->name, &ref->old_oid,
+ NULL, &null_oid);
+ }
result = delete_refs("fetch: prune", &refnames, 0);
string_list_clear(&refnames, 0);
@@ -1561,6 +1580,47 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, int pru
return exit_code;
}
+static int run_post_fetch_hook(void)
+{
+ int ret = 0, x;
+ struct child_process proc = CHILD_PROCESS_INIT;
+ const char *argv[2];
+
+ if (!(argv[0] = find_hook("post-fetch")))
+ return 0;
+ argv[1] = NULL;
+
+ proc.argv = argv;
+ proc.in = -1;
+
+ if (start_command(&proc)) {
+ finish_command(&proc);
+ return -1;
+ }
+
+ sigchain_push(SIGPIPE, SIG_IGN);
+
+ if (write_in_full(proc.in, post_fetch_sb.buf, post_fetch_sb.len) < 0) {
+ /* We do not mind if a hook does not read all refs. */
+ if (errno != EPIPE)
+ ret = -1;
+ }
+
+ strbuf_release(&post_fetch_sb);
+
+ x = close(proc.in);
+ if (!ret)
+ ret = x;
+
+ sigchain_pop(SIGPIPE);
+
+ x = finish_command(&proc);
+ if (!ret)
+ ret = x;
+
+ return ret;
+}
+
int cmd_fetch(int argc, const char **argv, const char *prefix)
{
int i;
@@ -1669,6 +1729,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
close_all_packs(the_repository->objects);
+ run_post_fetch_hook();
+
argv_array_pushl(&argv_gc_auto, "gc", "--auto", NULL);
if (verbosity < 0)
argv_array_push(&argv_gc_auto, "--quiet");
diff --git a/builtin/revert.c b/builtin/revert.c
index c93393c89..0b7e578cc 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -8,6 +8,8 @@
#include "dir.h"
#include "sequencer.h"
#include "branch.h"
+#include "refs.h"
+#include "run-command.h"
/*
* This implements the builtins revert and cherry-pick.
@@ -223,12 +225,24 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
+ struct object_id old_oid, new_oid;
+ char old_hex[GIT_MAX_HEXSZ + 1], new_hex[GIT_MAX_HEXSZ + 1];
int res;
+ if (read_ref("HEAD", &old_oid))
+ die(_("failed to read HEAD, cherry-pick failed"));
+
opts.action = REPLAY_PICK;
sequencer_init_config(&opts);
res = run_sequencer(argc, argv, &opts);
if (res < 0)
die(_("cherry-pick failed"));
+
+ if (read_ref("HEAD", &new_oid))
+ die(_("failed to read HEAD after cherry-pick"));
+
+ oid_to_hex_r(old_hex, &old_oid);
+ oid_to_hex_r(new_hex, &new_oid);
+ run_hook_le(0, "post-cherry-pick", old_hex, new_hex, NULL);
return res;
}
--
2.17.1
^ permalink raw reply related
* [PATCH 3/5] notes: Implement git-reverse-trailer-xrefs
From: Tejun Heo @ 2018-12-11 23:49 UTC (permalink / raw)
To: git, Junio C Hamano, Jeff King; +Cc: kernel-team, Tejun Heo
In-Reply-To: <20181211234909.2855638-1-tj@kernel.org>
From: Tejun Heo <htejun@fb.com>
Some trailers refer to other commits. Let's call them xrefs
(cross-references). For example, a cherry pick trailer points to the
source commit. It is sometimes useful to build a reverse map of these
xrefs - ie. source -> cherry-pick instead of cherry-pick -> source.
This patch implements git-reverse-trailer-xrefs which can process a
given trailer for the specified commits and generate and update the
matching refs/notes/xref- notes.
The command reverse-maps trailers beginning with --trailer-prefix and
stores them in --ref notes with each line tagged with --tag. It also
has a refs/notes/xref-cherry-picks preset for the three params.
Signed-off-by: Tejun Heo <htejun@fb.com>
---
Documentation/git-reverse-trailer-xrefs.txt | 136 +++++++++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/reverse-trailer-xrefs.c | 160 ++++++++++++++++++++
git.c | 1 +
5 files changed, 299 insertions(+)
create mode 100644 Documentation/git-reverse-trailer-xrefs.txt
create mode 100644 builtin/reverse-trailer-xrefs.c
diff --git a/Documentation/git-reverse-trailer-xrefs.txt b/Documentation/git-reverse-trailer-xrefs.txt
new file mode 100644
index 000000000..20d260486
--- /dev/null
+++ b/Documentation/git-reverse-trailer-xrefs.txt
@@ -0,0 +1,136 @@
+git-reverse-trailer-xrefs(1)
+============================
+
+NAME
+----
+git-reverse-trailer-xrefs - Record reverse-map of trailer commit references into notes
+
+SYNOPSIS
+--------
+[verse]
+`git reverse_trailer_xrefs` --xref-cherry-picks [--clear] [<options>] [<commit-ish>...]
+`git reverse_trailer_xrefs` --trailer-prefix=<prefix> --ref=<notes-ref> [--tag=<tag>] [<options>] [<commit-ish>...]
+`git reverse_trailer_xrefs` --ref=<notes-ref> --clear [<options>] [<commit-ish>...]
+
+
+DESCRIPTION
+-----------
+Record or clear reverse-map of trailer commit references in the
+specified notes ref.
+
+Some commit trailers reference other commits. For example,
+`git-cherry-pick -x` adds the following trailer to record the source
+commit.
+----------
+(cherry picked from commit <source-commit-id>)
+----------
+The reverse mappings of such cross references can be useful. For
+cherry-picks, it would allow finding all the cherry-picked commits of
+a given source commit. `git-reverse-trailer-xrefs` can be used to
+create and maintain such reverse mappings in notes.
+
+When used with `--xref-cherry-picks`, the cherry-pick trailers are
+parsed from the specified commits and the reverse mappings are
+recorded in the `refs/notes/xref-cherry-picks` notes of the source
+commits in the following format.
+----------
+Cherry-picked-to: <destination-commit-id>
+----------
+
+When a note with its notes ref starting with `refs/notes/xref-` is
+formatted to be displayed with the commit for, e.g., `git-show` or
+`git-log`, the destination commit is followed recursively and the
+matching notes are shown with increasing level of indentations.
+
+`--trailer-prefix`, `--notes` and `--tag` can be used to use a custom
+set of trailer, notes ref and reverse mapping tag.
+
+OPTIONS
+-------
+<commit-ish>...::
+ Commit-ish object names to describe. Defaults to HEAD if omitted.
+
+--xref-cherry-picks::
+ Use the preset to reverse map `git-cherry-pick -x`
+ trailers. `--trailer-prefix` is set to `(cherry-picked from
+ commit `, `--notes` is set to `refs/notes/xref-cherry-picks`
+ and `--tag` is set to `Cherry-picked-to`. This option can't be
+ specified with the three preset options.
+
+--trailer-prefix=<prefix>::
+ Process trailers which start with <prefix>. It is matched
+ character-by-character and should be followed by the
+ referenced commit ID. When there are multiple matching
+ trailers, the last one is used.
+
+--notes=<notes-ref>::
+ The notes ref to use for the reverse mapping. While this can
+ be any notes ref, it is recommented to use ones starting with
+ `refs/notes/xref-` as they are recognized as cross-referencing
+ notes and handled specially when updating and showing.
+
+--tag=<tag>::
+ Optional tag to use when generating reverse reference
+ notes. If specified, each note line is formatted as `<tag>:
+ <commit-id>`; otherwise, `<commit-id>`.
+
+--clear::
+ Instead of creating reverse mapping notes, clear them from the
+ specified commits.
+
+
+EXAMPLES
+--------
+
+Assume the following history. Development is happening in "master" and
+releases are branched off and fixes are cherry-picked into them.
+
+------------
+ D'---E'' release-B
+ /
+ C' E' release-D
+ / /
+A---B---C---D---E master
+------------
+
+The following cherry-picks took place.
+
+------------
+C -> C'
+D -> D'
+E -> E' -> E''
+------------
+
+The reverse mappings for all commits can be created using the
+following command.
+
+------------
+$ git reverse-trailer-xrefs --all --xref-cherry-picks
+------------
+
+With the notes added, where each commit ended up can be easily
+determined.
+
+------------
+$ git log --notes=xref-cherry-picks --oneline | git name-rev --name-only --stdin
+4b165af commit E
+Notes (xref-cherry-picks):
+ Cherry-picked-to: release-D
+ Cherry-picked-to: release-B
+
+82bf1f3 commit D
+Notes (xref-cherry-picks):
+ Cherry-picked-to: release-B~1
+
+364eccf commit C
+Notes (xref-cherry-picks):
+ Cherry-picked-to: release-B~2
+
+ed3adf3 commit B
+ddd1bf2 commit A
+------------
+
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 1a44c811a..3c23ecf9d 100644
--- a/Makefile
+++ b/Makefile
@@ -1086,6 +1086,7 @@ BUILTIN_OBJS += builtin/multi-pack-index.o
BUILTIN_OBJS += builtin/mv.o
BUILTIN_OBJS += builtin/name-rev.o
BUILTIN_OBJS += builtin/notes.o
+BUILTIN_OBJS += builtin/reverse-trailer-xrefs.o
BUILTIN_OBJS += builtin/pack-objects.o
BUILTIN_OBJS += builtin/pack-redundant.o
BUILTIN_OBJS += builtin/pack-refs.o
diff --git a/builtin.h b/builtin.h
index 6538932e9..51089e258 100644
--- a/builtin.h
+++ b/builtin.h
@@ -195,6 +195,7 @@ extern int cmd_multi_pack_index(int argc, const char **argv, const char *prefix)
extern int cmd_mv(int argc, const char **argv, const char *prefix);
extern int cmd_name_rev(int argc, const char **argv, const char *prefix);
extern int cmd_notes(int argc, const char **argv, const char *prefix);
+extern int cmd_reverse_trailer_xrefs(int argc, const char **argv, const char *prefix);
extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);
extern int cmd_pack_redundant(int argc, const char **argv, const char *prefix);
extern int cmd_patch_id(int argc, const char **argv, const char *prefix);
diff --git a/builtin/reverse-trailer-xrefs.c b/builtin/reverse-trailer-xrefs.c
new file mode 100644
index 000000000..0edb3b91a
--- /dev/null
+++ b/builtin/reverse-trailer-xrefs.c
@@ -0,0 +1,160 @@
+#include "builtin.h"
+#include "cache.h"
+#include "strbuf.h"
+#include "repository.h"
+#include "config.h"
+#include "commit.h"
+#include "blob.h"
+#include "notes.h"
+#include "notes-utils.h"
+#include "trailer.h"
+#include "revision.h"
+#include "list-objects.h"
+#include "object-store.h"
+#include "parse-options.h"
+
+static const char * const reverse_trailer_xrefs_usage[] = {
+ N_("git reverse_trailer_xrefs --xref-cherry-picks [--clear] [<options>] [<commit-ish>...]"),
+ N_("git reverse_trailer_xrefs --trailer-prefix=<prefix> --notes=<notes-ref> --tag=<tag> [<options>] [<commit-ish>...]"),
+ N_("git reverse_trailer_xrefs --notes=<notes-ref> --clear [<options>] [<commit-ish>...]"),
+ NULL
+};
+
+#define CHERRY_PICKED_PREFIX "(cherry picked from commit "
+#define CHERRY_PICKS_REF "refs/notes/xref-cherry-picks"
+#define CHERRY_PICKED_TO_TAG "Cherry-picked-to"
+
+static int verbose;
+
+static void clear_trailer_xref_note(struct commit *commit, void *data)
+{
+ struct notes_tree *tree = data;
+ int status;
+
+ status = remove_note(tree, commit->object.oid.hash);
+
+ if (verbose) {
+ if (status)
+ fprintf(stderr, "Object %s has no note\n",
+ oid_to_hex(&commit->object.oid));
+ else
+ fprintf(stderr, "Removing note for object %s\n",
+ oid_to_hex(&commit->object.oid));
+ }
+}
+
+static void record_trailer_xrefs(struct commit *commit, void *data)
+{
+ trailer_rev_xrefs_record(data, commit);
+}
+
+static int note_trailer_xrefs(struct notes_tree *tree,
+ struct commit *dst_commit,
+ struct object_array *src_objs, const char *tag)
+{
+ char dst_hex[GIT_MAX_HEXSZ + 1];
+ struct strbuf note = STRBUF_INIT;
+ struct object_id note_oid;
+ int i, ret;
+
+ oid_to_hex_r(dst_hex, &dst_commit->object.oid);
+
+ for (i = 0; i < src_objs->nr; i++) {
+ const char *hex = src_objs->objects[i].name;
+
+ if (tag)
+ strbuf_addf(¬e, "%s: %s\n", tag, hex);
+ else
+ strbuf_addf(¬e, "%s\n", tag);
+ if (verbose)
+ fprintf(stderr, "Adding note %s -> %s\n", dst_hex, hex);
+ }
+
+ ret = write_object_file(note.buf, note.len, blob_type, ¬e_oid);
+ strbuf_release(¬e);
+ if (ret)
+ return ret;
+
+ ret = add_note(tree, &dst_commit->object.oid, ¬e_oid, NULL);
+ return ret;
+}
+
+static int reverse_trailer_xrefs(struct rev_info *revs, int clear,
+ const char *trailer_prefix,
+ const char *notes_ref, const char *tag)
+{
+ struct notes_tree tree = { };
+ int i, ret;
+
+ init_notes(&tree, notes_ref, NULL, NOTES_INIT_WRITABLE);
+
+ if (prepare_revision_walk(revs))
+ die("revision walk setup failed");
+
+ if (clear) {
+ traverse_commit_list(revs, clear_trailer_xref_note, NULL, &tree);
+ } else {
+ struct trailer_rev_xrefs rxrefs;
+ struct commit *dst_commit;
+ struct object_array *src_objs;
+
+ trailer_rev_xrefs_init(&rxrefs, trailer_prefix);
+ traverse_commit_list(revs, record_trailer_xrefs, NULL, &rxrefs);
+
+ trailer_rev_xrefs_for_each(&rxrefs, i, dst_commit, src_objs) {
+ ret = note_trailer_xrefs(&tree, dst_commit, src_objs,
+ tag);
+ if (ret)
+ return ret;
+ }
+
+ trailer_rev_xrefs_release(&rxrefs);
+ }
+
+ commit_notes(&tree, "Notes updated by 'git reverse-trailer-xrefs'");
+ return 0;
+}
+
+int cmd_reverse_trailer_xrefs(int argc, const char **argv, const char *prefix)
+{
+ struct rev_info revs;
+ struct setup_revision_opt s_r_opt = {
+ .def = "HEAD",
+ .revarg_opt = REVARG_CANNOT_BE_FILENAME
+ };
+ int cherry = 0, clear = 0;
+ const char *trailer_prefix = NULL, *notes_ref = NULL, *tag = NULL;
+ struct option options[] = {
+ OPT_BOOL(0, "xref-cherry-picks", &cherry, N_("use preset for xref-cherry-picks notes")),
+ OPT_STRING(0, "trailer-prefix", &trailer_prefix, N_("prefix"), N_("process trailers starting with <prefix>")),
+ OPT_STRING(0, "notes", ¬es_ref, N_("notes-ref"), N_("update notes in <notes-ref>")),
+ OPT_STRING(0, "tag", &tag, N_("tag"), N_("tag xref notes with <tag>")),
+ OPT_BOOL(0, "clear", &clear, N_("clear trailer xref notes from the specified commits")),
+ OPT__VERBOSE(&verbose, N_("verbose")),
+ OPT_END()
+ };
+
+ git_config(git_default_config, NULL);
+
+ argc = parse_options(argc, argv, prefix, options,
+ reverse_trailer_xrefs_usage,
+ PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN);
+
+ init_revisions(&revs, prefix);
+ argc = setup_revisions(argc, argv, &revs, &s_r_opt);
+
+ if (cherry) {
+ trailer_prefix = CHERRY_PICKED_PREFIX;
+ notes_ref = CHERRY_PICKS_REF;
+ tag = CHERRY_PICKED_TO_TAG;
+ }
+
+ if (!notes_ref || (!clear && (!trailer_prefix || !tag)))
+ die(_("insufficient arguments"));
+
+ if (argc > 1)
+ die(_("unrecognized argument: %s"), argv[1]);
+
+ return reverse_trailer_xrefs(&revs, clear,
+ trailer_prefix, notes_ref, tag);
+}
diff --git a/git.c b/git.c
index 2f604a41e..4948c8e01 100644
--- a/git.c
+++ b/git.c
@@ -515,6 +515,7 @@ static struct cmd_struct commands[] = {
{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
{ "name-rev", cmd_name_rev, RUN_SETUP },
{ "notes", cmd_notes, RUN_SETUP },
+ { "reverse-trailer-xrefs", cmd_reverse_trailer_xrefs, RUN_SETUP },
{ "pack-objects", cmd_pack_objects, RUN_SETUP },
{ "pack-redundant", cmd_pack_redundant, RUN_SETUP | NO_PARSEOPT },
{ "pack-refs", cmd_pack_refs, RUN_SETUP },
--
2.17.1
^ permalink raw reply related
* [PATCH 2/5] notes: Implement special handlings for refs/notes/xref-
From: Tejun Heo @ 2018-12-11 23:49 UTC (permalink / raw)
To: git, Junio C Hamano, Jeff King; +Cc: kernel-team, Tejun Heo
In-Reply-To: <20181211234909.2855638-1-tj@kernel.org>
From: Tejun Heo <htejun@fb.com>
Some trailers refer to other commits. Let's call them xrefs
(cross-references). For example, a cherry pick trailer points to the
source commit. It is sometimes useful to build a reverse map of these
xrefs - ie. source -> cherry-pick instead of cherry-pick -> source.
These reverse-maps will be recorded in special notes whose refs start
with refs/notes/xref-. This patch implements the following special
handling for the xref notes.
* When xref notes are appended to an existing one, both parts get
parsed and dead or dupliate references are dropped, so that the
merged note contains only valid and unique xrefs.
* When xref notes are formatted for printing, the formatter recurses
into each xref and prints the nested xrefs with increasing
indentation to show the comprehensive xref chains.
The latter part will be documented by a future patch with the actual
use case.
Signed-off-by: Tejun Heo <htejun@fb.com>
---
Documentation/git-notes.txt | 8 ++
notes-merge.c | 9 ++
notes-utils.c | 2 +
notes-utils.h | 3 +-
notes.c | 260 +++++++++++++++++++++++++++++++++++-
notes.h | 9 ++
6 files changed, 288 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index df2b64dbb..872919ad4 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -88,6 +88,10 @@ the command can read the input given to the `post-rewrite` hook.)
Append to the notes of an existing object (defaults to HEAD).
Creates a new notes object if needed.
+ If the notes ref starts with `/refs/notes/xref-`, each
+ line is expected to contain `<tag>: <commit-id>` and
+ only lines with reachable unique commit IDs are kept.
+
edit::
Edit the notes for a given object (defaults to HEAD).
@@ -273,6 +277,10 @@ Note that if either the local or remote version contain duplicate lines
prior to the merge, these will also be removed by this notes merge
strategy.
+"cat_xrefs" is similar to "union" but expects each line to contain
+`<tag>: <commit-id>`. Only lines with reachable unique commit IDs are
+kept.
+
EXAMPLES
--------
diff --git a/notes-merge.c b/notes-merge.c
index bd05d50b0..3fe2389a1 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -464,6 +464,15 @@ static int merge_one_change(struct notes_merge_options *o,
die("failed to concatenate notes "
"(combine_notes_cat_sort_uniq)");
return 0;
+ case NOTES_MERGE_RESOLVE_CAT_XREFS:
+ if (o->verbosity >= 2)
+ printf("Concatenating unique and valid cross-references "
+ "in local and remote notes for %s\n",
+ oid_to_hex(&p->obj));
+ if (add_note(t, &p->obj, &p->remote, combine_notes_cat_xrefs))
+ die("failed to concatenate notes "
+ "(combine_notes_cat_xrefs)");
+ return 0;
}
die("Unknown strategy (%i).", o->strategy);
}
diff --git a/notes-utils.c b/notes-utils.c
index 14ea03178..db6363c39 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -70,6 +70,8 @@ int parse_notes_merge_strategy(const char *v, enum notes_merge_strategy *s)
*s = NOTES_MERGE_RESOLVE_UNION;
else if (!strcmp(v, "cat_sort_uniq"))
*s = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
+ else if (!strcmp(v, "cat_xrefs"))
+ *s = NOTES_MERGE_RESOLVE_CAT_XREFS;
else
return -1;
diff --git a/notes-utils.h b/notes-utils.h
index 540830652..b604f9b51 100644
--- a/notes-utils.h
+++ b/notes-utils.h
@@ -28,7 +28,8 @@ enum notes_merge_strategy {
NOTES_MERGE_RESOLVE_OURS,
NOTES_MERGE_RESOLVE_THEIRS,
NOTES_MERGE_RESOLVE_UNION,
- NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ
+ NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ,
+ NOTES_MERGE_RESOLVE_CAT_XREFS,
};
struct notes_rewrite_cfg {
diff --git a/notes.c b/notes.c
index 25cdce28b..835466787 100644
--- a/notes.c
+++ b/notes.c
@@ -9,6 +9,7 @@
#include "tree-walk.h"
#include "string-list.h"
#include "refs.h"
+#include "hashmap.h"
/*
* Use a non-balancing simple 16-tree structure with struct int_node as
@@ -996,8 +997,12 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
if (!notes_ref)
notes_ref = default_notes_ref();
- if (!combine_notes)
- combine_notes = combine_notes_concatenate;
+ if (!combine_notes) {
+ if (starts_with(notes_ref, "refs/notes/xref-"))
+ combine_notes = combine_notes_cat_xrefs;
+ else
+ combine_notes = combine_notes_concatenate;
+ }
t->root = (struct int_node *) xcalloc(1, sizeof(struct int_node));
t->first_non_note = NULL;
@@ -1189,6 +1194,76 @@ void free_notes(struct notes_tree *t)
memset(t, 0, sizeof(struct notes_tree));
}
+/*
+ * Parse a "[TAG:]HEX" line. @line is trimmed. If @tag_p is not NULL and
+ * TAG exists, the string is split. Returns the pointer to OID and updates
+ * *@tag_p to point to TAG.
+ */
+static char *parse_xref(char *line, char **tag_p)
+{
+ char *p, *hex;
+
+ /* ltrim */
+ while (isspace(*line))
+ line++;
+
+ p = strchr(line, ':');
+ if (p) {
+ if (tag_p) {
+ /* split and store TAG */
+ *tag_p = line;
+ *p = '\0';
+ }
+ /* trim whitespaces after ':' */
+ p++;
+ while (isspace(*p))
+ p++;
+ hex = p;
+ } else {
+ if (tag_p)
+ *tag_p = NULL;
+ hex = line;
+ }
+
+ /* rtrim */
+ p = hex;
+ while (*p != '\0' && !isspace(*p))
+ p++;
+ *p = '\0';
+ return hex;
+}
+
+static void walk_xrefs(const char *tree_ref, struct object_id *from_oid,
+ struct strbuf *sb, int level)
+{
+ struct object_array xrefs = OBJECT_ARRAY_INIT;
+ struct string_list lines = STRING_LIST_INIT_DUP;
+ int i;
+
+ read_xref_note(tree_ref, from_oid, &xrefs, &lines);
+
+ for (i = 0; i < xrefs.nr; i++) {
+ char *line = lines.items[i].string;
+ char *tag;
+
+ parse_xref(line, &tag);
+ strbuf_addf(sb, " %s%s%*s%s\n",
+ tag ?: "", tag ? ": " : "", 2 * level, "",
+ xrefs.objects[i].name);
+ if (xrefs.objects[i].item) {
+ if (level < 32)
+ walk_xrefs(tree_ref, &xrefs.objects[i].item->oid,
+ sb, level + 1);
+ else
+ warning("xref nested deeper than %d levels, terminating walk",
+ level);
+ }
+ }
+
+ object_array_clear(&xrefs);
+ string_list_clear(&lines, 0);
+}
+
/*
* Fill the given strbuf with the notes associated with the given object.
*
@@ -1208,6 +1283,7 @@ static void format_note(struct notes_tree *t, const struct object_id *object_oid
char *msg, *msg_p;
unsigned long linelen, msglen;
enum object_type type;
+ int format_xrefs;
if (!t)
t = &default_notes_tree;
@@ -1250,6 +1326,8 @@ static void format_note(struct notes_tree *t, const struct object_id *object_oid
}
}
+ format_xrefs = !raw && starts_with(t->ref, "refs/notes/xref-");
+
for (msg_p = msg; msg_p < msg + msglen; msg_p += linelen + 1) {
linelen = strchrnul(msg_p, '\n') - msg_p;
@@ -1257,6 +1335,14 @@ static void format_note(struct notes_tree *t, const struct object_id *object_oid
strbuf_addstr(sb, " ");
strbuf_add(sb, msg_p, linelen);
strbuf_addch(sb, '\n');
+
+ if (format_xrefs) {
+ struct object_id oid;
+
+ msg_p[linelen] = '\0';
+ if (!get_oid_hex(parse_xref(msg_p, NULL), &oid))
+ walk_xrefs(t->ref, &oid, sb, 1);
+ }
}
free(msg);
@@ -1309,3 +1395,173 @@ void expand_loose_notes_ref(struct strbuf *sb)
expand_notes_ref(sb);
}
}
+
+/*
+ * Parse a cross-referencing note.
+ *
+ * @note contains lines of "[TAG:]HEX" pointing to other commits. Read the
+ * target commits and add the objects to @result. If @result_lines is not
+ * NULL, it should point to a strdup'ing string_list. The verbatim note
+ * lines matching the target commits are appened to the list.
+ */
+static void parse_xref_note(const char *note, unsigned long size,
+ const struct object_id *commit_oid,
+ struct object_array *result,
+ struct string_list *result_lines)
+{
+ struct strbuf **lines, **pline;
+
+ lines = strbuf_split_buf(note, size, '\n', 0);
+
+ for (pline = lines; *pline; pline++) {
+ struct strbuf *line = *pline;
+ const char *target_hex;
+ struct object_id target_oid;
+ struct object *target_obj;
+
+ target_hex = parse_xref(line->buf, NULL);
+ if (get_oid_hex(target_hex, &target_oid)) {
+ if (commit_oid)
+ warning("read invalid sha1 on %s: %s",
+ oid_to_hex(commit_oid), line->buf);
+ continue;
+ }
+
+ target_obj = parse_object(the_repository, &target_oid);
+ if (!target_obj || target_obj->type != OBJ_COMMIT) {
+ if (commit_oid)
+ warning("read invalid commit on %s: %s",
+ oid_to_hex(commit_oid), line->buf);
+ continue;
+ }
+
+ add_object_array(target_obj, target_hex, result);
+ if (result_lines) {
+ assert(result_lines->strdup_strings);
+ string_list_append(result_lines, line->buf);
+ }
+ }
+
+ strbuf_list_free(lines);
+}
+
+struct notes_tree_entry {
+ struct hashmap_entry ent;
+ struct notes_tree tree;
+};
+
+static int notes_tree_cmp(const void *hashmap_cmp_fn_data,
+ const void *entry, const void *entry_or_key,
+ const void *keydata)
+{
+ const struct notes_tree_entry *e1 = entry;
+ const struct notes_tree_entry *e2 = entry_or_key;
+
+ return strcmp(e1->tree.ref, e2->tree.ref);
+}
+
+/*
+ * Read and parse a cross-referencing note.
+ *
+ * Read the @notes_ref note of @commit_oid and parse it with
+ * parse_xref_note().
+ */
+void read_xref_note(const char *notes_ref, const struct object_id *commit_oid,
+ struct object_array *result,
+ struct string_list *result_lines)
+{
+ static struct hashmap *notes_tree_map = NULL;
+ unsigned hash = memhash(notes_ref, strlen(notes_ref));
+ struct notes_tree_entry key, *ent;
+ const struct object_id *note_oid;
+ unsigned long size;
+ enum object_type type;
+ char *note;
+
+ if (!notes_tree_map) {
+ notes_tree_map = xcalloc(1, sizeof(struct hashmap));
+ hashmap_init(notes_tree_map, notes_tree_cmp, NULL, 0);
+ }
+
+ hashmap_entry_init(&key.ent, hash);
+ key.tree.ref = (char *)notes_ref;
+ ent = hashmap_get(notes_tree_map, &key, NULL);
+ if (!ent) {
+ ent = xcalloc(1, sizeof(struct notes_tree_entry));
+ init_notes(&ent->tree, notes_ref, NULL, 0);
+ hashmap_entry_init(&ent->ent, hash);
+ hashmap_put(notes_tree_map, ent);
+ }
+
+ note_oid = get_note(&ent->tree, commit_oid);
+ if (!note_oid)
+ return;
+
+ note = read_object_file(note_oid, &type, &size);
+ if (!size) {
+ free(note);
+ return;
+ }
+
+ parse_xref_note(note, size, commit_oid, result, result_lines);
+ free(note);
+}
+
+/*
+ * Combine a xref note in @new_oid into @cur_oid. Unreachable or duplicate
+ * xrefs are dropped. This is the default combine_notes callback for
+ * refs/notes/xref-.
+ */
+int combine_notes_cat_xrefs(struct object_id *cur_oid,
+ const struct object_id *new_oid)
+{
+ char *cur_msg = NULL, *new_msg = NULL;
+ unsigned long cur_len, new_len;
+ enum object_type cur_type, new_type;
+ struct object_array xrefs = OBJECT_ARRAY_INIT;
+ struct string_list lines = STRING_LIST_INIT_DUP;
+ struct strbuf output = STRBUF_INIT;
+ int i, j, cur_nr, ret;
+
+ /* read in both note blob objects */
+ if (!is_null_oid(new_oid))
+ new_msg = read_object_file(new_oid, &new_type, &new_len);
+ if (!new_msg || !new_len || new_type != OBJ_BLOB) {
+ free(new_msg);
+ return 0;
+ }
+ if (!is_null_oid(cur_oid))
+ cur_msg = read_object_file(cur_oid, &cur_type, &cur_len);
+ if (!cur_msg || !cur_len || cur_type != OBJ_BLOB) {
+ free(cur_msg);
+ free(new_msg);
+ oidcpy(cur_oid, new_oid);
+ return 0;
+ }
+
+ /* parse xrefs and de-dup */
+ parse_xref_note(cur_msg, cur_len, NULL, &xrefs, &lines);
+ cur_nr = xrefs.nr;
+ parse_xref_note(new_msg, new_len, NULL, &xrefs, &lines);
+
+ for (i = 0; i < cur_nr; i++)
+ for (j = cur_nr; j < xrefs.nr; j++)
+ if (!strcmp(xrefs.objects[i].name,
+ xrefs.objects[j].name))
+ lines.items[j].string[0] = '\0';
+
+ /* write out the combined object */
+ for (i = 0; i < lines.nr; i++)
+ if (lines.items[i].string[0] != '\0')
+ strbuf_addf(&output, "%s\n", lines.items[i].string);
+
+ ret = write_object_file(output.buf, output.len, blob_type, cur_oid);
+
+ strbuf_release(&output);
+ object_array_clear(&xrefs);
+ string_list_clear(&lines, 0);
+ free(cur_msg);
+ free(new_msg);
+
+ return ret;
+}
diff --git a/notes.h b/notes.h
index 414bc6855..09ca92f36 100644
--- a/notes.h
+++ b/notes.h
@@ -2,6 +2,7 @@
#define NOTES_H
#include "string-list.h"
+#include "object.h"
struct object_id;
struct strbuf;
@@ -317,4 +318,12 @@ void expand_notes_ref(struct strbuf *sb);
*/
void expand_loose_notes_ref(struct strbuf *sb);
+/* For refs/notes/xref- */
+void read_xref_note(const char *notes_ref, const struct object_id *commit_oid,
+ struct object_array *result,
+ struct string_list *result_lines);
+
+int combine_notes_cat_xrefs(struct object_id *cur_oid,
+ const struct object_id *new_oid);
+
#endif
--
2.17.1
^ permalink raw reply related
* [PATCH 1/5] trailer: Implement a helper to reverse-map trailer xrefs
From: Tejun Heo @ 2018-12-11 23:49 UTC (permalink / raw)
To: git, Junio C Hamano, Jeff King; +Cc: kernel-team, Tejun Heo
In-Reply-To: <20181211234909.2855638-1-tj@kernel.org>
From: Tejun Heo <htejun@fb.com>
Some trailers refer to other commits. Let's call them xrefs
(cross-references). For example, a cherry pick trailer points to the
source commit. It is sometimes useful to build a reverse map of these
xrefs - ie. source -> cherry-pick instead of cherry-pick -> source.
This patch implements trailer helpers to enable such reverse maps.
The helpers can process arbitrary trailers and once the xrefs are
recorded, the reverse-mapping can be iterated per source commit using
trailer_rev_xrefs_for_each().
Signed-off-by: Tejun Heo <htejun@fb.com>
---
trailer.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
trailer.h | 38 ++++++++++++++++++++
2 files changed, 143 insertions(+)
diff --git a/trailer.c b/trailer.c
index 0796f326b..15744600b 100644
--- a/trailer.c
+++ b/trailer.c
@@ -2,6 +2,7 @@
#include "config.h"
#include "string-list.h"
#include "run-command.h"
+#include "object-store.h"
#include "commit.h"
#include "tempfile.h"
#include "trailer.h"
@@ -1170,3 +1171,107 @@ void format_trailers_from_commit(struct strbuf *out, const char *msg,
format_trailer_info(out, &info, opts);
trailer_info_release(&info);
}
+
+implement_static_commit_slab(trailer_rxrefs_slab, struct object_array *);
+
+static struct object_array *get_trailer_rxrefs(
+ struct trailer_rev_xrefs *rxrefs, struct commit *commit)
+{
+ struct object_array **slot =
+ trailer_rxrefs_slab_peek(&rxrefs->slab, commit);
+
+ return slot ? *slot : NULL;
+}
+
+static struct object_array *get_create_trailer_rxrefs(
+ struct trailer_rev_xrefs *rxrefs, struct commit *commit)
+{
+ struct object_array **slot =
+ trailer_rxrefs_slab_at(&rxrefs->slab, commit);
+
+ if (*slot)
+ return *slot;
+
+ add_object_array(&commit->object, oid_to_hex(&commit->object.oid),
+ &rxrefs->dst_commits);
+ *slot = xmalloc(sizeof(struct object_array));
+ **slot = (struct object_array)OBJECT_ARRAY_INIT;
+ return *slot;
+}
+
+void trailer_rev_xrefs_init(struct trailer_rev_xrefs *rxrefs,
+ const char *trailer_prefix)
+{
+ rxrefs->trailer_prefix = xstrdup(trailer_prefix);
+ rxrefs->trailer_prefix_len = strlen(trailer_prefix);
+ init_trailer_rxrefs_slab(&rxrefs->slab);
+ rxrefs->dst_commits = (struct object_array)OBJECT_ARRAY_INIT;
+}
+
+/* record the reverse mapping of @commit's xref trailer */
+void trailer_rev_xrefs_record(struct trailer_rev_xrefs *rxrefs,
+ struct commit *commit)
+{
+ struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+ enum object_type type;
+ unsigned long size;
+ void *buffer;
+ struct trailer_info info;
+ int i;
+
+ buffer = read_object_file(&commit->object.oid, &type, &size);
+ trailer_info_get(&info, buffer, &opts);
+
+ /* when nested, the last trailer describes the latest cherry-pick */
+ for (i = info.trailer_nr - 1; i >= 0; i--) {
+ char *line = info.trailers[i];
+
+ if (starts_with(line, rxrefs->trailer_prefix)) {
+ struct object_id dst_oid;
+ struct object *dst_object;
+ struct commit *dst_commit;
+ struct object_array *src_objs;
+ char cherry_hex[GIT_MAX_HEXSZ + 1];
+
+ if (get_oid_hex(line + rxrefs->trailer_prefix_len,
+ &dst_oid))
+ continue;
+
+ dst_object = parse_object(the_repository, &dst_oid);
+ if (!dst_object || dst_object->type != OBJ_COMMIT)
+ continue;
+
+ dst_commit = (struct commit *)dst_object;
+ src_objs = get_create_trailer_rxrefs(rxrefs, dst_commit);
+
+ oid_to_hex_r(cherry_hex, &commit->object.oid);
+ add_object_array(&commit->object, cherry_hex, src_objs);
+ break;
+ }
+ }
+
+ free(buffer);
+}
+
+void trailer_rev_xrefs_release(struct trailer_rev_xrefs *rxrefs)
+{
+ clear_trailer_rxrefs_slab(&rxrefs->slab);
+ object_array_clear(&rxrefs->dst_commits);
+ free(rxrefs->trailer_prefix);
+}
+
+void trailer_rev_xrefs_next(struct trailer_rev_xrefs *rxrefs, int *idx_p,
+ struct commit **dst_commit_p,
+ struct object_array **src_objs_p)
+{
+ if (*idx_p >= rxrefs->dst_commits.nr) {
+ *dst_commit_p = NULL;
+ *src_objs_p = NULL;
+ return;
+ }
+
+ *dst_commit_p = (struct commit *)
+ rxrefs->dst_commits.objects[*idx_p].item;
+ *src_objs_p = get_trailer_rxrefs(rxrefs, *dst_commit_p);
+ (*idx_p)++;
+}
diff --git a/trailer.h b/trailer.h
index b99773964..07090a11f 100644
--- a/trailer.h
+++ b/trailer.h
@@ -2,6 +2,8 @@
#define TRAILER_H
#include "list.h"
+#include "object.h"
+#include "commit-slab.h"
struct strbuf;
@@ -99,4 +101,40 @@ void trailer_info_release(struct trailer_info *info);
void format_trailers_from_commit(struct strbuf *out, const char *msg,
const struct process_trailer_options *opts);
+/*
+ * Helpers to reverse trailers referencing to other commits.
+ *
+ * Some trailers, e.g. "(cherry picked from...)", references other commits.
+ * The following helpers can be used to reverse map those references. See
+ * builtin/reverse-trailer-xrefs.c for a usage example.
+ */
+declare_commit_slab(trailer_rxrefs_slab, struct object_array *);
+
+struct trailer_rev_xrefs {
+ char *trailer_prefix;
+ int trailer_prefix_len;
+ struct trailer_rxrefs_slab slab;
+ struct object_array dst_commits;
+};
+
+void trailer_rev_xrefs_init(struct trailer_rev_xrefs *rxrefs,
+ const char *trailer_prefix);
+void trailer_rev_xrefs_record(struct trailer_rev_xrefs *rxrefs,
+ struct commit *commit);
+void trailer_rev_xrefs_release(struct trailer_rev_xrefs *rxrefs);
+
+void trailer_rev_xrefs_next(struct trailer_rev_xrefs *rxrefs,
+ int *idx_p, struct commit **dst_commit_p,
+ struct object_array **src_objs_p);
+
+/*
+ * Iterate the recorded reverse mappings - @dst_commit was pointed to by
+ * commits in @src_objs.
+ */
+#define trailer_rev_xrefs_for_each(rxrefs, idx, dst_commit, src_objs) \
+ for ((idx) = 0, \
+ trailer_rev_xrefs_next(rxrefs, &(idx), &(dst_commit), &(src_objs));\
+ (dst_commit); \
+ trailer_rev_xrefs_next(rxrefs, &(idx), &(dst_commit), &(src_objs)))
+
#endif /* TRAILER_H */
--
2.17.1
^ permalink raw reply related
* [PATCHSET] git-reverse-trailer-xrefs: Reverse map cherry-picks and other cross-references
From: Tejun Heo @ 2018-12-11 23:49 UTC (permalink / raw)
To: git, Junio C Hamano, Jeff King; +Cc: kernel-team
Hello,
Some trailers refer to other commits. Let's call them xrefs
(cross-references). For example, a cherry pick trailer points to the
source commit. It is sometimes useful to build a reverse map of these
xrefs - ie. source -> cherry-pick instead of cherry-pick -> source.
This, e.g, can answer which releases a commit ended up in on
repositories which cherry-picks fixes back from the development
branch. Let's say the repository looks like the following.
D'---E'' release-B
/
C' E' release-D
/ /
A---B---C---D---E master
where the followings cherry-picks took place.
C -> C'
D -> D'
E -> E' -> E''
Using the reverse-mapping in this patchset, we can get the following
annotated log which succinctly shows which commit ended up where.
$ git log --notes=xref-cherry-picks --oneline | git name-rev --name-only --stdin
4b165af commit E
Notes (xref-cherry-picks):
Cherry-picked-to: release-D
Cherry-picked-to: release-B
82bf1f3 commit D
Notes (xref-cherry-picks):
Cherry-picked-to: release-B~1
364eccf commit C
Notes (xref-cherry-picks):
Cherry-picked-to: release-B~2
ed3adf3 commit B
ddd1bf2 commit A
This patchset implements generic trailer cross-reference
reverse-mapping and the necessary hooks and samples to keep
cherry-pick reverse-maps up-to-date.
diffstat follows. Thanks.
Documentation/git-cherry-pick.txt | 5
Documentation/git-fetch.txt | 5
Documentation/git-notes.txt | 8
Documentation/git-reverse-trailer-xrefs.txt | 145 +++++++++++++++
Documentation/githooks.txt | 23 ++
Makefile | 1
builtin.h | 1
builtin/fetch.c | 72 +++++++
builtin/reverse-trailer-xrefs.c | 160 +++++++++++++++++
builtin/revert.c | 14 +
git.c | 1
notes-merge.c | 9
notes-utils.c | 2
notes-utils.h | 3
notes.c | 260 +++++++++++++++++++++++++++-
notes.h | 9
t/t3321-notes-xref-cherry-picks.sh | 124 +++++++++++++
templates/hooks--post-cherry-pick.sample | 8
templates/hooks--post-fetch.sample | 30 +++
trailer.c | 105 +++++++++++
trailer.h | 38 ++++
21 files changed, 1015 insertions(+), 8 deletions(-)
--
tejun
^ permalink raw reply
* Re: [PATCH 0/8] introduce no-overlay and cached mode in git checkout
From: Thomas Gummerer @ 2018-12-11 22:52 UTC (permalink / raw)
To: Elijah Newren
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc
In-Reply-To: <CABPp-BFaWtDiBPuGQVU_+VGQtDkemDKnvjHhz+h1sbUGssffmQ@mail.gmail.com>
On 12/10, Elijah Newren wrote:
> On Sun, Dec 9, 2018 at 12:04 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > Here's the series I mentioned a couple of times on the list already,
> > introducing a no-overlay mode in 'git checkout'. The inspiration for
> > this came from Junios message in [*1*].
> >
> > Basically the idea is to also delete files when the match <pathspec>
> > in 'git checkout <tree-ish> -- <pathspec>' in the current tree, but
> > don't match <pathspec> in <tree-ish>. The rest of the cases are
> > already properly taken care of by 'git checkout'.
>
> Yes, but I'd put it a little differently:
>
> """
> Basically, the idea is when the user run "git checkout --no-overlay
> <tree-ish> -- <pathspec>" that the given pathspecs should exactly
> match <tree-ish> after the operation completes. This means that we
> also want to delete files that match <pathspec> if those paths are not
> found in <tree-ish>.
> """
>
> ...and maybe even toss in some comments about the fact that this is
> the way git checkout should have always behaved, it just traditionally
> hasn't. (You could also work in comments about how with this new mode
> the user can run git diff afterward with the given commit-ish and
> pathspecs and get back an empty diff, as expected, which wasn't true
> before. But maybe I'm belaboring the point.)
>
>
> > The final step in the series is to actually make use of this in 'git
> > stash', which simplifies the code there a bit. I am however happy to
> > hold off on this step until the stash-in-C series is merged, so we
> > don't delay that further.
> >
> > In addition to the no-overlay mode, we also add a --cached mode, which
> > works only on the index, thus similar to 'git reset <tree-ish> -- <pathspec>'.
>
> If you're adding a --cached mode to make it only work on the index,
> should there be a similar mode to allow it to only work on the working
> tree? (I'm not as concerned with that here, but I really think the
> new restore-files command by default should only operate on the
> working tree, and then have options to affect the index either in
> addition or instead of the working tree.)
Yeah I think that would be nice to have, though I'm not sure what we
would name it in 'git checkout'. Maybe just having it in 'git
restore-files' is good enough?
> > Actually deprecating 'git reset <tree-ish> -- <pathspec>' should come
> > later, probably not before Duy's restore-files command lands, as 'git
> > checkout --no-overlay <tree-ish> -- <pathspec>' is a bit cumbersome to
> > type compared to 'git reset <tree-ish> -- <pathspec>'.
>
> Makes sense.
>
> > My hope is also that the no-overlay mode could become the new default
> > in the restore-files command Duy is currently working on.
>
> Absolutely, yes. I don't want another broken command. :-)
>
>
> > No documentation yet, as I wanted to get this out for review first.
> > I'm not familiar with most of the code I touched here, so there may
> > well be much better ways to implement some of this, that I wasn't able
> > to figure out. I'd be very happy with some feedback around that.
> >
> > Another thing I'm not sure about is how to deal with conflicts. In
> > the cached mode this patch series is not dealing with it at all, as
> > 'git checkout -- <pathspec>' when pathspec matches a file with
> > conflicts doesn't update the index. For the no-overlay mode, the file
> > is removed if the corresponding stage is not found in the index. I'm
> > however not sure this is the right thing to do in all cases?
>
> Here's how I'd go about analyzing that...
>
> If the user passes a <tree-ish>, then the answer about what to do is
> pretty obvious; the <tree-ish> didn't have conflicts, so conflicted
> paths in the index that match the pathspec should be overwritten with
> whatever version of those paths existed in <tree-ish> (possibly
> implying deletion of some paths).
>
> Also, as you point out, --cached means only modify the index and not
> the working tree; so if they specify both --cached and provide no
> tree, then they've specified a no-op.
>
> So it's only interesting when you have conflicts in the index and
> specify --no-overlay without a <tree-ish> or --cached. This boils
> down to "how do we update the working tree to match the index, when
> the index is conflicted?" A couple points to consider:
> * This is somewhat of an edge case
> * In the normal case --no-overlay is only different from --overlay
> behavior for directories; it'd be nice if that extended to all cases
I'm not sure I follow what you mean here. How is --no-overlay
different from --overlay with respect to directories? It's only
different with respect to deletions, no?
> * How does this command behave without a <tree-ish> when
> --no-overlay is specified and a directory is given for a <pathspec>
> and there aren't any conflicts? Are we being consistent with that
> behavior?
>
>
> However, I think it turns out that the answer is much simpler than all
> that initial analysis or what you say you've implemented. Here's why:
>
> If <pathspec> is a file which is present in both the working tree and
> the index and it has conflicts, then "git checkout -- <pathspec>" will
> currently throw an error:
I think what was missing from my original description, that actually
makes it slightly more interesting from what you describe below is the
'--ours' and '--theirs' flags in 'git checkout', with which one can
check out a version of the file in the working tree. This is where it
gets more interesting.
I think I got the right solution for that in patch 5, with deleting
the file if it's deleted in "their" version and we pass --theirs to
'git checkout', and analogous for --ours. I was just wondering if
there were any further edge cases that I can't think of right no.
> $ git checkout -- subdir/counting
> error: path 'subdir/counting' is unmerged
>
> In fact, even if every entry in subdir/ is a path that is in both the
> index and the working tree (so that --no-overlay and --overlay ought
> to behave the same), if any one of the files in subdir is conflicted,
> attempting to checkout the subdir will abort with this same error
> message and no paths will be updated at all:
>
> $ git checkout -- subdir
> error: path 'subdir/counting' is unmerged
>
> as such, the answer with what to do with --no-overlay mode is pretty
> clear: if the <pathspec> matches _any_ path that is conflicted, simply
> throw an error and abort the operation without making any changes at
> all.
^ permalink raw reply
* Re: [PATCH 5/8] checkout: introduce --{,no-}overlay option
From: Thomas Gummerer @ 2018-12-11 22:42 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Elijah Newren
In-Reply-To: <CACsJy8BWZyFWZ70y4beCEYvjbc2+X-2K+gOiY+=toK0y3xYzKw@mail.gmail.com>
On 12/10, Duy Nguyen wrote:
> On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > @@ -302,15 +310,29 @@ static int checkout_paths(const struct checkout_opts *opts,
> > ce->ce_flags &= ~CE_MATCHED;
> > if (!opts->ignore_skipworktree && ce_skip_worktree(ce))
> > continue;
> > - if (opts->source_tree && !(ce->ce_flags & CE_UPDATE))
> > - /*
> > - * "git checkout tree-ish -- path", but this entry
> > - * is in the original index; it will not be checked
> > - * out to the working tree and it does not matter
> > - * if pathspec matched this entry. We will not do
> > - * anything to this entry at all.
> > - */
> > - continue;
> > + if (opts->source_tree && !(ce->ce_flags & CE_UPDATE)) {
> > + if (!opts->overlay_mode &&
> > + ce_path_match(&the_index, ce, &opts->pathspec, ps_matched)) {
> > + /*
> > + * "git checkout --no-overlay <tree-ish> -- path",
> > + * and the path is not in tree-ish, but is in
> > + * the current index, which means that it should
> > + * be removed.
> > + */
> > + ce->ce_flags |= CE_MATCHED | CE_REMOVE | CE_WT_REMOVE;
> > + continue;
> > + } else {
>
> In non-overlay mode but when pathspec does not match, we come here too.
>
> > + /*
> > + * "git checkout tree-ish -- path", but this
> > + * entry is in the original index; it will not
>
> I think the missing key point in this comment block is "..is in the
> original index _and it's not in tree-ish_". In non-overlay mode, if
> pathspec does not match then it's safe to ignore too. But this logic
> starts too get to complex and hurt my brain.
Yes, that would make it a bit easier to read. I took a while to try
and refactor this to make it easier to read, but couldn't come up with
anything much better unfortunately. I'll have another stab at
simplifying the logic a bit for v2.
> > + * be checked out to the working tree and it
> > + * does not matter if pathspec matched this
> > + * entry. We will not do anything to this entry
> > + * at all.
> > + */
> > + continue;
> > + }
> > + }
> > /*
> > * Either this entry came from the tree-ish we are
> > * checking the paths out of, or we are checking out
>
> > @@ -1266,6 +1299,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> > "checkout", "control recursive updating of submodules",
> > PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater },
> > OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
> > + OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
>
> maybe add " (default)" to the help string.
Makes sense, will add.
> > OPT_END(),
> > };
> >
> --
> Duy
^ permalink raw reply
* Re: [PATCH 7/8] checkout: allow ignoring unmatched pathspec
From: Thomas Gummerer @ 2018-12-11 22:36 UTC (permalink / raw)
To: Elijah Newren
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc
In-Reply-To: <CABPp-BG9vjXF-entmun4+dMoOsZdjMgotSOtUUeNxO8c2VwgXA@mail.gmail.com>
On 12/10, Elijah Newren wrote:
> On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > Currently when 'git checkout -- <pathspec>...' is invoked with
> > multiple pathspecs, where one or more of the pathspecs don't match
> > anything, checkout errors out.
> >
> > This can be inconvenient in some cases, such as when using git
> > checkout from a script. Introduce a new --ignore-unmatched option,
> > which which allows us to ignore a non-matching pathspec instead of
> > erroring out.
> >
> > In a subsequent commit we're going to start using 'git checkout' in
> > 'git stash' and are going to make use of this feature.
>
> This makes sense, but seems incomplete. But to explain it, I think
> there's another bug I need to demonstrate first because it's related
> on builds on it. First, the setup:
>
> $ echo foo >subdir/newfile
> $ git add subdir/newfile
> $ echo bar >>subdir/newfile
> $ git status
> On branch A
> Changes to be committed:
> (use "git reset HEAD <file>..." to unstage)
>
> new file: subdir/newfile
>
> Changes not staged for commit:
> (use "git add <file>..." to update what will be committed)
> (use "git checkout -- <file>..." to discard changes in working directory)
>
> modified: subdir/newfile
>
> Now, does it do what we expect?
>
> $ git checkout HEAD -- subdir/newfile
> error: pathspec 'subdir/newfile' did not match any file(s) known to git
>
> This is the old overlay behavior; kinda lame, but you made no claims
> about fixing the default behavior. What about with your new option?
>
> $ git checkout --no-overlay HEAD -- subdir
> $ git status
> On branch A
> nothing to commit, working tree clean
>
> Yes, the feature seems to work as advertised. However, let's try
> again with a different variant:
>
> $ echo foo >subdir/newfile
> $ git checkout --no-overlay HEAD -- subdir
> $ git status
> On branch A
> Untracked files:
> (use "git add <file>..." to include in what will be committed)
>
> subdir/newfile
>
> Why is the file ignored and left there? Also:
>
> $ git checkout --no-overlay HEAD -- subdir/newfile
> error: pathspec 'subdir/newfile' did not match any file(s) known to git
>
> That seems wrong to me.
Ah interesting, this is a case I didn't consider. I'm a bit torn on
this one. My intention for the no overlay mode was that it would work
similar to what I'd expect 'git reset --hard -- <pathspec>' to work if
it existed, which means not removing untracked files if they exist.
While I think in the example you have above removing subdir/newfile
may be the right behaviour I'm not so sure in the case of 'git
checkout --no-overlay HEAD -- .' or ''git checkout --no-overlay HEAD
-- t/*' for example. I don't think that should remove all untracked
files in the repository or in the t/ directory. Removing untracked
files in that case would probably surprise users more than your case
above would.
I think it's okay to keep considering untracked files as special with
respect to how they are treated by 'git checkout --no-overlay'.
> The point of no-overlay is to make it match
> HEAD, and while subdir/newfile doesn't exist in HEAD or the index it
> does match in the working tree so the intent is clear. But let's say
> that the user did go ahead and specify your new flag:
>
>
> $ git checkout --no-overlay --ignore-unmatch HEAD -- subdir/newfile
> $ git status
> On branch A
> Untracked files:
> (use "git add <file>..." to include in what will be committed)
>
> subdir/newfile
>
> nothing added to commit but untracked files present (use "git add" to track)
>
> So now it avoids erroring out when the user does more work than
> necessary, but it still misses appropriately cleaning up the file.
Yeah this is a good point, this could be more confusing to the user
than the previous case in my opinion. Maybe I'll just drop this patch
for now (and the next one, as it's better to hold of until stash in C
lands anyway), and then try to do all this in-core for 'git stash'.
^ permalink raw reply
* Re: [PATCH 7/8] checkout: allow ignoring unmatched pathspec
From: Thomas Gummerer @ 2018-12-11 22:23 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Elijah Newren
In-Reply-To: <CACsJy8CjQHGANKf2Z=vJL=_ktoeXxOzQGL+VFJC4W63fzok78g@mail.gmail.com>
On 12/10, Duy Nguyen wrote:
> On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > Currently when 'git checkout -- <pathspec>...' is invoked with
> > multiple pathspecs, where one or more of the pathspecs don't match
> > anything, checkout errors out.
> >
> > This can be inconvenient in some cases, such as when using git
> > checkout from a script.
>
> Wait, should scripts go with read-tree, checkout-index or other
> plumbing commands instead?
Possibly. As mentioned in an other email, we do seem to have some
scripts in git.git that are using 'git checkout' already, but they are
using it in the checkout branch mode, rather than the checkout paths
mode that I would like to use it in git-stash.
But with the rewrite of 'git stash' in C, maybe this step is moot
anyway, and we can just call the checkout_paths function internally
without using the run_command API at all. We could then have an
internal mode for ignoring unmatched pathspecs that we wouldn't need
to expose to users.
> --
> Duy
^ permalink raw reply
* Re: [PATCH 6/8] checkout: add --cached option
From: Thomas Gummerer @ 2018-12-11 22:18 UTC (permalink / raw)
To: Elijah Newren
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc
In-Reply-To: <CABPp-BEyRNniNFBuMMFXjvuSz9RtP6R7TeBAWDJO+Y70oe7CBA@mail.gmail.com>
On 12/10, Elijah Newren wrote:
> On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > Add a new --cached option to git checkout, which works only on the
> > index, but not the working tree, similar to what 'git reset <tree-ish>
> > -- <pathspec>... does. Indeed the tests are adapted from the 'git
> > reset' tests.
> >
> > In the longer term the idea is to potentially deprecate 'git reset
> > <tree-ish> -- <pathspec>...', so the 'git reset' command becomes only
> > about re-pointing the HEAD, and not also about copying entries from
> > <tree-ish> to the index.
> >
> > Note that 'git checkout' by default works in overlay mode, meaning
> > files that match the pathspec that don't exist in <tree-ish>, but
> > exist in the index would not be removed. 'git checkout --no-overlay
> > --cached' can be used to get the same behaviour as 'git reset
> > <tree-ish> -- <pathspec>'.
>
> I think this argues _even more_ that --no-overlay should be the
> default. Your series is valuable even if we don't push on that, I'm
> just being noisy about what I think would be an even better world.
I think just having that mode in 'git restore-files' Duy is working on
may have to be enough for now.
> Also, I don't think I've mentioned it yet, but I'm really excited
> about this series and what you're doing. It's super cool. (Which I
> expected when I saw the description of the desired behavior, but I'm
> also liking and contemplating re-using some code...)
Thanks :)
> > One thing this patch doesn't currently deal with is conflicts.
> > Currently 'git checkout --{ours,theirs} -- <file-with-conflicts>'
> > doesn't do anything with the index, so the --cached option just
> > mirrors that behaviour. But given it doesn't even deal with
> > conflicts, the '--cached' option doesn't make much sense when no
> > <tree-ish> is given. As it operates only on the index, it's always a
> > no-op if no tree-ish is given.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> >
> > Maybe we can just disallow --cached without <tree-ish> given for now,
> > and possibly later allow it with some different behaviour for
> > conflicts, not sure what the best way forward here is. We can also
> > just make it update the index as appropriate, and have it behave
> > different than 'git checkout' curerntly does when handling conflicts?
>
> Huh?
> "git checkout -- <path>"
> means update <path> from the index, meaning the index is left alone
> (it's the source) and only the working tree is touched.
>
> When you add a flag named --cached to only update the index and not
> the working tree, then the index becomes the sole destination.
>
> Now we combine: no tree is specified means the index is the source of
> the writing, and --cached being specified means the index is the sole
> destination of the writing. Thus, you have a no-op. If the user
> specifies --cached and no tree, you should immediately exit with a
> message along the lines of "Nothing to do; no tree given and --cached
> specified." The presence of conflicts seems completely irrelevant to
> me here.
Ah yeah you're right, thanks for a sanity check. The command I was
most worried about was 'git checkout --cached --{ours,theirs} -- <pathspec>',
which I thought should update the index. But as we don't give any
tree-ish, I'm not sure anymore it should. Maybe just always exiting
with the message you mention above is the right thing to do.
> > builtin/checkout.c | 26 ++++++++--
> > t/t2016-checkout-patch.sh | 8 +++
> > t/t2026-checkout-cached.sh | 103 +++++++++++++++++++++++++++++++++++++
> > t/t9902-completion.sh | 1 +
> > 4 files changed, 135 insertions(+), 3 deletions(-)
> > create mode 100755 t/t2026-checkout-cached.sh
> >
> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> > index 0aef35bbc4..6ba85e9de5 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -45,6 +45,7 @@ struct checkout_opts {
> > int ignore_other_worktrees;
> > int show_progress;
> > int overlay_mode;
> > + int cached;
> > /*
> > * If new checkout options are added, skip_merge_working_tree
> > * should be updated accordingly.
> > @@ -288,6 +289,10 @@ static int checkout_paths(const struct checkout_opts *opts,
> > die(_("Cannot update paths and switch to branch '%s' at the same time."),
> > opts->new_branch);
> >
> > + if (opts->patch_mode && opts->cached)
> > + return run_add_interactive(revision, "--patch=reset",
> > + &opts->pathspec);
> > +
> > if (opts->patch_mode)
> > return run_add_interactive(revision, "--patch=checkout",
> > &opts->pathspec);
> > @@ -319,7 +324,9 @@ static int checkout_paths(const struct checkout_opts *opts,
> > * the current index, which means that it should
> > * be removed.
> > */
> > - ce->ce_flags |= CE_MATCHED | CE_REMOVE | CE_WT_REMOVE;
> > + ce->ce_flags |= CE_MATCHED | CE_REMOVE;
> > + if (!opts->cached)
> > + ce->ce_flags |= CE_WT_REMOVE;
> > continue;
> > } else {
> > /*
> > @@ -392,6 +399,9 @@ static int checkout_paths(const struct checkout_opts *opts,
> > for (pos = 0; pos < active_nr; pos++) {
> > struct cache_entry *ce = active_cache[pos];
> > if (ce->ce_flags & CE_MATCHED) {
> > + if (opts->cached) {
> > + continue;
> > + }
> > if (!ce_stage(ce)) {
> > errs |= checkout_entry(ce, &state, NULL);
> > continue;
> > @@ -571,6 +581,11 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
> > * not tested here
> > */
> >
> > + /*
> > + * opts->cached cannot be used with switching branches so is
> > + * not tested here
> > + */
> > +
> > /*
> > * If we aren't creating a new branch any changes or updates will
> > * happen in the existing branch. Since that could only be updating
> > @@ -1207,9 +1222,13 @@ static int checkout_branch(struct checkout_opts *opts,
> > die(_("'%s' cannot be used with switching branches"),
> > "--patch");
> >
> > - if (!opts->overlay_mode)
> > + if (opts->overlay_mode != -1)
> > + die(_("'%s' cannot be used with switching branches"),
> > + "--overlay/--no-overlay");
> > +
> > + if (opts->cached)
> > die(_("'%s' cannot be used with switching branches"),
> > - "--no-overlay");
> > + "--cached");
> >
> > if (opts->writeout_stage)
> > die(_("'%s' cannot be used with switching branches"),
> > @@ -1300,6 +1319,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> > PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater },
> > OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
> > OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
> > + OPT_BOOL(0, "cached", &opts.cached, N_("work on the index only")),
> > OPT_END(),
> > };
> >
> > diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> > index 47aeb0b167..e8774046e0 100755
> > --- a/t/t2016-checkout-patch.sh
> > +++ b/t/t2016-checkout-patch.sh
> > @@ -108,6 +108,14 @@ test_expect_success PERL 'path limiting works: foo inside dir' '
> > verify_state dir/foo head head
> > '
> >
> > +test_expect_success PERL 'git checkout --cached -p' '
> > + set_and_save_state dir/foo work work &&
> > + test_write_lines n y | git checkout --cached -p >output &&
> > + verify_state dir/foo work head &&
> > + verify_saved_state bar &&
> > + test_i18ngrep "Unstage" output
> > +'
> > +
> > test_expect_success PERL 'none of this moved HEAD' '
> > verify_saved_head
> > '
> > diff --git a/t/t2026-checkout-cached.sh b/t/t2026-checkout-cached.sh
> > new file mode 100755
> > index 0000000000..1b66192727
> > --- /dev/null
> > +++ b/t/t2026-checkout-cached.sh
> > @@ -0,0 +1,103 @@
> > +#!/bin/sh
> > +
> > +test_description='checkout --cached <pathspec>'
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'checkout --cached <pathspec>' '
> > + echo 1 >file1 &&
> > + echo 2 >file2 &&
> > + git add file1 file2 &&
> > + test_tick &&
> > + git commit -m files &&
> > + git rm file2 &&
> > + echo 3 >file3 &&
> > + echo 4 >file1 &&
> > + git add file1 file3 &&
> > + git checkout --cached HEAD -- file1 file2 &&
> > + test_must_fail git diff --quiet &&
> > +
> > + cat >expect <<-\EOF &&
> > + diff --git a/file1 b/file1
> > + index d00491f..b8626c4 100644
> > + --- a/file1
> > + +++ b/file1
> > + @@ -1 +1 @@
> > + -1
> > + +4
> > + diff --git a/file2 b/file2
> > + deleted file mode 100644
> > + index 0cfbf08..0000000
> > + --- a/file2
> > + +++ /dev/null
> > + @@ -1 +0,0 @@
> > + -2
> > + EOF
> > + git diff >actual &&
> > + test_cmp expect actual &&
> > +
> > + cat >expect <<-\EOF &&
> > + diff --git a/file3 b/file3
> > + new file mode 100644
> > + index 0000000..00750ed
> > + --- /dev/null
> > + +++ b/file3
> > + @@ -0,0 +1 @@
> > + +3
> > + EOF
> > + git diff --cached >actual &&
> > + test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'checking out an unmodified path is a no-op' '
> > + git reset --hard &&
> > + git checkout --cached HEAD -- file1 &&
> > + git diff-files --exit-code &&
> > + git diff-index --cached --exit-code HEAD
> > +'
> > +
> > +test_expect_success 'checking out specific path that is unmerged' '
> > + test_commit file3 file3 &&
> > + git rm --cached file2 &&
> > + echo 1234 >file2 &&
> > + F1=$(git rev-parse HEAD:file1) &&
> > + F2=$(git rev-parse HEAD:file2) &&
> > + F3=$(git rev-parse HEAD:file3) &&
> > + {
> > + echo "100644 $F1 1 file2" &&
> > + echo "100644 $F2 2 file2" &&
> > + echo "100644 $F3 3 file2"
> > + } | git update-index --index-info &&
> > + git ls-files -u &&
> > + git checkout --cached HEAD file2 &&
> > + test_must_fail git diff --quiet &&
> > + git diff-index --exit-code --cached HEAD
> > +'
> > +
> > +test_expect_success '--cached without --no-overlay does not remove entry from index' '
> > + test_must_fail git checkout --cached HEAD^ file3 &&
> > + git ls-files --error-unmatch -- file3
> > +'
> > +
> > +test_expect_success 'file is removed from the index with --no-overlay' '
> > + git checkout --cached --no-overlay HEAD^ file3 &&
> > + test_path_is_file file3 &&
> > + test_must_fail git ls-files --error-unmatch -- file3
> > +'
> > +
> > +test_expect_success 'test checkout --cached --no-overlay at given paths' '
> > + mkdir sub &&
> > + >sub/file1 &&
> > + >sub/file2 &&
> > + git update-index --add sub/file1 sub/file2 &&
> > + T=$(git write-tree) &&
> > + git checkout --cached --no-overlay HEAD sub/file2 &&
> > + test_must_fail git diff --quiet &&
> > + U=$(git write-tree) &&
>
> Do we need to worry at all about losing the exit status of write-tree
> in either invocation? In particular, if the second one for U fails
> somehow, we'd end up with $U being a blank string and we'd still
> probably get "$T" != "$U" below.
Hmm this seems to be a fairly common pattern in our test suite:
$ git grep -F '$(git write-tree)' t/* | wc -l
112
But maybe it's just something we used to do, but should move away
from. Just writing the output to a file shouldn't be much harder
either, I'll do that in the next iteration.
> You also had some rev-parse invocations hidden in a sub-shell in both
> this patch and patch 5, but subsequent commands relied on non-empty
> output out of those, so I figured those were fine. This one might be
> too, but I thought I'd at least mention it.
>
> > + echo "$T" &&
> > + echo "$U" &&
> > + test_must_fail git diff-index --cached --exit-code "$T" &&
> > + test "$T" != "$U"
> > +'
> > +
> > +test_done
> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > index a3fd9a9630..cbc304ace8 100755
> > --- a/t/t9902-completion.sh
> > +++ b/t/t9902-completion.sh
> > @@ -1437,6 +1437,7 @@ test_expect_success 'double dash "git checkout"' '
> > --no-quiet Z
> > --no-... Z
> > --overlay Z
> > + --cached Z
> > EOF
> > '
> >
> > --
> > 2.20.0.405.gbc1bbc6f85
^ permalink raw reply
* Re: [PATCH 5/8] checkout: introduce --{,no-}overlay option
From: Thomas Gummerer @ 2018-12-11 22:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren, Git Mailing List,
Nguyễn Thái Ngọc
In-Reply-To: <xmqqzhtclq22.fsf@gitster-ct.c.googlers.com>
On 12/11, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
>
> >> Note that 'git checkout -p <tree-ish> -- [<pathspec>]' already works
> >> this way, so no changes are needed for the patch mode. We disallow
> >> 'git checkout --overlay -p' to avoid confusing users who would expect
> >> to be able to force overlay mode in 'git checkout -p' this way.
> >
> > Whoa...that's interesting. To me, that argues even further that the
> > traditional checkout behavior was wrong all along and the choice of
> > --overlay vs. --no-overlay in the original implementation was a total
> > oversight. I'm really tempted to say that --no-overlay should just be
> > the default in checkout too...but maybe that's too high a hill to
> > climb, at least for now.
>
> You are exhibiting a typical reaction to a shiny new toy.
I wonder whether it's worth introducing a config option for this, so
people could use this new mode by default if they wanted to, without
having to type the extra argument?
'git checkout' is a plumbing command, but I see that some of the shell
scripts in git.git are using it. Though they are only using it in the
checkout branch mode, rather than the checkout paths mode.
> The original "checkout paths out of the trees and/or the index to
> recover what was lost" is modeled after "cp -R .../else/where .",
> which is an overlay operation, and you wouldn't imagine complaining
> that "cp -R" is wrong not to remove things in the target, to be
> equivalent to "rm -fr where && cp -R .../else/where .".
>
> Each has its own uses. We just didn't have the other half of the
> pair.
>
> If anything, I would consider "checkout -p" that does not do overlay
> is a bug that needs to be fixed.
>
^ permalink raw reply
* Re: [PATCH 3/8] entry: support CE_WT_REMOVE flag in checkout_entry
From: Thomas Gummerer @ 2018-12-11 22:00 UTC (permalink / raw)
To: Elijah Newren
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc
In-Reply-To: <CABPp-BHO7UmAm14HWf2tc+SVEZuWBqyvvrQg625fDsiDYPEL8g@mail.gmail.com>
On 12/10, Elijah Newren wrote:
> On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > 'checkout_entry()' currently only supports creating new entries in the
> > working tree, but not deleting them. Add the ability to remove
> > entries at the same time if the entry is marked with the CE_WT_REMOVE
> > flag.
> >
> > Currently this doesn't have any effect, as the CE_WT_REMOVE flag is
> > only used in unpack-tree, however we will make use of this in a
> > subsequent step in the series.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> > entry.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/entry.c b/entry.c
> > index 3ec148ceee..cd1c6601b6 100644
> > --- a/entry.c
> > +++ b/entry.c
> > @@ -441,6 +441,13 @@ int checkout_entry(struct cache_entry *ce,
> > static struct strbuf path = STRBUF_INIT;
> > struct stat st;
> >
> > + if (ce->ce_flags & CE_WT_REMOVE) {
> > + if (topath)
> > + BUG("Can't remove entry to a path");
>
> Minor nit: This error message is kinda hard to parse, for someone not
> that familiar with all the *_entry functions, like myself. Maybe add
> a comment before this line:
> /* No content and thus no path to create, so we have no pathname
> to return */
> or reword the error slightly? Or maybe it's fine and I was just
> confused from lack of code familiarity, but I'll throw it out there
> since I stumbled on it a bit.
I'll try to make it more clear in the new round, thanks!
> > + unlink_entry(ce);
> > + return 0;
> > + }
> > +
> > if (topath)
> > return write_entry(ce, topath, state, 1);
> >
> > --
> > 2.20.0.405.gbc1bbc6f85
^ permalink raw reply
* Re: [PATCH 4/8] read-cache: add invalidate parameter to remove_marked_cache_entries
From: Thomas Gummerer @ 2018-12-11 21:59 UTC (permalink / raw)
To: Elijah Newren
Cc: Nguyễn Thái Ngọc, Git Mailing List,
Junio C Hamano
In-Reply-To: <CABPp-BEkeRa7jOkDcNNpZMY9J9JmNGtMKjZeNv8i_u7jUFihcw@mail.gmail.com>
On 12/10, Elijah Newren wrote:
> On Mon, Dec 10, 2018 at 8:09 AM Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > >
> > > When marking cache entries for removal, and later removing them all at
> > > once using 'remove_marked_cache_entries()', cache entries currently
> > > have to be invalidated manually in the cache tree and in the untracked
> > > cache.
> > >
> > > Add an invalidate flag to the function. With the flag set, the
> > > function will take care of invalidating the path in the cache tree and
> > > in the untracked cache.
> > >
> > > This will be useful in a subsequent commit.
> > >
> > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > > ---
> > >
> > > For the two current callsites, unpack-trees seems to do this
> > > invalidation itself internally.
> >
> > I'm still a bit scared of this invalidation business in unpack-trees.
> > The thing is, we handle two separate index_state there, src_index and
> > result and invalidation has to be done on the right one (because index
> > extensions are on src_index until the very end of unpack-trees;
> > invalidating on 'result' would be no-op and wrong).
> > remove_marked_cache_entries() seems to be called on 'result' while
> > invalidate_ce_path() is on src_index, hm....
>
> Is Thomas avoiding problems here simply because merge is the only
> caller of unpack_trees with src_index != dst_index? Or does src_index
> == dst_index for checkout not actually help?
I'm trying to avoid problems in this patch by keeping status quo, and
not changing the cache-tree invalidation in any way. 'git checkout --
<pathspec>' doesn't use unpack-trees, so I don't think I have to worry
about src_index vs. dst_index.
In what I was saying above I was merely trying to explain why we don't
need invalidate the cache-tree in the 'remove_marked_cache_entries()'
function.
> If that does help with the checkout case, then allow me to find a
> different way to muddy the waters... I think I might want to make use
> of this function in the merge machinery at some point, so I either
> need to figure out how to convince you to verify if all this cache
> tree invalidation stuff is sane, or somehow figure out all the
> cache_tree stuff stuff myself so I can figure out what is right here.
> :-)
>
> > > I don't quite understand why we don't
> > > need it in split-index mode though. I assume it's because the cache
> > > tree in the main index would already have been invalidated? I didn't
> > > have much time to dig, but couldn't produce any failures with it
> > > either, so I assume not invalidating paths is the right thing to do
> > > here.
> >
> > Yeah I think it's because cache-tree and untracked cache are already
> > properly invalidated. This merge base thingy is done when we load the
> > index files up, not when we write them down. The "front" index may
> > record that a few paths in the base index are no longer valid and need
> > to be deleted. But untracked cache and cache-tree both should have
> > recorded that same info when these paths are marked for delete at
> > index write time.
Thanks, that makes sense to me.
^ permalink raw reply
* Re: [PATCH 1/8] move worktree tests to t24*
From: Thomas Gummerer @ 2018-12-11 21:50 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Elijah Newren
In-Reply-To: <CACsJy8AgbU9YyMHXdp=bkMncBO_Mu0FOQ4kSRkgacHzTJ0DrdA@mail.gmail.com>
On 12/10, Duy Nguyen wrote:
> On Sun, Dec 9, 2018 at 9:04 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > The 'git worktree' command used to be just another mode in 'git
> > checkout', namely 'git checkout --to'. When the tests for the latter
> > were retrofitted for the former, the test name was adjusted, but the
> > test number was kept, even though the test is testing a different
> > command now. t/README states: "Second digit tells the particular
> > command we are testing.", so 'git worktree' should have a separate
> > number just for itself.
> >
> > Move the worktree tests to t24* to adhere to that guideline. We're
> > going to make use of the free'd up numbers in a subsequent commit.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> > t/{t2025-worktree-add.sh => t2400-worktree-add.sh} | 0
> > t/{t2026-worktree-prune.sh => t2401-worktree-prune.sh} | 0
> > t/{t2027-worktree-list.sh => t2402-worktree-list.sh} | 0
> > 3 files changed, 0 insertions(+), 0 deletions(-)
> > rename t/{t2025-worktree-add.sh => t2400-worktree-add.sh} (100%)
> > rename t/{t2026-worktree-prune.sh => t2401-worktree-prune.sh} (100%)
> > rename t/{t2027-worktree-list.sh => t2402-worktree-list.sh} (100%)
>
> Heh.. I did the same thing (in my unsent switch-branch/restore-files
> series) and even used the same 24xx range :D You probably want to move
> t2028 and t2029 too (not sure if they have landed on 'master')
:) I unfortunately didn't have time to read the
switch-branch/restore-files series in detail, but good to know someone
thought the same way. I started this work before t2028 and t2029
landed on master, so I failed to notice them. But I'll rebase on
master and move these two tests as well, thanks for noticing.
> --
> Duy
^ permalink raw reply
* Re: [PATCH 0/3] Add a GIT_TEST_PROTOCOL_VERSION=X test mode
From: Ævar Arnfjörð Bjarmason @ 2018-12-11 21:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan
In-Reply-To: <20181211212135.21126-1-avarab@gmail.com>
On Tue, Dec 11 2018, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Dec 11 2018, Jeff King wrote:
>
>> On Tue, Dec 11, 2018 at 12:45:16PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>>> > I don't know if there's a good solution. I tried running the whole
>>> > test suite with v2 as the default. It does find this bug, but it has
>>> > a bunch of other problems (notably fetch-pack won't run as v2, but
>>> > some other tests I think also depend on v0's reachability rules,
>>> > which v2 is documented not to enforce).
>>>
>>> I think a global test mode for it would be a very good idea.
>>
>> Yeah, but somebody needs to pick through the dozens of false positives
>> for it to be useful.
>
> Here's that test mode. As noted in 3/3 there may be more bugs revealed
> by this, but let's first start by marking where the behavior differs.
...and I forgot to mention. This goes on top of Jeff's series here to
fix this "hidden refs" case.
> Ævar Arnfjörð Bjarmason (3):
> tests: add a special setup where for protocol.version
> tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=1
> tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
>
> protocol.c | 13 ++++++++++++-
> t/README | 4 ++++
> t/t0410-partial-clone.sh | 1 +
> t/t5400-send-pack.sh | 2 +-
> t/t5500-fetch-pack.sh | 4 +++-
> t/t5503-tagfollow.sh | 8 ++++----
> t/t5512-ls-remote.sh | 8 ++++----
> t/t5515-fetch-merge-logic.sh | 1 +
> t/t5516-fetch-push.sh | 4 +++-
> t/t5537-fetch-shallow.sh | 3 ++-
> t/t5552-skipping-fetch-negotiator.sh | 1 +
> t/t5601-clone.sh | 1 +
> t/t5616-partial-clone.sh | 3 ++-
> t/t5700-protocol-v1.sh | 1 +
> t/t5702-protocol-v2.sh | 1 +
> t/t7406-submodule-update.sh | 3 ++-
> 16 files changed, 43 insertions(+), 15 deletions(-)
^ permalink raw reply
* [PATCH 3/3] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
From: Ævar Arnfjörð Bjarmason @ 2018-12-11 21:21 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Ævar Arnfjörð Bjarmason
In-Reply-To: <20181211135501.GA13731@sigill.intra.peff.net>
Mark those tests that have behavior differences or bugs under
protocol.version=0.
Whether or not these tests should exhibit different behavior is
outside the scope of this change. Some (such as t5700-protocol-v1.sh)
clearly should, but others (such as t7406-submodule-update.sh) might
indicate bugs in the protocol v2 code.
Tracking down which is which is outside the scope of this
change. Let's first exhaustively annotate where the differences are,
so that we can spot future behavior differences or regressions.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t5500-fetch-pack.sh | 4 +++-
t/t5503-tagfollow.sh | 8 ++++----
t/t5512-ls-remote.sh | 8 ++++----
t/t5515-fetch-merge-logic.sh | 1 +
t/t5516-fetch-push.sh | 3 ++-
t/t5537-fetch-shallow.sh | 3 ++-
t/t5552-skipping-fetch-negotiator.sh | 1 +
t/t5616-partial-clone.sh | 3 ++-
t/t5700-protocol-v1.sh | 1 +
t/t7406-submodule-update.sh | 3 ++-
10 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 086f2c40f6..9c18875c9c 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -41,7 +41,8 @@ pull_to_client () {
test_expect_success "$number pull" '
(
cd client &&
- git fetch-pack -k -v .. $heads &&
+ GIT_TEST_PROTOCOL_VERSION=0 \
+ git fetch-pack -k -v .. $heads &&
case "$heads" in
*A*)
@@ -440,6 +441,7 @@ test_expect_success 'setup tests for the --stdin parameter' '
'
test_expect_success 'fetch refs from cmdline' '
+ sane_unset GIT_TEST_PROTOCOL_VERSION &&
(
cd client &&
git fetch-pack --no-progress .. $(cat ../input)
diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
index 4ca48f0276..220c677f24 100755
--- a/t/t5503-tagfollow.sh
+++ b/t/t5503-tagfollow.sh
@@ -56,7 +56,7 @@ test_expect_success 'fetch A (new commit : 1 connection)' '
rm -f $U &&
(
cd cloned &&
- GIT_TRACE_PACKET=$UPATH git fetch &&
+ GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION=0 git fetch &&
test $A = $(git rev-parse --verify origin/master)
) &&
get_needs $U >actual &&
@@ -86,7 +86,7 @@ test_expect_success 'fetch C, T (new branch, tag : 1 connection)' '
rm -f $U &&
(
cd cloned &&
- GIT_TRACE_PACKET=$UPATH git fetch &&
+ GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION=0 git fetch &&
test $C = $(git rev-parse --verify origin/cat) &&
test $T = $(git rev-parse --verify tag1) &&
test $A = $(git rev-parse --verify tag1^0)
@@ -122,7 +122,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' '
rm -f $U &&
(
cd cloned &&
- GIT_TRACE_PACKET=$UPATH git fetch &&
+ GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION=0 git fetch &&
test $B = $(git rev-parse --verify origin/master) &&
test $B = $(git rev-parse --verify tag2^0) &&
test $S = $(git rev-parse --verify tag2)
@@ -146,7 +146,7 @@ test_expect_success 'new clone fetch master and tags' '
cd clone2 &&
git init &&
git remote add origin .. &&
- GIT_TRACE_PACKET=$UPATH git fetch &&
+ GIT_TRACE_PACKET=$UPATH GIT_TEST_PROTOCOL_VERSION=0 git fetch &&
test $B = $(git rev-parse --verify origin/master) &&
test $S = $(git rev-parse --verify tag2) &&
test $B = $(git rev-parse --verify tag2^0) &&
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index ca69636fd5..28420c4f77 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -223,7 +223,7 @@ test_expect_success 'ls-remote --symref' '
$(git rev-parse refs/tags/mark1.10) refs/tags/mark1.10
$(git rev-parse refs/tags/mark1.2) refs/tags/mark1.2
EOF
- git ls-remote --symref >actual &&
+ GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref >actual &&
test_cmp expect actual
'
@@ -243,7 +243,7 @@ test_expect_failure 'ls-remote with filtered symref (--heads)' '
1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/foo
1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/master
EOF
- git ls-remote --symref --heads . >actual &&
+ GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual &&
test_cmp expect actual
'
@@ -252,9 +252,9 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/foo
1bd44cb9d13204b0fe1958db0082f5028a16eb3a refs/heads/master
EOF
- git ls-remote --symref --heads . >actual &&
+ GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual &&
test_cmp expect actual &&
- git ls-remote --symref . "refs/heads/*" >actual &&
+ GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref . "refs/heads/*" >actual &&
test_cmp expect actual
'
diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
index 36b0dbc01c..2a3d1d84d6 100755
--- a/t/t5515-fetch-merge-logic.sh
+++ b/t/t5515-fetch-merge-logic.sh
@@ -7,6 +7,7 @@
test_description='Merge logic in fetch'
. ./test-lib.sh
+sane_unset GIT_TEST_PROTOCOL_VERSION
LF='
'
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 49e5d305e5..0722d288cd 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1129,7 +1129,8 @@ do
'
done
-test_expect_success 'fetch exact SHA1' '
+test_expect_success 'fetch exact SHA1 in protocol v0' '
+ sane_unset GIT_TEST_PROTOCOL_VERSION &&
mk_test testrepo heads/master hidden/one &&
git push testrepo master:refs/hidden/one &&
(
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 6faf17e17a..f8f14c0ca2 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -127,7 +127,8 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '
git init notshallow &&
(
cd notshallow &&
- git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/*&&
+ GIT_TEST_PROTOCOL_VERSION=0 \
+ git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&
git for-each-ref --format="%(refname)" >actual.refs &&
cat <<EOF >expect.refs &&
refs/remotes/shallow/no-shallow
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index 30857b84a8..c5b39b8248 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -127,6 +127,7 @@ test_expect_success 'use ref advertisement to filter out commits' '
# not need to send any ancestors of "c3", but we still need to send "c3"
# itself.
test_config -C client fetch.negotiationalgorithm skipping &&
+ sane_unset GIT_TEST_PROTOCOL_VERSION &&
trace_fetch client origin to_fetch &&
have_sent c5 c4^ c2side &&
have_not_sent c4 c4^^ c4^^^
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 336f02a41a..feedf84ce1 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -144,7 +144,8 @@ test_expect_success 'manual prefetch of missing objects' '
sort >observed.oids &&
test_line_count = 6 observed.oids &&
- git -C pc1 fetch-pack --stdin "file://$(pwd)/srv.bare" <observed.oids &&
+ GIT_TEST_PROTOCOL_VERSION=0 \
+ git -C pc1 fetch-pack --stdin "file://$(pwd)/srv.bare" <observed.oids &&
git -C pc1 rev-list --quiet --objects --missing=print \
master..origin/master >revs &&
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index ba86a44eb1..244ff6879d 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -5,6 +5,7 @@ test_description='test git wire-protocol transition'
TEST_NO_CREATE_REPO=1
. ./test-lib.sh
+sane_unset GIT_TEST_PROTOCOL_VERSION
# Test protocol v1 with 'git://' transport
#
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index e87164aa8f..dd41a96c20 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -943,7 +943,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'
cd super3 &&
sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
mv -f .gitmodules.tmp .gitmodules &&
- test_must_fail git submodule update --init --depth=1 2>actual &&
+ test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 \
+ git submodule update --init --depth=1 2>actual &&
test_i18ngrep "Direct fetching of that commit failed." actual &&
git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
git submodule update --init --depth=1 >actual &&
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related
* [PATCH 2/3] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=1
From: Ævar Arnfjörð Bjarmason @ 2018-12-11 21:21 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Ævar Arnfjörð Bjarmason
In-Reply-To: <20181211135501.GA13731@sigill.intra.peff.net>
A few tests are broken under GIT_TEST_PROTOCOL_VERSION=1, which as
protocol.version in git-config(1) notes is just the
GIT_TEST_PROTOCOL_VERSION=0 with a version number.
All of these cases look OK to me, and don't seem to show any
regressions or other behavior differences that are unexpected. These
tests are either testing exact v0 trace output, or trying to test the
v2 protocol.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t0410-partial-clone.sh | 1 +
t/t5400-send-pack.sh | 2 +-
t/t5516-fetch-push.sh | 1 +
t/t5601-clone.sh | 1 +
t/t5702-protocol-v2.sh | 1 +
5 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index ba3887f178..786f96c467 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -171,6 +171,7 @@ test_expect_success 'fetching of missing objects' '
'
test_expect_success 'fetching of missing objects works with ref-in-want enabled' '
+ sane_unset GIT_TEST_PROTOCOL_VERSION &&
# ref-in-want requires protocol version 2
git -C server config protocol.version 2 &&
git -C server config uploadpack.allowrefinwant 1 &&
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index f1932ea431..b84618c925 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -288,7 +288,7 @@ test_expect_success 'receive-pack de-dupes .have lines' '
$shared .have
EOF
- GIT_TRACE_PACKET=$(pwd)/trace \
+ GIT_TRACE_PACKET=$(pwd)/trace GIT_TEST_PROTOCOL_VERSION=0 \
git push \
--receive-pack="unset GIT_TRACE_PACKET; git-receive-pack" \
fork HEAD:foo &&
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 37e8e80893..49e5d305e5 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1172,6 +1172,7 @@ test_expect_success 'fetch exact SHA1' '
'
test_expect_success 'fetch exact SHA1 in protocol v2' '
+ sane_unset GIT_TEST_PROTOCOL_VERSION &&
mk_test testrepo heads/master hidden/one &&
git push testrepo master:refs/hidden/one &&
git -C testrepo config transfer.hiderefs refs/hidden &&
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 8bbc7068ac..a9ce050ee9 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -345,6 +345,7 @@ expect_ssh () {
}
test_expect_success 'clone myhost:src uses ssh' '
+ sane_unset GIT_TEST_PROTOCOL_VERSION &&
git clone myhost:src ssh-clone &&
expect_ssh myhost src
'
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8..df7cc2a43a 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -5,6 +5,7 @@ test_description='test git wire-protocol version 2'
TEST_NO_CREATE_REPO=1
. ./test-lib.sh
+sane_unset GIT_TEST_PROTOCOL_VERSION
# Test protocol v2 with 'git://' transport
#
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related
* [PATCH 1/3] tests: add a special setup where for protocol.version
From: Ævar Arnfjörð Bjarmason @ 2018-12-11 21:21 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Ævar Arnfjörð Bjarmason
In-Reply-To: <20181211135501.GA13731@sigill.intra.peff.net>
Add a GIT_TEST_PROTOCOL_VERSION=X test mode which is equivalent to
running with protocol.version=X. This is needed to spot regressions
and differences such as "ls-refs" behaving differently with
transfer.hideRefs. See
https://public-inbox.org/git/20181211104236.GA6899@sigill.intra.peff.net/
for a fix for that regression.
With this all tests pass with GIT_TEST_PROTOCOL_VERSION=0, but fail
with GIT_TEST_PROTOCOL_VERSION=[1|2]. That's OK since this is a new
test mode, subsequent patches will fix up these test failures.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
protocol.c | 13 ++++++++++++-
t/README | 4 ++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/protocol.c b/protocol.c
index 5e636785d1..cb58cbb29a 100644
--- a/protocol.c
+++ b/protocol.c
@@ -17,7 +17,18 @@ static enum protocol_version parse_protocol_version(const char *value)
enum protocol_version get_protocol_version_config(void)
{
const char *value;
- if (!git_config_get_string_const("protocol.version", &value)) {
+ const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION";
+ const char *git_test_v = getenv(git_test_k);
+
+ if (git_test_v) {
+ enum protocol_version version = parse_protocol_version(git_test_v);
+
+ if (version == protocol_unknown_version)
+ die("unknown value for %s: %s", git_test_k,
+ git_test_v);
+
+ return version;
+ } else if (!git_config_get_string_const("protocol.version", &value)) {
enum protocol_version version = parse_protocol_version(value);
if (version == protocol_unknown_version)
diff --git a/t/README b/t/README
index 28711cc508..c5762a92bc 100644
--- a/t/README
+++ b/t/README
@@ -358,6 +358,10 @@ GIT_TEST_MULTI_PACK_INDEX=<boolean>, when true, forces the multi-pack-
index to be written after every 'git repack' command, and overrides the
'core.multiPackIndex' setting to true.
+GIT_TEST_PROTOCOL_VERSION=<'protocol.version' config value>, when set,
+runs the test suite with the given protocol.version. E.g. "0", "1" or
+"2".
+
Naming Tests
------------
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply related
* [PATCH 0/3] Add a GIT_TEST_PROTOCOL_VERSION=X test mode
From: Ævar Arnfjörð Bjarmason @ 2018-12-11 21:21 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Brandon Williams, Jonathan Tan,
Ævar Arnfjörð Bjarmason
In-Reply-To: <20181211135501.GA13731@sigill.intra.peff.net>
On Tue, Dec 11 2018, Jeff King wrote:
> On Tue, Dec 11, 2018 at 12:45:16PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > I don't know if there's a good solution. I tried running the whole
>> > test suite with v2 as the default. It does find this bug, but it has
>> > a bunch of other problems (notably fetch-pack won't run as v2, but
>> > some other tests I think also depend on v0's reachability rules,
>> > which v2 is documented not to enforce).
>>
>> I think a global test mode for it would be a very good idea.
>
> Yeah, but somebody needs to pick through the dozens of false positives
> for it to be useful.
Here's that test mode. As noted in 3/3 there may be more bugs revealed
by this, but let's first start by marking where the behavior differs.
Ævar Arnfjörð Bjarmason (3):
tests: add a special setup where for protocol.version
tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=1
tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
protocol.c | 13 ++++++++++++-
t/README | 4 ++++
t/t0410-partial-clone.sh | 1 +
t/t5400-send-pack.sh | 2 +-
t/t5500-fetch-pack.sh | 4 +++-
t/t5503-tagfollow.sh | 8 ++++----
t/t5512-ls-remote.sh | 8 ++++----
t/t5515-fetch-merge-logic.sh | 1 +
t/t5516-fetch-push.sh | 4 +++-
t/t5537-fetch-shallow.sh | 3 ++-
t/t5552-skipping-fetch-negotiator.sh | 1 +
t/t5601-clone.sh | 1 +
t/t5616-partial-clone.sh | 3 ++-
t/t5700-protocol-v1.sh | 1 +
t/t5702-protocol-v2.sh | 1 +
t/t7406-submodule-update.sh | 3 ++-
16 files changed, 43 insertions(+), 15 deletions(-)
--
2.20.0.405.gbc1bbc6f85
^ permalink raw reply
* Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1
From: Ævar Arnfjörð Bjarmason @ 2018-12-11 20:51 UTC (permalink / raw)
To: Carlo Arenas; +Cc: sandals, git, pcre-dev
In-Reply-To: <CAPUEspjHs7+G+FHXjxb4rCcNLqaybbhZESik=14_9Q5h2HMzMA@mail.gmail.com>
On Tue, Dec 11 2018, Carlo Arenas wrote:
> On Mon, Dec 10, 2018 at 12:24 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> On Mon, Dec 10 2018, brian m. carlson wrote:
>> > Considering that some Linux users use PaX kernels with standard
>> > distributions and that most BSD kernels can be custom-compiled with a
>> > variety of options enabled or disabled, I think this is something we
>> > should detect dynamically.
>>
>> Right. I'm asking whether we're mixing up cases where it can always be
>> detected at compile-time on some systems v.s. cases where it'll
>> potentially change at runtime.
>
> the closer we come to a system specific issues is with macOS where the
> compiler (in some newer versions) is allocating the memory using the
> MAP_JIT flag, which seems was originally meant to be only used in iOS
> and has the strange characteristic of failing the mmap for versions
> older than 10.14 if it was called more than once.
>
> IMHO as brian pointed out, this is better done at runtime.
Sure. Just something I was wondering since it wasn't clear from the
patch. Makes sense, if it's always runtime (or not worth the effort to
divide the two) let's do that.
>> >> I'm inclined to suggest that we should have another ifdef here for "if
>> >> JIT fails I'd like it to die", so that e.g. packages I build (for
>> >> internal use) don't silently slow down in the future, only for me to
>> >> find some months later that someone enabled an overzealous SELinux
>> >> policy and we swept this under the rug.
>> >
>> > My view is that JIT is a nice performance optimization, but it's
>> > optional. I honestly don't think it should even be exposed through the
>> > API: if it works, then things are faster, and if it doesn't, then
>> > they're not. I don't see the value in an option for causing things to be
>> > broken if someone improves the security of the system.
>>
>> For many users that's definitely the case, but for others that's like
>> saying a RDBMS is still going to be functional if the "ORDER BY"
>> function degrades to bubblesort. The JIT improves performance my
>> multi-hundred percents sometimes, so some users (e.g. me) rely on that
>> not being silently degraded.
>
> the opposite is also true, specially considering that some old
> versions of pcre result in a segfault instead of an error message and
> therefore since there is no way to disable JIT, the only option left
> is not to use `git grep -P` (or the equivalent git log call)
Right, of course it segfaulting is a bug...
>> So I'm wondering if we can have something like:
>>
>> if (!jit)
>> if (must_have_jit)
>> BUG(...); // Like currently
>> else
>> fallback(); // new behavior
>
> I am wondering if something like a `git doctor` command might be an
> interesting alternative to this.
>
> This way we could (for ex: in NetBSD) give the user a hint of what to
> do to make their git grep -P faster when we detect we are running the
> fallback, and might be useful as well to provide hints for
> optimizations that could be used in other cases (probably even
> depending on the size of the git repository)
Such a command has been discussed before on-list. I think it's a good
idea for the more fuzzy things like optimization suggests, but for the
case of expecting something at compile-time where not having that at
runtime is a boolean state it's nicer to just die with a BUG(...).
> For your use case, you just need to add a crontab that will trigger an
> alarm if this command ever mentions PCRE
...The reason I'd like it to die is because it neatly and naturally
integrates with all existing test infrastructure I have. I.e. build
package, run stress tests on all sorts of machines, see that it passes,
and SELinux isn't ruining it or whatever.
I know this works now (I always get PCRE v2 JIT) because it doesn't die
or segfault. I'd like not to have it regress to having worse
performance.
Having a cronjob to test for "does PCRE v2 JIT still work?" is not as
easy & isn't a drop-in solution.
^ 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