threads / discuss / 53734

Git 2 force commits but Git 1 doesn't

Subject: Git 2 force commits but Git 1 doesn't

## tl;dr

19 messages between Jun 22, 2020 and Jun 24, 2020.

replies: 18people: 5as markdown or json

Michael Ward· Jun 22, 2020, 19:40 UTC · lore

We have some repositories we are hosting here using Apache's DAV module to handle remote connections.

The repositories are created using the following:

mkdir [reponame].git cd [reponame].git git --bare init git update-server-info

Our Apache location directive is as follows:
<Location /[reponame].git>
         DAV on
         AuthType Basic
         AuthName "Git"
         AuthBasicProvider ldap
         AuthLDAPUrl [ldap server info]
         <RequireAny>
                 require [ldap filter]
         </RequireAny>
</Location>

The repository config generates with the values in the core section below, and we add the receive and advice sections:

[core]
         repositoryformatversion = 0
         filemode = true
         bare = true
[receive]
         denyNonFastForwards = true
         denyDeletes = true
[advice]
         pushFetchFirst = true

The odd behavior comes when we have git 1 vs git 2 clients attempting to push in changes on the same branch. Git 1 clients will prompt the user that they are out of date and need to pull. Git 2 clients don't and will force push and overwrite the head revision. This occurs with either Git 1 or Git 2 on the server.

We've tested this with the latest Git 2 client on Fedora 32 and Git 1 client on CentOS 7.8. The other oddity is that even when the Git 2 client does a pull to receive changes before making changes and pushing, when another user pulls the change, there is a message shown that a commit was forced.

What am I missing in the repository settings to prevent forced pushes from working and force users to pull before being able to push?

Thanks!
Michael Ward
brian m. carlson· Jun 22, 2020, 20:21 UTC · re: Michael Ward · lore

Re: Git 2 force commits but Git 1 doesn't

On 2020-06-22 at 19:40:15, Michael Ward wrote:
Show 41 quoted lines
> We have some repositories we are hosting here using Apache's DAV module to
> handle remote connections.
> 
> The repositories are created using the following:
> 
> mkdir [reponame].git
> cd [reponame].git
> git --bare init
> git update-server-info
> 
> Our Apache location directive is as follows:
> 
> <Location /[reponame].git>
>         DAV on
>         AuthType Basic
>         AuthName "Git"
>         AuthBasicProvider ldap
>         AuthLDAPUrl [ldap server info]
>         <RequireAny>
>                 require [ldap filter]
>         </RequireAny>
> </Location>
> 
> The repository config generates with the values in the core section below,
> and we add the receive and advice sections:
> 
> [core]
>         repositoryformatversion = 0
>         filemode = true
>         bare = true
> [receive]
>         denyNonFastForwards = true
>         denyDeletes = true
> [advice]
>         pushFetchFirst = true
> 
> The odd behavior comes when we have git 1 vs git 2 clients attempting to
> push in changes on the same branch. Git 1 clients will prompt the user that
> they are out of date and need to pull. Git 2 clients don't and will force
> push and overwrite the head revision. This occurs with either Git 1 or Git 2
> on the server.

Are you seeing this behavior when users are doing a force push, or just a regular push? I see that there exists code for the DAV-based protocol to fail if a user attempts a regular push and is out of date, but I haven't verified it works.

If you're seeing this when users are doing a force push, then that's expected. The receive.* options have no effect here, since those require an appropriate git process to run on the server, and you're using the dumb (DAV-based) protocol, not the smart protocol. Therefore, no git process runs on the server, so all the checking is done on the client side and the client side allows force pushes with an appropriate option.

If you want to have more control over what's pushed, you'll need to use the smart protocol instead, which is outlined in the git-http-backend documentation.

As a note, there are a lot of differences between Git 2.0.0 and the latest version, Git 2.27.0, so it's probably best if you mention the full version when reporting issues. You haven't mentioned the specific versions you're using, but it's possible if you're using the CentOS 6 or 7 versions that they simply didn't support force pushing in this way.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Michael Ward· Jun 22, 2020, 20:30 UTC · re: brian m. carlson · lore

Re: Git 2 force commits but Git 1 doesn't

Versions in use are 2.27.2 and 1.8.3.1. This behavior is seen with regular pushes.

I'll look into the http-backend functionality. If that will help control that, we'll definitely want to use that instead. What surprises me, though, is that even with DAV a 1.8 client appears to work correctly in that it will warn the user that their push is about to clobber the head, but 2.27 doesn't.

