* Re: [PATCH] Put sha1dc on a diet
From: Johannes Schindelin @ 2017-03-01 22:51 UTC (permalink / raw)
To: Linus Torvalds
Cc: Junio C Hamano, Jeff King, Marc Stevens, Dan Shumow,
Git Mailing List
In-Reply-To: <CA+55aFys5oQ0RySQ+Xv0ZDussr-xZNh4_b3+Upx_d9VPWmpM8Q@mail.gmail.com>
Hi,
On Wed, 1 Mar 2017, Linus Torvalds wrote:
> On Wed, Mar 1, 2017 at 1:56 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Footnote *1*: I know, it is easy to forget that some developers cannot
> > choose their tools, or even their hardware. In the past, we seemed to take
> > appropriate care, though.
>
> I don't think you need to worry about the Windows side.
I am not. I build G?t for Windows using GCC.
My concern is about that unexpected turn "oh, let's just switch to C99
because, well, because my compiler canehandle it, and everybody else
should just switch tn a modern compiler". That really sounded careless.
> That can continue to do something else.
>
> When I advocated perhaps using USE_SHA1DC by default, I definitely did
> not mean it in a "everywhere, regardless of issues" manner.
>
> For example, the conmfig.mak.uname script already explicitly asks for
> "BLK_SHA1 = YesPlease" for Windows. Don't bother changing that, it's an
> explicit choice.
That setting is only in git.git's version, not in gxt-for-windows/git.git.
We switched to OpenSSL because of speed improvements, in particular with
recent Intel processors.
> But the Linux rules don't actually specify which SHA1 version to use,
> so the main Makefile currently defaults to just using openssl.
>
> So that's the "default" choice I think we might want to change. Not
> the "we're windows, and explicitly want BLK_SHA1 because of
> environment and build infrastructure".
Since we switched away from BLOCK_SHA1, any such change would affect Git
for Windews.
But I think bigger than just developers on Windows OS. There are many
developers out there working on large repositories (yes, much larger than
Linux). Also using Macs and Linux. I am not at all sure that we want to
give them an updated Git they cannot fail to notice to be much slower than
before.
Don't get me wrong: I *hope* that you'll manage to get sha1dc
competitively fast. If you don't, well, then we simply cannot use it by
default for *all* of our calls (you already pointed out that the pack
index' checksum does not need collision detection, and in fact, *any*
operation that works on implicitly trusted data falls into the same court,
e.g. `git add`).
Ciao,
Johannes
^ permalink raw reply
* Re: What's cooking in git.git (Mar 2017, #01; Wed, 1)
From: René Scharfe @ 2017-03-01 22:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq60jsfww5.fsf@gitster.mtv.corp.google.com>
Am 01.03.2017 um 23:35 schrieb Junio C Hamano:
> * rs/log-email-subject (2017-03-01) 2 commits
> - pretty: use fmt_output_email_subject()
> - log-tree: factor out fmt_output_email_subject()
>
> Code clean-up.
>
> Will merge to 'next'.
Could you please squash this in? We only use a single context (as
opposed to an array), so it doesn't have to be especially compact,
and using a bitfield slows down half of the tests in p4000 by 3%
for me.
---
commit.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/commit.h b/commit.h
index 459daef94a..528272ac9b 100644
--- a/commit.h
+++ b/commit.h
@@ -154,7 +154,7 @@ struct pretty_print_context {
int preserve_subject;
struct date_mode date_mode;
unsigned date_mode_explicit:1;
- unsigned print_email_subject:1;
+ int print_email_subject;
int expand_tabs_in_log;
int need_8bit_cte;
char *notes_message;
--
2.12.0
^ permalink raw reply related
* Re: [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1
From: Junio C Hamano @ 2017-03-01 23:05 UTC (permalink / raw)
To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <xmqqpoi0eho8.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>
>> The callchain for handling each argument contains the function
>> revision.c:get_sha1 where the shorthand for "-" ~ "@{-1}" has already been
>> implemented in a previous patch; the complete callchain leading to that
>> function is:
>>
>> 1. merge.c:collect_parents
>> 2. commit.c:get_merge_parent : this function calls revision.c:get_sha1
>>
>> This patch also adds a test for checking that the shorthand works properly
>
> This breaks "git merge".
>
>> +test_expect_success 'merge - should work' '
>> + git checkout testing-2 &&
>> + git merge - &&
>> + git rev-parse HEAD HEAD^^ | sort >actual &&
>> + git rev-parse master testing-1 | sort >expect &&
>> + test_cmp expect actual
>
> This test is not sufficient to catch a regression I seem to be
> seeing.
>
> $ git checkout side
> $ git checkout pu
> $ git merge -
>
> used to say "Merge branch 'side' into pu". With this series merged,
> I seem to be getting "Merge commit '-' into pu".
You stopped at get_sha1_1() in your 3817cebabc ("sha1_name.c: teach
get_sha1_1 "-" shorthand for "@{-1}"", 2017-02-25), instead of going
down to get_sha1_basic() and teaching it that "-" means the same
thing as "@{-1}", which would in turn require you to teach
dwim_ref() that "-" is the same thing as "@{-1}". As dwim_ref()
does not know about "-" and does not expand it to the refname like
it expands "@{-1}", it would break and that is why 3817cebabc punts
at a bit higher in the callchain.
The breakage by this patch to "git merge" happens for the same
reason. cmd_merge() calls collect_parents() which annotates the
commits that are merged with their textual name, which used to be
"@{-1}" without this patch but now "-" is passed as-is. This
annotation will be given to merge_name(), and the first thing it
does is dwim_ref(). The function knows what to do with "@{-1}",
but it does not know what to do with "-", and that is why you end up
producing "Merge commit '-' into ...".
Dropping this patch from the series would make things consistent
with what was done in 3817cebabc and I think that is a sensible
place to stop. After the dust settles, We _can_ later dig further
by teaching dwim_ref() and friends what "-" means, and when it is
done, this patch would become useful.
Thanks.
^ permalink raw reply
* [PATCH v2 0/2] Minor changes to skip gitweb tests without Time::HiRes
From: Ævar Arnfjörð Bjarmason @ 2017-03-01 21:15 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jakub Narębski,
Ævar Arnfjörð Bjarmason
In-Reply-To: <4b34e3a0-3da7-d821-2a7f-9a420ac1d3f6@gmail.com>
This was originally just one small patch, but Jakub Narębski pointed
out that calling the module "unusable" when we failed to load it was
confusing, so now the start of this series is just a rephrasing of an
existing error message I copied.
Ævar Arnfjörð Bjarmason (2):
gitweb tests: Change confusing "skip_all" phrasing
gitweb tests: Skip tests when we don't have Time::HiRes
t/gitweb-lib.sh | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
--
2.11.0
^ permalink raw reply
* Re: [PATCH] Put sha1dc on a diet
From: Linus Torvalds @ 2017-03-01 23:05 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, Jeff King, Marc Stevens, Dan Shumow,
Git Mailing List
In-Reply-To: <alpine.DEB.2.20.1703012334400.3767@virtualbox>
On Wed, Mar 1, 2017 at 2:51 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> But I think bigger than just developers on Windows OS. There are many
> developers out there working on large repositories (yes, much larger than
> Linux). Also using Macs and Linux. I am not at all sure that we want to
> give them an updated Git they cannot fail to notice to be much slower than
> before.
Johannes, have you *tried* the patches?
I really don't think you have. It is completely unnoticeable in any
normal situation. The two cases that it's noticeable is:
- a full fsck is noticeable slower
- a full non-local clone is slower (but not really noticeably so
since the network traffic dominates).
In other words, I think you're making shit up. I don't think you
understand how little the SHA1 performance actually matters. It's
noticeable in benchmarks. It's not noticeable in any normal operation.
.. and yes, I've actually been running the patches locally since I
posted my first version (which apparently didn't go out to the list
because of list size limits) and now running the version in 'pu'.
Linus
^ permalink raw reply
* Re: What's cooking in git.git (Mar 2017, #01; Wed, 1)
From: Junio C Hamano @ 2017-03-01 23:08 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <52f043cc-7b39-5ab7-bcdc-894aeb402c12@web.de>
René Scharfe <l.s.r@web.de> writes:
> Am 01.03.2017 um 23:35 schrieb Junio C Hamano:
>> * rs/log-email-subject (2017-03-01) 2 commits
>> - pretty: use fmt_output_email_subject()
>> - log-tree: factor out fmt_output_email_subject()
>>
>> Code clean-up.
>>
>> Will merge to 'next'.
>
> Could you please squash this in? We only use a single context (as
> opposed to an array), so it doesn't have to be especially compact,
> and using a bitfield slows down half of the tests in p4000 by 3%
> for me.
I thought I saw the keyword "bitfield" to the solution for that 3%
somewhere in the thread and forgot when I updated the "What's
cooking" report.
Will do. Thanks for being careful.
>
> ---
> commit.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/commit.h b/commit.h
> index 459daef94a..528272ac9b 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -154,7 +154,7 @@ struct pretty_print_context {
> int preserve_subject;
> struct date_mode date_mode;
> unsigned date_mode_explicit:1;
> - unsigned print_email_subject:1;
> + int print_email_subject;
> int expand_tabs_in_log;
> int need_8bit_cte;
> char *notes_message;
^ permalink raw reply
* Re: [PATCH] Put sha1dc on a diet
From: Jeff King @ 2017-03-01 23:13 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Dan Shumow, Git Mailing List, Junio C Hamano, Marc Stevens
In-Reply-To: <CA+55aFwr1jncrk-cekn0Y8rs_S+zs7RrgQ-Jb-ZbgCvmVrHT_A@mail.gmail.com>
On Wed, Mar 01, 2017 at 12:58:39PM -0800, Linus Torvalds wrote:
>> I don't think that helps. The sha1 over the pack-file takes about 1.3s
>> with openssl, and 5s with sha1dc. So we already know the increase there
>> is only a few seconds, not a few minutes.
>
> OK. I guess what w could easily do is to just add an argument to
> git_SHA1_Init() to say whether we want checking or not, and only use the
> checking functions when receiving objects (which would include creating new
> objects from files, and obviously deck).
>
> You'd still eat the cost on the receiving side of a clone, but that's when
> you really want the checking anyway. At least it wouldn't be so visible on
> the sending side, which is all the hosting etc, where there might be server
> utilization issues.
>
> Would that make deployment happier? It should be an easy little flag to
> add, I think.
I don't think it makes all that big a difference. The sending side
wastes the extra 2-3 seconds of CPU to checksum the whole packfile, but
it's not inflating all the object contents in the first place (between
reachability bitmaps to get the list of objects in the first place, and
then verbatim reuse of pack contents).
Which isn't to say it's not reasonable to limit the checking to a few
spots (basically anything that's _writing_ objects). But I don't think
it makes a big difference to the server side of a fetch or clone.
-Peff
^ permalink raw reply
* [PATCH] README: create HTTP/HTTPS links from URLs in Markdown
From: Eric Wong @ 2017-03-01 22:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Markdown supports automatic links by surrounding URLs with
angle brackets, as documented in
<https://daringfireball.net/projects/markdown/syntax#autolink>
While we're at it, update URLs to avoid redirecting clients for
git-scm.com (by using HTTPS) and public-inbox.org (by adding a
trailing slash).
Signed-off-by: Eric Wong <e@80x24.org>
---
I was going to cite some style manuals (MLA, APA, etc),
but it seems current versions do not favor angle brackets.
However, this remains consistent with the recommendations in
RFC 2369 <https://ietf.org/rfc/rfc2369.txt> for mail headers.
README.md | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/README.md b/README.md
index c0cd5580e..f17af66a9 100644
--- a/README.md
+++ b/README.md
@@ -12,7 +12,7 @@ Torvalds with help of a group of hackers around the net.
Please read the file [INSTALL][] for installation instructions.
-Many Git online resources are accessible from http://git-scm.com/
+Many Git online resources are accessible from <https://git-scm.com/>
including full documentation and Git related tools.
See [Documentation/gittutorial.txt][] to get started, then see
@@ -33,8 +33,8 @@ requests, comments and patches to git@vger.kernel.org (read
[Documentation/SubmittingPatches][] for instructions on patch submission).
To subscribe to the list, send an email with just "subscribe git" in
the body to majordomo@vger.kernel.org. The mailing list archives are
-available at https://public-inbox.org/git,
-http://marc.info/?l=git and other archival sites.
+available at <https://public-inbox.org/git/>,
+<http://marc.info/?l=git> and other archival sites.
The maintainer frequently sends the "What's cooking" reports that
list the current status of various development topics to the mailing
--
EW
^ permalink raw reply related
* Re: [PATCH] Put sha1dc on a diet
From: Jeff King @ 2017-03-01 23:19 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Schindelin, Junio C Hamano, Marc Stevens, Dan Shumow,
Git Mailing List
In-Reply-To: <CA+55aFy9=jBJT36FC2HiAeabJBssY=jE=zLxwrXWzhpiFkMUXg@mail.gmail.com>
On Wed, Mar 01, 2017 at 03:05:25PM -0800, Linus Torvalds wrote:
> On Wed, Mar 1, 2017 at 2:51 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > But I think bigger than just developers on Windows OS. There are many
> > developers out there working on large repositories (yes, much larger than
> > Linux). Also using Macs and Linux. I am not at all sure that we want to
> > give them an updated Git they cannot fail to notice to be much slower than
> > before.
>
> Johannes, have you *tried* the patches?
>
> I really don't think you have. It is completely unnoticeable in any
> normal situation. The two cases that it's noticeable is:
>
> - a full fsck is noticeable slower
>
> - a full non-local clone is slower (but not really noticeably so
> since the network traffic dominates).
>
> In other words, I think you're making shit up. I don't think you
> understand how little the SHA1 performance actually matters. It's
> noticeable in benchmarks. It's not noticeable in any normal operation.
>
> .. and yes, I've actually been running the patches locally since I
> posted my first version (which apparently didn't go out to the list
> because of list size limits) and now running the version in 'pu'.
You have to remember that some of the Git for Windows users are doing
horrific things like using repositories with 450MB .git/index files, and
the speed to compute the sha1 during an update is noticeable there.
IMHO that is a good sign that the right approach is to switch to an
index format that doesn't require rewriting all 450MB to update one
entry. But obviously that is a much harder problem than just using an
optimized sha1 implementation.
I do think that could argue for turning on the collision detection only
during object-write operations, which is where it matters. It would be
really trivial to flip the "check collisions" bit on sha1dc. But I
suspect you could go faster still by compiling against two separate
implementations: the fast-as-possible one (which could be openssl or
blk-sha1), and the slower-but-careful sha1dc.
-Peff
^ permalink raw reply
* Re: [PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions
From: Junio C Hamano @ 2017-03-01 23:18 UTC (permalink / raw)
To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <1488007487-12965-7-git-send-email-kannan.siddharth12@gmail.com>
Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> revert.c:run_sequencer calls setup_revisions right after replacing "-" with
> "@{-1}" for this shorthand. A previous patch taught setup_revisions to handle
> this shorthand by doing the required replacement inside revision.c:get_sha1_1.
>
> Hence, the code here is redundant and has been removed.
>
> This patch also adds a test to check that revert recognizes the "-" shorthand.
Unlike "merge" [*1*], I think this one is OK because "git revert
$commit" does not try to say _how_ the commit was given, and most
importantly, it does not say what branch the reverted thing was.
Thanks.
[Footnote]
*1* Probably "checkout" would exhibit the same issue as we saw in
5/6 for "git merge" if you remove the "- to @{-1}" conversion
from it.
^ permalink raw reply
* Re: [PATCH v1 1/1] git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Junio C Hamano @ 2017-03-01 23:29 UTC (permalink / raw)
To: Mike Crowe; +Cc: tboegi, git
In-Reply-To: <20170301212535.GA6878@mcrowe.com>
Mike Crowe <mac@mcrowe.com> writes:
> With the above patch, both "git diff" and "git diff --quiet" report that
> there are no changes. Previously Git would report the extra newline
> correctly.
I sent an updated one that (I think) fixes the real issue, which the
extra would_convert_to_git() calls added in the older iteration to
diff_filespec_check_stat_unmatch() were merely papering over.
It would be nice to see if it fixes the issue for you.
Thanks.
^ permalink raw reply
* Re: [PATCH] Put sha1dc on a diet
From: Linus Torvalds @ 2017-03-01 23:38 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Git Mailing List, Dan Shumow, Marc Stevens
In-Reply-To: <20170301203427.e5xa5ej3czli7c3o@sigill.intra.peff.net>
On Wed, Mar 1, 2017 at 12:34 PM, Jeff King <peff@peff.net> wrote:
>
> I don't think that helps. The sha1 over the pack-file takes about 1.3s
> with openssl, and 5s with sha1dc. So we already know the increase there
> is only a few seconds, not a few minutes.
Yeah, I did a few statistics by adding just logging of "SHA1_Init()"
calls. For that network clone situation, the call distribution is
1 SHA1: Init at builtin/index-pack.c:326
841228 SHA1: Init at builtin/index-pack.c:450
2 SHA1: Init at csum-file.c:152
4415756 SHA1: Init at sha1_file.c:3218
(the line numbers are a bit off from 'pu', because I obviously have
the logging code).
The big number (one for every object) is from
write_sha1_file_prepare(), which we'd want to be the strong collision
checking version because those are things we're about to create git
objects out of. It's called from
- hash_sha1_file() - doesn't actually write the object, but is used
to calculate the sha for incoming data after applying the delta, for
example.
- write_sha1_file() - many uses, actually writes the object
- hash_sha1_file_literally() - git hash-object
and that index-pack.c:450 is from unpack_entry_data() for the base
non-delta objects (which should also be the strong kind).
So all of them should check against collision attacks, so none of them
seem to be things you'd want to optimize away..
So I was wrong in thinking that there were a lot of unnecessary SHA1
calculations in that load. They all look like they should be done with
the slower checking code.
Oh well.
Linus
^ permalink raw reply
* Re: Delta compression not so effective
From: Marius Storm-Olsen @ 2017-03-01 23:59 UTC (permalink / raw)
To: Martin Langhoff; +Cc: Git Mailing List
In-Reply-To: <CACPiFC+=ZpHT=xh7Y8f68BcXxNYx8EFJfzqqG2ub4NL=uREu7g@mail.gmail.com>
On 3/1/2017 14:19, Martin Langhoff wrote:
> On Wed, Mar 1, 2017 at 8:51 AM, Marius Storm-Olsen <mstormo@gmail.com> wrote:
>> BUT, even still, I would expect Git's delta compression to be quite effective, compared to the compression present in SVN.
>
> jar files are zipfiles. They don't delta in any useful form, and in
> fact they differ even if they contain identical binary files inside.
If you look through the initial post, you'll see that the jar in
question is in fact a tool (BFG) by Roberto Tyley, which is basically
git filter-branch on steroids. I used it to quickly filter out the
extern/ folder, just to prove most of the original size stems from that
particular folder. That's all.
The repo does not contain zip or jar files. A few images and other
compressed formats (except a few 100MBs of proprietary files, which
never change), but nothing unusual.
>> Commits: 32988
>> DB (server) size: 139GB
>
> Are you certain of the on-disk storage at the SVN server? Ideally,
> you've taken the size with a low-level tool like `du -sh
> /path/to/SVNRoot`.
139GB is from 'du -sh' on the SVN server. I imported (via SubGit)
directly from the (hotcopied) SVN folder on the server. So true SVN size.
> Even with no delta compression (as per Junio and Linus' discussion),
> based on past experience importing jar/wars/binaries from SVN into
> git... I'd expect git's worst case to be on-par with SVN, perhaps ~5%
> larger due to compression headers on uncompressible data.
Yes, I was expecting a Git repo <139GB, but like Linus mentioned,
something must be knocking the delta search off its feet, so it bails
out. Loose object -> 'hard' repack didn't show that much difference.
Thanks!
--
.marius
^ permalink raw reply
* Re: [PATCH 2/2] pretty: use fmt_output_email_subject()
From: René Scharfe @ 2017-03-01 15:41 UTC (permalink / raw)
To: Jeff King, Adrian Dudau; +Cc: git@vger.kernel.org
In-Reply-To: <58e05599-5dc4-9881-d8c0-89ad1f2e3838@web.de>
Am 01.03.2017 um 12:37 schrieb René Scharfe:
> Add the email-style subject prefix (e.g. "Subject: [PATCH] ") directly
> when it's needed instead of letting log_write_email_headers() prepare
> it in a static buffer in advance. This simplifies storage ownership and
> code flow.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> This slows down the last three tests in p4000 by ca. 3% for some reason,
> so we may want to only do the first part for now, which is performance
> neutral on my machine.
Update below.
> diff --git a/commit.h b/commit.h
> index 9c12abb911..459daef94a 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -142,21 +142,24 @@ static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
> return (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);
> }
>
> +struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
> +
> struct pretty_print_context {
> /*
> * Callers should tweak these to change the behavior of pp_* functions.
> */
> enum cmit_fmt fmt;
> int abbrev;
> - const char *subject;
> const char *after_subject;
> int preserve_subject;
> struct date_mode date_mode;
> unsigned date_mode_explicit:1;
> + unsigned print_email_subject:1;
Turning this into an int restores performance according to p4000.
Didn't know that bitfields can be *that* expensive.
René
^ permalink raw reply
* Re: Delta compression not so effective
From: Marius Storm-Olsen @ 2017-03-02 0:12 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List
In-Reply-To: <CA+55aFx7QFqrHw4e72vOdM5z0rw1CCkL2-UX8ej5CLSBWjLNLA@mail.gmail.com>
On 3/1/2017 12:30, Linus Torvalds wrote:
> On Wed, Mar 1, 2017 at 9:57 AM, Marius Storm-Olsen <mstormo@gmail.com> wrote:
>>
>> Indeed, I did do a
>> -c pack.threads=20 --window-memory=6g
>> to 'git repack', since the machine is a 20-core (40 threads) machine with
>> 126GB of RAM.
>>
>> So I guess with these sized objects, even at 6GB per thread, it's not enough
>> to get a big enough Window for proper delta-packing?
>
> Hmm. The 6GB window should be plenty good enough, unless your blobs
> are in the gigabyte range too.
No, the list of git verify-objects in the previous post was from the
bottom of the sorted list, so those are the largest blobs, ~249MB..
>> This repo took >14hr to repack on 20 threads though ("compression" step was
>> very fast, but stuck 95% of the time in "writing objects"), so I can only
>> imagine how long a pack.threads=1 will take :)
>
> Actually, it's usually the compression phase that should be slow - but
> if something is limiting finding deltas (so that we abort early), then
> that would certainly tend to speed up compression.
>
> The "writing objects" phase should be mainly about the actual IO.
> Which should be much faster *if* you actually find deltas.
So, this repo must be knocking several parts of Git's insides. I was
curious about why it was so slow on the writing objects part, since the
whole repo is on a 4x RAID 5, 7k spindels. Now, they are not SSDs sure,
but the thing has ~400MB/s continuous throughput available.
iostat -m 5 showed trickle read/write to the process, and 80-100% CPU
single thread (since the "write objects" stage is single threaded,
obviously).
The failing delta must be triggering other negative behavior.
> For example, the sorting code thinks that objects with the same name
> across the history are good sources of deltas. But it may be that for
> your case, the binary blobs that you have don't tend to actually
> change in the history, so that heuristic doesn't end up doing
> anything.
These are generally just DLLs (debug & release), which content is
updated due to upstream project updates. So, filenames/paths tend to
stay identical, while content changes throughout history.
> The sorting does use the size and the type too, but the "filename
> hash" (which isn't really a hash, it's something nasty to give
> reasonable results for the case where files get renamed) is the main
> sort key.
>
> So you might well want to look at the sorting code too. If filenames
> (particularly the end of filenames) for the blobs aren't good hints
> for the sorting code, that sort might end up spreading all the blobs
> out rather than sort them by size.
Filenames are fairly static, and the bulk of the 6000 biggest
non-delta'ed blobs are the same DLLs (multiple of them)
> And again, if that happens, the "can I delta these two objects" code
> will notice that the size of the objects are wildly different and
> won't even bother trying. Which speeds up the "compressing" phase, of
> course, but then because you don't get any good deltas, the "writing
> out" phase sucks donkey balls because it does zlib compression on big
> objects and writes them out to disk.
Right, now on this machine, I really didn't notice much difference
between standard zlib level and doing -9. The 203GB version was actually
with zlib=9.
> So there are certainly multiple possible reasons for the deltification
> to not work well for you.
>
> Hos sensitive is your material? Could you make a smaller repo with
> some of the blobs that still show the symptoms? I don't think I want
> to download 206GB of data even if my internet access is good.
Pretty sensitive, and not sure how I can reproduce this reasonable well.
However, I can easily recompile git with any recommended
instrumentation/printfs, if you have any suggestions of good places to
start? If anyone have good file/line numbers, I'll give that a go, and
report back?
Thanks!
--
.marius
^ permalink raw reply
* [PATCH 09/18] update submodules: add a config option to determine if submodules are updated
From: Stefan Beller @ 2017-03-02 0:47 UTC (permalink / raw)
To: sbeller; +Cc: git, bmwill, novalis, sandals, hvoigt, gitster, jrnieder, ramsay
In-Reply-To: <20170302004759.27852-1-sbeller@google.com>
In later patches we introduce the options and flag for commands
that modify the working directory, e.g. git-checkout.
Have a central place to store such settings whether we want to update
a submodule.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 6 ++++++
submodule.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/submodule.c b/submodule.c
index 04d185738f..591f4a694e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -17,6 +17,7 @@
#include "worktree.h"
static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
+static int config_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
static int parallel_jobs = 1;
static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;
static int initialized_fetch_ref_tips;
@@ -542,6 +543,11 @@ void set_config_fetch_recurse_submodules(int value)
config_fetch_recurse_submodules = value;
}
+void set_config_update_recurse_submodules(int value)
+{
+ config_update_recurse_submodules = value;
+}
+
static int has_remote(const char *refname, const struct object_id *oid,
int flags, void *cb_data)
{
diff --git a/submodule.h b/submodule.h
index 0b915bd3ac..b4e60c08d2 100644
--- a/submodule.h
+++ b/submodule.h
@@ -64,6 +64,7 @@ extern void show_submodule_inline_diff(FILE *f, const char *path,
const char *del, const char *add, const char *reset,
const struct diff_options *opt);
extern void set_config_fetch_recurse_submodules(int value);
+extern void set_config_update_recurse_submodules(int value);
extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
extern int fetch_populated_submodules(const struct argv_array *options,
const char *prefix, int command_line_option,
--
2.12.0.rc1.52.ge239d7e709.dirty
^ permalink raw reply related
* [PATCH 02/18] lib-submodule-update.sh: do not use ./. as submodule remote
From: Stefan Beller @ 2017-03-02 0:47 UTC (permalink / raw)
To: sbeller; +Cc: git, bmwill, novalis, sandals, hvoigt, gitster, jrnieder, ramsay
In-Reply-To: <20170302004759.27852-1-sbeller@google.com>
Adding the repository itself as a submodule does not make sense in the
real world. In our test suite we used to do that out of convenience in
some tests as the current repository has easiest access for setting up
'just a submodule'.
However this doesn't quite test the real world, so let's do not follow
this pattern any further and actually create an independent repository
that we can use as a submodule.
When using './.' as the remote the superproject and submodule share the
same objects, such that testing if a given sha1 is a valid commit works
in either repository. As running commands in an unpopulated submodule
fall back to the superproject, this happens in `reset_work_tree_to`
to determine if we need to populate the submodule. Fix this bug by
checking in the actual remote now.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
t/lib-submodule-update.sh | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 5df528ea81..c0d6325133 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -37,6 +37,17 @@
#
create_lib_submodule_repo () {
+ git init submodule_update_sub1 &&
+ (
+ cd submodule_update_sub1 &&
+ echo "expect" >>.gitignore &&
+ echo "actual" >>.gitignore &&
+ echo "x" >file1 &&
+ echo "y" >file2 &&
+ git add .gitignore file1 file2 &&
+ git commit -m "Base inside first submodule" &&
+ git branch "no_submodule"
+ ) &&
git init submodule_update_repo &&
(
cd submodule_update_repo &&
@@ -49,7 +60,7 @@ create_lib_submodule_repo () {
git branch "no_submodule" &&
git checkout -b "add_sub1" &&
- git submodule add ./. sub1 &&
+ git submodule add ../submodule_update_sub1 sub1 &&
git config -f .gitmodules submodule.sub1.ignore all &&
git config submodule.sub1.ignore all &&
git add .gitmodules &&
@@ -162,7 +173,7 @@ reset_work_tree_to () {
test_must_be_empty actual &&
sha1=$(git rev-parse --revs-only HEAD:sub1) &&
if test -n "$sha1" &&
- test $(cd "sub1" && git rev-parse --verify "$sha1^{commit}")
+ test $(cd "../submodule_update_sub1" && git rev-parse --verify "$sha1^{commit}")
then
git submodule update --init --recursive "sub1"
fi
--
2.12.0.rc1.52.ge239d7e709.dirty
^ permalink raw reply related
* [PATCH 10/18] submodules: introduce check to see whether to touch a submodule
From: Stefan Beller @ 2017-03-02 0:47 UTC (permalink / raw)
To: sbeller; +Cc: git, bmwill, novalis, sandals, hvoigt, gitster, jrnieder, ramsay
In-Reply-To: <20170302004759.27852-1-sbeller@google.com>
In later patches we introduce the --recurse-submodule flag for commands
that modify the working directory, e.g. git-checkout.
It is potentially expensive to check if a submodule needs an update,
because a common theme to interact with submodules is to spawn a child
process for each interaction.
So let's introduce a function that checks if a submodule needs
to be checked for an update before attempting the update.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 16 ++++++++++++++++
submodule.h | 7 +++++++
2 files changed, 23 insertions(+)
diff --git a/submodule.c b/submodule.c
index 591f4a694e..8b2c0212be 100644
--- a/submodule.c
+++ b/submodule.c
@@ -548,6 +548,22 @@ void set_config_update_recurse_submodules(int value)
config_update_recurse_submodules = value;
}
+int should_update_submodules(void)
+{
+ return config_update_recurse_submodules == RECURSE_SUBMODULES_ON;
+}
+
+const struct submodule *submodule_from_ce(const struct cache_entry *ce)
+{
+ if (!S_ISGITLINK(ce->ce_mode))
+ return NULL;
+
+ if (!should_update_submodules())
+ return NULL;
+
+ return submodule_from_path(null_sha1, ce->name);
+}
+
static int has_remote(const char *refname, const struct object_id *oid,
int flags, void *cb_data)
{
diff --git a/submodule.h b/submodule.h
index b4e60c08d2..6f3fe85c7c 100644
--- a/submodule.h
+++ b/submodule.h
@@ -65,6 +65,13 @@ extern void show_submodule_inline_diff(FILE *f, const char *path,
const struct diff_options *opt);
extern void set_config_fetch_recurse_submodules(int value);
extern void set_config_update_recurse_submodules(int value);
+/* Check if we want to update any submodule.*/
+extern int should_update_submodules(void);
+/*
+ * Returns the submodule struct if the given ce entry is a submodule
+ * and it should be updated. Returns NULL otherwise.
+ */
+extern const struct submodule *submodule_from_ce(const struct cache_entry *ce);
extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
extern int fetch_populated_submodules(const struct argv_array *options,
const char *prefix, int command_line_option,
--
2.12.0.rc1.52.ge239d7e709.dirty
^ permalink raw reply related
* [PATCH 12/18] update submodules: add submodule_move_head
From: Stefan Beller @ 2017-03-02 0:47 UTC (permalink / raw)
To: sbeller; +Cc: git, bmwill, novalis, sandals, hvoigt, gitster, jrnieder, ramsay
In-Reply-To: <20170302004759.27852-1-sbeller@google.com>
In later patches we introduce the options and flag for commands
that modify the working directory, e.g. git-checkout.
This piece of code will be used universally for
all these working tree modifications as it
* supports dry run to answer the question:
"Is it safe to change the submodule to this new state?"
e.g. is it overwriting untracked files or are there local
changes that would be overwritten?
* supports a force flag that can be used for resetting
the tree.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
submodule.h | 7 ++++
2 files changed, 142 insertions(+)
diff --git a/submodule.c b/submodule.c
index 0b2596e88a..bc5fecf8c5 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1239,6 +1239,141 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
return ret;
}
+static int submodule_has_dirty_index(const struct submodule *sub)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+
+ prepare_submodule_repo_env_no_git_dir(&cp.env_array);
+
+ cp.git_cmd = 1;
+ argv_array_pushl(&cp.args, "diff-index", "--quiet", \
+ "--cached", "HEAD", NULL);
+ cp.no_stdin = 1;
+ cp.no_stdout = 1;
+ cp.dir = sub->path;
+ if (start_command(&cp))
+ die("could not recurse into submodule '%s'", sub->path);
+
+ return finish_command(&cp);
+}
+
+static void submodule_reset_index(const char *path)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ prepare_submodule_repo_env_no_git_dir(&cp.env_array);
+
+ cp.git_cmd = 1;
+ cp.no_stdin = 1;
+ cp.dir = path;
+
+ argv_array_pushf(&cp.args, "--super-prefix=%s/", path);
+ argv_array_pushl(&cp.args, "read-tree", "-u", "--reset", NULL);
+
+ argv_array_push(&cp.args, EMPTY_TREE_SHA1_HEX);
+
+ if (run_command(&cp))
+ die("could not reset submodule index");
+}
+
+/**
+ * Moves a submodule at a given path from a given head to another new head.
+ * For edge cases (a submodule coming into existence or removing a submodule)
+ * pass NULL for old or new respectively.
+ */
+int submodule_move_head(const char *path,
+ const char *old,
+ const char *new,
+ unsigned flags)
+{
+ int ret = 0;
+ struct child_process cp = CHILD_PROCESS_INIT;
+ const struct submodule *sub;
+
+ sub = submodule_from_path(null_sha1, path);
+
+ if (!sub)
+ die("BUG: could not get submodule information for '%s'", path);
+
+ if (old && !(flags & SUBMODULE_MOVE_HEAD_FORCE)) {
+ /* Check if the submodule has a dirty index. */
+ if (submodule_has_dirty_index(sub))
+ return error(_("submodule '%s' has dirty index"), path);
+ }
+
+ if (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {
+ if (old) {
+ if (!submodule_uses_gitfile(path))
+ absorb_git_dir_into_superproject("", path,
+ ABSORB_GITDIR_RECURSE_SUBMODULES);
+ } else {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addf(&sb, "%s/modules/%s",
+ get_git_common_dir(), sub->name);
+ connect_work_tree_and_git_dir(path, sb.buf);
+ strbuf_release(&sb);
+
+ /* make sure the index is clean as well */
+ submodule_reset_index(path);
+ }
+ }
+
+ prepare_submodule_repo_env_no_git_dir(&cp.env_array);
+
+ cp.git_cmd = 1;
+ cp.no_stdin = 1;
+ cp.dir = path;
+
+ argv_array_pushf(&cp.args, "--super-prefix=%s/", path);
+ argv_array_pushl(&cp.args, "read-tree", NULL);
+
+ if (flags & SUBMODULE_MOVE_HEAD_DRY_RUN)
+ argv_array_push(&cp.args, "-n");
+ else
+ argv_array_push(&cp.args, "-u");
+
+ if (flags & SUBMODULE_MOVE_HEAD_FORCE)
+ argv_array_push(&cp.args, "--reset");
+ else
+ argv_array_push(&cp.args, "-m");
+
+ argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);
+ argv_array_push(&cp.args, new ? new : EMPTY_TREE_SHA1_HEX);
+
+ if (run_command(&cp)) {
+ ret = -1;
+ goto out;
+ }
+
+ if (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {
+ if (new) {
+ struct child_process cp1 = CHILD_PROCESS_INIT;
+ /* also set the HEAD accordingly */
+ cp1.git_cmd = 1;
+ cp1.no_stdin = 1;
+ cp1.dir = path;
+
+ argv_array_pushl(&cp1.args, "update-ref", "HEAD",
+ new ? new : EMPTY_TREE_SHA1_HEX, NULL);
+
+ if (run_command(&cp1)) {
+ ret = -1;
+ goto out;
+ }
+ } else {
+ struct strbuf sb = STRBUF_INIT;
+
+ strbuf_addf(&sb, "%s/.git", path);
+ unlink_or_warn(sb.buf);
+ strbuf_release(&sb);
+
+ if (is_empty_dir(path))
+ rmdir_or_warn(path);
+ }
+ }
+out:
+ return ret;
+}
+
static int find_first_merges(struct object_array *result, const char *path,
struct commit *a, struct commit *b)
{
diff --git a/submodule.h b/submodule.h
index 6f3fe85c7c..4cdf6445f7 100644
--- a/submodule.h
+++ b/submodule.h
@@ -96,6 +96,13 @@ extern int push_unpushed_submodules(struct sha1_array *commits,
extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
extern int parallel_submodules(void);
+#define SUBMODULE_MOVE_HEAD_DRY_RUN (1<<0)
+#define SUBMODULE_MOVE_HEAD_FORCE (1<<1)
+extern int submodule_move_head(const char *path,
+ const char *old,
+ const char *new,
+ unsigned flags);
+
/*
* Prepare the "env_array" parameter of a "struct child_process" for executing
* a submodule by clearing any repo-specific envirionment variables, but
--
2.12.0.rc1.52.ge239d7e709.dirty
^ permalink raw reply related
* [PATCH 08/18] update submodules: add submodule config parsing
From: Stefan Beller @ 2017-03-02 0:47 UTC (permalink / raw)
To: sbeller; +Cc: git, bmwill, novalis, sandals, hvoigt, gitster, jrnieder, ramsay
In-Reply-To: <20170302004759.27852-1-sbeller@google.com>
Similar to b33a15b08 (push: add recurseSubmodules config option,
2015-11-17) and 027771fcb1 (submodule: allow erroneous values for the
fetchRecurseSubmodules option, 2015-08-17), we add submodule-config code
that is later used to parse whether we are interested in updating
submodules.
We need the `die_on_error` parameter to be able to call this parsing
function for the config file as well, which if incorrect lets Git die.
As we're just touching the header file, also mark all functions extern.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
submodule-config.c | 20 ++++++++++++++++++++
submodule-config.h | 17 +++++++++--------
2 files changed, 29 insertions(+), 8 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index 93453909cf..3e8e380d98 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -234,6 +234,26 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)
return parse_fetch_recurse(opt, arg, 1);
}
+static int parse_update_recurse(const char *opt, const char *arg,
+ int die_on_error)
+{
+ switch (git_config_maybe_bool(opt, arg)) {
+ case 1:
+ return RECURSE_SUBMODULES_ON;
+ case 0:
+ return RECURSE_SUBMODULES_OFF;
+ default:
+ if (die_on_error)
+ die("bad %s argument: %s", opt, arg);
+ return RECURSE_SUBMODULES_ERROR;
+ }
+}
+
+int parse_update_recurse_submodules_arg(const char *opt, const char *arg)
+{
+ return parse_update_recurse(opt, arg, 1);
+}
+
static int parse_push_recurse(const char *opt, const char *arg,
int die_on_error)
{
diff --git a/submodule-config.h b/submodule-config.h
index 70f19363fd..d434ecdb45 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -22,16 +22,17 @@ struct submodule {
int recommend_shallow;
};
-int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
-int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
-int parse_submodule_config_option(const char *var, const char *value);
-const struct submodule *submodule_from_name(const unsigned char *commit_or_tree,
- const char *name);
-const struct submodule *submodule_from_path(const unsigned char *commit_or_tree,
- const char *path);
+extern int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
+extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);
+extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
+extern int parse_submodule_config_option(const char *var, const char *value);
+extern const struct submodule *submodule_from_name(
+ const unsigned char *commit_or_tree, const char *name);
+extern const struct submodule *submodule_from_path(
+ const unsigned char *commit_or_tree, const char *path);
extern int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
unsigned char *gitmodules_sha1,
struct strbuf *rev);
-void submodule_free(void);
+extern void submodule_free(void);
#endif /* SUBMODULE_CONFIG_H */
--
2.12.0.rc1.52.ge239d7e709.dirty
^ permalink raw reply related
* [PATCH 04/18] lib-submodule-update.sh: replace sha1 by hash
From: Stefan Beller @ 2017-03-02 0:47 UTC (permalink / raw)
To: sbeller; +Cc: git, bmwill, novalis, sandals, hvoigt, gitster, jrnieder, ramsay
In-Reply-To: <20170302004759.27852-1-sbeller@google.com>
Cleaning up code by generalising it.
Currently the mailing list discusses yet again how
to migrate away from sha1.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
t/lib-submodule-update.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index a906c92cfb..83202c54fc 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -171,9 +171,9 @@ reset_work_tree_to () {
git checkout -f "$1" &&
git status -u -s >actual &&
test_must_be_empty actual &&
- sha1=$(git rev-parse --revs-only HEAD:sub1) &&
- if test -n "$sha1" &&
- test $(cd "../submodule_update_sub1" && git rev-parse --verify "$sha1^{commit}")
+ hash=$(git rev-parse --revs-only HEAD:sub1) &&
+ if test -n "$hash" &&
+ test $(cd "../submodule_update_sub1" && git rev-parse --verify "$hash^{commit}")
then
git submodule update --init --recursive "sub1"
fi
--
2.12.0.rc1.52.ge239d7e709.dirty
^ permalink raw reply related
* [PATCH 16/18] entry.c: update submodules when interesting
From: Stefan Beller @ 2017-03-02 0:47 UTC (permalink / raw)
To: sbeller; +Cc: git, bmwill, novalis, sandals, hvoigt, gitster, jrnieder, ramsay
In-Reply-To: <20170302004759.27852-1-sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
entry.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/entry.c b/entry.c
index c6eea240b6..d2b512da90 100644
--- a/entry.c
+++ b/entry.c
@@ -2,6 +2,7 @@
#include "blob.h"
#include "dir.h"
#include "streaming.h"
+#include "submodule.h"
static void create_directories(const char *path, int path_len,
const struct checkout *state)
@@ -146,6 +147,7 @@ static int write_entry(struct cache_entry *ce,
unsigned long size;
size_t wrote, newsize = 0;
struct stat st;
+ const struct submodule *sub;
if (ce_mode_s_ifmt == S_IFREG) {
struct stream_filter *filter = get_stream_filter(ce->name,
@@ -203,6 +205,10 @@ static int write_entry(struct cache_entry *ce,
return error("cannot create temporary submodule %s", path);
if (mkdir(path, 0777) < 0)
return error("cannot create submodule directory %s", path);
+ sub = submodule_from_ce(ce);
+ if (sub)
+ return submodule_move_head(ce->name,
+ NULL, oid_to_hex(&ce->oid), SUBMODULE_MOVE_HEAD_FORCE);
break;
default:
return error("unknown file mode for %s in index", path);
@@ -259,7 +265,31 @@ int checkout_entry(struct cache_entry *ce,
strbuf_add(&path, ce->name, ce_namelen(ce));
if (!check_path(path.buf, path.len, &st, state->base_dir_len)) {
+ const struct submodule *sub;
unsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);
+ /*
+ * Needs to be checked before !changed returns early,
+ * as the possibly empty directory was not changed
+ */
+ sub = submodule_from_ce(ce);
+ if (sub) {
+ int err;
+ if (!is_submodule_populated_gently(ce->name, &err)) {
+ struct stat sb;
+ if (lstat(ce->name, &sb))
+ die(_("could not stat file '%s'"), ce->name);
+ if (!(st.st_mode & S_IFDIR))
+ unlink_or_warn(ce->name);
+
+ return submodule_move_head(ce->name,
+ NULL, oid_to_hex(&ce->oid),
+ SUBMODULE_MOVE_HEAD_FORCE);
+ } else
+ return submodule_move_head(ce->name,
+ "HEAD", oid_to_hex(&ce->oid),
+ SUBMODULE_MOVE_HEAD_FORCE);
+ }
+
if (!changed)
return 0;
if (!state->force) {
--
2.12.0.rc1.52.ge239d7e709.dirty
^ permalink raw reply related
* [PATCH 06/18] make is_submodule_populated gently
From: Stefan Beller @ 2017-03-02 0:47 UTC (permalink / raw)
To: sbeller; +Cc: git, bmwill, novalis, sandals, hvoigt, gitster, jrnieder, ramsay
In-Reply-To: <20170302004759.27852-1-sbeller@google.com>
We need the gentle version in a later patch. As we have just one caller,
migrate the caller.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/grep.c | 2 +-
submodule.c | 7 ++-----
submodule.h | 8 +++++++-
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef499..b17835aed6 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -616,7 +616,7 @@ static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
{
if (!is_submodule_initialized(path))
return 0;
- if (!is_submodule_populated(path)) {
+ if (!is_submodule_populated_gently(path, NULL)) {
/*
* If searching history, check for the presense of the
* submodule's gitdir before skipping the submodule.
diff --git a/submodule.c b/submodule.c
index 3b98766a6b..0e55372f37 100644
--- a/submodule.c
+++ b/submodule.c
@@ -234,15 +234,12 @@ int is_submodule_initialized(const char *path)
return ret;
}
-/*
- * Determine if a submodule has been populated at a given 'path'
- */
-int is_submodule_populated(const char *path)
+int is_submodule_populated_gently(const char *path, int *return_error_code)
{
int ret = 0;
char *gitdir = xstrfmt("%s/.git", path);
- if (resolve_gitdir(gitdir))
+ if (resolve_gitdir_gently(gitdir, return_error_code))
ret = 1;
free(gitdir);
diff --git a/submodule.h b/submodule.h
index 05ab674f06..0b915bd3ac 100644
--- a/submodule.h
+++ b/submodule.h
@@ -41,7 +41,13 @@ extern int submodule_config(const char *var, const char *value, void *cb);
extern void gitmodules_config(void);
extern void gitmodules_config_sha1(const unsigned char *commit_sha1);
extern int is_submodule_initialized(const char *path);
-extern int is_submodule_populated(const char *path);
+/*
+ * Determine if a submodule has been populated at a given 'path' by checking if
+ * the <path>/.git resolves to a valid git repository.
+ * If return_error_code is NULL, die on error.
+ * Otherwise the return error code is the same as of resolve_gitdir_gently.
+ */
+extern int is_submodule_populated_gently(const char *path, int *return_error_code);
extern int parse_submodule_update_strategy(const char *value,
struct submodule_update_strategy *dst);
extern const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
--
2.12.0.rc1.52.ge239d7e709.dirty
^ permalink raw reply related
* [PATCH 07/18] connect_work_tree_and_git_dir: safely create leading directories
From: Stefan Beller @ 2017-03-02 0:47 UTC (permalink / raw)
To: sbeller; +Cc: git, bmwill, novalis, sandals, hvoigt, gitster, jrnieder, ramsay
In-Reply-To: <20170302004759.27852-1-sbeller@google.com>
In a later patch we'll use connect_work_tree_and_git_dir when the
directory for the gitlink file doesn't exist yet. This patch makes
connect_work_tree_and_git_dir safe to use for both cases of
either the git dir or the working dir missing.
To do so, we need to call safe_create_leading_directories[_const]
on both directories. However this has to happen before we construct
the absolute paths as real_pathdup assumes the directories to
be there already.
So for both the config file in the git dir as well as the .git link
file we need to
a) construct the name
b) call SCLD
c) get the absolute path
d) once a-c is done for both we can consume the absolute path
to compute the relative path to each other and store those
relative paths.
The implementation provided here puts a) and b) for both cases first,
and then performs c and d after.
One of the two users of 'connect_work_tree_and_git_dir' already checked
for the directory being there, so we can loose that check as
connect_work_tree_and_git_dir handles this functionality now.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
dir.c | 32 +++++++++++++++++++++-----------
submodule.c | 11 ++---------
2 files changed, 23 insertions(+), 20 deletions(-)
diff --git a/dir.c b/dir.c
index 4541f9e146..6f52af7abb 100644
--- a/dir.c
+++ b/dir.c
@@ -2728,23 +2728,33 @@ void untracked_cache_add_to_index(struct index_state *istate,
/* Update gitfile and core.worktree setting to connect work tree and git dir */
void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
{
- struct strbuf file_name = STRBUF_INIT;
+ struct strbuf gitfile_sb = STRBUF_INIT;
+ struct strbuf cfg_sb = STRBUF_INIT;
struct strbuf rel_path = STRBUF_INIT;
- char *git_dir = real_pathdup(git_dir_);
- char *work_tree = real_pathdup(work_tree_);
+ char *git_dir, *work_tree;
- /* Update gitfile */
- strbuf_addf(&file_name, "%s/.git", work_tree);
- write_file(file_name.buf, "gitdir: %s",
- relative_path(git_dir, work_tree, &rel_path));
+ /* Prepare .git file */
+ strbuf_addf(&gitfile_sb, "%s/.git", work_tree_);
+ if (safe_create_leading_directories_const(gitfile_sb.buf))
+ die(_("could not create directories for %s"), gitfile_sb.buf);
+
+ /* Prepare config file */
+ strbuf_addf(&cfg_sb, "%s/config", git_dir_);
+ if (safe_create_leading_directories_const(cfg_sb.buf))
+ die(_("could not create directories for %s"), cfg_sb.buf);
+ git_dir = real_pathdup(git_dir_);
+ work_tree = real_pathdup(work_tree_);
+
+ /* Write .git file */
+ write_file(gitfile_sb.buf, "gitdir: %s",
+ relative_path(git_dir, work_tree, &rel_path));
/* Update core.worktree setting */
- strbuf_reset(&file_name);
- strbuf_addf(&file_name, "%s/config", git_dir);
- git_config_set_in_file(file_name.buf, "core.worktree",
+ git_config_set_in_file(cfg_sb.buf, "core.worktree",
relative_path(work_tree, git_dir, &rel_path));
- strbuf_release(&file_name);
+ strbuf_release(&gitfile_sb);
+ strbuf_release(&cfg_sb);
strbuf_release(&rel_path);
free(work_tree);
free(git_dir);
diff --git a/submodule.c b/submodule.c
index 0e55372f37..04d185738f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1442,8 +1442,6 @@ void absorb_git_dir_into_superproject(const char *prefix,
/* Not populated? */
if (!sub_git_dir) {
- char *real_new_git_dir;
- const char *new_git_dir;
const struct submodule *sub;
if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
@@ -1466,13 +1464,8 @@ void absorb_git_dir_into_superproject(const char *prefix,
sub = submodule_from_path(null_sha1, path);
if (!sub)
die(_("could not lookup name for submodule '%s'"), path);
- new_git_dir = git_path("modules/%s", sub->name);
- if (safe_create_leading_directories_const(new_git_dir) < 0)
- die(_("could not create directory '%s'"), new_git_dir);
- real_new_git_dir = real_pathdup(new_git_dir);
- connect_work_tree_and_git_dir(path, real_new_git_dir);
-
- free(real_new_git_dir);
+ connect_work_tree_and_git_dir(path,
+ git_path("modules/%s", sub->name));
} else {
/* Is it already absorbed into the superprojects git dir? */
char *real_sub_git_dir = real_pathdup(sub_git_dir);
--
2.12.0.rc1.52.ge239d7e709.dirty
^ permalink raw reply related
* [PATCH 17/18] builtin/checkout: add --recurse-submodules switch
From: Stefan Beller @ 2017-03-02 0:47 UTC (permalink / raw)
To: sbeller; +Cc: git, bmwill, novalis, sandals, hvoigt, gitster, jrnieder, ramsay
In-Reply-To: <20170302004759.27852-1-sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Documentation/git-checkout.txt | 7 +++++++
builtin/checkout.c | 28 ++++++++++++++++++++++++++++
t/lib-submodule-update.sh | 23 +++++++++++++++++------
t/t2013-checkout-submodule.sh | 5 +++++
4 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 8e2c0662dd..d6399c0af8 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -256,6 +256,13 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
out anyway. In other words, the ref can be held by more than one
worktree.
+--[no-]recurse-submodules::
+ Using --recurse-submodules will update the content of all initialized
+ submodules according to the commit recorded in the superproject. If
+ local modifications in a submodule would be overwritten the checkout
+ will fail unless `-f` is used. If nothing (or --no-recurse-submodules)
+ is used, the work trees of submodules will not be updated.
+
<branch>::
Branch to checkout; if it refers to a branch (i.e., a name that,
when prepended with "refs/heads/", is a valid ref), then that
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f174f50303..e9c5fcfaf8 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -21,12 +21,31 @@
#include "submodule-config.h"
#include "submodule.h"
+static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+
static const char * const checkout_usage[] = {
N_("git checkout [<options>] <branch>"),
N_("git checkout [<options>] [<branch>] -- <file>..."),
NULL,
};
+static int option_parse_recurse_submodules(const struct option *opt,
+ const char *arg, int unset)
+{
+ if (unset) {
+ recurse_submodules = RECURSE_SUBMODULES_OFF;
+ return 0;
+ }
+ if (arg)
+ recurse_submodules =
+ parse_update_recurse_submodules_arg(opt->long_name,
+ arg);
+ else
+ recurse_submodules = RECURSE_SUBMODULES_ON;
+
+ return 0;
+}
+
struct checkout_opts {
int patch_mode;
int quiet;
@@ -1163,6 +1182,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
N_("second guess 'git checkout <no-such-branch>'")),
OPT_BOOL(0, "ignore-other-worktrees", &opts.ignore_other_worktrees,
N_("do not check if another worktree is holding the given ref")),
+ { OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules,
+ "checkout", "control recursive updating of submodules",
+ PARSE_OPT_OPTARG, option_parse_recurse_submodules },
OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
OPT_END(),
};
@@ -1193,6 +1215,12 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
}
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
+ git_config(submodule_config, NULL);
+ if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
+ set_config_update_recurse_submodules(recurse_submodules);
+ }
+
if ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1)
die(_("-b, -B and --orphan are mutually exclusive"));
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index aa196d4d42..d71972c3db 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -782,6 +782,11 @@ test_submodule_forced_switch () {
test_submodule_switch_recursing () {
command="$1"
+ RESULT=success
+ if test "$KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS" = 1
+ then
+ RESULT=failure
+ fi
######################### Appearing submodule #########################
# Switching to a commit letting a submodule appear checks it out ...
test_expect_success "$command: added submodule is checked out" '
@@ -891,7 +896,7 @@ test_submodule_switch_recursing () {
'
# Replacing a submodule with files in a directory must succeeds
# when the submodule is clean
- test_expect_success "$command: replace submodule with a directory" '
+ test_expect_$RESULT "$command: replace submodule with a directory" '
prolog &&
reset_work_tree_to_interested add_sub1 &&
(
@@ -903,7 +908,7 @@ test_submodule_switch_recursing () {
)
'
# ... absorbing a .git directory.
- test_expect_success "$command: replace submodule containing a .git directory with a directory must absorb the git dir" '
+ test_expect_$RESULT "$command: replace submodule containing a .git directory with a directory must absorb the git dir" '
prolog &&
reset_work_tree_to_interested add_sub1 &&
(
@@ -931,7 +936,7 @@ test_submodule_switch_recursing () {
'
# ... must check its local work tree for untracked files
- test_expect_success "$command: replace submodule with a file must fail with untracked files" '
+ test_expect_$RESULT "$command: replace submodule with a file must fail with untracked files" '
prolog &&
reset_work_tree_to_interested add_sub1 &&
(
@@ -987,7 +992,8 @@ test_submodule_switch_recursing () {
)
'
- test_expect_success "$command: modified submodule updates submodule recursively" '
+ # recursing deeper than one level doesn't work yet.
+ test_expect_failure "$command: modified submodule updates submodule recursively" '
prolog &&
reset_work_tree_to_interested add_nested_sub &&
(
@@ -1006,6 +1012,11 @@ test_submodule_switch_recursing () {
# the superproject as well as the submodule is allowed.
test_submodule_forced_switch_recursing () {
command="$1"
+ RESULT=success
+ if test "$KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS" = 1
+ then
+ RESULT=failure
+ fi
######################### Appearing submodule #########################
# Switching to a commit letting a submodule appear creates empty dir ...
test_expect_success "$command: added submodule is checked out" '
@@ -1100,7 +1111,7 @@ test_submodule_forced_switch_recursing () {
)
'
# Replacing a submodule with files in a directory ...
- test_expect_success "$command: replace submodule with a directory" '
+ test_expect_$RESULT "$command: replace submodule with a directory" '
prolog &&
reset_work_tree_to_interested add_sub1 &&
(
@@ -1151,7 +1162,7 @@ test_submodule_forced_switch_recursing () {
'
# ... but stops for untracked files that would be lost
- test_expect_success "$command: replace submodule with a file" '
+ test_expect_$RESULT "$command: replace submodule with a file stops for untracked files" '
prolog &&
reset_work_tree_to_interested add_sub1 &&
(
diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index 6847f75822..aa35223369 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh
@@ -63,6 +63,11 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
! test -s actual
'
+KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
+test_submodule_switch_recursing "git checkout --recurse-submodules"
+
+test_submodule_forced_switch_recursing "git checkout -f --recurse-submodules"
+
test_submodule_switch "git checkout"
test_submodule_forced_switch "git checkout -f"
--
2.12.0.rc1.52.ge239d7e709.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