GitSCM: fix default_branch and target_cset bugs (bug 1945697) #219
Conversation
shtrom
force-pushed
the
bug1945697/git-default_branch-push_target
branch
from
4d56597 to
08d4fea
Compare
shtrom
changed the title
Depending on the git command run (with/without push_target), Git will behave differently (presumably checking if the explicit push_target exists yet), and will return a different error, which may not contain redactable strings. The full exception body, though, always will.
shtrom
force-pushed
the
bug1945697/git-default_branch-push_target
branch
from
7c062f0 to
68054b4
Compare
…t_branch-push_target
| if not push_target: | ||
| push_target = self.default_branch | ||
|
|
||
| command += [f"HEAD:{push_target}"] |
There was a problem hiding this comment.
This essentially pushes HEAD to push_target? I was thinking we could move the appropriate branch to HEAD with git branch <branch> HEAD -f and then git push origin <branch>, but this works too.
There was a problem hiding this comment.
Yep. All the additional commits have been implicitly added to HEAD. I don't think we even know the work branch name in other places (we could read it out, but it would be no better than using HEAD)
We should already be on the work branch from the update_repo call, but that's the only time we make the switch explicitly, everything after that assumes it's in the right position.
Could be better in theory, and more explicit about the branch to work on, but I think it would add a lot of overhead vs. setting HEAD once, and assuming it is correct (<- famous last words right there.)
Co-authored-by: Connor Sheehan <[email protected]>
| work_branch = f"lando-{datetime.now().strftime(ISO8601_TIMESTAMP_BASIC)}" | ||
| self._git_run( | ||
| "checkout", "--force", "-B", branch, f"origin/{branch}", cwd=self.path | ||
| "checkout", "--force", "-B", work_branch, target_cset, cwd=self.path | ||
| ) |
There was a problem hiding this comment.
Should we clean up after this? Otherwise we will accumulate a bunch of branches that will never be used again.
There was a problem hiding this comment.
We should.
I initially was thinking that, as we use --prune during the update, we'd purge them. I'm not so sure it is sufficient now, as those work branches don't have upstreams, and therefore will stick around forever.
In practice, it's probably not a big deal, because the pushes are fast-forwards, so we won't have dangling commits just due to old branches pointing to them, they will just point to non-tip commits.
But it would be better to clean the branches up anyway, yeah.
| remote_branch = f"origin/{target_cset}" | ||
| if self._git_run("branch", "--list", "--remote", remote_branch, cwd=self.path): | ||
| # If the branch exists remotely, make sure we get the up-to-date version. | ||
| target_cset = remote_branch |
There was a problem hiding this comment.
Curious why we are going with that target_cset terminology instead of a more git-centric one?
There was a problem hiding this comment.
(copying response to correct comment) Weight of history. It's what the standalone HgRepo object was using, and it got into the abstract_scm.
For git I'd use something like base_ref, but the meaning isn't changed.
No description provided.