Issue 6068640302497792: Issue 2311 - Track remote Git branches when required (Closed)
DescriptionIssue 2311 - Track remote Git branches when required
Patch Set 1 #
Total comments: 12
Patch Set 2 : Make Git.pull track all remote branches #Patch Set 3 : Rebased #
Total comments: 4
Patch Set 4 : Track "master" branch too #Patch Set 5 : Only need to fetch remote branches from origin #
Total comments: 11
Patch Set 6 : Addressed feedback #
Total comments: 4
Patch Set 7 : Improved comment #
Total comments: 3
Patch Set 8 : Simplified regexp by not avoiding HEAD #
Total comments: 3
Patch Set 9 : Simplify regexp some more #MessagesTotal messages: 30
Patch Set 1
http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "checkout", rev], cwd=repo) Wasn't the problem you try to address here, that branches other than master that haven't been manually marked as tracking, aren't even fetched?
http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "checkout", rev], cwd=repo) On 2015/04/30 17:54:34, Sebastian Noack wrote: > Wasn't the problem you try to address here, that branches other than master that > haven't been manually marked as tracking, aren't even fetched? So it turns out the remote branches are fetched, the problem is that local tracking branches aren't created for them. Checking out the revision here causes a tracking branch of the same name to be created where relevant. It seems to work pretty well, only the required remote branch is tracked and it is only done when required (when the revision isn't resolved already).
http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "checkout", rev], cwd=repo) On 2015/04/30 19:31:46, kzar wrote: > On 2015/04/30 17:54:34, Sebastian Noack wrote: > > Wasn't the problem you try to address here, that branches other than master > that > > haven't been manually marked as tracking, aren't even fetched? > > So it turns out the remote branches are fetched, the problem is that local > tracking branches aren't created for them. Checking out the revision here causes > a tracking branch of the same name to be created where relevant. > > It seems to work pretty well, only the required remote branch is tracked and it > is only done when required (when the revision isn't resolved already). Isn't this call redundant with the one in Git.update()? I don't get how this should change anything. Are you sure that this is the right thing to do here?
http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "checkout", rev], cwd=repo) On 2015/04/30 20:53:57, Sebastian Noack wrote: > On 2015/04/30 19:31:46, kzar wrote: > > On 2015/04/30 17:54:34, Sebastian Noack wrote: > > > Wasn't the problem you try to address here, that branches other than master > > that > > > haven't been manually marked as tracking, aren't even fetched? > > > > So it turns out the remote branches are fetched, the problem is that local > > tracking branches aren't created for them. Checking out the revision here > causes > > a tracking branch of the same name to be created where relevant. > > > > It seems to work pretty well, only the required remote branch is tracked and > it > > is only done when required (when the revision isn't resolved already). > > Isn't this call redundant with the one in Git.update()? I don't get how this > should change anything. Are you sure that this is the right thing to do here? As of Git 1.6.6 [1] checking out a local branch when there is no local branch by that name but there is a remote branch of that name creates a local tracking branch before checking it out. We need to make sure the tracking branch is created here, before the later checkout, because without it the revision_id can't be resolved. This approach seems to work fine, only difference really is that should the revision not be found by this checkout a `subprocess.CalledProcessError` exception will be thrown instead of the later "Failed to resolve revision" exception. (If that's an issue I can catch the `subprocess.CalledProcessError` here.) The other difference is that the branch is checked out now, but that doesn't really matter IMHO. If it works here the later one would have worked too. (Actually the later check out uses the resolved revision ID instead of the branch name, so they both do something. My point is that they either both work or both fail.) [1] https://raw.githubusercontent.com/git/git/master/Documentation/RelNotes/1.6.6...
http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "checkout", rev], cwd=repo) On 2015/05/01 09:43:55, kzar wrote: > On 2015/04/30 20:53:57, Sebastian Noack wrote: > > On 2015/04/30 19:31:46, kzar wrote: > > > On 2015/04/30 17:54:34, Sebastian Noack wrote: > > > > Wasn't the problem you try to address here, that branches other than > master > > > that > > > > haven't been manually marked as tracking, aren't even fetched? > > > > > > So it turns out the remote branches are fetched, the problem is that local > > > tracking branches aren't created for them. Checking out the revision here > > causes > > > a tracking branch of the same name to be created where relevant. > > > > > > It seems to work pretty well, only the required remote branch is tracked and > > it > > > is only done when required (when the revision isn't resolved already). > > > > Isn't this call redundant with the one in Git.update()? I don't get how this > > should change anything. Are you sure that this is the right thing to do here? > > As of Git 1.6.6 [1] checking out a local branch when there is no local branch by > that name but there is a remote branch of that name creates a local tracking > branch before checking it out. We need to make sure the tracking branch is > created here, before the later checkout, because without it the revision_id > can't be resolved. > > This approach seems to work fine, only difference really is that should the > revision not be found by this checkout a `subprocess.CalledProcessError` > exception will be thrown instead of the later "Failed to resolve revision" > exception. (If that's an issue I can catch the `subprocess.CalledProcessError` > here.) > > The other difference is that the branch is checked out now, but that doesn't > really matter IMHO. If it works here the later one would have worked too. > (Actually the later check out uses the resolved revision ID instead of the > branch name, so they both do something. My point is that they either both work > or both fail.) > > [1] > https://raw.githubusercontent.com/git/git/master/Documentation/RelNotes/1.6.6... If I understand the issue correctly, this change is rather pointless. As it merely makes sure the Git.pull() fails if Git.update() would fail, to generate a different output in an error scenario. But in the end of the day, either ensure_dependency succeeds or fails regardless of this change. But on the other hand this change would mess up the abstraction, and duplicates code that is even executed redundantly.
http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "checkout", rev], cwd=repo) On 2015/05/04 11:44:31, Sebastian Noack wrote: > On 2015/05/01 09:43:55, kzar wrote: > > On 2015/04/30 20:53:57, Sebastian Noack wrote: > > > On 2015/04/30 19:31:46, kzar wrote: > > > > On 2015/04/30 17:54:34, Sebastian Noack wrote: > > > > > Wasn't the problem you try to address here, that branches other than > > master > > > > that > > > > > haven't been manually marked as tracking, aren't even fetched? > > > > > > > > So it turns out the remote branches are fetched, the problem is that local > > > > tracking branches aren't created for them. Checking out the revision here > > > causes > > > > a tracking branch of the same name to be created where relevant. > > > > > > > > It seems to work pretty well, only the required remote branch is tracked > and > > > it > > > > is only done when required (when the revision isn't resolved already). > > > > > > Isn't this call redundant with the one in Git.update()? I don't get how this > > > should change anything. Are you sure that this is the right thing to do > here? > > > > As of Git 1.6.6 [1] checking out a local branch when there is no local branch > by > > that name but there is a remote branch of that name creates a local tracking > > branch before checking it out. We need to make sure the tracking branch is > > created here, before the later checkout, because without it the revision_id > > can't be resolved. > > > > This approach seems to work fine, only difference really is that should the > > revision not be found by this checkout a `subprocess.CalledProcessError` > > exception will be thrown instead of the later "Failed to resolve revision" > > exception. (If that's an issue I can catch the `subprocess.CalledProcessError` > > here.) > > > > The other difference is that the branch is checked out now, but that doesn't > > really matter IMHO. If it works here the later one would have worked too. > > (Actually the later check out uses the resolved revision ID instead of the > > branch name, so they both do something. My point is that they either both work > > or both fail.) > > > > [1] > > > https://raw.githubusercontent.com/git/git/master/Documentation/RelNotes/1.6.6... > > If I understand the issue correctly, this change is rather pointless. As it > merely makes sure the Git.pull() fails if Git.update() would fail, to generate a > different output in an error scenario. But in the end of the day, either > ensure_dependency succeeds or fails regardless of this change. But on the other > hand this change would mess up the abstraction, and duplicates code that is even > executed redundantly. Then you do not understand this change properly. Without this change if I set a branch name as repository revision ensure_dependencies will fail. The reason it fails is because we do not tell Git to checkout the revision until we ensure it exists and that we can resolve it to a commit hash. The problem is that the checkout in this case _causes_ the revision (branch name) to exist and to be able to resolve to a commit hash. We therefore need to attempt the checkout before we ensure the revision resolves, hence my change. If you think this change is redundant try it for yourself, create a Git repository on Github or somewhere that contains a branch. Put the Git URL as the dependency name and the branch name as the revision in your dependencies file and see what the script does. Try it with and without my change, you will see what I mean.
http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "checkout", rev], cwd=repo) Ok, got it. But this still seems like a silly hack to me. Why don't you just change the git fetch command line to "git fetch --quiet origin <rev>" as I initially suggested? Then we will explicitly fetch the revision we checkout in the next step, without breaking the abstraction here.
http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "checkout", rev], cwd=repo) On 2015/05/05 15:08:54, Sebastian Noack wrote: > Ok, got it. But this still seems like a silly hack to me. Why don't you just > change the git fetch command line to "git fetch --quiet origin <rev>" as I > initially suggested? Then we will explicitly fetch the revision we checkout in > the next step, without breaking the abstraction here. The remote branch has already been fetched, we just need to create a tracking branch for it. `git fetch origin branch-name` does not create a tracking branch for the remote branch - `git checkout branch-name` does. (Another way to create the tracking branch would be `git branch --track branch-name origin/branch-name` but obviously that would fail if the specified revision was a commit hash instead of a branch name, or if the tracking branch had already been created. We could do it that way however and just ignore the exceptions thrown I suppose.)
http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "checkout", rev], cwd=repo) On 2015/05/05 15:26:52, kzar wrote: > (Another way to create the tracking branch would be `git branch --track > branch-name origin/branch-name` but obviously that would fail if the specified > revision was a commit hash instead of a branch name, or if the tracking branch > had already been created. We could do it that way however and just ignore the > exceptions thrown I suppose.) I think I'd prefer that one. If something went wrong we still get an exception indicating the error on "git fetch" or "git checkout". So It should be safe to ignore errors here. That way we don't need to mess with the abstraction at least. And .update() won't behave incosistent for Mercurial and Git, unexpectly checking out a revision on Git.
http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "checkout", rev], cwd=repo) But either way, this will only work if you specify a branch name. If you specify a commit that isn't in any tracked branch or tag, it won't work as "git fetch --all --tags" only fetches tracking branches and tags.
http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "checkout", rev], cwd=repo) After thinking a little more about the problem and discussing it with Wladimir, we came to the conclusion that we have to implement Git.pull() like below, to make it work like "hg pull" and to eliminate all limitations with git branches: # Make sure the local list of remote branches is up-to-date git fetch # Mark all remote branches as tracking for remote in `git branch --remotes`; do do git branch --track $remote done # Fetch all tracking branches and tags git fetch --all --tags
Patch Set 2 : Make Git.pull track all remote branches http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5629499534213120/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "checkout", rev], cwd=repo) On 2015/05/05 17:35:05, Sebastian Noack wrote: > After thinking a little more about the problem and discussing it with Wladimir, > we came to the conclusion that we have to implement Git.pull() like below, to > make it work like "hg pull" and to eliminate all limitations with git branches: > > # Make sure the local list of remote branches is up-to-date > git fetch > > # Mark all remote branches as tracking > for remote in `git branch --remotes`; do > do git branch --track $remote > done > > # Fetch all tracking branches and tags > git fetch --all --tags Done.
Patch Set 3 : Rebased
http://codereview.adblockplus.org/6068640302497792/diff/5673385510043648/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5673385510043648/ensu... ensure_dependencies.py:104: for match in re.finditer(r"origin/(\S+)", remotes): I think we should track all remote branches. Not only those from the remote called "origin". http://codereview.adblockplus.org/6068640302497792/diff/5673385510043648/ensu... ensure_dependencies.py:106: if local not in ["HEAD", "master"]: I think we also should explicitly mark "master" as tracking. As far as I know "git clone" will only track and fetch the branch that is currently checkout in the remote, which isn't necessarily called "master".
Patch Set 4 : Track "master" branch too http://codereview.adblockplus.org/6068640302497792/diff/5673385510043648/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5673385510043648/ensu... ensure_dependencies.py:104: for match in re.finditer(r"origin/(\S+)", remotes): On 2015/05/06 11:23:10, Sebastian Noack wrote: > I think we should track all remote branches. Not only those from the remote > called "origin". As discussed in IRC I would prefer to keep this as it is. http://codereview.adblockplus.org/6068640302497792/diff/5673385510043648/ensu... ensure_dependencies.py:106: if local not in ["HEAD", "master"]: On 2015/05/06 11:23:10, Sebastian Noack wrote: > I think we also should explicitly mark "master" as tracking. As far as I know > "git clone" will only track and fetch the branch that is currently checkout in > the remote, which isn't necessarily called "master". Done.
Patch Set 5 : Only need to fetch remote branches from origin
http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... File ensure_dependencies.py (left): http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... ensure_dependencies.py:100: subprocess.check_call(["git", "fetch", "--quiet", "--all", "--tags"], cwd=repo) Please specify "origin" instead "--all" here like above. http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... ensure_dependencies.py:100: subprocess.check_call(["git", "fetch", "--quiet", "--all", "--tags"], cwd=repo) If you move the --tags option to the first "git fetch" call, you could make this one conditional, skipping it when no new branches were marked as tracking. This would speedup Git.pull() by factor 2 in the common case. http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... ensure_dependencies.py:104: for match in re.finditer(r"origin/(\S+)", remotes): This regex will match "notorigin/foo" as well. That regex would work correctly: re.finditer(r"(?:^|\s)(origin/(\S+))", remotes) Or maybe even better, you could exclude "HEAD" here as well: re.finditer(r"(?:^|\s)(origin/((?!HEAD$)\S+))", s, re.M) http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... ensure_dependencies.py:105: remote, local = match.group(0), match.group(1) With the regex suggested above, you can simply call match.groups() here: remote, local = match.groups() http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... ensure_dependencies.py:107: with open(os.devnull, "w") as devnull: Nit: Please use "wb", no need to have newlines unified here on Windows.
http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... ensure_dependencies.py:104: for match in re.finditer(r"origin/(\S+)", remotes): On 2015/05/06 12:18:55, Sebastian Noack wrote: > This regex will match "notorigin/foo" as well. That regex would work correctly: > > re.finditer(r"(?:^|\s)(origin/(\S+))", remotes) > > Or maybe even better, you could exclude "HEAD" here as well: > > re.finditer(r"(?:^|\s)(origin/((?!HEAD$)\S+))", s, re.M) Alternatively, not using a regex, might result in a little more but easier to understand code: for line in remotes.splitlines(): if '/' not in line: continue remote, branch = line.strip().split('/', 1) if remote != 'origin' or branch == 'HEAD': continue What do think? I leave it up to you.
Patch Set 6 : Addressed feedback http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... File ensure_dependencies.py (left): http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... ensure_dependencies.py:100: subprocess.check_call(["git", "fetch", "--quiet", "--all", "--tags"], cwd=repo) On 2015/05/06 12:18:55, Sebastian Noack wrote: > If you move the --tags option to the first "git fetch" call, you could make this > one conditional, skipping it when no new branches were marked as tracking. This > would speedup Git.pull() by factor 2 in the common case. Done. http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... ensure_dependencies.py:104: for match in re.finditer(r"origin/(\S+)", remotes): On 2015/05/06 12:18:55, Sebastian Noack wrote: > This regex will match "notorigin/foo" as well. That regex would work correctly: > > re.finditer(r"(?:^|\s)(origin/(\S+))", remotes) > > Or maybe even better, you could exclude "HEAD" here as well: > > re.finditer(r"(?:^|\s)(origin/((?!HEAD$)\S+))", s, re.M) Done. http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... ensure_dependencies.py:104: for match in re.finditer(r"origin/(\S+)", remotes): On 2015/05/06 12:45:00, Sebastian Noack wrote: > On 2015/05/06 12:18:55, Sebastian Noack wrote: > > This regex will match "notorigin/foo" as well. That regex would work > correctly: > > > > re.finditer(r"(?:^|\s)(origin/(\S+))", remotes) > > > > Or maybe even better, you could exclude "HEAD" here as well: > > > > re.finditer(r"(?:^|\s)(origin/((?!HEAD$)\S+))", s, re.M) > > Alternatively, not using a regex, might result in a little more but easier to > understand code: > > for line in remotes.splitlines(): > if '/' not in line: > continue > remote, branch = line.strip().split('/', 1) > if remote != 'origin' or branch == 'HEAD': > continue > > What do think? I leave it up to you. I will stick with the regexp, one line in the output is " origin/HEAD -> origin/master" and so I think the logic would need to be more complex to handle that. http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... ensure_dependencies.py:105: remote, local = match.group(0), match.group(1) On 2015/05/06 12:18:55, Sebastian Noack wrote: > With the regex suggested above, you can simply call match.groups() here: > > remote, local = match.groups() Done. http://codereview.adblockplus.org/6068640302497792/diff/5661458385862656/ensu... ensure_dependencies.py:107: with open(os.devnull, "w") as devnull: On 2015/05/06 12:18:55, Sebastian Noack wrote: > Nit: Please use "wb", no need to have newlines unified here on Windows. Done. http://codereview.adblockplus.org/6068640302497792/diff/5665117697998848/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5665117697998848/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "fetch", "--quiet", "--all", "--tags"], cwd=repo) Note: I left --all here on purpose, I think it's a friendly thing to do to fetch all remotes at this point. If you have manually added remotes it will be helpful, if not then it costs nothing. (Also it's what was done previously anyway.)
http://codereview.adblockplus.org/6068640302497792/diff/5665117697998848/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5665117697998848/ensu... ensure_dependencies.py:100: # First perform a fetch so we have a list of remote branch names and tags This comment isn't 100% accurate. I think it should rather be something like: "Fetch tracking branches and new tags. This will also update the list of available remote branches." http://codereview.adblockplus.org/6068640302497792/diff/5665117697998848/ensu... ensure_dependencies.py:101: subprocess.check_call(["git", "fetch", "--quiet", "--all", "--tags"], cwd=repo) On 2015/05/06 12:51:04, kzar wrote: > Note: I left --all here on purpose, I think it's a friendly thing to do to fetch > all remotes at this point. If you have manually added remotes it will be > helpful, if not then it costs nothing. (Also it's what was done previously > anyway.) Not sure if I agree. But it's not important.
Patch Set 7 : Improved comment http://codereview.adblockplus.org/6068640302497792/diff/5665117697998848/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5665117697998848/ensu... ensure_dependencies.py:100: # First perform a fetch so we have a list of remote branch names and tags On 2015/05/06 13:24:08, Sebastian Noack wrote: > This comment isn't 100% accurate. I think it should rather be something like: > > "Fetch tracking branches and new tags. This will also update the list of > available remote branches." Done.
LGTM
http://codereview.adblockplus.org/6068640302497792/diff/5723151296102400/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5723151296102400/ensu... ensure_dependencies.py:105: for match in re.finditer(r"(?:^|\s)(origin/((?!HEAD(?:$|\s))\S+))", remotes): Is HEAD really worth excluding in the regexp? The following should be much clearer: if local == 'HEAD': continue
http://codereview.adblockplus.org/6068640302497792/diff/5723151296102400/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5723151296102400/ensu... ensure_dependencies.py:105: for match in re.finditer(r"(?:^|\s)(origin/((?!HEAD(?:$|\s))\S+))", remotes): On 2015/05/06 14:27:41, Wladimir Palant wrote: > Is HEAD really worth excluding in the regexp? The following should be much > clearer: > > if local == 'HEAD': > continue We actually had something like that before. I suggested to move the check into the regex however. But if you think it's more clear to have it below fine with me. Theoretically, we don't even need that check. "git branch --track" will just fail for HEAD, and the code below handles it like it's already a tracked branch.
Patch Set 8 : Simplified regexp by not avoiding HEAD http://codereview.adblockplus.org/6068640302497792/diff/5723151296102400/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5723151296102400/ensu... ensure_dependencies.py:105: for match in re.finditer(r"(?:^|\s)(origin/((?!HEAD(?:$|\s))\S+))", remotes): On 2015/05/06 14:31:48, Sebastian Noack wrote: > On 2015/05/06 14:27:41, Wladimir Palant wrote: > > Is HEAD really worth excluding in the regexp? The following should be much > > clearer: > > > > if local == 'HEAD': > > continue > > We actually had something like that before. I suggested to move the check into > the regex however. But if you think it's more clear to have it below fine with > me. Theoretically, we don't even need that check. "git branch --track" will just > fail for HEAD, and the code below handles it like it's already a tracked branch. Yea, I guess we don't need the check at all. Done.
http://codereview.adblockplus.org/6068640302497792/diff/5150194068881408/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5150194068881408/ensu... ensure_dependencies.py:105: for match in re.finditer(r"(?:^|\s)(origin/(\S+))", remotes): I just realized that this regexp will find the branch checkout in the remote repository twice, e.g: origin/HEAD -> origin/master origin/master Not a big deal, but you can simply avoid that without adding any complexity: re.finditer(r"^\s*(origin/(\S+))", remotes, re.M)
Patch Set 9 : Simplify regexp some more http://codereview.adblockplus.org/6068640302497792/diff/5150194068881408/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5150194068881408/ensu... ensure_dependencies.py:105: for match in re.finditer(r"(?:^|\s)(origin/(\S+))", remotes): On 2015/05/06 14:45:12, Sebastian Noack wrote: > I just realized that this regexp will find the branch checkout in the remote > repository twice, e.g: > > origin/HEAD -> origin/master > origin/master > > Not a big deal, but you can simply avoid that without adding any complexity: > > re.finditer(r"^\s*(origin/(\S+))", remotes, re.M) Oh yea, also I realised if we check for the end of the line too we actually can skip HEAD while we're at it :)
http://codereview.adblockplus.org/6068640302497792/diff/5150194068881408/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/6068640302497792/diff/5150194068881408/ensu... ensure_dependencies.py:105: for match in re.finditer(r"(?:^|\s)(origin/(\S+))", remotes): On 2015/05/06 15:11:13, kzar wrote: > On 2015/05/06 14:45:12, Sebastian Noack wrote: > > I just realized that this regexp will find the branch checkout in the remote > > repository twice, e.g: > > > > origin/HEAD -> origin/master > > origin/master > > > > Not a big deal, but you can simply avoid that without adding any complexity: > > > > re.finditer(r"^\s*(origin/(\S+))", remotes, re.M) > > Oh yea, also I realised if we check for the end of the line too we actually can > skip HEAD while we're at it :) Yeah, realized that as well, but slightly preferred to not check for end of line to keep assumptions as small as possible, in case the output changes in future versions or dependent on configuration. However, it doesn't matter much. So LGTM either with or without matching end of line.
LGTM 
|
