tmp-objdir: fix regressions in core.fsyncobjectfiles=batch by neerajsi-msft · Pull Request #1067 · gitgitgadget/git

neerajsi-msft · 2021-10-26T21:20:15Z

Fix prune code to be able to work against multiple cruft directories. I noticed this in self-review.
When dscho enabled core.fsyncobjectfiles=batch in git-for-windows we saw some test-failures in
update-index tests. The root cause is that setup_work_tree does a chdir_notify, which erases the
tmp-objdir state. I now unapply and reapply the tmp-objdir around setup_git_env.

This branch autosquashes cleanly and it needs to be merged with ns/batched-fsync, where
it currently merges cleanly.

CC:[email protected]
cc: Neeraj Singh [email protected]

The tmp_objdir API provides the ability to create temporary object directories, but was designed with the goal of having subprocesses access these object stores, followed by the main process migrating objects from it to the main object store or just deleting it. The subprocesses would view it as their primary datastore and write to it. Here we add the tmp_objdir_replace_primary_odb function that replaces the current process's writable "main" object directory with the specified one. The previous main object directory is restored in either tmp_objdir_migrate or tmp_objdir_destroy. For the --remerge-diff usecase, add a new `will_destroy` flag in `struct object_database` to mark ephemeral object databases that do not require fsync durability. Add 'git prune' support for removing temporary object databases, and make sure that they have a name starting with tmp_ and containing an operation-specific name. Based-on-patch-by: Elijah Newren <[email protected]> Signed-off-by: Neeraj Singh <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>

When creating a subprocess with a temporary ODB, we set the GIT_QUARANTINE_ENVIRONMENT env var to tell child Git processes not to update refs, since the tmp-objdir may go away. Introduce a similar mechanism for in-process temporary ODBs when we call tmp_objdir_replace_primary_odb. Now both mechanisms set the disable_ref_updates flag on the odb, which is queried by the ref_transaction_prepare function. Note: This change adds an assumption that the state of the_repository is relevant for any ref transaction that might be initiated. Unwinding this assumption should be straightforward by saving the relevant repository to query in the transaction or the ref_store. Peff's test case was invoking ref updates via the cachetextconv setting. That particular code silently does nothing when a ref update is forbidden. See the call to notes_cache_put in fill_textconv where errors are ignored. Reported-by: Jeff King <[email protected]> Signed-off-by: Neeraj Singh <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>

gitgitgadget · 2021-10-26T21:20:55Z

There is an issue in commit 0c86c99:
Commit not signed off

gitgitgadget · 2021-10-26T21:31:09Z

There is an issue in commit 0c86c99:
Commit not signed off

Fix prune code to be able to delete multiple object directories. I wasn't properly resetting the strbuf with the path. Signed-off-by: Neeraj Singh <[email protected]>

When setup_work_tree executes, it redoes setup of the object database path and various other aspects of the_repository. This destroys the temporary object database state. This commit removes the temporary object database and reapplies it around the operations in the chdir_notify callback. Signed-off-by: Neeraj Singh <[email protected]>

neerajsi-msft · 2021-10-26T22:34:52Z

/submit

gitgitgadget · 2021-10-26T22:35:38Z

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1067/neerajsi-msft/ns/tmp-objdir-fixes-v1

To fetch this version to local tag pr-1067/neerajsi-msft/ns/tmp-objdir-fixes-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1067/neerajsi-msft/ns/tmp-objdir-fixes-v1

gitgitgadget · 2021-10-27T12:48:20Z

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Neeraj,

On Tue, 26 Oct 2021, Neeraj K. Singh via GitGitGadget wrote:

>  * Fix prune code to be able to work against multiple cruft directories. I
>    noticed this in self-review.
>
>  * When dscho enabled core.fsyncobjectfiles=batch in git-for-windows we saw
>    some test-failures in update-index tests. The root cause is that
>    setup_work_tree does a chdir_notify, which erases the tmp-objdir state. I
>    now unapply and reapply the tmp-objdir around setup_git_env.
>
> This branch autosquashes cleanly and it needs to be merged with
> ns/batched-fsync, where it currently merges cleanly.
>
> Neeraj Singh (2):
>   fixup! tmp-objdir: new API for creating temporary writable databases
>   fixup! tmp-objdir: new API for creating temporary writable databases

Thank you for the fast work on the fixes!

I applied both patches to the PR branch and pushed; Let's see how the CI
over at https://github.com/git-for-windows/git/pull/3492 pans out.

Please note the original patch made it into `next` already (and is hence
subject to follow-up patches rather than being rewritten).

Therefore, you may need to reword the commit messages so that they stand
on their own, as follow-up commits.

And alternative would be to ask Junio to kick the topic out of `next` and
back to `seen`, in which case you will probably be asked to submit a new
iteration of the original patch.

Thank you again!
Dscho

