Re: [PATCH v4 4/6] update-index: use the bulk-checkin infrastructure

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 21, 2021, 23:46 UTC
Message-ID: <87ee9h8p0a.fsf@evledraar.gmail.com>
In-Reply-To: <f7f756f3932cdbca587de397598758c685bac29a.1632176111.git.gitgitgadget@gmail.com>

On Mon, Sep 20 2021, Neeraj Singh via GitGitGadget wrote:

Show 17 quoted lines

> From: Neeraj Singh <neerajsi@microsoft.com>
>
> The update-index functionality is used internally by 'git stash push' to
> setup the internal stashed commit.
>
> This change enables bulk-checkin for update-index infrastructure to
> speed up adding new objects to the object database by leveraging the
> pack functionality and the new bulk-fsync functionality. This mode
> is enabled when passing paths to update-index via the --stdin flag,
> as is done by 'git stash'.
>
> There is some risk with this change, since under batch fsync, the object
> files will not be available until the update-index is entirely complete.
> This usage is unlikely, since any tool invoking update-index and
> expecting to see objects would have to snoop the output of --verbose to
> find out when update-index has actually processed a given path.
> Additionally the index is locked for the duration of the update.

Would you really need to sniff the verbose output? If I'm streaming data to update-index now it looks like I could assume before that update-index would have done the work if I managed to fflush() to it, since it's processing a line at a time and doing the work in that line-at-a-time loop.

I.e. you could print lines to it, and then do concurrent object lookups knowing the data was written already...

I think this is probably fine, but that case seems way likelier than someone sniffing back the verbose output, presumably for the "add" in update_one(), but that's called in the getline_fn() loop...

All of this makes me wonder why this isn't using tmp-objdir.c, i.e. we could have our cake and eat it too by writing the "real" objects, and then just renaming them between directories instead. But perhaps the answer has something to do with the metadata issues I raised.

And well, tmp-objdir.c isn't going to help someone in practice that's relying on this "update-index --stdin" behavior, as they won't know where we staged the temporary files...