* Re: [RFC PATCH 1/3] merge-ort.c: finalize ODB transactions after each step
From: Elijah Newren @ 2023-11-11 3:45 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Jeff King, Patrick Steinhardt, Junio C Hamano,
Johannes Schindelin
In-Reply-To: <c615a61d32644b64ef8f47feb47ec909286c56b3.1699381371.git.me@ttaylorr.com>
On Tue, Nov 7, 2023 at 10:22 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> In a previous commit, the ORT merge backend learned how to use the
> bulk-checkin mechanism to emit a single pack containing any new objects
> created during the merge. This functionality was implemented by setting
> up a new ODB transaction, and finalizing it at the end of the merge via
> `process_entries()`.
>
> In a future commit, we will extend this functionality to the new `git
> replay` command, which needs to see objects from previous steps in order
> to replay each commit.
>
> As a step towards implementing this, teach the ORT backend to flush the
> ODB transaction at the end of each step in `process_entries()`, and then
> finalize the result with `end_odb_transaction()` when calling
> `merge_finalize()`.
process_entries() contains a for loop inside it, so "end of each step
in `process_entries()`" sounds like you are flushing after entry, i.e.
several times per commit being replayed.
Perhaps "at the end of `process_entries()` (thus creating one pack per
commit)" ?
(Of course, the fact that we need this change earlier, in the other
series, kinda makes this point moot. Just thought I'd mention it
anyway in case it comes back up in your restructuring.)
^ permalink raw reply
* Re: [RFC PATCH 0/3] replay: implement support for writing new objects to a pack
From: Elijah Newren @ 2023-11-11 3:42 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Jeff King, Patrick Steinhardt, Junio C Hamano,
Johannes Schindelin
In-Reply-To: <cover.1699381371.git.me@ttaylorr.com>
On Tue, Nov 7, 2023 at 10:22 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> (Based on a combination of Christian's cc/git-replay and my
> tb/merge-tree-write-pack branches).
>
> This RFC demonstrates extending the new `--write-pack` option that
> `merge-tree` recently learned to the `replay` builtin as well.
>
> The approach is as follows:
>
> - write a pack out after each step in the replay operation, so that
> subsequent steps may see any new object(s) created during previous
> steps
>
> - combine those packs into one before migrating them back into the
> main object store
>
> This is accomplished with a combination of the bulk-checkin and
> tmp-objdir APIs, with some minor modifications made to when we flush out
> and finalize bulk-checkin transactions.
>
> The benefit to this approach is that we bound the number of inodes
> required per replayed commit to a constant (by default, 3: one for the
> .pack, one for the .idx, and another for the .rev file), instead of
> having each operation take an unbounded number of inodes proportional to
> the number of new objects created during that step. We also only migrate
> a single pack back to the main object store.
Isn't it actually 4? Since you only put blobs and trees into the bulk
checkin packfiles, the commit object will still be loose.
> In other words, this makes the maximum number of inodes required by
> 'replay' grow proportional to the number of commits being replayed,
> instead of the number of new *objects* created as a result of the replay
> operation.
As per comments on the other series, we actually need 2 packs per
commit (when replaying non-merge commits, we don't have to worry about
recursive merges, so 2*depth is just 2), so this would be 7 inodes per
commit (3 files per pack * 2 packs + 1 loose commit object).
I was curious what the comparative number of loose objects might be if
we didn't use the bulk checking, so:
$ cd linux.git
$ git rev-list --no-merges --count HEAD
1141436
$ time git log --oneline --no-merges --name-only | wc -l
3781628
$ python -c 'print(3781628/1141436)'
3.3130442705504293
So, if repositories are similar to linux, that's a bit over 3 files
modified per commit. It's a bit harder to count trees, but let's take
a wild guess and say that each file is 7 directories (because I'm too
lazy to do real research and 7 happens to give me nice round numbers
later), so that's up to 7*3 tree objects. So 3 file objects + 21 tree
objects + 1 commit object = 25 loose objects.
So, your scheme certainly seems to reduce number of inodes, but does
it introduce a different scaling issue? git-gc repacks when there are
>= 50 packs, or when there are >= 6700 loose objects, suggesting that
if we exceed those numbers, we might start seeing performance suffer.
We would hit 50 packs with a mere 25 commits being replayed, and
wouldn't expect to get to 6700 loose objects until we replayed about
268 commits (6700/25). Does this mean we are risking worse
performance degradation with this scheme than with just using loose
objects until the end of the operation?
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Taylor Blau @ 2023-11-11 1:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren, git, Eric W. Biederman, Jeff King,
Patrick Steinhardt
In-Reply-To: <xmqqmsvlrv5t.fsf@gitster.g>
On Sat, Nov 11, 2023 at 09:27:42AM +0900, Junio C Hamano wrote:
> Elijah Newren <newren@gmail.com> writes:
>
> > I believe the above is built on an assumption that any objects written
> > will not need to be read until after the merge is completed. And we
> > might have a nesting issue too...
> > ...
> > This is unsafe; the object may need to be read later within the same
> > merge.
>
> Thanks for a good analysis. I agree.
Ditto. I responded to Elijah more in-depth elsewhere in the thread, but
I think for your purposes it is OK to discard this series.
Thanks,
Taylor
^ permalink raw reply
* Re: first-class conflicts?
From: Junio C Hamano @ 2023-11-11 1:31 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Sandra Snan, git, Dragan Simic, rsbecker
In-Reply-To: <ZUmJyFs7z7wdmLVK@mit.edu>
"Theodore Ts'o" <tytso@mit.edu> writes:
> And if you attempt to commit the merge without resolving the
> conflicts, git won't let you:
>
> error: Committing is not possible because you have unmerged files.
> hint: Fix them up in the work tree, and then use 'git add/rm <file>'
> hint: as appropriate to mark resolution and make a commit.
>
> So it's hard to miss the indications of the content conflict, because
> if you try to commit without resolving them, it's not a warning, it's
> an outright error.
Correct but with a caveat: it is too easy for lazy folks to
circumvent the safety by mistake with "commit -a".
I wonder if it would help users to add a new configuration option
for those who want to live safer that tells "commit -a" to leave
unmerged paths alone and require the unmerged paths to be added
explicitly (which may have to extend to cover things like "add -u"
and "add .").
Perhaps not. I often find myself doing "git add -u" after resolving
conflicts and re-reading the result, without an explicit pathspec.
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Taylor Blau @ 2023-11-11 1:24 UTC (permalink / raw)
To: Elijah Newren
Cc: git, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <CABPp-BEfy9VOvimP9==ry_rZXu=metOQ8s=_-XiG_Pdx9c06Ww@mail.gmail.com>
On Fri, Nov 10, 2023 at 03:51:18PM -0800, Elijah Newren wrote:
> > @@ -2108,10 +2109,19 @@ static int handle_content_merge(struct merge_options *opt,
> > if ((merge_status < 0) || !result_buf.ptr)
> > ret = error(_("failed to execute internal merge"));
> >
> > - if (!ret &&
> > - write_object_file(result_buf.ptr, result_buf.size,
> > - OBJ_BLOB, &result->oid))
> > - ret = error(_("unable to add %s to database"), path);
> > + if (!ret) {
> > + ret = opt->write_pack
> > + ? index_blob_bulk_checkin_incore(&result->oid,
> > + result_buf.ptr,
> > + result_buf.size,
> > + path, 1)
> > + : write_object_file(result_buf.ptr,
> > + result_buf.size,
> > + OBJ_BLOB, &result->oid);
> > + if (ret)
> > + ret = error(_("unable to add %s to database"),
> > + path);
> > + }
> >
> > free(result_buf.ptr);
> > if (ret)
>
> This is unsafe; the object may need to be read later within the same
> merge. Perhaps the simplest example related to your testcase is
> modifying the middle section of that testcase (I'll highlight where
> when I comment on the testcase) to read:
>
> test_commit -C repo base file "$(test_seq 3 5)" &&
> git -C repo branch -M main &&
> git -C repo checkout -b side main &&
> test_commit -C repo side-file file "$(test_seq 1 5)" &&
> test_commit -C repo side-file2 file2 "$(test_seq 1 6)" &&
> git -C repo checkout -b other main &&
> test_commit -C repo other-file file "$(test_seq 3 7)" &&
> git -C repo mv file file2 &&
> git -C repo commit -m other-file2 &&
>
> find repo/$packdir -type f -name "pack-*.idx" >packs.before &&
> git -C repo merge-tree --write-pack side other &&
>
> In words, what I'm doing here is having both sides modify "file" (the
> same as you did) but also having one side rename "file"->"file2". The
> side that doesn't rename "file" also introduces a new "file2". ort
> needs to merge the three versions of "file" to get a new blob object,
> and then merge that with the content from the brand new "file2". More
> complicated cases are possible, of course. Anyway, with the modified
> testcase above, merge-tree will fail with:
>
> fatal: unable to read blob object 06e567b11dfdafeaf7d3edcc89864149383aeab6
>
> I think (untested) that you could fix this by creating two packs
> instead of just one. In particular, add a call to
> flush_odb_transcation() after the "redo_after_renames" block in
> merge_ort_nonrecursive_internal(). (It's probably easier to see that
> you could place the flush_odb_transaction() call inside
> detect_and_process_renames() just after the process_renames() call,
> but when redo_after_renames is true you'd end up with three packs
> instead of just two).
Great analysis, thanks for catching this error. I tested your approach,
and indeed a flush_odb_transaction() call after the process_renames()
call in detect_and_process_renames() does do the trick.
> What happens with the odb transaction stuff if no new objects are
> written before the call to flush_odb_transaction? Will that be a
> problem?
I think that the bulk-checkin code is flexible enough to understand that
we shouldn't do anything when there aren't any objects to pack.
> (Since any tree written will not be re-read within the same merge, the
> other write_object_file() call you changed does not have the same
> problem as the one here.)
>
> >@@ -4332,9 +4349,13 @@ static int process_entries(struct merge_options *opt,
> > fflush(stdout);
> > BUG("dir_metadata accounting completely off; shouldn't happen");
> > }
> >- if (write_tree(result_oid, &dir_metadata.versions, 0,
> >+ if (write_tree(opt, result_oid, &dir_metadata.versions, 0,
> > opt->repo->hash_algo->rawsz) < 0)
> > ret = -1;
> >
> > +
> > + if (opt->write_pack)
> > + end_odb_transaction();
> > +
>
> Is the end_odb_transaction() here going to fail with an "Unbalanced
> ODB transaction nesting" when faced with a recursive merge?
I think so, and we should have a test-case demonstrating that. In the
remaining three patches that I posted to extend this approach to 'git
replay', I moved this call elsewhere in such a way that I think
> Perhaps flushing here, and then calling end_odb_transaction() in
> merge_finalize(), much as you do in your replay-and-write-pack series,
> should actually be moved to this commit and included here?
Yep, exactly.
> This does mean that for a recursive merge, that you'll get up to 2*N
> packfiles, where N is the depth of the recursive merge.
We definitely want to avoid that ;-). I think there are a couple of
potential directions forward here, but the most promising one I think is
due to Johannes who suggests that we write loose objects into a
temporary directory with a replace_tmp_objdir() call, and then repack
that side directory before migrating a single pack back into the main
object store.
Thanks,
eaylor
^ permalink raw reply
* Re: first-class conflicts?
From: Theodore Ts'o @ 2023-11-07 0:50 UTC (permalink / raw)
To: Sandra Snan; +Cc: git, Dragan Simic, rsbecker
In-Reply-To: <Gr..Y5kkszDx87g@idiomdrottning.org>
On Mon, Nov 06, 2023 at 10:45:03PM +0000, Sandra Snan wrote:
> Randall, thank you for that.
>
> I just have sometimes wish git could be a little more aware of them beyond
> just storing them with ASCII art in the files themselves (and alerting /
> warning when they happen but I often can't properly see those warnings flash
> by so I end up having to search for the conflict markers manually). So if
> conflicts are a thing that *can* happen, it'd be better if vc could know
> about them which would make some of the rebases simpler as in jj. That
> doesn't mean we wanna adopt the jj workflow of deliberately checking in
> conflicts (not even locally), just be able to deal with them better if it
> does happen.
Well, if you miss them, "git status" does show that there are conflicts:
Unmerged paths:
(use "git add <file>..." to mark resolution)
both modified: version.h
And if you attempt to commit the merge without resolving the
conflicts, git won't let you:
error: Committing is not possible because you have unmerged files.
hint: Fix them up in the work tree, and then use 'git add/rm <file>'
hint: as appropriate to mark resolution and make a commit.
So it's hard to miss the indications of the content conflict, because
if you try to commit without resolving them, it's not a warning, it's
an outright error.
Cheers,
- Ted
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Junio C Hamano @ 2023-11-11 0:27 UTC (permalink / raw)
To: Elijah Newren
Cc: Taylor Blau, git, Eric W. Biederman, Jeff King,
Patrick Steinhardt
In-Reply-To: <CABPp-BEfy9VOvimP9==ry_rZXu=metOQ8s=_-XiG_Pdx9c06Ww@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> I believe the above is built on an assumption that any objects written
> will not need to be read until after the merge is completed. And we
> might have a nesting issue too...
> ...
> This is unsafe; the object may need to be read later within the same
> merge.
Thanks for a good analysis. I agree.
^ permalink raw reply
* Re: [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Junio C Hamano @ 2023-11-11 0:20 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, git
In-Reply-To: <xmqq34xdtac3.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> I am not so surprised that this one was missed, though. I didn't
> point this one out during my review of the previous round, either,
> and not everybody is as careful as you are.
Ah, sorry, thist came out in a way I did not mean to.
I didn't mean "I did not point it out explicitly. It is not
surprising if a contributor who was not careful did not find it on
their own and took initiative to fix it themselves".
I meant "I failed to spot it myself hence I didn't point it out in
my review---I was not being so careful to aim for thoroughly cover
and find all the similar issues".
In any case, I'll tweak it while queueing. Thanks for noticing.
diff --git i/t/valgrind/valgrind.sh w/t/valgrind/valgrind.sh
index 9fbf90cee7..3c8ee19975 100755
--- i/t/valgrind/valgrind.sh
+++ w/t/valgrind/valgrind.sh
@@ -23,7 +23,7 @@ memcheck)
VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
test 3 -gt "$VALGRIND_MAJOR" ||
- ( test 3 -eq "$VALGRIND_MAJOR" && test 4 -gt "$VALGRIND_MINOR" ) ||
+ { test 3 -eq "$VALGRIND_MAJOR" && test 4 -gt "$VALGRIND_MINOR"; } ||
TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
;;
*)
^ permalink raw reply related
* Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Elijah Newren @ 2023-11-11 0:17 UTC (permalink / raw)
To: Eric Sunshine
Cc: Taylor Blau, Patrick Steinhardt, git, Eric W. Biederman,
Jeff King, Junio C Hamano
In-Reply-To: <CAPig+cTjQe6FWo98LxvDS=s3dOs33SUUJa=x-bkyWHNBMx+XFw@mail.gmail.com>
On Wed, Oct 25, 2023 at 10:22 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Wed, Oct 25, 2023 at 11:44 AM Taylor Blau <me@ttaylorr.com> wrote:
> > On Wed, Oct 25, 2023 at 09:58:06AM +0200, Patrick Steinhardt wrote:
> > > On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:
> > > > In order to support streaming from a location in memory, we must
> > > > implement a new kind of bulk_checkin_source that does just that. These
> > > > implementation in spread out across:
> > >
>
> Perhaps:
>
> s/implementation in/implementations are/
Was just about to comment with the same suggestion; I had a hard time
parsing the commit message as well because of this.
^ permalink raw reply
* Re: [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Junio C Hamano @ 2023-11-11 0:14 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, git
In-Reply-To: <20231110214423.GC2758295@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Nov 10, 2023 at 11:01:15AM +0100, Patrick Steinhardt wrote:
>
>> diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
>> index 669ebaf68be..9fbf90cee7c 100755
>> --- a/t/valgrind/valgrind.sh
>> +++ b/t/valgrind/valgrind.sh
>> @@ -23,7 +23,7 @@ memcheck)
>> VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
>> VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
>> test 3 -gt "$VALGRIND_MAJOR" ||
>> - test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
>> + ( test 3 -eq "$VALGRIND_MAJOR" && test 4 -gt "$VALGRIND_MINOR" ) ||
>> TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
>
> I was surprised to see this one as a subshell after you adjusted the
> other.
;-)
I am not so surprised that this one was missed, though. I didn't
point this one out during my review of the previous round, either,
and not everybody is as careful as you are.
Thanks, both.
^ permalink raw reply
* Re: [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Junio C Hamano @ 2023-11-11 0:12 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King
In-Reply-To: <2967c8ebb460934eb4aaaaebe5941bff643d4a94.1699609940.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Our coding guidelines say to not use `test` with `-a` and `-o` because
> it can easily lead to bugs. Convert trivial cases where we still use
> these to instead instead concatenate multiple invocations of `test` via
> `&&` and `||`, respectively.
>
> While not all of the converted instances can cause ambiguity, it is
> worth getting rid of all of them regardless:
>
> - It becomes easier to reason about the code as we do not have to
> argue why one use of `-a`/`-o` is okay while another one isn't.
>
> - We don't encourage people to use these expressions.
Thanks for these additional notes. Nicely done.
^ permalink raw reply
* Re: [PATCH v4 0/3] t: improve compatibility with NixOS
From: Junio C Hamano @ 2023-11-11 0:10 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King
In-Reply-To: <cover.1699596457.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Three changes compared to v3:
>
> - Switched from `test <expr> -a <expr>` to `test <expr> && test
> <expr>`.
>
> - Improved the commit message to explain why the new
> runtime-detected paths are only used as a fallback.
>
> - Rebased on top of 0e3b67e2aa (ci: add support for GitLab CI,
> 2023-11-09), which has been merged to `next` and caused conflicts.
Please, no. "The other topic has been merged to 'next'" is *not* a
good reason to do this. Before that, the other topic was in 'seen'
and causing conflicts already, so getting into 'next' did not create
any new reason for rebasing.
I'll manage this time, but please do not do such a rebase unless you
are asked to do so. The rebase will force me to do (1) detach from
'next' and apply these, (2) discard the result and detach from the
base of where the previous iteration is queued, (3) apply the new
iteration with "am -3" to undo the rebase, before I can compare the
new iteration with the old iteration.
> Technically speaking this series also depends on 0763c3a2c4 (http:
> update curl http/2 info matching for curl 8.3.0, 2023-09-15), without
> which the tests will fail on NixOS machines with a recent libcurl.
Thanks for that note. This topic has been queued on top of
v2.43.0-rc1 which has 0763c3a2c4, so we'd be safe.
Will queue.
^ permalink raw reply
* tb/merge-tree-write-pack [Was: Re: What's cooking in git.git (Nov 2023, #04; Thu, 9)]
From: Elijah Newren @ 2023-11-11 0:00 UTC (permalink / raw)
To: Taylor Blau; +Cc: Junio C Hamano, Jeff King, Git Mailing List
In-Reply-To: <ZUzcmsfJe6jk4fTk@nand.local>
Hi,
On Thu, Nov 9, 2023 at 5:21 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Thu, Nov 09, 2023 at 02:40:28AM +0900, Junio C Hamano wrote:
> > * tb/merge-tree-write-pack (2023-10-23) 5 commits
> > - builtin/merge-tree.c: implement support for `--write-pack`
> > - bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
> > - bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
> > - bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
> > - bulk-checkin: extract abstract `bulk_checkin_source`
> >
> > "git merge-tree" learned "--write-pack" to record its result
> > without creating loose objects.
> >
> > Comments?
> > source: <cover.1698101088.git.me@ttaylorr.com>
>
[...]
> I am fairly confident that tb/merge-tree-write-pack is ready to go. I'll
> spin off a separate thread based on that branch and cc/git-replay as a
> non-RFC series that extends this approach to 'git replay', so we'll be
> ready to go there once Christian's series progresses.
Sorry for being so late to review. I posted some comments on patch 5
in relation to repeated file merges within a single (non-recursive)
merge, and in relation to recursive merges. Both are cases that'd be
really easy to miss if you're not familiar with the merge code, but I
think will cause the code to die with fatal errors. I provided my
guesses at some potential fixes for both issues.
^ permalink raw reply
* Re: [PATCH v4 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Junio C Hamano @ 2023-11-11 0:00 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King
In-Reply-To: <41b9dada2e0b2e713328e6a4d31f713a2d3ffa38.1699596457.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> +if test -x "$DEFAULT_HTTPD_PATH"
> +then
> + DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
> +fi
With this patch, my test run starts like so:
rm -f -r 'test-results'
*** prove ***
apache2: Could not open configuration file /etc/apache2/apache2.conf: No such file or directory
...
I find the error message leaking mildly annoying, and would suggest
doing something like the following on top.
diff --git c/t/lib-httpd.sh w/t/lib-httpd.sh
index 0a74922d7f..03493ee72b 100644
--- c/t/lib-httpd.sh
+++ w/t/lib-httpd.sh
@@ -68,7 +68,7 @@ done
if test -x "$DEFAULT_HTTPD_PATH"
then
- DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
+ DETECTED_HTTPD_ROOT="$("$DEFAULT_HTTPD_PATH" -V 2>/dev/null | sed -n 's/^ -D HTTPD_ROOT="\(.*\)"$/\1/p')"
fi
for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
^ permalink raw reply related
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Elijah Newren @ 2023-11-10 23:51 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Eric W. Biederman, Jeff King, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <3595db76a525fcebc3c896e231246704b044310c.1698101088.git.me@ttaylorr.com>
Hi,
Sorry for taking so long to find some time to review. And sorry for
the bad news, but...
On Mon, Oct 23, 2023 at 3:45 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> When using merge-tree often within a repository[^1], it is possible to
> generate a relatively large number of loose objects, which can result in
> degraded performance, and inode exhaustion in extreme cases.
>
> Building on the functionality introduced in previous commits, the
> bulk-checkin machinery now has support to write arbitrary blob and tree
> objects which are small enough to be held in-core. We can use this to
> write any blob/tree objects generated by ORT into a separate pack
> instead of writing them out individually as loose.
>
> This functionality is gated behind a new `--write-pack` option to
> `merge-tree` that works with the (non-deprecated) `--write-tree` mode.
>
> The implementation is relatively straightforward. There are two spots
> within the ORT mechanism where we call `write_object_file()`, one for
> content differences within blobs, and another to assemble any new trees
> necessary to construct the merge. In each of those locations,
> conditionally replace calls to `write_object_file()` with
> `index_blob_bulk_checkin_incore()` or `index_tree_bulk_checkin_incore()`
> depending on which kind of object we are writing.
>
> The only remaining task is to begin and end the transaction necessary to
> initialize the bulk-checkin machinery, and move any new pack(s) it
> created into the main object store.
I believe the above is built on an assumption that any objects written
will not need to be read until after the merge is completed. And we
might have a nesting issue too...
> @@ -2108,10 +2109,19 @@ static int handle_content_merge(struct merge_options *opt,
> if ((merge_status < 0) || !result_buf.ptr)
> ret = error(_("failed to execute internal merge"));
>
> - if (!ret &&
> - write_object_file(result_buf.ptr, result_buf.size,
> - OBJ_BLOB, &result->oid))
> - ret = error(_("unable to add %s to database"), path);
> + if (!ret) {
> + ret = opt->write_pack
> + ? index_blob_bulk_checkin_incore(&result->oid,
> + result_buf.ptr,
> + result_buf.size,
> + path, 1)
> + : write_object_file(result_buf.ptr,
> + result_buf.size,
> + OBJ_BLOB, &result->oid);
> + if (ret)
> + ret = error(_("unable to add %s to database"),
> + path);
> + }
>
> free(result_buf.ptr);
> if (ret)
This is unsafe; the object may need to be read later within the same
merge. Perhaps the simplest example related to your testcase is
modifying the middle section of that testcase (I'll highlight where
when I comment on the testcase) to read:
test_commit -C repo base file "$(test_seq 3 5)" &&
git -C repo branch -M main &&
git -C repo checkout -b side main &&
test_commit -C repo side-file file "$(test_seq 1 5)" &&
test_commit -C repo side-file2 file2 "$(test_seq 1 6)" &&
git -C repo checkout -b other main &&
test_commit -C repo other-file file "$(test_seq 3 7)" &&
git -C repo mv file file2 &&
git -C repo commit -m other-file2 &&
find repo/$packdir -type f -name "pack-*.idx" >packs.before &&
git -C repo merge-tree --write-pack side other &&
In words, what I'm doing here is having both sides modify "file" (the
same as you did) but also having one side rename "file"->"file2". The
side that doesn't rename "file" also introduces a new "file2". ort
needs to merge the three versions of "file" to get a new blob object,
and then merge that with the content from the brand new "file2". More
complicated cases are possible, of course. Anyway, with the modified
testcase above, merge-tree will fail with:
fatal: unable to read blob object 06e567b11dfdafeaf7d3edcc89864149383aeab6
I think (untested) that you could fix this by creating two packs
instead of just one. In particular, add a call to
flush_odb_transcation() after the "redo_after_renames" block in
merge_ort_nonrecursive_internal(). (It's probably easier to see that
you could place the flush_odb_transaction() call inside
detect_and_process_renames() just after the process_renames() call,
but when redo_after_renames is true you'd end up with three packs
instead of just two).
What happens with the odb transaction stuff if no new objects are
written before the call to flush_odb_transaction? Will that be a
problem?
(Since any tree written will not be re-read within the same merge, the
other write_object_file() call you changed does not have the same
problem as the one here.)
>@@ -4332,9 +4349,13 @@ static int process_entries(struct merge_options *opt,
> fflush(stdout);
> BUG("dir_metadata accounting completely off; shouldn't happen");
> }
>- if (write_tree(result_oid, &dir_metadata.versions, 0,
>+ if (write_tree(opt, result_oid, &dir_metadata.versions, 0,
> opt->repo->hash_algo->rawsz) < 0)
> ret = -1;
>
> +
> + if (opt->write_pack)
> + end_odb_transaction();
> +
Is the end_odb_transaction() here going to fail with an "Unbalanced
ODB transaction nesting" when faced with a recursive merge?
Perhaps flushing here, and then calling end_odb_transaction() in
merge_finalize(), much as you do in your replay-and-write-pack series,
should actually be moved to this commit and included here?
This does mean that for a recursive merge, that you'll get up to 2*N
packfiles, where N is the depth of the recursive merge.
> + test_commit -C repo base file "$(test_seq 3 5)" &&
> + git -C repo branch -M main &&
> + git -C repo checkout -b side main &&
> + test_commit -C repo side file "$(test_seq 1 5)" &&
> + git -C repo checkout -b other main &&
> + test_commit -C repo other file "$(test_seq 3 7)" &&
> +
> + find repo/$packdir -type f -name "pack-*.idx" >packs.before &&
> + tree="$(git -C repo merge-tree --write-pack \
> + refs/tags/side refs/tags/other)" &&
These were the lines from your testcase that I replaced to show the bug.
^ permalink raw reply
* Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Junio C Hamano @ 2023-11-10 23:39 UTC (permalink / raw)
To: Jeff King; +Cc: Taylor Blau, git
In-Reply-To: <20231110220142.GH2758295@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I think dropping those is a bad direction. The point of adding
> pair_chunk_expect() is that we could use it consistently.
I think so, too. If we are adding a helper to avoid common
mistakes, and if it cannot be used in some situations, then the
helper needs to be improved.
^ permalink raw reply
* Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Junio C Hamano @ 2023-11-10 23:38 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King
In-Reply-To: <ZU5Z/Z4PcdNP5U1r@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> But I don't think we enjoy the same benefits in the MIDX scenario. In
> this case, the num_objects field is just:
>
> m->num_objects = ntohl(m->chunk_oid_fanout[255])
>
> so I don't think we can make the same guarantees about what is and isn't
> save to compute under size_t arithmetic.
So ..., from the correctness's point of view, if we do not mind
st_mult() dying, the "multiply-and-compare" should give us much more
robust results. If we do mind st_mult() dying, we could pair the
"truncating-division-and-compare" you wrote with "ensure that chunk
size is a multiple of record size" to achieve the same, I would
think. I.e.,
if (chunk_size % pcd->record_size ||
chunk_size / pcd->record_size != pcd->record_nr)
return -1;
or something like that.
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Junio C Hamano @ 2023-11-10 23:31 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: Jeff King, git
In-Reply-To: <ZU4widBlHljjg9lL@debian>
Alejandro Colomar <alx@kernel.org> writes:
> [sendemail]
> sendmailcmd = mutt -H - && true
Ah, this is fun, an ugly but serviceable trick to ignore the command
line arguments. I briefly wondered it would work equally well and
much more readable to simply append "#", but the above should work
just fine.
^ permalink raw reply
* Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
From: Junio C Hamano @ 2023-11-10 23:27 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jeff King, git
In-Reply-To: <ZU4DiTQbKyuuT55k@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Nov 10, 2023 at 08:02:32AM +0900, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>>
>> >> # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
>> >> process_subtree_split_trailer () {
>> >> - assert test $# = 2 -o $# = 3
>> >> + assert test $# -ge 2
>> >> + assert test $# -le 3
>> > ...
>> >> - elif test $# -eq 1 -o $# -eq 2
>> >> + elif test $# -eq 1 || test $# -eq 2
>> >
>> > OK, this one is a straight-forward use of "||".
>>
>> Yes, but why not consistently use the range notation like the
>> earlier one here, or below?
>
> I opted to go for the "obvious" conversion, if there was one easily
> available, to make the diff easier to read.
... and due to the limitation of "assert" we cannot do the obvious
test $# = 2 || test $# = 3
and feed it to "assert" (and for equality with $#, = and -eq would
work equally fine, and = is much more readable, by the way).
OK, then.
^ permalink raw reply
* Re: first-class conflicts?
From: Elijah Newren @ 2023-11-10 22:57 UTC (permalink / raw)
To: phillip.wood; +Cc: Sandra Snan, git, Martin von Zweigbergk, Randall S. Becker
In-Reply-To: <a1cd2dba-3a74-4b41-8585-209b4a13b8c4@gmail.com>
Hi Phillip,
On Thu, Nov 9, 2023 at 6:45 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
[...]
> > This is a great thing to think about and bring up. However, I'm not
> > sure what part of it actually needs to be preserved; in fact, it's not
> > clear to me that any of it needs preserving -- especially not the
> > files read by "git commit". A commit was already created, after all.
> >
> > It seems that CHERRY_PICK_HEAD/REVERT_HEAD files exist primarily to
> > clue in that we are in-the-middle-of-<op>, and the conflict header
> > (the "tree A + tree B - tree C" thing; whatever that's called)
> > similarly provides signal that the commit still has conflicts.
> > Secondarily, these files contain information about the tree we came
> > from and its parent tree, which allows users to investigate the diff
> > between those...but that information is also available from the
> > conflict header in the recorded commit. The CHERRY_PICK_HEAD and
> > REVERT_HEAD files could also be used to access the commit message, but
> > that would have been stored in the conflicted commit as well. Are
> > there any other pieces of information I'm missing?
>
> Mainly that I'm an idiot and forgot we were actually creating a commit
> and can store the message and authorship there!
You're definitely not an idiot. The whole problem space is new and
different, so it's easy to overlook or forget certain details, and
even to make completely different assumptions than others and have no
one aware that we're operating with similar sounding but entirely
different mental models.
> More seriously I think
> being able to inspect the commit being cherry-picked (including the
> original commit message) is useful so we'd need to recreate something
> like CHERRY_PICK_HEAD when the conflict commit is checked out.
So, I see a few issues with this:
1) Even if we were to create CHERRY_PICK_HEAD as you envision, that
doesn't necessarily guarantee the user can view the original commit
because they may not have it. It may have been a local-only commit
that wasn't pushed or pulled to the person who is now investigating
it.
2a) You highlight the original commit message, but if someone doesn't
want to immediately resolve conflicts, why would they be modifying the
commit message?
2b) Even if users did want to modify the commit message without
resolving conflicts, how would they do so? Rebasing has
interactivity, but cherry-picking doesn't. And interactivity seems to
be something people probably wouldn't use together with storing
conflicts; the point of interactivity is to tweak things further and
fix them up, suggesting they'd want to be running in
address-conflicts-now mode.
> Recreating CHERRY_PICK_HEAD is useful for "git status" as well.
"git status" uses this file to determine if it should display
information about currently being in the middle of a cherry-pick
operation. Putting such a file in place would thus be misleading,
because we aren't in a cherry-pick operation anymore; that has
completed already. I would not expect the suggested commands printed
by git-status while it thinks we're in such a state (namely, "git
cherry-pick [--continue|--skip|--abort]") to work either. So, I'd
argue it would be a bug to create such a file when checking out a
conflicted-commit.
Of course, we would want git-status to display information about the
current commit being conflicted, but I think that could be based on
the simple conflict header without additional info.
> I think
> that means storing a little more that just the "tree A + tree B - tree
> C" thing.
I'm totally willing to believe there will be cases where more info is
needed. I'm suspecting that conflicts with certain kinds of renames,
or which were performed with certain types of strategies or strategy
options might be some examples. But I'm not sure I'm understanding
why CHERRY_PICK_HEAD should be one of those cases.
> > I think the big piece here is whether we also want to adopt jj's
> > behavior of automatically rebasing all descendant commits when
> > checking out and amending some historical commit (or at least having
> > the option of doing so). That behavior allows users to amend commits
> > to resolve conflicts without figuring out complicated interactive
> > rebases to fix all the descendant commits across all relevant
> > branches.
>
> That's a potentially attractive option which is fairly simple to
> implement locally as I think you can use the commit DAG to find all the
> descendants though that could be expensive if there are lots of
> branches. However, if we're going to share conflicts I think we'd need
> something like "hg evolve" - if I push a commit with conflicts and you
> base some work on it and then I resolve the conflict and push again you
> would want to your work to be rebased onto my conflict resolution.
Ooh, that's an interesting point.
> To handle "rebase --exec" we could store the exec command and run it when
> the conflicts are resolved.
So, my assumption is that even if we add the ability to commit
conflicts and even if we default to auto-committing them during
cherry-picks or non-interactive rebases, there will still be people
who want to resolve conflicts as they are hit rather than
auto-committing them, and thus that stop-on-conflict should always be
an option. In the world where a user has this choice, I think it'd be
rare for users to want to auto-commit conflicts with --exec. I'd
suggest that --exec, and even --interactive, would default to stopping
on conflicts and waiting for the user to resolve even if
auto-commit-on-conflict is the default in other cases.
That leaves me wondering if there are any cases where users want to
auto-commit conflicts in.conjunction with --exec, which I'm already
struggling to come up with, _and_ that would further want the exec
commands to be preserved in the conflicted commits (and any descendant
commits?) for later usage. Maybe there's a case for that, but I'm not
coming up with it right now.
Also, another way of looking at this is that my current mental model
is that the cherry-pick or rebase operation is completed once it has
handled each of the commits in its list; the operation does not extend
until all the conflicts in the commits it creates are resolved. The
fact that rebases do not extend until conflicts are resolved is
important because you can later further rebase conflicted-commits (as
Martin alludes to in his emails); considering the old rebase(s) to
still be in progress while a new one starts might get excessively
complex to handle. The reason all of this matters to --exec is that
--exec is part of the rebase operation; once the rebase operation is
done, the --exec stuff is also done. (And thus, if you don't want
--exec to run on conflicted commits, then don't opt for
auto-committing conflicts.).
> Also I wonder how annoying it would be in cases where I just want to
> rebase and resolve the conflicts now. At the moment "git rebase" stops
> at the conflict, with this feature I'd have to go and checkout the
> conflicted commit and fix the conflicts after the rebase had finished.
I agree that would often be annoying. Personally, I think that
auto-committing conflicts as a feature should at most be an option
(even if perhaps the default in some cases), not a new mandatory
worldview. And I'm currently not convinced that even if it were
implemented it should be the default in any cases.
> > Without that feature, I agree this might be a bit more
> > difficult,
>
> Yes, when I wrote my original message I was imagining that we'd stop at
> the first conflicting pick and store all the rebase state like some kind
> of stash on steroids so it could be continued when the conflict was
> resolved. It would be much simpler to try and avoid that.
Yeah, this is an example of how completely different mental models we
can come up with when none of us (other than Martin) know much about
the problem space. I suspect there's at least a few more examples
like this where we still have very different mental models, and
perhaps some gems to be found by mixing and matching them.
> > There are some special state files related to half-completed
> > operations (e.g. squash commits when we haven't yet reached the final
> > one in the sequence, a file to note that we want to edit a commit
> > message once the user has finished resolving conflicts, whether we
> > need to create a new root commit), but again, the operation has
> > completed and commits have been created with appropriate parentage and
> > commit messages so I don't think these are useful anymore either.
>
> Yes, though we may want to remember which commits were squashed together
> so the user can inspect that when resolving conflicts.
Ooh, that's interesting...though it does run into the problem of users
not having access to the original commits.
> > The biggest issue is perhaps that REBASE_HEAD is used in the
> > implementation of `git rebase --show-current-patch`, but all
> > information stored in that is still accessible -- the commit message
> > is stored in the commit, the author time is stored in the commit, and
> > the trees involved are in the conflict header. The only thing missing
> > is committer timestamp, which isn't relevant anyway.
>
> The commit message may have been edited so we lose the original message
> but I'm not sure how important that is.
Is this a reversal from your comment earlier in your email about the
importance of the original commit message for CHERRY_PICK_HEAD? :-)
> > The only ones I'm pausing a bit on are the strategy and
> > strategy-options. Those might be useful somehow...but I can't
> > currently quite put my finger on explaining how they would be useful
> > and I'm not sure they are.
>
> I can't think of an immediate use for them. When we re-create conflicts
> we do it per-file based on the index entries created by the original
> merge so I don't think we need to know anything about the strategy or
> strategy-options.
But we don't have index entries. We only have trees in this
conflicted commit, and when users check it out, they probably expect
conflicted index entries to be put into place. Can we correctly
regenerate the right conflicted index entries from the original trees
without the strategy and strategy-options command line flags? I
suspect there might be problems here, and user-defined merge
strategies could really throw a wrench in the works. Hmm...
> > Am I missing anything?
>
> exec commands? If the user runs "git rebase --exec" and there are
> conflicts then we'd need to defer running the exec commands until the
> conflicts are resolved. For something like "git rebase --exec 'make
> test'" that should be fine. I wonder if there are corner cases where the
> exec command changes HEAD though.
We talked about exec commands above, as well as the assumption whether
auto-committing conflicts should be mandatory vs. an option, so I
won't repeat that here. It was definitely a very interesting topic to
bring up though; thanks!
[...]
> Yes, it would certainly be lots of work.
...but even if none of us get time and prioritization to work on it, I
personally find it a really interesting topic to discuss and explore.
Thanks for joining in and bringing up many good points!
^ permalink raw reply
* Re: [PATCH 2/7] commit-graph: read `OIDL` chunk with `pair_chunk_expect()`
From: Jeff King @ 2023-11-10 22:10 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
In-Reply-To: <5b3c0b99f8052733bb714122582ab229556c94ef.1699569246.git.me@ttaylorr.com>
On Thu, Nov 09, 2023 at 05:34:14PM -0500, Taylor Blau wrote:
> @@ -435,8 +425,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
> error(_("commit-graph required OID fanout chunk missing or corrupted"));
> goto free_and_return;
> }
> - if (read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph)) {
> - error(_("commit-graph required OID lookup chunk missing or corrupted"));
> + if (pair_chunk_expect(cf, GRAPH_CHUNKID_OIDLOOKUP,
> + &graph->chunk_oid_lookup, graph->hash_len,
> + graph->num_commits)) {
> + error(_("commit-graph OID lookup chunk is the wrong size"));
> goto free_and_return;
I know the original message was vague, but I think the new one is
actively misleading in the case of a missing chunk. We'll say "wrong
size", but it was not present at all!
-Peff
^ permalink raw reply
* Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Jeff King @ 2023-11-10 22:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git
In-Reply-To: <20231110215747.GG2758295@coredump.intra.peff.net>
On Fri, Nov 10, 2023 at 04:57:47PM -0500, Jeff King wrote:
> One of those patches calls out the truncating division issue, but to
> summarize: IMHO this is OK, as what we really want to know is "is it big
> enough that we can always ask for NR records of size ELEM", which
> division gives us. If we do want to be more precise, but also avoid
> die(), we'd need a variant of st_mult() that returns a boolean. I didn't
> think it was worth it for this case (but arguably it is something that
> would be useful to have in general).
Oh, and obviously there is another option here if we want to be more
careful but don't want to introduce an st_mult() variant: we can use "%"
to check for divisibility ourselves.
I don't think it's worth doing that in every individual size-check, but
maybe it would be in a central pair_chunk_expect().
-Peff
^ permalink raw reply
* Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Jeff King @ 2023-11-10 22:08 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
In-Reply-To: <af5fe3b7237caeba8f970e967933db96c83a230e.1699569246.git.me@ttaylorr.com>
On Thu, Nov 09, 2023 at 05:34:11PM -0500, Taylor Blau wrote:
> +static int pair_chunk_expect_fn(const unsigned char *chunk_start,
> + size_t chunk_size,
> + void *data)
> +{
> + struct pair_chunk_data *pcd = data;
> + if (chunk_size / pcd->record_size != pcd->record_nr)
> + return -1;
> + *pcd->p = chunk_start;
> + return 0;
> +}
I think this is taking us backwards in terms of the output the user sees
(see the message I just sent elsewhere in the thread). The user won't be
able to tell the difference between a missing chunk and one with the
wrong size.
And we miss the opportunity to give more details about the expected and
detected sizes of the chunks.
If the two-line error message really bothers you, then...
> +int pair_chunk_expect(struct chunkfile *cf,
> + uint32_t chunk_id,
> + const unsigned char **p,
> + size_t record_size,
> + size_t record_nr)
> +{
> + struct pair_chunk_data pcd = {
> + .p = p,
> + .record_size = record_size,
> + .record_nr = record_nr,
> + };
> + return read_chunk(cf, chunk_id, pair_chunk_expect_fn, &pcd);
> +}
...this is where we could take a CHUNK_REQUIRED flag, and so:
ret = read_chunk(...);
/* pair_chunk_expect_fn() wrote an error already for other cases */
if (ret == CHUNK_MISSING)
error("chunk not found");
return ret;
And then the callers become a very nice:
if (pair_chunk_expect(cf, id, &ptr, size, nr, CHUNK_REQUIRED))
return -1;
You probably would need to pass some kind of string giving more context
for the error messages, though. We can show the chunk id, but the
context of "commit-graph" vs "midx" is important.
-Peff
^ permalink raw reply
* Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Jeff King @ 2023-11-10 22:01 UTC (permalink / raw)
To: Taylor Blau; +Cc: Junio C Hamano, git
In-Reply-To: <ZU5Z/Z4PcdNP5U1r@nand.local>
On Fri, Nov 10, 2023 at 11:27:41AM -0500, Taylor Blau wrote:
> Hmm. I was thinking of Peff's "commit-graph: handle overflow in
> chunk_size checks", but I think that I was overly eager in applying the
> same reasoning to the MIDX code.
>
> The important piece of the rationale in that patch is as follows:
>
> In the current code this is only possible for the CDAT chunk, but
> the reasons are quite subtle. We compute g->num_commits by dividing
> the size of the OIDL chunk by the hash length (since it consists of
> a bunch of hashes). So we know that any size_t multiplication that
> uses a value smaller than the hash length cannot overflow. And the
> CDAT records are the only ones that are larger (the others are just
> 4-byte records). So it's worth fixing all of these, to make it clear
> that they're not subject to overflow (without having to reason about
> seemingly unrelated code).
>
> In particular, that g->num_commits is computed by dividing the length of
> the OIDL chunk by the hash length, thus any size_t multiplication of
> g->num_commits with a value smaller than that hash length cannot
> overflow.
>
> But I don't think we enjoy the same benefits in the MIDX scenario. In
> this case, the num_objects field is just:
>
> m->num_objects = ntohl(m->chunk_oid_fanout[255])
>
> so I don't think we can make the same guarantees about what is and isn't
> save to compute under size_t arithmetic.
Yes, the logic does not apply to the midx code (just like the graph code
after we switch to using the fanout value later in my series). But that
paragraph was just explaining why only the CDAT chunk was _currently_
vulnerable.
The more interesting question is the paragraphs after that, about
whether it is OK to die() or not when we see overflow (and IMHO it is
not for commit-graphs).
The situation _is_ different there for midx's, because we immediately
die() if we see a bogus midx anyway. But I think that is wrong, and
something we may want to change in the long run. Both because it's the
wrong thing for lib-ification, but also because we can easily keep going
if the midx is broken; we can still use the individual pack idx files.
> I'd be curious what Peff has to say, but if he agrees with me, I'd
> recommend taking the first five patches, and dropping the two
> MIDX-related ones.
I think dropping those is a bad direction. The point of adding
pair_chunk_expect() is that we could use it consistently.
-Peff
^ permalink raw reply
* Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Jeff King @ 2023-11-10 21:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git
In-Reply-To: <xmqqedgyw6jv.fsf@gitster.g>
On Fri, Nov 10, 2023 at 01:55:48PM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > +static int pair_chunk_expect_fn(const unsigned char *chunk_start,
> > + size_t chunk_size,
> > + void *data)
> > +{
> > + struct pair_chunk_data *pcd = data;
> > + if (chunk_size / pcd->record_size != pcd->record_nr)
> > + return -1;
> > + *pcd->p = chunk_start;
> > + return 0;
> > +}
>
> I know one of the original places did the "divide the whole by
> per-record size and see if it matches the number of records", the
> same as we see above, but the check in the above could also be
>
> if (chunk_size != st_mult(pcd->record_size, pcd->record_nr))
> return -1;
>
> which would also catch the case where chunk_size is not a multiple
> of the record size. Your conversion of OOFF in midx.c loses this
> protection as the original uses the multiplication-and-compare, but
> the rewrite to call pair_chunk_expect would call the above and
> checks with the truncating-divide-and-compare.
>
> Does the distinction matter? I dunno. If the record/chunk
> alignment is asserted elsewhere, then the distinction should not
> matter, but even if it were, seeing a truncating division used in
> any validation makes my skin tingle.
Yes, the distinction does matter. If pair_chunk_expect_fn() used
st_mult(), then it would die on overflow, rather than returning an
error. For commit-graph files this is propagated up, and we continue the
operation by falling back to the non-graph code. There's a test in t5318
that will fail in this case. See patches 1 and 6 in my series for more
discussion.
One of those patches calls out the truncating division issue, but to
summarize: IMHO this is OK, as what we really want to know is "is it big
enough that we can always ask for NR records of size ELEM", which
division gives us. If we do want to be more precise, but also avoid
die(), we'd need a variant of st_mult() that returns a boolean. I didn't
think it was worth it for this case (but arguably it is something that
would be useful to have in general).
-Peff
^ permalink raw reply
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