git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 3/5] git push: verify refs early

From
Junio C Hamano <gitster@pobox.com>
Date
Feb 13, 2012, 22:16 UTC
Message-ID
<7v4nuucrbm.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1329164235-29955-4-git-send-email-drizzd@aon.at>
Clemens Buchacher <drizzd@aon.at> writes:
> I suppose with some effort, this could be done for smart HTTP as well.
> But I am not sure if we actually want the overhead of the additional
> ping-pong for HTTP.
Hrm, I am confused.

The updated protocol exchange, if I am reading your patch correctly, would go like this (S stands for the sender, R for the receiver):

        R: Here are the tips of my refs
        ----
        S: I'd like to update your refs this way
        ----
      + R: No you cannot because all updates will fail, go away
        or
      + R: You may proceed, as some updates may succeed
        ----
        S: Here is the packfile
        ----
        R: Here is how I processed your request

Given that this makes the sender stall for both smart HTTP and native protocol, don't your worries about the additional ping-pong apply equally to both transports?

If it is not worth doing for smart HTTP, I wonder if it is worth doing for native transport. After all, "all updates will fail" is hopefully the less likely case, and with this protocol extension, we end up penalizing the common case with an extra stall for everybody, regardless of the transport.

I dunno.

All other patches in this "series" looked nice fixes and improvements, but I am not sure about this change. At least I am not yet convinced.

Show 258 quoted lines
>  builtin/receive-pack.c |   83 ++++++++++++++++++++++++++++++++++++++----------
>  builtin/send-pack.c    |   43 +++++++++++++++++--------
>  send-pack.h            |    3 +-
>  3 files changed, 97 insertions(+), 32 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 0afb8b2..0129d9c 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -34,6 +34,8 @@ static int unpack_limit = 100;
>  static int report_status;
>  static int use_sideband;
>  static int quiet;
> +static int verify_refs;
> +static int stateless_rpc;
>  static int prefer_ofs_delta = 1;
>  static int auto_update_server_info;
>  static int auto_gc = 1;
> @@ -123,7 +125,7 @@ static void show_ref(const char *path, const unsigned char *sha1)
>  	else
>  		packet_write(1, "%s %s%c%s%s\n",
>  			     sha1_to_hex(sha1), path, 0,
> -			     " report-status delete-refs side-band-64k quiet",
> +			     " report-status delete-refs side-band-64k quiet verify-refs",
>  			     prefer_ofs_delta ? " ofs-delta" : "");
>  	sent_capabilities = 1;
>  }
> @@ -410,14 +412,13 @@ static void refuse_unconfigured_deny_delete_current(void)
>  		rp_error("%s", refuse_unconfigured_deny_delete_current_msg[i]);
>  }
>  
> -static const char *update(struct command *cmd)
> +static const char *verify_ref(struct command *cmd)
>  {
>  	const char *name = cmd->ref_name;
>  	struct strbuf namespaced_name_buf = STRBUF_INIT;
>  	const char *namespaced_name;
>  	unsigned char *old_sha1 = cmd->old_sha1;
>  	unsigned char *new_sha1 = cmd->new_sha1;
> -	struct ref_lock *lock;
>  
>  	/* only refs/... are allowed */
>  	if (prefixcmp(name, "refs/") || check_refname_format(name + 5, 0)) {
> @@ -444,12 +445,6 @@ static const char *update(struct command *cmd)
>  		}
>  	}
>  
> -	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
> -		error("unpack should have generated %s, "
> -		      "but I can't find it!", sha1_to_hex(new_sha1));
> -		return "bad pack";
> -	}
> -
>  	if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
>  		if (deny_deletes && !prefixcmp(name, "refs/heads/")) {
>  			rp_error("denying ref deletion for %s", name);
> @@ -473,6 +468,27 @@ static const char *update(struct command *cmd)
>  		}
>  	}
>  
> +	return NULL;
> +}
> +
> +static const char *update(struct command *cmd)
> +{
> +	const char *name = cmd->ref_name;
> +	struct strbuf namespaced_name_buf = STRBUF_INIT;
> +	const char *namespaced_name;
> +	unsigned char *old_sha1 = cmd->old_sha1;
> +	unsigned char *new_sha1 = cmd->new_sha1;
> +	struct ref_lock *lock;
> +
> +	strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name);
> +	namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
> +
> +	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
> +		error("unpack should have generated %s, "
> +		      "but I can't find it!", sha1_to_hex(new_sha1));
> +		return "bad pack";
> +	}
> +
>  	if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
>  	    !is_null_sha1(old_sha1) &&
>  	    !prefixcmp(name, "refs/heads/")) {
> @@ -692,10 +708,41 @@ static int iterate_receive_command_list(void *cb_data, unsigned char sha1[20])
>  	return -1; /* end of list */
>  }
>  
> +static int verify_ref_commands(struct command *commands)
> +{
> +	unsigned char sha1[20];
> +	int commands_ok;
> +	struct command *cmd;
> +
> +	free(head_name_to_free);
> +	head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
> +
> +	commands_ok = 0;
> +	for (cmd = commands; cmd; cmd = cmd->next) {
> +		cmd->error_string = verify_ref(cmd);
> +		if (!cmd->error_string)
> +			commands_ok++;
> +	}
> +
> +	if (verify_refs && !stateless_rpc) {
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		packet_buf_write(&buf, "verify-refs %s\n",
> +			 commands_ok > 0 ? "ok" : "no valid refs");
> +
> +		if (use_sideband)
> +			send_sideband(1, 1, buf.buf, buf.len, use_sideband);
> +		else
> +			safe_write(1, buf.buf, buf.len);
> +		strbuf_release(&buf);
> +	}
> +
> +	return commands_ok;
> +}
> +
>  static void execute_commands(struct command *commands, const char *unpacker_error)
>  {
>  	struct command *cmd;
> -	unsigned char sha1[20];
>  
>  	if (unpacker_error) {
>  		for (cmd = commands; cmd; cmd = cmd->next)
> @@ -718,9 +765,6 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
>  
>  	check_aliased_updates(commands);
>  
> -	free(head_name_to_free);
> -	head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
> -
>  	for (cmd = commands; cmd; cmd = cmd->next) {
>  		if (cmd->error_string)
>  			continue;
> @@ -766,6 +810,8 @@ static struct command *read_head_info(void)
>  				use_sideband = LARGE_PACKET_MAX;
>  			if (parse_feature_request(feature_list, "quiet"))
>  				quiet = 1;
> +			if (parse_feature_request(feature_list, "verify-refs"))
> +				verify_refs = 1;
>  		}
>  		cmd = xcalloc(1, sizeof(struct command) + len - 80);
>  		hashcpy(cmd->old_sha1, old_sha1);
> @@ -905,7 +951,6 @@ static int delete_only(struct command *commands)
>  int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>  {
>  	int advertise_refs = 0;
> -	int stateless_rpc = 0;
>  	int i;
>  	char *dir = NULL;
>  	struct command *commands;
> @@ -962,11 +1007,15 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>  		return 0;
>  
>  	if ((commands = read_head_info()) != NULL) {
> +		int commands_ok;
>  		const char *unpack_status = NULL;
>  
> -		if (!delete_only(commands))
> -			unpack_status = unpack();
> -		execute_commands(commands, unpack_status);
> +		commands_ok = verify_ref_commands(commands);
> +		if (!verify_refs || commands_ok > 0) {
> +			if (!delete_only(commands))
> +				unpack_status = unpack();
> +			execute_commands(commands, unpack_status);
> +		}
>  		if (pack_lockfile)
>  			unlink_or_warn(pack_lockfile);
>  		if (report_status)
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 71f258e..7d514ca 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -265,6 +265,8 @@ int send_pack(struct send_pack_args *args,
>  		use_sideband = 1;
>  	if (!server_supports("quiet"))
>  		args->quiet = 0;
> +	if (server_supports("verify-refs"))
> +		args->verify_refs = 1;
>  
>  	if (!remote_refs) {
>  		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
> @@ -303,12 +305,13 @@ int send_pack(struct send_pack_args *args,
>  			char *old_hex = sha1_to_hex(ref->old_sha1);
>  			char *new_hex = sha1_to_hex(ref->new_sha1);
>  
> -			if (!cmds_sent && (status_report || use_sideband || args->quiet)) {
> -				packet_buf_write(&req_buf, "%s %s %s%c%s%s%s",
> +			if (!cmds_sent && (status_report || use_sideband || args->quiet || args->verify_refs)) {
> +				packet_buf_write(&req_buf, "%s %s %s%c%s%s%s%s",
>  					old_hex, new_hex, ref->name, 0,
>  					status_report ? " report-status" : "",
>  					use_sideband ? " side-band-64k" : "",
> -					args->quiet ? " quiet" : "");
> +					args->quiet ? " quiet" : "",
> +					args->verify_refs ? " verify-refs" : "");
>  			}
>  			else
>  				packet_buf_write(&req_buf, "%s %s %s",
> @@ -341,17 +344,29 @@ int send_pack(struct send_pack_args *args,
>  		in = demux.out;
>  	}
>  
> -	if (new_refs && cmds_sent) {
> -		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
> -			for (ref = remote_refs; ref; ref = ref->next)
> -				ref->status = REF_STATUS_NONE;
> -			if (args->stateless_rpc)
> -				close(out);
> -			if (git_connection_is_socket(conn))
> -				shutdown(fd[0], SHUT_WR);
> -			if (use_sideband)
> -				finish_async(&demux);
> -			return -1;
> +	if (cmds_sent) {
> +		int verify_refs_status = 0;
> +
> +		if (args->verify_refs && !args->stateless_rpc) {
> +			char line[1000];
> +			int len = packet_read_line(in, line, sizeof(line));
> +			if (len < 15 || memcmp(line, "verify-refs ", 12))
> +				return error("did not receive remote status");
> +			verify_refs_status = memcmp(line, "verify-refs ok\n", 15);
> +		}
> +
> +		if (!verify_refs_status && new_refs) {
> +			if (pack_objects(out, remote_refs, extra_have, args) < 0) {
> +				for (ref = remote_refs; ref; ref = ref->next)
> +					ref->status = REF_STATUS_NONE;
> +				if (args->stateless_rpc)
> +					close(out);
> +				if (git_connection_is_socket(conn))
> +					shutdown(fd[0], SHUT_WR);
> +				if (use_sideband)
> +					finish_async(&demux);
> +				return -1;
> +			}
>  		}
>  	}
>  	if (args->stateless_rpc && cmds_sent)
> diff --git a/send-pack.h b/send-pack.h
> index 05d7ab1..87edaa5 100644
> --- a/send-pack.h
> +++ b/send-pack.h
> @@ -11,7 +11,8 @@ struct send_pack_args {
>  		use_thin_pack:1,
>  		use_ofs_delta:1,
>  		dry_run:1,
> -		stateless_rpc:1;
> +		stateless_rpc:1,
> +		verify_refs:1;
>  };
>  
>  int send_pack(struct send_pack_args *args,
Previous: Clemens BuchacherNext: Clemens Buchacher
Message 11 of 16 in “optionally deny all pushes”
  1. optionally deny all pushesClemens Buchacher, Jan 21, 2012
  2. Junio C HamanoJan 21, 2012
  3. 0/5 verify refs earlyClemens Buchacher, Feb 13, 2012
  4. 1/5 git rev-list: fix invalid typecastClemens Buchacher, Feb 13, 2012
  5. Junio C HamanoFeb 13, 2012
  6. 2/5 do not override receive-pack errorsClemens Buchacher, Feb 13, 2012
  7. Junio C HamanoFeb 13, 2012
  8. Clemens BuchacherFeb 14, 2012
  9. Junio C HamanoFeb 14, 2012
  10. 3/5 git push: verify refs earlyClemens Buchacher, Feb 13, 2012
  11. Junio C HamanoFeb 13, 2012
  12. Clemens BuchacherFeb 14, 2012
  13. 4/5 t5541: use configured port numberClemens Buchacher, Feb 13, 2012
  14. Junio C HamanoFeb 13, 2012
  15. 5/5 push/fetch/clone --no-progress suppresses progress outputClemens Buchacher, Feb 13, 2012
  16. Junio C HamanoFeb 13, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.