Re: [PATCH v3] upload-pack: send shallow info over stdin to pack-objects

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2014, 15:23 UTC
Message-ID: <xmqqeh2arsbi.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CACsJy8AcYBXi6LjyJDeEnogPTXfqYXqijXaLY=bUgNnd4cT_Fg@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 29 quoted lines

> On Sat, Mar 8, 2014 at 1:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> On the receive-pack side, the comment at the bottom of
>>>> preprare_shallow_update() makes it clear that, if we wanted to use
>>>> hooks, we cannot avoid having the proposed new shallow-file in a
>>>> temporary file, which is unfortunate.  Do we have a similar issue on
>>>> hooks on the upload-pack side?
>>>
>>> No. I don't think we have hooks on upload-pack.
>>
>> The question was not only about "do we happen to work OK with the
>> current code?" but about "are we closing the door for the future?"
>>
>> If we ever need to add hooks to upload-pack, with the updated
>> approach, its operation will not be affected by the temporary
>> shallow file tailored for this specific customer.  Which I think is
>> a good thing in general.
>>
>> But at the same time, it means that its operation cannot be
>> customized for the specific customer, taking into account what they
>> lack (which can be gleaned by looking at the temporary shallow
>> information).  I do think that it is not a big downside, but that is
>> merely my knee-jerk reaction.
>
> When upload-pack learns about hooks, I think we'll need to go back
> with --shallow-file, perhaps we a secure temporary place to write in.
> I don't see another way out. Not really sure why upload-pack needs
> customization though. The only case I can think of is to prevent most
> users from fetching certain objects, but that does not sound
> realistic..

I was more thinking along the lines of logging.

But you are right that we can easily revisit --shallow-file interface later.

> Of course.. So what should we do with this? Go with this "no temp
> file" approach and deal with hooks when they come, or prepare now and
> introduce a secure temp. area?

I was rather hoping that we did not have to worry about temporary files. This patch solves the issue for the service people would expect to be read-only the most, and it is a good step in the right direction. So let's follow through with the approach and not worry about "secure temporary" for now.