* Re: [PATCH 1/3] serve: pass "config context" through to individual commands
From: Jeff King @ 2018-12-14 9:55 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Brandon Williams, Jonathan Tan
In-Reply-To: <20181214092820.GB7121@google.com>
On Fri, Dec 14, 2018 at 01:28:20AM -0800, Jonathan Nieder wrote:
> > Certainly if that information was carried from the client request it
> > would work fine, and ls-refs would have enough to know which config to
> > respect. But I could not find any documentation on this, nor discussion
> > of plans for a v2 push.
>
> Interesting. The last discussion of push v2 plans was in
> https://public-inbox.org/git/20180717210915.139521-1-bmwill@google.com/.
> Expect to hear more soon.
The words "ls-refs" and "advertisement" are notably absent from that
thread. ;)
> > I'm conceptually OK with that, but if that is the plan for going
> > forward, it was not at all obvious to me (and it does feel rather
> > implicit).
>
> Don't get me wrong: I haven't wrapped my head around config context
> and how it fits into the broader picture yet, but it may be a very
> good thing to have. So please consider this comment to be about the
> commit message only.
If we're OK with accepting that the client will pass along the
fetch/push context for each individual command, then I don't think we
would ever need this. It is literally about relaying the fact of "the
original request was via upload-pack". If the commands already know the
context the client is interested in from another method, then I don't
think they should ever need to care about that fact.
> Based on the motivation you're describing here, I think treating it as
> uploadpack and adding a NEEDSWORK comment would be a good way forward.
> If we're moving toward a world with more protocol commands that don't
> fit in the upload-pack / receive-pack categories, then we need to
> figure out in more detail what that world looks like:
>
> - do we keep on adding new endpoints, in the same spirit as
> upload-archive? If so, what endpoint should a client use to get
> capabilities before it decides which endpoint to use?
>
> - do we merge everything in "git serve" except where a specific
> endpoint is needed for protocol v0 compatibility? That would lose
> the ability to distinguish fetches from pushes without looking at
> the body of requests (which is useful to some people for monitoring,
> blocking, etc) --- do we consider that to be an acceptable loss?
>
> - once we've decided what the future should look like, how does the
> transition to that future look?
I agree those are all interesting open questions. I didn't want to solve
any of them now, but just fix this (IMHO pretty serious) regression. I
was mostly trying to do so without making any assumptions about where
we'd go in the future (and even NEEDSWORK feels a little funny; it's not
clear to me whether that work is going to be needed or not).
> > Without this information, in patch 3 ls-refs cannot know to look at
> > uploadpack.hiderefs, unless it makes the implicit assumption that it is
> > always serving a fetch.
>
> I think that's a reasonable assumption to make, especially if made
> explicit using a simple comment. :)
The big danger is that somebody does implement a "push" command and
forgets to touch ls-refs. That would be wrong and buggy for more reasons
than this (as you noted earlier, it should handle .have refs somehow).
But what worries me is that the failure mode for the bug is to start
exposing refs which are meant to be hidden. Which to me is a little more
serious than "the new functionality doesn't work".
So I guess I considered it to mostly be defensive (and I'd be fine if it
was later ripped out when a more elegant approach becomes obvious).
That said, I'm not totally opposed to the implicit thing if that's where
we all think the protocol code should be headed. The patch is certainly
smaller. The whole series could be replaced with this:
-- >8 --
Subject: [PATCH] upload-pack: support hidden refs with protocol v2
In the v2 protocol, upload-pack's advertisement has been moved to the
"ls-refs" command. That command does not respect hidden-ref config (like
transfer.hiderefs) at all, and advertises everything.
While there are some features that are not supported in v2 (e.g., v2
always allows fetching any sha1 without respect to advertisements), the
lack of this feature is not documented and is likely just a bug. Let's
make it work, as otherwise upgrading a server to a v2-capable git will
start exposing these refs that the repository admin has asked to remain
hidden.
Note that we assume we're operating on behalf of a fetch here, since
that's the only thing implemented in v2 at this point. See the in-code
comment.
Signed-off-by: Jeff King <peff@peff.net>
---
ls-refs.c | 16 ++++++++++++++++
t/t5512-ls-remote.sh | 6 ++++++
2 files changed, 22 insertions(+)
diff --git a/ls-refs.c b/ls-refs.c
index a06f12eca8..9c9a7c647f 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -5,6 +5,7 @@
#include "argv-array.h"
#include "ls-refs.h"
#include "pkt-line.h"
+#include "config.h"
/*
* Check if one of the prefixes is a prefix of the ref.
@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
const char *refname_nons = strip_namespace(refname);
struct strbuf refline = STRBUF_INIT;
+ if (ref_is_hidden(refname_nons, refname))
+ return 0;
+
if (!ref_match(&data->prefixes, refname))
return 0;
@@ -69,6 +73,16 @@ static int send_ref(const char *refname, const struct object_id *oid,
return 0;
}
+static int ls_refs_config(const char *var, const char *value, void *data)
+{
+ /*
+ * We only serve fetches over v2 for now, so respect only "uploadpack"
+ * config. This may need to eventually be expanded to "receive", but we
+ * don't yet know how that information will be passed to ls-refs.
+ */
+ return parse_hide_refs_config(var, value, "uploadpack");
+}
+
int ls_refs(struct repository *r, struct argv_array *keys,
struct packet_reader *request)
{
@@ -76,6 +90,8 @@ int ls_refs(struct repository *r, struct argv_array *keys,
memset(&data, 0, sizeof(data));
+ git_config(ls_refs_config, NULL);
+
while (packet_reader_read(request) != PACKET_READ_FLUSH) {
const char *arg = request->line;
const char *out;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2e..ca69636fd5 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
grep refs/tags/magic actual
'
+test_expect_success 'protocol v2 supports hiderefs' '
+ test_config uploadpack.hiderefs refs/tags &&
+ git -c protocol.version=2 ls-remote . >actual &&
+ ! grep refs/tags actual
+'
+
test_expect_success 'ls-remote --symref' '
git fetch origin &&
cat >expect <<-EOF &&
--
2.20.0.738.gdb22cab611
^ permalink raw reply related
* Re: [Question] Complex textconv text
From: Jeff King @ 2018-12-14 10:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Randall S. Becker, git
In-Reply-To: <xmqqo99odcou.fsf@gitster-ct.c.googlers.com>
On Fri, Dec 14, 2018 at 12:12:01PM +0900, Junio C Hamano wrote:
> > I have a strange situation and need help with resolving funky characters in
> > .git/config. My situation is this:
> >
> > [diff "*.dat"]
> > textconv = enscribe-conv
> > --format=-a1\(A=-a1,-a16,-a32\|P=-a1,-a32,-a16\|=-a1,-d,a14\),-a224
> >
> > Basically this is a formatter for diff so that I can show structured binary
> > data. The unquoted syntax of the format string is:
> > --format=-a1(A=-a1,-a16,-a32|P=-a1,-a32,-a16|=-a1,-d,a14),-a224
> >
> > Content is not really important. The issue is that git is reporting fatal:
> > bad config line 2 in file .git/config when I escape the (, ), and |
> > characters.
>
> That failure is understandable, as
>
> The following escape sequences (beside `\"` and `\\`) are recognized:
> `\n` for newline character (NL), `\t` for horizontal tabulation (HT, TAB)
> and `\b` for backspace (BS). Other char escape sequences (including octal
> escape sequences) are invalid.
>
> is what Documentation/config.txt says. \(, \) and \| is not a way
> to escape these letters from the .git/config parser (they do not
> need to be escaped from .git/config parser)..
Yes. It's a little unfriendly that we don't pass them through, since
that would do what Randall wants here. But doing so would be unfriendly
to somebody who _did_ want to quote them, and is confused why there are
passed through literally.
At any rate, the "right" way here is to quote the backslashes that are
meant to be passed through to the shell, like:
textconv = foo \\(X\\|\\)
Note that if you write the config using git-config, that's what it
produces:
$ git config -f file foo.bar '\(X\|Y\)'
$ cat file
[foo]
bar = \\(X\\|Y\\)
I agree that your double-quote suggestion is more readable (or as
Randall eventually figured out, hiding it all in a real script ;) ).
-Peff
^ permalink raw reply
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
From: Jeff King @ 2018-12-14 10:12 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Brandon Williams, Jonathan Tan
In-Reply-To: <87pnu51kac.fsf@evledraar.gmail.com>
On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Now that we have this maybe we should discuss why these tests show
> different things:
>
> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> > index 086f2c40f6..8b1217ea26 100755
> > --- a/t/t5500-fetch-pack.sh
> > +++ b/t/t5500-fetch-pack.sh
> > @@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
> > test_commit -C server 6 &&
> >
> > git init client &&
> > - test_must_fail git -C client fetch-pack ../server \
> > +
> > + # Other protocol versions (e.g. 2) allow fetching an unadvertised
> > + # object, so run this test with the default protocol version (0).
> > + test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \
> > $(git -C server rev-parse refs/heads/master^) 2>err &&
>
> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
> v2 all the time?
Yeah, I actually didn't realize it until working on the earlier series,
but this is the documented behavior:
$ git grep -hC3 'want <oid>' Documentation/technical/protocol-v2.txt
A `fetch` request can take the following arguments:
want <oid>
Indicates to the server an object which the client wants to
retrieve. Wants can be anything and are not limited to
advertised objects.
An interesting implication of this at GitHub (and possibly other
hosters) is that it exposes objects from shared storage via unexpected
repos. If I fork torvalds/linux to peff/linux and push up object X, a v0
fetch will (by default) refuse to serve it to me. But v2 will happily
hand it over, which may confuse people into thinking that the object is
(or at least at some point was) in Linus's repository.
-Peff
^ permalink raw reply
* git-apply working on an index with smudged entries
From: Christian Halstrick @ 2018-12-14 10:15 UTC (permalink / raw)
To: Git; +Cc: thomas.wolf
I see that when I call "git apply --3way ..." on an index which was
previously created
by JGit and which contains smudged entries the command fails with
message "error: foo.txt:
does not match index". If I do a "git status" afterwards and then the
same "git apply --3way ..." it
succeeds. Looks like "git status" corrected the index in a way so "git
apply" can accept it.
Question:
- is this because the index which jgit created is so corrupt that only
special commands
like "git status" can repair it?
- or is "git apply" not trying hard enough to consume a index with
smudged entries
Here is the trace of a script from Thomas which shows the effect:
> git --version
git version 2.19.1
> jgit --version
jgit version 5.2.0-SNAPSHOT
> git init
Initialized empty Git repository in /Users/d032780/tmp/a/.git/
> echo "foo" > foo.txt
> git add foo.txt
> git commit -m "Initial commit"
[master (root-commit) ff6c56c] Initial commit
1 file changed, 1 insertion(+)
create mode 100644 foo.txt
> echo "bar" >> foo.txt
> git add foo.txt
> git commit -m "Second commit"
[master 2191919] Second commit
1 file changed, 1 insertion(+)
> echo "baz" >> foo.txt
> git add foo.txt
> git commit -m "Third commit"
[master d863c4a] Third commit
1 file changed, 1 insertion(+)
> git diff HEAD^1..HEAD foo.txt > ../foo.patch
> jgit reset --hard HEAD~
> git apply --3way ../foo.patch
error: foo.txt: does not match index
> git ls-files -sv --debug
H 100644 3bd1f0e29744a1f32b08d5650e62e2e62afb177c 0 foo.txt
ctime: 0:0
mtime: 1544782273:0
dev: 0 ino: 0
uid: 0 gid: 0
size: 0 flags: 0
> git status
On branch master
Untracked files:
...
> git ls-files -sv --debug
H 100644 3bd1f0e29744a1f32b08d5650e62e2e62afb177c 0 foo.txt
ctime: 1544782273:797529188
mtime: 1544782273:796936707
dev: 16777220 ino: 64184423
uid: 503 gid: 20
size: 8 flags: 0
> git apply --3way ../foo.patch
> cat foo.txt
foo
bar
baz
>
^ permalink raw reply
* Re: [PATCH v2 7/8] builtin/fetch-pack: support protocol version 2
From: Jeff King @ 2018-12-14 10:17 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Brandon Williams, Jonathan Tan
In-Reply-To: <20181213155817.27666-8-avarab@gmail.com>
On Thu, Dec 13, 2018 at 04:58:16PM +0100, Ævar Arnfjörð Bjarmason wrote:
> From: Jonathan Tan <jonathantanmy@google.com>
>
> Currently, if support for running Git's entire test suite with protocol
> v2 were added, some tests would fail because the fetch-pack CLI command
> dies if it encounters protocol v2. To avoid this, teach fetch-pack
> support for protocol v2.
I'm definitely on board with this goal.
> @@ -219,9 +220,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
> PACKET_READ_CHOMP_NEWLINE |
> PACKET_READ_GENTLE_ON_EOF);
>
> - switch (discover_version(&reader)) {
> + version = discover_version(&reader);
> + switch (version) {
> case protocol_v2:
> - die("support for protocol v2 not implemented yet");
> + get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);
> + break;
> case protocol_v1:
> case protocol_v0:
> get_remote_heads(&reader, &ref, 0, NULL, &shallow);
> @@ -231,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
> }
>
> ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
> - &shallow, pack_lockfile_ptr, protocol_v0);
> + &shallow, pack_lockfile_ptr, version);
This implementation looks absurdly simple. So simple that it makes me
wonder why we did not just do this in the first place. I.e., is there
some hidden gotcha blocking it, or was it simply that all of the
necessary work happened in the meantime and nobody bothered to flip the
switch?
-Peff
^ permalink raw reply
* Re: [PATCH] Simplify handling of setup_git_directory_gently() failure cases.
From: Johannes Schindelin @ 2018-12-14 10:32 UTC (permalink / raw)
To: Erin Dahlgren; +Cc: git, Junio C Hamano
In-Reply-To: <1544722211-13370-1-git-send-email-eedahlgren@gmail.com>
Hi Erin,
On Thu, 13 Dec 2018, Erin Dahlgren wrote:
> setup_git_directory_gently() expects two types of failures to
> discover a git directory (e.g. .git/):
>
> - GIT_DIR_HIT_CEILING: could not find a git directory in any
> parent directories of the cwd.
> - GIT_DIR_HIT_MOUNT_POINT: could not find a git directory in
> any parent directories up to the mount point of the cwd.
>
> Both cases are handled in a similar way, but there are misleading
> and unimportant differences. In both cases, setup_git_directory_gently()
> should:
>
> - Die if we are not in a git repository. Otherwise:
> - Set nongit_ok = 1, indicating that we are not in a git repository
> but this is ok.
> - Call strbuf_release() on any non-static struct strbufs that we
> allocated.
>
> Before this change are two misleading additional behaviors:
>
> - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no
> apparent reason. We never had the chance to change directories
> up to this point so chdir(current cwd) is pointless.
> - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer
> of a static struct strbuf (cwd). This is unnecessary because the
> struct is static so its buffer is always reachable. This is also
> misleading because nowhere else in the function is this buffer
> released.
>
> This change eliminates these two misleading additional behaviors and
> deletes setup_nogit() because the code is clearer without it. The
> result is that we can see clearly that GIT_DIR_HIT_CEILING and
> GIT_DIR_HIT_MOUNT_POINT lead to the same behavior (ignoring the
> different help messages).
>
> Thanks-to: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Erin Dahlgren <eedahlgren@gmail.com>
Thank you for working on this!
> ---
> setup.c | 34 +++++++++++++---------------------
> 1 file changed, 13 insertions(+), 21 deletions(-)
Nice! It's always good to see a code reduction with such a cleanup.
Just one thing:
> diff --git a/setup.c b/setup.c
> index 1be5037..b441e39 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -831,16 +831,6 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,
> return NULL;
> }
>
> -static const char *setup_nongit(const char *cwd, int *nongit_ok)
> -{
> - if (!nongit_ok)
> - die(_("not a git repository (or any of the parent directories): %s"), DEFAULT_GIT_DIR_ENVIRONMENT);
> - if (chdir(cwd))
> - die_errno(_("cannot come back to cwd"));
> - *nongit_ok = 1;
> - return NULL;
> -}
> -
> static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_len)
> {
> struct stat buf;
> @@ -1097,18 +1087,20 @@ const char *setup_git_directory_gently(int *nongit_ok)
> prefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);
> break;
> case GIT_DIR_HIT_CEILING:
> - prefix = setup_nongit(cwd.buf, nongit_ok);
> - break;
> + if (!nongit_ok)
> + die(_("not a git repository (or any of the parent directories): %s"),
> + DEFAULT_GIT_DIR_ENVIRONMENT);
> + *nongit_ok = 1;
> + strbuf_release(&dir);
> + return NULL;
> case GIT_DIR_HIT_MOUNT_POINT:
> - if (nongit_ok) {
> - *nongit_ok = 1;
> - strbuf_release(&cwd);
> - strbuf_release(&dir);
> - return NULL;
> - }
> - die(_("not a git repository (or any parent up to mount point %s)\n"
> - "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> - dir.buf);
> + if (!nongit_ok)
> + die(_("not a git repository (or any parent up to mount point %s)\n"
> + "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> + dir.buf);
This indentation would probably be better aligned with the first
double-quote on the `die` line.
Otherwise: ACK!
Thank you,
Johannes
> + *nongit_ok = 1;
> + strbuf_release(&dir);
> + return NULL;
> default:
> BUG("unhandled setup_git_directory_1() result");
> }
> --
> 2.7.4
>
>
^ permalink raw reply
* Re: [PATCH 0/1] worktree refs: fix case sensitivity for 'head'
From: Johannes Schindelin @ 2018-12-14 10:36 UTC (permalink / raw)
To: Michael Rappazzo via GitGitGadget; +Cc: git, Junio C Hamano
In-Reply-To: <pull.100.git.gitgitgadget@gmail.com>
Hi Michael,
On Thu, 13 Dec 2018, Michael Rappazzo via GitGitGadget wrote:
> Pull-Request: https://github.com/gitgitgadget/git/pull/100
What a nice thing that the 100th GitGitGadget Pull Request is celebrated
by a new GitGitGadget user.
Pleased,
Johannes
^ permalink raw reply
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
From: Ævar Arnfjörð Bjarmason @ 2018-12-14 10:55 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Brandon Williams, Jonathan Tan
In-Reply-To: <20181214101232.GC13465@sigill.intra.peff.net>
On Fri, Dec 14 2018, Jeff King wrote:
> On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> Now that we have this maybe we should discuss why these tests show
>> different things:
>>
>> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
>> > index 086f2c40f6..8b1217ea26 100755
>> > --- a/t/t5500-fetch-pack.sh
>> > +++ b/t/t5500-fetch-pack.sh
>> > @@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
>> > test_commit -C server 6 &&
>> >
>> > git init client &&
>> > - test_must_fail git -C client fetch-pack ../server \
>> > +
>> > + # Other protocol versions (e.g. 2) allow fetching an unadvertised
>> > + # object, so run this test with the default protocol version (0).
>> > + test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \
>> > $(git -C server rev-parse refs/heads/master^) 2>err &&
>>
>> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
>> v2 all the time?
>
> Yeah, I actually didn't realize it until working on the earlier series,
> but this is the documented behavior:
>
> $ git grep -hC3 'want <oid>' Documentation/technical/protocol-v2.txt
>
> A `fetch` request can take the following arguments:
>
> want <oid>
> Indicates to the server an object which the client wants to
> retrieve. Wants can be anything and are not limited to
> advertised objects.
>
> An interesting implication of this at GitHub (and possibly other
> hosters) is that it exposes objects from shared storage via unexpected
> repos. If I fork torvalds/linux to peff/linux and push up object X, a v0
> fetch will (by default) refuse to serve it to me. But v2 will happily
> hand it over, which may confuse people into thinking that the object is
> (or at least at some point was) in Linus's repository.
More importantly this bypasses the security guarantee we've had with the
default of uploadpack.allowAnySHA1InWant=false.
If I push a id_rsa to a topic branch on $githost and immediately realize
my mistake and delete it, someone with access to a log of SHA-1s
(e.g. an IRC bot spewing out from/to commits) won't be able to pull it
down. This is why we have that as a setting in the first place
(f8edeaa05d ("upload-pack: optionally allow fetching any sha1",
2016-11-11)).
Of course this is:
a) Subject to the "SECURITY" section of git-fetch, i.e. maybe this
doesn't even work in the face of a determined attacker.
b) Hosting sites like GitHub, GitLab, Gitweb etc. aren't doing
reachability checks when you view /commit/<id>. If I delete my topic
branch it'll be viewable until the next GC kicks in (which would
also be the case with uploadpack.allowAnySHA1InWant=true).
So I wonder how much we need to care about this in practice, but for
some configurations protocol v2 definitely breaks a security promise
we've been making, now whether that promise was always BS due to "a)"
above is another matter.
I'm inclined to say that in the face of that "SECURITY" section we
should just:
* Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
default. Make saying uploadpack.allowReachableSHA1InWant=false warn
with "this won't work, see SECURITY...".
* The uploadpack.allowTipSHA1InWant setting will also be turned on by
default, and will be much faster, since it'll just degrade to
uploadpack.allowReachableSHA1InWant=true and we won't need any
reachability check. We'll also warn saying that setting it is
useless.
Otherwise what's th point of these settings given what we're talking
about in that "SECURITY" section? It seems to just lure users into a
false sense of security. Better to not make promises we're not confident
we can keep, and say that if you push your id_rsa you need to run a
gc/prune on the server.
^ permalink raw reply
* Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2
From: Ævar Arnfjörð Bjarmason @ 2018-12-14 11:08 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Brandon Williams, Jonathan Tan
In-Reply-To: <87o99o1iot.fsf@evledraar.gmail.com>
On Fri, Dec 14 2018, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Dec 14 2018, Jeff King wrote:
>
>> On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>>> Now that we have this maybe we should discuss why these tests show
>>> different things:
>>>
>>> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
>>> > index 086f2c40f6..8b1217ea26 100755
>>> > --- a/t/t5500-fetch-pack.sh
>>> > +++ b/t/t5500-fetch-pack.sh
>>> > @@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
>>> > test_commit -C server 6 &&
>>> >
>>> > git init client &&
>>> > - test_must_fail git -C client fetch-pack ../server \
>>> > +
>>> > + # Other protocol versions (e.g. 2) allow fetching an unadvertised
>>> > + # object, so run this test with the default protocol version (0).
>>> > + test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \
>>> > $(git -C server rev-parse refs/heads/master^) 2>err &&
>>>
>>> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
>>> v2 all the time?
>>
>> Yeah, I actually didn't realize it until working on the earlier series,
>> but this is the documented behavior:
>>
>> $ git grep -hC3 'want <oid>' Documentation/technical/protocol-v2.txt
>>
>> A `fetch` request can take the following arguments:
>>
>> want <oid>
>> Indicates to the server an object which the client wants to
>> retrieve. Wants can be anything and are not limited to
>> advertised objects.
>>
>> An interesting implication of this at GitHub (and possibly other
>> hosters) is that it exposes objects from shared storage via unexpected
>> repos. If I fork torvalds/linux to peff/linux and push up object X, a v0
>> fetch will (by default) refuse to serve it to me. But v2 will happily
>> hand it over, which may confuse people into thinking that the object is
>> (or at least at some point was) in Linus's repository.
>
> More importantly this bypasses the security guarantee we've had with the
> default of uploadpack.allowAnySHA1InWant=false.
>
> If I push a id_rsa to a topic branch on $githost and immediately realize
> my mistake and delete it, someone with access to a log of SHA-1s
> (e.g. an IRC bot spewing out from/to commits) won't be able to pull it
> down. This is why we have that as a setting in the first place
> (f8edeaa05d ("upload-pack: optionally allow fetching any sha1",
> 2016-11-11)).
>
> Of course this is:
>
> a) Subject to the "SECURITY" section of git-fetch, i.e. maybe this
> doesn't even work in the face of a determined attacker.
>
> b) Hosting sites like GitHub, GitLab, Gitweb etc. aren't doing
> reachability checks when you view /commit/<id>. If I delete my topic
> branch it'll be viewable until the next GC kicks in (which would
> also be the case with uploadpack.allowAnySHA1InWant=true).
>
> So I wonder how much we need to care about this in practice, but for
> some configurations protocol v2 definitely breaks a security promise
> we've been making, now whether that promise was always BS due to "a)"
> above is another matter.
>
> I'm inclined to say that in the face of that "SECURITY" section we
> should just:
>
> * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
> default. Make saying uploadpack.allowReachableSHA1InWant=false warn
> with "this won't work, see SECURITY...".
>
> * The uploadpack.allowTipSHA1InWant setting will also be turned on by
> default, and will be much faster, since it'll just degrade to
> uploadpack.allowReachableSHA1InWant=true and we won't need any
> reachability check. We'll also warn saying that setting it is
> useless.
>
> Otherwise what's th point of these settings given what we're talking
> about in that "SECURITY" section? It seems to just lure users into a
> false sense of security. Better to not make promises we're not confident
> we can keep, and say that if you push your id_rsa you need to run a
> gc/prune on the server.
I sent that too fast. What I meant is that there's 3
settings. uploadpack.{allowTipSHA1InWant,allowReachableSHA1InWant,allowAnySHA1InWant}. Those
are, respectively, cheap/expensive/cheap to serve up.
We could make them cheap/cheap/cheap by just making the first two a
synonym for the 3rd (allowAnySHA1InWant), because as noted above
Also, parts of the v2 code, e.g. this in 685fbd3291 ("fetch-pack:
perform a fetch using v2", 2018-03-15):
/* v2 supports these by default */
allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
Seem to have intended to turn on allowReachableSHA1InWant, not
allowAnySHA1InWant, but apparently that's what we're doing?
In any case, regardless of what v2 intended there anything except having
allowAnySHA1InWant on by default seems irresponsible given x) us
documenting in "SECURITY" that it doesn't really work y) even if it did,
there being a *tiny* minority of people who'd in some way care about
this feature who don't also run some sort of web UI on the git server
that'll be bypassing this setting no matter if it worked as intended for
the wire protocol.
^ permalink raw reply
* Re: [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf
From: Johannes Schindelin @ 2018-12-14 11:10 UTC (permalink / raw)
To: brian m. carlson
Cc: Derrick Stolee via GitGitGadget, git, Junio C Hamano,
Derrick Stolee
In-Reply-To: <20181214005101.GT890086@genre.crustytoothpaste.net>
Hi Brian,
On Fri, 14 Dec 2018, brian m. carlson wrote:
> On Wed, Dec 12, 2018 at 10:14:53AM -0800, Derrick Stolee via GitGitGadget wrote:
> >
> > diff --git a/.gitattributes b/.gitattributes
> > index acf853e029..3738cea7eb 100644
> > --- a/.gitattributes
> > +++ b/.gitattributes
> > @@ -13,3 +13,4 @@
> > /Documentation/gitk.txt conflict-marker-size=32
> > /Documentation/user-manual.txt conflict-marker-size=32
> > /t/t????-*.sh conflict-marker-size=32
> > +/t/oid-info/* eol=lf
>
> Yeah, this seems like a sensible thing to do. I assumed the shell on
> Windows would read data as text files, not as binary files.
This is a tricky thing right there. The Bash we use is borrowed from the
MSYS2 project, which tries to stay as close to Unix/Linux as possible,
i.e. it does *not* treat Carriage Return as part of the line ending.
Changing that default would break tons of things, I would expect.
> It's kinda hard for me as a non-Windows user to predict what will need
> CRLF endings and what will need LF endings with Git on Windows.
Right. It is my hope that I get the Azure Pipelines support going soon, so
that Pull Requests on GitHub are tested on Windows, too. Hopefully that
would help.
Typically, I monitor the Windows builds of `pu` closely. But in this case,
it did not catch because the current Azure Pipeline that runs these tests
(triggered via Travis) specifically forces `core.autocrlf` to `false`, as
that definition pre-dates the work I've done to make Git's source code
CR/LF safe.
I do have a build definition to test with `core.autocrlf = true`, but that
only runs on Git for Windows' `master`, and once git.git has its own Azure
Pipeline, I plan on adding another phase to test that directly.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
From: Johannes Schindelin @ 2018-12-14 11:31 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Johannes Schindelin via GitGitGadget, git
In-Reply-To: <70faf8fb-5e98-9247-9955-71b8fc983567@kdbg.org>
[-- Attachment #1: Type: text/plain, Size: 877 bytes --]
Hi Hannes,
On Thu, 13 Dec 2018, Johannes Sixt wrote:
> Am 13.12.18 um 07:25 schrieb Johannes Sixt:
> > Am 13.12.18 um 03:48 schrieb Junio C Hamano:
> > > So,... what's the conclusion? The patch in the context of my tree
> > > would be a no-op, and we'd need a prerequisite change to the support
> > > function to accompany this patch to be effective?
> >
> > Correct, that is my conclusion.
>
> Moreover, there is no problem with your tree to begin with. I cloned into a
> destination without a drive letter:
>
> C:\>git clone R:\j6t\expat.git \Temp\expat
> Cloning into '\Temp\expat'...
> done.
>
> and it works fine. If I understood the description in patch 1/2 correctly,
> this should have failed.
Thank you for the analysis. I'll look which patches in Git for Windows
introduced that regression, then, and fold this here patch series into
that one.
Thanks,
Dscho
^ permalink raw reply
* t5601 breakage at 3cd325f7be (Merge branch 'js/protocol-advertise-multi' into pu, 2018-12-14)
From: Johannes Schindelin @ 2018-12-14 12:27 UTC (permalink / raw)
To: git; +Cc: Josh Steadmon, Ævar Arnfjörð Bjarmason, gitster
[-- Attachment #1: Type: text/plain, Size: 2056 bytes --]
Hi,
this morning Travis sounded quite a few claxons:
https://travis-ci.org/git/git/builds/467839114
It seems that quite a few tests in t5601-clone.sh fail, the first of which
reading like this:
-- snip --
expecting success:
git clone myhost:src ssh-clone &&
expect_ssh "-o SendEnv=GIT_PROTOCOL" myhost src
++ git clone myhost:src ssh-clone
Cloning into 'ssh-clone'...
++ expect_ssh '-o SendEnv=GIT_PROTOCOL' myhost src
++ test_when_finished '
(cd "$TRASH_DIRECTORY" && rm -f ssh-expect ssh-output.munged && >ssh-output)
'
++ test 0 = 0
++ test_cleanup='{
(cd "$TRASH_DIRECTORY" && rm -f ssh-expect ssh-output.munged && >ssh-output)
} && (exit "$eval_ret"); eval_ret=$?; :'
++ case "$#" in
++ echo 'ssh: -o SendEnv=GIT_PROTOCOL myhost git-upload-pack '\''src'\'''
++ cd '/Users/vsts/agent/2.144.0/work/1/s/t/trash directory.t5601-clone'
++ sed 's/ssh: -o SendEnv=GIT_PROTOCOL /ssh: /'
++ mv ssh-output.munged ssh-output
++ test_cmp ssh-expect ssh-output
++ diff -u ssh-expect ssh-output
--- ssh-expect 2018-12-14 04:30:28.000000000 +0000
+++ ssh-output 2018-12-14 04:30:28.000000000 +0000
@@ -1 +1 @@
-ssh: -o SendEnv=GIT_PROTOCOL myhost git-upload-pack 'src'
+ssh: myhost git-upload-pack 'src'
error: last command exited with $?=1
not ok 37 - clone myhost:src uses ssh
#
# git clone myhost:src ssh-clone &&
# expect_ssh "-o SendEnv=GIT_PROTOCOL" myhost src
#
-- snap --
I've bisected this down to 3cd325f7be (Merge branch
'js/protocol-advertise-multi' into pu, 2018-12-14), a merge, meaning that
two topic branches do not play nice with one another.
Staring at the breakage and the changes involved, I suspected that
391985d7c7 (tests: mark & fix tests broken under
GIT_TEST_PROTOCOL_VERSION=1, 2018-12-13) does not play well with the
merged 24c10f7473 (protocol: advertise multiple supported versions,
2018-11-16), and indeed, reverting 391985d7c7 on top of 3cd325f7be lets
t5601 pass again.
It would appear to me, then, that these two patches step on each others'
toes. Josh, Ævar, what should be done about this?
Ciao,
Johannes
^ permalink raw reply
* Bug: `git commit --fixup` with duplicate commit messages
From: Oliver Joseph Ash @ 2018-12-14 12:30 UTC (permalink / raw)
To: git
Hi,
I believe I have found a bug in `git commit --fixup`.
Steps to reproduce:
1. Create a git history with two commits (A and B) with the same
commit message (e.g. foo)
2. Create a new commit using `git commit --fixup {SHA}`, referring to
the last commit SHA
3. Run an interactive rebase
Expected: the fixup commit should appear below the last commit in the todo list.
Actual: the fixup commit appears below the first commit in the todo list.
It seems that the fixup commit is moved below the most recent commit
with a matching commit message, however in this case there are
duplicate commit messages. I would expect the fixup commit to be moved
below the commit SHA I specified when I ran `git commit --fixup`.
Thanks,
Olly
^ permalink raw reply
* [PATCH v2 4/4] t3506: validate '-m 1 -ff' is now accepted for non-merge commits
From: Sergey Organov @ 2018-12-14 4:53 UTC (permalink / raw)
To: git; +Cc: gitster
In-Reply-To: <cover.1544762343.git.sorganov@gmail.com>
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
t/t3506-cherry-pick-ff.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index fb889ac..127dd00 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -64,10 +64,10 @@ test_expect_success 'merge setup' '
git checkout -b new A
'
-test_expect_success 'cherry-pick a non-merge with --ff and -m should fail' '
+test_expect_success 'cherry-pick explicit first parent of a non-merge with --ff' '
git reset --hard A -- &&
- test_must_fail git cherry-pick --ff -m 1 B &&
- git diff --exit-code A --
+ git cherry-pick --ff -m 1 B &&
+ git diff --exit-code C --
'
test_expect_success 'cherry pick a merge with --ff but without -m should fail' '
--
2.10.0.1.g57b01a3
^ permalink raw reply related
* [PATCH v2 1/4] t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks
From: Sergey Organov @ 2018-12-14 4:53 UTC (permalink / raw)
To: git; +Cc: gitster
In-Reply-To: <cover.1544762343.git.sorganov@gmail.com>
We are going to allow 'git cherry-pick -m 1' for non-merge commits, so
this method to force failure will stop to work.
Use '-m 4' instead as it's very unlikely we will ever have such an
octopus in this test setup.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
t/t3510-cherry-pick-sequence.sh | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index c84eeef..941d502 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -61,7 +61,11 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '
test_expect_success 'cherry-pick persists opts correctly' '
pristine_detach initial &&
- test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&
+ # to make sure that the session to cherry-pick a sequence
+ # gets interrupted, use a high-enough number that is larger
+ # than the number of parents of any commit we have created
+ mainline=4 &&
+ test_expect_code 128 git cherry-pick -s -m $mainline --strategy=recursive -X patience -X ours initial..anotherpick &&
test_path_is_dir .git/sequencer &&
test_path_is_file .git/sequencer/head &&
test_path_is_file .git/sequencer/todo &&
@@ -69,7 +73,7 @@ test_expect_success 'cherry-pick persists opts correctly' '
echo "true" >expect &&
git config --file=.git/sequencer/opts --get-all options.signoff >actual &&
test_cmp expect actual &&
- echo "1" >expect &&
+ echo "$mainline" >expect &&
git config --file=.git/sequencer/opts --get-all options.mainline >actual &&
test_cmp expect actual &&
echo "recursive" >expect &&
--
2.10.0.1.g57b01a3
^ permalink raw reply related
* [PATCH v2 2/4] cherry-pick: do not error on non-merge commits when '-m 1' is specified
From: Sergey Organov @ 2018-12-14 4:53 UTC (permalink / raw)
To: git; +Cc: gitster
In-Reply-To: <cover.1544762343.git.sorganov@gmail.com>
When cherry-picking multiple commits, it's impossible to have both
merge- and non-merge commits on the same command-line. Not specifying
'-m 1' results in cherry-pick refusing to handle merge commits, while
specifying '-m 1' fails on non-merge commits.
This patch allows '-m 1' for non-merge commits. As mainline is always
the only parent for a non-merge commit, it makes little sense to
disable it.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
sequencer.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index e1a4dd1..d0fd61b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1766,9 +1766,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
return error(_("commit %s does not have parent %d"),
oid_to_hex(&commit->object.oid), opts->mainline);
parent = p->item;
- } else if (0 < opts->mainline)
- return error(_("mainline was specified but commit %s is not a merge."),
- oid_to_hex(&commit->object.oid));
+ } else if (1 < opts->mainline)
+ /*
+ * Non-first parent explicitly specified as mainline for
+ * non-merge commit
+ */
+ return error(_("commit %s does not have parent %d"),
+ oid_to_hex(&commit->object.oid), opts->mainline);
else
parent = commit->parents->item;
--
2.10.0.1.g57b01a3
^ permalink raw reply related
* [PATCH v2 3/4] t3502: validate '-m 1' argument is now accepted for non-merge commits
From: Sergey Organov @ 2018-12-14 4:53 UTC (permalink / raw)
To: git; +Cc: gitster
In-Reply-To: <cover.1544762343.git.sorganov@gmail.com>
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
t/t3502-cherry-pick-merge.sh | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index b160271..3259bd5 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -40,12 +40,12 @@ test_expect_success 'cherry-pick -m complains of bogus numbers' '
test_expect_code 129 git cherry-pick -m 0 b
'
-test_expect_success 'cherry-pick a non-merge with -m should fail' '
+test_expect_success 'cherry-pick explicit first parent of a non-merge' '
git reset --hard &&
git checkout a^0 &&
- test_expect_code 128 git cherry-pick -m 1 b &&
- git diff --exit-code a --
+ git cherry-pick -m 1 b &&
+ git diff --exit-code c --
'
@@ -84,12 +84,12 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f
'
-test_expect_success 'revert a non-merge with -m should fail' '
+test_expect_success 'revert explicit first parent of a non-merge' '
git reset --hard &&
git checkout c^0 &&
- test_must_fail git revert -m 1 b &&
- git diff --exit-code c
+ git revert -m 1 b &&
+ git diff --exit-code a
'
--
2.10.0.1.g57b01a3
^ permalink raw reply related
* [PATCH v2 0/4] Allow 'cherry-pick -m 1' for non-merge commits
From: Sergey Organov @ 2018-12-14 4:39 UTC (permalink / raw)
To: git; +Cc: gitster
In-Reply-To: <xmqq5zvwesvz.fsf@gitster-ct.c.googlers.com>
When cherry-picking multiple commits, it's impossible to have both
merge- and non-merge commits on the same command-line. Not specifying
'-m 1' results in cherry-pick refusing to handle merge commits, while
specifying '-m 1' fails on non-merge commits.
This patch series allow '-m 1' for non-merge commits as well as fixes
relevant tests in accordance.
It also opens the way to making '-m 1' the default, that would be
inline with the trends to assume first parent to be the mainline in
most workflows.
Sergey Organov (4):
t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks
cherry-pick: do not error on non-merge commits when '-m 1' is
specified
t3502: validate '-m 1' argument is now accepted for non-merge commits
t3506: validate '-m 1 -ff' is now accepted for non-merge commits
sequencer.c | 10 +++++++---
t/t3502-cherry-pick-merge.sh | 12 ++++++------
t/t3506-cherry-pick-ff.sh | 6 +++---
t/t3510-cherry-pick-sequence.sh | 8 ++++++--
4 files changed, 22 insertions(+), 14 deletions(-)
--
2.10.0.1.g57b01a3
^ permalink raw reply
* Re: Bug: `git commit --fixup` with duplicate commit messages
From: Johannes Schindelin @ 2018-12-14 13:39 UTC (permalink / raw)
To: Oliver Joseph Ash; +Cc: git
In-Reply-To: <CADBHO9E7Bzk9C5ciC416S+5-cS2ANA9d+CzSjKSG+HyxwrU+2w@mail.gmail.com>
Hi Oliver,
On Fri, 14 Dec 2018, Oliver Joseph Ash wrote:
> I believe I have found a bug in `git commit --fixup`.
>
> Steps to reproduce:
> 1. Create a git history with two commits (A and B) with the same
> commit message (e.g. foo)
> 2. Create a new commit using `git commit --fixup {SHA}`, referring to
> the last commit SHA
> 3. Run an interactive rebase
>
> Expected: the fixup commit should appear below the last commit in the todo list.
>
> Actual: the fixup commit appears below the first commit in the todo list.
>
> It seems that the fixup commit is moved below the most recent commit
> with a matching commit message, however in this case there are
> duplicate commit messages. I would expect the fixup commit to be moved
> below the commit SHA I specified when I ran `git commit --fixup`.
Yes, this is a problem of the design of this feature. It was done
partially, I believe, to survive multiple rebases *without* --autosquash
before a final rebase *with* --autosquash.
The behavior really is by design.
Ciao,
Johannes
^ permalink raw reply
* Re: Bug: `git commit --fixup` with duplicate commit messages
From: Rafael Ascensão @ 2018-12-14 14:00 UTC (permalink / raw)
To: Oliver Joseph Ash; +Cc: git
In-Reply-To: <CADBHO9E7Bzk9C5ciC416S+5-cS2ANA9d+CzSjKSG+HyxwrU+2w@mail.gmail.com>
On Fri, Dec 14, 2018 at 12:30:28PM +0000, Oliver Joseph Ash wrote:
> I believe I have found a bug in `git commit --fixup`.
That's not a bug, it's actually the documented behaviour of rebase
--autosquash.
As you figured out, the squash/fixup is based on whether the message
has the squash!/fixup! prefix and the subject matches. But it also
allows specifying hashes.
So for fixups, you can be explicit and use: git commit -m 'fixup! SHA'.
Similarly for squashes. (But a little less friendly as you'll need to
deal with passing an argument to -m that contains newlines).
But adding 'squash! SHA' when the editor opens should also work.
I believe the main reason this works based on subject message matching
is to be more friendly with scenarios where you're sharing series of
commits that include fixups and squashes. (Either via format patch, or
by rebasing the series on a newer base without --autosquash).
On such cases the commit you're trying to fixup will have a different
OID, so any hardcoded OID will be useless while commit message matching
works most of the time.
One potential improvement to this is to teach --fixup and --squash to
also add trailers like: "{fixup,squash}: SHA" or "target: SHA" and teach
--autosquash to respect it when possible.
Thoughts?
^ permalink raw reply
* Re: [PATCH 0/4] Expose gpgsig in pretty-print
From: John Passaro @ 2018-12-14 16:07 UTC (permalink / raw)
To: Michał Górny; +Cc: git, Junio C Hamano, Alexey Shumkin, Jeff King
In-Reply-To: <1544760713.970.1.camel@gentoo.org>
On Thu, Dec 13, 2018 at 11:12 PM Michał Górny <mgorny@gentoo.org> wrote:
>
> On Thu, 2018-12-13 at 16:22 -0500, John Passaro wrote:
> > Currently, users who do not have GPG installed have no way to discern
> > signed from unsigned commits without examining raw commit data. I
> > propose two new pretty-print placeholders to expose this information:
> >
> > %GR: full ("R"aw) contents of gpgsig header
> > %G+: Y/N if the commit has nonempty gpgsig header or not
> >
> > The second is of course much more likely to be used, but having exposed
> > the one, exposing the other too adds almost no complexity.
> >
> > I'm open to suggestion on the names of these placeholders.
> >
> > This commit is based on master but e5a329a279 ("run-command: report exec
> > failure" 2018-12-11) is required for the tests to pass.
> >
> > One note is that this change touches areas of the pretty-format
> > documentation that are radically revamped in aw/pretty-trailers: see
> > 42617752d4 ("doc: group pretty-format.txt placeholders descriptions"
> > 2018-12-08). I have another version of this branch based on that branch
> > as well, so you can use that in case conflicts with aw/pretty-trailers
> > arise.
> >
> > See:
> > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig
> > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig--based-on-aw-pretty-trailers
> >
> > John Passaro (4):
> > pretty: expose raw commit signature
> > t/t7510-signed-commit.sh: test new placeholders
> > doc, tests: pretty behavior when gpg missing
> > docs/pretty-formats: add explanation + copy edits
> >
> > Documentation/pretty-formats.txt | 21 ++++--
> > pretty.c | 36 ++++++++-
> > t/t7510-signed-commit.sh | 125 +++++++++++++++++++++++++++++--
> > 3 files changed, 167 insertions(+), 15 deletions(-)
> >
> >
> > base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
> > prerequisite-patch-id: aedfe228fd293714d9cd0392ac22ff1cba7365db
>
> Just a suggestion: since the raw signature is not very useful without
> the commit data to check it against, and the commit data is non-trivial
> to construct (requires mangling raw data anyway), maybe you could either
> add another placeholder to get the data for signature verification, or
> (alternatively or simultaneously) add a placeholder that prints both
> data and signature in the OpenPGP message format (i.e. something you can
> pass straight to 'gpg --verify').
>
That's a great idea!
Then I might rename the other new placeholders too:
%Gs: signed commit signature (blank when unsigned)
%Gp: signed commit payload (i.e. in practice minus the gpgsig header;
also blank when unsigned as well)
%Gq: query/question whether is signed commit ("Y"/"N")
Thus establishing %G<lowercase> as the gpg-related placeholders that
do not actually require gpg.
And add a test that %Gp%n%Gs or the like passes gpg --verify.
I'll put in a v2 later today or tomorrow. Thank you for the feedback!
--
JP
^ permalink raw reply
* Re: Bug: `git commit --fixup` with duplicate commit messages
From: John Passaro @ 2018-12-14 16:33 UTC (permalink / raw)
To: rafa.almas; +Cc: oliverjash, git
In-Reply-To: <20181214135818.7ta7y5feumzau4g3@rigel>
On Fri, 14 Dec 2018, Oliver Joseph Ash wrote:
> I believe I have found a bug in `git commit --fixup`.
>
> Steps to reproduce:
> 1. Create a git history with two commits (A and B) with the same
> commit message (e.g. foo)
> 2. Create a new commit using `git commit --fixup {SHA}`, referring to
> the last commit SHA
> 3. Run an interactive rebase
>
> Expected: the fixup commit should appear below the last commit in the todo list.
Perhaps another useful behavior, when the ambiguity exists (i.e. absent
the trailer Rafael suggested), would be to keep the existing behavior
but notify the user, and point to a new section in the docs on the
subject:
pick aaaa foo
fixup dddd fixup! foo
pick bbbb foo
# ambiguous autosquash: dddd may have targeted bbbb instead of aaaa.
pick cccc unrelated
# Rebase 0000..dddd onto 0000 (4 commands)
# This rebase includes one or more ambiguous autosquashes.
# See git help rebase for more information (under AMBIGUOUS AUTOSQUASH)
#
# Commands:
# (etc)
On Fri, Dec 14, 2018 at 9:00 AM Rafael Ascensão <rafa.almas@gmail.com> wrote:
>
> On Fri, Dec 14, 2018 at 12:30:28PM +0000, Oliver Joseph Ash wrote:
> > I believe I have found a bug in `git commit --fixup`.
>
> That's not a bug, it's actually the documented behaviour of rebase
> --autosquash.
>
> As you figured out, the squash/fixup is based on whether the message
> has the squash!/fixup! prefix and the subject matches. But it also
> allows specifying hashes.
>
> So for fixups, you can be explicit and use: git commit -m 'fixup! SHA'.
>
> Similarly for squashes. (But a little less friendly as you'll need to
> deal with passing an argument to -m that contains newlines).
>
> But adding 'squash! SHA' when the editor opens should also work.
>
> I believe the main reason this works based on subject message matching
> is to be more friendly with scenarios where you're sharing series of
> commits that include fixups and squashes. (Either via format patch, or
> by rebasing the series on a newer base without --autosquash).
>
> On such cases the commit you're trying to fixup will have a different
> OID, so any hardcoded OID will be useless while commit message matching
> works most of the time.
>
> One potential improvement to this is to teach --fixup and --squash to
> also add trailers like: "{fixup,squash}: SHA" or "target: SHA" and teach
> --autosquash to respect it when possible.
>
> Thoughts?
I like very much the proposal to use trailers to indicate the intended target.
"target: SHA" would probably be better. It could be used for a first pass and
if no match is found (in case of rebase or patch-by-mail) or the trailer is not
present, fall back to the default approach of matching by subject line and
noting other potential matches as described above).
^ permalink raw reply
* Re: [PATCH 0/4] Expose gpgsig in pretty-print
From: Michał Górny @ 2018-12-14 16:48 UTC (permalink / raw)
To: John Passaro; +Cc: git, Junio C Hamano, Alexey Shumkin, Jeff King
In-Reply-To: <CAJdN7KjExd6T+H4-wEupO2dg_mMWzeA22oYaskkfhz+GuFbfRQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3537 bytes --]
On Fri, 2018-12-14 at 11:07 -0500, John Passaro wrote:
> On Thu, Dec 13, 2018 at 11:12 PM Michał Górny <mgorny@gentoo.org> wrote:
> >
> > On Thu, 2018-12-13 at 16:22 -0500, John Passaro wrote:
> > > Currently, users who do not have GPG installed have no way to discern
> > > signed from unsigned commits without examining raw commit data. I
> > > propose two new pretty-print placeholders to expose this information:
> > >
> > > %GR: full ("R"aw) contents of gpgsig header
> > > %G+: Y/N if the commit has nonempty gpgsig header or not
> > >
> > > The second is of course much more likely to be used, but having exposed
> > > the one, exposing the other too adds almost no complexity.
> > >
> > > I'm open to suggestion on the names of these placeholders.
> > >
> > > This commit is based on master but e5a329a279 ("run-command: report exec
> > > failure" 2018-12-11) is required for the tests to pass.
> > >
> > > One note is that this change touches areas of the pretty-format
> > > documentation that are radically revamped in aw/pretty-trailers: see
> > > 42617752d4 ("doc: group pretty-format.txt placeholders descriptions"
> > > 2018-12-08). I have another version of this branch based on that branch
> > > as well, so you can use that in case conflicts with aw/pretty-trailers
> > > arise.
> > >
> > > See:
> > > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig
> > > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig--based-on-aw-pretty-trailers
> > >
> > > John Passaro (4):
> > > pretty: expose raw commit signature
> > > t/t7510-signed-commit.sh: test new placeholders
> > > doc, tests: pretty behavior when gpg missing
> > > docs/pretty-formats: add explanation + copy edits
> > >
> > > Documentation/pretty-formats.txt | 21 ++++--
> > > pretty.c | 36 ++++++++-
> > > t/t7510-signed-commit.sh | 125 +++++++++++++++++++++++++++++--
> > > 3 files changed, 167 insertions(+), 15 deletions(-)
> > >
> > >
> > > base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
> > > prerequisite-patch-id: aedfe228fd293714d9cd0392ac22ff1cba7365db
> >
> > Just a suggestion: since the raw signature is not very useful without
> > the commit data to check it against, and the commit data is non-trivial
> > to construct (requires mangling raw data anyway), maybe you could either
> > add another placeholder to get the data for signature verification, or
> > (alternatively or simultaneously) add a placeholder that prints both
> > data and signature in the OpenPGP message format (i.e. something you can
> > pass straight to 'gpg --verify').
> >
>
> That's a great idea!
>
> Then I might rename the other new placeholders too:
>
> %Gs: signed commit signature (blank when unsigned)
> %Gp: signed commit payload (i.e. in practice minus the gpgsig header;
> also blank when unsigned as well)
> %Gq: query/question whether is signed commit ("Y"/"N")
>
> Thus establishing %G<lowercase> as the gpg-related placeholders that
> do not actually require gpg.
>
> And add a test that %Gp%n%Gs or the like passes gpg --verify.
>
> I'll put in a v2 later today or tomorrow. Thank you for the feedback!
>
Technically speaking, '%Gp%n%Gs' sounds a bit odd, given that
the payload needs to be preceded by the PGP message header but instead
of footer it has the signature's header. Also note that some lines in
the payload may need to be escaped.
--
Best regards,
Michał Górny
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 963 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] worktree refs: fix case sensitivity for 'head'
From: Jacob Keller @ 2018-12-14 17:22 UTC (permalink / raw)
To: Duy Nguyen
Cc: Mike Rappazzo, Stefan Beller, gitgitgadget, Git mailing list,
Junio C Hamano
In-Reply-To: <CACsJy8D9qfBLOUCyca+ws66uHx_tgoFZSTbTBxxW2fRQmyr_Nw@mail.gmail.com>
On Thu, Dec 13, 2018 at 11:38 PM Duy Nguyen <pclouds@gmail.com> wrote:
> Even with a new ref storage, I'm pretty sure pseudo refs like HEAD,
> FETCH_HEAD... will forever be backed by filesystem. HEAD for example
> is part of the repository signature and must exist as a file. We could
> also lookup pseudo refs with readdir() instead of lstat(). On
> case-preserving-and-insensitive filesystems, we can reject "head" this
> way. But that comes with a high cost.
> --
> Duy
Once other refs are backed by something that doesn't depend on
filesystem case sensitivity, you could enforce that we only accept
call-caps HEAD as a psuedo ref, and always look up other spellings in
the other refs backend, though, right? So, yea the actual file may not
be case sensitive, but we would never create refs/head anymore for any
reason, so there would be no ambiguity if reading the refs/head vs
refs/HEAD on a case insensitive file system, since refs/head would no
longer be a legitimate ref stored as a file if you used a different
refs backend.
Basically, we'd be looking up HEAD by checking the file, but we'd stop
looking up head, hEAd, etc in the files, and instead use whatever
other refs backend for non-pseudo refs. Thus, it wouldn't matter,
since we'd never actually lookup the other spellings of HEAD as a
file. Wouldn't that solve the ambiguity, at least once a repository
has fully switched to some alternative refs backend for non-pseudo
refs? (Unless I mis-understand and refs/head could be an added pseudo
ref?)
Jake
^ permalink raw reply
* Re: [PATCH 1/1] worktree refs: fix case sensitivity for 'head'
From: Duy Nguyen @ 2018-12-14 17:38 UTC (permalink / raw)
To: Jacob Keller
Cc: Mike Rappazzo, Stefan Beller, gitgitgadget, Git Mailing List,
Junio C Hamano
In-Reply-To: <CA+P7+xoxE0o=5fMQrDoyCgWMQ-By2t1LdApecRDWmoXXCfnFuw@mail.gmail.com>
On Fri, Dec 14, 2018 at 6:22 PM Jacob Keller <jacob.keller@gmail.com> wrote:
>
> On Thu, Dec 13, 2018 at 11:38 PM Duy Nguyen <pclouds@gmail.com> wrote:
> > Even with a new ref storage, I'm pretty sure pseudo refs like HEAD,
> > FETCH_HEAD... will forever be backed by filesystem. HEAD for example
> > is part of the repository signature and must exist as a file. We could
> > also lookup pseudo refs with readdir() instead of lstat(). On
> > case-preserving-and-insensitive filesystems, we can reject "head" this
> > way. But that comes with a high cost.
> > --
> > Duy
>
> Once other refs are backed by something that doesn't depend on
> filesystem case sensitivity, you could enforce that we only accept
> call-caps HEAD as a psuedo ref, and always look up other spellings in
> the other refs backend, though, right?
Hmm.. yes. I don't know off hand if we have any pseudo refs in
lowercase. Unlikely so yes this should work.
> So, yea the actual file may not
> be case sensitive, but we would never create refs/head anymore for any
> reason, so there would be no ambiguity if reading the refs/head vs
> refs/HEAD on a case insensitive file system, since refs/head would no
> longer be a legitimate ref stored as a file if you used a different
> refs backend.
>
> Basically, we'd be looking up HEAD by checking the file, but we'd stop
> looking up head, hEAd, etc in the files, and instead use whatever
> other refs backend for non-pseudo refs. Thus, it wouldn't matter,
> since we'd never actually lookup the other spellings of HEAD as a
> file. Wouldn't that solve the ambiguity, at least once a repository
> has fully switched to some alternative refs backend for non-pseudo
> refs? (Unless I mis-understand and refs/head could be an added pseudo
> ref?)
No I think "pseudo refs" are those outside "refs" directory only. So
"refs/head" would be a "normal" ref.
> Jake
--
Duy
^ 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