Michael
On 6/22/20 3:21 PM, brian m. carlson wrote:
Show 64 quoted lines
> On 2020-06-22 at 19:40:15, Michael Ward wrote:
>> We have some repositories we are hosting here using Apache's DAV module to
>> handle remote connections.
>>
>> The repositories are created using the following:
>>
>> mkdir [reponame].git
>> cd [reponame].git
>> git --bare init
>> git update-server-info
>>
>> Our Apache location directive is as follows:
>>
>> <Location /[reponame].git>
>>          DAV on
>>          AuthType Basic
>>          AuthName "Git"
>>          AuthBasicProvider ldap
>>          AuthLDAPUrl [ldap server info]
>>          <RequireAny>
>>                  require [ldap filter]
>>          </RequireAny>
>> </Location>
>>
>> The repository config generates with the values in the core section below,
>> and we add the receive and advice sections:
>>
>> [core]
>>          repositoryformatversion = 0
>>          filemode = true
>>          bare = true
>> [receive]
>>          denyNonFastForwards = true
>>          denyDeletes = true
>> [advice]
>>          pushFetchFirst = true
>>
>> The odd behavior comes when we have git 1 vs git 2 clients attempting to
>> push in changes on the same branch. Git 1 clients will prompt the user that
>> they are out of date and need to pull. Git 2 clients don't and will force
>> push and overwrite the head revision. This occurs with either Git 1 or Git 2
>> on the server.
> Are you seeing this behavior when users are doing a force push, or just
> a regular push?  I see that there exists code for the DAV-based protocol
> to fail if a user attempts a regular push and is out of date, but I
> haven't verified it works.
>
> If you're seeing this when users are doing a force push, then that's
> expected.  The receive.* options have no effect here, since those
> require an appropriate git process to run on the server, and you're
> using the dumb (DAV-based) protocol, not the smart protocol.  Therefore,
> no git process runs on the server, so all the checking is done on the
> client side and the client side allows force pushes with an appropriate
> option.
>
> If you want to have more control over what's pushed, you'll need to use
> the smart protocol instead, which is outlined in the git-http-backend
> documentation.
>
> As a note, there are a lot of differences between Git 2.0.0 and the
> latest version, Git 2.27.0, so it's probably best if you mention the
> full version when reporting issues.  You haven't mentioned the specific
> versions you're using, but it's possible if you're using the CentOS 6 or
> 7 versions that they simply didn't support force pushing in this way.
Michael Ward· Jun 22, 2020, 20:31 UTC · re: Michael Ward · lore

Re: Git 2 force commits but Git 1 doesn't

Whoops, 2.26.2 is the Fedora version. Not 2.27.2.
Michael
On 6/22/20 3:30 PM, Michael Ward wrote:
Show 82 quoted lines
> Versions in use are 2.27.2 and 1.8.3.1. This behavior is seen with 
> regular pushes.
>
> I'll look into the http-backend functionality. If that will help 
> control that, we'll definitely want to use that instead. What 
> surprises me, though, is that even with DAV a 1.8 client appears to 
> work correctly in that it will warn the user that their push is about 
> to clobber the head, but 2.27 doesn't.
>
> Michael
>
> On 6/22/20 3:21 PM, brian m. carlson wrote:
>> On 2020-06-22 at 19:40:15, Michael Ward wrote:
>>> We have some repositories we are hosting here using Apache's DAV 
>>> module to
>>> handle remote connections.
>>>
>>> The repositories are created using the following:
>>>
>>> mkdir [reponame].git
>>> cd [reponame].git
>>> git --bare init
>>> git update-server-info
>>>
>>> Our Apache location directive is as follows:
>>>
>>> <Location /[reponame].git>
>>>          DAV on
>>>          AuthType Basic
>>>          AuthName "Git"
>>>          AuthBasicProvider ldap
>>>          AuthLDAPUrl [ldap server info]
>>>          <RequireAny>
>>>                  require [ldap filter]
>>>          </RequireAny>
>>> </Location>
>>>
>>> The repository config generates with the values in the core section 
>>> below,
>>> and we add the receive and advice sections:
>>>
>>> [core]
>>>          repositoryformatversion = 0
>>>          filemode = true
>>>          bare = true
>>> [receive]
>>>          denyNonFastForwards = true
>>>          denyDeletes = true
>>> [advice]
>>>          pushFetchFirst = true
>>>
>>> The odd behavior comes when we have git 1 vs git 2 clients 
>>> attempting to
>>> push in changes on the same branch. Git 1 clients will prompt the 
>>> user that
>>> they are out of date and need to pull. Git 2 clients don't and will 
>>> force
>>> push and overwrite the head revision. This occurs with either Git 1 
>>> or Git 2
>>> on the server.
>> Are you seeing this behavior when users are doing a force push, or just
>> a regular push?  I see that there exists code for the DAV-based protocol
>> to fail if a user attempts a regular push and is out of date, but I
>> haven't verified it works.
>>
>> If you're seeing this when users are doing a force push, then that's
>> expected.  The receive.* options have no effect here, since those
>> require an appropriate git process to run on the server, and you're
>> using the dumb (DAV-based) protocol, not the smart protocol. Therefore,
>> no git process runs on the server, so all the checking is done on the
>> client side and the client side allows force pushes with an appropriate
>> option.
>>
>> If you want to have more control over what's pushed, you'll need to use
>> the smart protocol instead, which is outlined in the git-http-backend
>> documentation.
>>
>> As a note, there are a lot of differences between Git 2.0.0 and the
>> latest version, Git 2.27.0, so it's probably best if you mention the
>> full version when reporting issues.  You haven't mentioned the specific
>> versions you're using, but it's possible if you're using the CentOS 6 or
>> 7 versions that they simply didn't support force pushing in this way.
brian m. carlson· Jun 22, 2020, 20:43 UTC · re: Michael Ward · lore

Re: Git 2 force commits but Git 1 doesn't

On 2020-06-22 at 20:30:06, Michael Ward wrote:
Show 8 quoted lines
> Versions in use are 2.27.2 and 1.8.3.1. This behavior is seen with regular
> pushes.
> 
> I'll look into the http-backend functionality. If that will help control
> that, we'll definitely want to use that instead. What surprises me, though,
> is that even with DAV a 1.8 client appears to work correctly in that it will
> warn the user that their push is about to clobber the head, but 2.27
> doesn't.

If you can provide a set of reproduction steps for this, I'd be happy to write a patch to fix it. We should do the right thing for non-force pushes in both cases.

