Re: [PATCH] upload-pack: Optionally allow fetching reachable sha1

From: Fredrik Medley <fredrik.medley@gmail.com>
Date: May 3, 2015, 20:13 UTC
Message-ID: <CABA5-znFVPunBxET-42jDTC00gH8VMkG280Ptyr8FUU6vfuiCA@mail.gmail.com>
In-Reply-To: <xmqqy4l5v9tm.fsf@gitster.dls.corp.google.com>

2015-05-03 19:40 GMT+02:00 Junio C Hamano <gitster@pobox.com>:

Show 10 quoted lines

> Fredrik Medley <fredrik.medley@gmail.com> writes:
>
>> For you who don't remember the email discussion, look through the references.
>
> Please don't do this.  Always describe the background yourself in
> the log message so that "git log" can be read offline.  "describe
> yourself" can be done by summarizing earlier discussion, borrowing
> others' words, of course.  And it is a very good idea to give
> references like you did after your summary to optionally allow
> people to verify your summary is correct.

Okay, I understand. My intention was to recapture the old thread, but to keep the part under the references for the commit message. When I get this answer, I do see that this is impossible to understand and I should probably have added a cover-letter instead. (This is my first time every I've supplied patches to such an open-source project.)

Is the text under the reference enough describing or should there be added some more background text?

Unless someone asks for more answers about my attempt to recap the old mail thread, I skip commenting on this part.

Show 6 quoted lines

>
> ...
>
>> With uploadpack.allowreachablesha1inwant configuration option set,
>
> "option set" on the server side, I presume?

Yes, can fix that. "...configuration option set on the server side, "git fetch" can..."

Show 19 quoted lines

>
>> "git fetch" can make a request with a "want" line that names an object
>> that has not been advertised (likely to have been obtained out of band or
>> from a submodule pointer). Only objects reachable from the branch tips,
>> hidden by transfer.hiderefs or not, will be processed.
>
> I am not sure if I am reading the last sentence correctly.  If there
> are two branches, 'master' (exposed) and 'occult' (hidden),
>
>     ---o---o---2---1---x master
>         \
>          o---2---1---y occult
>
> you can ask for the 40-hex object name of occult~2, but you cannot
> ask for the 40-hex object name of master~2?  Why such a restriction?
>
>  ... wait for an answer that justfies the restriction ...
>
> Does that justification applies for allowing occult~4 but not master~4?

I thought I wrote that both visible and hidden branches will be counted as branch tips. Obviously need to reformulate. Maybe: "Only objects reachable from the branch tips, i.e. the union of advertised branches and branches hidden by transfer.hiderefs, will be processed."

Show 14 quoted lines

>
>> @@ -456,8 +457,11 @@ static void check_non_tip(void)
>>       char namebuf[42]; /* ^ + SHA-1 + LF */
>>       int i;
>>
>> -     /* In the normal in-process case non-tip request can never happen */
>> -     if (!stateless_rpc)
>> +     /*
>> +      * In the normal in-process case without
>> +      * uploadpack.allowreachablesha1inwant, non-tip requests can never happen.
>> +      */
>
> That's quite an overlong line; if you switched it to multi-line,
> then fold the line once more, perhaps before "non-tip"?

Yes, can change that. This was to avoid more than 80 characters per line. Should I split to three lines then?

Show 8 quoted lines

>
>> @@ -728,6 +732,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
>>                            sha1_to_hex(sha1), refname_nons,
>>                            0, capabilities,
>>                            allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
>> +                          allow_reachable_sha1_in_want ? " allow-reachable-sha1-in-want" : "",
>
> Which printf format specifier does this new element correspond to?

Ouch! How could I miss that?! Fixing.