* [PATCH 4/5] ls-files: illustrate bug when recursing with relative pathspec
From: Brandon Williams @ 2017-02-24 23:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds
In-Reply-To: <20170224235100.52627-1-bmwill@google.com>
When using the --recurse-submodules flag with a relative pathspec which
includes "..", an error is produced inside the child process spawned for a
submodule. When creating the pathspec struct in the child, the ".." is
interpreted to mean "go up a directory" which causes an error stating that the
path ".." is outside of the repository.
While it is true that ".." is outside the scope of the submodule, it is
confusing to a user who originally invoked the command where ".." was indeed
still inside the scope of the superproject. Since the child process luanched
for the submodule has some context that it is operating underneath a
superproject, this error could be avoided.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
t/t3007-ls-files-recurse-submodules.sh | 50 ++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index a5426171d..d8ab10866 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -188,6 +188,56 @@ test_expect_success '--recurse-submodules and pathspecs' '
test_cmp expect actual
'
+test_expect_failure '--recurse-submodules and relative paths' '
+ # From top works
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ h.txt
+ sib/file
+ sub/file
+ submodule/.gitmodules
+ submodule/c
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/d
+ submodule/subsub/e.txt
+ EOF
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual &&
+
+ # Relative path to top errors out
+ cat >expect <<-\EOF &&
+ ../.gitmodules
+ ../a
+ b
+ ../h.txt
+ ../sib/file
+ ../sub/file
+ ../submodule/.gitmodules
+ ../submodule/c
+ ../submodule/f.TXT
+ ../submodule/g.txt
+ ../submodule/subsub/d
+ ../submodule/subsub/e.txt
+ EOF
+ git -C b ls-files --recurse-submodules -- .. >actual &&
+ test_cmp expect actual &&
+
+ # Relative path to submodule errors out
+ cat >expect <<-\EOF &&
+ ../submodule/.gitmodules
+ ../submodule/c
+ ../submodule/f.TXT
+ ../submodule/g.txt
+ ../submodule/subsub/d
+ ../submodule/subsub/e.txt
+ EOF
+ git -C b ls-files --recurse-submodules -- ../submodule >actual &&
+ test_cmp expect actual
+'
+
test_expect_success '--recurse-submodules does not support --error-unmatch' '
test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
test_i18ngrep "does not support --error-unmatch" actual
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH 5/5] ls-files: fix bug when recuring with relative pathspec
From: Brandon Williams @ 2017-02-24 23:51 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds
In-Reply-To: <20170224235100.52627-1-bmwill@google.com>
Fix a bug which causes a child process for a submodule to error out when a
relative pathspec with a ".." is provided in the superproject.
While at it, correctly construct the super-prefix to be used in a submodule
when not at the root of the repository.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/ls-files.c | 8 ++++++--
t/t3007-ls-files-recurse-submodules.sh | 2 +-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 159229081..89533ab8e 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -194,12 +194,15 @@ static void compile_submodule_options(const struct dir_struct *dir, int show_tag
static void show_gitlink(const struct cache_entry *ce)
{
struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf name = STRBUF_INIT;
int status;
int i;
+ quote_path_relative(ce->name, prefix, &name);
argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
super_prefix ? super_prefix : "",
- ce->name);
+ name.buf);
+ strbuf_release(&name);
argv_array_push(&cp.args, "ls-files");
argv_array_push(&cp.args, "--recurse-submodules");
@@ -615,7 +618,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
- PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+ PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
+ (super_prefix ? PATHSPEC_FROMROOT : 0),
prefix, argv);
/*
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index d8ab10866..1522ed235 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -188,7 +188,7 @@ test_expect_success '--recurse-submodules and pathspecs' '
test_cmp expect actual
'
-test_expect_failure '--recurse-submodules and relative paths' '
+test_expect_success '--recurse-submodules and relative paths' '
# From top works
cat >expect <<-\EOF &&
.gitmodules
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH 3/5] grep: fix bug when recuring with relative pathspec
From: Brandon Williams @ 2017-02-24 23:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds
In-Reply-To: <20170224235100.52627-1-bmwill@google.com>
Fix a bug which causes a child process for a submodule to error out when
a relative pathspec with a ".." is provided in the superproject.
While at it, correctly construct the super-prefix to be used in a
submodule when not at the root of the repository.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/grep.c | 8 ++++++--
t/t7814-grep-recurse-submodules.sh | 2 +-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef49..65f3413d1 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -538,6 +538,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
int status, i;
const char *end_of_base;
const char *name;
+ struct strbuf buf = STRBUF_INIT;
struct work_item *w = opt->output_priv;
end_of_base = strchr(gs->name, ':');
@@ -550,9 +551,11 @@ static int grep_submodule_launch(struct grep_opt *opt,
argv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);
/* Add super prefix */
+ quote_path_relative(name, opt->prefix, &buf);
argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
super_prefix ? super_prefix : "",
- name);
+ buf.buf);
+ strbuf_release(&buf);
argv_array_push(&cp.args, "grep");
/*
@@ -1199,7 +1202,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
- (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
+ (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0) |
+ (super_prefix ? PATHSPEC_FROMROOT : 0),
prefix, argv + i);
pathspec.max_depth = opt.max_depth;
pathspec.recursive = 1;
diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 418ba68fe..e0932b2b7 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -227,7 +227,7 @@ test_expect_success 'grep history with moved submoules' '
test_cmp expect actual
'
-test_expect_failure 'grep using relative path' '
+test_expect_success 'grep using relative path' '
test_when_finished "rm -rf parent sub" &&
git init sub &&
echo "foobar" >sub/file &&
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH 0/5] recursing submodules with relative pathspec (grep and ls-files)
From: Brandon Williams @ 2017-02-24 23:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds
It was discovered that when using the --recurse-submodules flag with `git grep`
and `git ls-files` and specifying a relative path when not at the root causes
the child processes spawned to error out with an error like:
fatal: ..: '..' is outside repository
While true that ".." is outside the scope of the submodule repository, it
probably doesn't make much sense to the user who gave that pathspec with
respect to the superproject. Since the child processes that are spawned to
handle the submodules have some context that they are executing underneath a
superproject (via the 'super_prefix'), they should be able to prevent dying
under this circumstance.
This series fixes this bug in both git grep and git ls-files as well as
correctly formatting the output from submodules to handle the relative paths
with "..".
One of the changes made to fix this was to add an additional flag for the
parse_pathspec() function in order to treat all paths provided as being from
the root of the repository. I hesitantly selected the name 'PATHSPEC_FROMROOT'
but I'm not fond of it since its too similar to the pathspec magic define
'PATHSPEC_FROMTOP'. So I'm open for naming suggestions.
Brandon Williams (5):
grep: illustrate bug when recursing with relative pathspec
pathspec: add PATHSPEC_FROMROOT flag
grep: fix bug when recuring with relative pathspec
ls-files: illustrate bug when recursing with relative pathspec
ls-files: fix bug when recuring with relative pathspec
builtin/grep.c | 8 ++++--
builtin/ls-files.c | 8 ++++--
pathspec.c | 2 +-
pathspec.h | 2 ++
t/t3007-ls-files-recurse-submodules.sh | 50 ++++++++++++++++++++++++++++++++++
t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++
6 files changed, 107 insertions(+), 5 deletions(-)
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply
* [PATCH 2/5] pathspec: add PATHSPEC_FROMROOT flag
From: Brandon Williams @ 2017-02-24 23:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds
In-Reply-To: <20170224235100.52627-1-bmwill@google.com>
Add the `PATHSPEC_FROMROOT` flag to allow callers to instruct
'parse_pathspec()' that all provided pathspecs are relative to the root
of the repository. This allows a caller to prevent a path that may be
outside of the repository from erroring out during the pathspec struct
construction.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 2 +-
pathspec.h | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/pathspec.c b/pathspec.c
index 7ababb315..ff622ba18 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -353,7 +353,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
if (pathspec_prefix >= 0) {
match = xstrdup(copyfrom);
prefixlen = pathspec_prefix;
- } else if (magic & PATHSPEC_FROMTOP) {
+ } else if ((magic & PATHSPEC_FROMTOP) || (flags & PATHSPEC_FROMROOT)) {
match = xstrdup(copyfrom);
prefixlen = 0;
} else {
diff --git a/pathspec.h b/pathspec.h
index 49fd823dd..cad197436 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -66,6 +66,8 @@ struct pathspec {
* allowed, then it will automatically set for every pathspec.
*/
#define PATHSPEC_LITERAL_PATH (1<<8)
+/* For callers that know all paths are relative to the root of the repository */
+#define PATHSPEC_FROMROOT (1<<9)
extern void parse_pathspec(struct pathspec *pathspec,
unsigned magic_mask,
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH 1/5] grep: illustrate bug when recursing with relative pathspec
From: Brandon Williams @ 2017-02-24 23:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds
In-Reply-To: <20170224235100.52627-1-bmwill@google.com>
When using the --recurse-submodules flag with a relative pathspec which
includes "..", an error is produced inside the child process spawned for
a submodule. When creating the pathspec struct in the child, the ".."
is interpreted to mean "go up a directory" which causes an error stating
that the path ".." is outside of the repository.
While it is true that ".." is outside the scope of the submodule, it is
confusing to a user who originally invoked the command where ".." was
indeed still inside the scope of the superproject. Since the child
process luanched for the submodule has some context that it is operating
underneath a superproject, this error could be avoided.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 67247a01d..418ba68fe 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -227,6 +227,48 @@ test_expect_success 'grep history with moved submoules' '
test_cmp expect actual
'
+test_expect_failure 'grep using relative path' '
+ test_when_finished "rm -rf parent sub" &&
+ git init sub &&
+ echo "foobar" >sub/file &&
+ git -C sub add file &&
+ git -C sub commit -m "add file" &&
+
+ git init parent &&
+ echo "foobar" >parent/file &&
+ git -C parent add file &&
+ mkdir parent/src &&
+ echo "foobar" >parent/src/file2 &&
+ git -C parent add src/file2 &&
+ git -C parent submodule add ../sub &&
+ git -C parent commit -m "add files and submodule" &&
+
+ # From top works
+ cat >expect <<-\EOF &&
+ file:foobar
+ src/file2:foobar
+ sub/file:foobar
+ EOF
+ git -C parent grep --recurse-submodules -e "foobar" >actual &&
+ test_cmp expect actual &&
+
+ # Relative path to top errors out
+ cat >expect <<-\EOF &&
+ ../file:foobar
+ file2:foobar
+ ../sub/file:foobar
+ EOF
+ git -C parent/src grep --recurse-submodules -e "foobar" -- .. >actual &&
+ test_cmp expect actual &&
+
+ # Relative path to submodule errors out
+ cat >expect <<-\EOF &&
+ ../sub/file:foobar
+ EOF
+ git -C parent/src grep --recurse-submodules -e "foobar" -- ../sub >actual &&
+ test_cmp expect actual
+'
+
test_incompatible_with_recurse_submodules ()
{
test_expect_success "--recurse-submodules and $1 are incompatible" "
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* Re: SHA1 collisions found
From: Ian Jackson @ 2017-02-24 23:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Joey Hess, git
In-Reply-To: <xmqq60jz5wbm.fsf@gitster.mtv.corp.google.com>
Junio C Hamano writes ("Re: SHA1 collisions found"):
> Ian Jackson <ijackson@chiark.greenend.org.uk> writes:
> > * Therefore the transition needs to be done by giving every object
> > two names (old and new hash function). Objects may refer to each
> > other by either name, but must pick one. The usual shape of
>
> I do not think it is necessrily so.
Indeed. And my latest thoughts involve instead having two parallel
systems of old and new objects.
> *1* In the above toy example, length being 40 vs 64 is used as a
> sign between SHA-1 and the new hash, and careful readers may
> wonder if we should use sha-3,20769079d22... or something like
> that that more explicity identifies what hash is used, so that
> we can pick a hash whose length is 64 when we transition again.
I have an idea for this. I think we should prefix new hashes with a
single uppercase letter, probably H.
Uppercase because: case-only-distinguished ref names are already
discouraged because they do not work properly on case-insensitive
filesystems; convention is that ref names are lowercase; so an
uppercase letter probably won't appear at the start of a ref name
component even though almost all existing software will treat it as
legal. So the result is that the new object names are unlikely to
collide with ref names.
(There is of course no need to store the H as a literal in filenames,
so the case-insensitive filesystem problem does not apply to ref
names.)
We should definitely not introduce new punctuation into object names.
That will cause a great deal of grief for existing software which has
to handle git object names and may thy to store them in
representations which assume that they match \w+.
The idea of using the length is a neat trick, but it cannot support
the dcurrent object name abbreviation approach unworkable.
Ian.
^ permalink raw reply
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-24 23:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ian Jackson, Joey Hess, git
In-Reply-To: <xmqq60jz5wbm.fsf@gitster.mtv.corp.google.com>
On Fri, Feb 24, 2017 at 09:32:13AM -0800, Junio C Hamano wrote:
> > * Therefore the transition needs to be done by giving every object
> > two names (old and new hash function). Objects may refer to each
> > other by either name, but must pick one. The usual shape of
>
> I do not think it is necessrily so. Existing code may not be able
> to read anything new, but you can make the new code understand
> object names in both formats, and for a smooth transition, I think
> the new code needs to.
>
> For example, a new commit that records a merge of an old and a new
> commit whose resulting tree happens to be the same as the tree of
> the old commit may begin like so:
>
> tree 21b97d4c4f968d1335f16292f954dfdbb91353f0
> parent 20769079d22a9f8010232bdf6131918c33a1bf6910232bdf6131918c33a1bf69
> parent 22af6fef9b6538c9e87e147a920be9509acf1ddd
>
> naming the only object whose name was done with new hash with the
> new longer hash, while recording the names of the other existing
> objects with SHA-1. We would need to extend the object format for
> tag (which would be trivial as the object reference is textual and
> similar to a commit) and tree (much harder), of course.
One thing I worry about in a mixed-hash setting is how often the two
will be mixed. That will lead to interoperability complications, but I
also think it creates security hazards (if I can convince you somehow to
refer to my evil colliding file by its sha1, for example, then I can
subvert the strength of the new hash).
So I'd much rather see strong rules like:
1. Once a repo has flag-day switched over to the new hash format[1],
new references are _always_ done with the new hash. Even ones that
point to pre-flag-day objects!
So you get a "commit-v2" object instead of a "commit", and it has a
distinct hash identity from its "commit" counterpart. You can point
to a classic "commit", but you do so by its new-hash.
The flag-day switch would probably be a repo config flag based on
repositoryformatversion (so old versions would just punt if they
see it). Let's call this flag "newhash" for lack of a better term.
2. Repos that have new-hash set will consider the new hash
format as primary, and always use it when writing and referring to
new objects (e.g., in refs). A (purely local) sha1->new mapping can
be maintained for doing old-style object lookups, or for quick
equivalence checks (this mapping might need to be bi-directional
for some use cases; I haven't thought hard enough about it to say
either way).
3. For protocol interop, the rules would be something like[2]:
a. If upload-pack is serving a newhash repo, it advertises
so in the capabilities.
Recent clients know that the rest of the conversation will
involve the new hash format. If they're cloning, they set the
newhash flag in their local config. If they're fetching, they
probably abort and say "please enable newhash" (because for an
existing repo, it probably needs to migrate refs, for example).
An old client would fail to send back the newhash capability,
and the server would abort the conversation at that point.
A new upload-pack serving a non-newhash repo behaves the same
as now (use sha1, happily interoperate with existing and new
clients).
b. receive-pack is more or less the mirror image.
A server for a newhash-flagged repo has a capability for "this
is a newhash repo" and advertises newhash refs. An existing
client might still try to push, but the server would reject it
unless it advertises "newhash" back to the server.
A newhash-enabled client on a non-newhash repo would abort more
gracefully ("please upgrade your local repo to newhash").
For a newhash-enabled server with a non-newhash repo, it would
probably not advertise anything (not even "I understand
newhash"). Because the process for converting to newhash is not
"just push some newhash objects", but an out-of-band flag-day
to convert it over.
That's just a sketch I came up with. There are probably holes. And it
definitely leaves a lot of _possible_ interoperability on the table in
favor of the flag-day approach. But I think the flag-day approach is a
lot easier to reason about. Both in the code, and in terms of the
security properties.
-Peff
[1] I was intentionally vague on "new hash format" here. Obviously there
are various contenders like SHA-256. But I think there's also an
open question of whether the new format should be a multi-hash
format. That would ease further transitions. At the same time, we
really _don't_ want people picking bespoke hashes for their
repositories. It creates complications in the code, and it destroys
a bunch of optimizations (like knowing when we are both talking
about the same object based on the hash).
So I am torn between "move to SHA-256 (or whatever)" and "move to a
hash format that encodes the hash-type in the first byte, but refuse
to allocate more than one hash for now".
[2] If we're having a flag-day event, this _might_ be time to consider
some of the breaking protocol changes that have been under
discussion. I'm really hesitant to complicate this already-tricky
issue by throwing in the kitchen sink. But if there's going to be a
flag day where you need to upgrade Git to access certain repos, it
might be nice if there's only one. I dunno.
^ permalink raw reply
* Re: SHA1 collisions found
From: Jakub Narębski @ 2017-02-24 23:35 UTC (permalink / raw)
To: Jeff King; +Cc: Joey Hess, git
In-Reply-To: <20170224230604.nt37uw5y3uehukfd@sigill.intra.peff.net>
W dniu 25.02.2017 o 00:06, Jeff King pisze:
> On Fri, Feb 24, 2017 at 11:47:46PM +0100, Jakub Narębski wrote:
>
>> I have just read on ArsTechnica[1] that while Git repository could be
>> corrupted (though this would require attackers to spend great amount
>> of resources creating their own collision, while as said elsewhere
>> in this thread allegedly easy to detect), putting two proof-of-concept
>> different PDFs with same size and SHA-1 actually *breaks* Subversion.
>> Repository can become corrupt, and stop accepting new commits.
>>
>> From what I understand people tried this, and Git doesn't exhibit
>> such problem. I wonder what assumptions SVN made that were broken...
>
> To be clear, nobody has generated a sha1 collision in Git yet, and you
> cannot blindly use the shattered PDFs to do so. Git's notion of the
> SHA-1 of an object include the header, so somebody would have to do a
> shattered-level collision search for something that starts with the
> correct "blob 1234\0" header.
What I meant by "Git doesn't exhibit such problem" (but was not clear
enough) is that Git doesn't break by just adding SHAttered.io PDFs
(which somebody had checked), but need customized attack.
>
> So we don't actually know how Git would behave in the face of a SHA-1
> collision. It would be pretty easy to simulate it with something like:
You are right that it would be good to know if such Git-geared customized
SHA-1 attack would break Git, or would it simply corrupt it (visibly
or not).
>
> ---
> diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
> index 22b125cf8..1be5b5ba3 100644
> --- a/block-sha1/sha1.c
> +++ b/block-sha1/sha1.c
> @@ -231,6 +231,16 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len)
> memcpy(ctx->W, data, len);
> }
>
> +/* sha1 of blobs containing "foo\n" and "bar\n" */
> +static const unsigned char foo_sha1[] = {
> + 0x25, 0x7c, 0xc5, 0x64, 0x2c, 0xb1, 0xa0, 0x54, 0xf0, 0x8c,
> + 0xc8, 0x3f, 0x2d, 0x94, 0x3e, 0x56, 0xfd, 0x3e, 0xbe, 0x99
> +};
> +static const unsigned char bar_sha1[] = {
> + 0x57, 0x16, 0xca, 0x59, 0x87, 0xcb, 0xf9, 0x7d, 0x6b, 0xb5,
> + 0x49, 0x20, 0xbe, 0xa6, 0xad, 0xde, 0x24, 0x2d, 0x87, 0xe6
> +};
> +
> void blk_SHA1_Final(unsigned char hashout[20], blk_SHA_CTX *ctx)
> {
> static const unsigned char pad[64] = { 0x80 };
> @@ -248,4 +258,8 @@ void blk_SHA1_Final(unsigned char hashout[20], blk_SHA_CTX *ctx)
> /* Output hash */
> for (i = 0; i < 5; i++)
> put_be32(hashout + i * 4, ctx->H[i]);
> +
> + /* pretend "foo" and "bar" collide */
> + if (!memcmp(hashout, bar_sha1, 20))
> + memcpy(hashout, foo_sha1, 20);
> }
>
^ permalink raw reply
* Re: [PATCH 3/3] parse_hide_refs_config: tell parse_config_key we don't want a subsection
From: Stefan Beller @ 2017-02-24 23:28 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git@vger.kernel.org
In-Reply-To: <20170224210816.ftcfa4jy4wltv2b7@sigill.intra.peff.net>
On Fri, Feb 24, 2017 at 1:08 PM, Jeff King <peff@peff.net> wrote:
> + (!parse_config_key(var, section, NULL, NULL, &key) &&
> + !strcmp(key, "hiderefs"))) {
Heh, see how fast my code gets replaced with even better code? ;)
All three patches look good,
Thanks,
Stefan
^ permalink raw reply
* Re: SHA1 collisions found
From: Øyvind A. Holm @ 2017-02-24 23:24 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Santiago Torres, Joey Hess, git
In-Reply-To: <e0ad3c81-aa2c-2eea-eb9e-17591b6b592c@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]
On 2017-02-25 00:05:34, Jakub Narębski wrote:
> W dniu 24.02.2017 o 23:53, Santiago Torres pisze:
> > On Fri, Feb 24, 2017 at 11:47:46PM +0100, Jakub Narębski wrote:
> > > I have just read on ArsTechnica[1] that while Git repository could
> > > be corrupted (though this would require attackers to spend great
> > > amount of resources creating their own collision, while as said
> > > elsewhere in this thread allegedly easy to detect), putting two
> > > proof-of-concept different PDFs with same size and SHA-1 actually
> > > *breaks* Subversion. Repository can become corrupt, and stop
> > > accepting new commits.
> >
> > From what I understood in the thread[1], it was the combination of
> > svn + git-svn together. I think Arstechnica may be a little bit
> > sensationalistic here.
>
> > [1] https://bugs.webkit.org/show_bug.cgi?id=168774#c27
>
> Thanks for the link. It looks like the problem was with svn itself
> (couldn't checkout, couldn't sync), but repository is recovered now,
> though not protected against the problem occurring again.
>
> Well, anyone with Subversion installed (so not me) can check it for
> himself/herself... though better do this with separate svnroot.
I tested this yesterday by adding the two PDF files to a Subversion
repository, and found that it wasn't able to clone ("checkout" in svn
speak) the repository after the two files had been committed. I posted
the results to the svn-dev mailing list, the thread is at
<https://svn.haxx.se/dev/archive-2017-02/0142.shtml>.
It seems as it only breaks the working copy because the pristine copies
are identified with a SHA1 sum, but the FSFS repository backend seems to
cope with it.
Regards,
Øyvind
+-| Øyvind A. Holm <sunny@sunbase.org> - N 60.37604° E 5.33339° |-+
| OpenPGP: 0xFB0CBEE894A506E5 - http://www.sunbase.org/pubkey.asc |
| Fingerprint: A006 05D6 E676 B319 55E2 E77E FB0C BEE8 94A5 06E5 |
+------------| 41517b2c-fae7-11e6-9521-db5caa6d21d3 |-------------+
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-24 23:06 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Joey Hess, git
In-Reply-To: <9cedbfa5-4095-15d8-639c-0e3b9b98d6b9@gmail.com>
On Fri, Feb 24, 2017 at 11:47:46PM +0100, Jakub Narębski wrote:
> I have just read on ArsTechnica[1] that while Git repository could be
> corrupted (though this would require attackers to spend great amount
> of resources creating their own collision, while as said elsewhere
> in this thread allegedly easy to detect), putting two proof-of-concept
> different PDFs with same size and SHA-1 actually *breaks* Subversion.
> Repository can become corrupt, and stop accepting new commits.
>
> From what I understand people tried this, and Git doesn't exhibit
> such problem. I wonder what assumptions SVN made that were broken...
To be clear, nobody has generated a sha1 collision in Git yet, and you
cannot blindly use the shattered PDFs to do so. Git's notion of the
SHA-1 of an object include the header, so somebody would have to do a
shattered-level collision search for something that starts with the
correct "blob 1234\0" header.
So we don't actually know how Git would behave in the face of a SHA-1
collision. It would be pretty easy to simulate it with something like:
---
diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
index 22b125cf8..1be5b5ba3 100644
--- a/block-sha1/sha1.c
+++ b/block-sha1/sha1.c
@@ -231,6 +231,16 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, unsigned long len)
memcpy(ctx->W, data, len);
}
+/* sha1 of blobs containing "foo\n" and "bar\n" */
+static const unsigned char foo_sha1[] = {
+ 0x25, 0x7c, 0xc5, 0x64, 0x2c, 0xb1, 0xa0, 0x54, 0xf0, 0x8c,
+ 0xc8, 0x3f, 0x2d, 0x94, 0x3e, 0x56, 0xfd, 0x3e, 0xbe, 0x99
+};
+static const unsigned char bar_sha1[] = {
+ 0x57, 0x16, 0xca, 0x59, 0x87, 0xcb, 0xf9, 0x7d, 0x6b, 0xb5,
+ 0x49, 0x20, 0xbe, 0xa6, 0xad, 0xde, 0x24, 0x2d, 0x87, 0xe6
+};
+
void blk_SHA1_Final(unsigned char hashout[20], blk_SHA_CTX *ctx)
{
static const unsigned char pad[64] = { 0x80 };
@@ -248,4 +258,8 @@ void blk_SHA1_Final(unsigned char hashout[20], blk_SHA_CTX *ctx)
/* Output hash */
for (i = 0; i < 5; i++)
put_be32(hashout + i * 4, ctx->H[i]);
+
+ /* pretend "foo" and "bar" collide */
+ if (!memcmp(hashout, bar_sha1, 20))
+ memcpy(hashout, foo_sha1, 20);
}
^ permalink raw reply related
* Re: SHA1 collisions found
From: Jakub Narębski @ 2017-02-24 23:05 UTC (permalink / raw)
To: Santiago Torres; +Cc: Joey Hess, git
In-Reply-To: <20170224225350.xb7rudyhowmsqdbc@LykOS.localdomain>
W dniu 24.02.2017 o 23:53, Santiago Torres pisze:
> On Fri, Feb 24, 2017 at 11:47:46PM +0100, Jakub Narębski wrote:
>>
>> I have just read on ArsTechnica[1] that while Git repository could be
>> corrupted (though this would require attackers to spend great amount
>> of resources creating their own collision, while as said elsewhere
>> in this thread allegedly easy to detect), putting two proof-of-concept
>> different PDFs with same size and SHA-1 actually *breaks* Subversion.
>> Repository can become corrupt, and stop accepting new commits.
>
> From what I understood in the thread[1], it was the combination of svn +
> git-svn together. I think Arstechnica may be a little bit
> sensationalistic here.
> [1] https://bugs.webkit.org/show_bug.cgi?id=168774#c27
Thanks for the link. It looks like the problem was with svn itself
(couldn't checkout, couldn't sync), but repository is recovered now,
though not protected against the problem occurring again.
Well, anyone with Subversion installed (so not me) can check it
for himself/herself... though better do this with separate svnroot.
Note that the breakage was an accident, trying to add test case
for SHA-1 collision in WebKit cache.
Best regards,
--
Jakub Narębski
^ permalink raw reply
* Re: SHA1 collisions found
From: Santiago Torres @ 2017-02-24 22:53 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Joey Hess, git
In-Reply-To: <9cedbfa5-4095-15d8-639c-0e3b9b98d6b9@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 736 bytes --]
On Fri, Feb 24, 2017 at 11:47:46PM +0100, Jakub Narębski wrote:
> I have just read on ArsTechnica[1] that while Git repository could be
> corrupted (though this would require attackers to spend great amount
> of resources creating their own collision, while as said elsewhere
> in this thread allegedly easy to detect), putting two proof-of-concept
> different PDFs with same size and SHA-1 actually *breaks* Subversion.
> Repository can become corrupt, and stop accepting new commits.
From what I understood in the thread[1], it was the combination of svn +
git-svn together. I think Arstechnica may be a little bit
sensationalistic here.
Cheers!
-Santiago.
[1] https://bugs.webkit.org/show_bug.cgi?id=168774#c27
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: SHA1 collisions found
From: Jakub Narębski @ 2017-02-24 22:47 UTC (permalink / raw)
To: Joey Hess, git
In-Reply-To: <20170223164306.spg2avxzukkggrpb@kitenet.net>
I have just read on ArsTechnica[1] that while Git repository could be
corrupted (though this would require attackers to spend great amount
of resources creating their own collision, while as said elsewhere
in this thread allegedly easy to detect), putting two proof-of-concept
different PDFs with same size and SHA-1 actually *breaks* Subversion.
Repository can become corrupt, and stop accepting new commits.
From what I understand people tried this, and Git doesn't exhibit
such problem. I wonder what assumptions SVN made that were broken...
The https://shattered.io/ page updated their Q&A section with this
information.
BTW. what's with that page use of "GIT" instead of "Git"??
[1]: https://arstechnica.com/security/2017/02/watershed-sha1-collision-just-broke-the-webkit-repository-others-may-follow/
"Watershed SHA1 collision just broke the WebKit repository, others may follow"
^ permalink raw reply
* Re: [PATCH v6 1/1] config: add conditional include
From: Philip Oakley @ 2017-02-24 22:08 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Jeff King, sschuberth, Matthieu Moy,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170224131425.32409-2-pclouds@gmail.com>
From: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
> Sometimes a set of repositories want to share configuration settings
> among themselves that are distinct from other such sets of repositories.
> A user may work on two projects, each of which have multiple
> repositories, and use one user.email for one project while using another
> for the other.
>
> Setting $GIT_DIR/.config works, but if the penalty of forgetting to
> update $GIT_DIR/.config is high (especially when you end up cloning
> often), it may not be the best way to go. Having the settings in
> ~/.gitconfig, which would work for just one set of repositories, would
> not well in such a situation. Having separate ${HOME}s may add more
> problems than it solves.
>
> Extend the include.path mechanism that lets a config file include
> another config file, so that the inclusion can be done only when some
> conditions hold. Then ~/.gitconfig can say "include config-project-A
> only when working on project-A" for each project A the user works on.
>
> In this patch, the only supported grouping is based on $GIT_DIR (in
> absolute path), so you would need to group repositories by directory, or
> something like that to take advantage of it.
>
> We already have include.path for unconditional includes. This patch goes
> with includeIf.<condition>.path to make it clearer that a condition is
> required. The new config has the same backward compatibility approach as
> include.path: older git versions that don't understand includeIf will
> simply ignore them.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> Documentation/config.txt | 61 +++++++++++++++++++++++++++++
> config.c | 97
> +++++++++++++++++++++++++++++++++++++++++++++++
> t/t1305-config-include.sh | 56 +++++++++++++++++++++++++++
> 3 files changed, 214 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 015346c417..6c0cd2a273 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -91,6 +91,56 @@ found at the location of the include directive. If the
> value of the
> relative to the configuration file in which the include directive was
> found. See below for examples.
>
> +Conditional includes
> +~~~~~~~~~~~~~~~~~~~~
> +
> +You can include one config file from another conditionally by setting
On first reading I thought this implied you can only have one `includeIf`
within the config file.
I think it is meant to mean that each `includeIf`could include one other
file, and that users can have multiple `includeIf` lines.
> +a `includeIf.<condition>.path` variable to the name of the file to be
> +included. The variable's value is treated the same way as `include.path`.
> +
--
Philip
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Junio C Hamano @ 2017-02-24 21:54 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, Jeff King, git, Jeff Hostetler
In-Reply-To: <9913e513-553e-eba6-e81a-9c21030dd767@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> It can be argued that in normal interactive use, it is hard to notice
> that another DLL is loaded. Don't forget, though, that on Windows it
> is not only the pure time to resolve the entry points, but also that
> typically virus scanners inspect every executable file that is loaded,
> which adds another share of time.
>
> I'll use the patch for daily work for a while to see whether it hurts.
Please ping this thread again when you have something to add. For
now, I'll demote this patch from 'next' to 'pu' when we rewind and
rebuild 'next' post 2.12 release.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/2] Documentation: Link descriptions of -z to core.quotePath
From: Jakub Narębski @ 2017-02-24 21:54 UTC (permalink / raw)
To: Andreas Heiduk, Junio C Hamano; +Cc: git
In-Reply-To: <1487968676-6126-3-git-send-email-asheiduk@gmail.com>
W dniu 24.02.2017 o 21:37, Andreas Heiduk pisze:
> Linking the description for pathname quoting to the configuration
> variable "core.quotePath" removes inconstistent and incomplete
> sections while also giving two hints how to deal with it: Either with
> "-c core.quotePath=false" or with "-z".
This patch I am not sure about. On one hand it improves consistency
(and makes information more complete), on the other hand it removes
information at hand and instead refers to other manpage.
Perhaps a better solution would be to craft a short description that
is both sufficiently complete, and refers to "core.quotePath" for
more details, and then transclude it with "include::quotepath.txt[]".
>
> Signed-off-by: Andreas Heiduk <asheiduk@gmail.com>
> ---
> Documentation/diff-format.txt | 7 ++++---
> Documentation/diff-generate-patch.txt | 7 +++----
> Documentation/diff-options.txt | 7 +++----
> Documentation/git-apply.txt | 7 +++----
> Documentation/git-commit.txt | 9 ++++++---
> Documentation/git-ls-files.txt | 10 ++++++----
> Documentation/git-ls-tree.txt | 10 +++++++---
> Documentation/git-status.txt | 7 +++----
> 8 files changed, 35 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/diff-format.txt b/Documentation/diff-format.txt
> index cf52626..706916c 100644
> --- a/Documentation/diff-format.txt
> +++ b/Documentation/diff-format.txt
> @@ -78,9 +78,10 @@ Example:
> :100644 100644 5be4a4...... 000000...... M file.c
> ------------------------------------------------
>
> -When `-z` option is not used, TAB, LF, and backslash characters
> -in pathnames are represented as `\t`, `\n`, and `\\`,
> -respectively.
> +Without the `-z` option, pathnames with "unusual" characters are
> +quoted as explained for the configuration variable `core.quotePath`
> +(see linkgit:git-config[1]). Using `-z` the filename is output
> +verbatim and the line is terminated by a NUL byte.
>
> diff format for merges
> ----------------------
> diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-generate-patch.txt
> index d2a7ff5..231105c 100644
> --- a/Documentation/diff-generate-patch.txt
> +++ b/Documentation/diff-generate-patch.txt
> @@ -53,10 +53,9 @@ The index line includes the SHA-1 checksum before and after the change.
> The <mode> is included if the file mode does not change; otherwise,
> separate lines indicate the old and the new mode.
>
> -3. TAB, LF, double quote and backslash characters in pathnames
> - are represented as `\t`, `\n`, `\"` and `\\`, respectively.
> - If there is need for such substitution then the whole
> - pathname is put in double quotes.
> +3. Pathnames with "unusual" characters are quoted as explained for
> + the configuration variable `core.quotePath` (see
> + linkgit:git-config[1]).
>
> 4. All the `file1` files in the output refer to files before the
> commit, and all the `file2` files refer to files after the commit.
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index e6215c3..7c28e73 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -192,10 +192,9 @@ ifndef::git-log[]
> given, do not munge pathnames and use NULs as output field terminators.
> endif::git-log[]
> +
> -Without this option, each pathname output will have TAB, LF, double quotes,
> -and backslash characters replaced with `\t`, `\n`, `\"`, and `\\`,
> -respectively, and the pathname will be enclosed in double quotes if
> -any of those replacements occurred.
> +Without this option, pathnames with "unusual" characters are munged as
> +explained for the configuration variable `core.quotePath` (see
> +linkgit:git-config[1]).
>
> --name-only::
> Show only names of changed files.
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index 8ddb207..a7a001b 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -108,10 +108,9 @@ the information is read from the current index instead.
> When `--numstat` has been given, do not munge pathnames,
> but use a NUL-terminated machine-readable format.
> +
> -Without this option, each pathname output will have TAB, LF, double quotes,
> -and backslash characters replaced with `\t`, `\n`, `\"`, and `\\`,
> -respectively, and the pathname will be enclosed in double quotes if
> -any of those replacements occurred.
> +Without this option, pathnames with "unusual" characters are munged as
> +explained for the configuration variable `core.quotePath` (see
> +linkgit:git-config[1]).
>
> -p<n>::
> Remove <n> leading slashes from traditional diff paths. The
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index 4f8f20a..25dcdcc 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -117,9 +117,12 @@ OPTIONS
>
> -z::
> --null::
> - When showing `short` or `porcelain` status output, terminate
> - entries in the status output with NUL, instead of LF. If no
> - format is given, implies the `--porcelain` output format.
> + When showing `short` or `porcelain` status output, print the
> + filename verbatim and terminate the entries with NUL, instead of LF.
> + If no format is given, implies the `--porcelain` output format.
> + Without the `-z` option, filenames with "unusual" characters are
> + quoted as explained for the configuration variable `core.quotePath`
> + (see linkgit:git-config[1]).
>
> -F <file>::
> --file=<file>::
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 446209e..1cab703 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -77,7 +77,8 @@ OPTIONS
> succeed.
>
> -z::
> - \0 line termination on output.
> + \0 line termination on output and do not quote filenames.
> + See OUTPUT below for more information.
>
> -x <pattern>::
> --exclude=<pattern>::
> @@ -196,9 +197,10 @@ the index records up to three such pairs; one from tree O in stage
> the user (or the porcelain) to see what should eventually be recorded at the
> path. (see linkgit:git-read-tree[1] for more information on state)
>
> -When `-z` option is not used, TAB, LF, and backslash characters
> -in pathnames are represented as `\t`, `\n`, and `\\`,
> -respectively.
> +Without the `-z` option, pathnames with "unusual" characters are
> +quoted as explained for the configuration variable `core.quotePath`
> +(see linkgit:git-config[1]). Using `-z` the filename is output
> +verbatim and the line is terminated by a NUL byte.
>
>
> Exclude Patterns
> diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
> index dbc91f9..9dee7be 100644
> --- a/Documentation/git-ls-tree.txt
> +++ b/Documentation/git-ls-tree.txt
> @@ -53,7 +53,8 @@ OPTIONS
> Show object size of blob (file) entries.
>
> -z::
> - \0 line termination on output.
> + \0 line termination on output and do not quote filenames.
> + See OUTPUT FORMAT below for more information.
>
> --name-only::
> --name-status::
> @@ -82,8 +83,6 @@ Output Format
> -------------
> <mode> SP <type> SP <object> TAB <file>
>
> -Unless the `-z` option is used, TAB, LF, and backslash characters
> -in pathnames are represented as `\t`, `\n`, and `\\`, respectively.
> This output format is compatible with what `--index-info --stdin` of
> 'git update-index' expects.
>
> @@ -95,6 +94,11 @@ Object size identified by <object> is given in bytes, and right-justified
> with minimum width of 7 characters. Object size is given only for blobs
> (file) entries; for other entries `-` character is used in place of size.
>
> +Without the `-z` option, pathnames with "unusual" characters are
> +quoted as explained for the configuration variable `core.quotePath`
> +(see linkgit:git-config[1]). Using `-z` the filename is output
> +verbatim and the line is terminated by a NUL byte.
> +
> GIT
> ---
> Part of the linkgit:git[1] suite
> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index 725065e..ba87365 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -322,10 +322,9 @@ When the `-z` option is given, pathnames are printed as is and
> without any quoting and lines are terminated with a NUL (ASCII 0x00)
> byte.
>
> -Otherwise, all pathnames will be "C-quoted" if they contain any tab,
> -linefeed, double quote, or backslash characters. In C-quoting, these
> -characters will be replaced with the corresponding C-style escape
> -sequences and the resulting pathname will be double quoted.
> +Without the `-z` option, pathnames with "unusual" characters are
> +quoted as explained for the configuration variable `core.quotePath`
> +(see linkgit:git-config[1]).
>
>
> CONFIGURATION
>
^ permalink raw reply
* Re: [PATCH v2 1/2] Documentation: Improve description for core.quotePath
From: Jakub Narębski @ 2017-02-24 21:43 UTC (permalink / raw)
To: Andreas Heiduk, gitster; +Cc: git
In-Reply-To: <1487968676-6126-2-git-send-email-asheiduk@gmail.com>
W dniu 24.02.2017 o 21:37, Andreas Heiduk pisze:
> Signed-off-by: Andreas Heiduk <asheiduk@gmail.com>
Thanks. This is good work.
> ---
> Documentation/config.txt | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1fee83c..fa06c2a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -347,16 +347,20 @@ core.checkStat::
> all fields, including the sub-second part of mtime and ctime.
>
> core.quotePath::
> - The commands that output paths (e.g. 'ls-files',
> - 'diff'), when not given the `-z` option, will quote
> - "unusual" characters in the pathname by enclosing the
> - pathname in a double-quote pair and with backslashes the
> - same way strings in C source code are quoted. If this
> - variable is set to false, the bytes higher than 0x80 are
> - not quoted but output as verbatim. Note that double
> - quote, backslash and control characters are always
> - quoted without `-z` regardless of the setting of this
> - variable.
> +
This empty line should not be here, I think.
> + Commands that output paths (e.g. 'ls-files', 'diff'), will
> + quote "unusual" characters in the pathname by enclosing the
> + pathname in double-quotes and escaping those characters with
> + backslashes in the same way C escapes control characters (e.g.
> + `\t` for TAB, `\n` for LF, `\\` for backslash) or bytes with
> + values larger than 0x80 (e.g. octal `\302\265` for "micro" in
I wonder if we can put UTF-8 in AsciiDoc, that is write "μ"
instead of spelling it "micro" (or: Greek letter "mu").
Or "µ" / "µ", though I wonder how well it is supported
in manpage, info and PDF outputs...
> + UTF-8). If this variable is set to false, bytes higher than
> + 0x80 are not considered "unusual" any more. Double-quotes,
> + backslash and control characters are always escaped regardless
> + of the setting of this variable. A simple space character is
> + not considered "unusual". Many commands can output pathnames
> + completely verbatim using the `-z` option. The default value
> + is true.
>
> core.eol::
> Sets the line ending type to use in the working directory for
>
^ permalink raw reply
* Re: [PATCH 2/3] parse_config_key: allow matching single-level config
From: Jeff King @ 2017-02-24 21:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git
In-Reply-To: <xmqqzihbz3nz.fsf@gitster.mtv.corp.google.com>
On Fri, Feb 24, 2017 at 01:20:48PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > The parse_config_key() function was introduced to make it
> > easier to match "section.subsection.key" variables. It also
> > handles the simpler "section.key", and the caller is
> > responsible for distinguishing the two from its
> > out-parameters.
> >
> > Most callers who _only_ want "section.key" would just use a
> > strcmp(var, "section.key"), since there is no parsing
> > required. However, they may still use parse_config_key() if
> > their "section" variable isn't a constant (an example of
> > this is in parse_hide_refs_config).
>
> Perhaps "only" at the end of the title?
Yeah, that would be an improvement.
> After grepping for call sites of this function, I think we can
> simplify quite a few instances of:
>
> if (parse_config_key(...) || !name)
> return ...;
I think you figured this out from your other response, but no, those are
the opposite case (it tricked me at first, too).
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] parse_config_key: allow matching single-level config
From: Junio C Hamano @ 2017-02-24 21:25 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git
In-Reply-To: <xmqqzihbz3nz.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> The parse_config_key() function was introduced to make it
>> easier to match "section.subsection.key" variables. It also
>> handles the simpler "section.key", and the caller is
>> responsible for distinguishing the two from its
>> out-parameters.
>>
>> Most callers who _only_ want "section.key" would just use a
>> strcmp(var, "section.key"), since there is no parsing
>> required. However, they may still use parse_config_key() if
>> their "section" variable isn't a constant (an example of
>> this is in parse_hide_refs_config).
>
> Perhaps "only" at the end of the title?
which still stands. My initial reaction was "eh, I didn't know it
was an error to call the function for two-level names".
> After grepping for call sites of this function, I think we can
> simplify quite a few instances of:
>
> if (parse_config_key(...) || !name)
> return ...;
This is false. Sorry for the noise.
^ permalink raw reply
* Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key
From: Junio C Hamano @ 2017-02-24 21:24 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git
In-Reply-To: <20170224212025.hcxusnrxijzb5qox@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Feb 24, 2017 at 01:18:48PM -0800, Junio C Hamano wrote:
>
>> > While I'm thinking about it, here are patches to do that. The third one
>> > I'd probably squash into yours (after ordering it to the end).
>> >
>> > [1/3]: parse_config_key: use skip_prefix instead of starts_with
>> > [2/3]: parse_config_key: allow matching single-level config
>> > [3/3]: parse_hide_refs_config: tell parse_config_key we don't want a subsection
>>
>> While you were doing that, I was grepping the call sites for
>> parse_config_key() and made sure that all of them are OK when fed
>> two level names. Most of them follow this pattern:
>>
>> if (parse_config_key(k, "diff", &name, &namelen, &type) || !name)
>> return -1;
>>
>> and ones that do not immediately check !name does either eventually
>> do so or have separate codepaths for handlihng two- and three-level
>> names.
>
> Yeah, I did that, too. :)
>
> I don't think there are any other sites to convert. And I don't think we
> can make the "!name" case easier (because some call-sites do want to
> handle both types). And it's not like it gets much easier anyway (unlike
> the opposite case, you _do_ need to declare the extra variables.
Yeah, because the rejection of !name as an error is not the only
reason these call sites have &name and &namelen---they want to use
that subsection name after that if() statement rejects malformed
input, so we cannot really lose them.
Thanks. All three looked good.
^ permalink raw reply
* Re: [PATCH 2/3] parse_config_key: allow matching single-level config
From: Junio C Hamano @ 2017-02-24 21:20 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git
In-Reply-To: <20170224210802.rpr5vdpqhsp3pt5v@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The parse_config_key() function was introduced to make it
> easier to match "section.subsection.key" variables. It also
> handles the simpler "section.key", and the caller is
> responsible for distinguishing the two from its
> out-parameters.
>
> Most callers who _only_ want "section.key" would just use a
> strcmp(var, "section.key"), since there is no parsing
> required. However, they may still use parse_config_key() if
> their "section" variable isn't a constant (an example of
> this is in parse_hide_refs_config).
Perhaps "only" at the end of the title?
After grepping for call sites of this function, I think we can
simplify quite a few instances of:
if (parse_config_key(...) || !name)
return ...;
^ permalink raw reply
* Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key
From: Jeff King @ 2017-02-24 21:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git
In-Reply-To: <xmqq4lzj1e4n.fsf@gitster.mtv.corp.google.com>
On Fri, Feb 24, 2017 at 01:18:48PM -0800, Junio C Hamano wrote:
> > While I'm thinking about it, here are patches to do that. The third one
> > I'd probably squash into yours (after ordering it to the end).
> >
> > [1/3]: parse_config_key: use skip_prefix instead of starts_with
> > [2/3]: parse_config_key: allow matching single-level config
> > [3/3]: parse_hide_refs_config: tell parse_config_key we don't want a subsection
>
> While you were doing that, I was grepping the call sites for
> parse_config_key() and made sure that all of them are OK when fed
> two level names. Most of them follow this pattern:
>
> if (parse_config_key(k, "diff", &name, &namelen, &type) || !name)
> return -1;
>
> and ones that do not immediately check !name does either eventually
> do so or have separate codepaths for handlihng two- and three-level
> names.
Yeah, I did that, too. :)
I don't think there are any other sites to convert. And I don't think we
can make the "!name" case easier (because some call-sites do want to
handle both types). And it's not like it gets much easier anyway (unlike
the opposite case, you _do_ need to declare the extra variables.
-Peff
^ permalink raw reply
* Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key
From: Junio C Hamano @ 2017-02-24 21:18 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git
In-Reply-To: <20170224210643.max6z2ykm3gbg7lw@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Feb 24, 2017 at 03:39:40PM -0500, Jeff King wrote:
>
>> This will start parsing "receive.foobar.hiderefs", which we don't want.
>> I think you need:
>>
>> !parse_config_key(var, section, &subsection, &subsection_len, &key) &&
>> !subsection &&
>> !strcmp(key, "hiderefs")
>>
>> Perhaps passing NULL for the subsection variable should cause
>> parse_config_key to return failure when there is a non-empty subsection.
>>
>> -Peff
>>
>> PS Outside of parse_config_key, this code would be nicer if it used
>> skip_prefix() instead of starts_with(). Since it's going away, I
>> don't think it matters, but I note that parse_config_key could
>> probably benefit from the same.
>
> While I'm thinking about it, here are patches to do that. The third one
> I'd probably squash into yours (after ordering it to the end).
>
> [1/3]: parse_config_key: use skip_prefix instead of starts_with
> [2/3]: parse_config_key: allow matching single-level config
> [3/3]: parse_hide_refs_config: tell parse_config_key we don't want a subsection
While you were doing that, I was grepping the call sites for
parse_config_key() and made sure that all of them are OK when fed
two level names. Most of them follow this pattern:
if (parse_config_key(k, "diff", &name, &namelen, &type) || !name)
return -1;
and ones that do not immediately check !name does either eventually
do so or have separate codepaths for handlihng two- and three-level
names.
^ 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