I will say that overall, few people use the DAV-based protocol, since it's significantly less efficient than the smart protocol, so it doesn't surprise me that there may be bugs there.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Michael Ward· Jun 22, 2020, 20:52 UTC · re: brian m. carlson · lore

Re: Git 2 force commits but Git 1 doesn't

Using the steps from my original email for how I had the repository set up (any user authentication scheme works), clone 2 copies from that repository (call them A and B). Make, commit, and push a change in A. Then make, commit, and push a change in B (without first pulling). With the 1.8 client, B will prompt that you're out of date and need to update. With the 2.26 client, B's commit will be pushed and be forced.

Thanks for your help.
Michael
On 6/22/20 3:43 PM, brian m. carlson wrote:
Show 16 quoted lines
> On 2020-06-22 at 20:30:06, Michael Ward wrote:
>> Versions in use are 2.27.2 and 1.8.3.1. This behavior is seen with regular
>> pushes.
>>
>> I'll look into the http-backend functionality. If that will help control
>> that, we'll definitely want to use that instead. What surprises me, though,
>> is that even with DAV a 1.8 client appears to work correctly in that it will
>> warn the user that their push is about to clobber the head, but 2.27
>> doesn't.
> If you can provide a set of reproduction steps for this, I'd be happy to
> write a patch to fix it.  We should do the right thing for non-force
> pushes in both cases.
>
> I will say that overall, few people use the DAV-based protocol, since
> it's significantly less efficient than the smart protocol, so it doesn't
> surprise me that there may be bugs there.
brian m. carlson· Jun 22, 2020, 21:09 UTC · re: Michael Ward · lore

Re: Git 2 force commits but Git 1 doesn't

On 2020-06-22 at 20:52:50, Michael Ward wrote:
Show 6 quoted lines
> Using the steps from my original email for how I had the repository set up
> (any user authentication scheme works), clone 2 copies from that repository
> (call them A and B). Make, commit, and push a change in A. Then make,
> commit, and push a change in B (without first pulling). With the 1.8 client,
> B will prompt that you're out of date and need to update. With the 2.26
> client, B's commit will be pushed and be forced.

I think we're going to need a more specific set of reproduction steps, because adding the following to t5540 succeeds (starting on branch "dev"):

test_expect_success 'non-force push fails if not up to date' '
	git push origin dev &&
	git reset --hard HEAD^ &&
	: >path3 &&
	git add path3 &&
	test_tick &&
	git commit -m dev &&
	test_must_fail git push origin dev &&
	git push origin +dev
'

That means that this is working in at least some cases. If you're still seeing this, can you provide a set of commands (e.g., a shell script) to initialize and create a new repository that triggers this, provided that "origin" refers to a suitable remote?

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Michael Ward· Jun 22, 2020, 22:17 UTC · re: brian m. carlson · lore

Re: Git 2 force commits but Git 1 doesn't

This is assuming that the repository is completely empty to start. Setup:

git clone [repository] repo1 git clone [repository] repo2 cd repo1 echo "test1" > testfile git add testfile git commit -m 'initializing test from 1' git push cd ../repo2 git pull cd ../repo1

Now for the issue:

echo "test1 update" >> testfile git add testfile git commit -m 'update test from 1' git push cd ../repo2 echo "test2" >> testfile git commit -m 'update test from 2' git push

At this point using the git 2.26 client if I pull in repo1, the commit with comment "update test from 1" is gone and the head is now the commit from 2 with "update test from 2" as the comment along with a borked tree. Using the 1.18 client, the push from 2 will prompt to pull first.

Michael
On 6/22/20 4:09 PM, brian m. carlson wrote:
Show 26 quoted lines
> On 2020-06-22 at 20:52:50, Michael Ward wrote:
>> Using the steps from my original email for how I had the repository set up
>> (any user authentication scheme works), clone 2 copies from that repository
>> (call them A and B). Make, commit, and push a change in A. Then make,
>> commit, and push a change in B (without first pulling). With the 1.8 client,
>> B will prompt that you're out of date and need to update. With the 2.26
>> client, B's commit will be pushed and be forced.
> I think we're going to need a more specific set of reproduction steps,
> because adding the following to t5540 succeeds (starting on branch
> "dev"):
>
> test_expect_success 'non-force push fails if not up to date' '
> 	git push origin dev &&
> 	git reset --hard HEAD^ &&
> 	: >path3 &&
> 	git add path3 &&
> 	test_tick &&
> 	git commit -m dev &&
> 	test_must_fail git push origin dev &&
> 	git push origin +dev
> '
>
> That means that this is working in at least some cases.  If you're still
> seeing this, can you provide a set of commands (e.g., a shell script) to
> initialize and create a new repository that triggers this, provided that
> "origin" refers to a suitable remote?
brian m. carlson· Jun 23, 2020, 01:05 UTC · re: Michael Ward · lore

Re: Git 2 force commits but Git 1 doesn't

