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