On Sat, Jan 3, 2015 at 1:53 AM, Duy Nguyen <pclouds@xxxxxxxxx> wrote:
> On Sat, Jan 3, 2015 at 9:20 AM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
>>> - 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.
>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html