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

[Jonathan Nieder <jrnieder@gmail.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) {
[...]
> -		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