Re: [Tarantool-patches] [PATCH 1/1] tuple: turn invalid bar update into nop

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the review!

Pushed to master. See 2 answers below.

>> diff --git a/src/box/xrow_update_bar.c b/src/box/xrow_update_bar.c
>> index 0033f0044..1d20d6c74 100644
>> --- a/src/box/xrow_update_bar.c
>> +++ b/src/box/xrow_update_bar.c
>> @@ -31,6 +31,22 @@
>>  #include "xrow_update_field.h"
>>  #include "tuple.h"
>>  
>> +/**
>> + * 'Commit' bar creation only when it is fully initialized and
>> + * valid. Because if all this is happening inside an upsert()
>> + * operation, it won't stop the whole xrow upsert. This field will
>> + * still be saved in the result tuple. But in case of an error
>> + * this operation should be skipped. So this is kept 'nop' when
>> + * error happens.
>> + */
>> +static inline int
>> +xrow_update_bar_commit(struct xrow_update_field *field)
> 
> Mb _done/_finish are better names? Up to you. 

I don't mind, changed to 'finish'.

>> +{
>> +	assert(field->type == XUPDATE_NOP);
>> +	field->type = XUPDATE_BAR;
>> +	return 0;
>> +}
>> +
>>  /**
>>   * Locate a field to update by @a op's JSON path and initialize
>>   * @a field as a bar update.
>> diff --git a/test/box/update.test.lua b/test/box/update.test.lua
>> index 1538cae9c..27cf55467 100644
>> --- a/test/box/update.test.lua
>> +++ b/test/box/update.test.lua
>> @@ -343,20 +343,52 @@ t2:update({{'!', 'g[6][1]', 50}})
>>  t4_array:update({{'!', '[4][1]', 100}})
>>  t4_map:update({{'!', '[4].a', 100}})
>>  -- Test errors.
>> -t:update({{'!', 'a', 100}}) -- No such field.
>> -t:update({{'!', 'f.a', 300}}) -- Key already exists.
>> -t:update({{'!', 'f.c.f[0]', 3.5}}) -- No such index, too small.
>> -t:update({{'!', 'f.c.f[100]', 100}}) -- No such index, too big.
>> -t:update({{'!', 'g[4][100]', 700}}) -- Insert index into map.
>> -t:update({{'!', 'g[1][1]', 300}})
>> -t:update({{'!', 'f.g.a', 700}}) -- Insert key into array.
>> -t:update({{'!', 'f.g[1].a', 700}})
>> -t:update({{'!', 'f[*].k', 20}}) -- 'Any' is not considered valid JSON.
> 
> New tests are cool, but mb it is worth reducing diff by placing
> new tests right after old ones? Don't insist tho.

Yeah, this is what I did, but still got all the diff. Because I
saved the operations array into a new variable 'ops', to avoid
the operations duplication. This changed the old lines as well.