[v8,0/3] push: add "--[no-]force-if-includes"

Message

Srinidhi Kaushik Sept. 27, 2020, 2:17 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 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 "<expect>" value, can cause the update that happened in-between
the checkout and forced push to be lost.

Changes since v7:
  - Clean up the way batching is done for "in_merge_bases_many()".

  - The timestamp check during "reflog" iteration has been moved
    to the end of the function because we should stop collecting
    entries older than the current one (i.e., in the next round).

  - Add a test case to show deletes should be allowed with the
    new option.

  - Minor changes to comments and function names.

Srinidhi Kaushik (3):
  push: add reflog check for "--force-if-includes"
  push: parse and set flag for "--force-if-includes"
  t, doc: update tests, reference for "--force-if-includes"

 Documentation/config/advice.txt |   9 +-
 Documentation/config/push.txt   |   6 +
 Documentation/git-push.txt      |  26 ++++-
 advice.c                        |   3 +
 advice.h                        |   2 +
 builtin/push.c                  |  27 +++++
 builtin/send-pack.c             |  12 ++
 remote-curl.c                   |  14 ++-
 remote.c                        | 188 ++++++++++++++++++++++++++++++--
 remote.h                        |  12 +-
 send-pack.c                     |   1 +
 t/t5533-push-cas.sh             | 140 ++++++++++++++++++++++++
 transport-helper.c              |  10 ++
 transport.c                     |   8 ++
 transport.h                     |  15 ++-
 15 files changed, 455 insertions(+), 18 deletions(-)

base-commit: 9bc233ae1cf19a49e51842c7959d80a675dbd1c0
--
2.28.0

Comments

Junio C Hamano Sept. 28, 2020, 5:31 p.m. UTC | #1

Srinidhi Kaushik <[e-mail]> writes:

> 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.

https://travis-ci.org/github/git/git/jobs/730962458 is a build of
'seen' with this topic, and the same 'seen' without this topic is
https://travis-ci.org/github/git/git/builds/730857608 that passes
all the jobs.  It is curious why one particular job fails while
others in the same build is OK.  I've seen this pattern for this
topic in the past few days.

The failure in t5533-push-cas.sh is sort-of understandable as the
topic directly touches the area of the code the failing test
exercises, but the failure in t3701 is totally unexpected.

You can go down to the bottom of the page and click on the
ci/print-test-failures.sh line to see which test piece fails.

Anything rings a bell?

Thanks.

SZEDER Gábor Sept. 28, 2020, 5:46 p.m. UTC | #2

On Mon, Sep 28, 2020 at 10:31:34AM -0700, Junio C Hamano wrote:
> Srinidhi Kaushik <[e-mail]> writes:
> 
> > 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.
> 
> https://travis-ci.org/github/git/git/jobs/730962458 is a build of
> 'seen' with this topic, and the same 'seen' without this topic is
> https://travis-ci.org/github/git/git/builds/730857608 that passes
> all the jobs.  It is curious why one particular job fails while
> others in the same build is OK.

That build runs the test suite with a bunch of GIT_TEST_* knobs
enabled, and the last two tests added in this series fail when run as:

  GIT_TEST_COMMIT_GRAPH=1 ./t5533-push-cas.sh

> The failure in t5533-push-cas.sh is sort-of understandable as the
> topic directly touches the area of the code the failing test
> exercises, but the failure in t3701 is totally unexpected.

That's not a failure, but the fix of a known breakage: we expect
failure from the scripted 'git add -i' in two tests, but the builtin
'git add -i' fixes those issues, and those two tests succeed with
GIT_TEST_ADD_I_USE_BUILTIN=1.

Srinidhi Kaushik Sept. 28, 2020, 7:34 p.m. UTC | #3

Hello,