On 2020-06-22 at 22:17:21, Michael Ward wrote:
Show 28 quoted lines
> This is assuming that the repository is completely empty to start. Setup:
> 
> git clone [repository] repo1
> git clone [repository] repo2
> cd repo1
> echo "test1" > testfile
> git add testfile
> git commit -m 'initializing test from 1'
> git push
> cd ../repo2
> git pull
> cd ../repo1
> 
> Now for the issue:
> 
> echo "test1 update" >> testfile
> git add testfile
> git commit -m 'update test from 1'
> git push
> cd ../repo2
> echo "test2" >> testfile
> git commit -m 'update test from 2'
> git push
> 
> At this point using the git 2.26 client if I pull in repo1, the commit with
> comment "update test from 1" is gone and the head is now the commit from 2
> with "update test from 2" as the comment along with a borked tree. Using the
> 1.18 client, the push from 2 will prompt to pull first.
Thanks, I can reproduce this with the following test in t5540:
test_expect_success 'non-force push fails if not up to date' '
	git init --bare "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_conflict.git &&
	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_conflict.git update-server-info &&
	git clone $HTTPD_URL/dumb/test_repo_conflict.git "$ROOT_PATH"/c1 &&
	git clone $HTTPD_URL/dumb/test_repo_conflict.git "$ROOT_PATH"/c2 &&
	test_commit -C "$ROOT_PATH/c1" path1 &&
	git -C "$ROOT_PATH/c1" push origin HEAD &&
	git -C "$ROOT_PATH/c2" pull &&
	test_commit -C "$ROOT_PATH/c1" path2 &&
	git -C "$ROOT_PATH/c1" push origin HEAD &&
	test_commit -C "$ROOT_PATH/c2" path3 &&
	git -C "$ROOT_PATH/c1" log --graph --all &&
	git -C "$ROOT_PATH/c2" log --graph --all &&
	test_must_fail git -C "$ROOT_PATH/c2" push origin HEAD
