{"thread":{"id":"41231","subject":"[PATCH] push --force-with-lease: Fix ref status reporting","startedAt":"2016-01-21T03:17:28Z","lastAt":"2016-01-26T02:36:57Z","messageCount":3,"participants":["Andrew Wheeler","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"276482","messageId":"1453346248-4489-1-git-send-email-agwheeler@gmail.com","threadId":"41231","inReplyTo":null,"subject":"[PATCH] push --force-with-lease: Fix ref status reporting","fromName":"Andrew Wheeler","fromEmail":"agwheeler@gmail.com","sentAt":"2016-01-21T03:17:28Z","receivedAt":"2016-01-21T03:17:28Z","isPatch":true,"sender":{"key":"agwheeler@gmail.com","avatar":null},"body":"From: Andrew Wheeler \n\nThe --force--with-lease push option leads to less\ndetailed status information than --force. In particular,\nthe output indicates that a reference was fast-forwarded,\neven when it was force-updated.\n\nModify the --force-with-lease ref status logic to leverage\nthe --force ref status logic when the \"lease\" conditions\nare met.\n\nSigned-off-by: Andrew Wheeler \n---\n remote.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 9d34b5a..bad6213 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1545,11 +1545,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t}\n \n \t\t/*\n-\t\t * Bypass the usual \"must fast-forward\" check but\n-\t\t * replace it with a weaker \"the old value must be\n-\t\t * this value we observed\". If the remote ref has\n-\t\t * moved and is now different from what we expect,\n-\t\t * reject any push.\n+\t\t * If the remote ref has moved and is now different\n+\t\t * from what we expect, reject any push.\n \t\t *\n \t\t * It also is an error if the user told us to check\n \t\t * with the remote-tracking branch to find the value\n@@ -1560,10 +1557,14 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t\tif (ref->expect_old_no_trackback ||\n \t\t\t oidcmp(&ref->old_oid, &ref->old_oid_expect))\n \t\t\t\treject_reason = REF_STATUS_REJECT_STALE;\n+\t\t\telse\n+\t\t\t\t/* If the ref isn't stale then force the update. */\n+\t\t\t\tforce_ref_update = 1;\n \t\t}\n \n \t\t/*\n-\t\t * The usual \"must fast-forward\" rules.\n+\t\t * If the update isn't already rejected then check\n+\t\t * the usual \"must fast-forward\" rules.\n \t\t *\n \t\t * Decide whether an individual refspec A:B can be\n \t\t * pushed. The push will succeed if any of the\n@@ -1580,9 +1581,10 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t *\n \t\t * (4) it is forced using the +A:B notation, or by\n \t\t * passing the --force argument\n+\t\t *\n \t\t */\n \n-\t\telse if (!ref->deletion && !is_null_oid(&ref->old_oid)) {\n+\t\tif (!reject_reason && !ref->deletion && !is_null_oid(&ref->old_oid)) {\n \t\t\tif (starts_with(ref->name, \"refs/tags/\"))\n \t\t\t\treject_reason = REF_STATUS_REJECT_ALREADY_EXISTS;\n \t\t\telse if (!has_object_file(&ref->old_oid))\n-- \n1.7.11.2\n"},{"id":"276731","messageId":"xmqqvb6hfo1u.fsf@gitster.mtv.corp.google.com","threadId":"41231","inReplyTo":"1453346248-4489-1-git-send-email-agwheeler@gmail.com","subject":"Re: [PATCH] push --force-with-lease: Fix ref status reporting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-25T19:37:33Z","receivedAt":"2016-01-25T19:37:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Wheeler writes:\n\n> From: Andrew Wheeler \n>\n> The --force--with-lease push option leads to less\n> detailed status information than --force. In particular,\n> the output indicates that a reference was fast-forwarded,\n> even when it was force-updated.\n>\n> Modify the --force-with-lease ref status logic to leverage\n> the --force ref status logic when the \"lease\" conditions\n> are met.\n\nThe description of the observed problem makes sense. Thanks for\nworking on this.\n\n> Signed-off-by: Andrew Wheeler \n> ---\n> remote.c | 16 +++++++++-------\n> 1 file changed, 9 insertions(+), 7 deletions(-)\n>\n> diff --git a/remote.c b/remote.c\n> index 9d34b5a..bad6213 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1545,11 +1545,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n> \t\t}\n> \n> \t\t/*\n> -\t\t * Bypass the usual \"must fast-forward\" check but\n> -\t\t * replace it with a weaker \"the old value must be\n> -\t\t * this value we observed\". If the remote ref has\n> -\t\t * moved and is now different from what we expect,\n> -\t\t * reject any push.\n> +\t\t * If the remote ref has moved and is now different\n> +\t\t * from what we expect, reject any push.\n> \t\t *\n\nThis simplification of the comment makes sense, especially with the\ncode that results from the change of the last \"else if\".\n\n> \t\t * It also is an error if the user told us to check\n> \t\t * with the remote-tracking branch to find the value\n> @@ -1560,10 +1557,14 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n> \t\t\tif (ref->expect_old_no_trackback ||\n> \t\t\t oidcmp(&ref->old_oid, &ref->old_oid_expect))\n> \t\t\t\treject_reason = REF_STATUS_REJECT_STALE;\n> +\t\t\telse\n> +\t\t\t\t/* If the ref isn't stale then force the update. */\n> +\t\t\t\tforce_ref_update = 1;\n> \t\t}\n> \n> \t\t/*\n> -\t\t * The usual \"must fast-forward\" rules.\n> +\t\t * If the update isn't already rejected then check\n> +\t\t * the usual \"must fast-forward\" rules.\n> \t\t *\n> \t\t * Decide whether an individual refspec A:B can be\n> \t\t * pushed. The push will succeed if any of the\n> @@ -1580,9 +1581,10 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n> \t\t *\n> \t\t * (4) it is forced using the +A:B notation, or by\n> \t\t * passing the --force argument\n> +\t\t *\n\nThis new blank line is probably unwanted.\n\n> \t\t */\n> \n> -\t\telse if (!ref->deletion && !is_null_oid(&ref->old_oid)) {\n> +\t\tif (!reject_reason && !ref->deletion && !is_null_oid(&ref->old_oid)) {\n> \t\t\tif (starts_with(ref->name, \"refs/tags/\"))\n> \t\t\t\treject_reason = REF_STATUS_REJECT_ALREADY_EXISTS;\n> \t\t\telse if (!has_object_file(&ref->old_oid))\n\nOK. So the idea is that a successful force-with-lease push would\nhave a zero reject_reason at this point, and the if/else cascade\nwould still trigger and would set it to STATUS_REJECT_NEEDS_FORCE,\njust like a usual forced push without a lease. And then because the\nlocal variable force_ref_update is set, it would report the forced\nsuccess exactly the same way as the usual forced push.\n\nSounds very sensible.\n\nDo we want to make sure that other people will not break this fix in\nthe future by adding a few tests, perhaps to t/t5533?\n\nThanks.\n"},{"id":"276751","messageId":"CAE=k0mt=wCA+evSXdDp1w5n5r34pyNoKvz14wfLdwzVTS+jGVQ@mail.gmail.com","threadId":"41231","inReplyTo":"xmqqvb6hfo1u.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] push --force-with-lease: Fix ref status reporting","fromName":"Andrew Wheeler","fromEmail":"awheeler@motorola.com","sentAt":"2016-01-26T02:36:57Z","receivedAt":"2016-01-26T02:36:57Z","isPatch":true,"sender":{"key":"awheeler@motorola.com","avatar":"https://avatars.githubusercontent.com/u/6362528?v=4"},"body":"On Mon, Jan 25, 2016 at 1:37 PM, Junio C Hamano wrote:\n\n> > * passing the --force argument\n> > + *\n>\n> This new blank line is probably unwanted.\n\n\n> Do we want to make sure that other people will not break this fix in\n> the future by adding a few tests, perhaps to t/t5533?\n>\n> Thanks.\n>\n\nGreat idea. I'll add some tests to t/t5533, remove that blank line\nabove, and share a new patch.\n\nThanks for the review!\n\n-andrew\n"}]}