>
>  builtin/prune.c |  1 +
>  environment.c   |  5 +++++
>  tmp-objdir.c    | 25 +++++++++++++++++++++++++
>  tmp-objdir.h    | 15 +++++++++++++++
>  4 files changed, 46 insertions(+)
>
>
> base-commit: 50741b157f2f90df76a60418e2781b2c1e6e3c78
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1067%2Fneerajsi-msft%2Fns%2Ftmp-objdir-fixes-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1067/neerajsi-msft/ns/tmp-objdir-fixes-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1067
> --
> gitgitgadget
>
>

gitgitgadget · 2021-10-27T21:13:08Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <[email protected]> writes:

>> Neeraj Singh (2):
>>   fixup! tmp-objdir: new API for creating temporary writable databases
>>   fixup! tmp-objdir: new API for creating temporary writable databases
>
> Thank you for the fast work on the fixes!
>
> I applied both patches to the PR branch and pushed; Let's see how the CI
> over at https://github.com/git-for-windows/git/pull/3492 pans out.
>
> Please note the original patch made it into `next` already (and is hence
> subject to follow-up patches rather than being rewritten).
>
> Therefore, you may need to reword the commit messages so that they stand
> on their own, as follow-up commits.
>
> And alternative would be to ask Junio to kick the topic out of `next` and
> back to `seen`, in which case you will probably be asked to submit a new
> iteration of the original patch.

Yeah, none of the above is attractive this late in the cycle X-<.

It probalby is best to queue the "fixup!" commits as they are on top
of ns/tmp-objdir, merge the result to two topics that depend on
ns/tmp-objdir, and keep them without merging them down, until the
release.  When it is time to rewind 'next' after the release, it
would be a good chance to get rid of these "oops, earlier we screwed
up" commits by redoing the tmp-objdir (and rebasing the other two
topics on top).

gitgitgadget · 2021-10-27T23:46:41Z

On the Git mailing list, Neeraj Singh wrote (reply to this):

On Wed, Oct 27, 2021 at 02:09:21PM -0700, Junio C Hamano wrote:
> Yeah, none of the above is attractive this late in the cycle X-<.
> 
> It probalby is best to queue the "fixup!" commits as they are on top
> of ns/tmp-objdir, merge the result to two topics that depend on
> ns/tmp-objdir, and keep them without merging them down, until the
> release.  When it is time to rewind 'next' after the release, it
> would be a good chance to get rid of these "oops, earlier we screwed
> up" commits by redoing the tmp-objdir (and rebasing the other two
> topics on top).
> 

Hi Junio,
Apologies for the breakage! I just want to be 100% clear here: is there
any action I should take with the patches, or will you handle the merge/rebase?

FYI for anyone trying this on git-for-windows, there's one additional patch at:
https://github.com/neerajsi-msft/git/commit/435e1d2e5e8fb422b0f08ff6a01a130584f7e249

That fixes a gfw-specific breakage that affects tmp_objdir_migrate and causes it to
infinitely create recursive directories until the disk fills up (surprisingly we don't
hit stack overflow first).

Thanks,
Neeraj

gitgitgadget · 2021-10-27T23:46:44Z

User Neeraj Singh <[email protected]> has been added to the cc: list.

gitgitgadget · 2021-10-28T00:34:45Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Neeraj Singh <[email protected]> writes:

> On Wed, Oct 27, 2021 at 02:09:21PM -0700, Junio C Hamano wrote:
>> Yeah, none of the above is attractive this late in the cycle X-<.
>> 
>> It probalby is best to queue the "fixup!" commits as they are on top
>> of ns/tmp-objdir, merge the result to two topics that depend on
>> ns/tmp-objdir, and keep them without merging them down, until the
>> release.  When it is time to rewind 'next' after the release, it
>> would be a good chance to get rid of these "oops, earlier we screwed
>> up" commits by redoing the tmp-objdir (and rebasing the other two
>> topics on top).
>> 
>
> Hi Junio,
> Apologies for the breakage! I just want to be 100% clear here: is there
> any action I should take with the patches, or will you handle the merge/rebase?

If we all agree on the above plan, then nothing for you for now, but
we'd ask you to send a cleaned-up patch after the upcoming release
when the 'next' branch gets rewound and rebuilt, at which time we
can get rid of the "oops, we screwed up" fixup patches.

Thanks for finding and sending in the fix.

gitgitgadget · 2021-10-28T05:31:10Z

This branch is now known as ns/tmp-objdir.

gitgitgadget · 2021-10-28T05:31:11Z

This patch series was integrated into seen via git@6745aa6.

gitgitgadget · 2021-10-28T05:31:12Z

This patch series was integrated into next via git@001a18c.

gitgitgadget · 2021-10-28T06:20:29Z

This patch series was integrated into seen via git@af7cb07.

