* Re: Delta compression not so effective
From: Junio C Hamano @ 2017-03-01 16:17 UTC (permalink / raw)
To: Marius Storm-Olsen; +Cc: Git Mailing List
In-Reply-To: <CAPc5daWXafzN0dpyd+kdHcLU_YSmZpiNz_i2rXn_2hbMN-9Xww@mail.gmail.com>
On Wed, Mar 1, 2017 at 8:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Just a hunch. s/F/f/ perhaps? "-F" does not allow Git to recover from poor
Nah, sorry for the noise. Between -F and -f there shouldn't be any difference.
^ permalink raw reply
* Re: [PATCH v5 24/24] t1406: new tests for submodule ref store
From: Michael Haggerty @ 2017-03-01 15:11 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <CACsJy8BOY8jsCrDYESJ6Gwy_p=J=c8m1qe64w18HX1Cv2AaYnA@mail.gmail.com>
On 03/01/2017 01:34 PM, Duy Nguyen wrote:
> On Wed, Mar 1, 2017 at 12:34 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>> t/t1406-submodule-ref-store.sh (new +x) | 95 +++++++++++++++++++++++++++++++++
>>> 1 file changed, 95 insertions(+)
>>> create mode 100755 t/t1406-submodule-ref-store.sh
>>>
>>> diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
>>> new file mode 100755
>>> index 000000000..3b30ba62f
>>> --- /dev/null
>>> +++ b/t/t1406-submodule-ref-store.sh
>>> [...]
>>
>> I haven't actually read this far in the patch series, but I noticed that
>> a test in this file fails:
>>
>>
>> t1406-submodule-ref-store.sh (Wstat: 256 Tests: 15
>> Failed: 1)
>> Failed test: 10
>> Non-zero exit status: 1
>>
>> I didn't have time to look into it more; let me know if you can't
>> reproduce it.
>
> Fantastic. No I couldn't reproduce it, even --valgrind did not
> complain. First step maybe just pushing your branch somewhere so I can
> try out if you're applying the patches via mail (maybe there's some
> changes in the base that affect this). .Otherwise /t1406-* -v -i might
> be enough clue for me to dig in, I hope.
I'm testing c5302654930070135eec9bc1b4ef99b14e0f28ee from Junio's GitHub
fork.
Unfortunately, the test succeeds (every time) when I run just `t1406-*`
or with `-d` or `-i` options, but fails (every time) when run as part of
the whole test suite, so it's a bit tricky to dig deeper.
By trial and error, I found that the test succeeds if I comment out the
"for_each_reflog()" test. By having that test write its results to
`/tmp` where they won't be deleted, I found that the problem is that the
`actual` results are not sorted correctly:
refs/heads/new-master 0x0
refs/heads/master 0x0
HEAD 0x1
I don't know why it's so Heisenbergish.
Michael
^ permalink raw reply
* Delta compression not so effective
From: Marius Storm-Olsen @ 2017-03-01 13:51 UTC (permalink / raw)
To: git
I have just converted an SVN repo to Git (using SubGit), where I feel
delta compression has let me down :)
Suffice it to say, this is a "traditional" SVN repo, with an extern/
blown out of proportion with many binary check-ins. BUT, even still, I
would expect Git's delta compression to be quite effective, compared to
the compression present in SVN. In this case however, the Git repo ends
up being 46% larger than the SVN DB.
Details - SVN:
Commits: 32988
DB (server) size: 139GB
Branches: 103
Tags: 1088
Details - Git:
$ git count-objects -v
count: 0
size: 0
in-pack: 666515
packs: 1
size-pack: 211933109
prune-packable: 0
garbage: 0
size-garbage: 0
$ du -sh .
203G .
$ java -jar ~/sources/bfg/bfg.jar --delete-folders extern
--no-blob-protection && \
git reflog expire --expire=now --all && \
git gc --prune=now --aggressive
$ git count-objects -v
count: 0
size: 0
in-pack: 495070
packs: 1
size-pack: 5765365
prune-packable: 0
garbage: 0
size-garbage: 0
$ du -sh .
5.6G .
When first importing, I disabled gc to avoid any repacking until
completed. When done importing, there was 209GB of all loose objects
(~670k files). With the hopes of quick consolidation, I did a
git -c gc.autoDetach=0 -c gc.reflogExpire=0 \
-c gc.reflogExpireUnreachable=0 -c gc.rerereresolved=0 \
-c gc.rerereunresolved=0 -c gc.pruneExpire=now \
gc --prune
which brought it down to 206GB in a single pack. I then ran
git repack -a -d -F --window=350 --depth=250
which took it down to 203GB, where I'm at right now.
However, this is still miles away from the 139GB in SVN's DB.
Any ideas what's going on, and why my results are so terrible, compared
to SVN?
Thanks!
--
.marius
^ permalink raw reply
* Re: [PATCH v5 24/24] t1406: new tests for submodule ref store
From: Duy Nguyen @ 2017-03-01 12:34 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <7e5ef9e7-bd90-1917-d8eb-c6310c2744ab@alum.mit.edu>
On Wed, Mar 1, 2017 at 12:34 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> t/t1406-submodule-ref-store.sh (new +x) | 95 +++++++++++++++++++++++++++++++++
>> 1 file changed, 95 insertions(+)
>> create mode 100755 t/t1406-submodule-ref-store.sh
>>
>> diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
>> new file mode 100755
>> index 000000000..3b30ba62f
>> --- /dev/null
>> +++ b/t/t1406-submodule-ref-store.sh
>> [...]
>
> I haven't actually read this far in the patch series, but I noticed that
> a test in this file fails:
>
>
> t1406-submodule-ref-store.sh (Wstat: 256 Tests: 15
> Failed: 1)
> Failed test: 10
> Non-zero exit status: 1
>
> I didn't have time to look into it more; let me know if you can't
> reproduce it.
Fantastic. No I couldn't reproduce it, even --valgrind did not
complain. First step maybe just pushing your branch somewhere so I can
try out if you're applying the patches via mail (maybe there's some
changes in the base that affect this). .Otherwise /t1406-* -v -i might
be enough clue for me to dig in, I hope.
--
Duy
^ permalink raw reply
* Re: [PATCH v5 12/24] refs.c: kill register_ref_store(), add register_submodule_ref_store()
From: Michael Haggerty @ 2017-03-01 12:31 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <CACsJy8BvKvo8_bRHH++dp53DaPbDVkKU0EUQC7ofYvBo+Tec0Q@mail.gmail.com>
On 03/01/2017 01:00 PM, Duy Nguyen wrote:
> On Wed, Mar 1, 2017 at 1:03 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> struct ref_store *get_ref_store(const char *submodule)
>>> {
>>> struct strbuf submodule_sb = STRBUF_INIT;
>>> @@ -1480,6 +1479,9 @@ struct ref_store *get_ref_store(const char *submodule)
>>> if (is_nonbare_repository_dir(&submodule_sb))
>>> refs = ref_store_init(submodule);
>>> strbuf_release(&submodule_sb);
>>> +
>>> + if (refs)
>>
>> I think `refs` should always be non-NULL here for the same reason.
>
> That's true if is_nonbar_repo... returns true. If it's false (e.g.
> uninitialized submodule) then refs remains NULL from before (I didn't
> know about this until I hit a segfault in rev-list in another series)
Oh, yes, true.
But given that, I think the code would be clearer if the two calls were
in the same if; i.e.,
refs = lookup_submodule_ref_store(submodule);
if (refs)
return refs;
strbuf_addstr(&submodule_sb, submodule);
if (is_nonbare_repository_dir(&submodule_sb)) {
refs = ref_store_init(submodule);
register_submodule_ref_store(refs, submodule);
}
strbuf_release(&submodule_sb);
return refs;
or even the `if (!is_nonbare_repository_dir(...)) goto cleanup;` pattern
to make it clearer that this is an error return.
Michael
^ permalink raw reply
* Re: [PATCH v5 09/24] refs.c: introduce get_main_ref_store()
From: Duy Nguyen @ 2017-03-01 12:06 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <0bef1e49-e96b-1666-9b88-f4262c2aeeba@alum.mit.edu>
On Wed, Mar 1, 2017 at 12:51 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> refs.c | 16 ++++++++++++----
>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 81b64b4ed..dab1a21ac 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1456,15 +1456,23 @@ static struct ref_store *ref_store_init(const char *submodule)
>> return refs;
>> }
>>
>> +static struct ref_store *get_main_ref_store(void)
>> +{
>> + struct ref_store *refs;
>> +
>> + if (main_ref_store)
>> + return main_ref_store;
>> +
>> + refs = ref_store_init(NULL);
>> + return refs;
>
> Unnecessary temporary variable?
That's the price for doing too much mechanical conversion :-P On
second read, I think I did that on purpose (or by luck) because the
"kill register_ref_store" patch needs this intermediate "refs" so we
can save it to main_ref_store. Cleaner future patch for the minor
uncleanliness in the current one.
--
Duy
^ permalink raw reply
* Re: [PATCH v5 12/24] refs.c: kill register_ref_store(), add register_submodule_ref_store()
From: Duy Nguyen @ 2017-03-01 12:00 UTC (permalink / raw)
To: Michael Haggerty
Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin,
Ramsay Jones, Stefan Beller, David Turner
In-Reply-To: <92ff53b6-8a85-9be8-e14f-18eb1fcaf732@alum.mit.edu>
On Wed, Mar 1, 2017 at 1:03 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> struct ref_store *get_ref_store(const char *submodule)
>> {
>> struct strbuf submodule_sb = STRBUF_INIT;
>> @@ -1480,6 +1479,9 @@ struct ref_store *get_ref_store(const char *submodule)
>> if (is_nonbare_repository_dir(&submodule_sb))
>> refs = ref_store_init(submodule);
>> strbuf_release(&submodule_sb);
>> +
>> + if (refs)
>
> I think `refs` should always be non-NULL here for the same reason.
That's true if is_nonbar_repo... returns true. If it's false (e.g.
uninitialized submodule) then refs remains NULL from before (I didn't
know about this until I hit a segfault in rev-list in another series)
--
Duy
^ permalink raw reply
* [PATCH 1/2] log-tree: factor out fmt_output_email_subject()
From: René Scharfe @ 2017-03-01 11:36 UTC (permalink / raw)
To: Jeff King, Adrian Dudau; +Cc: git@vger.kernel.org
In-Reply-To: <20170228181719.t67abnnnsdcx2xfd@sigill.intra.peff.net>
Use a strbuf to store the subject prefix string and move its
construction into its own function. This gets rid of two arbitrary
length limits and allows the string to be added by callers directly.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
log-tree.c | 40 ++++++++++++++++++++--------------------
log-tree.h | 1 +
2 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index 8c2415747a..44febb75ab 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -332,35 +332,33 @@ void fmt_output_commit(struct strbuf *filename,
strbuf_release(&subject);
}
+void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)
+{
+ if (opt->total > 0) {
+ strbuf_addf(sb, "Subject: [%s%s%0*d/%d] ",
+ opt->subject_prefix,
+ *opt->subject_prefix ? " " : "",
+ digits_in_number(opt->total),
+ opt->nr, opt->total);
+ } else if (opt->total == 0 && opt->subject_prefix && *opt->subject_prefix) {
+ strbuf_addf(sb, "Subject: [%s] ",
+ opt->subject_prefix);
+ } else {
+ strbuf_addstr(sb, "Subject: ");
+ }
+}
+
void log_write_email_headers(struct rev_info *opt, struct commit *commit,
const char **subject_p,
const char **extra_headers_p,
int *need_8bit_cte_p)
{
- const char *subject = NULL;
+ static struct strbuf subject = STRBUF_INIT;
const char *extra_headers = opt->extra_headers;
const char *name = oid_to_hex(opt->zero_commit ?
&null_oid : &commit->object.oid);
*need_8bit_cte_p = 0; /* unknown */
- if (opt->total > 0) {
- static char buffer[64];
- snprintf(buffer, sizeof(buffer),
- "Subject: [%s%s%0*d/%d] ",
- opt->subject_prefix,
- *opt->subject_prefix ? " " : "",
- digits_in_number(opt->total),
- opt->nr, opt->total);
- subject = buffer;
- } else if (opt->total == 0 && opt->subject_prefix && *opt->subject_prefix) {
- static char buffer[256];
- snprintf(buffer, sizeof(buffer),
- "Subject: [%s] ",
- opt->subject_prefix);
- subject = buffer;
- } else {
- subject = "Subject: ";
- }
fprintf(opt->diffopt.file, "From %s Mon Sep 17 00:00:00 2001\n", name);
graph_show_oneline(opt->graph);
@@ -417,7 +415,9 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
opt->diffopt.stat_sep = buffer;
strbuf_release(&filename);
}
- *subject_p = subject;
+ strbuf_reset(&subject);
+ fmt_output_email_subject(&subject, opt);
+ *subject_p = subject.buf;
*extra_headers_p = extra_headers;
}
diff --git a/log-tree.h b/log-tree.h
index c8116e60cd..dd75dd7770 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -30,5 +30,6 @@ void load_ref_decorations(int flags);
#define FORMAT_PATCH_NAME_MAX 64
void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);
+void fmt_output_email_subject(struct strbuf *, struct rev_info *);
#endif
--
2.12.0
^ permalink raw reply related
* [PATCH 2/2] pretty: use fmt_output_email_subject()
From: René Scharfe @ 2017-03-01 11:37 UTC (permalink / raw)
To: Jeff King, Adrian Dudau; +Cc: git@vger.kernel.org
In-Reply-To: <20170228181719.t67abnnnsdcx2xfd@sigill.intra.peff.net>
Add the email-style subject prefix (e.g. "Subject: [PATCH] ") directly
when it's needed instead of letting log_write_email_headers() prepare
it in a static buffer in advance. This simplifies storage ownership and
code flow.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
This slows down the last three tests in p4000 by ca. 3% for some reason,
so we may want to only do the first part for now, which is performance
neutral on my machine.
builtin/log.c | 5 +++--
builtin/shortlog.c | 2 +-
commit.h | 6 ++++--
log-tree.c | 9 +++------
log-tree.h | 1 -
pretty.c | 7 ++++---
6 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 55d20cc2d8..281af8c1ec 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -989,8 +989,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
return;
- log_write_email_headers(rev, head, &pp.subject, &pp.after_subject,
- &need_8bit_cte);
+ log_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte);
for (i = 0; !need_8bit_cte && i < nr; i++) {
const char *buf = get_commit_buffer(list[i], NULL);
@@ -1005,6 +1004,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
msg = body;
pp.fmt = CMIT_FMT_EMAIL;
pp.date_mode.type = DATE_RFC2822;
+ pp.rev = rev;
+ pp.print_email_subject = 1;
pp_user_info(&pp, NULL, &sb, committer, encoding);
pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
pp_remainder(&pp, &msg, &sb, 0);
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index c9585d475d..f78bb4818d 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -148,7 +148,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
ctx.fmt = CMIT_FMT_USERFORMAT;
ctx.abbrev = log->abbrev;
- ctx.subject = "";
+ ctx.print_email_subject = 1;
ctx.after_subject = "";
ctx.date_mode.type = DATE_NORMAL;
ctx.output_encoding = get_log_output_encoding();
diff --git a/commit.h b/commit.h
index 9c12abb911..459daef94a 100644
--- a/commit.h
+++ b/commit.h
@@ -142,21 +142,24 @@ static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
return (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);
}
+struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
+
struct pretty_print_context {
/*
* Callers should tweak these to change the behavior of pp_* functions.
*/
enum cmit_fmt fmt;
int abbrev;
- const char *subject;
const char *after_subject;
int preserve_subject;
struct date_mode date_mode;
unsigned date_mode_explicit:1;
+ unsigned print_email_subject:1;
int expand_tabs_in_log;
int need_8bit_cte;
char *notes_message;
struct reflog_walk_info *reflog_info;
+ struct rev_info *rev;
const char *output_encoding;
struct string_list *mailmap;
int color;
@@ -175,7 +178,6 @@ struct userformat_want {
};
extern int has_non_ascii(const char *text);
-struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
extern const char *logmsg_reencode(const struct commit *commit,
char **commit_encoding,
const char *output_encoding);
diff --git a/log-tree.c b/log-tree.c
index 44febb75ab..4618dd04ca 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -349,11 +349,9 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)
}
void log_write_email_headers(struct rev_info *opt, struct commit *commit,
- const char **subject_p,
const char **extra_headers_p,
int *need_8bit_cte_p)
{
- static struct strbuf subject = STRBUF_INIT;
const char *extra_headers = opt->extra_headers;
const char *name = oid_to_hex(opt->zero_commit ?
&null_oid : &commit->object.oid);
@@ -415,9 +413,6 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
opt->diffopt.stat_sep = buffer;
strbuf_release(&filename);
}
- strbuf_reset(&subject);
- fmt_output_email_subject(&subject, opt);
- *subject_p = subject.buf;
*extra_headers_p = extra_headers;
}
@@ -602,8 +597,10 @@ void show_log(struct rev_info *opt)
*/
if (cmit_fmt_is_mail(opt->commit_format)) {
- log_write_email_headers(opt, commit, &ctx.subject, &extra_headers,
+ log_write_email_headers(opt, commit, &extra_headers,
&ctx.need_8bit_cte);
+ ctx.rev = opt;
+ ctx.print_email_subject = 1;
} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {
fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), opt->diffopt.file);
if (opt->commit_format != CMIT_FMT_ONELINE)
diff --git a/log-tree.h b/log-tree.h
index dd75dd7770..48f11fb740 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -22,7 +22,6 @@ void format_decorations_extended(struct strbuf *sb, const struct commit *commit,
format_decorations_extended((strbuf), (commit), (color), " (", ", ", ")")
void show_decorations(struct rev_info *opt, struct commit *commit);
void log_write_email_headers(struct rev_info *opt, struct commit *commit,
- const char **subject_p,
const char **extra_headers_p,
int *need_8bit_cte_p);
void load_ref_decorations(int flags);
diff --git a/pretty.c b/pretty.c
index 5e683830d9..d0f86f5d85 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1607,8 +1607,9 @@ void pp_title_line(struct pretty_print_context *pp,
pp->preserve_subject ? "\n" : " ");
strbuf_grow(sb, title.len + 1024);
- if (pp->subject) {
- strbuf_addstr(sb, pp->subject);
+ if (pp->print_email_subject) {
+ if (pp->rev)
+ fmt_output_email_subject(sb, pp->rev);
if (needs_rfc2047_encoding(title.buf, title.len, RFC2047_SUBJECT))
add_rfc2047(sb, title.buf, title.len,
encoding, RFC2047_SUBJECT);
@@ -1818,7 +1819,7 @@ void pretty_print_commit(struct pretty_print_context *pp,
}
pp_header(pp, encoding, commit, &msg, sb);
- if (pp->fmt != CMIT_FMT_ONELINE && !pp->subject) {
+ if (pp->fmt != CMIT_FMT_ONELINE && !pp->print_email_subject) {
strbuf_addch(sb, '\n');
}
--
2.12.0
^ permalink raw reply related
* [PATCH v7 3/3] config: add conditional include
From: Nguyễn Thái Ngọc Duy @ 2017-03-01 11:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, sschuberth, Matthieu Moy,
Philip Oakley, Ramsay Jones,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170301112631.16497-1-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 | 64 +++++++++++++++++++++++++++++++++
config.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++
t/t1305-config-include.sh | 56 +++++++++++++++++++++++++++++
3 files changed, 212 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1fad746efd..2a41e84bab 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -90,6 +90,59 @@ found at the location of the include directive. If the value of the
be relative to the configuration file in which the include directive
was found. See below for examples.
+Conditional includes
+~~~~~~~~~~~~~~~~~~~~
+
+You can include a config file from another conditionally by setting 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`. `includeIf.<condition>path` supports multiple key
+values.
+
+The condition starts with a keyword followed by a colon and some data
+whose format and meaning depends on the keyword. Supported keywords
+are:
+
+`gitdir`::
+
+ The data that follows the keyword `gitdir:` is used as a glob
+ pattern. If the location of the .git directory matches the
+ pattern, the include condition is met.
++
+The .git location may be auto-discovered, or come from `$GIT_DIR`
+environment variable. If the repository is auto discovered via a .git
+file (e.g. from submodules, or a linked worktree), the .git location
+would be the final location where the .git directory is, not where the
+.git file is.
++
+The pattern can contain standard globbing wildcards and two additional
+ones, `**/` and `/**`, that can match multiple path components. Please
+refer to linkgit:gitignore[5] for details. For convenience:
+
+ * If the pattern starts with `~/`, `~` will be substituted with the
+ content of the environment variable `HOME`.
+
+ * If the pattern starts with `./`, it is replaced with the directory
+ containing the current config file.
+
+ * If the pattern does not start with either `~/`, `./` or `/`, `**/`
+ will be automatically prepended. For example, the pattern `foo/bar`
+ becomes `**/foo/bar` and would match `/any/path/to/foo/bar`.
+
+ * If the pattern ends with `/`, `**` will be automatically added. For
+ example, the pattern `foo/` becomes `foo/**`. In other words, it
+ matches "foo" and everything inside, recursively.
+
+`gitdir/i`::
+ This is the same as `gitdir` except that matching is done
+ case-insensitively (e.g. on case-insensitive file sytems)
+
+A few more notes on matching via `gitdir` and `gitdir/i`:
+
+ * Symlinks in `$GIT_DIR` are not resolved before matching.
+
+ * Note that "../" is not special and will match literally, which is
+ unlikely what you want.
Example
~~~~~~~
@@ -118,6 +171,17 @@ Example
path = foo ; expand "foo" relative to the current file
path = ~/foo ; expand "foo" in your `$HOME` directory
+ ; include if $GIT_DIR is /path/to/foo/.git
+ [includeIf "gitdir:/path/to/foo/.git"]
+ path = /path/to/foo.inc
+
+ ; include for all repositories inside /path/to/group
+ [includeIf "gitdir:/path/to/group/"]
+ path = /path/to/foo.inc
+
+ ; include for all repositories inside $HOME/to/group
+ [includeIf "gitdir:~/to/group/"]
+ path = /path/to/foo.inc
Values
~~~~~~
diff --git a/config.c b/config.c
index c6b874a7bf..0dac0f4cb2 100644
--- a/config.c
+++ b/config.c
@@ -13,6 +13,7 @@
#include "hashmap.h"
#include "string-list.h"
#include "utf8.h"
+#include "dir.h"
struct config_source {
struct config_source *prev;
@@ -170,9 +171,94 @@ static int handle_path_include(const char *path, struct config_include_data *inc
return ret;
}
+static int prepare_include_condition_pattern(struct strbuf *pat)
+{
+ struct strbuf path = STRBUF_INIT;
+ char *expanded;
+ int prefix = 0;
+
+ expanded = expand_user_path(pat->buf);
+ if (expanded) {
+ strbuf_reset(pat);
+ strbuf_addstr(pat, expanded);
+ free(expanded);
+ }
+
+ if (pat->buf[0] == '.' && is_dir_sep(pat->buf[1])) {
+ const char *slash;
+
+ if (!cf || !cf->path)
+ return error(_("relative config include "
+ "conditionals must come from files"));
+
+ strbuf_add_absolute_path(&path, cf->path);
+ slash = find_last_dir_sep(path.buf);
+ if (!slash)
+ die("BUG: how is this possible?");
+ strbuf_splice(pat, 0, 1, path.buf, slash - path.buf);
+ prefix = slash - path.buf + 1 /* slash */;
+ } else if (!is_absolute_path(pat->buf))
+ strbuf_insert(pat, 0, "**/", 3);
+
+ if (pat->len && is_dir_sep(pat->buf[pat->len - 1]))
+ strbuf_addstr(pat, "**");
+
+ strbuf_release(&path);
+ return prefix;
+}
+
+static int include_by_gitdir(const char *cond, size_t cond_len, int icase)
+{
+ struct strbuf text = STRBUF_INIT;
+ struct strbuf pattern = STRBUF_INIT;
+ int ret = 0, prefix;
+
+ strbuf_add_absolute_path(&text, get_git_dir());
+ strbuf_add(&pattern, cond, cond_len);
+ prefix = prepare_include_condition_pattern(&pattern);
+
+ if (prefix < 0)
+ goto done;
+
+ if (prefix > 0) {
+ /*
+ * perform literal matching on the prefix part so that
+ * any wildcard character in it can't create side effects.
+ */
+ if (text.len < prefix)
+ goto done;
+ if (!icase && strncmp(pattern.buf, text.buf, prefix))
+ goto done;
+ if (icase && strncasecmp(pattern.buf, text.buf, prefix))
+ goto done;
+ }
+
+ ret = !wildmatch(pattern.buf + prefix, text.buf + prefix,
+ icase ? WM_CASEFOLD : 0, NULL);
+
+done:
+ strbuf_release(&pattern);
+ strbuf_release(&text);
+ return ret;
+}
+
+static int include_condition_is_true(const char *cond, size_t cond_len)
+{
+
+ if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
+ return include_by_gitdir(cond, cond_len, 0);
+ else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
+ return include_by_gitdir(cond, cond_len, 1);
+
+ /* unknown conditionals are always false */
+ return 0;
+}
+
int git_config_include(const char *var, const char *value, void *data)
{
struct config_include_data *inc = data;
+ const char *cond, *key;
+ int cond_len;
int ret;
/*
@@ -185,6 +271,12 @@ int git_config_include(const char *var, const char *value, void *data)
if (!strcmp(var, "include.path"))
ret = handle_path_include(value, inc);
+
+ if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
+ (cond && include_condition_is_true(cond, cond_len)) &&
+ !strcmp(key, "path"))
+ ret = handle_path_include(value, inc);
+
return ret;
}
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 9ba2ba11c3..f0cd2056ba 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -152,6 +152,62 @@ test_expect_success 'relative includes from stdin line fail' '
test_must_fail git config --file - test.one
'
+test_expect_success 'conditional include, both unanchored' '
+ git init foo &&
+ (
+ cd foo &&
+ echo "[includeIf \"gitdir:foo/\"]path=bar" >>.git/config &&
+ echo "[test]one=1" >.git/bar &&
+ echo 1 >expect &&
+ git config test.one >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'conditional include, $HOME expansion' '
+ (
+ cd foo &&
+ echo "[includeIf \"gitdir:~/foo/\"]path=bar2" >>.git/config &&
+ echo "[test]two=2" >.git/bar2 &&
+ echo 2 >expect &&
+ git config test.two >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'conditional include, full pattern' '
+ (
+ cd foo &&
+ echo "[includeIf \"gitdir:**/foo/**\"]path=bar3" >>.git/config &&
+ echo "[test]three=3" >.git/bar3 &&
+ echo 3 >expect &&
+ git config test.three >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'conditional include, relative path' '
+ echo "[includeIf \"gitdir:./foo/.git\"]path=bar4" >>.gitconfig &&
+ echo "[test]four=4" >bar4 &&
+ (
+ cd foo &&
+ echo 4 >expect &&
+ git config test.four >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'conditional include, both unanchored, icase' '
+ (
+ cd foo &&
+ echo "[includeIf \"gitdir/i:FOO/\"]path=bar5" >>.git/config &&
+ echo "[test]five=5" >.git/bar5 &&
+ echo 5 >expect &&
+ git config test.five >actual &&
+ test_cmp expect actual
+ )
+'
+
test_expect_success 'include cycles are detected' '
cat >.gitconfig <<-\EOF &&
[test]value = gitconfig
--
2.11.0.157.gd943d85
^ permalink raw reply related
* [PATCH v7 0/3] Conditional config include
From: Nguyễn Thái Ngọc Duy @ 2017-03-01 11:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, sschuberth, Matthieu Moy,
Philip Oakley, Ramsay Jones,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170224131425.32409-1-pclouds@gmail.com>
v7 changes:
- typo fix here and there
- delete a dead TODO comment
- delete the error on unrecognized condition (we probably want a
better, general mechanism for troubleshooting config keys anyway)
- some fix up on include.path document because the "one or many"
confusion started from there
I don't have a good answer for Jeff's PS about includeIf ugliness. I
agree that includeif is ugly but includeIf looks a bit better. I don't
see a better option (if only "include" does not start or end with a
vowel...). Maybe includewith? Suggestions are welcome.
Interdiff below (just on patch 3, not the first two)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index aab3df04fb..2a41e84bab 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -93,9 +93,11 @@ was found. See below for examples.
Conditional includes
~~~~~~~~~~~~~~~~~~~~
-You can include one config file from another conditionally by setting
-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`.
+You can include a config file from another conditionally by setting 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`. `includeIf.<condition>path` supports multiple key
+values.
The condition starts with a keyword followed by a colon and some data
whose format and meaning depends on the keyword. Supported keywords
@@ -104,13 +106,14 @@ are:
`gitdir`::
The data that follows the keyword `gitdir:` is used as a glob
- pattern. If the location of the .git directory match the
+ pattern. If the location of the .git directory matches the
pattern, the include condition is met.
+
-The .git location which may be auto-discovered, or come from
-`$GIT_DIR` environment variable. If the repository auto discovered via
-a .git file (e.g. from submodules, or a linked worktree), the .git
-location would be the final location, not where the .git file is.
+The .git location may be auto-discovered, or come from `$GIT_DIR`
+environment variable. If the repository is auto discovered via a .git
+file (e.g. from submodules, or a linked worktree), the .git location
+would be the final location where the .git directory is, not where the
+.git file is.
+
The pattern can contain standard globbing wildcards and two additional
ones, `**/` and `/**`, that can match multiple path components. Please
@@ -169,15 +172,15 @@ Example
path = ~/foo ; expand "foo" in your `$HOME` directory
; include if $GIT_DIR is /path/to/foo/.git
- [include-if "gitdir:/path/to/foo/.git"]
+ [includeIf "gitdir:/path/to/foo/.git"]
path = /path/to/foo.inc
; include for all repositories inside /path/to/group
- [include-if "gitdir:/path/to/group/"]
+ [includeIf "gitdir:/path/to/group/"]
path = /path/to/foo.inc
; include for all repositories inside $HOME/to/group
- [include-if "gitdir:~/to/group/"]
+ [includeIf "gitdir:~/to/group/"]
path = /path/to/foo.inc
Values
diff --git a/config.c b/config.c
index ad16802c8a..0dac0f4cb2 100644
--- a/config.c
+++ b/config.c
@@ -191,7 +191,6 @@ static int prepare_include_condition_pattern(struct strbuf *pat)
return error(_("relative config include "
"conditionals must come from files"));
- /* TODO: escape wildcards */
strbuf_add_absolute_path(&path, cf->path);
slash = find_last_dir_sep(path.buf);
if (!slash)
@@ -245,16 +244,12 @@ static int include_by_gitdir(const char *cond, size_t cond_len, int icase)
static int include_condition_is_true(const char *cond, size_t cond_len)
{
- /* no condition (i.e., "include.path") is always true */
- if (!cond)
- return 1;
if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
return include_by_gitdir(cond, cond_len, 0);
else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
return include_by_gitdir(cond, cond_len, 1);
- error(_("unrecognized include condition: %.*s"), (int)cond_len, cond);
/* unknown conditionals are always false */
return 0;
}
@@ -278,7 +273,7 @@ int git_config_include(const char *var, const char *value, void *data)
ret = handle_path_include(value, inc);
if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
- include_condition_is_true(cond, cond_len) &&
+ (cond && include_condition_is_true(cond, cond_len)) &&
!strcmp(key, "path"))
ret = handle_path_include(value, inc);
Nguyễn Thái Ngọc Duy (3):
config.txt: clarify multiple key values in include.path
config.txt: reflow the second include.path paragraph
config: add conditional include
Documentation/config.txt | 77 +++++++++++++++++++++++++++++++++++----
config.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++
t/t1305-config-include.sh | 56 +++++++++++++++++++++++++++++
3 files changed, 218 insertions(+), 7 deletions(-)
--
2.11.0.157.gd943d85
^ permalink raw reply related
* [PATCH v7 2/3] config.txt: reflow the second include.path paragraph
From: Nguyễn Thái Ngọc Duy @ 2017-03-01 11:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, sschuberth, Matthieu Moy,
Philip Oakley, Ramsay Jones,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170301112631.16497-1-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/config.txt | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4748efbf36..1fad746efd 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -84,12 +84,11 @@ You can include a config file from another by setting the special
variable takes a pathname as its value, and is subject to tilde
expansion. `include.path` supports multiple key values.
-The
-included file is expanded immediately, as if its contents had been
+The included file is expanded immediately, as if its contents had been
found at the location of the include directive. If the value of the
-`include.path` variable is a relative path, the path is considered to be
-relative to the configuration file in which the include directive was
-found. See below for examples.
+`include.path` variable is a relative path, the path is considered to
+be relative to the configuration file in which the include directive
+was found. See below for examples.
Example
--
2.11.0.157.gd943d85
^ permalink raw reply related
* [PATCH v7 1/3] config.txt: clarify multiple key values in include.path
From: Nguyễn Thái Ngọc Duy @ 2017-03-01 11:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, sschuberth, Matthieu Moy,
Philip Oakley, Ramsay Jones,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170301112631.16497-1-pclouds@gmail.com>
The phrasing in this paragraph may give an impression that you can only
use it once. Rephrase it a bit.
Helped-by: Philip Oakley <philipoakley@iee.org>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Documentation/config.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 015346c417..4748efbf36 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -79,10 +79,10 @@ escape sequences) are invalid.
Includes
~~~~~~~~
-You can include one config file from another by setting the special
+You can include a config file from another by setting the special
`include.path` variable to the name of the file to be included. The
variable takes a pathname as its value, and is subject to tilde
-expansion.
+expansion. `include.path` supports multiple key values.
The
included file is expanded immediately, as if its contents had been
--
2.11.0.157.gd943d85
^ permalink raw reply related
* Re: git status --> Out of memory, realloc failed
From: René Scharfe @ 2017-03-01 10:02 UTC (permalink / raw)
To: Carsten Fuchs, git
In-Reply-To: <84c02ca1-269e-2f26-c625-476d7f087f5c@cafu.de>
Am 25.02.2017 um 11:13 schrieb Carsten Fuchs:
> Dear Git group,
>
> I use Git at a web hosting service, where my user account has a memory
> limit of 768 MB:
>
> (uiserver):p7715773:~$ uname -a
> Linux infongp-de15 3.14.0-ui16322-uiabi1-infong-amd64 #1 SMP Debian
> 3.14.79-2~ui80+4 (2016-11-17) x86_64 GNU/Linux
What's the output of "ulimit -a"?
> (uiserver):p7715773:~$ git --version
> git version 2.1.4
That's quite old. Can you try a more recent version easily (2.12.0 just
came out)? I don't remember a specific fix, and memory usage perhaps
even increased with newer versions, but ruling out already fixed issues
would be nice.
> (uiserver):p7715773:~/cafu$ git gc
> Zähle Objekte: 44293, Fertig.
> Komprimiere Objekte: 100% (24534/24534), Fertig.
> Schreibe Objekte: 100% (44293/44293), Fertig.
> Total 44293 (delta 17560), reused 41828 (delta 16708)
>
> (uiserver):p7715773:~/cafu$ git status
> fatal: Out of memory, realloc failed
> fatal: recursion detected in die handler
>
> The repository is tracking about 19000 files which together take 260 MB.
> The git server version is 2.7.4.1.g5468f9e (Bitbucket)
Is your repository publicly accessible?
René
^ permalink raw reply
* RE: SHA1 collisions found
From: Dan Shumow @ 2017-03-01 8:57 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Linus Torvalds, Joey Hess, Git Mailing List
In-Reply-To: <20170228192044.cn56puazsa3wtlkd@sigill.intra.peff.net>
> - Dan timed the sha1dc implementation with and without the collision
> detection enabled. The sha1 implementation is only 1.33x slower than
> block-sha1 (for raw sha1 time). Adding in the detection makes it
> 2.6x slower.
> So there's some potential gain from optimizing the sha1
> implementation, but ultimately we may be looking at a 2x slowdown to
> add in the collision detection.
I rearranged our code a little bit and interleaved the message expansion and rounds. This bring our raw SHA-1 implementation (without collision detection) down to 1.11x slower than the block-sha1 implementation in Git. Adding the collision detection brings us to 2.12x slower than the block-sha1 implementation. This was basically attacking the low hanging fruit in optimizing our implementation. There are some things that I haven't looked into yet, but I'm basically at the point of starting to compare the generated assembler to see what's different between our implementations.
OpenSSL's SHA1 implementation is implemented in assembler, so there's no way we're going to get close to that with just C level coding.
Thanks,
Dan
^ permalink raw reply
* Re: [PATCH 0/6] Use time_t
From: René Scharfe @ 2017-02-28 21:31 UTC (permalink / raw)
To: Johannes Schindelin, Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <alpine.DEB.2.20.1702282149160.3767@virtualbox>
Am 28.02.2017 um 21:54 schrieb Johannes Schindelin:
> Hi Junio,
>
> On Tue, 28 Feb 2017, Junio C Hamano wrote:
>
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> Am 28.02.2017 um 15:28 schrieb Jeff King:
>>>
>>>> It looks from the discussion like the sanest path forward is our own
>>>> signed-64bit timestamp_t. That's unfortunate compared to using the
>>>> standard time_t, but hopefully it would reduce the number of knobs
>>>> (like TIME_T_IS_INT64) in the long run.
>>>
>>> Glibc will get a way to enable 64-bit time_t on 32-bit platforms
>>> eventually (https://sourceware.org/glibc/wiki/Y2038ProofnessDesign).
>>> Can platforms that won't provide a 64-bit time_t by 2038 be actually
>>> used at that point? How would we get time information on them? How
>>> would a custom timestamp_t help us?
>>
>> That's a sensible "wait, let's step back a bit". I take it that you are
>> saying "time_t is just fine", and I am inclined to agree.
>>
>> Right now, they may be able to have future timestamps ranging to
>> year 2100 and switching to time_t would limit their ability to
>> express future time to 2038 but they would be able to express
>> timestamp in the past to cover most of 20th century. Given that
>> these 32-bit time_t software platforms will die off before year 2038
>> (either by underlying hardware getting obsolete, or software updated
>> to handle 64-bit time_t), the (temporary) loss of 2038-2100 range
>> would not be too big a deal to warrant additional complexity.
>
> You seem to assume that time_t is required to be signed. But from my
> understanding that is only guaranteed by POSIX, not by ISO C.
>
> We may very well buy ourselves a ton of trouble if we decide to switch to
> `time_t` rather than to `int64_t`.
True, and time_t doesn't even have to be an integer type. But which
platforms capable of running git use something else than int32_t or int64_t?
René
^ permalink raw reply
* Re: [PATCH 0/6] Use time_t
From: René Scharfe @ 2017-03-01 0:59 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Jeff King, git
In-Reply-To: <alpine.DEB.2.20.1703010000260.3767@virtualbox>
Am 01.03.2017 um 00:10 schrieb Johannes Schindelin:
> Hi René,
>
> On Tue, 28 Feb 2017, René Scharfe wrote:
>
>> Am 28.02.2017 um 21:54 schrieb Johannes Schindelin:
>>>
>>> On Tue, 28 Feb 2017, Junio C Hamano wrote:
>>>
>>>> René Scharfe <l.s.r@web.de> writes:
>>>>
>>>>> Am 28.02.2017 um 15:28 schrieb Jeff King:
>>>>>
>>>>>> It looks from the discussion like the sanest path forward is our
>>>>>> own signed-64bit timestamp_t. That's unfortunate compared to
>>>>>> using the standard time_t, but hopefully it would reduce the
>>>>>> number of knobs (like TIME_T_IS_INT64) in the long run.
>>>>>
>>>>> Glibc will get a way to enable 64-bit time_t on 32-bit platforms
>>>>> eventually
>>>>> (https://sourceware.org/glibc/wiki/Y2038ProofnessDesign). Can
>>>>> platforms that won't provide a 64-bit time_t by 2038 be actually
>>>>> used at that point? How would we get time information on them?
>>>>> How would a custom timestamp_t help us?
>>>>
>>>> That's a sensible "wait, let's step back a bit". I take it that you
>>>> are saying "time_t is just fine", and I am inclined to agree.
>>>>
>>>> Right now, they may be able to have future timestamps ranging to
>>>> year 2100 and switching to time_t would limit their ability to
>>>> express future time to 2038 but they would be able to express
>>>> timestamp in the past to cover most of 20th century. Given that
>>>> these 32-bit time_t software platforms will die off before year 2038
>>>> (either by underlying hardware getting obsolete, or software updated
>>>> to handle 64-bit time_t), the (temporary) loss of 2038-2100 range
>>>> would not be too big a deal to warrant additional complexity.
>>>
>>> You seem to assume that time_t is required to be signed. But from my
>>> understanding that is only guaranteed by POSIX, not by ISO C.
>>>
>>> We may very well buy ourselves a ton of trouble if we decide to switch
>>> to `time_t` rather than to `int64_t`.
>>
>> True, and time_t doesn't even have to be an integer type. But which
>> platforms capable of running git use something else than int32_t or
>> int64_t?
>
> That kind of thinking is dangerous. We don't know what platforms are
> running Git, and we have a very clear example how we got it very wrong
> recently, when we broke building with musl by requiring REG_STARTEND
> support [*1*].
In general that's true, and if nobody can add to the list (glibc:
int32_t on 32-bit platforms, int64_t on 64-bit platforms for now;
NetBSD, OpenBSD, Windows int64_t) then we shouldn't make assumptions
about time_t that go beyond the C standard -- at least not without
verifying them, e.g. with asserts or tests. I'd especially be
interested in hearing about platforms that use a floating point type.
Systems lacking REG_STARTEND can use compat/regex/ -- that's doesn't
sound too bad. (I didn't follow the original discussion too closely,
and don't mean to open it up again.)
> So why gamble? If we switch to uint64_t, it would definitely provide the
> smoothest upgrade path. It is what the code assumed implicitly when we
> broke 32-bit in v2.9.1.
We can assume that time_t exists everywhere and is used by functions
like gettimeofday(2) and localtime(3). Why invent our own layer on top?
What would it be able to do that time_t alone can't? I understand a
need to handle times before 1970 for historical documents, but what good
would it do to have the ability to handle dates beyond 2038, today?
Platforms that don't use the next two decades to provide a time_t that
can store the current time by then will have bigger problems than a
crippled git.
> If anybody really wants to support negative timestamps, it should be done
> on top of my work. My current patch series does not even start to try to
> address the ramifications of negative timestamps (see e.g. the use of
> strtoull() for parsing). It is quite unreasonable to ask for such a
> fundamental design change when it could very easily be done incrementally
> instead, when needed, by someone who needs it.
I'm confused. Your patch series converts to time_t. Why not use it?
That would prepare ourselves for the bright future when we'll have a
64-bit time_t in every libc. :)
René
^ permalink raw reply
* [PATCH] Put sha1dc on a diet
From: Linus Torvalds @ 2017-03-01 0:30 UTC (permalink / raw)
To: Junio C Hamano, Jeff King, Marc Stevens, Dan Shumow; +Cc: Git Mailing List
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Tue, 28 Feb 2017 16:12:32 -0800
Subject: [PATCH] Put sha1dc on a diet
This removes the unnecessary parts of the sha1dc code, shrinking things from
[torvalds@i7 git]$ size sha1dc/*.o
text data bss dec hex filename
277559 640 0 278199 43eb7 sha1dc/sha1.o
4438 11352 0 15790 3dae sha1dc/ubc_check.o
to
[torvalds@i7 git]$ size sha1dc/*.o
text data bss dec hex filename
13287 0 0 13287 33e7 sha1dc/sha1.o
4438 11352 0 15790 3dae sha1dc/ubc_check.o
so the sha1.o text size shrinks from about 271kB to about 13kB.
The shrinking comes mainly from only generating the recompressio functions
for the two rounds that are actually used (58 and 65), but also from
removing a couple of other unused functions. The sha1dc library lost its
"safe_hash" parameter to do that, since we check - and refuse to touch -
the colliding cases manually.
The git binary itself is about 2MB of text on my system. For other helper
binaries the size reduction is even more noticeable. A quarter MB here
and a quarter MB there, and suddenly you have a big binary ;)
This has been tested with the bad pdf image:
[torvalds@i7 git]$ ./t/helper/test-sha1 < ~/Downloads/bad.pdf
fatal: The SHA1 computation detected evidence of a collision attack;
refusing to process the contents.
although we obviously still don't have an actual git object to test with.
The normal git test-suite obviously also passes.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
I notice that sha1dc is in the 'pu' branch now, so let's put my money
where my mouth is, and send in the sha1dc diet patch.
sha1dc/sha1.c | 356 +++-------------------------------------------------------
sha1dc/sha1.h | 24 ----
2 files changed, 18 insertions(+), 362 deletions(-)
diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index 6569b403e..4910f0c35 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -9,6 +9,15 @@
#include "sha1dc/sha1.h"
#include "sha1dc/ubc_check.h"
+// function type for sha1_recompression_step_T (uint32_t ihvin[5], uint32_t ihvout[5], const uint32_t me2[80], const uint32_t state[5])
+// where 0 <= T < 80
+// me2 is an expanded message (the expansion of an original message block XOR'ed with a disturbance vector's message block difference)
+// state is the internal state (a,b,c,d,e) before step T of the SHA-1 compression function while processing the original message block
+// the function will return:
+// ihvin: the reconstructed input chaining value
+// ihvout: the reconstructed output chaining value
+typedef void(*sha1_recompression_type)(uint32_t*, uint32_t*, const uint32_t*, const uint32_t*);
+
#define rotate_right(x,n) (((x)>>(n))|((x)<<(32-(n))))
#define rotate_left(x,n) (((x)<<(n))|((x)>>(32-(n))))
@@ -39,212 +48,14 @@
-void sha1_message_expansion(uint32_t W[80])
+static void sha1_message_expansion(uint32_t W[80])
{
unsigned i;
for (i = 16; i < 80; ++i)
W[i] = rotate_left(W[i - 3] ^ W[i - 8] ^ W[i - 14] ^ W[i - 16], 1);
}
-void sha1_compression(uint32_t ihv[5], const uint32_t m[16])
-{
- uint32_t W[80];
- uint32_t a, b, c, d, e;
- unsigned i;
-
- memcpy(W, m, 16 * 4);
- for (i = 16; i < 80; ++i)
- W[i] = rotate_left(W[i - 3] ^ W[i - 8] ^ W[i - 14] ^ W[i - 16], 1);
-
- a = ihv[0];
- b = ihv[1];
- c = ihv[2];
- d = ihv[3];
- e = ihv[4];
-
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(a, b, c, d, e, W, 0);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(e, a, b, c, d, W, 1);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(d, e, a, b, c, W, 2);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(c, d, e, a, b, W, 3);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(b, c, d, e, a, W, 4);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(a, b, c, d, e, W, 5);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(e, a, b, c, d, W, 6);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(d, e, a, b, c, W, 7);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(c, d, e, a, b, W, 8);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(b, c, d, e, a, W, 9);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(a, b, c, d, e, W, 10);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(e, a, b, c, d, W, 11);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(d, e, a, b, c, W, 12);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(c, d, e, a, b, W, 13);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(b, c, d, e, a, W, 14);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(a, b, c, d, e, W, 15);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(e, a, b, c, d, W, 16);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(d, e, a, b, c, W, 17);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(c, d, e, a, b, W, 18);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(b, c, d, e, a, W, 19);
-
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(a, b, c, d, e, W, 20);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(e, a, b, c, d, W, 21);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(d, e, a, b, c, W, 22);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(c, d, e, a, b, W, 23);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(b, c, d, e, a, W, 24);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(a, b, c, d, e, W, 25);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(e, a, b, c, d, W, 26);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(d, e, a, b, c, W, 27);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(c, d, e, a, b, W, 28);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(b, c, d, e, a, W, 29);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(a, b, c, d, e, W, 30);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(e, a, b, c, d, W, 31);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(d, e, a, b, c, W, 32);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(c, d, e, a, b, W, 33);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(b, c, d, e, a, W, 34);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(a, b, c, d, e, W, 35);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(e, a, b, c, d, W, 36);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(d, e, a, b, c, W, 37);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(c, d, e, a, b, W, 38);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(b, c, d, e, a, W, 39);
-
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(a, b, c, d, e, W, 40);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(e, a, b, c, d, W, 41);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(d, e, a, b, c, W, 42);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(c, d, e, a, b, W, 43);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(b, c, d, e, a, W, 44);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(a, b, c, d, e, W, 45);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(e, a, b, c, d, W, 46);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(d, e, a, b, c, W, 47);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(c, d, e, a, b, W, 48);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(b, c, d, e, a, W, 49);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(a, b, c, d, e, W, 50);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(e, a, b, c, d, W, 51);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(d, e, a, b, c, W, 52);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(c, d, e, a, b, W, 53);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(b, c, d, e, a, W, 54);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(a, b, c, d, e, W, 55);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(e, a, b, c, d, W, 56);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(d, e, a, b, c, W, 57);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(c, d, e, a, b, W, 58);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(b, c, d, e, a, W, 59);
-
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(a, b, c, d, e, W, 60);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(e, a, b, c, d, W, 61);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(d, e, a, b, c, W, 62);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(c, d, e, a, b, W, 63);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(b, c, d, e, a, W, 64);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(a, b, c, d, e, W, 65);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(e, a, b, c, d, W, 66);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(d, e, a, b, c, W, 67);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(c, d, e, a, b, W, 68);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(b, c, d, e, a, W, 69);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(a, b, c, d, e, W, 70);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(e, a, b, c, d, W, 71);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(d, e, a, b, c, W, 72);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(c, d, e, a, b, W, 73);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(b, c, d, e, a, W, 74);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(a, b, c, d, e, W, 75);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(e, a, b, c, d, W, 76);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(d, e, a, b, c, W, 77);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(c, d, e, a, b, W, 78);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(b, c, d, e, a, W, 79);
-
- ihv[0] += a; ihv[1] += b; ihv[2] += c; ihv[3] += d; ihv[4] += e;
-}
-
-
-
-void sha1_compression_W(uint32_t ihv[5], const uint32_t W[80])
-{
- uint32_t a = ihv[0], b = ihv[1], c = ihv[2], d = ihv[3], e = ihv[4];
-
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(a, b, c, d, e, W, 0);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(e, a, b, c, d, W, 1);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(d, e, a, b, c, W, 2);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(c, d, e, a, b, W, 3);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(b, c, d, e, a, W, 4);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(a, b, c, d, e, W, 5);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(e, a, b, c, d, W, 6);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(d, e, a, b, c, W, 7);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(c, d, e, a, b, W, 8);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(b, c, d, e, a, W, 9);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(a, b, c, d, e, W, 10);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(e, a, b, c, d, W, 11);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(d, e, a, b, c, W, 12);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(c, d, e, a, b, W, 13);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(b, c, d, e, a, W, 14);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(a, b, c, d, e, W, 15);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(e, a, b, c, d, W, 16);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(d, e, a, b, c, W, 17);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(c, d, e, a, b, W, 18);
- HASHCLASH_SHA1COMPRESS_ROUND1_STEP(b, c, d, e, a, W, 19);
-
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(a, b, c, d, e, W, 20);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(e, a, b, c, d, W, 21);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(d, e, a, b, c, W, 22);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(c, d, e, a, b, W, 23);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(b, c, d, e, a, W, 24);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(a, b, c, d, e, W, 25);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(e, a, b, c, d, W, 26);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(d, e, a, b, c, W, 27);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(c, d, e, a, b, W, 28);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(b, c, d, e, a, W, 29);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(a, b, c, d, e, W, 30);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(e, a, b, c, d, W, 31);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(d, e, a, b, c, W, 32);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(c, d, e, a, b, W, 33);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(b, c, d, e, a, W, 34);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(a, b, c, d, e, W, 35);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(e, a, b, c, d, W, 36);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(d, e, a, b, c, W, 37);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(c, d, e, a, b, W, 38);
- HASHCLASH_SHA1COMPRESS_ROUND2_STEP(b, c, d, e, a, W, 39);
-
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(a, b, c, d, e, W, 40);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(e, a, b, c, d, W, 41);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(d, e, a, b, c, W, 42);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(c, d, e, a, b, W, 43);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(b, c, d, e, a, W, 44);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(a, b, c, d, e, W, 45);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(e, a, b, c, d, W, 46);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(d, e, a, b, c, W, 47);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(c, d, e, a, b, W, 48);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(b, c, d, e, a, W, 49);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(a, b, c, d, e, W, 50);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(e, a, b, c, d, W, 51);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(d, e, a, b, c, W, 52);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(c, d, e, a, b, W, 53);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(b, c, d, e, a, W, 54);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(a, b, c, d, e, W, 55);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(e, a, b, c, d, W, 56);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(d, e, a, b, c, W, 57);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(c, d, e, a, b, W, 58);
- HASHCLASH_SHA1COMPRESS_ROUND3_STEP(b, c, d, e, a, W, 59);
-
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(a, b, c, d, e, W, 60);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(e, a, b, c, d, W, 61);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(d, e, a, b, c, W, 62);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(c, d, e, a, b, W, 63);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(b, c, d, e, a, W, 64);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(a, b, c, d, e, W, 65);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(e, a, b, c, d, W, 66);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(d, e, a, b, c, W, 67);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(c, d, e, a, b, W, 68);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(b, c, d, e, a, W, 69);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(a, b, c, d, e, W, 70);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(e, a, b, c, d, W, 71);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(d, e, a, b, c, W, 72);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(c, d, e, a, b, W, 73);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(b, c, d, e, a, W, 74);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(a, b, c, d, e, W, 75);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(e, a, b, c, d, W, 76);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(d, e, a, b, c, W, 77);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(c, d, e, a, b, W, 78);
- HASHCLASH_SHA1COMPRESS_ROUND4_STEP(b, c, d, e, a, W, 79);
-
- ihv[0] += a; ihv[1] += b; ihv[2] += c; ihv[3] += d; ihv[4] += e;
-}
-
-
-
-void sha1_compression_states(uint32_t ihv[5], const uint32_t W[80], uint32_t states[80][5])
+static void sha1_compression_states(uint32_t ihv[5], const uint32_t W[80], uint32_t states[80][5])
{
uint32_t a = ihv[0], b = ihv[1], c = ihv[2], d = ihv[3], e = ihv[4];
@@ -664,7 +475,7 @@ void sha1_compression_states(uint32_t ihv[5], const uint32_t W[80], uint32_t sta
#define SHA1_RECOMPRESS(t) \
-void sha1recompress_fast_ ## t (uint32_t ihvin[5], uint32_t ihvout[5], const uint32_t me2[80], const uint32_t state[5]) \
+static void sha1recompress_fast_ ## t (uint32_t ihvin[5], uint32_t ihvout[5], const uint32_t me2[80], const uint32_t state[5]) \
{ \
uint32_t a = state[0], b = state[1], c = state[2], d = state[3], e = state[4]; \
if (t > 79) HASHCLASH_SHA1COMPRESS_ROUND4_STEP_BW(b, c, d, e, a, me2, 79); \
@@ -832,111 +643,20 @@ void sha1recompress_fast_ ## t (uint32_t ihvin[5], uint32_t ihvout[5], const uin
ihvout[0] = ihvin[0] + a; ihvout[1] = ihvin[1] + b; ihvout[2] = ihvin[2] + c; ihvout[3] = ihvin[3] + d; ihvout[4] = ihvin[4] + e; \
}
-SHA1_RECOMPRESS(0)
-SHA1_RECOMPRESS(1)
-SHA1_RECOMPRESS(2)
-SHA1_RECOMPRESS(3)
-SHA1_RECOMPRESS(4)
-SHA1_RECOMPRESS(5)
-SHA1_RECOMPRESS(6)
-SHA1_RECOMPRESS(7)
-SHA1_RECOMPRESS(8)
-SHA1_RECOMPRESS(9)
-
-SHA1_RECOMPRESS(10)
-SHA1_RECOMPRESS(11)
-SHA1_RECOMPRESS(12)
-SHA1_RECOMPRESS(13)
-SHA1_RECOMPRESS(14)
-SHA1_RECOMPRESS(15)
-SHA1_RECOMPRESS(16)
-SHA1_RECOMPRESS(17)
-SHA1_RECOMPRESS(18)
-SHA1_RECOMPRESS(19)
-
-SHA1_RECOMPRESS(20)
-SHA1_RECOMPRESS(21)
-SHA1_RECOMPRESS(22)
-SHA1_RECOMPRESS(23)
-SHA1_RECOMPRESS(24)
-SHA1_RECOMPRESS(25)
-SHA1_RECOMPRESS(26)
-SHA1_RECOMPRESS(27)
-SHA1_RECOMPRESS(28)
-SHA1_RECOMPRESS(29)
-
-SHA1_RECOMPRESS(30)
-SHA1_RECOMPRESS(31)
-SHA1_RECOMPRESS(32)
-SHA1_RECOMPRESS(33)
-SHA1_RECOMPRESS(34)
-SHA1_RECOMPRESS(35)
-SHA1_RECOMPRESS(36)
-SHA1_RECOMPRESS(37)
-SHA1_RECOMPRESS(38)
-SHA1_RECOMPRESS(39)
-
-SHA1_RECOMPRESS(40)
-SHA1_RECOMPRESS(41)
-SHA1_RECOMPRESS(42)
-SHA1_RECOMPRESS(43)
-SHA1_RECOMPRESS(44)
-SHA1_RECOMPRESS(45)
-SHA1_RECOMPRESS(46)
-SHA1_RECOMPRESS(47)
-SHA1_RECOMPRESS(48)
-SHA1_RECOMPRESS(49)
-
-SHA1_RECOMPRESS(50)
-SHA1_RECOMPRESS(51)
-SHA1_RECOMPRESS(52)
-SHA1_RECOMPRESS(53)
-SHA1_RECOMPRESS(54)
-SHA1_RECOMPRESS(55)
-SHA1_RECOMPRESS(56)
-SHA1_RECOMPRESS(57)
SHA1_RECOMPRESS(58)
-SHA1_RECOMPRESS(59)
-
-SHA1_RECOMPRESS(60)
-SHA1_RECOMPRESS(61)
-SHA1_RECOMPRESS(62)
-SHA1_RECOMPRESS(63)
-SHA1_RECOMPRESS(64)
SHA1_RECOMPRESS(65)
-SHA1_RECOMPRESS(66)
-SHA1_RECOMPRESS(67)
-SHA1_RECOMPRESS(68)
-SHA1_RECOMPRESS(69)
-
-SHA1_RECOMPRESS(70)
-SHA1_RECOMPRESS(71)
-SHA1_RECOMPRESS(72)
-SHA1_RECOMPRESS(73)
-SHA1_RECOMPRESS(74)
-SHA1_RECOMPRESS(75)
-SHA1_RECOMPRESS(76)
-SHA1_RECOMPRESS(77)
-SHA1_RECOMPRESS(78)
-SHA1_RECOMPRESS(79)
-
-sha1_recompression_type sha1_recompression_step[80] =
+
+static sha1_recompression_type sha1_recompression_step[80] =
{
- sha1recompress_fast_0, sha1recompress_fast_1, sha1recompress_fast_2, sha1recompress_fast_3, sha1recompress_fast_4, sha1recompress_fast_5, sha1recompress_fast_6, sha1recompress_fast_7, sha1recompress_fast_8, sha1recompress_fast_9,
- sha1recompress_fast_10, sha1recompress_fast_11, sha1recompress_fast_12, sha1recompress_fast_13, sha1recompress_fast_14, sha1recompress_fast_15, sha1recompress_fast_16, sha1recompress_fast_17, sha1recompress_fast_18, sha1recompress_fast_19,
- sha1recompress_fast_20, sha1recompress_fast_21, sha1recompress_fast_22, sha1recompress_fast_23, sha1recompress_fast_24, sha1recompress_fast_25, sha1recompress_fast_26, sha1recompress_fast_27, sha1recompress_fast_28, sha1recompress_fast_29,
- sha1recompress_fast_30, sha1recompress_fast_31, sha1recompress_fast_32, sha1recompress_fast_33, sha1recompress_fast_34, sha1recompress_fast_35, sha1recompress_fast_36, sha1recompress_fast_37, sha1recompress_fast_38, sha1recompress_fast_39,
- sha1recompress_fast_40, sha1recompress_fast_41, sha1recompress_fast_42, sha1recompress_fast_43, sha1recompress_fast_44, sha1recompress_fast_45, sha1recompress_fast_46, sha1recompress_fast_47, sha1recompress_fast_48, sha1recompress_fast_49,
- sha1recompress_fast_50, sha1recompress_fast_51, sha1recompress_fast_52, sha1recompress_fast_53, sha1recompress_fast_54, sha1recompress_fast_55, sha1recompress_fast_56, sha1recompress_fast_57, sha1recompress_fast_58, sha1recompress_fast_59,
- sha1recompress_fast_60, sha1recompress_fast_61, sha1recompress_fast_62, sha1recompress_fast_63, sha1recompress_fast_64, sha1recompress_fast_65, sha1recompress_fast_66, sha1recompress_fast_67, sha1recompress_fast_68, sha1recompress_fast_69,
- sha1recompress_fast_70, sha1recompress_fast_71, sha1recompress_fast_72, sha1recompress_fast_73, sha1recompress_fast_74, sha1recompress_fast_75, sha1recompress_fast_76, sha1recompress_fast_77, sha1recompress_fast_78, sha1recompress_fast_79,
+ [58] = sha1recompress_fast_58,
+ [65] = sha1recompress_fast_65,
};
-void sha1_process(SHA1_CTX* ctx, const uint32_t block[16])
+static void sha1_process(SHA1_CTX* ctx, const uint32_t block[16])
{
unsigned i, j;
uint32_t ubc_dv_mask[DVMASKSIZE];
@@ -973,12 +693,6 @@ void sha1_process(SHA1_CTX* ctx, const uint32_t block[16])
if (ctx->callback != NULL)
ctx->callback(ctx->total - 64, ctx->ihv1, ctx->ihv2, ctx->m1, ctx->m2);
- if (ctx->safe_hash)
- {
- sha1_compression_W(ctx->ihv, ctx->m1);
- sha1_compression_W(ctx->ihv, ctx->m1);
- }
-
break;
}
}
@@ -990,7 +704,7 @@ void sha1_process(SHA1_CTX* ctx, const uint32_t block[16])
-void swap_bytes(uint32_t val[16])
+static void swap_bytes(uint32_t val[16])
{
unsigned i;
for (i = 0; i < 16; ++i)
@@ -1011,7 +725,6 @@ void SHA1DCInit(SHA1_CTX* ctx)
ctx->ihv[3] = 0x10325476;
ctx->ihv[4] = 0xC3D2E1F0;
ctx->found_collision = 0;
- ctx->safe_hash = 1;
ctx->ubc_check = 1;
ctx->detect_coll = 1;
ctx->reduced_round_coll = 0;
@@ -1019,39 +732,6 @@ void SHA1DCInit(SHA1_CTX* ctx)
ctx->callback = NULL;
}
-void SHA1DCSetSafeHash(SHA1_CTX* ctx, int safehash)
-{
- if (safehash)
- ctx->safe_hash = 1;
- else
- ctx->safe_hash = 0;
-}
-
-
-void SHA1DCSetUseUBC(SHA1_CTX* ctx, int ubc_check)
-{
- if (ubc_check)
- ctx->ubc_check = 1;
- else
- ctx->ubc_check = 0;
-}
-
-void SHA1DCSetUseDetectColl(SHA1_CTX* ctx, int detect_coll)
-{
- if (detect_coll)
- ctx->detect_coll = 1;
- else
- ctx->detect_coll = 0;
-}
-
-void SHA1DCSetDetectReducedRoundCollision(SHA1_CTX* ctx, int reduced_round_coll)
-{
- if (reduced_round_coll)
- ctx->reduced_round_coll = 1;
- else
- ctx->reduced_round_coll = 0;
-}
-
void SHA1DCSetCallback(SHA1_CTX* ctx, collision_block_callback callback)
{
ctx->callback = callback;
diff --git a/sha1dc/sha1.h b/sha1dc/sha1.h
index 1bb0ace99..f126bf63b 100644
--- a/sha1dc/sha1.h
+++ b/sha1dc/sha1.h
@@ -5,29 +5,6 @@
* https://opensource.org/licenses/MIT
***/
-// uses SHA-1 message expansion to expand the first 16 words of W[] to 80 words
-void sha1_message_expansion(uint32_t W[80]);
-
-// sha-1 compression function; first version takes a message block pre-parsed as 16 32-bit integers, second version takes an already expanded message)
-void sha1_compression(uint32_t ihv[5], const uint32_t m[16]);
-void sha1_compression_W(uint32_t ihv[5], const uint32_t W[80]);
-
-// same as sha1_compression_W, but additionally store intermediate states
-// only stores states ii (the state between step ii-1 and step ii) when DOSTORESTATEii is defined in ubc_check.h
-void sha1_compression_states(uint32_t ihv[5], const uint32_t W[80], uint32_t states[80][5]);
-
-// function type for sha1_recompression_step_T (uint32_t ihvin[5], uint32_t ihvout[5], const uint32_t me2[80], const uint32_t state[5])
-// where 0 <= T < 80
-// me2 is an expanded message (the expansion of an original message block XOR'ed with a disturbance vector's message block difference)
-// state is the internal state (a,b,c,d,e) before step T of the SHA-1 compression function while processing the original message block
-// the function will return:
-// ihvin: the reconstructed input chaining value
-// ihvout: the reconstructed output chaining value
-typedef void(*sha1_recompression_type)(uint32_t*, uint32_t*, const uint32_t*, const uint32_t*);
-
-// table of sha1_recompression_step_0, ... , sha1_recompression_step_79
-extern sha1_recompression_type sha1_recompression_step[80];
-
// a callback function type that can be set to be called when a collision block has been found:
// void collision_block_callback(uint64_t byteoffset, const uint32_t ihvin1[5], const uint32_t ihvin2[5], const uint32_t m1[80], const uint32_t m2[80])
typedef void(*collision_block_callback)(uint64_t, const uint32_t*, const uint32_t*, const uint32_t*, const uint32_t*);
@@ -39,7 +16,6 @@ typedef struct {
unsigned char buffer[64];
int bigendian;
int found_collision;
- int safe_hash;
int detect_coll;
int ubc_check;
int reduced_round_coll;
--
2.12.0.4.g94589516d
^ permalink raw reply related
* Re: format-patch subject-prefix gets truncated when using the --numbered flag
From: Jeff King @ 2017-02-28 22:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Adrian Dudau, git@vger.kernel.org
In-Reply-To: <xmqqzih6jejg.fsf@gitster.mtv.corp.google.com>
On Tue, Feb 28, 2017 at 11:33:55AM -0800, Junio C Hamano wrote:
> And having said all that, if we really want to allow overlong
> subject prefix that would end up hiding the real title of the patch,
> a modern way to do so would be to use xstrfmt() like the attached
> toy-patch does.
If you are going to keep the ownership inside this function via statics,
you should use a strbuf. That lets you avoid reallocating on each entry
(and we call this once per commit in a traversal, though see below).
> Note that this is merely a small first step---you'd
> notice that "subject" is kept around as a "static" and only freed
> upon entry to this function for the second time, to preserve the
> ownership model of the original code. In a real "fix" (if this
> needs to be "fixed", that is), I think the ownership model of the
> storage used for *subject_p and *extra_headers_p needs to be updated
> so that it will become caller's responsibility to free them
> (similarly, the ownership model of opt->diffopt.stat_sep that is
> assigned the address of the static buffer[] in the same function
> needs to be revisited).
I agree the ownership model is ugly, and would be nicer if the caller
passed in a strbuf to write into (or a pointer out-parameter, I guess,
but I think strbufs make the ownership semantics much more obvious).
I would just worry about allocation overhead, but that is probably an
overblown concern for format-patch. It tends to be called on a much
smaller set of commits, and it is generally used with "-p", which
creates a lot of overhead already.
-Peff
^ permalink raw reply
* Re: Typesafer git hash patch
From: Linus Torvalds @ 2017-02-28 20:38 UTC (permalink / raw)
To: brian m. carlson, Junio C Hamano, Linus Torvalds,
Git Mailing List
In-Reply-To: <20170228201918.dpqrrijibdapemaf@genre.crustytoothpaste.net>
On Tue, Feb 28, 2017 at 12:19 PM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> The bigger issue is the assumptions in the code base that assume a given
> hash size.
Absolutely. And I think those are going to be the "real" patches.
I actually saw your status report about
"After another 27 commits, I've got it down from 1244 to 1119."
when I had just started, and I applauded the approach, but the numbers
made me go "ugh, that sounds painful".
What I actually wanted to do was to write a coccinelle script to
automate it, but it wasn't entirely obvious, and I started out just
doing it with "sed" and hand-fixing, and it got tedious byt then I had
done X% and just kept going.
I think an automated script that also guarantees that no code
generation could possibly change would have been lovely. We do that
over the kernel occasionally, where those kinds of patches that are
four thousand insertions/deletions in size are called "last Tuesday".
So my approach was really just brute-force and stupid, I admit it. I'm
not proud of the patch either.
It might be one of those "bandaid removal moments" - get it over and
done with and just rip that thing off ;)
So the patch is
216 files changed, 4174 insertions(+), 4080 deletions(-)
and about 27k lines and 950kB total size.
The list limits are apparently much lower than that, but I'll happily
send it to people in private.
Linus
^ permalink raw reply
* Re: SHA1 collisions found
From: Linus Torvalds @ 2017-02-28 23:11 UTC (permalink / raw)
To: Marc Stevens
Cc: Dan Shumow, Junio C Hamano, Jeff King, Joey Hess,
Git Mailing List
In-Reply-To: <259cc328-13f7-2d2d-c86f-8b4fc7da8e34@cwi.nl>
On Tue, Feb 28, 2017 at 2:50 PM, Marc Stevens <marc.stevens@cwi.nl> wrote:
>
> Because we only have 32 disturbance vectors to check, we have DVMASKSIZE
> equal to 1 and maski always 0.
> In the more general case when we add disturbance vectors this will not
> remain the case.
Ok, I didn't get why that happened, but it makes sense to me now.
> Of course for dedicated code this can be simplified, and some parts
> could be further optimized.
So I'd be worried about changing your tested code too much, since the
only test-cases we have are the two pdf files. If we screw up too
much, those will no longer show as collisions, but we could get tons
of false positives that we wouldn't see, so..
I'm wondering that since the disturbance vector cases themselves are a
fairly limited number, perhaps the code that generates this could be
changed to actually just generate the static calls rather than the
loop over the sha1_dvs[] array.
IOW, instead of generating this:
for (i = 0; sha1_dvs[i].dvType != 0; ++i) {
.. use sha1_dvs[i] values as indexes into function arrays etc..
}
maybe you could just make the generator generate that loop statically,
and have 32 function calls with the masks as constant arguments.
.. together with only generating the SHA1_RECOMPRESS() functions for
the cases that are actually used.
So it would still be entirely generated, but it would generate a
little bit more explicit code.
Of course, we could just edit out all the SHA_RECOMPRESS(x) cases by
hand, and only leave the two that are actually used.
As it is, the lib/sha1.c code generates about 250kB of code that is
never used if I read the code correctly (that's just the sha1.c code -
entirely ignoring all the avx2 etc versions that I haven't looked at,
and that I don't think git would use)
> Regarding the recompression functions, the ones needed are given in the
> sha1_dvs table,
> but also via preprocessor defines that are used to actually only store
> needed internal states:
> #define DOSTORESTATE58
> #define DOSTORESTATE65
Yeah, I guess we could use those #define's to cull the code "automatically".
But I think you could do it at the generation phase easily too, so
that we don't then introduce unnecessary differences when we try to
get rid of the extra fat ;)
> Note that as each disturbance vector has its own unique message differences
> (leading to different values for ctx->m2), our code does not loop over
> just 2 items.
> It loops over 32 distinct computations which have either of the 2
> starting points.
Yes, I already had to clarify my badly expressed writing on the git
list. My concerns were about the (very much not obvious) limited
values in the dvs array.
So the code superficially *looks* like it uses all those functions you
generate (and the maski value _looked_ like it was interesting), but
when looking closer it turns out that there's just a two different
function calls that it loops over (but it loops over them multiple
times, I agree).
Linus
^ permalink raw reply
* Re: [PATCH 0/6] Use time_t
From: Junio C Hamano @ 2017-02-28 22:27 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20170228200145.ymbqmxwrbbrwagks@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... We can certainly stick with it for now (it's awkward if you
> really do have an entry on Jan 1 1970, but other than that it's an OK
> marker). I agree that the most negatively value is probably a saner
> choice, but we can switch to it after the dust settles.
I was trying to suggest that we should strive to switch to the most
negative or whatever the most implausible value in the new range
(and leave it as a possible bug to be fixed if we missed a place
that still used "0 is impossible") while doing the ulong to time_t
(or timestamp_t that is i64).
"safer in the short term" wasn't meant to be "let's not spend time
to do quality work". As long as we are switching, we should follow
it through.
>> But we need to cross the bridge to signed timestamp sometime, and I
>> do not see any reason why that somtime should not be now.
>
> Yep.
>
> -Peff
^ permalink raw reply
* Re: Typesafer git hash patch
From: brian m. carlson @ 2017-02-28 20:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <xmqqvarujdmv.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 3410 bytes --]
On Tue, Feb 28, 2017 at 11:53:28AM -0800, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > That
> > actually can clean up some code, because right now we have duplicate
> > interfaces for some things that take an oid pointer and others take a
> > "const unsigned char *sha1", and that duplication essentially can go
> > away.
>
> ... I understand that this is an eventual goal of "struct object_id"
> effort.
Yes, it is.
> > ..., and the lines are generally shorter, eg
> >
> > - const unsigned char *mb = result->item->object.oid.hash;
> > - if (!hashcmp(mb, current_bad_oid->hash)) {
> >
> > turns into
> >
> > + const hash_t *mb = &result->item->object.oid;
> > + if (!hashcmp(mb, current_bad_oid)) {
>
> Hmph. I somehow thought the longer term directio for the above code
> would be to turn it into
>
> if (!oidcmp(&result->item->object.oid, ¤t_bad_oid))
>
> instead.
It is. The duplication is temporary. We've actually removed some of
the original SHA-1 functions in favor of the object_id versions.
> Having said all that, I do not offhand see a huge benefit of the
> current layout that has one layer between the hash (i.e. oid.hash)
> and the object name (i.e. oid) over "there is no need for oid.hash;
> oid is just a hash", which you seem to be doing.
>
> Except for cases where we need to be prepared to see two or more
> kinds of object names (in which case struct object_id would have an
> enum that tells you if oid.hash is SHA-1 or SHA-3-256), that is. Of
> course we can encode that in hash_t itself (e.g. multihash).
Right, this is indeed a benefit.
The bigger issue is the assumptions in the code base that assume a given
hash size. These assumptions are in a bunch of places, so pretty much
any time you see a number, you end up having to question what it means.
We have a 64 that means 40 (SHA-1 hex) plus 24 other values, for
example.
I've sent various series because I think that's the way that the Git
development community has most welcomed my contributions. I have
another 69 patches in two series (which are as of yet untested). I can
probably have almost all of the transition actually complete in another
couple weeks (because I have a day job which does not involve working on
Git).
I've hesitated to send additional patches to the list when my existing
changes haven't even hit next yet, since they're received little actual
testing.
I'm unsure if sending one enormous patch is actually going to be
welcome; I feel Junio and most of the regulars are probably not going to
be very excited about the idea, mostly because the reviewing task is
enormous, and the risk for breakage is also pretty high. We want to
move away from SHA-1 as quickly as possible, yes, but we don't want Git
to become unusably buggy.
I guess what I'm saying is that I'm fine if we want to have a more
aggressive timeline, and I can probably make that happen, at least if we
can wait a few weeks. If Junio and the other regulars are comfortable
merging one omnibus patch to get it moving even more quickly, I won't
stand in the way.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH 0/6] Use time_t
From: Johannes Schindelin @ 2017-02-28 23:10 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, Jeff King, git
In-Reply-To: <6bd6ef64-2749-f17a-aea8-d25b147892c5@web.de>
[-- Attachment #1: Type: text/plain, Size: 3521 bytes --]
Hi René,
On Tue, 28 Feb 2017, René Scharfe wrote:
> Am 28.02.2017 um 21:54 schrieb Johannes Schindelin:
> >
> > On Tue, 28 Feb 2017, Junio C Hamano wrote:
> >
> > > René Scharfe <l.s.r@web.de> writes:
> > >
> > > > Am 28.02.2017 um 15:28 schrieb Jeff King:
> > > >
> > > > > It looks from the discussion like the sanest path forward is our
> > > > > own signed-64bit timestamp_t. That's unfortunate compared to
> > > > > using the standard time_t, but hopefully it would reduce the
> > > > > number of knobs (like TIME_T_IS_INT64) in the long run.
> > > >
> > > > Glibc will get a way to enable 64-bit time_t on 32-bit platforms
> > > > eventually
> > > > (https://sourceware.org/glibc/wiki/Y2038ProofnessDesign). Can
> > > > platforms that won't provide a 64-bit time_t by 2038 be actually
> > > > used at that point? How would we get time information on them?
> > > > How would a custom timestamp_t help us?
> > >
> > > That's a sensible "wait, let's step back a bit". I take it that you
> > > are saying "time_t is just fine", and I am inclined to agree.
> > >
> > > Right now, they may be able to have future timestamps ranging to
> > > year 2100 and switching to time_t would limit their ability to
> > > express future time to 2038 but they would be able to express
> > > timestamp in the past to cover most of 20th century. Given that
> > > these 32-bit time_t software platforms will die off before year 2038
> > > (either by underlying hardware getting obsolete, or software updated
> > > to handle 64-bit time_t), the (temporary) loss of 2038-2100 range
> > > would not be too big a deal to warrant additional complexity.
> >
> > You seem to assume that time_t is required to be signed. But from my
> > understanding that is only guaranteed by POSIX, not by ISO C.
> >
> > We may very well buy ourselves a ton of trouble if we decide to switch
> > to `time_t` rather than to `int64_t`.
>
> True, and time_t doesn't even have to be an integer type. But which
> platforms capable of running git use something else than int32_t or
> int64_t?
That kind of thinking is dangerous. We don't know what platforms are
running Git, and we have a very clear example how we got it very wrong
recently, when we broke building with musl by requiring REG_STARTEND
support [*1*].
So why gamble? If we switch to uint64_t, it would definitely provide the
smoothest upgrade path. It is what the code assumed implicitly when we
broke 32-bit in v2.9.1.
If anybody really wants to support negative timestamps, it should be done
on top of my work. My current patch series does not even start to try to
address the ramifications of negative timestamps (see e.g. the use of
strtoull() for parsing). It is quite unreasonable to ask for such a
fundamental design change when it could very easily be done incrementally
instead, when needed, by someone who needs it.
My work would pave the way for that effort, of course. But this is really
as far as I can go with this patch series, given that I have bigger fish
to fry than to support negative timestamps.
Ciao,
Dscho
Footnote *1*: I still deeply regret deviating from my v1 that did *not*
require REG_STARTEND, but would have kept things working for platforms
without REG_STARTEND by simulating it.
But our thinking was: who would want to run Git in an environment so
ridiculously old that it does not have that clearly useful REG_STARTEND
support? Our answer was "nobody". And it was incorrect.
^ permalink raw reply
* Re: SHA1 collisions found
From: Marc Stevens @ 2017-02-28 22:50 UTC (permalink / raw)
To: Dan Shumow, Linus Torvalds, Junio C Hamano
Cc: Jeff King, Joey Hess, Git Mailing List
In-Reply-To: <CY1PR0301MB21078DDCA8C679983D22821FC4560@CY1PR0301MB2107.namprd03.prod.outlook.com>
You can also keep me in this thread, so we can help or answer any
further questions,
but I also appreciate the feedback on our project.
Like Dan Shumow said, our main focus on the library has been correctness
and then performance.
The entire files ubc_check.c and ubc_check.h are generated based on
cryptanalytic data,
in particular a list of 32 disturbance vectors and their unavoidable
attack conditions,
see https://github.com/cr-marcstevens/sha1collisiondetection-tools.
sha1.c and sha1.h were coded to work for any such generated ubc_check.c
and ubc_check.h.
That means that indeed we might have some superfluous code, once used
for testing, or for generality,
but nothing that should noticeably impact runtime performance.
Because we only have 32 disturbance vectors to check, we have DVMASKSIZE
equal to 1 and maski always 0.
In the more general case when we add disturbance vectors this will not
remain the case.
Of course for dedicated code this can be simplified, and some parts
could be further optimized.
Regarding the recompression functions, the ones needed are given in the
sha1_dvs table,
but also via preprocessor defines that are used to actually only store
needed internal states:
#define DOSTORESTATE58
#define DOSTORESTATE65
For each disturbance vector there is a window of which states you can
start the recompression from,
we've optimized it so there are only 2 unique starting points (58,65)
instead of 32.
These defines should be easy to use to remove superfluous compiled
recompression functions.
Note that as each disturbance vector has its own unique message differences
(leading to different values for ctx->m2), our code does not loop over
just 2 items.
It loops over 32 distinct computations which have either of the 2
starting points.
Finally, thanks for taking a close look at our code,
this helps bringing the library in better shape also for other software
projects.
Best regards,
Marc Stevens
On 2/28/2017 10:22 PM, Dan Shumow wrote:
> [Responses inline]
>
> No need to keep me "bcc'd" (though thanks for the consideration) -- I'm happy to ignore anything I don't want to be pulled into ;-)
>
> Here's a rollup of what needs to be done based on the discussion below:
>
> 1) Remove extraneous exports from sha1.h
> 2) Remove "safe mode" support.
> 3) Remove sha1_compression_W if it is not needed by the performance improvements.
> 4) Evaluate logic around storing states and generating recompression states. Remove defines that bloat code footprint.
>
> Thanks,
> Dan
>
>
> -----Original Message-----
> From: linus971@gmail.com [mailto:linus971@gmail.com] On Behalf Of Linus Torvalds
> Sent: Tuesday, February 28, 2017 11:34 AM
> To: Junio C Hamano <gitster@pobox.com>
> Cc: Jeff King <peff@peff.net>; Joey Hess <id@joeyh.name>; Git Mailing List <git@vger.kernel.org>
> Subject: Re: SHA1 collisions found
>
> On Tue, Feb 28, 2017 at 11:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> In a way similar to 8415558f55 ("sha1dc: avoid c99
>> declaration-after-statement", 2017-02-24), we would want this on top.
> There's a few other simplifications that could be done:
>
> (1) make the symbols static that aren't used.
>
> The sha1.h header ends up declaring several things that shouldn't have been exported.
>
> I suspect the code may have had some debug mode that got stripped out from it before making it public (or that was never there, and was just something the generating code could add).
>
> [danshu] Yes, this is reasonable. The emphasis of the code, heretofore, had been the illustration of our unavoidable bit condition performance improvement to counter cryptanalysis. I'm happy to remove the unused stuff from the public header.
>
> (2) get rid of the "safe mode" support.
>
> That one is meant for non-checking replacements where it generates a *different* hash for input with the collision fingerpring, but that's pointless for the git use when we abort on a collision fingerprint.
>
> [danshu] Yes, I agree that if you aren't using this it can be taken out. I believe Marc has some use cases / potentially consumers of this algorithm in mind. We can move it into separate header/source files for anyone who wants to use it.
>
> I think the first one will show that the sha1_compression() function isn't actually used, and with the removal of safe-mode I think
> sha1_compression_W() also is unused.
>
> [danshu] Some of the performance experiments that I've looked at involve putting the sha1_compression_W(...) back in. Though, that doesn't look like it's helping. If it is unused after the performance improvements, we'll take it out, or move it into its own file.
>
> Finally, only states 58 and 65 (out of all 80 states) are actually used, and from what I can tell, the 'maski' value is always 0, so the looping over 80 state masks is really just a loop over two.
>
> [danshu] So, while looking at performance optimizations, I specifically looked at how much removing storing the intermediate states helps -- And I found that it doesn't seem to make a difference for performance. My cursory hypothesis is because nothing is waiting on those writes to memory, the code moves on quickly. That said, it is a bunch of code that is essentially doing nothing and removing that is worthwhile. Though, partially what we're seeing here is that, as you point out below, we're working with generated code that we want to be general. Specifically, right now, we're checking only disturbance vectors that we know can be used to efficiently attack the compression function. It may be the case that further cryptanalysis uncovers more. We want to have a general enough approach that we can add scanning for new disturbance vectors if they're found later. Over specializing the code makes that more difficult, as currently the algorithm is data driven, and we don't need to write new code, but rather just add more data to check. One other note -- the "maski" field of the dv_info_t struct is not an index to check the state, but rather an index into the mask generated by the ubc check code, so that doesn't pertain to looping over the states. More on this below.
>
> The file has code top *generate* all the 80 sha1_recompression_step() functions, and I don't think the compiler is smart enough to notice that only two of them matter.
>
> [danshu] That's a good observation -- We should clean up the unused recompression steps, especially because that will generate a ton of object code. We should add some logic to only compile the functions that are used.
>
> And because 'maski' is always zero, thisL
>
> ubc_dv_mask[sha1_dvs[i].maski]
>
> code looks like it might as well just use ubc_dv_mask[0] - in fact the ubc_dv_mask[] "array" really is just a single-entry array anyway:
>
> #define DVMASKSIZE 1
>
> [danshu] The idea here is that we are currently checking 32 disturbance vectors with our bit mask. We're checking 32 DVs, because we have 32 bits of mask that we can use. The DVs are ordered by their probability of leading to an attack (which is directly correlated to the complexity of finding a collision.) Several of those DVs correspond to very low probability / high cost attacks, which we wouldn't expect to see in practice. We just have the space to check, so why not? However, improvements in cryptanalysis may make those attacks cheaper, in which case, we would potentially want to add more DVs to check, in which case we would expand the number of DVs and the mask.
>
> so that code has a few oddities in it. It's generated code, which is probably why.
>
> [danshu] Accurate, we're also just trying to be general enough that we can easily add more DVs later if need be. I don't know how likely that is, certainly the DVs that we're checking now are based on solid conjectures and rigorous analysis of the problem. Though we don't want to rule out that there will be subsequent cryptanalytic developments later. Marc can comment more here.
>
> Basically, some of it could be improved. In particular, the "generate code for 80 different recompression cases, but only ever use two of them" really looks like it would blow up the code generation footprint a lot.
>
> I'm adding Marc Stevens and Dan Shumow to this email (bcc'd, so that they don't get dragged into any unrelated email threads) in case they want to comment.
>
> I'm wondering if they perhaps have a cleaned-up version somewhere, or maybe they can tell me that I'm just full of sh*t and missed something.
>
> [danshu] Naw man, it looks pretty good, modulo a little bit of understandable confusion over 'maski' -- No fake news or alternative facts here ;-)
>
> Linus
^ 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