'
The relevant code is here:
			if (!has_object_file(&ref->old_oid) ||
			    !ref_newer(&ref->peer_ref->new_oid,
				       &ref->old_oid)) {

In this case, ref_newer returns 1 (true), which is wrong. _However_, if I add a debugging statement that prints ref_newer immediately above that line, like so:

			fprintf(stderr, "debug: a: %s %s %d\n", oid_to_hex(&ref->old_oid), oid_to_hex(&ref->peer_ref->new_oid), ref_newer(&ref->peer_ref->new_oid, &ref->old_oid));
The test starts passing (that is, ref_newer must return 0).

I'm CCing Derrick Stolee, to whom that code blames, because I'm not sure that we should be returning different results in this case with what must be the same arguments.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
René Scharfe· Jun 23, 2020, 08:59 UTC · re: brian m. carlson · lore

Re: Git 2 force commits but Git 1 doesn't

Am 23.06.20 um 03:05 schrieb brian m. carlson:
Show 65 quoted lines
> On 2020-06-22 at 22:17:21, Michael Ward wrote:
>> This is assuming that the repository is completely empty to start. Setup:
>>
>> git clone [repository] repo1
>> git clone [repository] repo2
>> cd repo1
>> echo "test1" > testfile
>> git add testfile
>> git commit -m 'initializing test from 1'
>> git push
>> cd ../repo2
>> git pull
>> cd ../repo1
>>
>> Now for the issue:
>>
>> echo "test1 update" >> testfile
>> git add testfile
>> git commit -m 'update test from 1'
>> git push
>> cd ../repo2
>> echo "test2" >> testfile
>> git commit -m 'update test from 2'
>> git push
>>
>> At this point using the git 2.26 client if I pull in repo1, the commit with
>> comment "update test from 1" is gone and the head is now the commit from 2
>> with "update test from 2" as the comment along with a borked tree. Using the
>> 1.18 client, the push from 2 will prompt to pull first.
>
> Thanks, I can reproduce this with the following test in t5540:
>
> test_expect_success 'non-force push fails if not up to date' '
> 	git init --bare "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_conflict.git &&
> 	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_conflict.git update-server-info &&
> 	git clone $HTTPD_URL/dumb/test_repo_conflict.git "$ROOT_PATH"/c1 &&
> 	git clone $HTTPD_URL/dumb/test_repo_conflict.git "$ROOT_PATH"/c2 &&
> 	test_commit -C "$ROOT_PATH/c1" path1 &&
> 	git -C "$ROOT_PATH/c1" push origin HEAD &&
> 	git -C "$ROOT_PATH/c2" pull &&
> 	test_commit -C "$ROOT_PATH/c1" path2 &&
> 	git -C "$ROOT_PATH/c1" push origin HEAD &&
> 	test_commit -C "$ROOT_PATH/c2" path3 &&
> 	git -C "$ROOT_PATH/c1" log --graph --all &&
> 	git -C "$ROOT_PATH/c2" log --graph --all &&
> 	test_must_fail git -C "$ROOT_PATH/c2" push origin HEAD
> '
>
> The relevant code is here:
>
> 			if (!has_object_file(&ref->old_oid) ||
> 			    !ref_newer(&ref->peer_ref->new_oid,
> 				       &ref->old_oid)) {
>
> In this case, ref_newer returns 1 (true), which is wrong.  _However_, if
> I add a debugging statement that prints ref_newer immediately above that
> line, like so:
>
> 			fprintf(stderr, "debug: a: %s %s %d\n", oid_to_hex(&ref->old_oid), oid_to_hex(&ref->peer_ref->new_oid), ref_newer(&ref->peer_ref->new_oid, &ref->old_oid));
>
> The test starts passing (that is, ref_newer must return 0).
>
> I'm CCing Derrick Stolee, to whom that code blames, because I'm not sure
> that we should be returning different results in this case with what
> must be the same arguments.

The following patch helps by avoiding a commit flag collision between http-push.c and commit-reach.c. I don't know if it causes other collisions, though.

How could we possibly check that? Perhaps by having a commit flag register (a global unsigned int) and having functions announce their bits in it. Colliding announcements would BUG().

diff --git a/http-push.c b/http-push.c
index 822f326599..99adbebdcf 100644
--- a/http-push.c
+++ b/http-push.c
@@ -70,10 +70,10 @@ enum XML_Status {
 #define LOCK_REFRESH 30

 /* Remember to update object flag allocation in object.h */
-#define LOCAL    (1u<<16)
-#define REMOTE   (1u<<17)
-#define FETCHING (1u<<18)
-#define PUSHING  (1u<<19)
+#define LOCAL    (1u<<11)
+#define REMOTE   (1u<<12)
+#define FETCHING (1u<<13)
+#define PUSHING  (1u<<14)

 /* We allow "recursive" symbolic refs. Only within reason, though */
 #define MAXDEPTH 5
diff --git a/object.h b/object.h
index b22328b838..a496d2e4e1 100644
--- a/object.h
+++ b/object.h
@@ -67,7 +67,7 @@ struct object_array {
  * builtin/blame.c:                        12-13
  * bisect.c:                                        16
  * bundle.c:                                        16
- * http-push.c:                                     16-----19
+ * http-push.c:                          11-----14
  * commit-graph.c:                                15
  * commit-reach.c:                                  16-----19
  * sha1-name.c:                                              20
brian m. carlson· Jun 23, 2020, 15:30 UTC · re: René Scharfe · lore

Re: Git 2 force commits but Git 1 doesn't

On 2020-06-23 at 08:59:28, René Scharfe wrote:
> The following patch helps by avoiding a commit flag collision between
> http-push.c and commit-reach.c.  I don't know if it causes other
> collisions, though.
Nice catch.

I don't think it collides with other things because the other things that overlap are either builtin code (which isn't used in git http-push) or upload-pack.c, which would already be having problems before this because it overlaps both before and after (and doesn't appear to be used in http-push either). Therefore, I feel confident in saying this probably doesn't make anything worse.

> How could we possibly check that?  Perhaps by having a commit flag
> register (a global unsigned int) and having functions announce their
> bits in it.  Colliding announcements would BUG().

By my count, we have only 88 individual bits used. If we moved all of the builtin functions plus upload-pack (which shouldn't overlap) to a single range, that would account for 53 bits, which would leave us 35 bits for all the rest of the code. Since we need at most 27 bits for a builtin command, if we used a 64-bit integer, we'd have space for all the remaining bits not to overlap, plus bits for the type and flags bits.

Since we'd be doing only bit operations on the flags variable, the performance impact on 32-bit systems would be very minimal, although we'd allocate an extra 4 bytes for struct object. I don't know if that's a problem.

Assuming we don't want to do that right now, may I have your sign-off for the following code, René, so I can add it to a patch along with my test?

Show 33 quoted lines
> diff --git a/http-push.c b/http-push.c
> index 822f326599..99adbebdcf 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -70,10 +70,10 @@ enum XML_Status {
>  #define LOCK_REFRESH 30
> 
>  /* Remember to update object flag allocation in object.h */
> -#define LOCAL    (1u<<16)
> -#define REMOTE   (1u<<17)
> -#define FETCHING (1u<<18)
> -#define PUSHING  (1u<<19)
> +#define LOCAL    (1u<<11)
> +#define REMOTE   (1u<<12)
> +#define FETCHING (1u<<13)
> +#define PUSHING  (1u<<14)
> 
>  /* We allow "recursive" symbolic refs. Only within reason, though */
>  #define MAXDEPTH 5
> diff --git a/object.h b/object.h
> index b22328b838..a496d2e4e1 100644
> --- a/object.h
> +++ b/object.h
> @@ -67,7 +67,7 @@ struct object_array {
>   * builtin/blame.c:                        12-13
>   * bisect.c:                                        16
>   * bundle.c:                                        16
> - * http-push.c:                                     16-----19
> + * http-push.c:                          11-----14
>   * commit-graph.c:                                15
>   * commit-reach.c:                                  16-----19
>   * sha1-name.c:                                              20
> 
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
René Scharfe· Jun 23, 2020, 16:42 UTC · re: brian m. carlson · lore

Re: Git 2 force commits but Git 1 doesn't

Am 23.06.20 um 17:30 schrieb brian m. carlson:
Show 17 quoted lines
> On 2020-06-23 at 08:59:28, René Scharfe wrote:
>> How could we possibly check that?  Perhaps by having a commit flag
>> register (a global unsigned int) and having functions announce their
>> bits in it.  Colliding announcements would BUG().
>
> By my count, we have only 88 individual bits used.  If we moved all of
> the builtin functions plus upload-pack (which shouldn't overlap) to a
> single range, that would account for 53 bits, which would leave us 35
> bits for all the rest of the code.  Since we need at most 27 bits for a
> builtin command, if we used a 64-bit integer, we'd have space for all
> the remaining bits not to overlap, plus bits for the type and flags
> bits.
>
> Since we'd be doing only bit operations on the flags variable, the
> performance impact on 32-bit systems would be very minimal, although
> we'd allocate an extra 4 bytes for struct object.  I don't know if
> that's a problem.

How many objects would we load into memory? 4 bytes would be OK if multiplied by a million or so (like in the Linux repo), but billions might cause problems.

The switch from SHA1 to SHA256 is going to add 12 bytes per object, so that might be a way to find out what happens if we add 4 bytes on top.

We could save 4 bytes on x64 by reducing FLAG_BITS from 29 to 28, by the way. Increasing it to 32 would be free. That's because parsed (1), type (3) and flags (29) currently occupy 33 bits, which are padded to 8 bytes. And bits 22-24 are only used by builtin/show-branch.c, so it should be easy to tighten the flags range just a bit. Weird.

Why do we have object flags and not commit flags anyway? (I may have asked that before, but can't find the answer..)

Show 37 quoted lines
> Assuming we don't want to do that right now, may I have your sign-off
> for the following code, René, so I can add it to a patch along with my
> test?
>
>> diff --git a/http-push.c b/http-push.c
>> index 822f326599..99adbebdcf 100644
>> --- a/http-push.c
>> +++ b/http-push.c
>> @@ -70,10 +70,10 @@ enum XML_Status {
>>  #define LOCK_REFRESH 30
>>
>>  /* Remember to update object flag allocation in object.h */
>> -#define LOCAL    (1u<<16)
>> -#define REMOTE   (1u<<17)
>> -#define FETCHING (1u<<18)
>> -#define PUSHING  (1u<<19)
>> +#define LOCAL    (1u<<11)
>> +#define REMOTE   (1u<<12)
>> +#define FETCHING (1u<<13)
>> +#define PUSHING  (1u<<14)
>>
>>  /* We allow "recursive" symbolic refs. Only within reason, though */
>>  #define MAXDEPTH 5
>> diff --git a/object.h b/object.h
>> index b22328b838..a496d2e4e1 100644
>> --- a/object.h
>> +++ b/object.h
>> @@ -67,7 +67,7 @@ struct object_array {
>>   * builtin/blame.c:                        12-13
>>   * bisect.c:                                        16
>>   * bundle.c:                                        16
>> - * http-push.c:                                     16-----19
>> + * http-push.c:                          11-----14
>>   * commit-graph.c:                                15
>>   * commit-reach.c:                                  16-----19
>>   * sha1-name.c:                                              20
>>

You're welcome to use it. Not sure if a sign-off is necessary, but here you have it:

Signed-off-by: René Scharfe <l.s.r@web.de>
brian m. carlson· Jun 23, 2020, 19:13 UTC · re: René Scharfe · lore

Re: Git 2 force commits but Git 1 doesn't

On 2020-06-23 at 16:42:48, René Scharfe wrote:
Show 5 quoted lines
> We could save 4 bytes on x64 by reducing FLAG_BITS from 29 to 28, by the
> way.  Increasing it to 32 would be free.  That's because parsed (1),
> type (3) and flags (29) currently occupy 33 bits, which are padded to 8
> bytes.  And bits 22-24 are only used by builtin/show-branch.c, so it
> should be easy to tighten the flags range just a bit.  Weird.

Actually, as you've just shown, we're already using 8 bytes, so increasing FLAG_BITS to 60 wouldn't actually allocate any additional memory. Unfortunately, C11 says that a "bit-field shall have a type that is a qualified or unqualified version of _Bool, signed int, unsigned int, or some other implementation-defined type," so using uint64_t here wouldn't be portable. So in that case we may want to shrink it to be just 4 bytes.

Show 42 quoted lines
> > Assuming we don't want to do that right now, may I have your sign-off
> > for the following code, René, so I can add it to a patch along with my
> > test?
> >
> >> diff --git a/http-push.c b/http-push.c
> >> index 822f326599..99adbebdcf 100644
> >> --- a/http-push.c
> >> +++ b/http-push.c
> >> @@ -70,10 +70,10 @@ enum XML_Status {
> >>  #define LOCK_REFRESH 30
> >>
> >>  /* Remember to update object flag allocation in object.h */
> >> -#define LOCAL    (1u<<16)
> >> -#define REMOTE   (1u<<17)
> >> -#define FETCHING (1u<<18)
> >> -#define PUSHING  (1u<<19)
> >> +#define LOCAL    (1u<<11)
> >> +#define REMOTE   (1u<<12)
> >> +#define FETCHING (1u<<13)
> >> +#define PUSHING  (1u<<14)
> >>
> >>  /* We allow "recursive" symbolic refs. Only within reason, though */
> >>  #define MAXDEPTH 5
> >> diff --git a/object.h b/object.h
> >> index b22328b838..a496d2e4e1 100644
> >> --- a/object.h
> >> +++ b/object.h
> >> @@ -67,7 +67,7 @@ struct object_array {
> >>   * builtin/blame.c:                        12-13
> >>   * bisect.c:                                        16
> >>   * bundle.c:                                        16
> >> - * http-push.c:                                     16-----19
> >> + * http-push.c:                          11-----14
> >>   * commit-graph.c:                                15
> >>   * commit-reach.c:                                  16-----19
> >>   * sha1-name.c:                                              20
> >>
> 
> You're welcome to use it.  Not sure if a sign-off is necessary, but
> here you have it:
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>

Thanks. You did the work here and I'd much prefer to add your sign-off to give you credit for that.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
René Scharfe· Jun 24, 2020, 13:05 UTC · re: René Scharfe · lore

Re: Git 2 force commits but Git 1 doesn't

Am 23.06.20 um 18:42 schrieb René Scharfe:
> Why do we have object flags and not commit flags anyway?  (I may have
> asked that before, but can't find the answer..)
Easy: There are flags that apply to other types of objects; e.g.
NOT_USER_GIVEN only applies to trees and blobs.

Adding a commit flags field sounds easy, but requires duplicating several functions that work with object flags, e.g. clear_commit_marks(). And there's the risk of confusing the two flags types.

René
brian m. carlson· Jun 23, 2020, 20:21 UTC · re: brian m. carlson · lore

[PATCH] http-push: ensure unforced pushes fail when data would be lost

When we push using the DAV-based protocol, the client is the one that performs the ref updates and therefore makes the checks to see whether an unforced push should be allowed. We make this check by determining if either (a) we lack the object file for the old value of the ref or (b) the new value of the ref is not newer than the old value, and in either case, reject the push.

However, the ref_newer function, which performs this latter check, has an odd behavior due to the reuse of certain object flags. Specifically, it will incorrectly return false in its first invocation and then correctly return true on a subsequent invocation. This occurs because the object flags used by http-push.c are the same as those used by commit-reach.c, which implements ref_newer, and one piece of code misinterprets the flags set by the other.

Note that this does not occur in all cases. For example, if the example used in the tests is changed to use one repository instead of two and rewind the head to add a commit, the test passes and we correctly reject the push. However, the example provided does trigger this behavior, and the code has been broken in this way since at least Git 2.0.0.

To solve this problem, let's move the two sets of object flags so that they don't overlap, since we're clearly using them at the same time. The new set should not conflict with other usage because other users are either builtin code (which is not compiled into git http-push) or upload-pack (which we similarly do not use here).

Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 http-push.c                 |  8 ++++----
 object.h                    |  2 +-
 t/t5540-http-push-webdav.sh | 16 ++++++++++++++++
 3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/http-push.c b/http-push.c
index 822f326599..99adbebdcf 100644
--- a/http-push.c
+++ b/http-push.c
@@ -70,10 +70,10 @@ enum XML_Status {
 #define LOCK_REFRESH 30
 
 /* Remember to update object flag allocation in object.h */
-#define LOCAL    (1u<<16)
-#define REMOTE   (1u<<17)
-#define FETCHING (1u<<18)
-#define PUSHING  (1u<<19)
+#define LOCAL    (1u<<11)
+#define REMOTE   (1u<<12)
+#define FETCHING (1u<<13)
+#define PUSHING  (1u<<14)
 
 /* We allow "recursive" symbolic refs. Only within reason, though */
 #define MAXDEPTH 5
diff --git a/object.h b/object.h
index b22328b838..a496d2e4e1 100644
--- a/object.h
+++ b/object.h
@@ -67,7 +67,7 @@ struct object_array {
  * builtin/blame.c:                        12-13
  * bisect.c:                                        16
  * bundle.c:                                        16
- * http-push.c:                                     16-----19
+ * http-push.c:                          11-----14
  * commit-graph.c:                                15
  * commit-reach.c:                                  16-----19
  * sha1-name.c:                                              20
diff --git a/t/t5540-http-push-webdav.sh b/t/t5540-http-push-webdav.sh
index d476c33509..450321fddb 100755
--- a/t/t5540-http-push-webdav.sh
+++ b/t/t5540-http-push-webdav.sh
@@ -126,6 +126,22 @@ test_expect_success 'create and delete remote branch' '
 	test_must_fail git show-ref --verify refs/remotes/origin/dev
 '
 
+test_expect_success 'non-force push fails if not up to date' '
+	git init --bare "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_conflict.git &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_conflict.git update-server-info &&
+	git clone $HTTPD_URL/dumb/test_repo_conflict.git "$ROOT_PATH"/c1 &&
+	git clone $HTTPD_URL/dumb/test_repo_conflict.git "$ROOT_PATH"/c2 &&
+	test_commit -C "$ROOT_PATH/c1" path1 &&
+	git -C "$ROOT_PATH/c1" push origin HEAD &&
+	git -C "$ROOT_PATH/c2" pull &&
+	test_commit -C "$ROOT_PATH/c1" path2 &&
+	git -C "$ROOT_PATH/c1" push origin HEAD &&
+	test_commit -C "$ROOT_PATH/c2" path3 &&
+	git -C "$ROOT_PATH/c1" log --graph --all &&
+	git -C "$ROOT_PATH/c2" log --graph --all &&
+	test_must_fail git -C "$ROOT_PATH/c2" push origin HEAD
+'
+
 test_expect_success 'MKCOL sends directory names with trailing slashes' '
 
 	! grep "\"MKCOL.*[^/] HTTP/[^ ]*\"" < "$HTTPD_ROOT_PATH"/access.log
Eric Sunshine· Jun 23, 2020, 21:28 UTC · re: brian m. carlson · lore

Re: [PATCH] http-push: ensure unforced pushes fail when data would be lost

On Tue, Jun 23, 2020 at 5:23 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 9 quoted lines
> When we push using the DAV-based protocol, the client is the one that
> performs the ref updates and therefore makes the checks to see whether
> an unforced push should be allowed.  We make this check by determining
> if either (a) we lack the object file for the old value of the ref or
> (b) the new value of the ref is not newer than the old value, and in
> either case, reject the push.
> [...]
> Signed-off-by: René Scharfe <l.s.r@web.de>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>

Do you want a "Reported-by: Michael Ward <mward@smartsoftwareinc.com>" in this vicinity?

brian m. carlson· Jun 23, 2020, 21:50 UTC · re: Eric Sunshine · lore

Re: [PATCH] http-push: ensure unforced pushes fail when data would be lost

On 2020-06-23 at 21:28:43, Eric Sunshine wrote:
Show 14 quoted lines
> On Tue, Jun 23, 2020 at 5:23 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > When we push using the DAV-based protocol, the client is the one that
> > performs the ref updates and therefore makes the checks to see whether
> > an unforced push should be allowed.  We make this check by determining
> > if either (a) we lack the object file for the old value of the ref or
> > (b) the new value of the ref is not newer than the old value, and in
> > either case, reject the push.
> > [...]
> > Signed-off-by: René Scharfe <l.s.r@web.de>
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> 
> Do you want a "Reported-by: Michael Ward <mward@smartsoftwareinc.com>"
> in this vicinity?
Yes, we do.  Thanks for reminding me.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
brian m. carlson· Jun 23, 2020, 21:52 UTC · re: brian m. carlson · lore

[PATCH v2] http-push: ensure unforced pushes fail when data would be lost

When we push using the DAV-based protocol, the client is the one that performs the ref updates and therefore makes the checks to see whether an unforced push should be allowed. We make this check by determining if either (a) we lack the object file for the old value of the ref or (b) the new value of the ref is not newer than the old value, and in either case, reject the push.

However, the ref_newer function, which performs this latter check, has an odd behavior due to the reuse of certain object flags. Specifically, it will incorrectly return false in its first invocation and then correctly return true on a subsequent invocation. This occurs because the object flags used by http-push.c are the same as those used by commit-reach.c, which implements ref_newer, and one piece of code misinterprets the flags set by the other.

Note that this does not occur in all cases. For example, if the example used in the tests is changed to use one repository instead of two and rewind the head to add a commit, the test passes and we correctly reject the push. However, the example provided does trigger this behavior, and the code has been broken in this way since at least Git 2.0.0.

To solve this problem, let's move the two sets of object flags so that they don't overlap, since we're clearly using them at the same time. The new set should not conflict with other usage because other users are either builtin code (which is not compiled into git http-push) or upload-pack (which we similarly do not use here).

Reported-by: Michael Ward <mward@smartsoftwareinc.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
Changes from v1:
* Add Reported-by.
 http-push.c                 |  8 ++++----
 object.h                    |  2 +-
 t/t5540-http-push-webdav.sh | 16 ++++++++++++++++
 3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/http-push.c b/http-push.c
index 822f326599..99adbebdcf 100644
--- a/http-push.c
+++ b/http-push.c
@@ -70,10 +70,10 @@ enum XML_Status {
 #define LOCK_REFRESH 30
 
 /* Remember to update object flag allocation in object.h */
-#define LOCAL    (1u<<16)
-#define REMOTE   (1u<<17)
-#define FETCHING (1u<<18)
-#define PUSHING  (1u<<19)
+#define LOCAL    (1u<<11)
+#define REMOTE   (1u<<12)
+#define FETCHING (1u<<13)
+#define PUSHING  (1u<<14)
 
 /* We allow "recursive" symbolic refs. Only within reason, though */
 #define MAXDEPTH 5
diff --git a/object.h b/object.h
index b22328b838..a496d2e4e1 100644
--- a/object.h
+++ b/object.h
@@ -67,7 +67,7 @@ struct object_array {
  * builtin/blame.c:                        12-13
  * bisect.c:                                        16
  * bundle.c:                                        16
- * http-push.c:                                     16-----19
+ * http-push.c:                          11-----14
  * commit-graph.c:                                15
  * commit-reach.c:                                  16-----19
  * sha1-name.c:                                              20
diff --git a/t/t5540-http-push-webdav.sh b/t/t5540-http-push-webdav.sh
index d476c33509..450321fddb 100755
--- a/t/t5540-http-push-webdav.sh
+++ b/t/t5540-http-push-webdav.sh
@@ -126,6 +126,22 @@ test_expect_success 'create and delete remote branch' '
 	test_must_fail git show-ref --verify refs/remotes/origin/dev
 '
 
+test_expect_success 'non-force push fails if not up to date' '
+	git init --bare "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_conflict.git &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_conflict.git update-server-info &&
+	git clone $HTTPD_URL/dumb/test_repo_conflict.git "$ROOT_PATH"/c1 &&
+	git clone $HTTPD_URL/dumb/test_repo_conflict.git "$ROOT_PATH"/c2 &&
+	test_commit -C "$ROOT_PATH/c1" path1 &&
+	git -C "$ROOT_PATH/c1" push origin HEAD &&
+	git -C "$ROOT_PATH/c2" pull &&
+	test_commit -C "$ROOT_PATH/c1" path2 &&
+	git -C "$ROOT_PATH/c1" push origin HEAD &&
+	test_commit -C "$ROOT_PATH/c2" path3 &&
+	git -C "$ROOT_PATH/c1" log --graph --all &&
+	git -C "$ROOT_PATH/c2" log --graph --all &&
+	test_must_fail git -C "$ROOT_PATH/c2" push origin HEAD
+'
+
 test_expect_success 'MKCOL sends directory names with trailing slashes' '
 
 	! grep "\"MKCOL.*[^/] HTTP/[^ ]*\"" < "$HTTPD_ROOT_PATH"/access.log
Junio C Hamano· Jun 23, 2020, 22:41 UTC · re: brian m. carlson · lore

Re: [PATCH v2] http-push: ensure unforced pushes fail when data would be lost

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 9 quoted lines
> To solve this problem, let's move the two sets of object flags so that
> they don't overlap, since we're clearly using them at the same time.
> The new set should not conflict with other usage because other users are
> either builtin code (which is not compiled into git http-push) or
> upload-pack (which we similarly do not use here).
>
> Reported-by: Michael Ward <mward@smartsoftwareinc.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Thanks.  Well explained and cleanly fixed.

← back to recent threads