dscho · 2021-10-28T08:36:31Z

This patch series was integrated into next via git@001a18c.

That's good news, as the breakage is fixed in next. When Git v2.34 comes around, and next is rewound, it is probably a good time to squash this into git#1076 and submit a new iteration of that PR.

gitgitgadget · 2021-10-29T07:39:26Z

This patch series was integrated into seen via git@10b875b.

gitgitgadget · 2021-10-29T22:40:08Z

This patch series was integrated into seen via git@a16745a.

gitgitgadget · 2021-10-29T23:37:17Z

This patch series was integrated into seen via git@74afafe.

gitgitgadget · 2021-10-30T00:20:51Z

There was a status update in the "Cooking" section about the branch ns/tmp-objdir on the Git mailing list:

New interface into the tmp-objdir API to help in-core use of the
quarantine feature.

On hold.
cf. <[email protected]>

gitgitgadget · 2021-11-02T00:22:10Z

This patch series was integrated into seen via git@e2b96b9.

gitgitgadget · 2021-11-02T06:12:12Z

There was a status update in the "Cooking" section about the branch ns/tmp-objdir on the Git mailing list:

New interface into the tmp-objdir API to help in-core use of the
quarantine feature.

On hold.
cf. <[email protected]>

gitgitgadget · 2021-11-03T18:54:28Z

This patch series was integrated into seen via git@671cab4.

gitgitgadget · 2021-11-10T22:33:10Z

This patch series was integrated into seen via git@62a9499.

gitgitgadget · 2021-11-11T00:19:49Z

This patch series was integrated into seen via git@5a6656e.

gitgitgadget · 2021-11-11T22:59:19Z

This patch series was integrated into seen via git@739ce11.

gitgitgadget · 2021-11-12T22:32:53Z

This patch series was integrated into seen via git@5375511.

gitgitgadget · 2021-11-12T23:54:03Z

This patch series was integrated into seen via git@69d5119.

gitgitgadget · 2021-11-14T08:03:20Z

This patch series was integrated into seen via git@6cfd30d.

gitgitgadget · 2021-11-15T17:05:18Z

This patch series was integrated into seen via git@69aafec.

gitgitgadget · 2021-11-15T17:44:54Z

There was a status update in the "Cooking" section about the branch ns/tmp-objdir on the Git mailing list:

New interface into the tmp-objdir API to help in-core use of the
quarantine feature.

On hold.
cf. <[email protected]>

gitgitgadget · 2021-11-20T05:57:58Z

This patch series was integrated into seen via git@41e1342.

gitgitgadget · 2021-11-20T07:20:52Z

There was a status update in the "Cooking" section about the branch ns/tmp-objdir on the Git mailing list:

New interface into the tmp-objdir API to help in-core use of the
quarantine feature.

On hold.
cf. <[email protected]>

gitgitgadget · 2021-11-22T02:34:24Z

This patch series was integrated into seen via git@1c958d7.

gitgitgadget · 2021-11-22T17:03:20Z

This patch series was integrated into seen via git@a3ff8db.

gitgitgadget · 2021-11-22T23:45:05Z

This patch series was integrated into seen via git@afed8ea.

gitgitgadget · 2021-11-23T01:32:01Z

This patch series was integrated into seen via git@45b9a03.

gitgitgadget · 2021-11-23T17:21:35Z

This patch series was integrated into seen via git@9c29f5e.

gitgitgadget · 2021-11-24T20:03:16Z

This patch series was integrated into seen via git@a3c0865.

gitgitgadget · 2021-11-25T06:02:33Z

There was a status update in the "Cooking" section about the branch ns/tmp-objdir on the Git mailing list:

New interface into the tmp-objdir API to help in-core use of the
quarantine feature.

On hold.
cf. <[email protected]>

gitgitgadget · 2021-11-30T00:15:56Z

This patch series was integrated into seen via git@1833f4c.

gitgitgadget · 2021-11-30T02:21:32Z

There was a status update in the "Cooking" section about the branch ns/tmp-objdir on the Git mailing list:

New interface into the tmp-objdir API to help in-core use of the
quarantine feature.

On hold.
cf. <[email protected]>

gitgitgadget · 2021-12-02T00:29:08Z

This patch series was integrated into seen via git@f8d1005.

gitgitgadget · 2021-12-02T22:00:56Z

This patch series was integrated into seen via git@1264f02.

gitgitgadget · 2021-12-03T23:50:05Z

This patch series was integrated into seen via git@1ee62e4.

gitgitgadget · 2021-12-04T01:41:06Z

There was a status update in the "Cooking" section about the branch ns/tmp-objdir on the Git mailing list:

New interface into the tmp-objdir API to help in-core use of the
quarantine feature.

