From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-3.9 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_PASS, SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 521AE1F66E for ; Fri, 4 Sep 2020 18:53:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726208AbgIDSxG (ORCPT ); Fri, 4 Sep 2020 14:53:06 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44568 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726047AbgIDSxF (ORCPT ); Fri, 4 Sep 2020 14:53:05 -0400 Received: from mail-pl1-x642.google.com (mail-pl1-x642.google.com [IPv6:2607:f8b0:4864:20::642]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A2D2DC061244 for ; Fri, 4 Sep 2020 11:53:05 -0700 (PDT) Received: by mail-pl1-x642.google.com with SMTP id s10so1586446plp.1 for ; Fri, 04 Sep 2020 11:53:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=heycKceXRpDyKao0Ponuo8xJ4NCCAuzU/z8yPsvKof4=; b=qUzkSJ3Wt0wCJecRt3emmFNa79d2MB6Fqgdz36m/UsvokzsWQDv3AGlDu2Tw3zNNs+ uNhBqdvEgi6fbkahSHzQ1SRvhMEW2QFseb2XPDnpdxHwZpQF30eq5ncV663RnO+XipDX Tfq3OegLe2dh+1ezg3GfrqQWgiRDv9nQSgWyYJpOCCuk0mtp6tsGMAUYE8rVIYyBzNLW WaM6J8U7CN1uFSaIiK7kTAem/QYqyXZCVuRHOzpIFJq15ceFjvp7SXuIjdDcKXjFfL3j Ei9zJwYQy6n+kc0wX+8d7cdCINJN+Clupf85ly5pyOmUbLQhwiPlcrOAX0PuVMm49kHI Rq8A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=heycKceXRpDyKao0Ponuo8xJ4NCCAuzU/z8yPsvKof4=; b=HnL0QLOqroEJ3Kp0UPZL8dFlG3SlQp4mECNFNo4y37IqTLf7TApRs2N+7nYV8GJJb6 fL7tjSArXV5jx58k8ywALGwoHqdj3bJ1aw8lPoI4fID28KkmaNkLVlgMHUcWDakrxm7Y rktU6wBK6fMswKyZLCu7/pNsba2NTXqSXg3Qi53wA2NaCnIitrTUMUXwEgH0rIybkw0F 2GKodnTwDrg0w11Zmf6THe6ZadV6ErbO4UyH6C0mxXsgqApk0wWmV2NfQE4bcP2DwMFa sdsm3XAd0rntbZG+CpQkgnF4zaZKLvmvZ4fPO/6Q8+wNDUQjmDhCmrIMb9ABxG5TUMpl rBAw== X-Gm-Message-State: AOAM530DY6Vf7+BC9DuPiDhwBGrfAL+Fl3JnY5AlTKHnr0PT97Dvg3DU thopzrT/MeazCT3EoM+M+91TPaIWZppKZ0mX X-Google-Smtp-Source: ABdhPJx0/yEbccWh2SIJc+/N1SwvcGGfVHXat+Dk5cCZFtjZlZozP6k/H7axEwGb4O1FZqb2rHJ4CA== X-Received: by 2002:a17:90a:8c8f:: with SMTP id b15mr9373226pjo.84.1599245584382; Fri, 04 Sep 2020 11:53:04 -0700 (PDT) Received: from localhost.localdomain ([124.123.105.81]) by smtp.gmail.com with ESMTPSA id u15sm6143651pjx.50.2020.09.04.11.53.02 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 04 Sep 2020 11:53:03 -0700 (PDT) From: Srinidhi Kaushik To: git@vger.kernel.org Cc: Johannes Schindelin , Srinidhi Kaushik Subject: [PATCH] push: make `--force-with-lease[=]` safer Date: Sat, 5 Sep 2020 00:21:47 +0530 Message-Id: <20200904185147.77439-1-shrinidhi.kaushik@gmail.com> X-Mailer: git-send-email 2.28.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --