[v2,0/2] push: make "--force-with-lease" safer

Message

Srinidhi Kaushik Sept. 12, 2020, 3:04 p.m. UTC

The `--force-with-lease[=<refname>[:<expect]]` option in `git-push`
makes sure that refs on remote aren't clobbered by unexpected changes
when the "<refname>" and "<expect>" ref values are explicitly specified.

For setups where the remote-tracking refs are implicitly updated by
tools that run in the background, not specifying an "<expect>" value
for force updates may result in loss of remote updates.

Let's say, we have a local branch that is based on a remote ref
that may be updated implicitly with a background fetch. If we decide
to rewrite changes on the remote, and base our local branch on the
current tip of the remote-tracking ref. If the remote ref was updated
by a push from another user, and it was fetched -- right after the
checkout, during rewrite or before push -- in the background, and if
we decide to force update our rewritten local changes on the remote
with `--force-with-lease[=<refname>]` (i.e, without specifying an
"<expect>" value) to `git-push`, the remote changes pushed from
another user during our rewrite may be lost.

The new option `--force-if-includes` will allow forcing an update only
if the tip of the remote-tracking ref has been integrated locally.
Using this along with `--force-with-lease`, during the time of push
can help preventing unintended remote overwrites.

Srinidhi Kaushik (2):
  push: add "--[no-]force-if-includes"
  push: enable "forceIfIncludesWithLease" by default

 Documentation/config/advice.txt   |   4 +
 Documentation/config/feature.txt  |   6 ++
 Documentation/config/push.txt     |   8 ++
 Documentation/git-push.txt        |  22 +++++
 advice.c                          |   3 +
 advice.h                          |   2 +
 builtin/push.c                    |  38 +++++++-
 builtin/send-pack.c               |  13 ++-
 remote.c                          | 129 ++++++++++++++++++++++++---
 remote.h                          |  14 ++-
 send-pack.c                       |   1 +
 t/t5533-push-cas.sh               |  53 ++++++++++++
 t/t5549-push-force-if-includes.sh | 139 ++++++++++++++++++++++++++++++
 transport-helper.c                |   5 ++
 transport.c                       |  24 +++++-
 transport.h                       |  12 +--
 16 files changed, 451 insertions(+), 22 deletions(-)
 create mode 100755 t/t5549-push-force-if-includes.sh

base-commit: 54e85e7af1ac9e9a92888060d6811ae767fea1bc
--
2.28.0

Comments

Junio C Hamano Sept. 12, 2020, 6:15 p.m. UTC | #1

Srinidhi Kaushik <[e-mail]> writes:

> The `--force-with-lease[=<refname>[:<expect]]` option in `git-push`
> makes sure that refs on remote aren't clobbered by unexpected changes
> when the "<refname>" and "<expect>" ref values are explicitly specified.

If you did a feature with different semantics to satisfy Dscho's
need, then this is no longer "make force-with-lease safer", I would
think.  Hopefully it is just the cover letter.

> The new option `--force-if-includes` will allow forcing an update only
> if the tip of the remote-tracking ref has been integrated locally.
> Using this along with `--force-with-lease`, during the time of push
> can help preventing unintended remote overwrites.

"if-includes" sounds quite sensible.  I think you want to lose the
word "lease" from the configuration variable name.  I do not think
it should be on by default, though.

Thanks.

Srinidhi Kaushik Sept. 12, 2020, 9:03 p.m. UTC | #2

Hi Junio,

On 09/12/2020 11:15, Junio C Hamano wrote:
> Srinidhi Kaushik <[e-mail]> writes:
> 
> > The `--force-with-lease[=<refname>[:<expect]]` option in `git-push`
> > makes sure that refs on remote aren't clobbered by unexpected changes
> > when the "<refname>" and "<expect>" ref values are explicitly specified.
> 
> If you did a feature with different semantics to satisfy Dscho's
> need, then this is no longer "make force-with-lease safer", I would
> think.  Hopefully it is just the cover letter.

Yes, this patch is about the new option, but I thought of keeping the
original reason for introducing it  in the cover letter for context.
I will add this as a note and change subjject cover letter in v3.
 
> > The new option `--force-if-includes` will allow forcing an update only
> > if the tip of the remote-tracking ref has been integrated locally.
> > Using this along with `--force-with-lease`, during the time of push
> > can help preventing unintended remote overwrites.
> 
> "if-includes" sounds quite sensible.  I think you want to lose the
> word "lease" from the configuration variable name.  I do not think
> it should be on by default, though.

Thanks; that makes sense. I am thinking of  just adding the option
as a command line argument without a configuration option. Will change
this in the next patch-set.

> [...]

Thanks again, for reviewing this.