Re: [PATCH v4 1/3] push: add reflog check for "--force-if-includes"

> Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> writes:
> 
>> +/* Checks if the ref is reachable from the reflog entry. */
>> +static int reflog_entry_reachable(struct object_id *o_oid,
>> +			       struct object_id *n_oid,
>> +			       const char *ident, timestamp_t timestamp,
>> +			       int tz, const char *message, void *cb_data)
>> +{
>> +	struct commit *local_commit;
>> +	struct commit *remote_commit = cb_data;
>> +
>> +	local_commit = lookup_commit_reference(the_repository, n_oid);
>> +	if (local_commit)
>> +		return in_merge_bases(remote_commit, local_commit);
>> +
>> +	return 0;
>> +}
> 
> Makes me wonder, if in_merge_bases() is so expensive that it makes
> sense to split the "were we exactly at the tip?" and "is one of the
> commits we were at a descendant of the tip?" into separate phases,
> if this part should be calling in_merge_bases() one by one.
> 
> Would it make more sense to iterate over reflog entries from newer
> to older, collect them in an array of pointers to "struct commit" in
> a batch of say 8 commits or less, and then ask in_merge_bases_many()
> if the remote_commit is an ancestor of one of these local commits?

> The traversal cost to start from one "local commit" to see if
> remote_commit is an ancestor of it using in_merge_bases() and
> in_merge_bases_many() should be the same and an additional traversal
> cost to start from more local commits should be negligible compared
> to the traversal itself, so making a call to in_merge_bases() for
> each local_commit smells somewhat suboptimal.
> 
> If we were talking about older parts of the history, optional
> generation numbers could change the equation somewhat, but the
> common case for the workflow this series is trying to help is that
> these local commits ane the remote tip are relatively new and it is
> not unlikely that the generation numbers have not been computed for
> them, which is why I suspect that in_merges_many may be a win.
> 
>> @@ -2301,6 +2380,15 @@ void apply_push_cas(struct push_cas_option *cas,
>>   		    struct ref *remote_refs)
>>   {
>>   	struct ref *ref;
>> -	for (ref = remote_refs; ref; ref = ref->next)
>> +	for (ref = remote_refs; ref; ref = ref->next) {
>>   		apply_cas(cas, remote, ref);
>> +
>> +		/*
>> +		 * If "compare-and-swap" is in "use_tracking[_for_rest]"
>> +		 * mode, and if "--foce-if-includes" was specified, run
>> +		 * the check.
>> +		 */
>> +		if (ref->if_includes)
>> +			check_if_includes_upstream(ref);
> 
> s/foce/force/;
> 
> I can see that the code is checking "and if force-if-includes was
> specified" part, but it is not immediately clear where the code
> checks if "--force-with-lease" is used with "tracking" and not with
> "the other side must be exactly this commit" mode here.
> 
>      ... goes and looks ...
> 
> Ah, ok, I found out.
> 
> The field name "if_includes", and the comment for the field in
> remote.h, are both misleading.  It gives an impression that the
> field being true means "--force-if-included is in use", but in
> reality the field means a lot more.  When it is true, it signals
> that "--force-if-included" is in use *and* for this ref we were told
> to use the "--force-with-lease" without an exact object name.  And
> that logic is not here, but has already happened in apply_cas().
> 
> Which makes the above comment correct.  We however need a better
> name for this field and/or an explanation for the field in the
> header file, or both, to avoid misleading readers.
> 
>> diff --git a/remote.h b/remote.h
>> index 5e3ea5a26d..38ab8539e2 100644
>> --- a/remote.h
>> +++ b/remote.h
>> @@ -104,7 +104,9 @@ struct ref {
>>   		forced_update:1,
>>   		expect_old_sha1:1,
>>   		exact_oid:1,
>> -		deletion:1;
>> +		deletion:1,
>> +		if_includes:1, /* If "--force-with-includes" was specified. */
> 
> The description needs to be tightened.
> 
>> +		unreachable:1; /* For "if_includes"; unreachable in reflog. */
> 
> 
> Thanks.
>