On 09/28/2020 19:46, SZEDER Gábor wrote:
> On Mon, Sep 28, 2020 at 10:31:34AM -0700, Junio C Hamano wrote:
> > Srinidhi Kaushik <[e-mail]> writes:
> > 
> > > 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.
> > 
> > https://travis-ci.org/github/git/git/jobs/730962458 is a build of
> > 'seen' with this topic, and the same 'seen' without this topic is
> > https://travis-ci.org/github/git/git/builds/730857608 that passes
> > all the jobs.  It is curious why one particular job fails while
> > others in the same build is OK.
> 
> That build runs the test suite with a bunch of GIT_TEST_* knobs
> enabled, and the last two tests added in this series fail when run as:
> 
>   GIT_TEST_COMMIT_GRAPH=1 ./t5533-push-cas.sh
 
Thanks for the heads-up. It turns out that "in_merge_bases_many()"
returns different results depending on "GIT_TEST_COMMIT_GRAPH".
Initially I thought that it might be related to batching the entries,
but that is not the case.

One of the tests that is failing is:
  cd src &&
  git switch branch &&
  test_commit I &&
  git switch master &&
  test_commit J &&
  git pull --rebase origin master &&
  git push --force-if-includes --force-with-lease="master"

Here, we are testing to check if forced updates are allowed after
the remote changes have been incorporated locally, which is true
in this case and should pass.

"in_merge_bases_many()" used in the check as follows:

  for (chunk = list.items; chunk < list.items + list.nr; chunk += size) {
	  size = list.items + list.nr - chunk;
	  if (MERGE_BASES_BATCH_SIZE < size)
	        size = MERGE_BASES_BATCH_SIZE;

	  if ((ret = in_merge_bases_many(commit, size, chunk)))
	        break;
  }

In "repo_in_merge_bases_many()" [1], the following condition evaluates
to true when "GIT_TEST_COMMIT_GRAPH" is 1.

	generation = commit_graph_generation(commit);
	if (generation > min_generation)
		return ret;

Unfortunately, I am unfamiliar with the code, and not sure why this
happens; I remember Junio mention [2] something about generation
numbers could it be related to that?

A possible "workaround" is to use "in_merge_bases()" for each of the
commits we collect in the list, and the tests seem to pass with
"GIT_TEST_COMMIT_GRAPH" being set; but I wonder if that's the right
way to fix this.

[1] https://git.kernel.org/pub/scm/git/git.git/tree/commit-reach.c#n319
[2] https://public-inbox.org/git/[e-mail]/

Thanks.

Junio C Hamano Sept. 28, 2020, 7:51 p.m. UTC | #4

Srinidhi Kaushik <[e-mail]> writes:

>> That build runs the test suite with a bunch of GIT_TEST_* knobs
>> enabled, and the last two tests added in this series fail when run as:
>> 
>>   GIT_TEST_COMMIT_GRAPH=1 ./t5533-push-cas.sh
>  
> Thanks for the heads-up. It turns out that "in_merge_bases_many()"
> returns different results depending on "GIT_TEST_COMMIT_GRAPH".
> Initially I thought that it might be related to batching the entries,
> but that is not the case.
>
> One of the tests that is failing is:
>   cd src &&
>   git switch branch &&
>   test_commit I &&
>   git switch master &&
>   test_commit J &&
>   git pull --rebase origin master &&
>   git push --force-if-includes --force-with-lease="master"
>
> Here, we are testing to check if forced updates are allowed after
> the remote changes have been incorporated locally, which is true
> in this case and should pass.
>
> "in_merge_bases_many()" used in the check as follows:
> ...
> Unfortunately, I am unfamiliar with the code, and not sure why this
> happens; I remember Junio mention [2] something about generation
> numbers could it be related to that?

Now it's time to summon the commit-graph folks.  I think we should
assume that it a bug in the code with commit-graph if it produces a
result that is different from the code without, until we prove
otherwise (e.g. in a history with clock-skew, traditional traversal
of A..B could give a wrong result where commit-graph may produce a
correct result.  I however think the topology-based merge-base
computation does not suffer from the same issue).

Thanks.

Junio C Hamano Sept. 28, 2020, 8 p.m. UTC | #5

SZEDER Gábor <[e-mail]> writes:

> That build runs the test suite with a bunch of GIT_TEST_* knobs
> enabled,...

Ahh, OK, I knew there was one "funny" job but did not realize
linux-gcc was that one.  It is understandable why that particular
job behaves differently from others ;-)

Thanks.