GNOME Bugzilla – Bug 618974
[PATCH 00/14] Restructure the _update member of GitBranch
Last modified: 2010-06-08 08:51:36 UTC
There are a lot of things happening inside _update, and I hope this series simplifies that a bit. The previous behavior is mostly kept, except in locations where I thought, that it could be improved: - Use '--ignore-submodules' to prevent commits in submodules to trigger a stashing operation. - (return to a) pull after the branch switch, but ensure a recently added branch is found when switching - Fail when there is an attempt to switch dirty branches. Uncommited changes on the same branch are still moved along. Dirk Wallenstein (14): [git] Introduce a predicate execution wrapper [git] Rewrite the branch name getter [git] Make git version checks more readable [git] Use a new predicate to test if a branch tracks a remote branch [git] Use a new predicate to determine the need to stash [git] Rename would_be_branch to wanted_branch [git] Extract the branch switch into a function [git] Try to find requested branch remotely if not available [git] Switch branches previous to stash and pull [git] Extract the pull operation into a function [git] Move the quiet option to its point of use [git] Extract sticky date operation into a function [git] Align local branch predicate with other predicates [git] Clarify branch switch destination calculation jhbuild/versioncontrol/git.py | 238 ++++++++++++++++++++++++++--------------- 1 files changed, 150 insertions(+), 88 deletions(-)
Created attachment 161336 [details] [review] [git] Introduce a predicate execution wrapper There are often git commands where only a boolean return value is of interest. This new wrapper can be easily used to create such predicates.
Created attachment 161337 [details] [review] [git] Rewrite the branch name getter The new getter returns a valid branchname or None in case of a detached head. The name is changed to have a '_' prefix. There are no external users.
Created attachment 161338 [details] [review] [git] Make git version checks more readable Wrap the generic version checker into a git specific one and use it.
Created attachment 161339 [details] [review] [git] Use a new predicate to test if a branch tracks a remote branch This allows the remote to have a different name than 'origin', and gives the operation a meaningful name.
Created attachment 161340 [details] [review] [git] Use a new predicate to determine the need to stash Introduce a predicate to test if the repository is dirty, optionally including existing submodules. This function is used to recognize uncommited changes to be stashed. This should now work more reliable in the face of changes in submodules, as the output of 'git diff --submodule' may have multiple lines, where not every line starts with 'Submodule '. To reliably exclude differences in submodules, a dependency to a two year old git version is added.
Created attachment 161341 [details] [review] [git] Rename would_be_branch to wanted_branch This seems to better describe the purpose.
Created attachment 161342 [details] [review] [git] Extract the branch switch into a function The new function will not switch a branch if there are uncommited changes. This will have no impact, yet, as a dirty state is always stashed prior to a branch switch.
Created attachment 161343 [details] [review] [git] Try to find requested branch remotely if not available If a branch switch is requested but the branch is not available, upstream will be fetched and the branch will be looked up again, before failing. This should make the branch switching independent of the concerns stated in (GNOME bug 591470) and commit f7d5f81b6ca02ee5.
Created attachment 161344 [details] [review] [git] Switch branches previous to stash and pull Now the refusal of the previously extracted function to switch dirty branches will take effect. It is very unlikely that a user wants to apply uncommited changes onto another branch by means of a jhbuild update operation. Such an operation certainly deserves manual inspection. This will now update the new branch instead of the previous one. The problem with not finding recently added remote branches is solved in a previous commit.
Created attachment 161345 [details] [review] [git] Extract the pull operation into a function The pull operation will only be executed if the current branch is tracking a remote branch.
Created attachment 161346 [details] [review] [git] Move the quiet option to its point of use
Created attachment 161347 [details] [review] [git] Extract sticky date operation into a function
Created attachment 161348 [details] [review] [git] Align local branch predicate with other predicates There's no need to log the simple test for a local branch. Therefore, the function can be aligned with the other predicates.
Created attachment 161349 [details] [review] [git] Clarify branch switch destination calculation Extract the calculation that determines if a branch switch is necessary into a function. This also simplifies the calculations a bit while preserving the previous behavior.
Created attachment 161732 [details] [review] [git] Consider staged, uncommited changes as dirty is_dirty made the wrong diff previously. This now considers any deviation between HEAD and the current working tree as dirty.
Created attachment 161734 [details] [review] Always trigger a build for dirty git branches A dirty git branch will be considered not up to date in every build_policy.
Created attachment 161834 [details] [review] Trigger a build for dirty branches more generally This removes the git dependency of the dirty branch check for the build_policy, and allows every branch type to support that, by defining an 'is_dirty' member.
I didn't get a deep look at the Git usage changes, I believe you know what you're doing. About the coding style, as a general rule I don't like prefixing methods with _ that much, and JHBuild should work with Python 2.3 (sth like quiet = ['-q'] if self.config.quiet_mode else [] is not supported).
Created attachment 161843 [details] [review] [git] Avoid the ternary operator for python-2.3 conformance This reverts the syntax change additionally introduced in the commit '[git] Move the quiet option to its point of use'
Created attachment 161844 [details] [review] [git] Remove the '_' prefix from all new functions This reverts the introduction of new functions with a '_' prefix in the recent commit series.
(In reply to comment #18) > I didn't get a deep look at the Git usage changes, I believe you know what > you're doing. About the coding style, as a general rule I don't like prefixing > methods with _ that much, and JHBuild should work with Python 2.3 (sth like > quiet = ['-q'] if self.config.quiet_mode else [] is not supported). I've tested those changes and it works alright. I've looked through the commits and I think the ternary operator was the only thing not conforming to python-2.3.
Ok, fine; you should go ahead and rebase your patches, then push.
Pushed everything; thanks.