Christian Couder <christian.couder@xxxxxxxxx> writes: >> Minor nit. Aren't the above two the same use case? > > In the first case only some large blobs are moved to slower storage > and in the other case all the blobs are moved to slower storage. So > yeah the use cases are very similar. Not sure if and how I can improve > the above wording though. Just by removing one or the other, it would be quite improved, no? Moving away some blobs could move away all or just a selected subset. > I think it can work if the call to write_filtered_pack() is either > before the call to close_object_store() or after it. It would just use > the tempfiles with their temporary name in the first case and with > their final name in the second case. > > write_filtered_pack() is very similar to write_cruft_pack() which is > called before the call to close_object_store(), so I prefer to keep it > before that call too, if possible, for consistency. As long as the set-up is not racy, either would be OK, as the names are not recorded in the end result. If the upstream tells the downstream the temporary's name and then finializes the temporary to the final name before the downstream reacts to the input, however, then by the time downstream starts working on the file, the file may not exist under its original, temporary name, and that kind of race was what I was worried about. > Perhaps this could be dealt with separately though, as I think we > might want to fix write_cruft_pack() first then. Sorry, I am not understanding this quite well. Do you mean we should add one more known-to-be-racy-and-incorrect codepath because there is already a codepath that needs to be fixed anyway? If write_cruft_pack() has a similar issue, then yeah, let's fix that first (testing might be tricky for any racy bugs). And let's use the same technique as used to fix it in this series, too, so that we do not reintroduce a similar bug due to racy setup. Thanks.
