Re: [PATCH v5 5/5] convert: add "status=delayed" to filter process protocol

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 24, 2017, 18:51 UTC
Message-ID: <xmqq37ap9q2i.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <A8FAEFDC-1ADA-4355-80DC-069A739F829C@gmail.com>

Lars Schneider <larsxschneider@gmail.com> writes:

Show 14 quoted lines

>> On 24 Jun 2017, at 16:19, Jeff King <peff@peff.net> wrote:
>> 
>> Speaking of which, _are_ we OK with out-of-order processing in things
>> like checkout? Certainly we care about deleting items before checking
>> out new ones (so getting rid of "foo/bar" to make room for "foo" and
>> vice versa).  I guess it's OK as long as the delayed items are always
>> filters that check out new items.
>
> Junio noticed that too but thinks we should be OK:
> "[...] We checkout removals first to make room so
> that creation of a path X can succeed if an existing path X/Y
> that used to want to see X as a directory can succeed [...]"
>
> http://public-inbox.org/git/xmqqvavotych.fsf@gitster.mtv.corp.google.com/

In which I said

   ...  I
   think "remove all and then create" you do not specifically have
   to worry about with the proposed change, but you may need to
   inspect and verify there aren't other kind of order dependency.

Yes, I think having two separate loops in the caller can help guaranteeing this, as long as the delayed items are always filters that check out new things. It will break once you have delayed removals but I do not see how such a thing would be necessary ;-)

But there may be other kinds of order dependency--I didn't look for them.

Show 15 quoted lines

>>> @@ -647,6 +653,14 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
>>> 	if (err)
>>> 		goto done;
>>> 
>>> +	if (CAP_DELAY & entry->supported_capabilities &&
>>> +	    dco && dco->state == CE_CAN_DELAY) {
>> 
>> In a complicated conditional with bit operations, we usually put the bit
>> operation in its own parentheses so it's more obvious that it wasn't
>> supposed to be "&&". Like:
>> 
>>  if ((CAP_DELAY & entry->supported_capabilities) &&
>>      dco && dco->state == CE_CAN_DELAY))
>
> Agreed!

Why wasn't this caught earlier?  I thought this is something gcc warns about.

Show 5 quoted lines

>> The operator precedence is such that it works without them, so this is
>> just a style question (I'd also usually put the flags field before the
>> flag itself, but that's really getting into aesthetics).
>
> You mean (entry & CAP_DELAY) instead of (CAP_DELAY & entry)?

Peff is continuing his explanation why (A & B && C) is technically correct and preferring ((A & B) && C) is purely stylistic. "A & B" binds tighter than "something && C" which means that (A & B && C) cannot be misinterpreted as (A & (B && C)).