* [PATCH 2/3] object-store: factor out odb_clear_loose_cache()
From: René Scharfe @ 2019-01-06 16:45 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason
In-Reply-To: <3512c798-aa42-6fba-ee82-d33a8985be91@web.de>
Add and use a function for emptying the loose object cache, so callers
don't have to know any of its implementation details.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
object-store.h | 3 +++
object.c | 2 +-
packfile.c | 7 ++-----
sha1-file.c | 7 +++++++
4 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/object-store.h b/object-store.h
index 7236c571c0..709bf856b6 100644
--- a/object-store.h
+++ b/object-store.h
@@ -61,6 +61,9 @@ void odb_load_loose_cache(struct object_directory *odb, int subdir_nr);
struct oid_array *odb_loose_cache(struct object_directory *odb,
const struct object_id *oid);
+/* Empty the loose object cache for the specified object directory. */
+void odb_clear_loose_cache(struct object_directory *odb);
+
struct packed_git {
struct packed_git *next;
struct list_head mru;
diff --git a/object.c b/object.c
index 79d636091c..a5c5cf830f 100644
--- a/object.c
+++ b/object.c
@@ -485,7 +485,7 @@ struct raw_object_store *raw_object_store_new(void)
static void free_object_directory(struct object_directory *odb)
{
free(odb->path);
- oid_array_clear(&odb->loose_objects_cache);
+ odb_clear_loose_cache(odb);
free(odb);
}
diff --git a/packfile.c b/packfile.c
index 8c6b47cc77..0fe9c21bf1 100644
--- a/packfile.c
+++ b/packfile.c
@@ -994,11 +994,8 @@ void reprepare_packed_git(struct repository *r)
{
struct object_directory *odb;
- for (odb = r->objects->odb; odb; odb = odb->next) {
- oid_array_clear(&odb->loose_objects_cache);
- memset(&odb->loose_objects_subdir_seen, 0,
- sizeof(odb->loose_objects_subdir_seen));
- }
+ for (odb = r->objects->odb; odb; odb = odb->next)
+ odb_clear_loose_cache(odb);
r->objects->approximate_object_count_valid = 0;
r->objects->packed_git_initialized = 0;
diff --git a/sha1-file.c b/sha1-file.c
index cb8583b634..2f965b2688 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -2178,6 +2178,13 @@ void odb_load_loose_cache(struct object_directory *odb, int subdir_nr)
strbuf_release(&buf);
}
+void odb_clear_loose_cache(struct object_directory *odb)
+{
+ oid_array_clear(&odb->loose_objects_cache);
+ memset(&odb->loose_objects_subdir_seen, 0,
+ sizeof(odb->loose_objects_subdir_seen));
+}
+
static int check_stream_sha1(git_zstream *stream,
const char *hdr,
unsigned long size,
--
2.20.1
^ permalink raw reply related
* [PATCH 3/3] object-store: use one oid_array per subdirectory for loose cache
From: René Scharfe @ 2019-01-06 16:45 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason
In-Reply-To: <3512c798-aa42-6fba-ee82-d33a8985be91@web.de>
The loose objects cache is filled one subdirectory at a time as needed.
It is stored in an oid_array, which has to be resorted after each add
operation. So when querying a wide range of objects, the partially
filled array needs to be resorted up to 255 times, which takes over 100
times longer than sorting once.
Use one oid_array for each subdirectory. This ensures that entries have
to only be sorted a single time. It also avoids eight binary search
steps for each cache lookup as a small bonus.
The cache is used for collision checks for the log placeholders %h, %t
and %p, and we can see the change speeding them up in a repository with
ca. 100 objects per subdirectory:
$ git count-objects
26733 objects, 68808 kilobytes
Test HEAD^ HEAD
--------------------------------------------------------------------
4205.1: log with %H 0.51(0.47+0.04) 0.51(0.49+0.02) +0.0%
4205.2: log with %h 0.84(0.82+0.02) 0.60(0.57+0.03) -28.6%
4205.3: log with %T 0.53(0.49+0.04) 0.52(0.48+0.03) -1.9%
4205.4: log with %t 0.84(0.80+0.04) 0.60(0.59+0.01) -28.6%
4205.5: log with %P 0.52(0.48+0.03) 0.51(0.50+0.01) -1.9%
4205.6: log with %p 0.85(0.78+0.06) 0.61(0.56+0.05) -28.2%
4205.7: log with %h-%h-%h 0.96(0.92+0.03) 0.69(0.64+0.04) -28.1%
Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
object-store.h | 2 +-
sha1-file.c | 9 ++++++---
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/object-store.h b/object-store.h
index 709bf856b6..2fb6c0e4db 100644
--- a/object-store.h
+++ b/object-store.h
@@ -20,7 +20,7 @@ struct object_directory {
* Be sure to call odb_load_loose_cache() before using.
*/
char loose_objects_subdir_seen[256];
- struct oid_array loose_objects_cache;
+ struct oid_array loose_objects_cache[256];
/*
* Path to the alternative object store. If this is a relative path,
diff --git a/sha1-file.c b/sha1-file.c
index 2f965b2688..c3c6e50704 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -2155,7 +2155,7 @@ struct oid_array *odb_loose_cache(struct object_directory *odb,
{
int subdir_nr = oid->hash[0];
odb_load_loose_cache(odb, subdir_nr);
- return &odb->loose_objects_cache;
+ return &odb->loose_objects_cache[subdir_nr];
}
void odb_load_loose_cache(struct object_directory *odb, int subdir_nr)
@@ -2173,14 +2173,17 @@ void odb_load_loose_cache(struct object_directory *odb, int subdir_nr)
for_each_file_in_obj_subdir(subdir_nr, &buf,
append_loose_object,
NULL, NULL,
- &odb->loose_objects_cache);
+ &odb->loose_objects_cache[subdir_nr]);
odb->loose_objects_subdir_seen[subdir_nr] = 1;
strbuf_release(&buf);
}
void odb_clear_loose_cache(struct object_directory *odb)
{
- oid_array_clear(&odb->loose_objects_cache);
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(odb->loose_objects_cache); i++)
+ oid_array_clear(&odb->loose_objects_cache[i]);
memset(&odb->loose_objects_subdir_seen, 0,
sizeof(odb->loose_objects_subdir_seen));
}
--
2.20.1
^ permalink raw reply related
* [PATCH 2/1] Revert "t/lib-git-daemon: record daemon log"
From: Thomas Gummerer @ 2019-01-06 17:53 UTC (permalink / raw)
To: Jeff King
Cc: Torsten Bögershausen, Git Mailing List, szeder.dev,
Jan Palus, Johannes Schindelin
In-Reply-To: <20181220164150.GB25639@hank.intra.tgummerer.com>
This reverts commit 314a73d658 (t/lib-git-daemon: record daemon log,
2018-01-25), which let tests use the output of git-daemon.
The previous commit removed the last user of deamon.log in the tests,
there's no good way to make checking for output in the log
race-proof. Revert this commit as well, to make sure others are not
tempted to use daemon.log in tests in the future, which would lead to
racy tests.
The original commit had one change that still makes sense, namely
switching read/echo for "read -r" and "printf", which relays the data
more faithfully. Don't revert that piece here, as it is still a
useful change.
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
t/lib-git-daemon.sh | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index f98de95c15..fd41229a8f 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -54,19 +54,11 @@ start_git_daemon() {
"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
>&3 2>git_daemon_output &
GIT_DAEMON_PID=$!
- >daemon.log
{
read -r line <&7
- printf "%s\n" "$line"
- printf >&4 "%s\n" "$line"
- (
- while read -r line <&7
- do
- printf "%s\n" "$line"
- printf >&4 "%s\n" "$line"
- done
- ) &
- } 7<git_daemon_output >>"$TRASH_DIRECTORY/daemon.log" &&
+ printf "%s\n" "$line" >&4
+ cat <&7 >&4 &
+ } 7<git_daemon_output &&
# Check expected output
if test x"$(expr "$line" : "\[[0-9]*\] \(.*\)")" != x"Ready to rumble"
--
2.20.1.153.gd81d796ee0
^ permalink raw reply related
* Re: [PATCH] t5570: drop racy test
From: Thomas Gummerer @ 2019-01-06 17:59 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, Torsten Bögershausen, Git Mailing List,
szeder.dev, Jan Palus
In-Reply-To: <nycvar.QRO.7.76.6.1812211638180.41@tvgsbejvaqbjf.bet>
On 12/21, Johannes Schindelin wrote:
> Hi Thomas & Peff,
>
> On Thu, 20 Dec 2018, Jeff King wrote:
>
> > On Thu, Dec 20, 2018 at 04:41:50PM +0000, Thomas Gummerer wrote:
> >
> > > Dscho also mentioned on #git-devel a while ago that he may have a look
> > > at actually making this test race-proof, but I guess he's been busy
> > > with the 2.20 release.
>
> And GitGitGadget. And working on the Azure Pipelines support. And
> mentoring two interns.
>
> This is what I still have in my internal ticket:
>
> Try to work around occasional t5570 failures in Git's test suite
>
> Seems that there is a race condition in
> https://github.com/git/git/blob/master/t/lib-git-daemon.sh#L48-L69
> that could possibly be solved by writing to the daemon.log
> directly, and showing the output only via `tail -f` (and only when
> running in verbose mode, as it simply won't make sense otherwise).
>
> However, if the preferred route is to go ahead and just remove that test
> altogether, I'm fine with that, too.
Right, I was of course completely unaware of that internal ticket. If
you still want to go that way there are certainly no objections from
me. I just want to make sure not more users run into this racyness,
and I guess you also may have more important/interesting things to
work on.
> The only reason, in my mind, why we still have `git-daemon` is that it
> allows for easy standing up your own Git server, e.g. as an ad-hoc way to
> collaborate in a small ad-hoc team. If we ever get to the point where we
> can stand up a minimal HTTP/HTTPS server with an internal Git command (not
> requiring sysadmin privileges), from my point of view `git-daemon` can
> even go the way of the Kale Island (but for much better reasons [*1*]).
>
> > > I'm also not sure it's worth spending a lot of time trying to fix this
> > > test, but I'd definitely be happy if someone proposes a different
> > > solution.
> >
> > Yeah. I'm sure it's fixable with enough effort, but I just think there
> > are more interesting and important things to work on.
> >
> > > --- >8 ---
> > > Subject: [PATCH] t5570: drop racy test
> >
> > So yeah, I'm still fine with this. But...
> >
> > > ---
> > > t/t5570-git-daemon.sh | 13 -------------
> > > 1 file changed, 13 deletions(-)
> >
> > This is the only user of daemon.log, so we could drop those bits from
> > lib-git-daemon.sh, too. That would also prevent people from adding new
> > tests, thinking that this was somehow not horribly racy). I.e.,
> > reverting 314a73d658 (t/lib-git-daemon: record daemon log, 2018-01-25).
Right that makes sense. I sent that as patch 2/1, but I'm happy to
squash those into one if that's preferred.
> Indeed, that would be good.
>
> The only reason to keep daemon.log that I can think of is to make
> debugging easier, but then, if it should become necessary, it is probably
> easier to freopen() stdout or stderr into a file in `git daemon`, anyway.
We do still print the output when tests are run in verbose mode, which
should be just as good as having the log in a separate file in most
cases I suspect.
> Ciao,
> Dscho
>
> Footnote *1*: Kale Island, along with Rapita, Rehana, Kakatina and Zollies
> is prominently featured in a scientific article at
> http://iopscience.iop.org/article/10.1088/1748-9326/11/5/054011 that is on
> my "important papers I read in 2018" list.
^ permalink raw reply
* Re: [PATCH] doc/config: do a better job of introducing 'worktree.guessRemote'
From: Thomas Gummerer @ 2019-01-06 18:09 UTC (permalink / raw)
To: Eric Sunshine
Cc: git,
Олег Самойлов
In-Reply-To: <20181223192435.24803-1-sunshine@sunshineco.com>
On 12/23, Eric Sunshine wrote:
> The documentation for this option jumps right in with "With `add`",
> without explaining that `add` is a sub-command of "git worktree".
> Together with rather odd grammatical structure of the remainder of the
> sentence, the description can be difficult for newcomers to understand.
> Clarify by improving the grammar and mentioning "git worktree add"
> explicitly.
>
> Reported-by: Олег Самойлов <splarv@ya.ru>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
Thanks, this reads much better indeed. I was briefly wondering if a
similar change is needed in the documentation for the 'git worktree'
command itself. It currently reads:
With `worktree add <path>`, without `<commit-ish>`, instead
of creating a new branch from HEAD, if there exists a tracking
branch in exactly one remote matching the basename of `<path>`,
base the new branch on the remote-tracking branch, and mark
the remote-tracking branch as "upstream" from the new branch.
I do think the documentation for the config option is slightly easier
to read, especially with your improvements below. Dunno if it's worth
adjusting the test in the 'git worktree' documentation as well?
> Reference: https://public-inbox.org/git/0E640233-B2CB-465D-9713-BBECE331CA80@ya.ru/
>
> Documentation/config/worktree.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config/worktree.txt b/Documentation/config/worktree.txt
> index b853798fc2..048e349482 100644
> --- a/Documentation/config/worktree.txt
> +++ b/Documentation/config/worktree.txt
> @@ -1,6 +1,6 @@
> worktree.guessRemote::
> - With `add`, if no branch argument, and neither of `-b` nor
> - `-B` nor `--detach` are given, the command defaults to
> + If no branch is specified and neither `-b` nor `-B` nor
> + `--detach` is used, then `git worktree add` defaults to
> creating a new branch from HEAD. If `worktree.guessRemote` is
> set to true, `worktree add` tries to find a remote-tracking
> branch whose name uniquely matches the new branch name. If
> --
> 2.20.1.415.g653613c723
>
^ permalink raw reply
* Re: [PATCH] config.mak.dev: add -Wformat
From: Thomas Gummerer @ 2019-01-06 18:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, Jeff King, git, Josh Steadmon, Masaya Suzuki
In-Reply-To: <xmqqa7khisue.fsf@gitster-ct.c.googlers.com>
On 01/03, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
> > In October, Thomas Gummerer wrote:
> >> On 10/12, Jonathan Nieder wrote:
> >>> Jeff King wrote:
> >>> ...
> >>>> -Wformat is part of -Wall, which we already turn on by default (even for
> >>>> non-developer builds).
> > ...
> > As discussed in [1], autoconf appears to not put -Wall in CFLAGS:
> >
> > $ make configure
> > GEN configure
> > $ ./configure
> > [...]
> > config.status: creating config.mak.autogen
> > config.status: executing config.mak.autogen commands
> > $ grep CFLAGS config.mak.autogen
> > CFLAGS = -g -O2
> > PTHREAD_CFLAGS=-pthread
> >
> > So this trap for the unwary is still around.
> >
> > Can we revive this patch? Does it just need a clearer commit message,
> > or were there other objections?
>
> I think it is a good idea to give fallback/redundancy for this
> case. I do not have strong opinion between -Wall and -Wformat,
> but I'd probably vote for the former if pressed.
Just catching up after some time off over Christmas, thanks for tying
this up!
I agree with the choice of adding -Wall to the CFLAGS here, so even if
it is not added to the CFLAGS generated by autoconf (or in mnually set
up CFLAGS such as in my original case), we still get a complete set of
warnings when DEVELOPER=YesPlease is set.
> -- >8 --
> From: Thomas Gummerer <t.gummerer@gmail.com>
> Date: Fri, 12 Oct 2018 19:40:37 +0100
> Subject: [PATCH] config.mak.dev: add -Wformat
>
> 801fa63a90 ("config.mak.dev: add -Wformat-security", 2018-09-08)
> added the "-Wformat-security" to the flags set in config.mak.dev.
> In the gcc man page this is documented as:
>
> If -Wformat is specified, also warn about uses of format
> functions that represent possible security problems. [...]
>
> The commit did however not add the "-Wformat" flag, but instead
> relied on the fact that "-Wall" is set in the Makefile by default
> and that "-Wformat" is part of "-Wall".
>
> Unfortunately, those who use config.mak.autogen generated with the
> autoconf to configure toolchain do *not* get "-Wall" in their CFLAGS
> and the added -Wformat-security had no effect. Worse yet, newer
> versions of gcc (gcc 8.2.1 in this particular case) warn about the
> lack of "-Wformat" and thus compilation fails only with this option
> set.
>
> We could fix it by adding "-Wformat", but in general we do want all
> checks included in "-Wall", so let's add it to config.mak.dev to
> cover more cases.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> Helped-by: Jeff King <peff@peff.net>
> Helped-by: Jonathan Nieder <jrnieder@gmail.com>
> [jc: s/-Wformat/-Wall/]
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> config.mak.dev | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/config.mak.dev b/config.mak.dev
> index bfbd3df4e8..74337f1f92 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -1,6 +1,7 @@
> ifeq ($(filter no-error,$(DEVOPTS)),)
> CFLAGS += -Werror
> endif
> +CFLAGS += -Wall
> CFLAGS += -Wdeclaration-after-statement
> CFLAGS += -Wformat-security
> CFLAGS += -Wno-format-zero-length
> --
> 2.20.1-2-gb21ebb671b
>
^ permalink raw reply
* Re: [PATCH v2 7/8] checkout: introduce --{,no-}overlay option
From: Thomas Gummerer @ 2019-01-06 18:18 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Duy Nguyen, Git Mailing List, Junio C Hamano, Elijah Newren
In-Reply-To: <CAPig+cSOyCQZXiG7sJWb12WzzujM-nsqqpt+cFZTFvXB1+-SVQ@mail.gmail.com>
On 12/23, Eric Sunshine wrote:
> On Sun, Dec 23, 2018 at 3:05 AM Duy Nguyen <pclouds@gmail.com> wrote:
> > On Thu, Dec 20, 2018 at 2:48 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > > +--[no-]overlay::
> > > + In the default overlay mode files `git checkout` never
> >
> > -ECANTPARSE. Maybe "files" should be removed from this line?
>
> Also, add a comma after "mode".
Will do, thanks both.
^ permalink raw reply
* Re: [PATCH] doc/config: do a better job of introducing 'worktree.guessRemote'
From: Eric Sunshine @ 2019-01-06 18:19 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Git List,
Олег Самойлов
In-Reply-To: <20190106180914.GE25639@hank.intra.tgummerer.com>
On Sun, Jan 6, 2019 at 1:09 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> On 12/23, Eric Sunshine wrote:
> > The documentation for this option jumps right in with "With `add`",
> > without explaining that `add` is a sub-command of "git worktree".
> > Together with rather odd grammatical structure of the remainder of the
> > sentence, the description can be difficult for newcomers to understand.
> > Clarify by improving the grammar and mentioning "git worktree add"
> > explicitly.
>
> Thanks, this reads much better indeed. I was briefly wondering if a
> similar change is needed in the documentation for the 'git worktree'
> command itself. It currently reads:
>
> With `worktree add <path>`, without `<commit-ish>`, instead
> of creating a new branch from HEAD, if there exists a tracking
> branch in exactly one remote matching the basename of `<path>`,
> base the new branch on the remote-tracking branch, and mark
> the remote-tracking branch as "upstream" from the new branch.
>
> I do think the documentation for the config option is slightly easier
> to read, especially with your improvements below. Dunno if it's worth
> adjusting the test in the 'git worktree' documentation as well?
Such a change to git-worktree.txt could be done, though I think it's
outside the scope of this patch since "With ...," is not nearly so
confusing in the context of git-worktree.txt given that the reader
_knows_ that he/she is reading (exclusively) about "git worktree".
Also, almost all of the options in git-worktree.txt are phrased "With
...,", so such a change would be more all-encompassing.
^ permalink raw reply
* Re: [PATCH v2 8/8] checkout: introduce checkout.overlayMode config
From: Thomas Gummerer @ 2019-01-06 18:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Nguyễn Thái Ngọc Duy, Elijah Newren
In-Reply-To: <xmqqo98yiq8i.fsf@gitster-ct.c.googlers.com>
On 01/02, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > In the previous patch we introduced a new no-overlay mode for git
> > checkout. Some users (such as the author of this commit) may want to
> > have this mode turned on by default as it matches their mental model
> > more closely. Make that possible by introducing a new config option
> > to that extend.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> > Documentation/config/checkout.txt | 7 +++++++
> > builtin/checkout.c | 8 +++++++-
> > t/t2025-checkout-no-overlay.sh | 10 ++++++++++
> > 3 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/config/checkout.txt b/Documentation/config/checkout.txt
> > index c4118fa196..53f917e15e 100644
> > --- a/Documentation/config/checkout.txt
> > +++ b/Documentation/config/checkout.txt
> > @@ -21,3 +21,10 @@ checkout.optimizeNewBranch::
> > will not update the skip-worktree bit in the index nor add/remove
> > files in the working directory to reflect the current sparse checkout
> > settings nor will it show the local changes.
> > +
> > +checkout.overlayMode::
> > + In the default overlay mode files `git checkout` never
> > + removes files from the index or the working tree.
>
> Technically the above "never removes" is incorrect.
>
> $ mv COPYING 1 && mkdir COPYING && mv 1 COPYING/COPYING
> $ git add COPYING
> $ git checkout HEAD COPYING
>
> would remove COPYING/1 from the index and from the working tree to
> make room.
Right, that's a case I didn't think about.
> Because I think that a bit of white lie like what you wrote would
> help readers understand the key point of "overlay or not overlay"
> better than an overly precise description of the reason why the
> removal in the above three-liner case is the right thing to do, I
> think the text in the patch is good enough at least for now, but I'd
> mention it in case somebody else can think of a better phrasing to
> covey the same key point without being technically incorrect.
Maybe it would be enough to say "... `git checkout` never removes
files, that are not in the tree being checked out, from the index or
the working tree"? It is more technically correct, but dunno making
the sentence harder to read is worth it.
> Thanks.
^ permalink raw reply
* Re: [PATCH 3/3] object-store: use one oid_array per subdirectory for loose cache
From: Ævar Arnfjörð Bjarmason @ 2019-01-06 20:38 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano, Jeff King
In-Reply-To: <c8dd851f-0a18-848f-8e58-cc0ee5f8e705@web.de>
On Sun, Jan 06 2019, René Scharfe wrote:
Thanks. I haven't done my own performance testing but at a glance this
looks good.
> The cache is used for collision checks for the log placeholders %h, %t
> and %p, and we can see the change speeding them up in a repository with
> ca. 100 objects per subdirectory:
>
> $ git count-objects
> 26733 objects, 68808 kilobytes
>
> Test HEAD^ HEAD
> --------------------------------------------------------------------
> 4205.1: log with %H 0.51(0.47+0.04) 0.51(0.49+0.02) +0.0%
> 4205.2: log with %h 0.84(0.82+0.02) 0.60(0.57+0.03) -28.6%
> 4205.3: log with %T 0.53(0.49+0.04) 0.52(0.48+0.03) -1.9%
> 4205.4: log with %t 0.84(0.80+0.04) 0.60(0.59+0.01) -28.6%
> 4205.5: log with %P 0.52(0.48+0.03) 0.51(0.50+0.01) -1.9%
> 4205.6: log with %p 0.85(0.78+0.06) 0.61(0.56+0.05) -28.2%
> 4205.7: log with %h-%h-%h 0.96(0.92+0.03) 0.69(0.64+0.04) -28.1%
Can you elaborate on the test setup required to get to the point where
you got these numbers for subsequent comparison, i.e. how you generated
the approx 100 objects per dir, what OS/version & storage type etc.
^ permalink raw reply
* Re: [PATCH v12 21/26] stash: optimize `get_untracked_files()` and `check_changes()`
From: Thomas Gummerer @ 2019-01-06 22:47 UTC (permalink / raw)
To: Paul-Sebastian Ungureanu; +Cc: git, Johannes.Schindelin
In-Reply-To: <17f45e4bb48bdf9588d87c7b134afd703dbd69e6.1545331726.git.ungureanupaulsebastian@gmail.com>
On 12/20, Paul-Sebastian Ungureanu wrote:
> This commits introduces a optimization by avoiding calling the
> same functions again. For example, `git stash push -u`
> would call at some points the following functions:
>
> * `check_changes()` (inside `do_push_stash()`)
> * `do_create_stash()`, which calls: `check_changes()` and
> `get_untracked_files()`
>
> Note that `check_changes()` also calls `get_untracked_files()`.
> So, `check_changes()` is called 2 times and `get_untracked_files()`
> 3 times.
>
> The old function `check_changes()` now consists of two functions:
> `get_untracked_files()` and `check_changes_tracked_files()`.
>
> These are the call chains for `push` and `create`:
>
> * `push_stash()` -> `do_push_stash()` -> `do_create_stash()`
>
> * `create_stash()` -> `do_create_stash()`
>
> To prevent calling the same functions over and over again,
> `check_changes()` inside `do_create_stash()` is now placed
> in the caller functions (`create_stash()` and `do_push_stash()`).
> This way `check_changes()` and `get_untracked files()` are called
> only one time.
>
> https://public-inbox.org/git/20180818223329.GJ11326@hank.intra.tgummerer.com/
I missed this the other time, but having this link in the commit
message is unnecessary, and it may go stale (though in this case it
includes the Message-ID, so it is still useful as long as some archive
exists. The commit message explains well enough what this change is
doing, even without the link to the review.
Sorry I missed these things in the earlier rounds somehow.
> Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
> ---
> builtin/stash--helper.c | 53 +++++++++++++++++++++++------------------
> 1 file changed, 30 insertions(+), 23 deletions(-)
>
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 19ead63c46..4b63352927 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -884,18 +884,18 @@ static int get_untracked_files(struct pathspec ps, int include_untracked,
> }
>
> /*
> - * The return value of `check_changes()` can be:
> + * The return value of `check_changes_tracked_files()` can be:
> *
> * < 0 if there was an error
> * = 0 if there are no changes.
> * > 0 if there are changes.
> */
> -static int check_changes(struct pathspec ps, int include_untracked)
> +
Unnecessary blank line after the comment here.
> +static int check_changes_tracked_files(struct pathspec ps)
> {
> int result;
> struct rev_info rev;
> struct object_id dummy;
> - struct strbuf out = STRBUF_INIT;
>
> /* No initial commit. */
> if (get_oid("HEAD", &dummy))
> @@ -923,14 +923,26 @@ static int check_changes(struct pathspec ps, int include_untracked)
> if (diff_result_code(&rev.diffopt, result))
> return 1;
>
> + return 0;
> +}
> +
> +/*
> + * The function will fill `untracked_files` with the names of untracked files
> + * It will return 1 if there were any changes and 0 if there were not.
> + */
> +
Unnecessary blank line.
> +static int check_changes(struct pathspec ps, int include_untracked,
> + struct strbuf *untracked_files)
> +{
> + int ret = 0;
> + if (check_changes_tracked_files(ps))
> + ret = 1;
> +
> if (include_untracked && get_untracked_files(ps, include_untracked,
> - &out)) {
> - strbuf_release(&out);
> - return 1;
> - }
> + untracked_files))
> + ret = 1;
>
> - strbuf_release(&out);
> - return 0;
> + return ret;
> }
>
> static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
> @@ -1141,7 +1153,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,
> head_commit = lookup_commit(the_repository, &info->b_commit);
> }
>
> - if (!check_changes(ps, include_untracked)) {
> + if (!check_changes(ps, include_untracked, &untracked_files)) {
> ret = 1;
> goto done;
> }
> @@ -1166,8 +1178,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,
> goto done;
> }
>
> - if (include_untracked && get_untracked_files(ps, include_untracked,
> - &untracked_files)) {
> + if (include_untracked) {
> if (save_untracked_files(info, &msg, untracked_files)) {
> if (!quiet)
> fprintf_ln(stderr, _("Cannot save "
> @@ -1252,20 +1263,15 @@ static int create_stash(int argc, const char **argv, const char *prefix)
> 0);
>
> memset(&ps, 0, sizeof(ps));
> - strbuf_addstr(&stash_msg_buf, stash_msg);
> - ret = do_create_stash(ps, &stash_msg_buf, include_untracked, 0, &info,
> - NULL, 0);
> + if (!check_changes_tracked_files(ps))
> + return 0;
>
> - if (!ret)
> + strbuf_addstr(&stash_msg_buf, stash_msg);
> + if (!(ret = do_create_stash(ps, &stash_msg_buf, 0, 0, &info, NULL, 0)))
It's a bit odd to have do_create_stash moved inside the if here, even
though it was introduced in this patch series. It makes the patch a
bit more noisy and harder to see what was changed here.
> printf_ln("%s", oid_to_hex(&info.w_commit));
>
> strbuf_release(&stash_msg_buf);
> -
> - /*
> - * ret can be 1 if there were no changes. In this case, we should
> - * not error out.
> - */
> - return ret < 0;
> + return ret;
> }
>
> static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
> @@ -1275,6 +1281,7 @@ static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
> struct stash_info info;
> struct strbuf patch = STRBUF_INIT;
> struct strbuf stash_msg_buf = STRBUF_INIT;
> + struct strbuf untracked_files = STRBUF_INIT;
Does this strbuf also need to be released at the end of the function?
>
> if (patch_mode && keep_index == -1)
> keep_index = 1;
> @@ -1309,7 +1316,7 @@ static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
> goto done;
> }
>
> - if (!check_changes(ps, include_untracked)) {
> + if (!check_changes(ps, include_untracked, &untracked_files)) {
> if (!quiet)
> printf_ln(_("No local changes to save"));
> goto done;
> --
> 2.20.1.441.g764a526393
>
^ permalink raw reply
* "git p4" fails when perforce login not needed
From: Peter Osterlund @ 2019-01-06 22:48 UTC (permalink / raw)
To: git; +Cc: Luke Diamand
Hi,
When I use "git p4 sync" to update a git repository from a perforce depot,
the operation fails with error message:
failure accessing depot: unknown error code info
When I run "p4 login -s" from a shell it reports:
'login' not necessary, no password set for this user.
The following patch fixes the problem for me:
--- /usr/libexec/git-core/git-p4~ 2018-12-15 14:51:07.000000000 +0100
+++ /usr/libexec/git-core/git-p4 2019-01-06 23:23:06.934176387 +0100
@@ -332,6 +332,8 @@
die_bad_access("p4 error: {0}".format(data))
else:
die_bad_access("unknown error")
+ elif code == "info":
+ return
else:
die_bad_access("unknown error code {0}".format(code))
Not sure if this helps, but running "p4 -G login -s | hexdump" gives:
00000000 7b 73 04 00 00 00 63 6f 64 65 73 04 00 00 00 69 |{s....codes....i|
00000010 6e 66 6f 73 05 00 00 00 6c 65 76 65 6c 69 00 00 |nfos....leveli..|
00000020 00 00 73 04 00 00 00 64 61 74 61 73 35 00 00 00 |..s....datas5...|
00000030 27 6c 6f 67 69 6e 27 20 6e 6f 74 20 6e 65 63 65 |'login' not nece|
00000040 73 73 61 72 79 2c 20 6e 6f 20 70 61 73 73 77 6f |ssary, no passwo|
00000050 72 64 20 73 65 74 20 66 6f 72 20 74 68 69 73 20 |rd set for this |
00000060 75 73 65 72 2e 30 |user.0|
00000066
Best regards,
--
Peter Osterlund - peterosterlund2@gmail.com
http://hem.bredband.net/petero2b
^ permalink raw reply
* gitk shows local uncommit changes after touch file + reload
From: Jacob Kroon @ 2019-01-06 22:51 UTC (permalink / raw)
To: git
Hi,
Not sure if this has already been reported, but I observe this odd
behaviour in gitk from master:
git status
gitk # everything looks good
touch <file-under-version-control>
gitk # gitk shows "local uncomitted changes" on the file I touched
git status
gitk # gitk is back to normal again, showing no local uncommitted changes
The issue has been discussed on stackoverflow here:
https://stackoverflow.com/questions/49990403/after-tar-untar-of-git-repo-gitk-shows-local-uncommitted-changes-not-checked
Any chance gitk could be changed to so that it doesn't display the
"local uncommitted changes" blob in this case ?
Regards Jacob
^ permalink raw reply
* Re: [PATCH 3/3] object-store: use one oid_array per subdirectory for loose cache
From: René Scharfe @ 2019-01-06 22:58 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano, Jeff King
In-Reply-To: <87imz1ed25.fsf@evledraar.gmail.com>
Am 06.01.2019 um 21:38 schrieb Ævar Arnfjörð Bjarmason:
>> $ git count-objects
>> 26733 objects, 68808 kilobytes
>>
>> Test HEAD^ HEAD
>> --------------------------------------------------------------------
>> 4205.1: log with %H 0.51(0.47+0.04) 0.51(0.49+0.02) +0.0%
>> 4205.2: log with %h 0.84(0.82+0.02) 0.60(0.57+0.03) -28.6%
>> 4205.3: log with %T 0.53(0.49+0.04) 0.52(0.48+0.03) -1.9%
>> 4205.4: log with %t 0.84(0.80+0.04) 0.60(0.59+0.01) -28.6%
>> 4205.5: log with %P 0.52(0.48+0.03) 0.51(0.50+0.01) -1.9%
>> 4205.6: log with %p 0.85(0.78+0.06) 0.61(0.56+0.05) -28.2%
>> 4205.7: log with %h-%h-%h 0.96(0.92+0.03) 0.69(0.64+0.04) -28.1%
>
> Can you elaborate on the test setup required to get to the point where
> you got these numbers for subsequent comparison, i.e. how you generated
> the approx 100 objects per dir, what OS/version & storage type etc.
I happened to have that many loose objects lying around. Numbers are
for Debian Testing on a Hyper-V VM on Windows 10 1893 on an SSD.
You could fake object directory entries with something like this:
for d in .git/objects/??
do
for i in $(seq 0 9)
do
>"$d/0000000000000000000000000000000000000$i"
done
done
René
^ permalink raw reply
* Re: [PATCH v12 25/26] stash: optionally use the scripted version again
From: Thomas Gummerer @ 2019-01-06 22:59 UTC (permalink / raw)
To: Paul-Sebastian Ungureanu; +Cc: git, Johannes.Schindelin
In-Reply-To: <c05bb05e6eb40af1921e02711ff96350c8165cf2.1545331726.git.ungureanupaulsebastian@gmail.com>
On 12/20, Paul-Sebastian Ungureanu wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> We recently converted the `git stash` command from Unix shell scripts
> to builtins.
>
> Let's end users a way out when they discover a bug in the
s/Let's/& give/ maybe?
The rest of the patch looks good to me.
> builtin command: `stash.useBuiltin`.
>
> As the file name `git-stash` is already in use, let's rename the
> scripted backend to `git-legacy-stash`.
>
> To make the test suite pass with `stash.useBuiltin=false`, this commit
> also backports rudimentary support for `-q` (but only *just* enough
> to appease the test suite), and adds a super-ugly hack to force exit
> code 129 for `git stash -h`.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> .gitignore | 1 +
> Makefile | 1 +
> builtin/stash.c | 35 +++++++++++++++++++++++++++++
> git-stash.sh => git-legacy-stash.sh | 34 +++++++++++++++++++++++++---
> git-sh-setup.sh | 1 +
> git.c | 7 +++++-
> 6 files changed, 75 insertions(+), 4 deletions(-)
> rename git-stash.sh => git-legacy-stash.sh (97%)
>
> diff --git a/.gitignore b/.gitignore
> index 0d77ea5894..7b0164675e 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -82,6 +82,7 @@
> /git-interpret-trailers
> /git-instaweb
> /git-legacy-rebase
> +/git-legacy-stash
> /git-log
> /git-ls-files
> /git-ls-remote
> diff --git a/Makefile b/Makefile
> index 8cee2731aa..810231a0b5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -617,6 +617,7 @@ SCRIPT_SH += git-merge-resolve.sh
> SCRIPT_SH += git-mergetool.sh
> SCRIPT_SH += git-quiltimport.sh
> SCRIPT_SH += git-legacy-rebase.sh
> +SCRIPT_SH += git-legacy-stash.sh
> SCRIPT_SH += git-remote-testgit.sh
> SCRIPT_SH += git-request-pull.sh
> SCRIPT_SH += git-submodule.sh
> diff --git a/builtin/stash.c b/builtin/stash.c
> index fe32ff42fd..346c9d2bb1 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -13,6 +13,7 @@
> #include "revision.h"
> #include "log-tree.h"
> #include "diffcore.h"
> +#include "exec-cmd.h"
>
> #define INCLUDE_ALL_FILES 2
>
> @@ -1513,6 +1514,26 @@ static int save_stash(int argc, const char **argv, const char *prefix)
> return ret;
> }
>
> +static int use_builtin_stash(void)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf out = STRBUF_INIT;
> + int ret;
> +
> + argv_array_pushl(&cp.args,
> + "config", "--bool", "stash.usebuiltin", NULL);
> + cp.git_cmd = 1;
> + if (capture_command(&cp, &out, 6)) {
> + strbuf_release(&out);
> + return 1;
> + }
> +
> + strbuf_trim(&out);
> + ret = !strcmp("true", out.buf);
> + strbuf_release(&out);
> + return ret;
> +}
> +
> int cmd_stash(int argc, const char **argv, const char *prefix)
> {
> int i = -1;
> @@ -1524,6 +1545,20 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
> OPT_END()
> };
>
> + if (!use_builtin_stash()) {
> + const char *path = mkpath("%s/git-legacy-stash",
> + git_exec_path());
> +
> + if (sane_execvp(path, (char **)argv) < 0)
> + die_errno(_("could not exec %s"), path);
> + else
> + BUG("sane_execvp() returned???");
> + }
> +
> + prefix = setup_git_directory();
> + trace_repo_setup(prefix);
> + setup_work_tree();
> +
> git_config(git_diff_basic_config, NULL);
>
> argc = parse_options(argc, argv, prefix, options, git_stash_usage,
> diff --git a/git-stash.sh b/git-legacy-stash.sh
> similarity index 97%
> rename from git-stash.sh
> rename to git-legacy-stash.sh
> index 789ce2f41d..8a8c4a9270 100755
> --- a/git-stash.sh
> +++ b/git-legacy-stash.sh
> @@ -80,6 +80,28 @@ clear_stash () {
> fi
> }
>
> +maybe_quiet () {
> + case "$1" in
> + --keep-stdout)
> + shift
> + if test -n "$GIT_QUIET"
> + then
> + eval "$@" 2>/dev/null
> + else
> + eval "$@"
> + fi
> + ;;
> + *)
> + if test -n "$GIT_QUIET"
> + then
> + eval "$@" >/dev/null 2>&1
> + else
> + eval "$@"
> + fi
> + ;;
> + esac
> +}
> +
> create_stash () {
>
> prepare_fallback_ident
> @@ -112,15 +134,18 @@ create_stash () {
> done
>
> git update-index -q --refresh
> - if no_changes "$@"
> + if maybe_quiet no_changes "$@"
> then
> exit 0
> fi
>
> # state of the base commit
> - if b_commit=$(git rev-parse --verify HEAD)
> + if b_commit=$(maybe_quiet --keep-stdout git rev-parse --verify HEAD)
> then
> head=$(git rev-list --oneline -n 1 HEAD --)
> + elif test -n "$GIT_QUIET"
> + then
> + exit 1
> else
> die "$(gettext "You do not have the initial commit yet")"
> fi
> @@ -315,7 +340,7 @@ push_stash () {
> test -n "$untracked" || git ls-files --error-unmatch -- "$@" >/dev/null || exit 1
>
> git update-index -q --refresh
> - if no_changes "$@"
> + if maybe_quiet no_changes "$@"
> then
> say "$(gettext "No local changes to save")"
> exit 0
> @@ -370,6 +395,9 @@ save_stash () {
> while test $# != 0
> do
> case "$1" in
> + -q|--quiet)
> + GIT_QUIET=t
> + ;;
> --)
> shift
> break
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 378928518b..10d9764185 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -101,6 +101,7 @@ $LONG_USAGE")"
> case "$1" in
> -h)
> echo "$LONG_USAGE"
> + case "$0" in *git-legacy-stash) exit 129;; esac
> exit
> esac
> fi
> diff --git a/git.c b/git.c
> index 8a20909eae..591ebe9409 100644
> --- a/git.c
> +++ b/git.c
> @@ -554,7 +554,12 @@ static struct cmd_struct commands[] = {
> { "show-index", cmd_show_index },
> { "show-ref", cmd_show_ref, RUN_SETUP },
> { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
> - { "stash", cmd_stash, RUN_SETUP | NEED_WORK_TREE },
> + /*
> + * NEEDSWORK: Until the builtin stash is thoroughly robust and no
> + * longer needs redirection to the stash shell script this is kept as
> + * is, then should be changed to RUN_SETUP | NEED_WORK_TREE
> + */
> + { "stash", cmd_stash },
> { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
> { "stripspace", cmd_stripspace },
> { "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },
> --
> 2.20.1.441.g764a526393
>
^ permalink raw reply
* Re: Suspicious fetch-pack behaviour
From: brian m. carlson @ 2019-01-07 3:37 UTC (permalink / raw)
To: Guilhem Bonnefille; +Cc: Git List
In-Reply-To: <CA+BUw6jXTt6QGXvdFjRDNqJcij+1hNP5xybUUuGqo3bY0=ueuA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1643 bytes --]
On Thu, Jan 03, 2019 at 10:52:48AM +0100, Guilhem Bonnefille wrote:
> Hi,
>
> One of my users reported a strange problem: a simple HTTPS clone did
> not work with Git 1.8.3.1 on RedHat 7.
> I did many tests and I was not able to understand why his clone don't
> work while I'm able to do it on other similar host.
>
> Nevertheless, we did more investigations. One of them: a raw strace.
> I discovered two strange behaviours:
> - fetch-pack closes its standard input and standard output and then
> tries to print the references on standard input and finaly dies.
> - git-remote-https does not react to fetch-pack death and continue
> polling an empty set of FD.
>
> Reading fetch-pack code, the behaviour is explicit:
> When "--stateless-rpc" is provided, fd is filled with standard input
> and standard ouput which are then closed.
> https://git.kernel.org/pub/scm/git/git.git/tree/builtin/fetch-pack.c?h=v1.8.3.1#n156
>
> Reading this, I did not understand why it could work.
> Any help appreciated.
When --stateless-rpc is passed, git fetch-pack usually has its standard
input and output wired up to the ends of a socket. Those file
descriptors are then passed to do_fetch_pack, which calls get_common to
negotiate refs with the remote side and get_pack to get the resulting
pack data. The negotiation should function regardless of the final ref
printing.
It's true that attempting to write to the standard output fails in that
case, but that's okay, since we wouldn't have wanted to write that data
to the socket anyway.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [PATCH 2/1] Revert "t/lib-git-daemon: record daemon log"
From: Jeff King @ 2019-01-07 8:20 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Junio C Hamano, Torsten Bögershausen, Git Mailing List,
szeder.dev, Jan Palus, Johannes Schindelin
In-Reply-To: <20190106175310.GC25639@hank.intra.tgummerer.com>
On Sun, Jan 06, 2019 at 05:53:10PM +0000, Thomas Gummerer wrote:
> This reverts commit 314a73d658 (t/lib-git-daemon: record daemon log,
> 2018-01-25), which let tests use the output of git-daemon.
>
> The previous commit removed the last user of deamon.log in the tests,
> there's no good way to make checking for output in the log
> race-proof. Revert this commit as well, to make sure others are not
> tempted to use daemon.log in tests in the future, which would lead to
> racy tests.
>
> The original commit had one change that still makes sense, namely
> switching read/echo for "read -r" and "printf", which relays the data
> more faithfully. Don't revert that piece here, as it is still a
> useful change.
>
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Yep, this looks good to me. Thanks for being extra careful with the
read/printf bits!
Looks like Junio already queued a99653a9b6 (Revert "t/lib-git-daemon:
record daemon log", 2018-12-28) on the tip of tg/t5570-drop-racy-test,
but that's a pure revert. I think we can replace it with this.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] object-store: factor out odb_loose_cache()
From: Jeff King @ 2019-01-07 8:27 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason
In-Reply-To: <b87e7e01-baa6-6fb2-7081-0042ecd3b6b7@web.de>
On Sun, Jan 06, 2019 at 05:45:30PM +0100, René Scharfe wrote:
> Add and use a function for loading the entries if a loose object
> subdirectory for a given object ID. It frees callers from deriving the
> fanout key; they can use the returned oid_array reference for lookups or
> forward range scans.
Much nicer.
> diff --git a/object-store.h b/object-store.h
> index 60758efad8..7236c571c0 100644
> --- a/object-store.h
> +++ b/object-store.h
> @@ -54,6 +54,13 @@ void add_to_alternates_memory(const char *dir);
> */
> void odb_load_loose_cache(struct object_directory *odb, int subdir_nr);
>
> +/*
> + * Populate and return the loose object cache array corresponding to the
> + * given object ID.
> + */
> +struct oid_array *odb_loose_cache(struct object_directory *odb,
> + const struct object_id *oid);
> +
I think the ugly-interfaced odb_load_loose_cache() can become "static"
now, as the only outside caller (from sha1-name.c) has gone away.
> +struct oid_array *odb_loose_cache(struct object_directory *odb,
> + const struct object_id *oid)
> +{
> + int subdir_nr = oid->hash[0];
> + odb_load_loose_cache(odb, subdir_nr);
> + return &odb->loose_objects_cache;
> +}
> +
> void odb_load_loose_cache(struct object_directory *odb, int subdir_nr)
You'd need to re-order these definitions, of course (or alternatively,
just fold the load function inline into odb_loose_cache()).
-Peff
^ permalink raw reply
* [PATCH 0/11] jk/loose-object-cache sha1/object_id fixups
From: Jeff King @ 2019-01-07 8:31 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason
In-Reply-To: <3512c798-aa42-6fba-ee82-d33a8985be91@web.de>
On Sun, Jan 06, 2019 at 05:39:14PM +0100, René Scharfe wrote:
> Am 28.12.2018 um 19:04 schrieb Junio C Hamano:
> > * jk/loose-object-cache (2018-11-24) 10 commits
> > (merged to 'next' on 2018-12-28 at 5a5faf384e)
> > + odb_load_loose_cache: fix strbuf leak
> > + fetch-pack: drop custom loose object cache
> > + sha1-file: use loose object cache for quick existence check
> > + object-store: provide helpers for loose_objects_cache
> > + sha1-file: use an object_directory for the main object dir
> > + handle alternates paths the same as the main object dir
> > + sha1_file_name(): overwrite buffer instead of appending
> > + rename "alternate_object_database" to "object_directory"
> > + submodule--helper: prefer strip_suffix() to ends_with()
> > + fsck: do not reuse child_process structs
> >
> > Originally merged to 'next' on 2018-11-24
> >
> > Code clean-up with optimization for the codepath that checks
> > (non-)existence of loose objects.
> >
> > Will merge to 'master'.
>
> So this has hit master in the meantime. We discussed a sort performance
> fix in [1]; I'll reply with a short series containing a cleaned-up and
> rebased version as a follow-up.
>
> object-store: factor out odb_loose_cache()
> object-store: factor out odb_clear_loose_cache()
> object-store: use one oid_array per subdirectory for loose cache
Thanks! With the exception of one tiny nit, this looks good to me.
I also cleaned up my sha1/object_id patch and rebased it on top of what
you have here. Though as I worked on it, it expanded in scope a bit.
Possibly it should be a separate series entirely, but that would create
some annoying textual conflicts on merge.
[01/11]: sha1-file: fix outdated sha1 comment references
[02/11]: update comment references to sha1_object_info()
[03/11]: http: use struct object_id instead of bare sha1
[04/11]: sha1-file: modernize loose object file functions
[05/11]: sha1-file: modernize loose header/stream functions
[06/11]: sha1-file: convert pass-through functions to object_id
[07/11]: convert has_sha1_file() callers to has_object_file()
[08/11]: sha1-file: drop has_sha1_file()
[09/11]: sha1-file: prefer "loose object file" to "sha1 file" in messages
[10/11]: sha1-file: avoid "sha1 file" for generic use in messages
[11/11]: prefer "hash mismatch" to "sha1 mismatch"
Documentation/git-fsck.txt | 6 +-
apply.c | 2 +-
builtin/cat-file.c | 6 +-
builtin/fetch.c | 7 +-
builtin/index-pack.c | 2 +-
builtin/pack-objects.c | 4 +-
builtin/reflog.c | 2 +-
builtin/show-ref.c | 2 +-
bulk-checkin.c | 2 +-
cache-tree.c | 4 +-
cache.h | 6 +-
http-push.c | 2 +-
http-walker.c | 10 +-
http.c | 14 +--
http.h | 6 +-
object-store.h | 20 ++--
object.c | 4 +-
refs.c | 2 +-
send-pack.c | 2 +-
sha1-file.c | 219 +++++++++++++++++--------------------
streaming.c | 16 +--
t/t1450-fsck.sh | 2 +-
22 files changed, 161 insertions(+), 179 deletions(-)
-Peff
^ permalink raw reply
* [PATCH 01/11] sha1-file: fix outdated sha1 comment references
From: Jeff King @ 2019-01-07 8:33 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason
In-Reply-To: <20190107083150.GC21362@sigill.intra.peff.net>
Commit 17e65451e3 (sha1_file: convert check_sha1_signature to struct
object_id, 2018-03-12) switched to using the name "oid", but forgot to
update the variable name in the comment.
Likewise, b4f5aca40e (sha1_file: convert read_sha1_file to struct
object_id, 2018-03-12) dropped the name read_sha1_file(), but missed a
comment which mentions it.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1-file.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sha1-file.c b/sha1-file.c
index c3c6e50704..e86bb28320 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -124,7 +124,7 @@ const char *empty_blob_oid_hex(void)
/*
* This is meant to hold a *small* number of objects that you would
- * want read_sha1_file() to be able to return, but yet you do not want
+ * want read_object_file() to be able to return, but yet you do not want
* to write them into the object store (e.g. a browse-only
* application).
*/
@@ -798,8 +798,8 @@ void *xmmap(void *start, size_t length,
/*
* With an in-core object data in "map", rehash it to make sure the
- * object name actually matches "sha1" to detect object corruption.
- * With "map" == NULL, try reading the object named with "sha1" using
+ * object name actually matches "oid" to detect object corruption.
+ * With "map" == NULL, try reading the object named with "oid" using
* the streaming interface and rehash it to do the same.
*/
int check_object_signature(const struct object_id *oid, void *map,
--
2.20.1.470.g640a3e2614
^ permalink raw reply related
* [PATCH 02/11] update comment references to sha1_object_info()
From: Jeff King @ 2019-01-07 8:34 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason
In-Reply-To: <20190107083150.GC21362@sigill.intra.peff.net>
Commit abef9020e3 (sha1_file: convert sha1_object_info* to object_id,
2018-03-12) renamed the function to oid_object_info(), but missed some
comments which mention it.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/cat-file.c | 6 +++---
builtin/pack-objects.c | 4 ++--
cache.h | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 2ca56fd086..baaafbdcf3 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -209,14 +209,14 @@ struct expand_data {
/*
* After a mark_query run, this object_info is set up to be
- * passed to sha1_object_info_extended. It will point to the data
+ * passed to oid_object_info_extended. It will point to the data
* elements above, so you can retrieve the response from there.
*/
struct object_info info;
/*
* This flag will be true if the requested batch format and options
- * don't require us to call sha1_object_info, which can then be
+ * don't require us to call oid_object_info, which can then be
* optimized out.
*/
unsigned skip_object_info : 1;
@@ -490,7 +490,7 @@ static int batch_objects(struct batch_options *opt)
/*
* Expand once with our special mark_query flag, which will prime the
- * object_info to be handed to sha1_object_info_extended for each
+ * object_info to be handed to oid_object_info_extended for each
* object.
*/
memset(&data, 0, sizeof(data));
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 24bba8147f..d4c3987f3e 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1642,7 +1642,7 @@ static void check_object(struct object_entry *entry)
/*
* No choice but to fall back to the recursive delta walk
- * with sha1_object_info() to find about the object type
+ * with oid_object_info() to find about the object type
* at this point...
*/
give_up:
@@ -1718,7 +1718,7 @@ static void drop_reused_delta(struct object_entry *entry)
if (packed_object_info(the_repository, IN_PACK(entry), entry->in_pack_offset, &oi) < 0) {
/*
* We failed to get the info from this pack for some reason;
- * fall back to sha1_object_info, which may find another copy.
+ * fall back to oid_object_info, which may find another copy.
* And if that fails, the error will be recorded in oe_type(entry)
* and dealt with in prepare_pack().
*/
diff --git a/cache.h b/cache.h
index ca36b44ee0..587512747b 100644
--- a/cache.h
+++ b/cache.h
@@ -1592,7 +1592,7 @@ extern int odb_mkstemp(struct strbuf *temp_filename, const char *pattern);
extern int odb_pack_keep(const char *name);
/*
- * Set this to 0 to prevent sha1_object_info_extended() from fetching missing
+ * Set this to 0 to prevent oid_object_info_extended() from fetching missing
* blobs. This has a difference only if extensions.partialClone is set.
*
* Its default value is 1.
--
2.20.1.470.g640a3e2614
^ permalink raw reply related
* [PATCH 03/11] http: use struct object_id instead of bare sha1
From: Jeff King @ 2019-01-07 8:34 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason
In-Reply-To: <20190107083150.GC21362@sigill.intra.peff.net>
The dumb-http walker code still passes around and stores object ids as
"unsigned char *sha1". Let's modernize it.
There's probably still more work to be done to handle dumb-http fetches
with a new, larger hash. But that can wait; this is enough that we can
now convert some of the low-level object routines that we call into from
here (and in fact, some of the "oid.hash" references added here will be
further improved in the next patch).
Signed-off-by: Jeff King <peff@peff.net>
---
http-push.c | 2 +-
http-walker.c | 6 +++---
http.c | 14 +++++++-------
http.h | 6 +++---
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/http-push.c b/http-push.c
index cd48590912..0141b0ad53 100644
--- a/http-push.c
+++ b/http-push.c
@@ -255,7 +255,7 @@ static void start_fetch_loose(struct transfer_request *request)
struct active_request_slot *slot;
struct http_object_request *obj_req;
- obj_req = new_http_object_request(repo->url, request->obj->oid.hash);
+ obj_req = new_http_object_request(repo->url, &request->obj->oid);
if (obj_req == NULL) {
request->state = ABORTED;
return;
diff --git a/http-walker.c b/http-walker.c
index 0a392c85b6..856716c63d 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -58,7 +58,7 @@ static void start_object_request(struct walker *walker,
struct active_request_slot *slot;
struct http_object_request *req;
- req = new_http_object_request(obj_req->repo->base, obj_req->oid.hash);
+ req = new_http_object_request(obj_req->repo->base, &obj_req->oid);
if (req == NULL) {
obj_req->state = ABORTED;
return;
@@ -543,11 +543,11 @@ 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 (!hasheq(obj_req->oid.hash, req->real_sha1)) {
+ } else if (!oideq(&obj_req->oid, &req->real_oid)) {
ret = error("File %s has bad hash", hex);
} else if (req->rename < 0) {
struct strbuf buf = STRBUF_INIT;
- loose_object_path(the_repository, &buf, req->sha1);
+ loose_object_path(the_repository, &buf, req->oid.hash);
ret = error("unable to write sha1 filename %s", buf.buf);
strbuf_release(&buf);
}
diff --git a/http.c b/http.c
index 0b6807cef9..8d42154792 100644
--- a/http.c
+++ b/http.c
@@ -2337,9 +2337,9 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
}
struct http_object_request *new_http_object_request(const char *base_url,
- unsigned char *sha1)
+ const struct object_id *oid)
{
- char *hex = sha1_to_hex(sha1);
+ char *hex = oid_to_hex(oid);
struct strbuf filename = STRBUF_INIT;
struct strbuf prevfile = STRBUF_INIT;
int prevlocal;
@@ -2350,10 +2350,10 @@ struct http_object_request *new_http_object_request(const char *base_url,
freq = xcalloc(1, sizeof(*freq));
strbuf_init(&freq->tmpfile, 0);
- hashcpy(freq->sha1, sha1);
+ oidcpy(&freq->oid, oid);
freq->localfile = -1;
- loose_object_path(the_repository, &filename, sha1);
+ loose_object_path(the_repository, &filename, oid->hash);
strbuf_addf(&freq->tmpfile, "%s.temp", filename.buf);
strbuf_addf(&prevfile, "%s.prev", filename.buf);
@@ -2495,16 +2495,16 @@ int finish_http_object_request(struct http_object_request *freq)
}
git_inflate_end(&freq->stream);
- git_SHA1_Final(freq->real_sha1, &freq->c);
+ git_SHA1_Final(freq->real_oid.hash, &freq->c);
if (freq->zret != Z_STREAM_END) {
unlink_or_warn(freq->tmpfile.buf);
return -1;
}
- if (!hasheq(freq->sha1, freq->real_sha1)) {
+ if (!oideq(&freq->oid, &freq->real_oid)) {
unlink_or_warn(freq->tmpfile.buf);
return -1;
}
- loose_object_path(the_repository, &filename, freq->sha1);
+ loose_object_path(the_repository, &filename, freq->oid.hash);
freq->rename = finalize_object_file(freq->tmpfile.buf, filename.buf);
strbuf_release(&filename);
diff --git a/http.h b/http.h
index d305ca1dc7..66c52b2e1e 100644
--- a/http.h
+++ b/http.h
@@ -224,8 +224,8 @@ struct http_object_request {
CURLcode curl_result;
char errorstr[CURL_ERROR_SIZE];
long http_code;
- unsigned char sha1[20];
- unsigned char real_sha1[20];
+ struct object_id oid;
+ struct object_id real_oid;
git_SHA_CTX c;
git_zstream stream;
int zret;
@@ -234,7 +234,7 @@ struct http_object_request {
};
extern struct http_object_request *new_http_object_request(
- const char *base_url, unsigned char *sha1);
+ const char *base_url, const struct object_id *oid);
extern void process_http_object_request(struct http_object_request *freq);
extern int finish_http_object_request(struct http_object_request *freq);
extern void abort_http_object_request(struct http_object_request *freq);
--
2.20.1.470.g640a3e2614
^ permalink raw reply related
* [PATCH 04/11] sha1-file: modernize loose object file functions
From: Jeff King @ 2019-01-07 8:35 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason
In-Reply-To: <20190107083150.GC21362@sigill.intra.peff.net>
The loose object access code in sha1-file.c is some of the oldest in
Git, and could use some modernizing. It mostly uses "unsigned char *"
for object ids, which these days should be "struct object_id".
It also uses the term "sha1_file" in many functions, which is confusing.
The term "loose_objects" is much better. It clearly distinguishes
them from packed objects (which didn't even exist back when the name
"sha1_file" came into being). And it also distinguishes it from the
checksummed-file concept in csum-file.c (which until recently was
actually called "struct sha1file"!).
This patch converts the functions {open,close,map,stat}_sha1_file() into
open_loose_object(), etc, and switches their sha1 arguments for
object_id structs. Similarly, path functions like fill_sha1_path()
become fill_loose_path() and use object_ids.
The function sha1_loose_object_info() already says "loose", so we can
just drop the "sha1" (and teach it to use object_id).
Signed-off-by: Jeff King <peff@peff.net>
---
http-walker.c | 2 +-
http.c | 4 +--
object-store.h | 8 +++--
sha1-file.c | 81 +++++++++++++++++++++++++-------------------------
streaming.c | 4 +--
5 files changed, 51 insertions(+), 48 deletions(-)
diff --git a/http-walker.c b/http-walker.c
index 856716c63d..29b59e2fe0 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -547,7 +547,7 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)
ret = error("File %s has bad hash", hex);
} else if (req->rename < 0) {
struct strbuf buf = STRBUF_INIT;
- loose_object_path(the_repository, &buf, req->oid.hash);
+ loose_object_path(the_repository, &buf, &req->oid);
ret = error("unable to write sha1 filename %s", buf.buf);
strbuf_release(&buf);
}
diff --git a/http.c b/http.c
index 8d42154792..43d06dd074 100644
--- a/http.c
+++ b/http.c
@@ -2353,7 +2353,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
oidcpy(&freq->oid, oid);
freq->localfile = -1;
- loose_object_path(the_repository, &filename, oid->hash);
+ loose_object_path(the_repository, &filename, oid);
strbuf_addf(&freq->tmpfile, "%s.temp", filename.buf);
strbuf_addf(&prevfile, "%s.prev", filename.buf);
@@ -2504,7 +2504,7 @@ int finish_http_object_request(struct http_object_request *freq)
unlink_or_warn(freq->tmpfile.buf);
return -1;
}
- loose_object_path(the_repository, &filename, freq->oid.hash);
+ loose_object_path(the_repository, &filename, &freq->oid);
freq->rename = finalize_object_file(freq->tmpfile.buf, filename.buf);
strbuf_release(&filename);
diff --git a/object-store.h b/object-store.h
index 2fb6c0e4db..6b1c408753 100644
--- a/object-store.h
+++ b/object-store.h
@@ -161,11 +161,13 @@ void raw_object_store_clear(struct raw_object_store *o);
/*
* Put in `buf` the name of the file in the local object database that
- * would be used to store a loose object with the specified sha1.
+ * would be used to store a loose object with the specified oid.
*/
-const char *loose_object_path(struct repository *r, struct strbuf *buf, const unsigned char *sha1);
+const char *loose_object_path(struct repository *r, struct strbuf *buf,
+ const struct object_id *oid);
-void *map_sha1_file(struct repository *r, const unsigned char *sha1, unsigned long *size);
+void *map_loose_object(struct repository *r, const struct object_id *oid,
+ unsigned long *size);
extern void *read_object_file_extended(const struct object_id *oid,
enum object_type *type,
diff --git a/sha1-file.c b/sha1-file.c
index e86bb28320..cd8e5f005a 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -333,12 +333,12 @@ int raceproof_create_file(const char *path, create_file_fn fn, void *cb)
return ret;
}
-static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)
+static void fill_loose_path(struct strbuf *buf, const struct object_id *oid)
{
int i;
for (i = 0; i < the_hash_algo->rawsz; i++) {
static char hex[] = "0123456789abcdef";
- unsigned int val = sha1[i];
+ unsigned int val = oid->hash[i];
strbuf_addch(buf, hex[val >> 4]);
strbuf_addch(buf, hex[val & 0xf]);
if (!i)
@@ -348,19 +348,19 @@ static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)
static const char *odb_loose_path(struct object_directory *odb,
struct strbuf *buf,
- const unsigned char *sha1)
+ const struct object_id *oid)
{
strbuf_reset(buf);
strbuf_addstr(buf, odb->path);
strbuf_addch(buf, '/');
- fill_sha1_path(buf, sha1);
+ fill_loose_path(buf, oid);
return buf->buf;
}
const char *loose_object_path(struct repository *r, struct strbuf *buf,
- const unsigned char *sha1)
+ const struct object_id *oid)
{
- return odb_loose_path(r->objects->odb, buf, sha1);
+ return odb_loose_path(r->objects->odb, buf, oid);
}
/*
@@ -721,7 +721,7 @@ static int check_and_freshen_odb(struct object_directory *odb,
int freshen)
{
static struct strbuf path = STRBUF_INIT;
- odb_loose_path(odb, &path, oid->hash);
+ odb_loose_path(odb, &path, oid);
return check_and_freshen_file(path.buf, freshen);
}
@@ -872,22 +872,22 @@ int git_open_cloexec(const char *name, int flags)
}
/*
- * Find "sha1" as a loose object in the local repository or in an alternate.
+ * Find "oid" as a loose object in the local repository or in an alternate.
* Returns 0 on success, negative on failure.
*
* The "path" out-parameter will give the path of the object we found (if any).
* Note that it may point to static storage and is only valid until another
- * call to stat_sha1_file().
+ * call to stat_loose_object().
*/
-static int stat_sha1_file(struct repository *r, const unsigned char *sha1,
- struct stat *st, const char **path)
+static int stat_loose_object(struct repository *r, const struct object_id *oid,
+ struct stat *st, const char **path)
{
struct object_directory *odb;
static struct strbuf buf = STRBUF_INIT;
prepare_alt_odb(r);
for (odb = r->objects->odb; odb; odb = odb->next) {
- *path = odb_loose_path(odb, &buf, sha1);
+ *path = odb_loose_path(odb, &buf, oid);
if (!lstat(*path, st))
return 0;
}
@@ -896,11 +896,11 @@ static int stat_sha1_file(struct repository *r, const unsigned char *sha1,
}
/*
- * Like stat_sha1_file(), but actually open the object and return the
+ * Like stat_loose_object(), but actually open the object and return the
* descriptor. See the caveats on the "path" parameter above.
*/
-static int open_sha1_file(struct repository *r,
- const unsigned char *sha1, const char **path)
+static int open_loose_object(struct repository *r,
+ const struct object_id *oid, const char **path)
{
int fd;
struct object_directory *odb;
@@ -909,7 +909,7 @@ static int open_sha1_file(struct repository *r,
prepare_alt_odb(r);
for (odb = r->objects->odb; odb; odb = odb->next) {
- *path = odb_loose_path(odb, &buf, sha1);
+ *path = odb_loose_path(odb, &buf, oid);
fd = git_open(*path);
if (fd >= 0)
return fd;
@@ -939,10 +939,10 @@ static int quick_has_loose(struct repository *r,
/*
* Map the loose object at "path" if it is not NULL, or the path found by
- * searching for a loose object named "sha1".
+ * searching for a loose object named "oid".
*/
-static void *map_sha1_file_1(struct repository *r, const char *path,
- const unsigned char *sha1, unsigned long *size)
+static void *map_loose_object_1(struct repository *r, const char *path,
+ const struct object_id *oid, unsigned long *size)
{
void *map;
int fd;
@@ -950,7 +950,7 @@ static void *map_sha1_file_1(struct repository *r, const char *path,
if (path)
fd = git_open(path);
else
- fd = open_sha1_file(r, sha1, &path);
+ fd = open_loose_object(r, oid, &path);
map = NULL;
if (fd >= 0) {
struct stat st;
@@ -969,10 +969,11 @@ static void *map_sha1_file_1(struct repository *r, const char *path,
return map;
}
-void *map_sha1_file(struct repository *r,
- const unsigned char *sha1, unsigned long *size)
+void *map_loose_object(struct repository *r,
+ const struct object_id *oid,
+ unsigned long *size)
{
- return map_sha1_file_1(r, NULL, sha1, size);
+ return map_loose_object_1(r, NULL, oid, size);
}
static int unpack_sha1_short_header(git_zstream *stream,
@@ -1161,9 +1162,9 @@ int parse_sha1_header(const char *hdr, unsigned long *sizep)
return parse_sha1_header_extended(hdr, &oi, 0);
}
-static int sha1_loose_object_info(struct repository *r,
- const unsigned char *sha1,
- struct object_info *oi, int flags)
+static int loose_object_info(struct repository *r,
+ const struct object_id *oid,
+ struct object_info *oi, int flags)
{
int status = 0;
unsigned long mapsize;
@@ -1188,15 +1189,15 @@ static int sha1_loose_object_info(struct repository *r,
const char *path;
struct stat st;
if (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))
- return quick_has_loose(r, sha1) ? 0 : -1;
- if (stat_sha1_file(r, sha1, &st, &path) < 0)
+ return quick_has_loose(r, oid->hash) ? 0 : -1;
+ if (stat_loose_object(r, oid, &st, &path) < 0)
return -1;
if (oi->disk_sizep)
*oi->disk_sizep = st.st_size;
return 0;
}
- map = map_sha1_file(r, sha1, &mapsize);
+ map = map_loose_object(r, oid, &mapsize);
if (!map)
return -1;
@@ -1208,22 +1209,22 @@ static int sha1_loose_object_info(struct repository *r,
if ((flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE)) {
if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, hdr, sizeof(hdr), &hdrbuf) < 0)
status = error(_("unable to unpack %s header with --allow-unknown-type"),
- sha1_to_hex(sha1));
+ oid_to_hex(oid));
} else if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
status = error(_("unable to unpack %s header"),
- sha1_to_hex(sha1));
+ oid_to_hex(oid));
if (status < 0)
; /* Do nothing */
else if (hdrbuf.len) {
if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)
status = error(_("unable to parse %s header with --allow-unknown-type"),
- sha1_to_hex(sha1));
+ oid_to_hex(oid));
} else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)
- status = error(_("unable to parse %s header"), sha1_to_hex(sha1));
+ status = error(_("unable to parse %s header"), oid_to_hex(oid));
if (status >= 0 && oi->contentp) {
*oi->contentp = unpack_sha1_rest(&stream, hdr,
- *oi->sizep, sha1);
+ *oi->sizep, oid->hash);
if (!*oi->contentp) {
git_inflate_end(&stream);
status = -1;
@@ -1289,7 +1290,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,
return -1;
/* Most likely it's a loose object. */
- if (!sha1_loose_object_info(r, real->hash, oi, flags))
+ if (!loose_object_info(r, real, oi, flags))
return 0;
/* Not a loose object; someone else may have just packed it. */
@@ -1417,7 +1418,7 @@ void *read_object_file_extended(const struct object_id *oid,
die(_("replacement %s not found for %s"),
oid_to_hex(repl), oid_to_hex(oid));
- if (!stat_sha1_file(the_repository, repl->hash, &st, &path))
+ if (!stat_loose_object(the_repository, repl, &st, &path))
die(_("loose object %s (stored in %s) is corrupt"),
oid_to_hex(repl), path);
@@ -1552,7 +1553,7 @@ int hash_object_file(const void *buf, unsigned long len, const char *type,
}
/* Finalize a file on disk, and close it. */
-static void close_sha1_file(int fd)
+static void close_loose_object(int fd)
{
if (fsync_object_files)
fsync_or_die(fd, "sha1 file");
@@ -1617,7 +1618,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,
static struct strbuf tmp_file = STRBUF_INIT;
static struct strbuf filename = STRBUF_INIT;
- loose_object_path(the_repository, &filename, oid->hash);
+ loose_object_path(the_repository, &filename, oid);
fd = create_tmpfile(&tmp_file, filename.buf);
if (fd < 0) {
@@ -1665,7 +1666,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,
die(_("confused by unstable object source data for %s"),
oid_to_hex(oid));
- close_sha1_file(fd);
+ close_loose_object(fd);
if (mtime) {
struct utimbuf utb;
@@ -2260,7 +2261,7 @@ int read_loose_object(const char *path,
*contents = NULL;
- map = map_sha1_file_1(the_repository, path, NULL, &mapsize);
+ map = map_loose_object_1(the_repository, path, NULL, &mapsize);
if (!map) {
error_errno(_("unable to mmap %s"), path);
goto out;
diff --git a/streaming.c b/streaming.c
index ac7c7a22f9..9049146bc1 100644
--- a/streaming.c
+++ b/streaming.c
@@ -338,8 +338,8 @@ static struct stream_vtbl loose_vtbl = {
static open_method_decl(loose)
{
- st->u.loose.mapped = map_sha1_file(the_repository,
- oid->hash, &st->u.loose.mapsize);
+ st->u.loose.mapped = map_loose_object(the_repository,
+ oid, &st->u.loose.mapsize);
if (!st->u.loose.mapped)
return -1;
if ((unpack_sha1_header(&st->z,
--
2.20.1.470.g640a3e2614
^ permalink raw reply related
* [PATCH 05/11] sha1-file: modernize loose header/stream functions
From: Jeff King @ 2019-01-07 8:37 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason
In-Reply-To: <20190107083150.GC21362@sigill.intra.peff.net>
As with the open/map/close functions for loose objects that were
recently converted, the functions for parsing the loose object stream
use the name "sha1" and a bare "unsigned char *". Let's fix that so that
unpack_sha1_header() becomes unpack_loose_header(), etc.
These conversions are less clear-cut than the file access functions.
You could argue that the they are parsing Git's canonical object format
(i.e., "type size\0contents", over which we compute the hash), which is
not strictly tied to loose storage. But in practice these functions are
used only for loose objects, and using the term "loose_header" (instead
of "object_header") distinguishes it from the object header found in
packfiles (which contains the same information in a different format).
Signed-off-by: Jeff King <peff@peff.net>
---
Of course "loose_object_header" would be even more exact, but is quite
long. ;)
cache.h | 4 +--
sha1-file.c | 84 +++++++++++++++++++++++++++--------------------------
streaming.c | 12 ++++----
3 files changed, 51 insertions(+), 49 deletions(-)
diff --git a/cache.h b/cache.h
index 587512747b..653c36d0b7 100644
--- a/cache.h
+++ b/cache.h
@@ -1269,8 +1269,8 @@ extern char *xdg_cache_home(const char *filename);
extern int git_open_cloexec(const char *name, int flags);
#define git_open(name) git_open_cloexec(name, O_RDONLY)
-extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);
-extern int parse_sha1_header(const char *hdr, unsigned long *sizep);
+extern int unpack_loose_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);
+extern int parse_loose_header(const char *hdr, unsigned long *sizep);
extern int check_object_signature(const struct object_id *oid, void *buf, unsigned long size, const char *type);
diff --git a/sha1-file.c b/sha1-file.c
index cd8e5f005a..4938258ed1 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -976,9 +976,9 @@ void *map_loose_object(struct repository *r,
return map_loose_object_1(r, NULL, oid, size);
}
-static int unpack_sha1_short_header(git_zstream *stream,
- unsigned char *map, unsigned long mapsize,
- void *buffer, unsigned long bufsiz)
+static int unpack_loose_short_header(git_zstream *stream,
+ unsigned char *map, unsigned long mapsize,
+ void *buffer, unsigned long bufsiz)
{
/* Get the data stream */
memset(stream, 0, sizeof(*stream));
@@ -991,12 +991,12 @@ static int unpack_sha1_short_header(git_zstream *stream,
return git_inflate(stream, 0);
}
-int unpack_sha1_header(git_zstream *stream,
- unsigned char *map, unsigned long mapsize,
- void *buffer, unsigned long bufsiz)
+int unpack_loose_header(git_zstream *stream,
+ unsigned char *map, unsigned long mapsize,
+ void *buffer, unsigned long bufsiz)
{
- int status = unpack_sha1_short_header(stream, map, mapsize,
- buffer, bufsiz);
+ int status = unpack_loose_short_header(stream, map, mapsize,
+ buffer, bufsiz);
if (status < Z_OK)
return status;
@@ -1007,13 +1007,13 @@ int unpack_sha1_header(git_zstream *stream,
return 0;
}
-static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
- unsigned long mapsize, void *buffer,
- unsigned long bufsiz, struct strbuf *header)
+static int unpack_loose_header_to_strbuf(git_zstream *stream, unsigned char *map,
+ unsigned long mapsize, void *buffer,
+ unsigned long bufsiz, struct strbuf *header)
{
int status;
- status = unpack_sha1_short_header(stream, map, mapsize, buffer, bufsiz);
+ status = unpack_loose_short_header(stream, map, mapsize, buffer, bufsiz);
if (status < Z_OK)
return -1;
@@ -1043,7 +1043,9 @@ static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
return -1;
}
-static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long size, const unsigned char *sha1)
+static void *unpack_loose_rest(git_zstream *stream,
+ void *buffer, unsigned long size,
+ const struct object_id *oid)
{
int bytes = strlen(buffer) + 1;
unsigned char *buf = xmallocz(size);
@@ -1080,10 +1082,10 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s
}
if (status < 0)
- error(_("corrupt loose object '%s'"), sha1_to_hex(sha1));
+ error(_("corrupt loose object '%s'"), oid_to_hex(oid));
else if (stream->avail_in)
error(_("garbage at end of loose object '%s'"),
- sha1_to_hex(sha1));
+ oid_to_hex(oid));
free(buf);
return NULL;
}
@@ -1093,8 +1095,8 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s
* too permissive for what we want to check. So do an anal
* object header parse by hand.
*/
-static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
- unsigned int flags)
+static int parse_loose_header_extended(const char *hdr, struct object_info *oi,
+ unsigned int flags)
{
const char *type_buf = hdr;
unsigned long size;
@@ -1154,12 +1156,12 @@ static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
return *hdr ? -1 : type;
}
-int parse_sha1_header(const char *hdr, unsigned long *sizep)
+int parse_loose_header(const char *hdr, unsigned long *sizep)
{
struct object_info oi = OBJECT_INFO_INIT;
oi.sizep = sizep;
- return parse_sha1_header_extended(hdr, &oi, 0);
+ return parse_loose_header_extended(hdr, &oi, 0);
}
static int loose_object_info(struct repository *r,
@@ -1207,24 +1209,24 @@ static int loose_object_info(struct repository *r,
if (oi->disk_sizep)
*oi->disk_sizep = mapsize;
if ((flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE)) {
- if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, hdr, sizeof(hdr), &hdrbuf) < 0)
+ if (unpack_loose_header_to_strbuf(&stream, map, mapsize, hdr, sizeof(hdr), &hdrbuf) < 0)
status = error(_("unable to unpack %s header with --allow-unknown-type"),
oid_to_hex(oid));
- } else if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
+ } else if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
status = error(_("unable to unpack %s header"),
oid_to_hex(oid));
if (status < 0)
; /* Do nothing */
else if (hdrbuf.len) {
- if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, flags)) < 0)
+ if ((status = parse_loose_header_extended(hdrbuf.buf, oi, flags)) < 0)
status = error(_("unable to parse %s header with --allow-unknown-type"),
oid_to_hex(oid));
- } else if ((status = parse_sha1_header_extended(hdr, oi, flags)) < 0)
+ } else if ((status = parse_loose_header_extended(hdr, oi, flags)) < 0)
status = error(_("unable to parse %s header"), oid_to_hex(oid));
if (status >= 0 && oi->contentp) {
- *oi->contentp = unpack_sha1_rest(&stream, hdr,
- *oi->sizep, oid->hash);
+ *oi->contentp = unpack_loose_rest(&stream, hdr,
+ *oi->sizep, oid);
if (!*oi->contentp) {
git_inflate_end(&stream);
status = -1;
@@ -2189,14 +2191,14 @@ void odb_clear_loose_cache(struct object_directory *odb)
sizeof(odb->loose_objects_subdir_seen));
}
-static int check_stream_sha1(git_zstream *stream,
- const char *hdr,
- unsigned long size,
- const char *path,
- const unsigned char *expected_sha1)
+static int check_stream_oid(git_zstream *stream,
+ const char *hdr,
+ unsigned long size,
+ const char *path,
+ const struct object_id *expected_oid)
{
git_hash_ctx c;
- unsigned char real_sha1[GIT_MAX_RAWSZ];
+ struct object_id real_oid;
unsigned char buf[4096];
unsigned long total_read;
int status = Z_OK;
@@ -2212,7 +2214,7 @@ static int check_stream_sha1(git_zstream *stream,
/*
* This size comparison must be "<=" to read the final zlib packets;
- * see the comment in unpack_sha1_rest for details.
+ * see the comment in unpack_loose_rest for details.
*/
while (total_read <= size &&
(status == Z_OK ||
@@ -2228,19 +2230,19 @@ static int check_stream_sha1(git_zstream *stream,
git_inflate_end(stream);
if (status != Z_STREAM_END) {
- error(_("corrupt loose object '%s'"), sha1_to_hex(expected_sha1));
+ error(_("corrupt loose object '%s'"), oid_to_hex(expected_oid));
return -1;
}
if (stream->avail_in) {
error(_("garbage at end of loose object '%s'"),
- sha1_to_hex(expected_sha1));
+ oid_to_hex(expected_oid));
return -1;
}
- the_hash_algo->final_fn(real_sha1, &c);
- if (!hasheq(expected_sha1, real_sha1)) {
+ the_hash_algo->final_fn(real_oid.hash, &c);
+ if (!oideq(expected_oid, &real_oid)) {
error(_("sha1 mismatch for %s (expected %s)"), path,
- sha1_to_hex(expected_sha1));
+ oid_to_hex(expected_oid));
return -1;
}
@@ -2267,12 +2269,12 @@ int read_loose_object(const char *path,
goto out;
}
- if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0) {
+ if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0) {
error(_("unable to unpack header of %s"), path);
goto out;
}
- *type = parse_sha1_header(hdr, size);
+ *type = parse_loose_header(hdr, size);
if (*type < 0) {
error(_("unable to parse header of %s"), path);
git_inflate_end(&stream);
@@ -2280,10 +2282,10 @@ int read_loose_object(const char *path,
}
if (*type == OBJ_BLOB && *size > big_file_threshold) {
- if (check_stream_sha1(&stream, hdr, *size, path, expected_oid->hash) < 0)
+ if (check_stream_oid(&stream, hdr, *size, path, expected_oid) < 0)
goto out;
} else {
- *contents = unpack_sha1_rest(&stream, hdr, *size, expected_oid->hash);
+ *contents = unpack_loose_rest(&stream, hdr, *size, expected_oid);
if (!*contents) {
error(_("unable to unpack contents of %s"), path);
git_inflate_end(&stream);
diff --git a/streaming.c b/streaming.c
index 9049146bc1..998e6285d7 100644
--- a/streaming.c
+++ b/streaming.c
@@ -342,12 +342,12 @@ static open_method_decl(loose)
oid, &st->u.loose.mapsize);
if (!st->u.loose.mapped)
return -1;
- if ((unpack_sha1_header(&st->z,
- st->u.loose.mapped,
- st->u.loose.mapsize,
- st->u.loose.hdr,
- sizeof(st->u.loose.hdr)) < 0) ||
- (parse_sha1_header(st->u.loose.hdr, &st->size) < 0)) {
+ if ((unpack_loose_header(&st->z,
+ st->u.loose.mapped,
+ st->u.loose.mapsize,
+ st->u.loose.hdr,
+ sizeof(st->u.loose.hdr)) < 0) ||
+ (parse_loose_header(st->u.loose.hdr, &st->size) < 0)) {
git_inflate_end(&st->z);
munmap(st->u.loose.mapped, st->u.loose.mapsize);
return -1;
--
2.20.1.470.g640a3e2614
^ permalink raw reply related
* [PATCH 06/11] sha1-file: convert pass-through functions to object_id
From: Jeff King @ 2019-01-07 8:37 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason
In-Reply-To: <20190107083150.GC21362@sigill.intra.peff.net>
These two static functions, read_object() and quick_has_loose(), both
have to hashcpy() their bare-sha1 arguments into object_id structs to
pass them along. Since all of their callers actually have object_id
structs in the first place, we can eliminate the copying by adjusting
their input parameters.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1-file.c | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/sha1-file.c b/sha1-file.c
index 4938258ed1..589a666686 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -922,16 +922,13 @@ static int open_loose_object(struct repository *r,
}
static int quick_has_loose(struct repository *r,
- const unsigned char *sha1)
+ const struct object_id *oid)
{
- struct object_id oid;
struct object_directory *odb;
- hashcpy(oid.hash, sha1);
-
prepare_alt_odb(r);
for (odb = r->objects->odb; odb; odb = odb->next) {
- if (oid_array_lookup(odb_loose_cache(odb, &oid), &oid) >= 0)
+ if (oid_array_lookup(odb_loose_cache(odb, oid), oid) >= 0)
return 1;
}
return 0;
@@ -1191,7 +1188,7 @@ static int loose_object_info(struct repository *r,
const char *path;
struct stat st;
if (!oi->disk_sizep && (flags & OBJECT_INFO_QUICK))
- return quick_has_loose(r, oid->hash) ? 0 : -1;
+ return quick_has_loose(r, oid) ? 0 : -1;
if (stat_loose_object(r, oid, &st, &path) < 0)
return -1;
if (oi->disk_sizep)
@@ -1355,19 +1352,16 @@ int oid_object_info(struct repository *r,
return type;
}
-static void *read_object(const unsigned char *sha1, enum object_type *type,
+static void *read_object(const struct object_id *oid, enum object_type *type,
unsigned long *size)
{
- struct object_id oid;
struct object_info oi = OBJECT_INFO_INIT;
void *content;
oi.typep = type;
oi.sizep = size;
oi.contentp = &content;
- hashcpy(oid.hash, sha1);
-
- if (oid_object_info_extended(the_repository, &oid, &oi, 0) < 0)
+ if (oid_object_info_extended(the_repository, oid, &oi, 0) < 0)
return NULL;
return content;
}
@@ -1408,7 +1402,7 @@ void *read_object_file_extended(const struct object_id *oid,
lookup_replace_object(the_repository, oid) : oid;
errno = 0;
- data = read_object(repl->hash, type, size);
+ data = read_object(repl, type, size);
if (data)
return data;
@@ -1748,7 +1742,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)
if (has_loose_object(oid))
return 0;
- buf = read_object(oid->hash, &type, &len);
+ buf = read_object(oid, &type, &len);
if (!buf)
return error(_("cannot read sha1_file for %s"), oid_to_hex(oid));
hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %"PRIuMAX , type_name(type), (uintmax_t)len) + 1;
--
2.20.1.470.g640a3e2614
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox