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

On Sat, Jan 3, 2015 at 1:53 AM, Duy Nguyen [off-list ref] wrote:

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

quoted

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

The intention of that patch is to move the code unrelated to executing commands
out of execute_commands. And I feel this error checking is not the core task of
executing commands, hence it should be moved out completely. Having one part
in  warn_if_skipped_connectivity_check and then the other error part
triggered by
an unsuccessful return doesn't improve the situation IMHO.

I think about moving the check for shallow_update inside that function and
to have a

        if (!shallow_update)
                return;

and additionally renaming the function to be more precise:

        assure_shallow_connectivity_checked();

These changes I can put into this patch.

Replacing error by a die command will go in an extra patch on top.
So I am understanding your answers such that we definitely want to prevent
a push if this future bug happen. Instead of incorporating that into
later patches
of the series to abort in case of this possible bug, we could just go
with s/error/die/
and the problem is solved.

quoted

                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.

Then this patch also helps with introducing a dedicated function to assure
the connectivity which we can reuse maybe.

Thanks,
Stefan