Expecting a reroll.
cf. <CANQDOddCC7+gGUy1VBxxwvN7ieP+N8mQhbxK2xx6ySqZc6U7-g@mail.gmail.com>
cf. <CABPp-BHq4DnkCZZ+HcnGiZYKHpWJgzMBjMDuWbbTFYPesm2sXA@mail.gmail.com>
cf. <[email protected]>
cf. <ef5a087813b7dfd232a9366eee09774d197e2307.1635287730.git.gitgitgadge=
[email protected]>

gitgitgadget · 2022-01-04T03:49:45Z

There was a status update in the "Graduated to 'master'" section about the branch ns/tmp-objdir on the Git mailing list:

New interface into the tmp-objdir API to help in-core use of the
quarantine feature.
source: <[email protected]>

dscho · 2022-01-04T11:04:37Z

@neerajsi-msft I guess this can be closed now? (Sorry, GitGitGadget's automatic checks seem to have been defeated by this PR)

neerajsi-msft added 2 commits October 4, 2021 11:49

neerajsi-msft force-pushed the ns/tmp-objdir-fixes branch from 2d3b3b6 to 9e78488 Compare October 26, 2021 21:30

neerajsi-msft added 2 commits October 26, 2021 14:59

fixup! tmp-objdir: new API for creating temporary writable databases

d244cc4

Fix prune code to be able to delete multiple object directories. I wasn't properly resetting the strbuf with the path. Signed-off-by: Neeraj Singh <[email protected]>

neerajsi-msft force-pushed the ns/tmp-objdir-fixes branch from 9e78488 to ef5a087 Compare October 26, 2021 22:32

gitgitgadget Bot added next seen labels Oct 28, 2021

dscho mentioned this pull request Oct 28, 2021

Implement a batched fsync option for core.fsyncObjectFiles git/git#1076

Closed

dscho force-pushed the ns/tmp-objdir branch 4 times, most recently from 34f333b to ecd81df Compare December 9, 2021 01:42

neerajsi-msft closed this Jan 4, 2022

Uh oh!

Conversation

neerajsi-msft commented Oct 26, 2021 • edited by gitgitgadget Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitgitgadget Bot commented Oct 26, 2021

Uh oh!

gitgitgadget Bot commented Oct 26, 2021

Uh oh!

neerajsi-msft commented Oct 26, 2021

Uh oh!

gitgitgadget Bot commented Oct 26, 2021

Uh oh!

gitgitgadget Bot commented Oct 27, 2021

Uh oh!

gitgitgadget Bot commented Oct 27, 2021

Uh oh!

gitgitgadget Bot commented Oct 27, 2021

Uh oh!

gitgitgadget Bot commented Oct 27, 2021

Uh oh!

gitgitgadget Bot commented Oct 28, 2021

Uh oh!

gitgitgadget Bot commented Oct 28, 2021

Uh oh!

gitgitgadget Bot commented Oct 28, 2021

Uh oh!

gitgitgadget Bot commented Oct 28, 2021

Uh oh!

gitgitgadget Bot commented Oct 28, 2021

Uh oh!

dscho commented Oct 28, 2021

Uh oh!

gitgitgadget Bot commented Oct 29, 2021

Uh oh!

gitgitgadget Bot commented Oct 29, 2021

Uh oh!

gitgitgadget Bot commented Oct 29, 2021

Uh oh!

gitgitgadget Bot commented Oct 30, 2021

Uh oh!

gitgitgadget Bot commented Nov 2, 2021

Uh oh!

gitgitgadget Bot commented Nov 2, 2021

Uh oh!

gitgitgadget Bot commented Nov 3, 2021

Uh oh!

gitgitgadget Bot commented Nov 10, 2021

Uh oh!

gitgitgadget Bot commented Nov 11, 2021

Uh oh!

gitgitgadget Bot commented Nov 11, 2021

Uh oh!

gitgitgadget Bot commented Nov 12, 2021

Uh oh!

gitgitgadget Bot commented Nov 12, 2021

Uh oh!

gitgitgadget Bot commented Nov 14, 2021

Uh oh!

gitgitgadget Bot commented Nov 15, 2021

Uh oh!

gitgitgadget Bot commented Nov 15, 2021

Uh oh!

gitgitgadget Bot commented Nov 20, 2021

Uh oh!

gitgitgadget Bot commented Nov 20, 2021

Uh oh!

gitgitgadget Bot commented Nov 22, 2021

Uh oh!

gitgitgadget Bot commented Nov 22, 2021

Uh oh!

gitgitgadget Bot commented Nov 22, 2021

Uh oh!

gitgitgadget Bot commented Nov 23, 2021

Uh oh!

gitgitgadget Bot commented Nov 23, 2021

Uh oh!

gitgitgadget Bot commented Nov 24, 2021

Uh oh!

gitgitgadget Bot commented Nov 25, 2021

neerajsi-msft commented Oct 26, 2021 •

edited by gitgitgadget Bot

Loading