Hi Junio and thanks for your time!
Show 6 quoted lines
> While your change may skip the code that segfaults, wouldn't it also > stop noticing a broken case where .peer_ref should have been set but > didn't, even when --revision=<rev> parameter is not used in the > command invocation? IOW, it is better to segfault and draw attention > by Git developers when a valid input is given by the end user and our > code misbehaves (e.g., and fails to to set .peer_ref as it should).
I totally agree with you.
Show 8 quoted lines
> Wouldn't the correct fix be more like the following? > > - split out parts from update_remote_refs() that are needed even in > option_rev mode into a separate helper function, and call that > from cmd_clone(). > > - make the call to update_remote_refs() conditional---specifically, > we shouldn't be calling it when option_rev is in effect.
Do we have another fix to make a conditional call to `find_ref_by_name()` when `option_rev` is in effect? Because from the doc of `--revision`, "... and detach `HEAD` to_<rev>_. ...", that means we don't need to know where the real HEAD points to?
> Also, isn't this something we can specify the expected behaviour in > tests? Not only we want to ensure that nothing segfaults, we would > want to make sure that the resulting repository has no refs and HEAD > is detached at the specified revision.
I'll add several tests to cover the bug after we make sure how to fix it.