Re: [PATCHv9 1/9] receive-pack.c: shorten the execute_commands loop over all commands

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 3, 2015, 02:20 UTC
Message-ID: <20150103022004.GI29365@google.com>
In-Reply-To: <1419982898-23108-2-git-send-email-sbeller@google.com>

(+cc: Duy, who understands shallow push well) Hi,

Stefan Beller wrote:

> This commit shortens execute_commands loop over all commands by moving

The commit message can be simplified by leaving out "This commit" and stating what the commit does in the imperative, focusing on what problem the commit solves.

For example:

	Make the main "execute_commands" loop in receive-pack easier to read
	by splitting out some steps into helper functions.  The new helper
	'should_process_cmd' checks if a ref update is unnecessary, whether
	due to an error having occured or for another reason.  The helper
	'check_shallow_bugs' warns if we have forgotten to run a connectivity
	check on a ref which is shallow for the client.

	This will help us to duplicate less code in a later patch when we make
	a second copy of the "execute_commands" loop.

	No functional change intended.

By the way, is there some clearer name for check_shallow_bugs? That name makes it sound like there are some bugs we are checking up on, whereas a name that makes it obvious what the function will do and saves me from having to read the function body would be ideal.

[...]

> +++ b/builtin/receive-pack.c

[...]

> @@ -1077,27 +1099,15 @@ static void execute_commands(struct command *commands,

[...]

>  	for (cmd = commands; cmd; cmd = cmd->next) {

[...]

Show 14 quoted lines

> -		if (shallow_update && !cmd->error_string &&
> -		    si->shallow_ref[cmd->index]) {
> -			error("BUG: connectivity check has not been run on ref %s",
> -			      cmd->ref_name);
> -			checked_connectivity = 0;
> -		}
>  	}
>
> -	if (shallow_update && !checked_connectivity)
> -		error("BUG: run 'git fsck' for safety.\n"
> -		      "If there are errors, try to remove "
> -		      "the reported refs above");
> +	if (shallow_update)
> +		check_shallow_bugs(commands, si);

In the same spirit of saving the reader from having to look at the body of check_shallow_bugs, would it make sense for the part that reacts to an error to be kept in the caller? E.g.:

	if (shallow_update && warn_if_skipped_connectivity_check(commands, si))
		error("BUG: run 'git fsck for safety.\n"
		      "If there are errors, try removing the refs reported above");

Is this error possible, by the way? update() does not return success unless it has reached the bottom block in the function. In the !is_null_sha1(new_sha1) case that means it calls update_shallow_ref(), which performs the connectivity check. In the is_null_sha1(new_sha1) case, update_shallow_info() does not set cmd->index and si->shallow_ref[cmd->index] cannot be set.

Perhaps this error message could be written in a way that makes it clearer that we really expect it not to happen, like

		die("BUG: connectivity check skipped in shallow push???");

(die() instead of error() to prevent refs from updating and pointing to a disconnected history).

Thoughts? Jonathan