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

On Sat, Jan 3, 2015 at 9:20 AM, Jonathan Nieder [off-list ref] wrote:

quoted

-     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?

That code is to prevent bugs in future. The whole operation is spread
out in many functions/steps and people may overlook.

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).

If connectivity test is not done, we don't know if history is really
disconnected. But yeah die() may be better (err on the safe side).
-- 
Duy