utils/update-checkout: Fix for mishandling detached HEAD in SR-3854 #7232
Conversation
In update_single_repository, before doing the rebase step, check whether the current HEAD is a "detached HEAD", normally the result of checking out a tag by previously invoking update-checkout with the --tag option. If HEAD is a detached HEAD, skip the rebase (and print an explanatory message), because rebasing would do the wrong thing and make a mess.
|
@swift-ci Please smoke test |
|
Nope, seems I still can't kick off tests... |
We now examine the exception raised by the "git symbolic-ref -q HEAD" command to check whether the exception was indeed due to a detached HEAD. If there was an actual error in the command, we re-raise the exception so that it can be handled like errors from other commands.
|
My error handling for the command "git symbolic-ref -q HEAD" originally failed to account for any possibility that this command would give a real error. The second commit I just made fixes this, checking the exception raised by the command, setting the detached-HEAD flag only if the error code is the value reserved for this condition, and otherwise passing the exception up the chain to be handled normally. |
|
@swift-ci Please smoke test |
|
@erg Suggestion: Squash commit? Seems like a good commit for it. @colinhowell IIRC to be able to kick off PR runs you need commit access. For commit access see this blog post: https://swift.org/blog/swift-commit-access/ @shahmishal My memory is right about the requirement for PR runs, right? |
|
@gottesmm You are correct, only users with commit access can trigger CI from Github. |
|
OK, @gottesmm, @shahmishal, thanks for the clarification. I'm obviously quite new here and in no rush to get commit access; I just wanted to understand what was going on. Thanks again. |
|
@colinhowell No worries. I have gotten this question a lot. I am thinking of proposing a section to the main README.md that specifically calls this out. Would that have been helpful? |
|
I think such a section would be helpful, yes. The general tone of the development documentation (quite rightly) strongly emphasizes the importance of testing changes, and I think a comment in the PR submission template may specifically ask the submitter to trigger an appropriate CI run, so it's natural to assume one should do so. At present there is no obvious indication that CI testing is restricted to those with commit access, either in the main README.md, in docs/ContinuousIntegration.md, or in the Contributing section on the swift.org website. (The website's Continuous Integration section does say "a member of the Swift team will trigger testing by the CI system", which in retrospect does imply restricted access, but I didn't notice that until now.) |
|
@gottesmm Just realized it doesn't help to reply to a comment if you fail to ping the person you're replying to. Sorry. :P |
|
@erg Thanks! |
|
@colinhowell Thanks for the patch! |
AnthonyLatsis
added
the
update-checkout
AnthonyLatsis
added
the
utils
In update_single_repository, before doing the rebase step, check whether
the current HEAD is a "detached HEAD", normally the result of checking
out a tag by previously invoking update-checkout with the --tag option.
If HEAD is a detached HEAD, skip the rebase (and print an explanatory
message), because rebasing would do the wrong thing and make a mess.
This changes how we handle any repository with a "detached HEAD". This state means that git's HEAD reference is not currently associated with any branch, and it is the normal and expected result of checking out a tag using update-checkout's --tag option.
Presently, update-checkout does a "git rebase FETCH_HEAD" as always. This fails badly in the case of a detached HEAD. The preceding "git fetch" will have updated FETCH_HEAD with the state of all the branches. Normally, the currently checked-out branch would come first, and all the others would be marked as not-for-merge. But with a detached HEAD, all branches are marked as not-for-merge. In this case, "git rebase FETCH_HEAD" does not just skip them all; it falls back to rebasing from the head of the default branch (usually "master"). This makes a mess of things; depending on the repository, you may see merge conflicts, and you definitely won't be in the state of the originally checked-out tag.
We solve this problem by checking for a detached HEAD just before the rebase step; if we find one, we skip the rebase and print an informatory message instead. To check for a detached HEAD, we use the command "git symbolic-ref -q HEAD". This is a git plumbing command which looks for the symbolic reference pointed to by HEAD (which will thus show the current branch); it exits quietly with an error return code if no such reference is found, which is the definition of a detached HEAD. Using this command, rather than parsing the output of a user command like "git status" or "git branch", is recommended for scripting by git's authors, who reserve the right to change the output of user commands. (See this blog posting.)
Since "git symbolic-ref -q HEAD" will return an error code if a detached HEAD is found, we wrap it in a try-except clause and set a flag detached_head if an error code is returned. This flag then determines whether a rebase is done or a message is printed. (The message is printed with a single print statement because I wanted it to print atomically, similar to how shell.run() invocations print.)
I've tested this script myself on both tag checkouts and ordinary checkouts and it seems to behave correctly.
@erg, @gottesmm, your feedback would be appreciated. I'd also appreciate it if someone could kick off test runs.
Resolves #46439.