* [PATCH v5 3/3] connect: advertized capability is not a ref
From: Jonathan Tan @ 2016-09-09 20:17 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, jrnieder, gitster
In-Reply-To: <cover.1473452153.git.jonathantanmy@google.com>
When cloning an empty repository served by standard git, "git clone" produces
the following reassuring message:
$ git clone git://localhost/tmp/empty
Cloning into 'empty'...
warning: You appear to have cloned an empty repository.
Checking connectivity... done.
Meanwhile when cloning an empty repository served by JGit, the output is more
haphazard:
$ git clone git://localhost/tmp/empty
Cloning into 'empty'...
Checking connectivity... done.
warning: remote HEAD refers to nonexistent ref, unable to checkout.
This is a common command to run immediately after creating a remote repository
as preparation for adding content to populate it and pushing. The warning is
confusing and needlessly worrying.
The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities
with no refs in upload service., 2013-08-08), JGit's ref advertisement includes
a ref named capabilities^{} to advertise its capabilities on (following the
specification in pack-protocol.txt), while git's ref advertisement is empty in
this case. This allows the client to learn about the server's capabilities and
is needed, for example, for fetch-by-sha1 to work when no refs are advertised.
This also affects "ls-remote". For example, against an empty repository served
by JGit:
$ git ls-remote git://localhost/tmp/empty
0000000000000000000000000000000000000000 capabilities^{}
Git advertises the same capabilities^{} ref in its ref advertisement for push
but since it never did so for fetch, the client didn't need to handle this
case. Handle it.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
connect.c | 14 ++++++++++++++
t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+)
diff --git a/connect.c b/connect.c
index 0c01a49..7224b5e 100644
--- a/connect.c
+++ b/connect.c
@@ -123,6 +123,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
* response does not necessarily mean an ACL problem, though.
*/
int saw_response;
+ int got_dummy_ref_with_capabilities_declaration = 0;
*list = NULL;
for (saw_response = 0; ; saw_response = 1) {
@@ -172,8 +173,21 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
continue;
}
+ if (!strcmp(name, "capabilities^{}")) {
+ if (saw_response)
+ die("protocol error: unexpected capabilities^{}");
+ if (got_dummy_ref_with_capabilities_declaration)
+ die("protocol error: multiple capabilities^{}");
+ got_dummy_ref_with_capabilities_declaration = 1;
+ continue;
+ }
+
if (!check_ref(name, flags))
continue;
+
+ if (got_dummy_ref_with_capabilities_declaration)
+ die("protocol error: unexpected ref after capabilities^{}");
+
ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
oidcpy(&ref->old_oid, &old_oid);
*list = ref;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 819b9dd..befdfee 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -207,5 +207,45 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
test_cmp expect actual
'
+test_lazy_prereq GIT_DAEMON '
+ test_tristate GIT_TEST_GIT_DAEMON &&
+ test "$GIT_TEST_GIT_DAEMON" != false
+'
+
+# This test spawns a daemon, so run it only if the user would be OK with
+# testing with git-daemon.
+test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
+ JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}} &&
+ JGIT_DAEMON_PID= &&
+ git init --bare empty.git &&
+ >empty.git/git-daemon-export-ok &&
+ mkfifo jgit_daemon_output &&
+ {
+ jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output &
+ JGIT_DAEMON_PID=$!
+ } &&
+ test_when_finished kill "$JGIT_DAEMON_PID" &&
+ {
+ read line &&
+ case $line in
+ Exporting*)
+ ;;
+ *)
+ echo "Expected: Exporting" &&
+ false;;
+ esac &&
+ read line &&
+ case $line in
+ "Listening on"*)
+ ;;
+ *)
+ echo "Expected: Listening on" &&
+ false;;
+ esac
+ } <jgit_daemon_output &&
+ # --exit-code asks the command to exit with 2 when no
+ # matching refs are found.
+ test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
+'
test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v5 1/3] tests: move test_lazy_prereq JGIT to test-lib.sh
From: Jonathan Tan @ 2016-09-09 20:17 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, jrnieder, gitster
In-Reply-To: <cover.1473452153.git.jonathantanmy@google.com>
This enables JGIT to be used as a prereq in invocations of
test_expect_success (and other functions) in other test scripts.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
t/t5310-pack-bitmaps.sh | 4 ----
t/test-lib.sh | 4 ++++
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3893afd..1e376ea 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '
git pack-objects --stdout --revs <revs >/dev/null
'
-test_lazy_prereq JGIT '
- type jgit
-'
-
test_expect_success JGIT 'we can read jgit bitmaps' '
git clone . compat-jgit &&
(
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d731d66..c9c1037 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '
test "$uid" != 0
'
+test_lazy_prereq JGIT '
+ type jgit
+'
+
# SANITY is about "can you correctly predict what the filesystem would
# do by only looking at the permission bits of the files and
# directories?" A typical example of !SANITY is running the test
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v5 0/3] handle empty spec-compliant remote repos correctly
From: Jonathan Tan @ 2016-09-09 20:17 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, jrnieder, gitster
In-Reply-To: <cover.1472836026.git.jonathantanmy@google.com>
Updates from PATCH v4:
o use fatal errors ("die") instead of warnings for protocol errors
o reworded commit message - removed last sentence and wrote "(following the
specification in pack-protocol.txt)" in one of the earlier paragraphs
Jonathan Nieder (1):
connect: tighten check for unexpected early hang up
Jonathan Tan (2):
tests: move test_lazy_prereq JGIT to test-lib.sh
connect: advertized capability is not a ref
connect.c | 32 ++++++++++++++++++++++++++------
t/t5310-pack-bitmaps.sh | 4 ----
t/t5512-ls-remote.sh | 40 ++++++++++++++++++++++++++++++++++++++++
t/test-lib.sh | 4 ++++
4 files changed, 70 insertions(+), 10 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* [PATCH v5 2/3] connect: tighten check for unexpected early hang up
From: Jonathan Tan @ 2016-09-09 20:17 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder, gitster
In-Reply-To: <cover.1473452153.git.jonathantanmy@google.com>
From: Jonathan Nieder <jrnieder@gmail.com>
A server hanging up immediately to mark access being denied does not
send any .have refs, shallow lines, or anything else before hanging
up. If the server has sent anything, then the hangup is unexpected.
That is, if the server hangs up after a shallow line but before sending
any refs, then git should tell me so:
fatal: The remote end hung up upon initial contact
instead of suggesting an access control problem:
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
Noticed while examining this code. This case isn't likely to come up
in practice but tightening the check makes the code easier to read and
manipulate.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
connect.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/connect.c b/connect.c
index 722dc3f..0c01a49 100644
--- a/connect.c
+++ b/connect.c
@@ -43,9 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)
return check_ref(ref->name, flags);
}
-static void die_initial_contact(int got_at_least_one_head)
+static void die_initial_contact(int unexpected)
{
- if (got_at_least_one_head)
+ if (unexpected)
die("The remote end hung up upon initial contact");
else
die("Could not read from remote repository.\n\n"
@@ -115,10 +115,17 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
struct sha1_array *shallow_points)
{
struct ref **orig_list = list;
- int got_at_least_one_head = 0;
+
+ /*
+ * A hang-up after seeing some response from the other end
+ * means that it is unexpected, as we know the other end is
+ * willing to talk to us. A hang-up before seeing any
+ * response does not necessarily mean an ACL problem, though.
+ */
+ int saw_response;
*list = NULL;
- for (;;) {
+ for (saw_response = 0; ; saw_response = 1) {
struct ref *ref;
struct object_id old_oid;
char *name;
@@ -131,7 +138,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
PACKET_READ_GENTLE_ON_EOF |
PACKET_READ_CHOMP_NEWLINE);
if (len < 0)
- die_initial_contact(got_at_least_one_head);
+ die_initial_contact(saw_response);
if (!len)
break;
@@ -171,7 +178,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
oidcpy(&ref->old_oid, &old_oid);
*list = ref;
list = &ref->next;
- got_at_least_one_head = 1;
}
annotate_refs_with_symref_info(*orig_list);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH v4 3/3] connect: advertized capability is not a ref
From: Junio C Hamano @ 2016-09-09 20:09 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, jrnieder, spearce, sbeller, peff
In-Reply-To: <ac55dc281e6875df8abcc6ed06d5f258a53dd251.1473441620.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> Git advertises the same capabilities^{} ref in its ref advertisement for push
> but since it never did so for fetch, the client didn't need to handle this
> case. Handle it.
>
> In this aspect, JGit is compliant with the specification in pack-protocol.txt.
The last sentence somehow looks out of place.
> + int got_dummy_ref_with_capabilities_declaration = 0;
>
> *list = NULL;
> for (saw_response = 0; ; saw_response = 1) {
> @@ -172,8 +173,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
> continue;
> }
>
> + if (!strcmp(name, "capabilities^{}")) {
> + if (saw_response)
> + warning("protocol error: unexpected capabilities^{}, "
> + "continuing anyway");
OK. saw_response tells us that we saw ".have", a valid ref, or "shallow",
but "capabilities^{}" should happen before any of them, so that is a
protocol violation. Makes perfect sense.
> + if (got_dummy_ref_with_capabilities_declaration)
> + warning("protocol error: multiple capabilities^{}, "
> + "continuing anyway");
> + got_dummy_ref_with_capabilities_declaration = 1;
> + continue;
> + }
> +
> if (!check_ref(name, flags))
> continue;
> +
> + if (got_dummy_ref_with_capabilities_declaration)
> + warning("protocol error: unexpected ref after capabilities^{}, "
> + "using this ref and continuing anyway");
Likewise. "capabilities^{}" is used when we cannot piggyback the
capability list after a real ref, so it is unusual to see a real ref
after seeing one. Makes perfect sense.
Do we want to abort the connection in these cases, I wonder, though?
Thanks.
^ permalink raw reply
* Re: [PATCH] Move format-patch base commit and prerequisites before email signature
From: Josh Triplett @ 2016-09-09 20:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqq7fakc12z.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 09, 2016 at 12:41:56PM -0700, Junio C Hamano wrote:
> So here is a suggested replacement. I notice that in the MIME case,
> we do not leave any blank line between the last line of the patch
> and the baseinfo, which makes it look a bit strange, e.g. output of
> "format-patch --attach=mimemime -1" may end like this:
>
> + test_write_lines 1 2 >expect &&
> + test_cmp expect actual
> +'
> +
> test_expect_success 'format-patch --pretty=mboxrd' '
> sp=" " &&
> cat >msg <<-INPUT_END &&
> base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b
>
> --------------mimemime--
>
> We may want to tweak it a bit further.
>
> -- >8 --
> From: Josh Triplett <josh@joshtriplett.org>
> Date: Wed, 7 Sep 2016 18:12:01 -0700
> Subject: [PATCH] format-patch: show base info before email signature
>
> Any text below the "-- " for the email signature gets treated as part of
> the signature, and many mail clients will trim it from the quoted text
> for a reply. Move it above the signature, so people can reply to it
> more easily.
>
> Similarly, when producing the patch as a MIME attachment, the
> original code placed the base info after the attached part, which
> would be discarded. Move the base info to the end of the part,
> still inside the part boundary.
>
> Add tests for the exact format of the email signature, and add tests
> to ensure that the base info appears before the email signature when
> producing a plain-text output, and that it appears before the part
> boundary when producing a MIME attachment.
>
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Looks good to me.
> builtin/log.c | 4 ++--
> t/t4014-format-patch.sh | 30 +++++++++++++++++++++++++-----
> 2 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 92dc34d..d69d5e6 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1042,7 +1042,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
> diff_flush(&opts);
>
> fprintf(rev->diffopt.file, "\n");
> - print_signature(rev->diffopt.file);
> }
>
> static const char *clean_message_id(const char *msg_id)
> @@ -1720,6 +1719,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> make_cover_letter(&rev, use_stdout,
> origin, nr, list, branch_name, quiet);
> print_bases(&bases, rev.diffopt.file);
> + print_signature(rev.diffopt.file);
> total++;
> start_number--;
> }
> @@ -1779,13 +1779,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> if (!use_stdout)
> rev.shown_one = 0;
> if (shown) {
> + print_bases(&bases, rev.diffopt.file);
> if (rev.mime_boundary)
> fprintf(rev.diffopt.file, "\n--%s%s--\n\n\n",
> mime_boundary_leader,
> rev.mime_boundary);
> else
> print_signature(rev.diffopt.file);
> - print_bases(&bases, rev.diffopt.file);
> }
> if (!use_stdout)
> fclose(rev.diffopt.file);
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index b0579dd..535857e 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -754,9 +754,22 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '
> git format-patch --ignore-if-in-upstream HEAD
> '
>
> +git_version="$(git --version | sed "s/.* //")"
> +
> +signature() {
> + printf "%s\n%s\n\n" "-- " "${1:-$git_version}"
> +}
> +
> +test_expect_success 'format-patch default signature' '
> + git format-patch --stdout -1 | tail -n 3 >output &&
> + signature >expect &&
> + test_cmp expect output
> +'
> +
> test_expect_success 'format-patch --signature' '
> - git format-patch --stdout --signature="my sig" -1 >output &&
> - grep "my sig" output
> + git format-patch --stdout --signature="my sig" -1 | tail -n 3 >output &&
> + signature "my sig" >expect &&
> + test_cmp expect output
> '
>
> test_expect_success 'format-patch with format.signature config' '
> @@ -1502,12 +1515,11 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '
>
> test_expect_success 'format-patch --base' '
> git checkout side &&
> - git format-patch --stdout --base=HEAD~3 -1 >patch &&
> - grep "^base-commit:" patch >actual &&
> - grep "^prerequisite-patch-id:" patch >>actual &&
> + git format-patch --stdout --base=HEAD~3 -1 | tail -n 6 >actual &&
> echo "base-commit: $(git rev-parse HEAD~3)" >expected &&
> echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk "{print \$1}")" >>expected &&
> echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk "{print \$1}")" >>expected &&
> + signature >> expected &&
> test_cmp expected actual
> '
>
> @@ -1605,6 +1617,14 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '
> test_cmp expected actual
> '
>
> +test_expect_success 'format-patch --base with --attach' '
> + git format-patch --attach=mimemime --stdout --base=HEAD~ -1 >patch &&
> + sed -n -e "/^base-commit:/s/.*/1/p" -e "/^---*mimemime--$/s/.*/2/p" \
> + patch >actual &&
> + test_write_lines 1 2 >expect &&
> + test_cmp expect actual
> +'
> +
> test_expect_success 'format-patch --pretty=mboxrd' '
> sp=" " &&
> cat >msg <<-INPUT_END &&
> --
> 2.10.0-339-gc0c747f
>
^ permalink raw reply
* Re: [PATCH] Move format-patch base commit and prerequisites before email signature
From: Junio C Hamano @ 2016-09-09 19:41 UTC (permalink / raw)
To: Josh Triplett, Jeff King; +Cc: git
In-Reply-To: <xmqqd1kef5k5.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> On Thu, Sep 08, 2016 at 11:54:08AM -0700, Josh Triplett wrote:
>>
>>> > your problem description
>>> > looks perfect. I am still not sure if the code does a reasonable
>>> > thing in MIME case, though.
>>>
>>> It *looks* correct to me.
>>
>> Hmm. It looks correct to me, too; ...
>> ...
>> So this is actually fixing a bug,...
>
> Yes, I actually wanted to hear that from Josh and have that in the
> proposed log message ;-).
So here is a suggested replacement. I notice that in the MIME case,
we do not leave any blank line between the last line of the patch
and the baseinfo, which makes it look a bit strange, e.g. output of
"format-patch --attach=mimemime -1" may end like this:
+ test_write_lines 1 2 >expect &&
+ test_cmp expect actual
+'
+
test_expect_success 'format-patch --pretty=mboxrd' '
sp=" " &&
cat >msg <<-INPUT_END &&
base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b
--------------mimemime--
We may want to tweak it a bit further.
-- >8 --
From: Josh Triplett <josh@joshtriplett.org>
Date: Wed, 7 Sep 2016 18:12:01 -0700
Subject: [PATCH] format-patch: show base info before email signature
Any text below the "-- " for the email signature gets treated as part of
the signature, and many mail clients will trim it from the quoted text
for a reply. Move it above the signature, so people can reply to it
more easily.
Similarly, when producing the patch as a MIME attachment, the
original code placed the base info after the attached part, which
would be discarded. Move the base info to the end of the part,
still inside the part boundary.
Add tests for the exact format of the email signature, and add tests
to ensure that the base info appears before the email signature when
producing a plain-text output, and that it appears before the part
boundary when producing a MIME attachment.
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/log.c | 4 ++--
t/t4014-format-patch.sh | 30 +++++++++++++++++++++++++-----
2 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 92dc34d..d69d5e6 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1042,7 +1042,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
diff_flush(&opts);
fprintf(rev->diffopt.file, "\n");
- print_signature(rev->diffopt.file);
}
static const char *clean_message_id(const char *msg_id)
@@ -1720,6 +1719,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
make_cover_letter(&rev, use_stdout,
origin, nr, list, branch_name, quiet);
print_bases(&bases, rev.diffopt.file);
+ print_signature(rev.diffopt.file);
total++;
start_number--;
}
@@ -1779,13 +1779,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
if (!use_stdout)
rev.shown_one = 0;
if (shown) {
+ print_bases(&bases, rev.diffopt.file);
if (rev.mime_boundary)
fprintf(rev.diffopt.file, "\n--%s%s--\n\n\n",
mime_boundary_leader,
rev.mime_boundary);
else
print_signature(rev.diffopt.file);
- print_bases(&bases, rev.diffopt.file);
}
if (!use_stdout)
fclose(rev.diffopt.file);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index b0579dd..535857e 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -754,9 +754,22 @@ test_expect_success 'format-patch --ignore-if-in-upstream HEAD' '
git format-patch --ignore-if-in-upstream HEAD
'
+git_version="$(git --version | sed "s/.* //")"
+
+signature() {
+ printf "%s\n%s\n\n" "-- " "${1:-$git_version}"
+}
+
+test_expect_success 'format-patch default signature' '
+ git format-patch --stdout -1 | tail -n 3 >output &&
+ signature >expect &&
+ test_cmp expect output
+'
+
test_expect_success 'format-patch --signature' '
- git format-patch --stdout --signature="my sig" -1 >output &&
- grep "my sig" output
+ git format-patch --stdout --signature="my sig" -1 | tail -n 3 >output &&
+ signature "my sig" >expect &&
+ test_cmp expect output
'
test_expect_success 'format-patch with format.signature config' '
@@ -1502,12 +1515,11 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '
test_expect_success 'format-patch --base' '
git checkout side &&
- git format-patch --stdout --base=HEAD~3 -1 >patch &&
- grep "^base-commit:" patch >actual &&
- grep "^prerequisite-patch-id:" patch >>actual &&
+ git format-patch --stdout --base=HEAD~3 -1 | tail -n 6 >actual &&
echo "base-commit: $(git rev-parse HEAD~3)" >expected &&
echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk "{print \$1}")" >>expected &&
echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk "{print \$1}")" >>expected &&
+ signature >> expected &&
test_cmp expected actual
'
@@ -1605,6 +1617,14 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '
test_cmp expected actual
'
+test_expect_success 'format-patch --base with --attach' '
+ git format-patch --attach=mimemime --stdout --base=HEAD~ -1 >patch &&
+ sed -n -e "/^base-commit:/s/.*/1/p" -e "/^---*mimemime--$/s/.*/2/p" \
+ patch >actual &&
+ test_write_lines 1 2 >expect &&
+ test_cmp expect actual
+'
+
test_expect_success 'format-patch --pretty=mboxrd' '
sp=" " &&
cat >msg <<-INPUT_END &&
--
2.10.0-339-gc0c747f
^ permalink raw reply related
* Re: [PATCH v4 3/3] connect: advertized capability is not a ref
From: Jonathan Nieder @ 2016-09-09 19:40 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, spearce, sbeller, gitster, peff
In-Reply-To: <ac55dc281e6875df8abcc6ed06d5f258a53dd251.1473441620.git.jonathantanmy@google.com>
Jonathan Tan wrote:
> --- a/connect.c
> +++ b/connect.c
> @@ -172,8 +173,24 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
> continue;
> }
>
> + if (!strcmp(name, "capabilities^{}")) {
> + if (saw_response)
> + warning("protocol error: unexpected capabilities^{}, "
> + "continuing anyway");
Please use die() for these.
The warning is directed at the wrong user. The end-user isn't going
to be able to fix the server. The server owner is going to say "Git
works fine --- I'll ignore this". Client authors are going to
*eventually* discover the bad server and have to work around it. So
everyone suffers.
I feel strongly about this: because there are no servers that violate
this, it should be a fatal error. If we find a server that violates
this, we should weaken the spec and make all violations of the spec
still a fatal error.
The rest looks good.
Thanks for your patience,
Jonathan
^ permalink raw reply
* Re: [PATCH 2/3] diff_flush_patch_id: stop returning error result
From: Jeff King @ 2016-09-09 19:37 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Michael Haggerty, Kevin Willford, Xiaolong Ye, Josh Triplett
In-Reply-To: <alpine.DEB.2.20.1609091455180.129229@virtualbox>
On Fri, Sep 09, 2016 at 02:58:25PM +0200, Johannes Schindelin wrote:
> > Yes, I agree that this is the opposite direction of libification. And I
> > agree that the current message is not very helpful.
> >
> > But I am not sure that returning the error up the stack will actually
> > help somebody move forward. The reason these are all die() calls in the
> > rest of the diff code is that they are generally indicative of
> > unrecoverable repository corruption. So any advice does not really
> > depend on what operation you are performing; it is always "stop what you
> > are doing immediately, run fsck, and try to get the broken objects from
> > somebody else".
> >
> > So IMHO, on balance this is not hurting anything.
>
> Well, you make such a situation even worse than it already is.
>
> It would be one thing to change the code to actually say "stop what you
> are doing immediately, run `git fsck` and try to get the broken objects
> from somewhere else", *before* saying how to proceed after that.
>
> But that is not what your patch does.
>
> What your patch does is to remove *even the possibility* of saying how to
> proceed after getting the repository corruption fixed. And instead of
> saying how the corruption could be fixed, it outputs a terse "cannot read
> files to diff".
>
> I do not think that is a wise direction.
First, do not blame me for the terse "cannot read files to diff". That
is the current message. And my patch does not make changing that message
any more difficult. You are welcome to change it in its error() form.
You are welcome to change it in the resulting die().
The quality of that message is totally orthogonal to what the patch is
doing.
The _only_ thing it is losing is the ability to for the caller to then
additionally say "once you have finished uncorrupting the repository,
you can resume your operation with ...".
My point is that this is not useful advice. No callers give it, and I
don't foresee other callers giving it. My argument above was basically
that it is such an exceptional condition it is not worth worrying about.
-Peff
^ permalink raw reply
* [PATCH v2] checkout: eliminate unnecessary merge for trivial checkout
From: Ben Peart @ 2016-09-09 19:25 UTC (permalink / raw)
To: git; +Cc: gitster, pclouds, peartben, Ben Peart
Teach git to avoid unnecessary merge during trivial checkout. When
running 'git checkout -b foo' git follows a common code path through
the expensive merge_working_tree even when it is unnecessary. As a
result, 95% of the time is spent in merge_working_tree doing the 2-way
merge between the new and old commit trees that is unneeded.
The time breakdown is as follows:
merge_working_tree <-- 95%
unpack_trees <-- 80%
traverse_trees <-- 50%
cache_tree_update <-- 17%
mark_new_skip_worktree <-- 10%
With a large repo, this cost is pronounced. Using "git checkout -b r"
to create and switch to a new branch costs 166 seconds (all times worst
case with a cold file system cache).
git.c:406 trace: built-in: git 'checkout' '-b' 'r'
read-cache.c:1667 performance: 17.442926555 s: read_index_from
name-hash.c:128 performance: 2.912145231 s: lazy_init_name_hash
read-cache.c:2208 performance: 4.387713335 s: write_locked_index
trace.c:420 performance: 166.458921289 s: git command:
'c:\Users\benpeart\bin\git.exe' 'checkout' '-b' 'r'
Switched to a new branch 'r'
By adding a test to skip the unnecessary call to merge_working_tree in
this case reduces the cost to 16 seconds.
git.c:406 trace: built-in: git 'checkout' '-b' 's'
read-cache.c:1667 performance: 16.100742476 s: read_index_from
trace.c:420 performance: 16.461547867 s: git command: 'c:\Users\benpeart\bin\git.exe' 'checkout' '-b' 's'
Switched to a new branch 's'
Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
builtin/checkout.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 95 insertions(+), 4 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8672d07..4396cb3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -38,6 +38,10 @@ struct checkout_opts {
int ignore_skipworktree;
int ignore_other_worktrees;
int show_progress;
+ /*
+ * If new checkout options are added, needs_working_tree_merge
+ * should be updated accordingly.
+ */
const char *new_branch;
const char *new_branch_force;
@@ -802,6 +806,87 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)
free(refs.objects);
}
+static int needs_working_tree_merge(const struct checkout_opts *opts,
+ const struct branch_info *old,
+ const struct branch_info *new)
+{
+ /*
+ * We must do the merge if we are actually moving to a new
+ * commit tree.
+ */
+ if (!old->commit || !new->commit ||
+ oidcmp(&old->commit->tree->object.oid, &new->commit->tree->object.oid))
+ return 1;
+
+ /*
+ * opts->patch_mode cannot be used with switching branches so is
+ * not tested here
+ */
+
+ /*
+ * opts->quiet only impacts output so doesn't require a merge
+ */
+
+ /*
+ * Honor the explicit request for a three-way merge or to throw away
+ * local changes
+ */
+ if (opts->merge || opts->force)
+ return 1;
+
+ /*
+ * Checking out the requested commit may require updating the working
+ * directory and index, let the merge handle it.
+ */
+ if (opts->force_detach)
+ return 1;
+
+ /*
+ * opts->writeout_stage cannot be used with switching branches so is
+ * not tested here
+ */
+
+ /*
+ * Honor the explicit ignore requests
+ */
+ if (!opts->overwrite_ignore || opts->ignore_skipworktree
+ || opts->ignore_other_worktrees)
+ return 1;
+
+ /*
+ * opts->show_progress only impacts output so doesn't require a merge
+ */
+
+ /*
+ * If we're not creating a new branch, by definition we're changing
+ * the existing one so need to do the merge
+ */
+ if (!opts->new_branch)
+ return 1;
+
+ /*
+ * new_branch_force is defined to "create/reset and checkout a branch"
+ * so needs to go through the merge to do the reset
+ */
+ if (opts->new_branch_force)
+ return 1;
+
+ /*
+ * A new orphaned branch requrires the index and the working tree to be
+ * adjusted to <start_point>
+ */
+ if (opts->new_orphan_branch)
+ return 1;
+
+ /*
+ * Remaining variables are not checkout options but used to track state
+ * that doesn't trigger the need for a merge.
+ */
+
+ return 0;
+}
+
+
static int switch_branches(const struct checkout_opts *opts,
struct branch_info *new)
{
@@ -827,10 +912,16 @@ static int switch_branches(const struct checkout_opts *opts,
parse_commit_or_die(new->commit);
}
- ret = merge_working_tree(opts, &old, new, &writeout_error);
- if (ret) {
- free(path_to_free);
- return ret;
+ /*
+ * Optimize the performance of "git checkout foo" by skipping the call
+ * to merge_working_tree where possible.
+ */
+ if (needs_working_tree_merge(opts, &old, new)) {
+ ret = merge_working_tree(opts, &old, new, &writeout_error);
+ if (ret) {
+ free(path_to_free);
+ return ret;
+ }
}
if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)
--
2.10.0.windows.1
^ permalink raw reply related
* Re: [PATCH 07/13] i18n: merge-recursive: mark error messages for translation
From: Junio C Hamano @ 2016-09-09 19:23 UTC (permalink / raw)
To: Vasco Almeida; +Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason
In-Reply-To: <1473259758-11836-7-git-send-email-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> Lowercase first word of such error messages following the usual style.
"Change X to lowercase" is fine, but "Lowercase" is not a verb.
I'd reword it to "Downcase the first word...".
Other than that all 04-13/13 looked fine to me.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/3] checkout.txt: document a common case that ignores ambiguation rules
From: Junio C Hamano @ 2016-09-09 19:21 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <xmqq7famgnwo.fsf@gitster.mtv.corp.google.com>
I'll queue the following as "fixup!" for now. It reminds me that
the title also needs rewording; we do not have rules to make things
ambiguous ;-)
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 46b04b1..8e2c066 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -419,14 +419,17 @@ $ git reflog -2 HEAD # or
$ git log -g -2 HEAD
------------
-ARGUMENT AMBIGUATION
---------------------
+ARGUMENT DISAMBIGUATION
+-----------------------
When there is only one argument given and it is not `--` (e.g. "git
-checkout abc"), "abc" could be seen as either a `<tree-ish>` or a
-`<pathspec>`, but Git will assume the argument is a `<tree-ish>`, which is
-a common case for switching branches. Use `git checkout -- <pathspec>`
-form if you mean it to be a pathspec.
+checkout abc"), and when the argument is both a valid `<tree-ish>`
+(e.g. a branch "abc" exists) and a valid `<pathspec>` (e.g. a file
+or a directory whose name is "abc" exists), Git would usually ask
+you to disambiguate. Because checking out a branch is so common an
+operation, however, "git checkout abc" takes "abc" as a `<tree-ish>`
+in such a situation. Use `git checkout -- <pathspec>` if you want
+to checkout these paths out of the index.
EXAMPLES
--------
--
2.10.0-339-gc0c747f
^ permalink raw reply related
* Re: [PATCH 22/22] sequencer: refactor write_message()
From: Jakub Narębski @ 2016-09-09 19:11 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609091639430.129229@virtualbox>
Hello Johannes,
W dniu 09.09.2016 o 16:40, Johannes Schindelin napisał:
> On Fri, 2 Sep 2016, Jakub Narębski wrote:
>> W dniu 01.09.2016 o 16:20, Johannes Schindelin pisze:
>>> On Thu, 1 Sep 2016, Jakub Narębski wrote:
>>>> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
>>
>>>>> if (commit_lock_file(&msg_file) < 0)
>>>>> return error(_("Error wrapping up %s."), filename);
>>>>
>>>> Another "while at it"... though the one that can be safely postponed
>>>> (well, the make message easier to understand part, not the quote
>>>> filename part):
>>>>
>>>> return error(_("Error wrapping up writing to '%s'."), filename);
>>>
>>> As I inherited this message, I'll keep it.
>>
>> Well, please then add quotes while at it, at least, for consistency
>>
>> return error(_("Error wrapping up '%s'."), filename);
>
> I may do that as a final patch, once all the other concerns are addressed.
> I really do not want to change the error message during the conversion.
Is not wanting to change error messages during conversion because of
your use of Scientist tool to catch errors in conversion process?, as
you wrote in
https://blogs.msdn.microsoft.com/visualstudioalm/2016/09/03/whats-new-in-git-for-windows-2-10/
BTW. could you tell us what were those three regression caught by the
cross-validation?
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 13/22] sequencer: remember the onelines when parsing the todo file
From: Jakub Narębski @ 2016-09-09 19:06 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1609091708480.129229@virtualbox>
Hello Johannes,
W dniu 09.09.2016 o 17:12, Johannes Schindelin napisał:
> On Thu, 1 Sep 2016, Junio C Hamano wrote:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> I was sort of expecting that, when you do the preserve-merges mode
>> of "rebase -i", you would need to jump around, doing "we have
>> reconstructed the side branch on a new 'onto', let's give the result
>> this temporary name ':1', and then switch to the trunk (which would
>> call for 'reset <commit>' instruction) and merge that thing (which
>> would be 'merge :1' or perhaps called 'pick :1')", and at that point
>> you no longer validate the object references upfront.
>
> Except that is not how --preserve-merges works: it *still* uses the SHA-1s
> as identifiers, even when the SHA-1 may have changed in the meantime.
>
> That is part of why it was a bad design.
When preserving merges, there are (as far as I understand it), two
problems:
- what it means to preserve changes (which change to pick,
that is what is the mainline changes rebase is re-applying)
- what are parents of the merge commit (at least one parent
would be usually rewritten)
Maybe the internal (and perhaps also user-visible) representation
of merge in instruction sheet could use the notation of filter-branch,
that is 'map(<sha-1>)'... it could also imply the mainline.
That is the instruction in the internal instruction sheet could
look like this:
merge -m 1 map(2fd4e1c67a2d28fced849ee1bb76e7391b93eb12) da39a3ee5e6b4b0d3255bfef95601890afd80709 \t Merge 'foo' into master
Note that it has nothing to do with this series!
Best regards,
--
Jakub Narębski
^ permalink raw reply
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"
From: Junio C Hamano @ 2016-09-09 19:00 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, Dakota Hawkins, git
In-Reply-To: <xmqq7faldpgp.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> On Thu, 11 Aug 2016, Junio C Hamano wrote:
>>>
>>> Do you have a preference either way to help us decide if we want to
>>> take this change or not?
>>
>> I have no strong preference. I guess that it does not hurt to go with the
>> patch, and it would probably help in a few cases.
>
> OK. Let me dig the change back and how well it still fits ;-)
Ah, I already had it in my tree lest I forget. Let me mark it for
merging down to 'master'.
Thanks.
^ permalink raw reply
* Re: Announcing Git User's Survey 2016 [was: Working with public-inbox.org]
From: Jakub Narębski @ 2016-09-09 18:51 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Duy Nguyen, Jeff King, Stefan Beller, meta, git@vger.kernel.org,
Eric Wong
In-Reply-To: <alpine.DEB.2.20.1609091503410.129229@virtualbox>
Hello Johannes,
W dniu 09.09.2016 o 15:06, Johannes Schindelin napisał:
> On Sun, 28 Aug 2016, Jakub Narębski wrote:
>> W dniu 28.08.2016 o 10:38, Johannes Schindelin pisze:
>>
>>> I would like to strongly caution against putting too much stock into
>>> this users' survey. It is the best we have, granted. Yet I have not
>>> heard from anybody that they participated in the survey, unless they
>>> were also subscribed to the Git mailing list.
>>
>> I tried in past and will try for this year Git User's Survey to be
>> announced more widely than just Git mailing list (git@vger.kernel.org).
>
> I did not mean to criticise you. I think you are doing the best you can,
> and it is valuable.
[...]
>
> The question is not so much how to advertise the survey. I skip almost all
> surveys I am asked to participate in, because I am just a little bit busy
> all the time. I feel that my colleagues do the same. Unless forced to take
> a survey, they skip it.
Right, that's a problem. Thanks for reminding me.
I hope that the fact that by default (via the use of cookies) you can
return to Survs.com survey at later time (assuming that you do it from the
same computer and the same web browser), and continue responding. Taking
30 minutes or more at once may be a problem, taking 10 x 3 minutes may
not be.
But I won't have too much hope...
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH] git-gui: respect commit.gpgsign again
From: Junio C Hamano @ 2016-09-09 18:29 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Pat Thoyts
In-Reply-To: <550e7de5e08e53b5893e1ce021bffee468adfc9d.1473424091.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> As of v2.9.0, `git commit-tree` no longer heeds the `commit.gpgsign`
> config setting. This broke committing in Git GUI.
Thanks. Will shift it up to apply to my copy of git-gui project and
then pull in the result.
> git-gui/lib/commit.tcl | 3 +++
> 1 file changed, 3 insertions(+)
^ permalink raw reply
* Re: [PATCH v3 17/17] sequencer: ensure to release the lock when we could not read the index
From: Junio C Hamano @ 2016-09-09 18:26 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Eric Sunshine
In-Reply-To: <4c7876d88eac80d9cf05847477d4d468a7c01acc.1473431645.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> A future caller of read_and_refresh_cache() may want to do more than just
> print some helpful advice in case of failure.
I recall commenting on unreleased locks on other parts of the series
but didn't see this. Looks good. Thanks for being thorough.
> diff --git a/sequencer.c b/sequencer.c
> index d92a632..eec8a60 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -644,14 +644,18 @@ static int read_and_refresh_cache(struct replay_opts *opts)
> {
> static struct lock_file index_lock;
> int index_fd = hold_locked_index(&index_lock, 0);
> - if (read_index_preload(&the_index, NULL) < 0)
> + if (read_index_preload(&the_index, NULL) < 0) {
> + rollback_lock_file(&index_lock);
> return error(_("git %s: failed to read the index"),
> action_name(opts));
> + }
> refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
> if (the_index.cache_changed && index_fd >= 0) {
> - if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
> + if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {
> + rollback_lock_file(&index_lock);
> return error(_("git %s: failed to refresh the index"),
> action_name(opts));
> + }
> }
> rollback_lock_file(&index_lock);
> return 0;
^ permalink raw reply
* Re: [PATCH v3 16/17] lib'ify checkout_fast_forward_to()
From: Junio C Hamano @ 2016-09-09 18:23 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Eric Sunshine
In-Reply-To: <f624be21280cb2f7624ed7bb4d890903cccf1c7d.1473431645.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Instead of dying there, let the caller high up in the callchain
> notice the error and handle it (by dying, still).
>
> The only callers of checkout_fast_forward_to(), cmd_merge(),
> pull_into_void(), cmd_pull() and sequencer's fast_forward_to(),
> already check the return value and handle it appropriately. With this
> step, we make it notice an error return from this function.
>
> So this is a safe conversion to make checkout_fast_forward_to()
> callable from new callers that want it not to die, without changing
> the external behaviour of anything existing.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
I'll retitle this to
sequencer: lib'ify chckout_fast_forward()
and checkout_fast_forward_to() in the second paragraph to match the
reality. Other than that, the above analysis matches what I see in
the code and the libification done here looks correct.
Thanks.
> merge.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/merge.c b/merge.c
> index 5db7d56..23866c9 100644
> --- a/merge.c
> +++ b/merge.c
> @@ -57,7 +57,8 @@ int checkout_fast_forward(const unsigned char *head,
>
> refresh_cache(REFRESH_QUIET);
>
> - hold_locked_index(lock_file, 1);
> + if (hold_locked_index(lock_file, 0) < 0)
> + return -1;
>
> memset(&trees, 0, sizeof(trees));
> memset(&opts, 0, sizeof(opts));
> @@ -90,7 +91,9 @@ int checkout_fast_forward(const unsigned char *head,
> }
> if (unpack_trees(nr_trees, t, &opts))
> return -1;
> - if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
> - die(_("unable to write new index file"));
> + if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) {
> + rollback_lock_file(lock_file);
> + return error(_("unable to write new index file"));
> + }
> return 0;
> }
^ permalink raw reply
* Re: [PATCH v3 3/4] cat-file --textconv/--filters: allow specifying the path separately
From: Junio C Hamano @ 2016-09-09 18:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Torsten Bögershausen, Jeff King
In-Reply-To: <9e01395c0732b47b382d2363a2ef6f4bf361666e.1473415827.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> +test_expect_success '----path=<path> complains without --textconv/--filters' '
I wonder where this "----path" came from; it wasn't in v2 I queued,
but somehow came back mysteriously.
Will locally amend.
> + sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
> + test_must_fail git cat-file --path=hello.txt blob $sha1 >actual 2>err &&
> + test ! -s actual &&
> + grep "path.*needs.*filters" err
> +'
> +
> test_done
^ permalink raw reply
* Re: git commit -p with file arguments
From: Junio C Hamano @ 2016-09-09 18:03 UTC (permalink / raw)
To: Jacob Keller; +Cc: Christian Neukirchen, Git mailing list
In-Reply-To: <CA+P7+xoN+q_Kst=qXG_HRznxbN7cbyi5uZe15zq1c16EifeK1Q@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> It wants to commit bar too because you already added bar before. It works like:
>
> "git add bar && git add -p foo && git commit" does it not?
>
> I fail to see why "git commit -p <path>" would unstage the bar you
> already added? Or am I missing some assumption here?
Yes.
"git commit -p <pathspec>" were added originally for lazy people who
do not want to type "git add -p <pathspec> && git commit", which
matches your expectation. If you already added "bar" that is
outside of the <pathspec> given to "add -p", the final "git commit"
step would record the latest contents of "bar" in it.
For obvious reasons, "git commit -p <pathspec>" cannot be a
short-hand to "git add -p <pathspec> && git commit <pathspec>", so
the current behaviour was the best they could do for those who aded
"commit -p", I guess.
^ permalink raw reply
* Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function
From: Junio C Hamano @ 2016-09-09 17:49 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King
In-Reply-To: <alpine.DEB.2.20.1609091151510.129229@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Also I agree with Peff that a test with an embedded NUL would be a
>> good thing.
>
> This is something I will leave to somebody else, as it was not my
> intention to fix this and I *really* have more pressing things to do right
> now... Sorry!
As I said a few minutes ago, I think we can stop _before_ worrying
about an embedded NUL, which is something we haven't handled before
anyway so it is a new feature that can be built later outside the
scope of this series.
^ permalink raw reply
* Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Junio C Hamano @ 2016-09-09 17:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, git
In-Reply-To: <alpine.DEB.2.20.1609091158550.129229@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Besides avoiding a segfault, one of the benefits of regcomp_buf() is
>> that we will now find pickaxe-regex strings inside mixed binary/text
>> files. But it's not clear to me that NetBSD's implementation does this.
>>
>> I guess we can assume it is fine (it is certainly no _worse_ than the
>> current behavior), and if people's platforms do not handle it, they can
>> build with NO_REGEX.
>
> René mentioned in f96e567 (grep: use REG_STARTEND for all matching if
> available, 2010-05-22) something along the lines of REG_STARTEND being
> able to parse beyond NULs. My interpretation of NetBSD's documentation
> agrees with your interpretation, though, that the buffers are still
> thought of as being NUL-terminated, even if rm_eo makes the code *not*
> look at that particular NUL.
>
> Be that as it may: it is completely outside the purpose of my patch series
> to take care of making it possible for Git's regex functions to match
> buffers with embedded NULs.
I think you two have agreed that regexec_buf() wrapper that always
relies on REG_STARTEND is a good first step whether we want to do an
embedded NUL, that we just need that good first step for now, and
that that good first step would not block future progress, i.e. our
wanting to handle embedded NUL.
So let's see how well the first step flies in practice. We tell
people to build with NO_REGEX if their platform's regexp does not do
(any form of) REG_STARTEND. We _might_ later have to tell them to
set NO_REGX even if they are on NetBSD and has REG_STARTEND that may
not handle embedded NUL the way we want, but that can safely be left
to the future.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/3] t0001: work around the bug that reads config file before repo setup
From: Jacob Keller @ 2016-09-09 17:45 UTC (permalink / raw)
To: Jeff King; +Cc: Duy Nguyen, Git Mailing List, Michael J Gruber, Max Nordlund
In-Reply-To: <20160909112217.gfalcbtiy2tzzhm5@sigill.intra.peff.net>
On Fri, Sep 9, 2016 at 4:22 AM, Jeff King <peff@peff.net> wrote:
> If you're curious what the fix looks like, it's in:
>
> https://github.com/peff/git jk/config-repo-setup
>
> The actual fix is in the final patch, but it needed a lot of preparatory
> work to avoid breaking various programs that made bad assumptions (and
> in the process, I uncovered a ton of other minor bugs).
>
> This is just a preview in case you're interested, for two reasons:
>
> 1. I literally _just_ put the finishing touches on it, and it's
> extensive and tricky enough that I really should give it one more
> proofread.
>
> 2. There may be other related fallouts from the bug related to running
> "git init /path/to/foo" when "/path/to/foo" already exists (and in
> that case we _do_ want to read its config, but not the config from
> an existing repository). This may all just work fine, but I need to
> think about some tests.
>
> -Peff
I looked over the series and it all seems like solid improvements to
me, and I agree with the reasoning and logic. It's nice to also read
very descriptive commit messages that clearly explain each
improvement.
Regards,
Jake
^ permalink raw reply
* [PATCH v4 2/3] connect: tighten check for unexpected early hang up
From: Jonathan Tan @ 2016-09-09 17:36 UTC (permalink / raw)
To: git; +Cc: Jonathan Nieder, spearce, sbeller, gitster, peff
In-Reply-To: <cover.1473441620.git.jonathantanmy@google.com>
From: Jonathan Nieder <jrnieder@gmail.com>
A server hanging up immediately to mark access being denied does not
send any .have refs, shallow lines, or anything else before hanging
up. If the server has sent anything, then the hangup is unexpected.
That is, if the server hangs up after a shallow line but before sending
any refs, then git should tell me so:
fatal: The remote end hung up upon initial contact
instead of suggesting an access control problem:
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
Noticed while examining this code. This case isn't likely to come up
in practice but tightening the check makes the code easier to read and
manipulate.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
connect.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/connect.c b/connect.c
index 722dc3f..0c01a49 100644
--- a/connect.c
+++ b/connect.c
@@ -43,9 +43,9 @@ int check_ref_type(const struct ref *ref, int flags)
return check_ref(ref->name, flags);
}
-static void die_initial_contact(int got_at_least_one_head)
+static void die_initial_contact(int unexpected)
{
- if (got_at_least_one_head)
+ if (unexpected)
die("The remote end hung up upon initial contact");
else
die("Could not read from remote repository.\n\n"
@@ -115,10 +115,17 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
struct sha1_array *shallow_points)
{
struct ref **orig_list = list;
- int got_at_least_one_head = 0;
+
+ /*
+ * A hang-up after seeing some response from the other end
+ * means that it is unexpected, as we know the other end is
+ * willing to talk to us. A hang-up before seeing any
+ * response does not necessarily mean an ACL problem, though.
+ */
+ int saw_response;
*list = NULL;
- for (;;) {
+ for (saw_response = 0; ; saw_response = 1) {
struct ref *ref;
struct object_id old_oid;
char *name;
@@ -131,7 +138,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
PACKET_READ_GENTLE_ON_EOF |
PACKET_READ_CHOMP_NEWLINE);
if (len < 0)
- die_initial_contact(got_at_least_one_head);
+ die_initial_contact(saw_response);
if (!len)
break;
@@ -171,7 +178,6 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
oidcpy(&ref->old_oid, &old_oid);
*list = ref;
list = &ref->next;
- got_at_least_one_head = 1;
}
annotate_refs_with_symref_info(*orig_list);
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox