[PATCH 0/3] Clarify interaction between signed pushes and push options

[PATCH 0/3] Clarify interaction between signed pushes and push options

[Jonathan Tan]

We noticed this when trying to use Git to make a signed push with push
options to a server using JGit (which rejects such pushes because the
Git client makes requests that are, strictly speaking, incompatible with
the documented protocol).

There have been several commits (see the commits linked in the commit
messages of the patches sent in reply to this e-mail) to support push
options (that are read by receive hooks) when using "git push", but the
interaction between push options and signed pushes are not very clear.
Here are some patches (containing both code and documentation) that
clarify this interaction.

I would appreciate a review of this - if we could make the protocol
clear, we could then update JGit to use the updated protocol and be no
longer incompatible with existing Git clients.

Jonathan Tan (3):
  docs: correct receive.advertisePushOptions default
  receive-pack: verify push options in cert
  protocol docs: explain receive-pack push options

 Documentation/config.txt                  |  5 ++--
 Documentation/git-receive-pack.txt        | 10 +++++++
 Documentation/technical/pack-protocol.txt | 32 ++++++++++++++++----
 builtin/receive-pack.c                    | 49 ++++++++++++++++++++++++++++---
 t/t5534-push-signed.sh                    | 15 ++++++++++
 5 files changed, 98 insertions(+), 13 deletions(-)

-- 
2.13.0.rc1.294.g07d810a77f-goog

[PATCH 1/3] docs: correct receive.advertisePushOptions default

[Jonathan Tan]

In commit c714e45 ("receive-pack: implement advertising and receiving
push options", 2016-07-14), receive-pack was taught to (among other
things) advertise that it understood push options, depending on
configuration. It was documented that it advertised such ability by
default; however, it actually does not. (In that commit, notice that
advertise_push_options defaults to 0, unlike advertise_atomic_push which
defaults to 1.)

Update the documentation to state that it does not advertise the ability
by default.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/config.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d5..f49a2f3cb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2620,9 +2620,8 @@ receive.advertiseAtomic::
 	capability, set this variable to false.
 
 receive.advertisePushOptions::
-	By default, git-receive-pack will advertise the push options
-	capability to its clients. If you don't want to advertise this
-	capability, set this variable to false.
+	When set to true, git-receive-pack will advertise the push options
+	capability to its clients.
 
 receive.autogc::
 	By default, git-receive-pack will run "git-gc --auto" after
-- 
2.13.0.rc1.294.g07d810a77f-goog

Re: [PATCH 1/3] docs: correct receive.advertisePushOptions default

[Brandon Williams]

On 05/05, Jonathan Tan wrote:
> In commit c714e45 ("receive-pack: implement advertising and receiving
> push options", 2016-07-14), receive-pack was taught to (among other
> things) advertise that it understood push options, depending on
> configuration. It was documented that it advertised such ability by
> default; however, it actually does not. (In that commit, notice that
> advertise_push_options defaults to 0, unlike advertise_atomic_push which
> defaults to 1.)

This looks like a good fix to the documentation as advertise_push_options
does indeed default to 0.

> 
> Update the documentation to state that it does not advertise the ability
> by default.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  Documentation/config.txt | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874d5..f49a2f3cb 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2620,9 +2620,8 @@ receive.advertiseAtomic::
>  	capability, set this variable to false.
>  
>  receive.advertisePushOptions::
> -	By default, git-receive-pack will advertise the push options
> -	capability to its clients. If you don't want to advertise this
> -	capability, set this variable to false.
> +	When set to true, git-receive-pack will advertise the push options
> +	capability to its clients.
>  
>  receive.autogc::
>  	By default, git-receive-pack will run "git-gc --auto" after
> -- 
> 2.13.0.rc1.294.g07d810a77f-goog
> 

-- 
Brandon Williams

Re: [PATCH 1/3] docs: correct receive.advertisePushOptions default

[Stefan Beller]

On Fri, May 5, 2017 at 4:50 PM, Brandon Williams <bmwill@google.com> wrote:
> On 05/05, Jonathan Tan wrote:
>> In commit c714e45 ("receive-pack: implement advertising and receiving
>> push options", 2016-07-14), receive-pack was taught to (among other
>> things) advertise that it understood push options, depending on
>> configuration. It was documented that it advertised such ability by
>> default; however, it actually does not. (In that commit, notice that
>> advertise_push_options defaults to 0, unlike advertise_atomic_push which
>> defaults to 1.)
>
> This looks like a good fix to the documentation as advertise_push_options
> does indeed default to 0.
>

Indeed.

Thanks,
Stefan

Re: [PATCH 1/3] docs: correct receive.advertisePushOptions default

[Jonathan Nieder]

Jonathan Tan wrote:

> In commit c714e45 ("receive-pack: implement advertising and receiving
> push options", 2016-07-14), receive-pack was taught to (among other
> things) advertise that it understood push options, depending on
> configuration. It was documented that it advertised such ability by
> default; however, it actually does not. (In that commit, notice that
> advertise_push_options defaults to 0, unlike advertise_atomic_push which
> defaults to 1.)
>
> Update the documentation to state that it does not advertise the ability
> by default.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  Documentation/config.txt | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874d5..f49a2f3cb 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2620,9 +2620,8 @@ receive.advertiseAtomic::
>  	capability, set this variable to false.
>  
>  receive.advertisePushOptions::
> -	By default, git-receive-pack will advertise the push options
> -	capability to its clients. If you don't want to advertise this
> -	capability, set this variable to false.
> +	When set to true, git-receive-pack will advertise the push options
> +	capability to its clients.

Good catch.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Possible improvements:

- Should this also say "The default is false"?

- git-receive-pack(1) doesn't say anything about push options, so
  without more context it's hard for a new git admin taking over
  from someone who had set this up to understand what's going on.
  Should this have a pointer to the pre-receive + post-receive
  sections of githooks(5)?

- Speaking of which, should git-receive-pack(1) say something
  about push options (for example to also have a pointer to
  githooks(5))?

- git-push(1) has the same problem: when describing the -o option, it
  doesn't give a pointer to where to find more detail (though it's a
  little more helpful then this one since it includes the word "hook").

Thanks,
Jonathan

[PATCH 2/3] receive-pack: verify push options in cert

[Jonathan Tan]

In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack
was taught to include push options both within the signed cert (if the
push is a signed push) and outside the signed cert; however,
receive-pack ignores push options within the cert, only handling push
options outside the cert.

Teach receive-pack, in the case that push options are provided for a
signed push, to verify that the push options both within the cert and
outside the cert are consistent, and to provide the results of that
verification to the receive hooks as an environment variable (just like
it currently does for the nonce verification).

This sets in stone the requirement that send-pack redundantly send its
push options in 2 places, but I think that this is better than the
alternatives. Sending push options only within the cert is
backwards-incompatible with existing Git servers (which read push
options only from outside the cert), and sending push options only
outside the cert means that the push options are not signed for.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/git-receive-pack.txt | 10 ++++++++
 builtin/receive-pack.c             | 49 ++++++++++++++++++++++++++++++++++----
 t/t5534-push-signed.sh             | 15 ++++++++++++
 3 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 86a4b32f0..f50ca0f29 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -106,6 +106,16 @@ the following environment variables:
 	Also read about `receive.certNonceSlop` variable in
 	linkgit:git-config[1].
 
+`GIT_PUSH_CERT_OPTION_STATUS`::
+`BAD`;;
+	For backwards compatibility reasons, when accepting a signed
+	push, receive-pack requires that push options be written both
+	inside and outside the certificate. ("git push" does this
+	automatically.) `BAD` is returned if they are inconsistent.
+`OK`;;
+	The push options inside and outside the certificate are
+	consistent.
+
 This hook is called before any refname is updated and before any
 fast-forward checks are performed.
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f96834f42..fe26c2f72 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -81,6 +81,9 @@ static long nonce_stamp_slop;
 static unsigned long nonce_stamp_slop_limit;
 static struct ref_transaction *transaction;
 
+static const char *PUSH_OPTION_BAD = "BAD";
+static const char *PUSH_OPTION_OK = "OK";
+
 static enum {
 	KEEPALIVE_NEVER = 0,
 	KEEPALIVE_AFTER_NUL,
@@ -473,7 +476,8 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)
  * after dropping "_commit" from its name and possibly moving it out
  * of commit.c
  */
-static char *find_header(const char *msg, size_t len, const char *key)
+static char *find_header(const char *msg, size_t len, const char *key,
+			 const char **next_line)
 {
 	int key_len = strlen(key);
 	const char *line = msg;
@@ -486,6 +490,8 @@ static char *find_header(const char *msg, size_t len, const char *key)
 		if (line + key_len < eol &&
 		    !memcmp(line, key, key_len) && line[key_len] == ' ') {
 			int offset = key_len + 1;
+			if (next_line)
+				*next_line = *eol ? eol + 1 : eol;
 			return xmemdupz(line + offset, (eol - line) - offset);
 		}
 		line = *eol ? eol + 1 : NULL;
@@ -495,7 +501,7 @@ static char *find_header(const char *msg, size_t len, const char *key)
 
 static const char *check_nonce(const char *buf, size_t len)
 {
-	char *nonce = find_header(buf, len, "nonce");
+	char *nonce = find_header(buf, len, "nonce", NULL);
 	unsigned long stamp, ostamp;
 	char *bohmac, *expect = NULL;
 	const char *retval = NONCE_BAD;
@@ -575,9 +581,40 @@ static const char *check_nonce(const char *buf, size_t len)
 	return retval;
 }
 
-static void prepare_push_cert_sha1(struct child_process *proc)
+static const char *check_push_option(const char *buf, size_t len,
+				     const struct string_list *push_options)
+{
+	int options_seen = 0;
+	char *option;
+	const char *next_line;
+	const char *retval = PUSH_OPTION_OK;
+
+	while ((option = find_header(buf, len, "push-option", &next_line))) {
+		len -= (next_line - buf);
+		buf = next_line;
+		options_seen++;
+		if (options_seen > push_options->nr
+		    || strcmp(option,
+			      push_options->items[options_seen - 1].string)) {
+			retval = PUSH_OPTION_BAD;
+			goto leave;
+		}
+		free(option);
+	}
+
+	if (options_seen != push_options->nr)
+		retval = PUSH_OPTION_BAD;
+
+leave:
+	free(option);
+	return retval;
+}
+
+static void prepare_push_cert_sha1(struct child_process *proc,
+				   const struct string_list *push_options)
 {
 	static int already_done;
+	static const char *push_option_status;
 
 	if (!push_cert.len)
 		return;
@@ -609,6 +646,8 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 		strbuf_release(&gpg_output);
 		strbuf_release(&gpg_status);
 		nonce_status = check_nonce(push_cert.buf, bogs);
+		push_option_status = check_push_option(push_cert.buf, bogs,
+						       push_options);
 	}
 	if (!is_null_sha1(push_cert_sha1)) {
 		argv_array_pushf(&proc->env_array, "GIT_PUSH_CERT=%s",
@@ -631,6 +670,8 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 						 "GIT_PUSH_CERT_NONCE_SLOP=%ld",
 						 nonce_stamp_slop);
 		}
+		argv_array_pushf(&proc->env_array, "GIT_PUSH_CERT_OPTION_STATUS=%s",
+				 push_option_status);
 	}
 }
 
@@ -683,7 +724,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
 		proc.err = muxer.in;
 	}
 
-	prepare_push_cert_sha1(&proc);
+	prepare_push_cert_sha1(&proc, feed_state->push_options);
 
 	code = start_command(&proc);
 	if (code) {
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index ecb8d446a..2607b8797 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -124,6 +124,21 @@ test_expect_success GPG 'signed push sends push certificate' '
 	test_cmp expect dst/push-cert-status
 '
 
+test_expect_success GPG 'signed push reports push option status in receive hook' '
+	prepare_dst &&
+	mkdir -p dst/.git/hooks &&
+	git -C dst config receive.certnonceseed sekrit &&
+	git -C dst config receive.advertisepushoptions 1 &&
+	write_script dst/.git/hooks/post-receive <<-\EOF &&
+		# record the push option status
+		echo "$GIT_PUSH_CERT_OPTION_STATUS" > ../push-option-status
+	EOF
+
+	git push --push-option="foo" --push-option="bar" --signed dst noop ff &&
+
+	test "$(cat dst/push-option-status)" = "OK"
+'
+
 test_expect_success GPG 'fail without key and heed user.signingkey' '
 	prepare_dst &&
 	mkdir -p dst/.git/hooks &&
-- 
2.13.0.rc1.294.g07d810a77f-goog

Re: [PATCH 2/3] receive-pack: verify push options in cert

[Stefan Beller]

On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
> In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack
> was taught to include push options both within the signed cert (if the
> push is a signed push) and outside the signed cert; however,
> receive-pack ignores push options within the cert, only handling push
> options outside the cert.
>
> Teach receive-pack, in the case that push options are provided for a
> signed push, to verify that the push options both within the cert and
> outside the cert are consistent, and to provide the results of that
> verification to the receive hooks as an environment variable (just like
> it currently does for the nonce verification).
>
> This sets in stone the requirement that send-pack redundantly send its
> push options in 2 places, but I think that this is better than the
> alternatives. Sending push options only within the cert is
> backwards-incompatible with existing Git servers (which read push
> options only from outside the cert), and sending push options only
> outside the cert means that the push options are not signed for.

As the combination of push certs and push options are broken
(at least when using JGit as well, which are used in Gerrit
installations), I would not feel too bad about actually going
the backwards-incompatible way.

>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  Documentation/git-receive-pack.txt | 10 ++++++++
>  builtin/receive-pack.c             | 49 ++++++++++++++++++++++++++++++++++----
>  t/t5534-push-signed.sh             | 15 ++++++++++++
>  3 files changed, 70 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
> index 86a4b32f0..f50ca0f29 100644
> --- a/Documentation/git-receive-pack.txt
> +++ b/Documentation/git-receive-pack.txt
> @@ -106,6 +106,16 @@ the following environment variables:
>         Also read about `receive.certNonceSlop` variable in
>         linkgit:git-config[1].
>
> +`GIT_PUSH_CERT_OPTION_STATUS`::
> +`BAD`;;
> +       For backwards compatibility reasons, when accepting a signed
> +       push, receive-pack requires that push options be written both
> +       inside and outside the certificate. ("git push" does this
> +       automatically.) `BAD` is returned if they are inconsistent.
> +`OK`;;
> +       The push options inside and outside the certificate are
> +       consistent.
> +
>  This hook is called before any refname is updated and before any
>  fast-forward checks are performed.
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index f96834f42..fe26c2f72 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -81,6 +81,9 @@ static long nonce_stamp_slop;
>  static unsigned long nonce_stamp_slop_limit;
>  static struct ref_transaction *transaction;
>
> +static const char *PUSH_OPTION_BAD = "BAD";
> +static const char *PUSH_OPTION_OK = "OK";
> +
>  static enum {
>         KEEPALIVE_NEVER = 0,
>         KEEPALIVE_AFTER_NUL,
> @@ -473,7 +476,8 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)
>   * after dropping "_commit" from its name and possibly moving it out
>   * of commit.c
>   */
> -static char *find_header(const char *msg, size_t len, const char *key)
> +static char *find_header(const char *msg, size_t len, const char *key,
> +                        const char **next_line)
>  {
>         int key_len = strlen(key);
>         const char *line = msg;
> @@ -486,6 +490,8 @@ static char *find_header(const char *msg, size_t len, const char *key)
>                 if (line + key_len < eol &&
>                     !memcmp(line, key, key_len) && line[key_len] == ' ') {
>                         int offset = key_len + 1;
> +                       if (next_line)
> +                               *next_line = *eol ? eol + 1 : eol;
>                         return xmemdupz(line + offset, (eol - line) - offset);
>                 }
>                 line = *eol ? eol + 1 : NULL;
> @@ -495,7 +501,7 @@ static char *find_header(const char *msg, size_t len, const char *key)
>
>  static const char *check_nonce(const char *buf, size_t len)
>  {
> -       char *nonce = find_header(buf, len, "nonce");
> +       char *nonce = find_header(buf, len, "nonce", NULL);
>         unsigned long stamp, ostamp;
>         char *bohmac, *expect = NULL;
>         const char *retval = NONCE_BAD;
> @@ -575,9 +581,40 @@ static const char *check_nonce(const char *buf, size_t len)
>         return retval;
>  }
>
> -static void prepare_push_cert_sha1(struct child_process *proc)
> +static const char *check_push_option(const char *buf, size_t len,
> +                                    const struct string_list *push_options)
> +{
> +       int options_seen = 0;
> +       char *option;
> +       const char *next_line;
> +       const char *retval = PUSH_OPTION_OK;
> +
> +       while ((option = find_header(buf, len, "push-option", &next_line))) {
> +               len -= (next_line - buf);
> +               buf = next_line;
> +               options_seen++;
> +               if (options_seen > push_options->nr
> +                   || strcmp(option,
> +                             push_options->items[options_seen - 1].string)) {
> +                       retval = PUSH_OPTION_BAD;
> +                       goto leave;
> +               }
> +               free(option);
> +       }
> +
> +       if (options_seen != push_options->nr)
> +               retval = PUSH_OPTION_BAD;
> +
> +leave:
> +       free(option);
> +       return retval;
> +}
> +
> +static void prepare_push_cert_sha1(struct child_process *proc,
> +                                  const struct string_list *push_options)
>  {
>         static int already_done;
> +       static const char *push_option_status;
>
>         if (!push_cert.len)
>                 return;
> @@ -609,6 +646,8 @@ static void prepare_push_cert_sha1(struct child_process *proc)
>                 strbuf_release(&gpg_output);
>                 strbuf_release(&gpg_status);
>                 nonce_status = check_nonce(push_cert.buf, bogs);
> +               push_option_status = check_push_option(push_cert.buf, bogs,
> +                                                      push_options);
>         }
>         if (!is_null_sha1(push_cert_sha1)) {
>                 argv_array_pushf(&proc->env_array, "GIT_PUSH_CERT=%s",
> @@ -631,6 +670,8 @@ static void prepare_push_cert_sha1(struct child_process *proc)
>                                                  "GIT_PUSH_CERT_NONCE_SLOP=%ld",
>                                                  nonce_stamp_slop);
>                 }
> +               argv_array_pushf(&proc->env_array, "GIT_PUSH_CERT_OPTION_STATUS=%s",
> +                                push_option_status);
>         }
>  }
>
> @@ -683,7 +724,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
>                 proc.err = muxer.in;
>         }
>
> -       prepare_push_cert_sha1(&proc);
> +       prepare_push_cert_sha1(&proc, feed_state->push_options);
>
>         code = start_command(&proc);
>         if (code) {
> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
> index ecb8d446a..2607b8797 100755
> --- a/t/t5534-push-signed.sh
> +++ b/t/t5534-push-signed.sh
> @@ -124,6 +124,21 @@ test_expect_success GPG 'signed push sends push certificate' '
>         test_cmp expect dst/push-cert-status
>  '
>
> +test_expect_success GPG 'signed push reports push option status in receive hook' '
> +       prepare_dst &&
> +       mkdir -p dst/.git/hooks &&
> +       git -C dst config receive.certnonceseed sekrit &&
> +       git -C dst config receive.advertisepushoptions 1 &&
> +       write_script dst/.git/hooks/post-receive <<-\EOF &&
> +               # record the push option status
> +               echo "$GIT_PUSH_CERT_OPTION_STATUS" > ../push-option-status
> +       EOF
> +
> +       git push --push-option="foo" --push-option="bar" --signed dst noop ff &&
> +
> +       test "$(cat dst/push-option-status)" = "OK"
> +'
> +

I think in this context we'd really want to test the negative way as
well, reporting BAD?
I am unsure how easy it is to forge push options in the test, though.
The code looks good, but I only reviewed it for a minute.

Thanks,
Stefan

Re: [PATCH 2/3] receive-pack: verify push options in cert

[Brandon Williams]

On 05/05, Stefan Beller wrote:
> On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
> > In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack
> > was taught to include push options both within the signed cert (if the
> > push is a signed push) and outside the signed cert; however,
> > receive-pack ignores push options within the cert, only handling push
> > options outside the cert.
> >
> > Teach receive-pack, in the case that push options are provided for a
> > signed push, to verify that the push options both within the cert and
> > outside the cert are consistent, and to provide the results of that
> > verification to the receive hooks as an environment variable (just like
> > it currently does for the nonce verification).
> >
> > This sets in stone the requirement that send-pack redundantly send its
> > push options in 2 places, but I think that this is better than the
> > alternatives. Sending push options only within the cert is
> > backwards-incompatible with existing Git servers (which read push
> > options only from outside the cert), and sending push options only
> > outside the cert means that the push options are not signed for.
> 
> As the combination of push certs and push options are broken
> (at least when using JGit as well, which are used in Gerrit
> installations), I would not feel too bad about actually going
> the backwards-incompatible way.

yeah just think of the bits that could be saved!

But in all seriousness, I'd agree that doing the backwards-incompatible
way would be cleaner in the longer run (esp since they are broken
currently).


-- 
Brandon Williams

Re: [PATCH 2/3] receive-pack: verify push options in cert

[Jonathan Nieder]

Hi,

Stefan Beller wrote:
> On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan <jonathantanmy@google.com> wrote:

>> This sets in stone the requirement that send-pack redundantly send its
>> push options in 2 places, but I think that this is better than the
>> alternatives. Sending push options only within the cert is
>> backwards-incompatible with existing Git servers (which read push
>> options only from outside the cert), and sending push options only
>> outside the cert means that the push options are not signed for.
>
> As the combination of push certs and push options are broken
> (at least when using JGit as well, which are used in Gerrit
> installations), I would not feel too bad about actually going
> the backwards-incompatible way.

Per offline discussion, what you're proposing is to omit the second
copy of push options from the client request, so the server does not
have to see two copies.  I agree that that would be a better protocol,
but I think that ship has sailed.

Current versions of git the client and git the server are able to
interoperate without trouble (though the server-side bug is ugly in
terms of what the push certs mean).

Current versions of JGit the server *require* that the push options be
omitted from the push certificate.  I don't think there exists a
sensible way to interoperate with that, so we can ignore that JGit
server behavior as a bug (like you've said).

If we want to omit the second copy of the push certs (which sounds
like a reasonable thing to want), that would require a new capability
to be added to the protocol to do so.  That way, interoperability with
existing versions of git the client wouldn't be broken.  That could
happen on top of this series --- it is not needed for fixing the bug
that this series fixes.

To summarize:

 1. I agree that we shouldn't feel bad about breaking compatibility
    with the JGit server behavior at issue, since there is no
    reasonable way to be compatible with it.  And that's what this
    series does --- it breaks compatibility with the broken versions
    of JGit server and picks what the Git client has been doing
    instead.

 2. I don't think we should break new versions of Git's ability to
    interoperate with current versions of the server, even though I
    consider the current server behavior to be broken.

 3. If someone is interested in getting rid of the redundant second
    copy of push options, we have a way to do so, by introducing a
    new capability

Thanks,
Jonathan

Re: [PATCH 2/3] receive-pack: verify push options in cert

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack
> was taught to include push options both within the signed cert (if the
> push is a signed push) and outside the signed cert; however,
> receive-pack ignores push options within the cert, only handling push
> options outside the cert.

Yikes.  Thanks for fixing it.

[...]
> --- a/Documentation/git-receive-pack.txt
> +++ b/Documentation/git-receive-pack.txt
> @@ -106,6 +106,16 @@ the following environment variables:
>  	Also read about `receive.certNonceSlop` variable in
>  	linkgit:git-config[1].
>  
> +`GIT_PUSH_CERT_OPTION_STATUS`::
> +`BAD`;;
> +	For backwards compatibility reasons, when accepting a signed
> +	push, receive-pack requires that push options be written both
> +	inside and outside the certificate. ("git push" does this
> +	automatically.) `BAD` is returned if they are inconsistent.

In this manual page the reader doesn't need to know it's for backword
compatibility.  It is simply what the protocol requires.

The protocol doc and especially the commit message are better places
to talk about rationale (as you already are doing nicely in patch 3).

> +`OK`;;
> +	The push options inside and outside the certificate are
> +	consistent.

Hm.  I would have thought it would be simpler to simply reject the
push without invoking hooks in the BAD case.  But
GIT_PUSH_CERT_NONCE_STATUS provides precedent for this, forcing me to
think longer.

Is the idea that invoking the hook despite a bad client is a way to
provide an opportunity to audit log the error?

... okay, I've thought longer.  I think this is a different kind of
error than a bad nonce: for example, a bad nonce might be the result
of a misconfiguration that makes the client get the wrong nonce or a
sign of a caller trying to brute-force a stable nonce.  For
comparison, this error is a more simple protocol violation, like a
malformed pkt-line.

When we see a malformed pkt-line, we error out and do not invoke the
pre-receive hook.  For this error I think we should behave the same
way: show an error to the user and abort the connection, without
invoking hooks.

[...]
> --- a/t/t5534-push-signed.sh
> +++ b/t/t5534-push-signed.sh
> @@ -124,6 +124,21 @@ test_expect_success GPG 'signed push sends push certificate' '
>  	test_cmp expect dst/push-cert-status
>  '
>  
> +test_expect_success GPG 'signed push reports push option status in receive hook' '

Is there a straightforward way to test the error case?  (If there
isn't, I can live with that --- it would just be nice to have a test
that the behavior introduced here is preserved.)

Thanks and hope that helps,
Jonathan

[PATCH 3/3] protocol docs: explain receive-pack push options

[Jonathan Tan]

Support for push options in the receive-pack protocol (and all Git
components that speak it) have been added over a few commits, but not
fully documented (especially its interaction with signed pushes). Update
the protocol documentation to include the relevant details.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/technical/pack-protocol.txt | 32 +++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index 5b0ba3ef2..cf7cb48c3 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the obj-id currently on
 the server, the obj-id the client would like to update it to and the name
 of the reference.
 
-This list is followed by a flush-pkt. Then the push options are transmitted
-one per packet followed by another flush-pkt. After that the packfile that
-should contain all the objects that the server will need to complete the new
-references will be sent.
+This list is followed by a flush-pkt.
 
 ----
-  update-request    =  *shallow ( command-list | push-cert ) [packfile]
+  update-request    =  *shallow ( command-list | push-cert )
 
   shallow           =  PKT-LINE("shallow" SP obj-id)
 
@@ -500,12 +497,35 @@ references will be sent.
 		      PKT-LINE("pusher" SP ident LF)
 		      PKT-LINE("pushee" SP url LF)
 		      PKT-LINE("nonce" SP nonce LF)
+		      *PKT-LINE("push-option" SP push-option LF)
 		      PKT-LINE(LF)
 		      *PKT-LINE(command LF)
 		      *PKT-LINE(gpg-signature-lines LF)
 		      PKT-LINE("push-cert-end" LF)
 
-  packfile          = "PACK" 28*(OCTET)
+  push-option       =  1*CHAR
+----
+
+If the server has advertised the 'push-options' capability and the client has
+specified 'push-options' as part of the capability list above, the client then
+sends its push options followed by a flush-pkt.
+
+----
+  push-options      =  *PKT-LINE(push-option) flush-pkt
+----
+
+For backwards compatibility with older Git servers, if the client sends a push
+cert and push options, it SHOULD send its push options both embedded within the
+push cert and after the push cert. (Note that the push options within the cert
+are prefixed, but the push options after the cert are not.) Both these lists
+SHOULD be consistent.
+
+After that the packfile that
+should contain all the objects that the server will need to complete the new
+references will be sent.
+
+----
+  packfile          =  "PACK" 28*(OCTET)
 ----
 
 If the receiving end does not support delete-refs, the sending end MUST
-- 
2.13.0.rc1.294.g07d810a77f-goog

Re: [PATCH 3/3] protocol docs: explain receive-pack push options

[Stefan Beller]

On Fri, May 5, 2017 at 4:46 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
> Support for push options in the receive-pack protocol (and all Git
> components that speak it) have been added over a few commits, but not
> fully documented (especially its interaction with signed pushes). Update
> the protocol documentation to include the relevant details.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  Documentation/technical/pack-protocol.txt | 32 +++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
> index 5b0ba3ef2..cf7cb48c3 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the obj-id currently on
>  the server, the obj-id the client would like to update it to and the name
>  of the reference.
>
> -This list is followed by a flush-pkt. Then the push options are transmitted
> -one per packet followed by another flush-pkt. After that the packfile that
> -should contain all the objects that the server will need to complete the new
> -references will be sent.
> +This list is followed by a flush-pkt.
>
>  ----
> -  update-request    =  *shallow ( command-list | push-cert ) [packfile]
> +  update-request    =  *shallow ( command-list | push-cert )
>
>    shallow           =  PKT-LINE("shallow" SP obj-id)
>
> @@ -500,12 +497,35 @@ references will be sent.
>                       PKT-LINE("pusher" SP ident LF)
>                       PKT-LINE("pushee" SP url LF)
>                       PKT-LINE("nonce" SP nonce LF)
> +                     *PKT-LINE("push-option" SP push-option LF)
>                       PKT-LINE(LF)
>                       *PKT-LINE(command LF)
>                       *PKT-LINE(gpg-signature-lines LF)
>                       PKT-LINE("push-cert-end" LF)
>
> -  packfile          = "PACK" 28*(OCTET)
> +  push-option       =  1*CHAR
> +----
> +
> +If the server has advertised the 'push-options' capability and the client has
> +specified 'push-options' as part of the capability list above, the client then
> +sends its push options followed by a flush-pkt.

The CHAR of the push-options SHOULD NOT include an LF. Well I guess that may
be obvious when looking at PKT-LINE("push-option" SP push-option LF),
not sure if we want to mention it here.

> +
> +----
> +  push-options      =  *PKT-LINE(push-option) flush-pkt
> +----
> +
> +For backwards compatibility with older Git servers, if the client sends a push
> +cert and push options, it SHOULD send its push options both embedded within the
> +push cert and after the push cert. (Note that the push options within the cert
> +are prefixed, but the push options after the cert are not.) Both these lists
> +SHOULD be consistent.

s/SHOULD/MUST/ ?

> +
> +After that the packfile that
> +should contain all the objects that the server will need to complete the new
> +references will be sent.
> +
> +----
> +  packfile          =  "PACK" 28*(OCTET)
>  ----
>
>  If the receiving end does not support delete-refs, the sending end MUST
> --
> 2.13.0.rc1.294.g07d810a77f-goog

Thanks for writing the docs.

Stefan


>

Re: [PATCH 3/3] protocol docs: explain receive-pack push options

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> Support for push options in the receive-pack protocol (and all Git
> components that speak it) have been added over a few commits, but not
> fully documented (especially its interaction with signed pushes). Update
> the protocol documentation to include the relevant details.

Thanks.  This could be combined with the previous patch, since that
patch is enforcing what this patch documents.

[...]
> -This list is followed by a flush-pkt. Then the push options are transmitted
> -one per packet followed by another flush-pkt. After that the packfile that
> -should contain all the objects that the server will need to complete the new
> -references will be sent.
> +This list is followed by a flush-pkt.

I think this removed more than intended.

Before c714e45f (receive-pack: implement advertising and receiving
push options, 2016-07-14), this said

	This list is followed by a flush-pkt and then the packfile that should
	contain all the objects that the server will need to complete the new
	references.

which I think would work fine.

[...]
> -  update-request    =  *shallow ( command-list | push-cert ) [packfile]
> +  update-request    =  *shallow ( command-list | push-cert )

This seems confusing to me both before and after.  How does
update-request get used?  Is it supposed to include the packfile or not?

Where do push-options fit into an unsigned request?

[...]
> @@ -500,12 +497,35 @@ references will be sent.
>  		      PKT-LINE("pusher" SP ident LF)
>  		      PKT-LINE("pushee" SP url LF)
>  		      PKT-LINE("nonce" SP nonce LF)
> +		      *PKT-LINE("push-option" SP push-option LF)
>  		      PKT-LINE(LF)
>  		      *PKT-LINE(command LF)
>  		      *PKT-LINE(gpg-signature-lines LF)
>  		      PKT-LINE("push-cert-end" LF)
>  
> -  packfile          = "PACK" 28*(OCTET)
> +  push-option       =  1*CHAR
> +----
> +
> +If the server has advertised the 'push-options' capability and the client has
> +specified 'push-options' as part of the capability list above, the client then
> +sends its push options followed by a flush-pkt.
> +
> +----
> +  push-options      =  *PKT-LINE(push-option) flush-pkt
> +----
> +
> +For backwards compatibility with older Git servers, if the client sends a push
> +cert and push options, it SHOULD send its push options both embedded within the

This needs to be a MUST, not SHOULD.

> +push cert and after the push cert. (Note that the push options within the cert
> +are prefixed, but the push options after the cert are not.) Both these lists
> +SHOULD be consistent.

Likewise this one.

What does it mean to be consistent?

> +
> +After that the packfile that
> +should contain all the objects that the server will need to complete the new
> +references will be sent.
> +
> +----
> +  packfile          =  "PACK" 28*(OCTET)
>  ----

Thanks,
Jonathan

Re: [PATCH 3/3] protocol docs: explain receive-pack push options

[Jonathan Tan]

On 05/05/2017 05:26 PM, Jonathan Nieder wrote:
>> -This list is followed by a flush-pkt. Then the push options are transmitted
>> -one per packet followed by another flush-pkt. After that the packfile that
>> -should contain all the objects that the server will need to complete the new
>> -references will be sent.
>> +This list is followed by a flush-pkt.
>
> I think this removed more than intended.
>
> Before c714e45f (receive-pack: implement advertising and receiving
> push options, 2016-07-14), this said
>
> 	This list is followed by a flush-pkt and then the packfile that should
> 	contain all the objects that the server will need to complete the new
> 	references.
>
> which I think would work fine.

That wouldn't work fine because there is no mention of push options - 
hence the modification in c714e45f to talk about push options, but that 
is also not accurate because push options (and, more importantly, the 
associated flush-pkt) are not sent if the client doesn't have any.

> [...]
>> -  update-request    =  *shallow ( command-list | push-cert ) [packfile]
>> +  update-request    =  *shallow ( command-list | push-cert )
>
> This seems confusing to me both before and after.  How does
> update-request get used?  Is it supposed to include the packfile or not?
>
> Where do push-options fit into an unsigned request?

I've updated "update-request" to "update-requests" to show that this is 
the "list of reference update requests" mentioned in the previous paragraph.

This is only the ref part - I've chosen to describe push options and 
packfile in separate sections, because there are very specific rules 
that determine whether the push option section must be included or omitted.

>
> [...]
>> @@ -500,12 +497,35 @@ references will be sent.
>>  		      PKT-LINE("pusher" SP ident LF)
>>  		      PKT-LINE("pushee" SP url LF)
>>  		      PKT-LINE("nonce" SP nonce LF)
>> +		      *PKT-LINE("push-option" SP push-option LF)
>>  		      PKT-LINE(LF)
>>  		      *PKT-LINE(command LF)
>>  		      *PKT-LINE(gpg-signature-lines LF)
>>  		      PKT-LINE("push-cert-end" LF)
>>
>> -  packfile          = "PACK" 28*(OCTET)
>> +  push-option       =  1*CHAR
>> +----
>> +
>> +If the server has advertised the 'push-options' capability and the client has
>> +specified 'push-options' as part of the capability list above, the client then
>> +sends its push options followed by a flush-pkt.
>> +
>> +----
>> +  push-options      =  *PKT-LINE(push-option) flush-pkt
>> +----
>> +
>> +For backwards compatibility with older Git servers, if the client sends a push
>> +cert and push options, it SHOULD send its push options both embedded within the
>
> This needs to be a MUST, not SHOULD.
>
>> +push cert and after the push cert. (Note that the push options within the cert
>> +are prefixed, but the push options after the cert are not.) Both these lists
>> +SHOULD be consistent.
>
> Likewise this one.
>
> What does it mean to be consistent?

Changed to "MUST be the same, modulo the prefix".

Re: [PATCH 0/3] Clarify interaction between signed pushes and push options

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> We noticed this when trying to use Git to make a signed push with push
> options to a server using JGit (which rejects such pushes because the
> Git client makes requests that are, strictly speaking, incompatible with
> the documented protocol).
>
> There have been several commits (see the commits linked in the commit
> messages of the patches sent in reply to this e-mail) to support push
> options (that are read by receive hooks) when using "git push", but the
> interaction between push options and signed pushes are not very clear.
> Here are some patches (containing both code and documentation) that
> clarify this interaction.
>
> I would appreciate a review of this - if we could make the protocol
> clear, we could then update JGit to use the updated protocol and be no
> longer incompatible with existing Git clients.

I've seen the exchanges on list on these three patches, and I agree
with what Jonathan said.

These are going in the right direction in general but a few nits
need to be picked before becoming ready for 'next'.

Thanks.

>
> Jonathan Tan (3):
>   docs: correct receive.advertisePushOptions default
>   receive-pack: verify push options in cert
>   protocol docs: explain receive-pack push options
>
>  Documentation/config.txt                  |  5 ++--
>  Documentation/git-receive-pack.txt        | 10 +++++++
>  Documentation/technical/pack-protocol.txt | 32 ++++++++++++++++----
>  builtin/receive-pack.c                    | 49 ++++++++++++++++++++++++++++---
>  t/t5534-push-signed.sh                    | 15 ++++++++++
>  5 files changed, 98 insertions(+), 13 deletions(-)

[PATCH v2 0/2] Clarify interaction between signed pushes and push options

[Jonathan Tan]

Changes from v1:
 - merged last 2 patches
 - patch 1:
   - updated advertisePushOptions doc to say "False by default"
 - patch 2 (formerly 2-3):
   - reject mismatching push options (similar to how a pre-receive hook
     can reject a push) instead of merely reporting it
   - added test to check failure (in addition to success)
   - modified pack-protocol.txt to describe new behavior

Jonathan Tan (2):
  docs: correct receive.advertisePushOptions default
  receive-pack: verify push options in cert

 Documentation/config.txt                  |  5 ++-
 Documentation/technical/pack-protocol.txt | 32 +++++++++++++++----
 builtin/receive-pack.c                    | 51 +++++++++++++++++++++++++++++--
 t/t5534-push-signed.sh                    | 37 ++++++++++++++++++++++
 4 files changed, 114 insertions(+), 11 deletions(-)

-- 
2.13.0.rc1.294.g07d810a77f-goog

[PATCH v2 1/2] docs: correct receive.advertisePushOptions default

[Jonathan Tan]

In commit c714e45 ("receive-pack: implement advertising and receiving
push options", 2016-07-14), receive-pack was taught to (among other
things) advertise that it understood push options, depending on
configuration. It was documented that it advertised such ability by
default; however, it actually does not. (In that commit, notice that
advertise_push_options defaults to 0, unlike advertise_atomic_push which
defaults to 1.)

Update the documentation to state that it does not advertise the ability
by default.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/config.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d5..3a847a62e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2620,9 +2620,8 @@ receive.advertiseAtomic::
 	capability, set this variable to false.
 
 receive.advertisePushOptions::
-	By default, git-receive-pack will advertise the push options
-	capability to its clients. If you don't want to advertise this
-	capability, set this variable to false.
+	When set to true, git-receive-pack will advertise the push options
+	capability to its clients. False by default.
 
 receive.autogc::
 	By default, git-receive-pack will run "git-gc --auto" after
-- 
2.13.0.rc1.294.g07d810a77f-goog

[PATCH v2 2/2] receive-pack: verify push options in cert

[Jonathan Tan]

In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack
was taught to include push options both within the signed cert (if the
push is a signed push) and outside the signed cert; however,
receive-pack ignores push options within the cert, only handling push
options outside the cert.

Teach receive-pack, in the case that push options are provided for a
signed push, to verify that the push options both within the cert and
outside the cert are consistent.

This sets in stone the requirement that send-pack redundantly send its
push options in 2 places, but I think that this is better than the
alternatives. Sending push options only within the cert is
backwards-incompatible with existing Git servers (which read push
options only from outside the cert), and sending push options only
outside the cert means that the push options are not signed for.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/technical/pack-protocol.txt | 32 +++++++++++++++----
 builtin/receive-pack.c                    | 51 +++++++++++++++++++++++++++++--
 t/t5534-push-signed.sh                    | 37 ++++++++++++++++++++++
 3 files changed, 112 insertions(+), 8 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index 5b0ba3ef2..a34917153 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the obj-id currently on
 the server, the obj-id the client would like to update it to and the name
 of the reference.
 
-This list is followed by a flush-pkt. Then the push options are transmitted
-one per packet followed by another flush-pkt. After that the packfile that
-should contain all the objects that the server will need to complete the new
-references will be sent.
+This list is followed by a flush-pkt.
 
 ----
-  update-request    =  *shallow ( command-list | push-cert ) [packfile]
+  update-requests   =  *shallow ( command-list | push-cert )
 
   shallow           =  PKT-LINE("shallow" SP obj-id)
 
@@ -500,12 +497,35 @@ references will be sent.
 		      PKT-LINE("pusher" SP ident LF)
 		      PKT-LINE("pushee" SP url LF)
 		      PKT-LINE("nonce" SP nonce LF)
+		      *PKT-LINE("push-option" SP push-option LF)
 		      PKT-LINE(LF)
 		      *PKT-LINE(command LF)
 		      *PKT-LINE(gpg-signature-lines LF)
 		      PKT-LINE("push-cert-end" LF)
 
-  packfile          = "PACK" 28*(OCTET)
+  push-option       =  1*( VCHAR | SP )
+----
+
+If the server has advertised the 'push-options' capability and the client has
+specified 'push-options' as part of the capability list above, the client then
+sends its push options followed by a flush-pkt.
+
+----
+  push-options      =  *PKT-LINE(push-option) flush-pkt
+----
+
+For backwards compatibility with older Git servers, if the client sends a push
+cert and push options, it MUST send its push options both embedded within the
+push cert and after the push cert. (Note that the push options within the cert
+are prefixed, but the push options after the cert are not.) Both these lists
+MUST be the same, modulo the prefix.
+
+After that the packfile that
+should contain all the objects that the server will need to complete the new
+references will be sent.
+
+----
+  packfile          =  "PACK" 28*(OCTET)
 ----
 
 If the receiving end does not support delete-refs, the sending end MUST
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f96834f42..fff5c7a54 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -473,7 +473,8 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)
  * after dropping "_commit" from its name and possibly moving it out
  * of commit.c
  */
-static char *find_header(const char *msg, size_t len, const char *key)
+static char *find_header(const char *msg, size_t len, const char *key,
+			 const char **next_line)
 {
 	int key_len = strlen(key);
 	const char *line = msg;
@@ -486,6 +487,8 @@ static char *find_header(const char *msg, size_t len, const char *key)
 		if (line + key_len < eol &&
 		    !memcmp(line, key, key_len) && line[key_len] == ' ') {
 			int offset = key_len + 1;
+			if (next_line)
+				*next_line = *eol ? eol + 1 : eol;
 			return xmemdupz(line + offset, (eol - line) - offset);
 		}
 		line = *eol ? eol + 1 : NULL;
@@ -495,7 +498,7 @@ static char *find_header(const char *msg, size_t len, const char *key)
 
 static const char *check_nonce(const char *buf, size_t len)
 {
-	char *nonce = find_header(buf, len, "nonce");
+	char *nonce = find_header(buf, len, "nonce", NULL);
 	unsigned long stamp, ostamp;
 	char *bohmac, *expect = NULL;
 	const char *retval = NONCE_BAD;
@@ -575,6 +578,45 @@ static const char *check_nonce(const char *buf, size_t len)
 	return retval;
 }
 
+/*
+ * Return 1 if there is no push_cert or if the push options in push_cert are
+ * the same as those in the argument; 0 otherwise.
+ */
+static int check_cert_push_options(const struct string_list *push_options)
+{
+	const char *buf = push_cert.buf;
+	int len = push_cert.len;
+
+	char *option;
+	const char *next_line;
+	int options_seen = 0;
+
+	int retval = 1;
+
+	if (!len)
+		return 1;
+
+	while ((option = find_header(buf, len, "push-option", &next_line))) {
+		len -= (next_line - buf);
+		buf = next_line;
+		options_seen++;
+		if (options_seen > push_options->nr
+		    || strcmp(option,
+			      push_options->items[options_seen - 1].string)) {
+			retval = 0;
+			goto leave;
+		}
+		free(option);
+	}
+
+	if (options_seen != push_options->nr)
+		retval = 0;
+
+leave:
+	free(option);
+	return retval;
+}
+
 static void prepare_push_cert_sha1(struct child_process *proc)
 {
 	static int already_done;
@@ -1929,6 +1971,11 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 
 		if (use_push_options)
 			read_push_options(&push_options);
+		if (!check_cert_push_options(&push_options)) {
+			struct command *cmd;
+			for (cmd = commands; cmd; cmd = cmd->next)
+				cmd->error_string = "inconsistent push options";
+		}
 
 		prepare_shallow_info(&si, &shallow);
 		if (!si.nr_ours && !si.nr_theirs)
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index ecb8d446a..0ef6f66b5 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -124,6 +124,43 @@ test_expect_success GPG 'signed push sends push certificate' '
 	test_cmp expect dst/push-cert-status
 '
 
+test_expect_success GPG 'inconsistent push options in signed push not allowed' '
+	# First, invoke receive-pack with dummy input to obtain its preamble.
+	prepare_dst &&
+	git -C dst config receive.certnonceseed sekrit &&
+	git -C dst config receive.advertisepushoptions 1 &&
+	printf xxxx | test_might_fail git receive-pack dst >preamble &&
+
+	# Then, invoke push. Simulate a receive-pack that sends the preamble we
+	# obtained, followed by a dummy packet.
+	write_script myscript <<-\EOF &&
+		cat preamble &&
+		printf xxxx &&
+		cat >push
+	EOF
+	test_might_fail git push --push-option="foo" --push-option="bar" \
+		--receive-pack="\"$(pwd)/myscript\"" --signed dst --delete ff &&
+
+	# Replay the push output on a fresh dst, checking that ff is truly
+	# deleted.
+	prepare_dst &&
+	git -C dst config receive.certnonceseed sekrit &&
+	git -C dst config receive.advertisepushoptions 1 &&
+	git receive-pack dst <push &&
+	test_must_fail git -C dst rev-parse ff &&
+
+	# Tweak the push output to make the push option outside the cert
+	# different, then replay it on a fresh dst, checking that ff is not
+	# deleted.
+	sed -i "s/\\([^ ]\\)bar/\\1baz/" push &&
+	prepare_dst &&
+	git -C dst config receive.certnonceseed sekrit &&
+	git -C dst config receive.advertisepushoptions 1 &&
+	git receive-pack dst <push >out &&
+	git -C dst rev-parse ff &&
+	grep "inconsistent push options" out
+'
+
 test_expect_success GPG 'fail without key and heed user.signingkey' '
 	prepare_dst &&
 	mkdir -p dst/.git/hooks &&
-- 
2.13.0.rc1.294.g07d810a77f-goog

Re: [PATCH v2 2/2] receive-pack: verify push options in cert

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Teach receive-pack, in the case that push options are provided for a
> signed push, to verify that the push options both within the cert and
> outside the cert are consistent.

Thanks.  The idea was that the certificate should record how the
push was made fully, hence we need two copies.  The one outside the
certificate is meant to be actually used, but obviously we need to
make sure that matches what is recorded in the certificate.

In retrospect, we could have required the receiver who groks signed
pushes to only look inside the certificate for options etc. so that
the sender can omit the "extra" copies outside the certificate, but
that is not how the current protocol is structured, hence ...

> This sets in stone the requirement that send-pack redundantly send its
> push options in 2 places,...

... this requirement.

Thanks.

Re: [PATCH v2 2/2] receive-pack: verify push options in cert

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> +	# Tweak the push output to make the push option outside the cert
> +	# different, then replay it on a fresh dst, checking that ff is not
> +	# deleted.
> +	sed -i "s/\\([^ ]\\)bar/\\1baz/" push &&

This is not portable.  Didn't you get an error from the lint
checker?

The attached may not be enough if "push" is a binary file, though,
in which case perl may be your friend ;-)

 t/t5534-push-signed.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 0ef6f66b5d..effe769688 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -152,11 +152,11 @@ test_expect_success GPG 'inconsistent push options in signed push not allowed' '
 	# Tweak the push output to make the push option outside the cert
 	# different, then replay it on a fresh dst, checking that ff is not
 	# deleted.
-	sed -i "s/\\([^ ]\\)bar/\\1baz/" push &&
+	sed -e "s/\\([^ ]\\)bar/\\1baz/" push >push.tweak &&
 	prepare_dst &&
 	git -C dst config receive.certnonceseed sekrit &&
 	git -C dst config receive.advertisepushoptions 1 &&
-	git receive-pack dst <push >out &&
+	git receive-pack dst <push.tweak >out &&
 	git -C dst rev-parse ff &&
 	grep "inconsistent push options" out
 '

[PATCH] fixup! use perl instead of sed

[Jonathan Tan]

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---

Thanks - I didn't realize the existence of the test lint. Here's a
fixup. Let me know if you prefer a full reroll.

I had to use perl because "push" is a binary file (the first line
contains a NUL).


 t/t5534-push-signed.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 0ef6f66b5..3ee58dafb 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -152,11 +152,11 @@ test_expect_success GPG 'inconsistent push options in signed push not allowed' '
 	# Tweak the push output to make the push option outside the cert
 	# different, then replay it on a fresh dst, checking that ff is not
 	# deleted.
-	sed -i "s/\\([^ ]\\)bar/\\1baz/" push &&
+	perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak &&
 	prepare_dst &&
 	git -C dst config receive.certnonceseed sekrit &&
 	git -C dst config receive.advertisepushoptions 1 &&
-	git receive-pack dst <push >out &&
+	git receive-pack dst <push.tweak >out &&
 	git -C dst rev-parse ff &&
 	grep "inconsistent push options" out
 '
-- 
2.13.0.rc2.291.g57267f2277-goog

Re: [PATCH] fixup! use perl instead of sed

[Ævar Arnfjörð Bjarmason]

On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>
> Thanks - I didn't realize the existence of the test lint. Here's a
> fixup. Let me know if you prefer a full reroll.
>
> I had to use perl because "push" is a binary file (the first line
> contains a NUL).
>
>
>  t/t5534-push-signed.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
> index 0ef6f66b5..3ee58dafb 100755
> --- a/t/t5534-push-signed.sh
> +++ b/t/t5534-push-signed.sh
> @@ -152,11 +152,11 @@ test_expect_success GPG 'inconsistent push options in signed push not allowed' '
>         # Tweak the push output to make the push option outside the cert
>         # different, then replay it on a fresh dst, checking that ff is not
>         # deleted.
> -       sed -i "s/\\([^ ]\\)bar/\\1baz/" push &&
> +       perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak &&
>         prepare_dst &&
>         git -C dst config receive.certnonceseed sekrit &&
>         git -C dst config receive.advertisepushoptions 1 &&
> -       git receive-pack dst <push >out &&
> +       git receive-pack dst <push.tweak >out &&

The test should have a PERL prerequisite now, that's missing.

Also using \1 will likely be deprecated in future versions of perl at
some point:

    $ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/"
    \1 better written as $1 at -e line 1.
    hifoo

Finally, you can just use -i like you did with sed, no need for the tempfile:

    $ echo hibar >push
    $ perl -pi -e 's/([^ ])bar/$1baz/' push
    $ cat push
    hibaz

>         git -C dst rev-parse ff &&
>         grep "inconsistent push options" out
>  '
> --
> 2.13.0.rc2.291.g57267f2277-goog
>

[PATCH v3 0/2] Clarify interaction between signed pushes and push options

[Jonathan Tan]

Changes from v2:
 - incorporated Junio's suggestions
 - incorporated Ævar's suggestions

Jonathan Tan (2):
  docs: correct receive.advertisePushOptions default
  receive-pack: verify push options in cert

 Documentation/config.txt                  |  5 ++-
 Documentation/technical/pack-protocol.txt | 32 +++++++++++++++----
 builtin/receive-pack.c                    | 51 +++++++++++++++++++++++++++++--
 t/t5534-push-signed.sh                    | 37 ++++++++++++++++++++++
 4 files changed, 114 insertions(+), 11 deletions(-)

-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH v3 1/2] docs: correct receive.advertisePushOptions default

[Jonathan Tan]

In commit c714e45 ("receive-pack: implement advertising and receiving
push options", 2016-07-14), receive-pack was taught to (among other
things) advertise that it understood push options, depending on
configuration. It was documented that it advertised such ability by
default; however, it actually does not. (In that commit, notice that
advertise_push_options defaults to 0, unlike advertise_atomic_push which
defaults to 1.)

Update the documentation to state that it does not advertise the ability
by default.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/config.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d5..3a847a62e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2620,9 +2620,8 @@ receive.advertiseAtomic::
 	capability, set this variable to false.
 
 receive.advertisePushOptions::
-	By default, git-receive-pack will advertise the push options
-	capability to its clients. If you don't want to advertise this
-	capability, set this variable to false.
+	When set to true, git-receive-pack will advertise the push options
+	capability to its clients. False by default.
 
 receive.autogc::
 	By default, git-receive-pack will run "git-gc --auto" after
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH v3 2/2] receive-pack: verify push options in cert

[Jonathan Tan]

In commit f6a4e61 ("push: accept push options", 2016-07-14), send-pack
was taught to include push options both within the signed cert (if the
push is a signed push) and outside the signed cert; however,
receive-pack ignores push options within the cert, only handling push
options outside the cert.

Teach receive-pack, in the case that push options are provided for a
signed push, to verify that the push options both within the cert and
outside the cert are consistent.

This sets in stone the requirement that send-pack redundantly send its
push options in 2 places, but I think that this is better than the
alternatives. Sending push options only within the cert is
backwards-incompatible with existing Git servers (which read push
options only from outside the cert), and sending push options only
outside the cert means that the push options are not signed for.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/technical/pack-protocol.txt | 32 +++++++++++++++----
 builtin/receive-pack.c                    | 51 +++++++++++++++++++++++++++++--
 t/t5534-push-signed.sh                    | 37 ++++++++++++++++++++++
 3 files changed, 112 insertions(+), 8 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index 5b0ba3ef2..a34917153 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -473,13 +473,10 @@ that it wants to update, it sends a line listing the obj-id currently on
 the server, the obj-id the client would like to update it to and the name
 of the reference.
 
-This list is followed by a flush-pkt. Then the push options are transmitted
-one per packet followed by another flush-pkt. After that the packfile that
-should contain all the objects that the server will need to complete the new
-references will be sent.
+This list is followed by a flush-pkt.
 
 ----
-  update-request    =  *shallow ( command-list | push-cert ) [packfile]
+  update-requests   =  *shallow ( command-list | push-cert )
 
   shallow           =  PKT-LINE("shallow" SP obj-id)
 
@@ -500,12 +497,35 @@ references will be sent.
 		      PKT-LINE("pusher" SP ident LF)
 		      PKT-LINE("pushee" SP url LF)
 		      PKT-LINE("nonce" SP nonce LF)
+		      *PKT-LINE("push-option" SP push-option LF)
 		      PKT-LINE(LF)
 		      *PKT-LINE(command LF)
 		      *PKT-LINE(gpg-signature-lines LF)
 		      PKT-LINE("push-cert-end" LF)
 
-  packfile          = "PACK" 28*(OCTET)
+  push-option       =  1*( VCHAR | SP )
+----
+
+If the server has advertised the 'push-options' capability and the client has
+specified 'push-options' as part of the capability list above, the client then
+sends its push options followed by a flush-pkt.
+
+----
+  push-options      =  *PKT-LINE(push-option) flush-pkt
+----
+
+For backwards compatibility with older Git servers, if the client sends a push
+cert and push options, it MUST send its push options both embedded within the
+push cert and after the push cert. (Note that the push options within the cert
+are prefixed, but the push options after the cert are not.) Both these lists
+MUST be the same, modulo the prefix.
+
+After that the packfile that
+should contain all the objects that the server will need to complete the new
+references will be sent.
+
+----
+  packfile          =  "PACK" 28*(OCTET)
 ----
 
 If the receiving end does not support delete-refs, the sending end MUST
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f96834f42..fff5c7a54 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -473,7 +473,8 @@ static char *prepare_push_cert_nonce(const char *path, unsigned long stamp)
  * after dropping "_commit" from its name and possibly moving it out
  * of commit.c
  */
-static char *find_header(const char *msg, size_t len, const char *key)
+static char *find_header(const char *msg, size_t len, const char *key,
+			 const char **next_line)
 {
 	int key_len = strlen(key);
 	const char *line = msg;
@@ -486,6 +487,8 @@ static char *find_header(const char *msg, size_t len, const char *key)
 		if (line + key_len < eol &&
 		    !memcmp(line, key, key_len) && line[key_len] == ' ') {
 			int offset = key_len + 1;
+			if (next_line)
+				*next_line = *eol ? eol + 1 : eol;
 			return xmemdupz(line + offset, (eol - line) - offset);
 		}
 		line = *eol ? eol + 1 : NULL;
@@ -495,7 +498,7 @@ static char *find_header(const char *msg, size_t len, const char *key)
 
 static const char *check_nonce(const char *buf, size_t len)
 {
-	char *nonce = find_header(buf, len, "nonce");
+	char *nonce = find_header(buf, len, "nonce", NULL);
 	unsigned long stamp, ostamp;
 	char *bohmac, *expect = NULL;
 	const char *retval = NONCE_BAD;
@@ -575,6 +578,45 @@ static const char *check_nonce(const char *buf, size_t len)
 	return retval;
 }
 
+/*
+ * Return 1 if there is no push_cert or if the push options in push_cert are
+ * the same as those in the argument; 0 otherwise.
+ */
+static int check_cert_push_options(const struct string_list *push_options)
+{
+	const char *buf = push_cert.buf;
+	int len = push_cert.len;
+
+	char *option;
+	const char *next_line;
+	int options_seen = 0;
+
+	int retval = 1;
+
+	if (!len)
+		return 1;
+
+	while ((option = find_header(buf, len, "push-option", &next_line))) {
+		len -= (next_line - buf);
+		buf = next_line;
+		options_seen++;
+		if (options_seen > push_options->nr
+		    || strcmp(option,
+			      push_options->items[options_seen - 1].string)) {
+			retval = 0;
+			goto leave;
+		}
+		free(option);
+	}
+
+	if (options_seen != push_options->nr)
+		retval = 0;
+
+leave:
+	free(option);
+	return retval;
+}
+
 static void prepare_push_cert_sha1(struct child_process *proc)
 {
 	static int already_done;
@@ -1929,6 +1971,11 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 
 		if (use_push_options)
 			read_push_options(&push_options);
+		if (!check_cert_push_options(&push_options)) {
+			struct command *cmd;
+			for (cmd = commands; cmd; cmd = cmd->next)
+				cmd->error_string = "inconsistent push options";
+		}
 
 		prepare_shallow_info(&si, &shallow);
 		if (!si.nr_ours && !si.nr_theirs)
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index ecb8d446a..177b933a7 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -124,6 +124,43 @@ test_expect_success GPG 'signed push sends push certificate' '
 	test_cmp expect dst/push-cert-status
 '
 
+test_expect_success GPG,PERL 'inconsistent push options in signed push not allowed' '
+	# First, invoke receive-pack with dummy input to obtain its preamble.
+	prepare_dst &&
+	git -C dst config receive.certnonceseed sekrit &&
+	git -C dst config receive.advertisepushoptions 1 &&
+	printf xxxx | test_might_fail git receive-pack dst >preamble &&
+
+	# Then, invoke push. Simulate a receive-pack that sends the preamble we
+	# obtained, followed by a dummy packet.
+	write_script myscript <<-\EOF &&
+		cat preamble &&
+		printf xxxx &&
+		cat >push
+	EOF
+	test_might_fail git push --push-option="foo" --push-option="bar" \
+		--receive-pack="\"$(pwd)/myscript\"" --signed dst --delete ff &&
+
+	# Replay the push output on a fresh dst, checking that ff is truly
+	# deleted.
+	prepare_dst &&
+	git -C dst config receive.certnonceseed sekrit &&
+	git -C dst config receive.advertisepushoptions 1 &&
+	git receive-pack dst <push &&
+	test_must_fail git -C dst rev-parse ff &&
+
+	# Tweak the push output to make the push option outside the cert
+	# different, then replay it on a fresh dst, checking that ff is not
+	# deleted.
+	perl -pi -e "s/([^ ])bar/\$1baz/" push &&
+	prepare_dst &&
+	git -C dst config receive.certnonceseed sekrit &&
+	git -C dst config receive.advertisepushoptions 1 &&
+	git receive-pack dst <push >out &&
+	git -C dst rev-parse ff &&
+	grep "inconsistent push options" out
+'
+
 test_expect_success GPG 'fail without key and heed user.signingkey' '
 	prepare_dst &&
 	mkdir -p dst/.git/hooks &&
-- 
2.13.0.rc2.291.g57267f2277-goog

[PATCH v3] fixup! don't use perl -i because it's not portable

[Jonathan Tan]

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---

Ah...thanks, Johannes, for spotting this. Here's a fixup.

 t/t5534-push-signed.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 177b933a7..807267b7f 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -152,11 +152,11 @@ test_expect_success GPG,PERL 'inconsistent push options in signed push not allow
 	# Tweak the push output to make the push option outside the cert
 	# different, then replay it on a fresh dst, checking that ff is not
 	# deleted.
-	perl -pi -e "s/([^ ])bar/\$1baz/" push &&
+	perl -pe "s/([^ ])bar/\$1baz/" push >push.tweak &&
 	prepare_dst &&
 	git -C dst config receive.certnonceseed sekrit &&
 	git -C dst config receive.advertisepushoptions 1 &&
-	git receive-pack dst <push >out &&
+	git receive-pack dst <push.tweak >out &&
 	git -C dst rev-parse ff &&
 	grep "inconsistent push options" out
 '
-- 
2.13.0.rc2.291.g57267f2277-goog

Re: [PATCH] fixup! use perl instead of sed

[Johannes Sixt]

Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason:
> Finally, you can just use -i like you did with sed, no need for the tempfile:

Nope. Some implementations of perl attempt to remove the file that it 
has just opened. That doesn't work on Windows. You have to supply a 
backup file name as in `perl -i.bak ...` :-(

> 
>      $ echo hibar >push
>      $ perl -pi -e 's/([^ ])bar/$1baz/' push
>      $ cat push
>      hibaz

-- Hannes

Re: [PATCH] fixup! use perl instead of sed

[Jonathan Tan]

On 05/09/2017 01:43 PM, Johannes Sixt wrote:
> Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason:
>> Finally, you can just use -i like you did with sed, no need for the
>> tempfile:
>
> Nope. Some implementations of perl attempt to remove the file that it
> has just opened. That doesn't work on Windows. You have to supply a
> backup file name as in `perl -i.bak ...` :-(
>
>>
>>      $ echo hibar >push
>>      $ perl -pi -e 's/([^ ])bar/$1baz/' push
>>      $ cat push
>>      hibaz
>
> -- Hannes

Thanks - sent a fixup [1] in reply to my PATCH v3 cover letter (but 
forgot to CC you).

[1] <20170509210158.17898-1-jonathantanmy@google.com>

Re: [PATCH] fixup! use perl instead of sed

[Ævar Arnfjörð Bjarmason]

On Tue, May 9, 2017 at 10:43 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason:
>>
>> Finally, you can just use -i like you did with sed, no need for the
>> tempfile:
>
>
> Nope. Some implementations of perl attempt to remove the file that it has
> just opened. That doesn't work on Windows. You have to supply a backup file
> name as in `perl -i.bak ...` :-(

This should have been fixed in 2002, and is in 5.8, the oldest perl
release we support: https://github.com/Perl/perl5/commit/c030f24b81 &
http://www.nntp.perl.org/group/perl.perl5.porters/2002/05/msg60275.html

But maybe __CYGWIN__ isn't always defined on Windows, maybe this was a
mingw build or something and perl was missing a test for this when
support for that was added?

This is obviously a trivial issue for git, but if it's a bug in perl
I'd like to fix that.

>>
>>      $ echo hibar >push
>>      $ perl -pi -e 's/([^ ])bar/$1baz/' push
>>      $ cat push
>>      hibaz
>
>
> -- Hannes

Re: [PATCH] fixup! use perl instead of sed

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
> ...
>>         # Tweak the push output to make the push option outside the cert
>>         # different, then replay it on a fresh dst, checking that ff is not
>>         # deleted.
>> -       sed -i "s/\\([^ ]\\)bar/\\1baz/" push &&
>> +       perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak &&
>>         prepare_dst &&
>>         git -C dst config receive.certnonceseed sekrit &&
>>         git -C dst config receive.advertisepushoptions 1 &&
>> -       git receive-pack dst <push >out &&
>> +       git receive-pack dst <push.tweak >out &&
>
> The test should have a PERL prerequisite now, that's missing.

For a single-liner like this, our stance has always been that t/
scripts can assume _some_ version of Perl interpreter available for
use, cf. t/README where it lists prerequisites and explains them:

     - PERL

       Git wasn't compiled with NO_PERL=YesPlease.

       Even without the PERL prerequisite, tests can assume there is a
       usable perl interpreter at $PERL_PATH, though it need not be
       particularly modern.

So unless "receive-pack" that is being tested here requires Perl at
runtime, we do not want PERL prerequisite for this test.

> Also using \1 will likely be deprecated in future versions of perl at
> some point:
>
>     $ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/"
>     \1 better written as $1 at -e line 1.
>     hifoo

Very good advice from a Perl expert; thanks.

Re: [PATCH] fixup! use perl instead of sed

[Ævar Arnfjörð Bjarmason]

On Wed, May 10, 2017 at 12:38 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Tue, May 9, 2017 at 6:45 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
>> ...
>>>         # Tweak the push output to make the push option outside the cert
>>>         # different, then replay it on a fresh dst, checking that ff is not
>>>         # deleted.
>>> -       sed -i "s/\\([^ ]\\)bar/\\1baz/" push &&
>>> +       perl -pe "s/([^ ])bar/\\1baz/" push >push.tweak &&
>>>         prepare_dst &&
>>>         git -C dst config receive.certnonceseed sekrit &&
>>>         git -C dst config receive.advertisepushoptions 1 &&
>>> -       git receive-pack dst <push >out &&
>>> +       git receive-pack dst <push.tweak >out &&
>>
>> The test should have a PERL prerequisite now, that's missing.
>
> For a single-liner like this, our stance has always been that t/
> scripts can assume _some_ version of Perl interpreter available for
> use, cf. t/README where it lists prerequisites and explains them:
>
>      - PERL
>
>        Git wasn't compiled with NO_PERL=YesPlease.
>
>        Even without the PERL prerequisite, tests can assume there is a
>        usable perl interpreter at $PERL_PATH, though it need not be
>        particularly modern.
>
> So unless "receive-pack" that is being tested here requires Perl at
> runtime, we do not want PERL prerequisite for this test.

Oops, sorry about that.

>> Also using \1 will likely be deprecated in future versions of perl at
>> some point:
>>
>>     $ echo hifoo | perl -wpe "s/([^ ])bar/\\1baz/"
>>     \1 better written as $1 at -e line 1.
>>     hifoo
>
> Very good advice from a Perl expert; thanks.

No problem. Hopefully my noisy advice will at least lead to fixing a
bug in perl if there is one :)