Git 2 force commits but Git 1 doesn't

Git 2 force commits but Git 1 doesn't

[Michael Ward]

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]

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: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[Michael Ward]

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.

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

[Michael Ward]

Whoops, 2.26.2 is the Fedora version. Not 2.27.2.

Michael

On 6/22/20 3:30 PM, 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.
>
> 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.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

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: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[Michael Ward]

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

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: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[Michael Ward]

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:
> 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?

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2499 bytes --]

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.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[René Scharfe]

Am 23.06.20 um 03:05 schrieb brian m. carlson:
> 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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2961 bytes --]

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?

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[René Scharfe]

Am 23.06.20 um 17:30 schrieb brian m. carlson:
> 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..)

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2688 bytes --]

On 2020-06-23 at 16:42:48, René Scharfe wrote:
> 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[brian m. carlson]

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

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

[Eric Sunshine]

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?

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

On 2020-06-23 at 21:28:43, Eric Sunshine wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[brian m. carlson]

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

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

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

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

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

[René Scharfe]

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é