* git clone http:// fails some times with "Request for d53.. aborted"
From: Yaroslav Halchenko @ 2016-09-09 22:19 UTC (permalink / raw)
To: git
even when (v 2.7.0) ran on the box where the server is, so
unlikely to be network issue
or from my laptop (v 2.9.3) with ok but wifi with a weakish signal to the
access point:
$> ( set -e; for s in {1..100}; do rm -rf fbirn_phaseIII ; git clone http://datasets.datalad.org/nidm/fbirn_phaseIII/.git; done; )
Cloning into 'fbirn_phaseIII'...
Checking connectivity... done.
Cloning into 'fbirn_phaseIII'...
error: Request for d53302dfc7ad13b786923927021039d21a10d5bd aborted
error: Unable to find d53302dfc7ad13b786923927021039d21a10d5bd under http://datasets.datalad.org/nidm/fbirn_phaseIII/.git
Cannot obtain needed tree d53302dfc7ad13b786923927021039d21a10d5bd
while processing commit 22dd4c49417cad6f4082ac2aebef45da8b6e473d.
error: fetch failed.
even if I build fresh 2.10.0, discovered that we can trace now CURL calls
$> export PATH=$HOME/proj/misc/git:$PATH
$> git version
git version 2.10.0
$> ( set -e; for s in {1..100}; do rm -rf fbirn_phaseIII ; GIT_TRACE_CURL=/tmp/git-trace-curl-failed.log git clone http://datasets.datalad.org/nidm/fbirn_phaseIII/.git && mv /tmp/git-trace-curl-failed.log /tmp/git-trace-curl-good.log || exit 1; done; )
Cloning into 'fbirn_phaseIII'...
warning: templates not found /home/yoh/share/git-core/templates
... a few more ...
Cloning into 'fbirn_phaseIII'...
warning: templates not found /home/yoh/share/git-core/templates
error: Request for d53302dfc7ad13b786923927021039d21a10d5bd aborted
error: Unable to find d53302dfc7ad13b786923927021039d21a10d5bd under http://datasets.datalad.org/nidm/fbirn_phaseIII/.git
Cannot obtain needed tree d53302dfc7ad13b786923927021039d21a10d5bd
while processing commit 22dd4c49417cad6f4082ac2aebef45da8b6e473d.
error: fetch failed.
( set -e; for s in {1..100}; do; rm -rf fbirn_phaseIII; GIT_TRACE_CURL= git ) 18.70s user 8.01s system 34% cpu 1:16.84 total
NB
- note that it is d53302dfc7ad13b786923927021039d21a10d5bd all the time!
- that one corresponds to a tree
$> git show d53302dfc7ad13b786923927021039d21a10d5bd
tree d53302dfc7ad13b786923927021039d21a10d5bd
1ec/
da9/
- I do not see that object being reported as failed to be found on the server:
$> grep d5/3302dfc7ad13b786923927021039d21a10d5bd /var/log/apache2/datasets.datalad.org-error.log
$> grep d5/3302dfc7ad13b786923927021039d21a10d5bd /var/log/apache2/datasets.datalad.org-access.log
129.170.31.34 - - [09/Sep/2016:14:14:25 -0400] "GET /nidm/fbirn_phaseIII/.git/objects/d5/3302dfc7ad13b786923927021039d21a10d5bd HTTP/1.1" 200 72
...
- and the logs say that only "good" run had it requested:
$> zgrep 3302dfc7ad13b786923927021039d21a10d5bd git-trace-curl-*.log.gz
git-trace-curl-good.log.gz:18:09:13.749268 http.c:517 => Send header: GET /nidm/fbirn_phaseIII/.git/objects/d5/3302dfc7ad13b786923927021039d21a10d5bd HTTP/1.1
and here are those two GIT_TRACE_CURL traces from good and bad runs -- may be
someone with a better clue could figure it out or any other information I
should provide to help pin point why that "request for d5... aborted" ;)
http://www.onerussian.com/tmp/git-trace-curl-good.log.gz
http://www.onerussian.com/tmp/git-trace-curl-failed.log.gz
Thanks in advance and please CC me in replies
--
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik
^ permalink raw reply
* Re: [RFC/PATCH] ls-files: adding support for submodules
From: Junio C Hamano @ 2016-09-09 23:01 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <1473458004-41460-1-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> Allow ls-files to recognize submodules in order to retrieve a list of
> files from a repository's submodules. This is done by forking off a
> process to recursively call ls-files on all submodules.
While I see why "ls-files --recurse-submodules" sounds nice ("hey, I
can get list of _all_ the files here"), and I am quite happy with
the quality of implementation (not just the code but its
documentation and test) especially from a first-time contributor, I
am not quite sure what the utility of this new feature would be,
especially given that the command is a plumbing, i.e. meant to be a
useful building block for scripts.
If I get
$ git ls-files --recurse-submodules
Makefile
lib/Makefile
lib/hello.py
main.py
goodbye.py
out of the command, what can I do with it without knowing where the
submodule boundaries are? It's not like I can just do
git ls-files --recurse-submodule |
while read path
do
git update-index --add "$path"
done
when "lib/" is a submodule. Instead, I'd need to go to "lib/" and
then add "Makefile" and "hello.py" from there.
> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
> new file mode 100644
> index 0000000..78deded
> --- /dev/null
> +++ b/t/t3007-ls-files-recurse-submodules.sh
> @@ -0,0 +1,103 @@
> +...
> +test_expect_success 'setup directory structure and submodules' '
> +...
> +'
> +
> +cat >expect <<EOF
> +.gitmodules
> +a
> +b/b
> +submodule/c
> +EOF
We used to do that when we didn't know better.
Please don't do things outside test_expect_* block, especially in a
new script.
^ permalink raw reply
* Re: How to simulate a real checkout to test a new smudge filter?
From: john smith @ 2016-09-09 23:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narębski, Torsten Bögershausen, git
In-Reply-To: <xmqqwpik8zy3.fsf@gitster.mtv.corp.google.com>
On 9/10/16, Junio C Hamano <gitster@pobox.com> wrote:
> The clean and smudge operations should look _only_ at the contents
> they are filtering, and nothing else, and the clean/smudge filtering
> mechanism is designed to support that use case. It is not designed
> to do things like embedding the name of the branch that is being
> checked out into the result.
Ok, I think I get it. It was actually my original problem with smudge
filters because I wanted them to be run after *every* checkout even if
file contents stayed the same (hence the subject).
Now Jakub suggested post-checkout hook in conjunction with only clean
filter for my problem of managing dotfiles but I think I don't fully
get it. The problem is that in the scenario presented in my last
e-mail clean filter is run in the situation which doesn't like a
checkin to me. Is `git checkout <PATH>' doing a *checkin*" under the
hood so that the clean filter is called? What does actually `checkin'
mean in Git? I thought that checkin it's the same as committing a
file into the repository.
--
<wempwer@gmail.com>
^ permalink raw reply
* Re: How to simulate a real checkout to test a new smudge filter?
From: Junio C Hamano @ 2016-09-09 23:12 UTC (permalink / raw)
To: john smith; +Cc: Jakub Narębski, Torsten Bögershausen, git
In-Reply-To: <CAKmQUfYCUq=kUTau1-3NjjPVBk6WkP3KdTjgMC8sZtU8h=H4iQ@mail.gmail.com>
john smith <wempwer@gmail.com> writes:
> ... get it. The problem is that in the scenario presented in my last
> e-mail clean filter is run in the situation which doesn't like a
> checkin to me. Is `git checkout <PATH>' doing a *checkin*" under the
> hood so that the clean filter is called? What does actually `checkin'
> mean in Git? I thought that checkin it's the same as committing a
> file into the repository.
I do not know what Jakub meant.
When we need to see if we need to update a working tree file, and
when we cannot tell if you modified the working tree file since you
checked it out, we may have to run the clean filter (and other
conversions collectively called "convert-to-git") on the contents in
it and see if the resulting blob matches what is recorded in the
index (if they match, there is no need to update the working tree
file; if they don't match, we either report that you have a local
modification, or we overwrite it with the contents, depending on
what operation we are doing for the user).
^ permalink raw reply
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"
From: Dakota Hawkins @ 2016-09-09 23:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Jeff King, Git Mailing List
In-Reply-To: <xmqqk2ekc2zl.fsf@gitster.mtv.corp.google.com>
You guys are the best, I'm really impressed with all of the responses
to this issue! Thank you all for all of your hard work!
Dakota
On Fri, Sep 9, 2016 at 3:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>> On Thu, 11 Aug 2016, Junio C Hamano wrote:
>>>>
>>>> Do you have a preference either way to help us decide if we want to
>>>> take this change or not?
>>>
>>> I have no strong preference. I guess that it does not hurt to go with the
>>> patch, and it would probably help in a few cases.
>>
>> OK. Let me dig the change back and how well it still fits ;-)
>
> Ah, I already had it in my tree lest I forget. Let me mark it for
> merging down to 'master'.
>
> Thanks.
^ permalink raw reply
* Re: If a branch moves a submodule, "merge --ff[-only]" succeeds while "merge --no-ff" fails with conflicts
From: Dakota Hawkins @ 2016-09-09 23:37 UTC (permalink / raw)
To: Stefan Beller; +Cc: Git Mailing List, mwitte
In-Reply-To: <CAG0BQX=FFWqR=45g6buujzK2jknx8aZnQoiV_m-QZhcx4Dd52g@mail.gmail.com>
Any ideas on this anywhere?
In my opinion if a merge can fast-forward without conflict it should
trivially be able to non-fast-forward without conflict.
Also, I'm not too familiar/comfortable with mailing list etiquette,
and I don't want to be a bother by continuing to ping this thread.
Thanks,
Dakota
On Tue, Sep 6, 2016 at 4:02 PM, Dakota Hawkins <dakotahawkins@gmail.com> wrote:
> On Tue, Sep 6, 2016 at 3:00 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Fri, Sep 2, 2016 at 12:22 PM, Dakota Hawkins <dakotahawkins@gmail.com> wrote:
>>> Below is a simple reproduction of the issue.
>>>
>>> The _real_ problem is that this is how our pull request merges work,
>>
>> So your workflow is the problem or is the actual bug just exposed in
>> your workflow?
>
> I believe the workflow just exposes the problem, which is that
> generically for this case a fast-forward merge works without conflicts
> while a non-fast-forward merge fails with conflicts. Sorry if this
> confuses the issue, it's just how we experienced it so I wanted to add
> that background.
>
>>> ## Test fast-forward merge, this will work
>>> git checkout -b merge-ff-test master # warning: unable to rmdir
>>> submodule-location-2: Directory not empty
>>> rm -rf ./submodule-location-2
>>> git merge --ff-only move-submodule
>>>
>>
>> And no reset/rm in between, i.e. we still have
>> submodule-location-2 from merge-ff-test still around?
>
> That is true in that example, but somewhat immaterial. The first merge
> was only to demonstrate that a fast-forward merge works without
> conflict. The simplest reproduction is to skip that and get straight
> to the failure case:
>
> ## Clone and setup branches
> git clone https://github.com/dakotahawkins/submodule-move-merge-bug-main-repo.git
> cd submodule-move-merge-bug-main-repo
> git branch move-submodule origin/move-submodule
> git checkout -b merge-no-ff-test master
>
> ## This will fail
> git merge --no-ff move-submodule
> # Auto-merging submodule-location-2
> # Adding as submodule-location-2~move-submodule instead
> # Automatic merge failed; fix conflicts and then commit the result.
>
> Does that make things a little bit clearer?
>
> Dakota
^ permalink raw reply
* Re: [RFC/PATCH] ls-files: adding support for submodules
From: Stefan Beller @ 2016-09-09 23:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <xmqqh99o8ypm.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 9, 2016 at 4:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> Allow ls-files to recognize submodules in order to retrieve a list of
>> files from a repository's submodules. This is done by forking off a
>> process to recursively call ls-files on all submodules.
>
> While I see why "ls-files --recurse-submodules" sounds nice ("hey, I
> can get list of _all_ the files here"), and I am quite happy with
> the quality of implementation (not just the code but its
> documentation and test)
The plan is to hook the ls-files machinery into
git-grep as the way of obtaining files to grep for a pattern.
As git-grep has a thread pool already, getting the list of files
single threaded is more beneficial as e.g. calling git-grep recursively
for submodules, as then you may have a fork bomb.
>
>> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
> We used to do that when we didn't know better.
>
Oops, a lot of stuff slipped through the first internal review.
^ permalink raw reply
* Re: If a branch moves a submodule, "merge --ff[-only]" succeeds while "merge --no-ff" fails with conflicts
From: Stefan Beller @ 2016-09-09 23:57 UTC (permalink / raw)
To: Dakota Hawkins; +Cc: Git Mailing List, mwitte
In-Reply-To: <CAG0BQXmyW-Hzdyj4rTviToEVbJ73d94P9GcfFVR6P2XCsA2t-g@mail.gmail.com>
On Fri, Sep 9, 2016 at 4:37 PM, Dakota Hawkins <dakotahawkins@gmail.com> wrote:
> Any ideas on this anywhere?
>
> In my opinion if a merge can fast-forward without conflict it should
> trivially be able to non-fast-forward without conflict.
Yes, I agree. Though the submodules still have a lot of sharp edges
that you can injure yourself with.
I am sorry to let you down here, but I did not have time and mental
capacity to actually dive into the issue further this week.
I hoped someone else would have picked up this thread and have
a good idea.
As you know currently the checkout doesn't touch submodules, i.e.
you have to run "git submodule update" whenever the submodule
changes. So when you checkout a different part of history, that moved
a submodule, this will fail as the submodule still resides at the old
place (and may have path name conflict with another thing)
and is not there at the new place.
I plan to fix that in the near future (read: teach git checkout to know
about submodules), mind that there is already a patch series for
exactly that out there in one of the branches of
https://github.com/jlehmann/git-submod-enhancements
This seems to be not an easy fix; Someone tried already and getting
it polished enough to include it upstream was not successful.
>
> Also, I'm not too familiar/comfortable with mailing list etiquette,
> and I don't want to be a bother by continuing to ping this thread.
Pinging is fine, as it is rather easy to ignore mails on a mailing list. ;)
I just don't know if it increases likelihood of someone responding.
Thanks,
Stefan
^ permalink raw reply
* Re: If a branch moves a submodule, "merge --ff[-only]" succeeds while "merge --no-ff" fails with conflicts
From: Dakota Hawkins @ 2016-09-10 0:17 UTC (permalink / raw)
To: Stefan Beller; +Cc: Git Mailing List, mwitte
In-Reply-To: <CAGZ79kapz7PR7vO2oyWE8WygXr+hTSJeZfZgpyeERv+Z1jTjCQ@mail.gmail.com>
> As you know currently the checkout doesn't touch submodules, i.e.
> you have to run "git submodule update" whenever the submodule
> changes. So when you checkout a different part of history, that moved
> a submodule, this will fail as the submodule still resides at the old
> place (and may have path name conflict with another thing)
> and is not there at the new place.
I _think_ (may be wrong) that you can reproduce this without ever
updating the submodules at all. I may be missing/forgetting something
in my config: I normally have global post-checkout and post-merge
hooks to run a submodule update, however I realized that and (I think)
disabled them for the second more concise reproduction I sent. I moved
them to a "no" subdirectory in my hook directory -- git doesn't
recursively look for hooks, does it? Anyway, I don't think that's a
difference in my case. It may be, I agree, if you happen to be
reproducing this issue locally in your working repository for some
reason.
That thought process makes sense to me because as I mentioned
initially these failures presented on our remote (bitbucket server, if
it matters) for a pull-request merge. I don't know for certain but I
don't believe the server's copy of the repo would need to update the
submodules at all, ever.
>>
>> Also, I'm not too familiar/comfortable with mailing list etiquette,
>> and I don't want to be a bother by continuing to ping this thread.
>
> Pinging is fine, as it is rather easy to ignore mails on a mailing list. ;)
> I just don't know if it increases likelihood of someone responding.
Apparently it got you on the hook! Thanks for taking the bait :)
Dakota
^ permalink raw reply
* Re: git clone http:// fails some times with "Request for d53.. aborted"
From: Eric Wong @ 2016-09-10 0:21 UTC (permalink / raw)
To: Yaroslav Halchenko; +Cc: git
In-Reply-To: <20160909221942.GS9830@onerussian.com>
(Not sure how much time I'll have to continue in the next few weeks,
just jotting down my debugging progress so far...)
Yaroslav Halchenko <yoh@onerussian.com> wrote:
> even when (v 2.7.0) ran on the box where the server is, so
> unlikely to be network issue
>
> or from my laptop (v 2.9.3) with ok but wifi with a weakish signal to the
> access point:
>
> $> ( set -e; for s in {1..100}; do rm -rf fbirn_phaseIII ; git clone http://datasets.datalad.org/nidm/fbirn_phaseIII/.git; done; )
> Cloning into 'fbirn_phaseIII'...
> Checking connectivity... done.
> Cloning into 'fbirn_phaseIII'...
> error: Request for d53302dfc7ad13b786923927021039d21a10d5bd aborted
> error: Unable to find d53302dfc7ad13b786923927021039d21a10d5bd under http://datasets.datalad.org/nidm/fbirn_phaseIII/.git
> Cannot obtain needed tree d53302dfc7ad13b786923927021039d21a10d5bd
> while processing commit 22dd4c49417cad6f4082ac2aebef45da8b6e473d.
> error: fetch failed.
>
> even if I build fresh 2.10.0, discovered that we can trace now CURL calls
Also, GIT_CURL_VERBOSE=1 is useful, too
And GIT_HTTP_MAX_REQUESTS=1 (default 5) can be used to limit concurrency
if there's a suspected concurrency bug.
Initially, I was worried some of my 2.10.0 http-walker speedups were
responsible, but it does not seem to be the case since it happens on old
versions, too...
I'm trying with the following to track state==ABORTED requests:
diff --git a/http-walker.c b/http-walker.c
index 0b24255..4f25b07 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -56,6 +56,7 @@ static void start_object_request(struct walker *walker,
req = new_http_object_request(obj_req->repo->base, obj_req->sha1);
if (req == NULL) {
+ warning("obj aborted at %d", __LINE__);
obj_req->state = ABORTED;
return;
}
diff --git a/http.c b/http.c
index cd40b01..cac5db9 100644
--- a/http.c
+++ b/http.c
@@ -1022,6 +1022,8 @@ int start_active_slot(struct active_request_slot *slot)
if (curlm_result != CURLM_OK &&
curlm_result != CURLM_CALL_MULTI_PERFORM) {
+ warning("curl_multi_add_handle failed: %s",
+ curl_multi_strerror(curlm_result));
active_requests--;
slot->in_use = 0;
return 0;
And getting a few of the following:
warning: curl_multi_add_handle failed: The easy handle is already added to a multi handle
(error code: CURLM_ADDED_ALREADY)
I suspect there is some state management bug. And yes, it's
intermittent, I'm testing from my server (dcvr.yhbt.net) with good
connectivity and it does not happen all the time.
It also happens regardless of GIT_HTTP_MAX_REQUESTS being 1 or 5
(default), too.
So yeah, hopefully this info is helpful to someone else on the list
in case I don't return soon.
^ permalink raw reply related
* Re: [PATCH v4 1/3] tests: move test_lazy_prereq JGIT to test-lib.sh
From: Torsten Bögershausen @ 2016-09-10 5:51 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, jrnieder, spearce, sbeller, gitster, peff
In-Reply-To: <37c4e775948b88b2d5af1caee3a70e36f00bc88a.1473441620.git.jonathantanmy@google.com>
On Fri, Sep 09, 2016 at 10:36:28AM -0700, Jonathan Tan wrote:
[]
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index d731d66..c9c1037 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '
> test "$uid" != 0
> '
>
> +test_lazy_prereq JGIT '
> + type jgit
> +'
> +
Minor note:
Typically the stdout of `which` is suppressed like this:
if ! type cvs >/dev/null 2>&1
^ permalink raw reply
* Re: [PATCH v4 1/3] tests: move test_lazy_prereq JGIT to test-lib.sh
From: Jeff King @ 2016-09-10 6:00 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Jonathan Tan, git, jrnieder, spearce, sbeller, gitster
In-Reply-To: <20160910055119.GA11001@tb-raspi>
On Sat, Sep 10, 2016 at 05:51:19AM +0000, Torsten Bögershausen wrote:
> > +test_lazy_prereq JGIT '
> > + type jgit
> > +'
> > +
>
> Minor note:
> Typically the stdout of `which` is suppressed like this:
>
> if ! type cvs >/dev/null 2>&1
But we don't want to suppress the output here; the test harness
redirects the lazy_prereq output to /dev/null unless you specify "-v".
And there is no need for "if", because what we care about is the exit
code of the commands inside the lazy_prereq.
-Peff
^ permalink raw reply
* Re: [PATCH v7 10/10] convert: add filter.<driver>.process option
From: Torsten Bögershausen @ 2016-09-10 6:29 UTC (permalink / raw)
To: larsxschneider
Cc: git, peff, gitster, sbeller, Johannes.Schindelin, jnareb,
mlbright, jacob.keller
In-Reply-To: <20160908182132.50788-11-larsxschneider@gmail.com>
On Thu, Sep 08, 2016 at 08:21:32PM +0200, larsxschneider@gmail.com wrote:
[]
> +packet: git> git-filter-client
> +packet: git> version=2
> +packet: git> version=42
> +packet: git> 0000
> +packet: git< git-filter-server
> +packet: git< version=2
> +packet: git> clean=true
> +packet: git> smudge=true
> +packet: git> not-yet-invented=true
> +packet: git> 0000
> +packet: git< clean=true
> +packet: git< smudge=true
> +packet: git< 0000
It's probalby only me who has difficulties to distinguish
'>' from '<'.
packet: git> git-filter-client
packet: git> version=2
packet: git> version=42
packet: git> 0000
packet: filter> git-filter-server
packet: filter> version=2
(Otherwise the dialoge description is nice)
> +------------------------
> +Supported filter capabilities in version 2 are "clean" and
> +"smudge".
> +
> +Afterwards Git sends a list of "key=value" pairs terminated with
> +a flush packet. The list will contain at least the filter command
> +(based on the supported capabilities) and the pathname of the file
> +to filter relative to the repository root. Right after these packets
> +Git sends the content split in zero or more pkt-line packets and a
> +flush packet to terminate content.
> +------------------------
> +packet: git> command=smudge\n
> +packet: git> pathname=path/testfile.dat\n
How do we send pathnames the have '\n' ?
Not really recommended, but allowed.
And here I am a little bit lost, is each of the lines packed into
a pkt-line ?
command=smudge is packet as pkt-line and pathname= is packed into
another one ? (The we don't need the '\n' at all)
Or go both lines into one pkt-line (thats what I think), then
we don't need the '\n' afther the pathname.
And in this case the pathname is always the last element, and a '\n'
may occur in the pathname, since we know the length of the packet
we know how long the pathname must be.
> +packet: git> 0000
> +packet: git> CONTENT
> +packet: git> 0000
> +------------------------
> +
> +The filter is expected to respond with a list of "key=value" pairs
> +terminated with a flush packet. If the filter does not experience
> +problems then the list must contain a "success" status. Right after
> +these packets the filter is expected to send the content in zero
> +or more pkt-line packets and a flush packet at the end. Finally, a
> +second list of "key=value" pairs terminated with a flush packet
> +is expected. The filter can change the status in the second list.
> +------------------------
> +packet: git< status=success\n
> +packet: git< 0000
> +packet: git< SMUDGED_CONTENT
> +packet: git< 0000
> +packet: git< 0000 # empty list!
> +------------------------
> +
> +If the result content is empty then the filter is expected to respond
> +with a success status and an empty list.
> +------------------------
> +packet: git< status=success\n
> +packet: git< 0000
> +packet: git< 0000 # empty content!
> +packet: git< 0000 # empty list!
> +------------------------
> +
> +In case the filter cannot or does not want to process the content,
Does not want ?
I can see something like "I read through the file, there is nothing
to do. So Git, I don't send anything back, you know where the file is.
> +it is expected to respond with an "error" status. Depending on the
> +`filter.<driver>.required` flag Git will interpret that as error
> +but it will not stop or restart the filter process.
> +------------------------
> +packet: git< status=error\n
> +packet: git< 0000
> +------------------------
> +
> +If the filter experiences an error during processing, then it can
> +send the status "error" after the content was (partially or
> +completely) sent. Depending on the `filter.<driver>.required` flag
> +Git will interpret that as error but it will not stop or restart the
> +filter process.
> +------------------------
> +packet: git< status=success\n
> +packet: git< 0000
> +packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT
> +packet: git< 0000
> +packet: git< status=error\n
> +packet: git< 0000
> +------------------------
> +
> +If the filter dies during the communication or does not adhere to
> +the protocol then Git will stop the filter process and restart it
My personal comment:
When a filter is mis-behaving, Git should say so loud and clear, and
die().
The filter process can be left running, so that it can be debugged.
Here I stopped the review for a moment,
I still think that Git shouldn't kill anything, because we loose
the ability to debug these processes.
^ permalink raw reply
* Re: [PATCH] git-gui: respect commit.gpgsign again
From: Johannes Schindelin @ 2016-09-10 7:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Pat Thoyts
In-Reply-To: <xmqqr38tapv0.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 9 Sep 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > As of v2.9.0, `git commit-tree` no longer heeds the `commit.gpgsign`
> > config setting. This broke committing in Git GUI.
>
> Thanks. Will shift it up to apply to my copy of git-gui project and
> then pull in the result.
Thanks. There are a couple more git-gui patches waiting for quite a long
time. So you prefer them still as patches to git-gui.git?
Also, I just noticed poor wording. Would you mind fixing it up by
s/committing/& with GPG signature/
?
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH v3 2/4] cat-file: introduce the --filters option
From: Johannes Schindelin @ 2016-09-10 7:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Torsten Bögershausen, Jeff King
In-Reply-To: <xmqqr38tc7d1.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 9 Sep 2016, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > So I would not mind if we define the semantics of "--filters" as
> > such (as long as it is clearly documented, of course). AFAICS, the
> > batch interface does not call filter_object() for non-blobs, and by
> > returning successfully without doing anything special for a symbolic
> > link from filter_object() automatically gives us the "by default
> > return as-is, but give processed output only for regular file blobs"
> > semantics to the batch mode.
> >
> > But for a non-batch mode, it feels somewhat funny to be giving the
> > as-is output without saying anything to symbolic links; we can argue
> > that it is being consistent with what we do in the batch mode,
> > though.
>
> In other words, instead of trying to be consistent by erroring out
> in non-regular blob case, I think the attached change on top would
> make more sense, by consistently passing the object contents as-is
> for all "not filtered" cases, whether it is run from the batch mode
> or from the command line.
>
> builtin/cat-file.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index f8a3a08..99cb525 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -33,12 +33,7 @@ static int filter_object(const char *path, unsigned mode,
> if (!*buf)
> return error(_("cannot read object %s '%s'"),
> sha1_to_hex(sha1), path);
> - if (type != OBJ_BLOB) {
> - free(*buf);
> - return error(_("blob expected for %s '%s'"),
> - sha1_to_hex(sha1), path);
> - }
> - if (S_ISREG(mode)) {
> + if ((type == OBJ_BLOB) && S_ISREG(mode)) {
> struct strbuf strbuf = STRBUF_INIT;
> if (convert_to_working_tree(path, *buf, *size, &strbuf)) {
> free(*buf);
Yes, that makes most sense to me, too.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 3/4] cat-file --textconv/--filters: allow specifying the path separately
From: Johannes Schindelin @ 2016-09-10 7:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Torsten Bögershausen, Jeff King
In-Reply-To: <xmqq4m5pc5hx.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 9 Sep 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > +test_expect_success '----path=<path> complains without --textconv/--filters' '
>
> I wonder where this "----path" came from; it wasn't in v2 I queued,
> but somehow came back mysteriously.
No idea how it crept back in. Sorry.
> Will locally amend.
Thanks!
Dscho
^ permalink raw reply
* Re: Missing RPM spec file in tarball
From: Johannes Schindelin @ 2016-09-10 8:15 UTC (permalink / raw)
To: Stefan Beller; +Cc: Sergio Martín Turiel, git@vger.kernel.org
In-Reply-To: <CAGZ79kYu=0ik9W9g5yCbc5142_a2fb1er45TFEZ6RrekZRZM9w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 388 bytes --]
Hi Stefan,
On Fri, 9 Sep 2016, Stefan Beller wrote:
> On Fri, Sep 9, 2016 at 9:19 AM, Sergio Martín Turiel
> <sergio.martin@accelya.com> wrote:
>
> > Can you tell me what I'm doing wrong?
>
> Not crying out loud when that commit was discussed on the
> mailing list. ;)
Umm, I think it would be more: "Not stepping up to maintain the RPM
specs"...
;-)
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 04/13] i18n: blame: mark error messages for translation
From: Jean-Noël AVILA @ 2016-09-10 9:41 UTC (permalink / raw)
To: Vasco Almeida; +Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason
In-Reply-To: <1473259758-11836-4-git-send-email-vascomalmeida@sapo.pt>
On mercredi 7 septembre 2016 14:49:08 CEST Vasco Almeida wrote:
> Mark error messages for translation passed to die() function.
> Change "Cannot" to lowercase following the usual style.
>
> Reflect changes to test by using test_i18ngrep.
>
> Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
> ---
> builtin/blame.c | 12 ++++++------
> t/t8003-blame-corner-cases.sh | 4 ++--
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index a5bbf91..3fee197 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2601,7 +2601,7 @@ int cmd_blame(int argc, const char **argv, const char
> *prefix)
>
> if (incremental || (output_option & OUTPUT_PORCELAIN)) {
> if (show_progress > 0)
> - die("--progress can't be used with --incremental or porcelain
formats");
> + die(_("--progress can't be used with --incremental or porcelain
> formats")); show_progress = 0;
> } else if (show_progress < 0)
> show_progress = isatty(2);
> @@ -2727,7 +2727,7 @@ int cmd_blame(int argc, const char **argv, const char
> *prefix) sb.commits.compare = compare_commits_by_commit_date;
> }
> else if (contents_from)
> - die("--contents and --reverse do not blend well.");
> + die(_("--contents and --reverse do not blend well."));
> else {
> final_commit_name = prepare_initial(&sb);
> sb.commits.compare = compare_commits_by_reverse_commit_date;
> @@ -2747,12 +2747,12 @@ int cmd_blame(int argc, const char **argv, const
> char *prefix) add_pending_object(&revs, &(sb.final->object), ":");
> }
> else if (contents_from)
> - die("Cannot use --contents with final commit object name");
> + die(_("cannot use --contents with final commit object name"));
>
> if (reverse && revs.first_parent_only) {
> final_commit = find_single_final(sb.revs, NULL);
> if (!final_commit)
> - die("--reverse and --first-parent together require specified latest
> commit"); + die(_("--reverse and --first-parent together require
> specified latest commit")); }
>
> /*
> @@ -2779,7 +2779,7 @@ int cmd_blame(int argc, const char **argv, const char
> *prefix) }
>
> if (oidcmp(&c->object.oid, &sb.final->object.oid))
> - die("--reverse --first-parent together require range along first-
parent
> chain"); + die(_("--reverse --first-parent together require range along
> first-parent chain")); }
>
> if (is_null_oid(&sb.final->object.oid)) {
> @@ -2820,7 +2820,7 @@ int cmd_blame(int argc, const char **argv, const char
> *prefix) &bottom, &top, sb.path))
> usage(blame_usage);
> if (lno < top || ((lno || bottom) && lno < bottom))
> - die("file %s has only %lu lines", path, lno);
> + die(_("file %s has only %lu lines"), path, lno);
Here a plural version is needed.
> if (bottom < 1)
> bottom = 1;
> if (top < 1)
> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
> index e48370d..661f9d4 100755
> --- a/t/t8003-blame-corner-cases.sh
> +++ b/t/t8003-blame-corner-cases.sh
> @@ -212,12 +212,12 @@ EOF
>
> test_expect_success 'blame -L with invalid start' '
> test_must_fail git blame -L5 tres 2>errors &&
> - grep "has only 2 lines" errors
> + test_i18ngrep "has only 2 lines" errors
> '
>
> test_expect_success 'blame -L with invalid end' '
> test_must_fail git blame -L1,5 tres 2>errors &&
> - grep "has only 2 lines" errors
> + test_i18ngrep "has only 2 lines" errors
> '
>
> test_expect_success 'blame parses <end> part of -L' '
^ permalink raw reply
* Re: git commit -p with file arguments
From: Jakub Narębski @ 2016-09-10 9:52 UTC (permalink / raw)
To: Christian Neukirchen; +Cc: git, Junio C Hamano, Jacob Keller
In-Reply-To: <87inu4bxt7.fsf@juno.home.vuxu.org>
W dniu 09.09.2016 o 22:52, Christian Neukirchen napisał:
> Jakub Narębski <jnareb@gmail.com> writes:
>
>> Which means that with "git add -p <pathspec> && git commit <pathspec>",
>> the "git add -p <pathspec>" would carefully craft the <pathspec> state
>> in the index... and "git commit <pathspec>" would take worktree version
>> of <pathspec> for commit, ignoring what was in the index :-(
>>
>> Currently there is no way to create commit out of subset of the index,
>> e.g. with "git commit :0:<path>"
>
> I played around with creating a new index just for "add -p" and then
> committing that one. Seems to have worked...
>
> Perhaps I'll just wrap git-commit myself then.
What I wanted to say is that there is no built-in way to create commit
out of subset of the index. You can of course do what "git commit <file>"
does, that is use temporary index file (GIT_INDEX_FILE etc.; see
contrib/examples/git-commit.sh).
I wonder, if git-commit is to acquire such feature, what would be the
best interface. "git commit :0:./<path>"? "git commit -o -p <path>"
(that is, "git commit --only --patch <pathspec>")?
--
Jakub Narębski
^ permalink raw reply
* Re: How to simulate a real checkout to test a new smudge filter?
From: Jakub Narębski @ 2016-09-10 10:32 UTC (permalink / raw)
To: john smith, Junio C Hamano; +Cc: Torsten Bögershausen, git
In-Reply-To: <CAKmQUfYCUq=kUTau1-3NjjPVBk6WkP3KdTjgMC8sZtU8h=H4iQ@mail.gmail.com>
W dniu 10.09.2016 o 01:07, john smith pisze:
> On 9/10/16, Junio C Hamano <gitster@pobox.com> wrote:
>> The clean and smudge operations should look _only_ at the contents
>> they are filtering, and nothing else, and the clean/smudge filtering
>> mechanism is designed to support that use case. It is not designed
>> to do things like embedding the name of the branch that is being
>> checked out into the result.
>
> Ok, I think I get it. It was actually my original problem with smudge
> filters because I wanted them to be run after *every* checkout even if
> file contents stayed the same (hence the subject).
And that's not the case, for performance reasons.
>
> Now Jakub suggested post-checkout hook in conjunction with only clean
> filter for my problem of managing dotfiles but I think I don't fully
> get it. The problem is that in the scenario presented in my last
> e-mail clean filter is run in the situation which doesn't like a
> checkin to me. Is `git checkout <PATH>' doing a *checkin*" under the
> hood so that the clean filter is called? What does actually `checkin'
> mean in Git? I thought that checkin it's the same as committing a
> file into the repository.
I was wrong, I'm sorry. My mistake.
You would need post-checkout hook together with clean / smudge filters
(though you could get by without smudge filter, at least in theory...).
The `post-checkout` hook could run e.g. "git checkout -- '*.conf'"
to force use of smudge filter, after checking that it was a change
of branch ("git checkout <commit-ish>"). See githooks(5) for details
(last, third parameter passed to hook script is 1, rather than 0).
Unfortunately I don't see a way to query .gitattributes files to
find out all patterns that match specific attribute; you would need
to duplicate configuration:
.gitattributes:
*.conf filter=transform
.git/config
[filter "transform"]
clean = replace-with-placeholder %f
smudge = expand-with-branchname %f
.git/hooks/post-checkout
#!/bin/sh
test "$3" -eq "1" && git checkout -- '*.conf'
Or something like that.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 1/2 v5] pack-objects: respect --local/--honor-pack-keep/--incremental when bitmap is in use
From: Kirill Smelkov @ 2016-09-10 14:57 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Vicent Marti, Jérome Perrin, Isabelle Vallet,
Kazuhiko Shiozaki, Julien Muchembled, git
In-Reply-To: <20160818175222.bmm3ivjheokf2qzl@sigill.intra.peff.net>
On Thu, Aug 18, 2016 at 01:52:22PM -0400, Jeff King wrote:
> On Tue, Aug 09, 2016 at 10:31:43PM +0300, Kirill Smelkov wrote:
>
> > Since 6b8fda2d (pack-objects: use bitmaps when packing objects) there
> > are two codepaths in pack-objects: with & without using bitmap
> > reachability index.
>
> Sorry, I got distracted from reviewing these patches. I'll give them a
> detailed look now and hopefully we can finalize the topic.
Jeff, thanks for feedback. On my side I'm sorry for the delay because I
was travelling and only recently got back to work.
> > In want_object_in_pack() we care to start the checks from already found
> > pack, if we have one, this way determining the answer right away
> > in case neither --local nor --honour-pack-keep are active. In
> > particular, as p5310-pack-bitmaps.sh shows, we do not do harm to
> > served-with-bitmap clones performance-wise:
> >
> > Test 56dfeb62 this tree
> > -----------------------------------------------------------------
> > 5310.2: repack to disk 9.63(8.67+0.33) 9.47(8.55+0.28) -1.7%
> > 5310.3: simulated clone 2.07(2.17+0.12) 2.03(2.14+0.12) -1.9%
> > 5310.4: simulated fetch 0.78(1.03+0.02) 0.76(1.00+0.03) -2.6%
> > 5310.6: partial bitmap 1.97(2.43+0.15) 1.92(2.36+0.14) -2.5%
> >
> > with all differences strangely showing we are a bit faster now, but
> > probably all being within noise.
>
> Good to know there is no regression. It is curious that there is a
> slight _improvement_ across the board. Do we have an explanation for
> that? It seems odd that noise would be so consistent.
Yes, I too thought it and it turned out to be t/perf/run does not copy
config.mak.autogen & friends to build/ and I'm using autoconf with
CFLAGS="-march=native -O3 ..."
Junio, I could not resist to the following:
---- 8< ----
From: Kirill Smelkov <kirr@nexedi.com>
Subject: [PATCH] t/perf/run: Don't forget to copy config.mak.autogen & friends
to build area
Otherwise for people who use autotools-based configure in main worktree,
the performance testing results will be inconsistent as work and build
trees could be using e.g. different optimization levels.
See e.g.
http://public-inbox.org/git/20160818175222.bmm3ivjheokf2qzl@sigill.intra.peff.net/
for example.
NOTE config.status has to be copied because otherwise without it the build
would want to run reconfigure this way loosing just copied config.mak.autogen.
Signed-off-by: Kirill Smelkov <kirr@nexedi.com>
---
t/perf/run | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/perf/run b/t/perf/run
index cfd7012..aa383c2 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -30,7 +30,7 @@ unpack_git_rev () {
}
build_git_rev () {
rev=$1
- cp ../../config.mak build/$rev/config.mak
+ cp -t build/$rev ../../{config.mak,config.mak.autogen,config.status}
(cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||
die "failed to build revision '$mydir'"
}
--
2.9.2.701.gf965a18.dirty
---- 8< ----
With corrected t/perf/run the timings are more realistic - e.g. 3
consecutive runs of `./run 56dfeb62 . ./p5310-pack-bitmaps.sh`:
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.08(8.20+0.25) 9.09(8.14+0.32) +0.1%
5310.3: simulated clone 1.92(2.12+0.08) 1.93(2.12+0.09) +0.5%
5310.4: simulated fetch 0.82(1.07+0.04) 0.82(1.06+0.04) +0.0%
5310.6: partial bitmap 1.96(2.42+0.13) 1.95(2.40+0.15) -0.5%
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.11(8.16+0.32) 9.11(8.19+0.28) +0.0%
5310.3: simulated clone 1.93(2.14+0.07) 1.92(2.11+0.10) -0.5%
5310.4: simulated fetch 0.82(1.06+0.04) 0.82(1.04+0.05) +0.0%
5310.6: partial bitmap 1.95(2.38+0.16) 1.94(2.39+0.14) -0.5%
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.13(8.17+0.31) 9.07(8.13+0.28) -0.7%
5310.3: simulated clone 1.92(2.13+0.07) 1.91(2.12+0.06) -0.5%
5310.4: simulated fetch 0.82(1.08+0.03) 0.82(1.08+0.03) +0.0%
5310.6: partial bitmap 1.96(2.43+0.14) 1.96(2.42+0.14) +0.0%
> > And in the general case we care not to have duplicate
> > find_pack_entry_one(*found_pack) calls. Worst what can happen is we can
> > call want_found_object(*found_pack) -- newly introduced helper for
> > checking whether we want object -- twice, but since want_found_object()
> > is very lightweight it does not make any difference.
>
> I had trouble parsing this. I think maybe:
>
> In the general case we do not want to call find_pack_entry_one() more
> than once, because it is expensive. This patch splits the loop in
> want_object_in_pack() into two parts: finding the object and seeing if
> it impacts our choice to include it in the pack. We may call the
> inexpensive want_found_object() twice, but we will never call
> find_pack_entry_one() if we do not need to.
Ok, thanks for the advice.
>
> > +static int want_found_object(int exclude, struct packed_git *p)
> > +{
> > + if (exclude)
> > + return 1;
> > + if (incremental)
> > + return 0;
> > +
> > + /*
> > + * When asked to do --local (do not include an object that appears in a
> > + * pack we borrow from elsewhere) or --honor-pack-keep (do not include
> > + * an object that appears in a pack marked with .keep), finding a pack
> > + * that matches the criteria is sufficient for us to decide to omit it.
> > + * However, even if this pack does not satisfy the criteria, we need to
> > + * make sure no copy of this object appears in _any_ pack that makes us
> > + * to omit the object, so we need to check all the packs. Signal that by
> > + * returning -1 to the caller.
> > + */
> > + if (!ignore_packed_keep &&
> > + (!local || !have_non_local_packs))
> > + return 1;
>
> Hmm. The comment says "-1", but the return says "1". That is because the
> comment is describing the return that happens at the end. :)
>
> I wonder if the last sentence should be:
>
> We can check here whether these options can possibly matter; if not,
> we can return early from the function here. Otherwise, we signal "-1"
> at the end to tell the caller that we do not know either way, and it
> needs to check more packs.
Thanks for the catch and hint. I've changed it to the following:
We can however first check whether these options can possible matter;
if they do not matter we know we want the object in generated pack.
Otherwise, we signal "-1" at the end to tell the caller that we do
not know either way, and it needs to check more packs.
full version:
/*
* When asked to do --local (do not include an object that appears in a
* pack we borrow from elsewhere) or --honor-pack-keep (do not include
* an object that appears in a pack marked with .keep), finding a pack
* that matches the criteria is sufficient for us to decide to omit it.
* However, even if this pack does not satisfy the criteria, we need to
* make sure no copy of this object appears in _any_ pack that makes us
* to omit the object, so we need to check all the packs.
*
* We can however first check whether these options can possible matter;
* if they do not matter we know we want the object in generated pack.
* Otherwise, we signal "-1" at the end to tell the caller that we do
* not know either way, and it needs to check more packs.
*/
Hope it is ok.
> > - *found_pack = NULL;
> > - *found_offset = 0;
> > + /*
> > + * If we already know the pack object lives in, start checks from that
> > + * pack - in the usual case when neither --local was given nor .keep files
> > + * are present we will determine the answer right now.
> > + */
> > + if (*found_pack) {
> > + want = want_found_object(exclude, *found_pack);
> > + if (want != -1)
> > + return want;
> > + }
>
> Looks correct. Though it is not really "start checks from..." anymore,
> but rather "do a quick check to see if we can quit early, and otherwise
> start the loop". That might be nitpicking, though.
I see. Your version is ok, but to me 'start checks from ...' is a bit
more natural and explaining (yes, all subjective and depending on
taste), so if possible I'd prefer to leave it as is.
>
> > for (p = packed_git; p; p = p->next) {
> > - off_t offset = find_pack_entry_one(sha1, p);
> > + off_t offset;
> > +
> > + if (p == *found_pack)
> > + offset = *found_offset;
> > + else
> > + offset = find_pack_entry_one(sha1, p);
> > +
>
> This hunk will conflict with the MRU optimizations in 'next', but I
> think the resolution should be pretty trivial.
Yes.
> > static int add_object_entry(const unsigned char *sha1, enum object_type type,
> > const char *name, int exclude)
> > {
> > - struct packed_git *found_pack;
> > - off_t found_offset;
> > + struct packed_git *found_pack = NULL;
> > + off_t found_offset = 0;
>
> I think technically we don't need to initialize found_offset here (it is
> considered only if *found_pack is not NULL), but it doesn't hurt to make
> our starting assumptions clear.
Yes, found_pack != NULL is indicator whether we have found_pack /
found_offset info, but it makes it much clear and defending from
mistakes to set both found_{pack,offset} into known initial state.
> > @@ -1073,6 +1097,9 @@ static int add_object_entry_from_bitmap(const unsigned char *sha1,
> > if (have_duplicate_entry(sha1, 0, &index_pos))
> > return 0;
> >
> > + if (!want_object_in_pack(sha1, 0, &pack, &offset))
> > + return 0;
> > +
>
> And this caller doesn't need to worry about initialization, because of
> course it knows it has a pack/offset already. Good.
Yes, we have this info from bitmap walker calling us.
> > diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> > index 3893afd..a278d30 100755
> > --- a/t/t5310-pack-bitmaps.sh
> > +++ b/t/t5310-pack-bitmaps.sh
>
> Tests look OK. I saw a few style nitpicks, but I think they are not even
> against our style guide but more "I would have written it like this" and
> are not even worth quibbling over.
>
> So I think the code here is fine, and I just had a few minor complaints
> on comment and commit message clarity.
Thanks for feedback. Yes tastes can differ but your comments regarding
commit message and want_found_object() were objectively (imho) worth it
and there I've made the adjustments.
Please expect updated patch to be send as reply to this mail.
Thanks again for feedback,
Kirill
^ permalink raw reply related
* Re: [PATCH 2/2 v7] pack-objects: use reachability bitmap index when generating non-stdout pack
From: Kirill Smelkov @ 2016-09-10 14:59 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Vicent Marti, Jérome Perrin, Isabelle Vallet,
Kazuhiko Shiozaki, Julien Muchembled, git
In-Reply-To: <20160818180615.q25p57v35m2xxtww@sigill.intra.peff.net>
On Thu, Aug 18, 2016 at 02:06:15PM -0400, Jeff King wrote:
> On Tue, Aug 09, 2016 at 10:32:17PM +0300, Kirill Smelkov wrote:
>
> > Subject: Re: [PATCH 2/2 v7] pack-objects: use reachability bitmap index when
> > generating non-stdout pack
>
> This is v7, but as I understand your numbering, it goes with v5 of patch
> 1/2 that I just reviewed (usually we just increment the version number
> on the whole series and treat it as a unit, even if some patches didn't
> change from version to version).
The reason those patches are having their own numbers is that they are
orthogonal to each other and can be applied / rejected independently.
Since I though Junio might want to pick them up as separate topics they
were versioned separately.
But ok, since now we have them considered both together, their next
versions posted will be uniform v8.
> > So we can teach pack-objects to use bitmap index for initial object
> > counting phase when generating resultant pack file too:
> >
> > - if we care it is not activated under git-repack:
>
> Do you mean "if we take care that it is not..." here?
>
> (I think you might just be getting tripped up in the English idioms;
> "care" means that we have a preference; "to take care" means that we are
> being careful).
Ok, I've might have been tripped and thanks for the catch up. I've changed to
"if we take care to not let it be activated under git-repack"
>
> > - if we know bitmap index generation is not enabled for resultant pack:
> >
> > Current code has singleton bitmap_git so cannot work simultaneously
> > with two bitmap indices.
>
> Minor English fixes:
>
> The current code has a singleton bitmap_git, so it cannot work
> simultaneously with two bitmap indices.
ok.
> > - if we keep pack reuse enabled still only for "send-to-stdout" case:
> >
> > Because on pack reuse raw entries are directly written out to destination
> > pack by write_reused_pack() bypassing needed for pack index generation
> > bookkeeping done by regular codepath in write_one() and friends.
>
> Ditto on English:
>
> On pack reuse raw entries are directly written out to the destination
> pack by write_reused_pack(), bypassing the need for pack index
> generation bookkeeping done by the regular code path in write_one()
> and friends.
>
> I think this is missing the implication. Why wouldn't we want to reuse
> in this case? Certainly we don't when doing a "careful" on-disk repack.
> I suspect the answer is that we cannot write a ".idx" off of the result
> of write_reused_pack(), and write-to-disk always includes the .idx.
Yes, mentioning pack-to-file needs to generate .idx makes it more clear
and thanks for pointing this out. I've changed this item to the
following (picking some of your English corrections):
- if we keep pack reuse enabled still only for "send-to-stdout" case:
Because pack-to-file needs to generate index for destination pack, and
currently on pack reuse raw entries are directly written out to the
destination pack by write_reused_pack(), bypassing needed for pack index
generation bookkeeping done by regular codepath in write_one() and
friends.
( In the future we might teach pack-reuse code about cases when index
also needs to be generated for resultant pack and remove
pack-reuse-only-for-stdout limitation )
Hope it is ok.
> > More context:
> >
> > http://marc.info/?t=146792101400001&r=1&w=2
>
> Can we turn this into a link to public-inbox? We have just been bit by
> all of our old links to gmane dying, and they cannot easily be replaced
> because they use a gmane-specific article number. public-inbox URLs use
> message-ids, which should be usable for other archives if public-inbox
> goes away.
Yes, makes sense to put msgid here. I've added
http://public-inbox.org/git/20160707190917.20011-1-kirr@nexedi.com/T/#t
> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index b1007f2..c92d7fc 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
>
> The code here looks fine.
Thanks.
> > diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> > index a278d30..9602e9a 100755
> > --- a/t/t5310-pack-bitmaps.sh
> > +++ b/t/t5310-pack-bitmaps.sh
> > @@ -196,6 +196,18 @@ test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '
> > ! has_any packbitmap.objects 3b.objects
> > '
> >
> > +test_expect_success 'pack-objects to file can use bitmap' '
> > + # make sure we still have 1 bitmap index from previous tests
> > + ls .git/objects/pack/ | grep bitmap >output &&
> > + test_line_count = 1 output &&
> > + # verify equivalent packs are generated with/without using bitmap index
> > + packasha1=$(git pack-objects --no-use-bitmap-index --all packa </dev/null) &&
> > + packbsha1=$(git pack-objects --use-bitmap-index --all packb </dev/null) &&
> > + list_packed_objects <packa-$packasha1.idx >packa.objects &&
> > + list_packed_objects <packb-$packbsha1.idx >packb.objects &&
> > + test_cmp packa.objects packb.objects
> > +'
>
> Of course we can't know if bitmaps were actually used, or if they were
> turned off under the hood. But at least this exercises the code a bit.
Yes, I was thinking how to know the bitmap codepath was actually active,
and without adding debugging points there is no way (at least I could
not find it).
> You could possibly add a perf test which shows off the improvement, but
> I don't think it's strictly necessary.
Good idea. I've added this
---- 8< ----
diff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh
index de2a224..bb91dbb 100755
--- a/t/perf/p5310-pack-bitmaps.sh
+++ b/t/perf/p5310-pack-bitmaps.sh
@@ -32,6 +32,14 @@ test_perf 'simulated fetch' '
} | git pack-objects --revs --stdout >/dev/null
'
+test_perf 'pack to file' '
+ git pack-objects --all pack1 </dev/null >/dev/null
+'
+
+test_perf 'pack to file (bitmap)' '
+ git pack-objects --use-bitmap-index --all pack1b </dev/null >/dev/null
+'
+
test_expect_success 'create partial bitmap state' '
# pick a commit to represent the repo tip in the past
cutoff=$(git rev-list HEAD~100 -1) &&
@@ -53,8 +61,12 @@ test_expect_success 'create partial bitmap state' '
git update-ref HEAD $orig_tip
'
-test_perf 'partial bitmap' '
+test_perf 'clone (partial bitmap)' '
git pack-objects --stdout --all </dev/null >/dev/null
'
+test_perf 'pack to file (partial bitmap)' '
+ git pack-objects --use-bitmap-index --all pack2b </dev/null >/dev/null
+'
+
test_done
---- 8< ----
Test 56dfeb62 this tree
--------------------------------------------------------------------------------
5310.2: repack to disk 8.98(8.05+0.29) 9.05(8.08+0.33) +0.8%
5310.3: simulated clone 2.02(2.27+0.09) 2.01(2.25+0.08) -0.5%
5310.4: simulated fetch 0.81(1.07+0.02) 0.81(1.05+0.04) +0.0%
5310.5: pack to file 7.58(7.04+0.28) 7.60(7.04+0.30) +0.3%
5310.6: pack to file (bitmap) 7.55(7.02+0.28) 3.25(2.82+0.18) -57.0%
5310.8: clone (partial bitmap) 1.83(2.26+0.12) 1.82(2.22+0.14) -0.5%
5310.9: pack to file (partial bitmap) 6.86(6.58+0.30) 2.87(2.74+0.20) -58.2%
Kirill
^ permalink raw reply related
* [PATCH 1/2 v8] pack-objects: respect --local/--honor-pack-keep/--incremental when bitmap is in use
From: Kirill Smelkov @ 2016-09-10 15:01 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Vicent Marti, Jérome Perrin, Isabelle Vallet,
Kazuhiko Shiozaki, Julien Muchembled, git, Kirill Smelkov
In-Reply-To: <20160910145738.x6ihp2gqzpk7dbi3@teco.navytux.spb.ru>
Since 6b8fda2d (pack-objects: use bitmaps when packing objects) there
are two codepaths in pack-objects: with & without using bitmap
reachability index.
However add_object_entry_from_bitmap(), despite its non-bitmapped
counterpart add_object_entry(), in no way does check for whether --local
or --honor-pack-keep or --incremental should be respected. In
non-bitmapped codepath this is handled in want_object_in_pack(), but
bitmapped codepath has simply no such checking at all.
The bitmapped codepath however was allowing to pass in all those options
and with bitmap indices still being used under such conditions -
potentially giving wrong output (e.g. including objects from non-local or
.keep'ed pack).
We can easily fix this by noting the following: when an object comes to
add_object_entry_from_bitmap() it can come for two reasons:
1. entries coming from main pack covered by bitmap index, and
2. object coming from, possibly alternate, loose or other packs.
"2" can be already handled by want_object_in_pack() and to cover
"1" we can teach want_object_in_pack() to expect that *found_pack can be
non-NULL, meaning calling client already found object's pack entry.
In want_object_in_pack() we care to start the checks from already found
pack, if we have one, this way determining the answer right away
in case neither --local nor --honour-pack-keep are active. In
particular, as p5310-pack-bitmaps.sh shows (3 consecutive runs), we do
not do harm to served-with-bitmap clones performance-wise:
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.08(8.20+0.25) 9.09(8.14+0.32) +0.1%
5310.3: simulated clone 1.92(2.12+0.08) 1.93(2.12+0.09) +0.5%
5310.4: simulated fetch 0.82(1.07+0.04) 0.82(1.06+0.04) +0.0%
5310.6: partial bitmap 1.96(2.42+0.13) 1.95(2.40+0.15) -0.5%
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.11(8.16+0.32) 9.11(8.19+0.28) +0.0%
5310.3: simulated clone 1.93(2.14+0.07) 1.92(2.11+0.10) -0.5%
5310.4: simulated fetch 0.82(1.06+0.04) 0.82(1.04+0.05) +0.0%
5310.6: partial bitmap 1.95(2.38+0.16) 1.94(2.39+0.14) -0.5%
Test 56dfeb62 this tree
-----------------------------------------------------------------
5310.2: repack to disk 9.13(8.17+0.31) 9.07(8.13+0.28) -0.7%
5310.3: simulated clone 1.92(2.13+0.07) 1.91(2.12+0.06) -0.5%
5310.4: simulated fetch 0.82(1.08+0.03) 0.82(1.08+0.03) +0.0%
5310.6: partial bitmap 1.96(2.43+0.14) 1.96(2.42+0.14) +0.0%
with delta timings showing they are all within noise from run to run.
In the general case we do not want to call find_pack_entry_one() more than
once, because it is expensive. This patch splits the loop in
want_object_in_pack() into two parts: finding the object and seeing if it
impacts our choice to include it in the pack. We may call the inexpensive
want_found_object() twice, but we will never call find_pack_entry_one() if we
do not need to.
I appreciate help and discussing this change with Junio C Hamano and
Jeff King.
Signed-off-by: Kirill Smelkov <kirr@nexedi.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/pack-objects.c | 97 ++++++++++++++++++++++++++++++++-----------------
t/t5310-pack-bitmaps.sh | 92 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 156 insertions(+), 33 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c4c2a3c..19668d3 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -944,13 +944,48 @@ static int have_duplicate_entry(const unsigned char *sha1,
return 1;
}
+static int want_found_object(int exclude, struct packed_git *p)
+{
+ if (exclude)
+ return 1;
+ if (incremental)
+ return 0;
+
+ /*
+ * When asked to do --local (do not include an object that appears in a
+ * pack we borrow from elsewhere) or --honor-pack-keep (do not include
+ * an object that appears in a pack marked with .keep), finding a pack
+ * that matches the criteria is sufficient for us to decide to omit it.
+ * However, even if this pack does not satisfy the criteria, we need to
+ * make sure no copy of this object appears in _any_ pack that makes us
+ * to omit the object, so we need to check all the packs.
+ *
+ * We can however first check whether these options can possible matter;
+ * if they do not matter we know we want the object in generated pack.
+ * Otherwise, we signal "-1" at the end to tell the caller that we do
+ * not know either way, and it needs to check more packs.
+ */
+ if (!ignore_packed_keep &&
+ (!local || !have_non_local_packs))
+ return 1;
+
+ if (local && !p->pack_local)
+ return 0;
+ if (ignore_packed_keep && p->pack_local && p->pack_keep)
+ return 0;
+
+ /* we don't know yet; keep looking for more packs */
+ return -1;
+}
+
/*
* Check whether we want the object in the pack (e.g., we do not want
* objects found in non-local stores if the "--local" option was used).
*
- * As a side effect of this check, we will find the packed version of this
- * object, if any. We therefore pass out the pack information to avoid having
- * to look it up again later.
+ * If the caller already knows an existing pack it wants to take the object
+ * from, that is passed in *found_pack and *found_offset; otherwise this
+ * function finds if there is any pack that has the object and returns the pack
+ * and its offset in these variables.
*/
static int want_object_in_pack(const unsigned char *sha1,
int exclude,
@@ -958,15 +993,30 @@ static int want_object_in_pack(const unsigned char *sha1,
off_t *found_offset)
{
struct packed_git *p;
+ int want;
if (!exclude && local && has_loose_object_nonlocal(sha1))
return 0;
- *found_pack = NULL;
- *found_offset = 0;
+ /*
+ * If we already know the pack object lives in, start checks from that
+ * pack - in the usual case when neither --local was given nor .keep files
+ * are present we will determine the answer right now.
+ */
+ if (*found_pack) {
+ want = want_found_object(exclude, *found_pack);
+ if (want != -1)
+ return want;
+ }
for (p = packed_git; p; p = p->next) {
- off_t offset = find_pack_entry_one(sha1, p);
+ off_t offset;
+
+ if (p == *found_pack)
+ offset = *found_offset;
+ else
+ offset = find_pack_entry_one(sha1, p);
+
if (offset) {
if (!*found_pack) {
if (!is_pack_valid(p))
@@ -974,31 +1024,9 @@ static int want_object_in_pack(const unsigned char *sha1,
*found_offset = offset;
*found_pack = p;
}
- if (exclude)
- return 1;
- if (incremental)
- return 0;
-
- /*
- * When asked to do --local (do not include an
- * object that appears in a pack we borrow
- * from elsewhere) or --honor-pack-keep (do not
- * include an object that appears in a pack marked
- * with .keep), we need to make sure no copy of this
- * object come from in _any_ pack that causes us to
- * omit it, and need to complete this loop. When
- * neither option is in effect, we know the object
- * we just found is going to be packed, so break
- * out of the loop to return 1 now.
- */
- if (!ignore_packed_keep &&
- (!local || !have_non_local_packs))
- break;
-
- if (local && !p->pack_local)
- return 0;
- if (ignore_packed_keep && p->pack_local && p->pack_keep)
- return 0;
+ want = want_found_object(exclude, p);
+ if (want != -1)
+ return want;
}
}
@@ -1039,8 +1067,8 @@ static const char no_closure_warning[] = N_(
static int add_object_entry(const unsigned char *sha1, enum object_type type,
const char *name, int exclude)
{
- struct packed_git *found_pack;
- off_t found_offset;
+ struct packed_git *found_pack = NULL;
+ off_t found_offset = 0;
uint32_t index_pos;
if (have_duplicate_entry(sha1, exclude, &index_pos))
@@ -1073,6 +1101,9 @@ static int add_object_entry_from_bitmap(const unsigned char *sha1,
if (have_duplicate_entry(sha1, 0, &index_pos))
return 0;
+ if (!want_object_in_pack(sha1, 0, &pack, &offset))
+ return 0;
+
create_object_entry(sha1, type, name_hash, 0, 0, index_pos, pack, offset);
display_progress(progress_state, nr_result);
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3893afd..a278d30 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -7,6 +7,18 @@ objpath () {
echo ".git/objects/$(echo "$1" | sed -e 's|\(..\)|\1/|')"
}
+# show objects present in pack ($1 should be associated *.idx)
+list_packed_objects () {
+ git show-index <"$1" | cut -d' ' -f2
+}
+
+# has_any pattern-file content-file
+# tests whether content-file has any entry from pattern-file with entries being
+# whole lines.
+has_any () {
+ grep -Ff "$1" "$2"
+}
+
test_expect_success 'setup repo with moderate-sized history' '
for i in $(test_seq 1 10); do
test_commit $i
@@ -16,6 +28,7 @@ test_expect_success 'setup repo with moderate-sized history' '
test_commit side-$i
done &&
git checkout master &&
+ bitmaptip=$(git rev-parse master) &&
blob=$(echo tagged-blob | git hash-object -w --stdin) &&
git tag tagged-blob $blob &&
git config repack.writebitmaps true &&
@@ -118,6 +131,71 @@ test_expect_success 'incremental repack can disable bitmaps' '
git repack -d --no-write-bitmap-index
'
+test_expect_success 'pack-objects respects --local (non-local loose)' '
+ git init --bare alt.git &&
+ echo $(pwd)/alt.git/objects >.git/objects/info/alternates &&
+ echo content1 >file1 &&
+ # non-local loose object which is not present in bitmapped pack
+ altblob=$(GIT_DIR=alt.git git hash-object -w file1) &&
+ # non-local loose object which is also present in bitmapped pack
+ git cat-file blob $blob | GIT_DIR=alt.git git hash-object -w --stdin &&
+ git add file1 &&
+ test_tick &&
+ git commit -m commit_file1 &&
+ echo HEAD | git pack-objects --local --stdout --revs >1.pack &&
+ git index-pack 1.pack &&
+ list_packed_objects 1.idx >1.objects &&
+ printf "%s\n" "$altblob" "$blob" >nonlocal-loose &&
+ ! has_any nonlocal-loose 1.objects
+'
+
+test_expect_success 'pack-objects respects --honor-pack-keep (local non-bitmapped pack)' '
+ echo content2 >file2 &&
+ blob2=$(git hash-object -w file2) &&
+ git add file2 &&
+ test_tick &&
+ git commit -m commit_file2 &&
+ printf "%s\n" "$blob2" "$bitmaptip" >keepobjects &&
+ pack2=$(git pack-objects pack2 <keepobjects) &&
+ mv pack2-$pack2.* .git/objects/pack/ &&
+ >.git/objects/pack/pack2-$pack2.keep &&
+ rm $(objpath $blob2) &&
+ echo HEAD | git pack-objects --honor-pack-keep --stdout --revs >2a.pack &&
+ git index-pack 2a.pack &&
+ list_packed_objects 2a.idx >2a.objects &&
+ ! has_any keepobjects 2a.objects
+'
+
+test_expect_success 'pack-objects respects --local (non-local pack)' '
+ mv .git/objects/pack/pack2-$pack2.* alt.git/objects/pack/ &&
+ echo HEAD | git pack-objects --local --stdout --revs >2b.pack &&
+ git index-pack 2b.pack &&
+ list_packed_objects 2b.idx >2b.objects &&
+ ! has_any keepobjects 2b.objects
+'
+
+test_expect_success 'pack-objects respects --honor-pack-keep (local bitmapped pack)' '
+ ls .git/objects/pack/ | grep bitmap >output &&
+ test_line_count = 1 output &&
+ packbitmap=$(basename $(cat output) .bitmap) &&
+ list_packed_objects .git/objects/pack/$packbitmap.idx >packbitmap.objects &&
+ test_when_finished "rm -f .git/objects/pack/$packbitmap.keep" &&
+ >.git/objects/pack/$packbitmap.keep &&
+ echo HEAD | git pack-objects --honor-pack-keep --stdout --revs >3a.pack &&
+ git index-pack 3a.pack &&
+ list_packed_objects 3a.idx >3a.objects &&
+ ! has_any packbitmap.objects 3a.objects
+'
+
+test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '
+ mv .git/objects/pack/$packbitmap.* alt.git/objects/pack/ &&
+ test_when_finished "mv alt.git/objects/pack/$packbitmap.* .git/objects/pack/" &&
+ echo HEAD | git pack-objects --local --stdout --revs >3b.pack &&
+ git index-pack 3b.pack &&
+ list_packed_objects 3b.idx >3b.objects &&
+ ! has_any packbitmap.objects 3b.objects
+'
+
test_expect_success 'full repack, reusing previous bitmaps' '
git repack -ad &&
ls .git/objects/pack/ | grep bitmap >output &&
@@ -143,6 +221,20 @@ test_expect_success 'create objects for missing-HAVE tests' '
EOF
'
+test_expect_success 'pack-objects respects --incremental' '
+ cat >revs2 <<-EOF &&
+ HEAD
+ $commit
+ EOF
+ git pack-objects --incremental --stdout --revs <revs2 >4.pack &&
+ git index-pack 4.pack &&
+ list_packed_objects 4.idx >4.objects &&
+ test_line_count = 4 4.objects &&
+ git rev-list --objects $commit >revlist &&
+ cut -d" " -f1 revlist |sort >objects &&
+ test_cmp 4.objects objects
+'
+
test_expect_success 'pack with missing blob' '
rm $(objpath $blob) &&
git pack-objects --stdout --revs <revs >/dev/null
--
2.9.2.701.gf965a18.dirty
^ permalink raw reply related
* [PATCH 2/2 v8] pack-objects: use reachability bitmap index when generating non-stdout pack
From: Kirill Smelkov @ 2016-09-10 15:01 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Vicent Marti, Jérome Perrin, Isabelle Vallet,
Kazuhiko Shiozaki, Julien Muchembled, git, Kirill Smelkov
In-Reply-To: <20160910145925.xbbus7eck5ineika@teco.navytux.spb.ru>
Starting from 6b8fda2d (pack-objects: use bitmaps when packing objects)
if a repository has bitmap index, pack-objects can nicely speedup
"Counting objects" graph traversal phase. That however was done only for
case when resultant pack is sent to stdout, not written into a file.
The reason here is for on-disk repack by default we want:
- to produce good pack (with bitmap index not-yet-packed objects are
emitted to pack in suboptimal order).
- to use more robust pack-generation codepath (avoiding possible
bugs in bitmap code and possible bitmap index corruption).
Jeff King further explains:
The reason for this split is that pack-objects tries to determine how
"careful" it should be based on whether we are packing to disk or to
stdout. Packing to disk implies "git repack", and that we will likely
delete the old packs after finishing. We want to be more careful (so
as not to carry forward a corruption, and to generate a more optimal
pack), and we presumably run less frequently and can afford extra CPU.
Whereas packing to stdout implies serving a remote via "git fetch" or
"git push". This happens more frequently (e.g., a server handling many
fetching clients), and we assume the receiving end takes more
responsibility for verifying the data.
But this isn't always the case. One might want to generate on-disk
packfiles for a specialized object transfer. Just using "--stdout" and
writing to a file is not optimal, as it will not generate the matching
pack index.
So it would be useful to have some way of overriding this heuristic:
to tell pack-objects that even though it should generate on-disk
files, it is still OK to use the reachability bitmaps to do the
traversal.
So we can teach pack-objects to use bitmap index for initial object
counting phase when generating resultant pack file too:
- if we take care to not let it be activated under git-repack:
See above about repack robustness and not forward-carrying corruption.
- if we know bitmap index generation is not enabled for resultant pack:
The current code has singleton bitmap_git, so it cannot work
simultaneously with two bitmap indices.
We also want to avoid (at least with current implementation)
generating bitmaps off of bitmaps. The reason here is: when generating
a pack, not-yet-packed objects will be emitted into pack in
suboptimal order and added to tail of the bitmap as "extended entries".
When the resultant pack + some new objects in associated repository
are in turn used to generate another pack with bitmap, the situation
repeats: new objects are again not emitted optimally and just added to
bitmap tail - not in recency order.
So the pack badness can grow over time when at each step we have
bitmapped pack + some other objects. That's why we want to avoid
generating bitmaps off of bitmaps, not to let pack badness grow.
- if we keep pack reuse enabled still only for "send-to-stdout" case:
Because pack-to-file needs to generate index for destination pack, and
currently on pack reuse raw entries are directly written out to the
destination pack by write_reused_pack(), bypassing needed for pack index
generation bookkeeping done by regular codepath in write_one() and
friends.
( In the future we might teach pack-reuse code about cases when index
also needs to be generated for resultant pack and remove
pack-reuse-only-for-stdout limitation )
This way for pack-objects -> file we get nice speedup:
erp5.git[1] (~230MB) extracted from ~ 5GB lab.nexedi.com backup
repository managed by git-backup[2] via
time echo 0186ac99 | git pack-objects --revs erp5pack
before: 37.2s
after: 26.2s
And for `git repack -adb` packed git.git
time echo 5c589a73 | git pack-objects --revs gitpack
before: 7.1s
after: 3.6s
i.e. it can be 30% - 50% speedup for pack extraction.
git-backup extracts many packs on repositories restoration. That was my
initial motivation for the patch.
[1] https://lab.nexedi.com/nexedi/erp5
[2] https://lab.nexedi.com/kirr/git-backup
NOTE
Jeff also suggests that pack.useBitmaps was probably a mistake to
introduce originally. This way we are not adding another config point,
but instead just always default to-file pack-objects not to use bitmap
index: Tools which need to generate on-disk packs with using bitmap, can
pass --use-bitmap-index explicitly. And git-repack does never pass
--use-bitmap-index, so this way we can be sure regular on-disk repacking
remains robust.
NOTE2
`git pack-objects --stdout >file.pack` + `git index-pack file.pack` is much slower
than `git pack-objects file.pack`. Extracting erp5.git pack from
lab.nexedi.com backup repository:
$ time echo 0186ac99 | git pack-objects --stdout --revs >erp5pack-stdout.pack
real 0m22.309s
user 0m21.148s
sys 0m0.932s
$ time git index-pack erp5pack-stdout.pack
real 0m50.873s <-- more than 2 times slower than time to generate pack itself!
user 0m49.300s
sys 0m1.360s
So the time for
`pack-object --stdout >file.pack` + `index-pack file.pack` is 72s,
while
`pack-objects file.pack` which does both pack and index is 27s.
And even
`pack-objects --no-use-bitmap-index file.pack` is 37s.
Jeff explains:
The packfile does not carry the sha1 of the objects. A receiving
index-pack has to compute them itself, including inflating and applying
all of the deltas.
that's why for `git-backup restore` we want to teach `git pack-objects
file.pack` to use bitmaps instead of using `git pack-objects --stdout
>file.pack` + `git index-pack file.pack`.
NOTE3
The speedup is now tracked via t/perf/p5310-pack-bitmaps.sh
Test 56dfeb62 this tree
--------------------------------------------------------------------------------
5310.2: repack to disk 8.98(8.05+0.29) 9.05(8.08+0.33) +0.8%
5310.3: simulated clone 2.02(2.27+0.09) 2.01(2.25+0.08) -0.5%
5310.4: simulated fetch 0.81(1.07+0.02) 0.81(1.05+0.04) +0.0%
5310.5: pack to file 7.58(7.04+0.28) 7.60(7.04+0.30) +0.3%
5310.6: pack to file (bitmap) 7.55(7.02+0.28) 3.25(2.82+0.18) -57.0%
5310.8: clone (partial bitmap) 1.83(2.26+0.12) 1.82(2.22+0.14) -0.5%
5310.9: pack to file (partial bitmap) 6.86(6.58+0.30) 2.87(2.74+0.20) -58.2%
More context:
http://marc.info/?t=146792101400001&r=1&w=2
http://public-inbox.org/git/20160707190917.20011-1-kirr@nexedi.com/T/#t
Cc: Vicent Marti <tanoku@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Kirill Smelkov <kirr@nexedi.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/pack-objects.c | 31 ++++++++++++++++++++++++-------
t/perf/p5310-pack-bitmaps.sh | 14 +++++++++++++-
t/t5310-pack-bitmaps.sh | 12 ++++++++++++
3 files changed, 49 insertions(+), 8 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 19668d3..d48c290 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -67,7 +67,8 @@ static struct packed_git *reuse_packfile;
static uint32_t reuse_packfile_objects;
static off_t reuse_packfile_offset;
-static int use_bitmap_index = 1;
+static int use_bitmap_index_default = 1;
+static int use_bitmap_index = -1;
static int write_bitmap_index;
static uint16_t write_bitmap_options;
@@ -2274,7 +2275,7 @@ static int git_pack_config(const char *k, const char *v, void *cb)
write_bitmap_options &= ~BITMAP_OPT_HASH_CACHE;
}
if (!strcmp(k, "pack.usebitmaps")) {
- use_bitmap_index = git_config_bool(k, v);
+ use_bitmap_index_default = git_config_bool(k, v);
return 0;
}
if (!strcmp(k, "pack.threads")) {
@@ -2523,13 +2524,13 @@ static void loosen_unused_packed_objects(struct rev_info *revs)
}
/*
- * This tracks any options which a reader of the pack might
- * not understand, and which would therefore prevent blind reuse
- * of what we have on disk.
+ * This tracks any options which pack-reuse code expects to be on, or which a
+ * reader of the pack might not understand, and which would therefore prevent
+ * blind reuse of what we have on disk.
*/
static int pack_options_allow_reuse(void)
{
- return allow_ofs_delta;
+ return pack_to_stdout && allow_ofs_delta;
}
static int get_object_list_from_bitmap(struct rev_info *revs)
@@ -2822,7 +2823,23 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
if (!rev_list_all || !rev_list_reflog || !rev_list_index)
unpack_unreachable_expiration = 0;
- if (!use_internal_rev_list || !pack_to_stdout || is_repository_shallow())
+ /*
+ * "soft" reasons not to use bitmaps - for on-disk repack by default we want
+ *
+ * - to produce good pack (with bitmap index not-yet-packed objects are
+ * packed in suboptimal order).
+ *
+ * - to use more robust pack-generation codepath (avoiding possible
+ * bugs in bitmap code and possible bitmap index corruption).
+ */
+ if (!pack_to_stdout)
+ use_bitmap_index_default = 0;
+
+ if (use_bitmap_index < 0)
+ use_bitmap_index = use_bitmap_index_default;
+
+ /* "hard" reasons not to use bitmaps; these just won't work at all */
+ if (!use_internal_rev_list || (!pack_to_stdout && write_bitmap_index) || is_repository_shallow())
use_bitmap_index = 0;
if (pack_to_stdout || !rev_list_all)
diff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh
index de2a224..bb91dbb 100755
--- a/t/perf/p5310-pack-bitmaps.sh
+++ b/t/perf/p5310-pack-bitmaps.sh
@@ -32,6 +32,14 @@ test_perf 'simulated fetch' '
} | git pack-objects --revs --stdout >/dev/null
'
+test_perf 'pack to file' '
+ git pack-objects --all pack1 </dev/null >/dev/null
+'
+
+test_perf 'pack to file (bitmap)' '
+ git pack-objects --use-bitmap-index --all pack1b </dev/null >/dev/null
+'
+
test_expect_success 'create partial bitmap state' '
# pick a commit to represent the repo tip in the past
cutoff=$(git rev-list HEAD~100 -1) &&
@@ -53,8 +61,12 @@ test_expect_success 'create partial bitmap state' '
git update-ref HEAD $orig_tip
'
-test_perf 'partial bitmap' '
+test_perf 'clone (partial bitmap)' '
git pack-objects --stdout --all </dev/null >/dev/null
'
+test_perf 'pack to file (partial bitmap)' '
+ git pack-objects --use-bitmap-index --all pack2b </dev/null >/dev/null
+'
+
test_done
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index a278d30..9602e9a 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -196,6 +196,18 @@ test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '
! has_any packbitmap.objects 3b.objects
'
+test_expect_success 'pack-objects to file can use bitmap' '
+ # make sure we still have 1 bitmap index from previous tests
+ ls .git/objects/pack/ | grep bitmap >output &&
+ test_line_count = 1 output &&
+ # verify equivalent packs are generated with/without using bitmap index
+ packasha1=$(git pack-objects --no-use-bitmap-index --all packa </dev/null) &&
+ packbsha1=$(git pack-objects --use-bitmap-index --all packb </dev/null) &&
+ list_packed_objects <packa-$packasha1.idx >packa.objects &&
+ list_packed_objects <packb-$packbsha1.idx >packb.objects &&
+ test_cmp packa.objects packb.objects
+'
+
test_expect_success 'full repack, reusing previous bitmaps' '
git repack -ad &&
ls .git/objects/pack/ | grep bitmap >output &&
--
2.9.2.701.gf965a18.dirty
^ permalink raw reply related
* [PATCH] t/perf/run: Don't forget to copy config.mak.autogen & friends to build area
From: Kirill Smelkov @ 2016-09-10 15:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Vicent Marti, Jérome Perrin, Isabelle Vallet,
Kazuhiko Shiozaki, Julien Muchembled, git, Kirill Smelkov
In-Reply-To: <20160910145738.x6ihp2gqzpk7dbi3@teco.navytux.spb.ru>
Otherwise for people who use autotools-based configure in main worktree,
the performance testing results will be inconsistent as work and build
trees could be using e.g. different optimization levels.
See e.g.
http://public-inbox.org/git/20160818175222.bmm3ivjheokf2qzl@sigill.intra.peff.net/
for example.
NOTE config.status has to be copied because otherwise without it the build
would want to run reconfigure this way loosing just copied config.mak.autogen.
Signed-off-by: Kirill Smelkov <kirr@nexedi.com>
---
( Resending as separate patch-mail, just in case )
t/perf/run | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/perf/run b/t/perf/run
index cfd7012..aa383c2 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -30,7 +30,7 @@ unpack_git_rev () {
}
build_git_rev () {
rev=$1
- cp ../../config.mak build/$rev/config.mak
+ cp -t build/$rev ../../{config.mak,config.mak.autogen,config.status}
(cd build/$rev && make $GIT_PERF_MAKE_OPTS) ||
die "failed to build revision '$mydir'"
}
--
2.9.2.701.gf965a18.dirty
^ 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