On 19/09/2020 21:03, Junio C Hamano wrote:
Srinidhi Kaushik <shrinidhi.kaushik@xxxxxxxxx> 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?
As I said before[1] I think we should also be checking the reflog dates
so that we do not look at any local reflog entries that are older than
the most recent reflog entry for the remote tracking branch. This
protects against a background fetch when the remote has been rewound and
it would also reduce the number of calls to in_merge_bases().
Best Wishes
Phillip
[1]
https://lore.kernel.org/git/624d9e35-29b8-4012-a3d6-e9b00a9e4485@xxxxxxxxx/
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.
