Hi Junio,
On 13 Mar 2022, at 3:58, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> diff --git a/reset.c b/reset.c
>> index e3383a93343..f8e32fcc240 100644
>> --- a/reset.c
>> +++ b/reset.c
>> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
>> if (opts->branch_msg && !opts->branch)
>> BUG("branch reflog message given without a branch");
>>
>> + if (switch_to_branch && opts->flags & RESET_HEAD_DETACH)
>
> It's just style thing but it probably is easier to read to have
> an extra () around the bitwise-&.
>
>> + BUG("attempting to detach HEAD when branch is given");
>
> I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH
> when switch_to_branch == NULL. If there isn't, it could be that
> we can get rid of RESET_HEAD_DETACH bit and base this decision
> solely on switch_to_branch'es NULLness.
>
> But that is totally outside the scope of this fix. I'd prefer to
> see a narrowly and sharply focused fix, and to be quite honest, I
> would be happier if this assertion weren't in this topic.
I'm okay with taking this out, but would be good to get an ack from Phillip.
>
>> if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
>> ret = -1;
>> goto leave_reset_head;
>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index 0643d015255..d5a8ee39fc4 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -394,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '
>> git rebase main other
>> '
>>
>> +test_expect_success 'switch to non-branch detaches HEAD' '
>> + git checkout main &&
>> + old_main=$(git rev-parse HEAD) &&
>> + git rebase First Second^0 &&
>
> Good. For reproduction, the fork-point "First" does not have to be
> a raw object name. "Second^0" not being a branch name does matter.
>
>> + test_cmp_rev HEAD Second &&
>> + test_cmp_rev main $old_main &&
>> + test_must_fail git symbolic-ref HEAD
>
> All correct and to the point. Good.
Thanks to Phillip for his help on this!
>
> Will queue. Thanks.
More of a question about protocol. If there are some minor adjustments that are
not really worth more disussion on the ML (like removing the BUG assertion),
is it still protocol to send another series or just re-roll on the branch without
sending out another patch series?
Thanks!
John
>
>> +'
>> +
>> test_expect_success 'refuse to switch to branch checked out elsewhere' '
>> git checkout main &&
>> git worktree add wt &&
