[DRAFT] for testing : Fix 4Gb limit for large files on Git for Windows

[PhilipOakley]

This patch series should fix the large file limit on Windows where 'long' is only 32 bits.

Hopefully this PR will go through the CI/Test pipeline to check if all the patches pass the test suit
in a proper incremental manner. Plus test the MSVC=1 option.

Signed-off-by: Philip Oakley philipoakley@iee.org

The series did compile at every step, and rebased cleanly on the latest shears/pu.

[PhilipOakley]

The DCO bot is in error (at least from my viewpoint). The ref commit was provided by the signed off by author. It was a necessary pre-requisite, though does have some obvious conflict fixes with the upstream pu.

Commit sha: 9d0e45d, Author: Philip Oakley, Committer: Philip Oakley;
Expected "Philip Oakley philipoakley@iee.org", but got "Torsten Bögershausen tboegi@web.de".

Not sure if I should just double sign Torsten's patch?

[PhilipOakley]

I went back and rebase the series -i and amended the author for the first patch to be Torsten and force pushed.

Unsure how to get the CI testing to get started.

[dscho]

Not sure if I should just double sign Torsten's patch?

Probably, but you will want to make sure that the recorded author matches the first Signed-off-by: line.

[dscho]

Could I ask you to re-target this PR to master instead of shears/pu? The latter is a really fast-moving target, hard to catch.

[dscho]

(You will of course want to run git rebase -i --onto git-for-windows/master 5539b3d26c81c622c40ea34f117a0afe4c789293 and force-push...)

[PhilipOakley]

@dscho I'll rebuild/rebase the series in the next day or three, applying Torsten's patch directly at the start of the series.

I'd deliberately targeted pu to at least be ahead of the game for the conflicts with the extern that are being considered upstream (which raises the issue of how to pass around pre-determined rerere redo fixups for such conflicts

As a side issue, I'm having problems fathoming how the MSVC=1 build should work seeing as I need to patch compat\vcbuild\MSVC-DEFS-GEN which is generated apparently by the vcpkg but I can't find where. See my 8f635b2f-a42b-b160-bc51-5fdcb552f6f3@iee.org on the googlegroups list https://groups.google.com/forum/#!topic/git-for-windows/Y99a0dzlVJY.

[PhilipOakley]

@dscho

I've decided against fighting the conflicts until the extern series lands in shears/master.

(What's cooking in git.git (May 2019, #1; Thu, 9))

• dl/no-extern-in-func-decl (2019-05-05) 3 commits
  (merged to 'next' on 2019-05-09 at d165ac4)
  + *.[ch]: manually align parameter lists
  + *.[ch]: remove extern from function declarations using sed
  + *.[ch]: remove extern from function declarations using spatch

Mechanically and systematically drop "extern" from function
declarlation.

Will merge to 'master'.

Once that's landed my rebase should be cleaner and easier - it was too easy to get confused as to which way all the conflicts were going, especially as they are, for the purpose of the series, incidental irrelevances.

[dscho]

I've decided against fighting the conflicts until the extern series lands in shears/master.

Makes a ton of sense to me, @PhilipOakley !

[PhilipOakley]

I've decided against fighting the conflicts until the extern series lands in shears/master.

Makes a ton of sense to me, @PhilipOakley !

@dscho I see Junio has announced Git v2.22.0-rc0, with the extern series included. Could you ping me when shears/master gets the update? 🚥

[PhilipOakley]

just seen (follow up to the rc0 announcement):

phili@Philip-Win10 MINGW64 /usr/src/git (size_t7)
$ git fetch --all
Fetching origin
From https://github.com/git-for-windows/git
   d4e5e1ea92..898cf2707e  master     -> origin/master
Fetching my
Fetching junio
Fetching gitster
Fetching dscho-git

So I hope to get on it today.

[dscho]

So I hope to get on it today.

Oh, sorry! I was so heads-down into getting Git for Windows v2.22.0-rc0 out the door (and then on trying to tie up a few loose ends in Git v2.22.0-rc0 in time for -rc1) that I missed this.

The update of master that you received does not include the changes in v2.22.0-rc0.

You may ask "why?" and it is a very legitimate question. The answer boils down to "I want to keep the door open for Git for Windows v2.21.1, if need be".

You see, I am not at all a fan of many release branches. And my automation really is centered around master, so it would make things a bit hairy if I had to publish v2.21.1 after rebasing master onto the v2.22.0-rc* train.

So the update to master that you got is a rebase onto v2.21.0, essentially, for the sole purpose of restructuring the entire branch thicket (and applying all the fixup!s/squash!s). In other words, if you call git diff d4e5e1ea92..898cf2707e, you will see that I only reworded one comment and moved it slightly. That is the tree diff. The range-diff looks quite a bit different, though! I moved tons of branches into the ready-for-upstream thicket, in preparation for upstreaming them after Git for Windows v2.22.0 is out.

However, hope is near ;-)

