From: Srinidhi Kaushik Date: Fri, 04 Sep 2020 18:51:47 GMT Subject: [PATCH] push: make `--force-with-lease[=]` safer Message-ID: <20200904185147.77439-1-shrinidhi.kaushik@gmail.com> The `--force-with-lease` option in `git-push`, makes sure that refs on remote aren't clobbered by unexpected changes when the "" ref value is explicitly specified. For other cases (i.e., `--force-with-lease[=]`) where the tip of the remote tracking branch is populated as the "" value, there is a possibility of allowing unwanted overwrites on the remote side when some tools that implicitly fetch remote-tracking refs in the background are used with the repository. If a remote-tracking ref was updated when a rewrite is happening locally and if those changes are pushed by omitting the "" value in `--force-with-lease`, any new changes from the updated tip will be lost locally and will be overwritten on the remote. This problem can be addressed by checking the `reflog` of the branch that is being pushed and verify if there in a entry with the remote tracking ref. By running this check, we can ensure that refs being are fetched in the background while a "lease" is being held are not overlooked before a push, and any new changes can be acknowledged and (if necessary) integrated locally. The new check will cause `git-push` to fail if it detects the presence of any updated refs that we do not have locally and reject the push stating `implicit fetch` as the reason. An experimental configuration setting: `push.rejectImplicitFetch` which defaults to `true` (when `features.experimental` is enabled) has been added, to allow `git-push` to reject a push if the check fails. Signed-off-by: Srinidhi Kaushik --- Hello, I picked this up from #leftoverbits over at GitHub [1] from the open issues list. This idea [2], for a safer `--force-with-lease` was originally proposed by Johannes on the mailing list. [1]: https://github.com/gitgitgadget/git/issues/640 [2]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.1808272306271.73@tvgsbejvaqbjf.bet/ Thanks. Documentation/config/feature.txt | 3 + Documentation/config/push.txt | 14 +++++ Documentation/git-push.txt | 6 ++ builtin/send-pack.c | 5 ++ remote.c | 96 +++++++++++++++++++++++++++++--- remote.h | 4 +- send-pack.c | 1 + t/t5533-push-cas.sh | 86 ++++++++++++++++++++++++++++ transport-helper.c | 5 ++ transport.c | 5 ++ 10 files changed, 217 insertions(+), 8 deletions(-) diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt index c0cbf2bb1c..f93e9fd898 100644 --- a/Documentation/config/feature.txt +++ b/Documentation/config/feature.txt @@ -18,6 +18,9 @@ skipping more commits at a time, reducing the number of round trips. * `protocol.version=2` speeds up fetches from repositories with many refs by allowing the client to specify which refs to list before the server lists them. ++ +* `push.rejectImplicitFetch=true` runs additional checks for linkgit:git-push[1] +`--force-with-lease` to mitigate implicit updates of remote-tracking refs. feature.manyFiles:: Enable config options that optimize for repos with many files in the diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt index f5e5b38c68..1a7184034d 100644 --- a/Documentation/config/push.txt +++ b/Documentation/config/push.txt @@ -114,3 +114,17 @@ push.recurseSubmodules:: specifying '--recurse-submodules=check|on-demand|no'. If not set, 'no' is used by default, unless 'submodule.recurse' is set (in which case a 'true' value means 'on-demand'). + +push.rejectImplicitFetch:: + If set to `true`, runs additional checks for the `--force-with-lease` + option when used with linkgit:git-push[1] if the expected value for + the remote ref is unspecified (`--force-with-lease[=]`), and + instead asked depend on the current value of the remote-tracking ref. + The check ensures that the commit at the tip of the remote-tracking + branch -- which may have been implicitly updated by tools that fetch + remote refs by running linkgit:git-fetch[1] in the background -- has + been integrated locally, when holding the "lease". If the new changes + from such remote-tracking refs have not been updated locally before + pushing, linkgit:git-push[1] will fail indicating the reject reason + as `implicit fetch`. Enabling `feature.experimental` makes this option + default to `true`. diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt index 3b8053447e..2176a743f3 100644 --- a/Documentation/git-push.txt +++ b/Documentation/git-push.txt @@ -320,6 +320,12 @@ seen and are willing to overwrite, then rewrite history, and finally force push changes to `master` if the remote version is still at `base`, regardless of what your local `remotes/origin/master` has been updated to in the background. ++ +Alternatively, setting the (experimental) `push.rejectImplicitFetch` option +to `true` will ensure changes from remote-tracking refs that are updated in the +background using linkgit:git-fetch[1] are accounted for (either by integrating +them locally, or explicitly specifying an overwrite), by rejecting to update +such refs. -f:: --force:: diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 2b9610f121..6500a8267a 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -69,6 +69,11 @@ static void print_helper_status(struct ref *ref) msg = "stale info"; break; + case REF_STATUS_REJECT_IMPLICIT_FETCH: + res = "error"; + msg = "implicit fetch"; + break; + case REF_STATUS_REJECT_ALREADY_EXISTS: res = "error"; msg = "already exists"; diff --git a/remote.c b/remote.c index c5ed74f91c..ee2dedd15b 100644 --- a/remote.c +++ b/remote.c @@ -49,6 +49,8 @@ static const char *pushremote_name; static struct rewrites rewrites; static struct rewrites rewrites_push; +static struct object_id cas_reflog_check_oid; + static int valid_remote(const struct remote *remote) { return (!!remote->url) || (!!remote->foreign_vcs); @@ -1446,6 +1448,22 @@ int match_push_refs(struct ref *src, struct ref **dst, return 0; } +/* + * Consider `push.rejectImplicitFetch` to be set to true if experimental + * features are enabled; use user-defined value if set explicitly. + */ +int reject_implicit_fetch() +{ + int conf = 0; + if (!git_config_get_bool("push.rejectImplicitFetch", &conf)) + return conf; + + if (!git_config_get_bool("feature.experimental", &conf)) + return conf; + + return conf; +} + void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, int force_update) { @@ -1471,16 +1489,21 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, * If the remote ref has moved and is now different * from what we expect, reject any push. * - * It also is an error if the user told us to check - * with the remote-tracking branch to find the value - * to expect, but we did not have such a tracking - * branch. + * It also is an error if the user told us to check with the + * remote-tracking branch to find the value to expect, but we + * did not have such a tracking branch, or we have one that + * has new changes. */ if (ref->expect_old_sha1) { if (!oideq(&ref->old_oid, &ref->old_oid_expect)) reject_reason = REF_STATUS_REJECT_STALE; + else if (reject_implicit_fetch() && ref->implicit_fetch) + reject_reason = REF_STATUS_REJECT_IMPLICIT_FETCH; else - /* If the ref isn't stale then force the update. */ + /* + * If the ref isn't stale, or there was no + * implicit fetch, force the update. + */ force_ref_update = 1; } @@ -2272,23 +2295,67 @@ static int remote_tracking(struct remote *remote, const char *refname, return 0; } +static int oid_in_reflog_ent(struct object_id *ooid, struct object_id *noid, + const char *ident, timestamp_t timestamp, int tz, + const char *message, void *cb_data) +{ + return oideq(noid, &cas_reflog_check_oid); +} + +/* + * Iterate through the reflog of a local branch and check if the tip of the + * remote-tracking branch is reachable from one of the entries. + */ +static int remote_ref_in_reflog(const struct object_id *r_oid, + const struct object_id *l_oid, + const char *local_ref_name) +{ + int ret = 0; + cas_reflog_check_oid = *r_oid; + + struct commit *r_commit, *l_commit; + l_commit = lookup_commit_reference(the_repository, l_oid); + r_commit = lookup_commit_reference(the_repository, r_oid); + + /* + * If the remote-tracking ref is an ancestor of the local ref (a merge, + * for instance) there is no need to iterate through the reflog entries + * to ensure reachability; it can be skipped to return early instead. + */ + ret = (r_commit && l_commit) ? in_merge_bases(r_commit, l_commit) : 0; + if (ret) + goto skip; + + ret = for_each_reflog_ent_reverse(local_ref_name, + oid_in_reflog_ent, + NULL); +skip: + return ret; +} + static void apply_cas(struct push_cas_option *cas, struct remote *remote, struct ref *ref) { - int i; + int i, do_reflog_check = 0; + struct object_id oid; + struct ref *local_ref = get_local_ref(ref->name); /* Find an explicit --