mbox series

[0/2] tmp-objdir: fix regressions in core.fsyncobjectfiles=batch

Message ID pull.1067.git.1635287730.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series tmp-objdir: fix regressions in core.fsyncobjectfiles=batch | expand

Message

Johannes Schindelin via GitGitGadget Oct. 26, 2021, 10:35 p.m. UTC
* 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

 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

Comments

Johannes Schindelin Oct. 27, 2021, 12:44 p.m. UTC | #1
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
>
>
Junio C Hamano Oct. 27, 2021, 9:09 p.m. UTC | #2
Johannes Schindelin <Johannes.Schindelin@gmx.de> 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).
Neeraj Singh Oct. 27, 2021, 10:57 p.m. UTC | #3
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
Junio C Hamano Oct. 28, 2021, 12:30 a.m. UTC | #4
Neeraj Singh <nksingh85@gmail.com> 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.