I do, of course, make those -rc* previews from Git commits, but those live on prepare-for-v2.22.0 (and are tested in #2192).

And once Git v2.22.0 is close enough (I won't publish a Git for Windows v2.21.1 if I expect to publish a v2.22.0 within a week, unless there are serious security concerns), I will fast-forward master to prepare-for-v2.22.0 (possibly merging master into the prepare-for-v2.22.0 before that).

Therefore, I would suggest to simply re-target your branch to prepare-for-v2.22.0 for the moment, that'll be stable enough.

[dscho]

Many of the casts were about having an abundance of caution regarding the whole 'implementation defined' aspect of this here 'Bug' (i.e. we can't trust anything for the 64 vs 32 bit long issue ;-)

I understand that. Yet I think some of that caution erred on the side of redundancy, and I would rather not spend the time on the mailing list trying to defend unnecessary casts...

[tboegi]

@dscho:
I couldn't find a way to split up the "big patch" into smaller ones without risking
that the steps in between don't compile cleanly.
(And my personal view is that it is the same amount of work to review a patch
with 84 file changed or to review 84 patches changing 1 file each)
@ PhilipOakley I re-worded the commit message and rebased to gfw/master:

https://github.com/tboegi/git/tree/tb.190828_convert_size_t_mk_size_t

[dscho]

my personal view is that it is the same amount of work to review a patch
with 84 file changed or to review 84 patches changing 1 file each

Having reviewed many patch series, I much prefer reviewing a dozen relatively short patch series over the course of a dozen weeks to having to review all the changes in one big patch. Remember: there are patches so simple and elegant that there is no obvious place for bugs to hide, and there are patches so gigantic and repetitive that there is no obvious place for bugs to hide (but plenty of non-obvious places).

No, I still think we need to do our best job to make this thing easy to review.

If you can augment that big patch by a small Coccinelle patch that generates it, that would make it reviewable, of course. But that enormous wall of diff? Not reviewable at all.

[PhilipOakley]

@dscho The first size_t commit is not going away. It has always been that big because there are compilers that do not accept less. This is part of the inconsistent behaviours of these implementation defined behaviours (the type size_t is not a C89 feature, even though large pointer spaces are allowed you can't name them).

It maybe that someone wants to split it into a few "won't compile on my machine" patches, but I'd rather we stay with what we have. Torsten was compiling for Rasbian (gcc (Raspbian 6.3.0-18+rpi1+deb9u1) 6.3.0 20170516), not GfW anyway. Martin Koegler's series (mk/use-size-t-in-zlib still on pu) was started in 2017 as a >4Gb issue (found by searching the archive for LLP64)

The size_t stuff actually had compile warnings back as far as 2007. see https://public-inbox.org/git/7vabr0djqr.fsf@gitster.siamese.dyndns.org/, so at some point maybe we need to bite the bullet and actually do the big change.

...

my main problem is the test system, which is something I'm not that familiar with.

I now see the tests as being:
Test 0 check we are linked to the right library (the compile flags helper reports, and match prereqs)
Test 1 needs to: 'add large file as loose object', invokes zlib only, check it reads back OK, without invoking wider code sections.
Test 2 should be ' commit large file as a pack', invokes crc32 checking (same pointer issue as zlib, but fresh code)

Maybe need a size graded test: 1.5Gb, 3.5Gb, 5.5Gb to walk through the two potential barriers at 2Gb and 4Gb.

We also need an easily accessible compiler that has something equivalent to the former -Wshorten-64-to-32 (or vice versa) e.g. Clang? (un-tested). - Just found https://public-inbox.org/git/20160705170558.10906-1-pclouds@gmail.com/ in the archives, so it's something.
...

[dscho]

The first size_t commit is not going away.

I remember clearly that I was able to split out a part from it, earlier this year, into a self-contained commit.

Yes, it is possible to do this incrementally. It absolutely is. You can easily pass unsigned long to functions accepting size_t, that is not a problem! So you can fix an API function to accept size_t, leave its callers alone (until you take care of them, in a later patch). The trick is to break this PR down into bite-sized chunks, so that we can benefit maximally from code review (if the patches are too big, you will not benefit from that extra safety).

[dscho]

The first size_t commit is not going away.

I remember clearly that I was able to split out a part from it, earlier this year, into a self-contained commit.

I think I even mentioned this somewhere in this PR or in a related one. And note: I did not even try hard. So I don't buy the argument "this patch is big, it cannot be broken down further". I don't buy that at all.

[dscho]

The first size_t commit is not going away. It has always been that big because there are compilers that do not accept less.

Let #3487 serve as a counter argument, Tiny, well-contained changes from unsigned long to size_t that compile and pass, and even demonstrably fix bugs.

@PhilipOakley If you are truly interested in pushing this forward, I encourage you to have a look at the end of this PR comment, where I follow the rabbit hole of the call chain of read_object_file(). This lays out a natural plan to split up that big commit in neat, compileable patches: just convert the individual functions of that call tree, leaves first.

Granted, that won't be as easily written as a semi-automated search-and-replace. But then, such a semi-automated, huge commit has not a slither of a chance to get reviewed on the Git mailing list, let alone accepted.

[PhilipOakley]

a natural plan to split up that big commit in neat, compileable patches: just convert the individual functions of that call tree, leaves first.

I had a simplistic look at the call tree to see how many times each was referenced.

    read_object_file()  38 ***
        repo_read_object_file() 5 
            read_object_file_extended() 2
                read_object() 3 (2 identical fns)
                    oid_object_info_extended() 26 [if's]
                        do_oid_object_info_extended() 2 [ret]
                            loose_object_info() 1 [self]
                                parse_loose_header_extended()  3[if's]
                            packed_object_info()  5
                                cache_or_unpack_entry()  1  **
                                    unpack_entry() 1
                                        unpack_object_header() 7
                                        -> unpack_object_header_buffer() 3 + t/1
                                get_size_from_delta() 2
                                    get_delta_hdr_size() 4

following your rabbit hole of the call chain (URLs in link) of read_object_file().

My first wondering was : Depth first or Width first?
There is the potential for a bit of a combinatorial explosion there.
(I did notice how the solution there for git-lfs used casts and flipped to the fn marking for the tbd steps)

I noticed there were two identical static void *read_object( definitions (object-file.c and packfile.c) though I wasn't sure if it was supposed to happen.

I may, as a narrow starter, just do hash-object for a loose object only (behind a bigFileThreshold protection..), to try to drain rod the process with a simple testable example. (i.e. no pack files at this step.! )

[dscho]

My first wondering was : Depth first or Width first?

You could try both approaches, and then go with the one that is easier to review.

[dscho]

I guess we can close this in favor of #6068...

[tboegi]

I guess we can close this in favor of #6068...

@dscho Unless I am wrong: #6068 (now replaced by #6069) is not fully replacing this PR.

[dscho]

@tboegi right you are. But together with #6289, it is ;-) Unless I missed something?

[tboegi]

@tboegi right you are. But together with #6289, it is ;-) Unless I missed something?

@dscho In short: OK for me to close this.

[dscho]

For the record, I am slowly upstreaming this:

• Handle cloning of objects larger than 4GB on Windows gitgitgadget/git#2102
• More work supporting objects larger than 4GB on Windows gitgitgadget/git#2137
• Support hashing objects larger than 4GB on Windows gitgitgadget/git#2138
• Next size_t stop: pack-objects/delta gitgitgadget/git#2175
• TBD: the rest of 7b85e2a...a9a851bd6f99^2