mbox series

[v3,0/7] push: add "--[no-]force-if-includes"

Message ID 20200913145413.18351-1-shrinidhi.kaushik@gmail.com
Headers show
Series push: add "--[no-]force-if-includes" | expand

Message

Srinidhi Kaushik Sept. 13, 2020, 2:54 p.m. UTC
Add a new option: "--force-if-includes" to `git-push` where forced
updates are allowed only if the tip of the remote-tracking ref has
been integrated locally, by verifying if the tip of the remote-tracking
ref -- on which a local branch has based on -- is reachable from at
least one of the "reflog" entries of the branch about to be updated
by force on the remote.

This option can also be used with `--force-with-lease` with setups
where the remote-tracking refs of the repository are implicitly
updated in the background to help prevent unintended remote
overwrites.

If a local branch is based on a remote ref for a rewrite, and if that
remote-tracking ref is updated by a push from another repository after
it has been checked out locally, force updating that branch to remote
with `--force-with-lease[=<refname>[:expect]]` without specifying the
"<refname>" or "<expect>" values, can cause the update that happened
in-between the checkout and forced push to be lost.

Changes since v2:
  * Removed configuration option for setting "--force-if-includes"
    with "--force-with-lease".
  * Broke up the patch into smaller commits.

base-commit: 54e85e7af1ac9e9a92888060d6811ae767fea1bc

Srinidhi Kaushik (7):
  remote: add reflog check for "force-if-includes"
  transport: add flag for "--[no-]force-if-includes"
  send-pack: check ref status for "force-if-includes"
  transport-helper: update ref status for "force-if-includes"
  builtin/push: add option "--[no-]force-if-includes"
  doc: add reference for "--[no-]force-if-includes"
  t: add tests for "force-if-includes"

 Documentation/config/advice.txt   |   9 +-
 Documentation/git-push.txt        |  19 ++++
 advice.c                          |   3 +
 advice.h                          |   2 +
 builtin/push.c                    |  20 +++-
 builtin/send-pack.c               |   5 +
 remote.c                          | 135 +++++++++++++++++++++++--
 remote.h                          |  14 ++-
 send-pack.c                       |   1 +
 t/t5533-push-cas.sh               |  26 +++++
 t/t5549-push-force-if-includes.sh | 161 ++++++++++++++++++++++++++++++
 transport-helper.c                |   5 +
 transport.c                       |  18 +++-
 transport.h                       |  12 ++-
 14 files changed, 411 insertions(+), 19 deletions(-)
 create mode 100755 t/t5549-push-force-if-includes.sh

--
2.28.0

Comments

Johannes Schindelin Sept. 16, 2020, 12:47 p.m. UTC | #1
Hi Srinidhi,

On Sun, 13 Sep 2020, Srinidhi Kaushik wrote:

> Add a new option: "--force-if-includes" to `git-push` where forced
> updates are allowed only if the tip of the remote-tracking ref has
> been integrated locally, by verifying if the tip of the remote-tracking
> ref -- on which a local branch has based on -- is reachable from at
> least one of the "reflog" entries of the branch about to be updated
> by force on the remote.
>
> This option can also be used with `--force-with-lease` with setups
> where the remote-tracking refs of the repository are implicitly
> updated in the background to help prevent unintended remote
> overwrites.
>
> If a local branch is based on a remote ref for a rewrite, and if that
> remote-tracking ref is updated by a push from another repository after
> it has been checked out locally, force updating that branch to remote
> with `--force-with-lease[=<refname>[:expect]]` without specifying the
> "<refname>" or "<expect>" values, can cause the update that happened
> in-between the checkout and forced push to be lost.

Thank you for working on this! I gave this an incomplete look-over, and
offered some suggestions that you hopefully find useful.

> Changes since v2:
>   * Removed configuration option for setting "--force-if-includes"
>     with "--force-with-lease".
>   * Broke up the patch into smaller commits.

While the commits all seem to be compiling individually, I am not really a
fan of introducing a function without a caller that shows how it is
supposed to work. I'd rather see some incremental story, and in this case,
I think if _I_ were to submit this patch series, I would probably have
only two commits: one that extends the already-existing code path to turn
that `use_tracking` flag into that `enum`, and the second patch which
wires up the option, adds the documentation and the tests.

However, please do not let my tastes dictate how you want to present the
work, although I hope that my suggestion inspires you ;-)

Ciao,
Dscho