Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1805193002:
git cl: Rework Changelist class for Rietveld/Gerrit use. (Closed)
|
Created:
4 years, 9 months ago by tandrii(chromium) Modified:
4 years, 9 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@G100 Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
Descriptiongit cl: Rework Changelist class for Rietveld/Gerrit use.
This adds pluggable codereview-specific implementations into
Changelist class. The specific implementation is chosen at
Changelist automatically, with Rietveld being default for
backwards compatibility.
Gerrit implementation for Gerrit is incomplete, and will be
added in later CLs. However, it is sufficient to ensure
current functionality of this tool is not diminished.
Sadly, the base class isn't completely free from Rietveld
assumptions because of presubmit_support. Apparently, PRESUBMIT
scripts can make use of Rietveld instance for RPCs directly.
This use doesn't make sense for Gerrit, which substitutes
rietveld instance with a dummy object, which raises exception
on any attribute access with a diagnostic message.
This also includes refactoring of some related code which
(ab)used ChangeList. Overall, this CL adds a few extra call to
git config in order to determine which codereview to use, but
but it shouldn't have any performance impact.
BUG=579160
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299462
Patch Set 1 #Patch Set 2 : rebaes #Patch Set 3 : Working! #
Total comments: 22
Patch Set 4 : ready for review #
Total comments: 33
Patch Set 5 : with FetchDescription #
Total comments: 18
Patch Set 6 : rebase #
Total comments: 8
Patch Set 7 : Reivew #Patch Set 8 : sergiyb: apparently pylint doesnt like 'global x' without assignment 'x=' #Messages
Total messages: 57 (34 generated)
Description was changed from ========== git cl close for Gerrit. BUG= ========== to ========== git cl: Refactor ChangeList class for Rietveld/Gerrit use This splits the class into codereview-independent base class and two subclasses. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. A new utility, named ChangeList, will load the right implementation based on circumstances, defaulting to Rietveld for backwards compatibility. For now, the logic is kept simple with an view towards allowing a developer to use either codereview system on the same repo, potentially overriding what repo's codereview.settings file sets. Sadly, the the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds an extra call to git executable, but it shouldn't have any performance impact. BUG= ==========
tandrii@chromium.org changed reviewers: + andybons@chromium.org, phajdan.jr@chromium.org
Description was changed from ========== git cl: Refactor ChangeList class for Rietveld/Gerrit use This splits the class into codereview-independent base class and two subclasses. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. A new utility, named ChangeList, will load the right implementation based on circumstances, defaulting to Rietveld for backwards compatibility. For now, the logic is kept simple with an view towards allowing a developer to use either codereview system on the same repo, potentially overriding what repo's codereview.settings file sets. Sadly, the the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds an extra call to git executable, but it shouldn't have any performance impact. BUG= ========== to ========== git cl: Refactor ChangeList class for Rietveld/Gerrit use This splits the class into codereview-independent base class and two subclasses. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. A new utility, named ChangeList, will load the right implementation based on circumstances, defaulting to Rietveld for backwards compatibility. For now, the logic is kept simple with an view towards allowing a developer to use either codereview system on the same repo, potentially overriding what repo's codereview.settings file sets. Sadly, the the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds an extra call to git executable, but it shouldn't have any performance impact. BUG= ==========
Description was changed from ========== git cl: Refactor ChangeList class for Rietveld/Gerrit use This splits the class into codereview-independent base class and two subclasses. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. A new utility, named ChangeList, will load the right implementation based on circumstances, defaulting to Rietveld for backwards compatibility. For now, the logic is kept simple with an view towards allowing a developer to use either codereview system on the same repo, potentially overriding what repo's codereview.settings file sets. Sadly, the the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds an extra call to git executable, but it shouldn't have any performance impact. BUG= ========== to ========== git cl: Refactor ChangeList class for Rietveld/Gerrit use This splits the class into codereview-independent base class and two subclasses. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. A new utility, named ChangeList, will load the right implementation based on circumstances, defaulting to Rietveld for backwards compatibility. For now, the logic is kept simple with an view towards allowing a developer to use either codereview system on the same repo, potentially overriding what repo's codereview.settings file sets. Sadly, the the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds an extra call to git executable, but it shouldn't have any performance impact. BUG=579160 ==========
please, read the description first. I don't expect to land this very soon, as some of the follow ups are still not working well. So, don't feel obliged to give LG-TM fast.
tandrii@chromium.org changed reviewers: + machenbach@chromium.org
+machenbach@ for early EMEA feedback :)
I CL description: s/Sadly, the the/Sadly, the/ s/with an view/with a view
I'll mainly rubberstamp stuff about the gerrit world as I don't know the details there. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode821 git_cl.py:821: return RunGit(['symbolic-ref', 'HEAD'], The old implementation of ChangeList GetBranch could call this method to avoid duplication. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode836 git_cl.py:836: def Changelist(branchref=None, issue=None, auth_config=None): Crazy stuff like this might break - not sure if it's the only occurrence: https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/git_... https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode844 git_cl.py:844: # class directly. As you write, this piece needs more refactoring in the future. It would be nice if we could determine which class to create outside of those classes. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode847 git_cl.py:847: # Check if there is associated Rietveld Issue first, nit: s/Issue/issue https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode861 git_cl.py:861: return gerrit_cl() Why call? https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode1132 git_cl.py:1132: return self.description Another case where the base class knows about the subclasses, i.e. it knows that a description attribute was set. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode1249 git_cl.py:1249: """Returns name of git config setting which stores issue number.""" Not sure about this documentation and the same below. Maybe copy the same text as used in the subclasses? https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode1476 git_cl.py:1476: return 'branch.%s.gerritpatchset' % self.GetBranch() I maybe wouldn't call the CL a refactoring, as it also contains a substantial amount of new code and new features like this. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode2765 git_cl.py:2765: cl = GerritChangelist() I assume that's for the issue==None case? Or could somebody change the settings while already having an issue and then call upload again? Maybe consider adding this logic to the Changelist factory method and going through that one instead. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode3439 git_cl.py:3439: cl = RietveldChangelist(auth_config=auth_config) Does that deserve a TODO? Or is it a rietveld-only feature?
Description was changed from ========== git cl: Refactor ChangeList class for Rietveld/Gerrit use This splits the class into codereview-independent base class and two subclasses. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. A new utility, named ChangeList, will load the right implementation based on circumstances, defaulting to Rietveld for backwards compatibility. For now, the logic is kept simple with an view towards allowing a developer to use either codereview system on the same repo, potentially overriding what repo's codereview.settings file sets. Sadly, the the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds an extra call to git executable, but it shouldn't have any performance impact. BUG=579160 ========== to ========== git cl: Rework ChangeList class for Rietveld/Gerrit use This splits the class into codereview-independent base class and two subclasses. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. A new utility, named ChangeList, will load the right implementation based on circumstances, defaulting to Rietveld for backwards compatibility. For now, the logic is kept simple with an view towards allowing a developer to use either codereview system on the same repo, potentially overriding what repo's codereview.settings file sets. Sadly, the the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds an extra call to git executable, but it shouldn't have any performance impact. BUG=579160 ==========
Description was changed from ========== git cl: Rework ChangeList class for Rietveld/Gerrit use This splits the class into codereview-independent base class and two subclasses. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. A new utility, named ChangeList, will load the right implementation based on circumstances, defaulting to Rietveld for backwards compatibility. For now, the logic is kept simple with an view towards allowing a developer to use either codereview system on the same repo, potentially overriding what repo's codereview.settings file sets. Sadly, the the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds an extra call to git executable, but it shouldn't have any performance impact. BUG=579160 ========== to ========== git cl: Rework ChangeList class for Rietveld/Gerrit use This splits the class into codereview-independent base class and two subclasses. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. A new utility, named ChangeList, will load the right implementation based on circumstances, defaulting to Rietveld for backwards compatibility. For now, the logic is kept simple with n view towards allowing a developer to use either codereview system on the same repo, potentially overriding what repo's codereview.settings file sets. Sadly, the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds an extra call to git executable, but it shouldn't have any performance impact. BUG=579160 ==========
On 2016/03/16 08:17:15, Michael Achenbach wrote: > I CL description: > s/Sadly, the the/Sadly, the/ > s/with an view/with a view Fixed.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805193002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805193002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== git cl: Rework ChangeList class for Rietveld/Gerrit use This splits the class into codereview-independent base class and two subclasses. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. A new utility, named ChangeList, will load the right implementation based on circumstances, defaulting to Rietveld for backwards compatibility. For now, the logic is kept simple with n view towards allowing a developer to use either codereview system on the same repo, potentially overriding what repo's codereview.settings file sets. Sadly, the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds an extra call to git executable, but it shouldn't have any performance impact. BUG=579160 ========== to ========== git cl: Rework Changelist class for Rietveld/Gerrit use. This adds pluggable codereview-specific implementations into Changelist class. The specific implementation is chosen at Changelist automatically, with Rietveld being default for backwards compatibility. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. Sadly, the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds a few extra call to git config in order to determine which codereview to use, but but it shouldn't have any performance impact. BUG=579160 ==========
tandrii@chromium.org changed reviewers: + sergiyb@chromium.org
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:120001) has been deleted
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805193002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805193002/200001
Patchset #5 (id:160001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:180001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805193002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805193002/220001
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805193002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805193002/240001
Patchset #4 (id:200001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/1805193002/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode821 git_cl.py:821: return RunGit(['symbolic-ref', 'HEAD'], On 2016/03/16 09:28:19, Michael Achenbach wrote: > The old implementation of ChangeList GetBranch could call this method to avoid > duplication. Done. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode836 git_cl.py:836: def Changelist(branchref=None, issue=None, auth_config=None): On 2016/03/16 09:28:19, Michael Achenbach wrote: > Crazy stuff like this might break - not sure if it's the only occurrence: > https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/git_... I'm uploading fixes to the rest of depot_tools. If outsides use it, well, there is only one sure way to figure out = landing this. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode844 git_cl.py:844: # class directly. On 2016/03/16 09:28:19, Michael Achenbach wrote: > As you write, this piece needs more refactoring in the future. It would be nice > if we could determine which class to create outside of those classes. so, i haven't yet decided what signals should be used for decision. But there are three info sources to determine which type of CL: 1. per-repo setting (either codereview.settings or .git/config record) 2. overriding on cmd 3. per-branch setting Currently, only #1 is supported, but I think I'll need #3, and possibly #2. So, for now, i'm merely ensuring we are defaulting to Rietveld, with a possibility for Gerrit override. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode847 git_cl.py:847: # Check if there is associated Rietveld Issue first, On 2016/03/16 09:28:19, Michael Achenbach wrote: > nit: s/Issue/issue Done. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode861 git_cl.py:861: return gerrit_cl() On 2016/03/16 09:28:19, Michael Achenbach wrote: > Why call? bug. I wonder, how the hell did it work in the test. Fixed. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode1132 git_cl.py:1132: return self.description On 2016/03/16 09:28:19, Michael Achenbach wrote: > Another case where the base class knows about the subclasses, i.e. it knows that > a description attribute was set. fixed. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode1249 git_cl.py:1249: """Returns name of git config setting which stores issue number.""" On 2016/03/16 09:28:19, Michael Achenbach wrote: > Not sure about this documentation and the same below. Maybe copy the same text > as used in the subclasses? Removed from subclasses and fixed here, to avoid confusion. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode1476 git_cl.py:1476: return 'branch.%s.gerritpatchset' % self.GetBranch() On 2016/03/16 09:28:19, Michael Achenbach wrote: > I maybe wouldn't call the CL a refactoring, as it also contains a substantial > amount of new code and new features like this. Good eye :) Yeah, this bit is new, indeed. And so is the hook's rietveld replacement. So, I've renamed it to rework. Is that better? https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode2765 git_cl.py:2765: cl = GerritChangelist() On 2016/03/16 09:28:19, Michael Achenbach wrote: > I assume that's for the issue==None case? Or could somebody change the settings > while already having an issue and then call upload again? I have exactly a use case for the latter, though it's complicated enough that I might as well give up on it. For now this is as it is to stay closer to prev. behavior. > Maybe consider adding this logic to the Changelist factory method and going > through that one instead. I considered doing exactly that, but it'd would complicate *this* CL too much, and I don't want to revert-reland it many times :) I'm thus postponing this decision for later. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode3439 git_cl.py:3439: cl = RietveldChangelist(auth_config=auth_config) On 2016/03/16 09:28:19, Michael Achenbach wrote: > Does that deserve a TODO? Or is it a rietveld-only feature? For now, it's sooo Rietveld-specific, that TODO at this line is pointless. The whole "git cl patch" would need Gerrit implementation from scratch = it's more than TODO.
Patchset #4 (id:220001) has been deleted
https://codereview.chromium.org/1805193002/diff/240001/git_cl.py File git_cl.py (left): https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#oldcode1337 git_cl.py:1337: def _RietveldServer(self): OMG, I would never have guessed what this function returns from the name... reminds me of Verified class in the CQ code, but worse. Thanks for renaming it. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode888 git_cl.py:888: if codereview == 'gerrit': Why not use an enum? Or at least define these as constants, so that users can autocomplete to reduce typos. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode893 git_cl.py:893: assert codereview in ('rietveld', 'gerrit') nit: IMHO assert False, 'unsupported codereview' would be enough https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode903 git_cl.py:903: tmp_rietveld = self._codereview_impl # Save Rietveld object. Why not check if it's gerrit first? Then you won't need to save a tmp object here. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode991 git_cl.py:991: 'or verify this branch is set up to track another \n' nit: remove \n in the end - let the terminal break the long line where needed. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1185 git_cl.py:1185: if self._codereview_impl.GetCodereviewServer(): nit: codereview_server = self._codereview_impl.GetCodereviewServer and then re-use twice https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1255 git_cl.py:1255: 'If all fails, contact maruel@') % e) discussed this with tandrii offline: 'maruel@' was here before, but otherwise I'd recommend putting group address rather than direct names, e.g. chrome-infra@... but it's up to tandrii and maruel https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1258 git_cl.py:1258: # Forward methods to codereview specific implementation. I think this was intended to be above the def __getattr__ definition as it seems to apply to the rest of the methods in this class https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1316 git_cl.py:1316: def GetRieveldObjForPresubmit(self): Would be nice to have a doc here to know what kind of object is expected and/or what to return if the implementation can not provide one... or just mention that this is a hack and that no-one should ever implement this in a subclass :-) https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1343 git_cl.py:1343: assert settings nit: please add global settings on the line before... even though we don't modify it here, it's confusing for the reader and requires knowing that it's defined somewhere on the global level https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1376 git_cl.py:1376: '404 (not found)\n' nit: please remove \n everywhere a sentence continues. let the terminal do the breaking... feel free to do it in another patch though https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode2119 git_cl.py:2119: # TODO(tandrii): refactor to use CLs list instead of branches list. Why? Sometimes I use 'git cl status' to see if a branch has an associated CL or not. It's true that I can do the same by checking if it's missing from the list or not, but IMHO it's nice when a branch is listed and explicitly marked as not having an associated CL. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode2616 git_cl.py:2616: cl._codereview_impl.GetAuthConfig())) Since this is also Rietveld-specific and accesses private field, please add a TODO comment in the beginning of RietveldUpload: # TODO(tandrii): Move into implementation class. or better yet actually move RietveldUpload and GerritUpload into _RietveldChangelistImpl and _GerritChangelistImpl classes respectively and wrap both of them with a public Upload method in Changelist. In this case, get rid of cl parameter. The methods can be left here for compatibilty: def RietveldUpload(options, args, cl, change): cl.Upload(options, args, change) def GerritUpload(options, args, cl, change): cl.Upload(options, args, change) https://codereview.chromium.org/1805193002/diff/260001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1514 git_cl.py:1514: def __init__(self, changelist, auth_config=None): please add a comment explaining that auth_config parameter is needed for legacy-compatibility here https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1522 git_cl.py:1522: if self.GetCodereviewServer(): did you miss 'not' here? https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1530 git_cl.py:1530: if self.GetIssue(): I think GetIssue is part of the Changelist class, not _ChangelistCodereviewBase or this class... so this will probably fail https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1540 git_cl.py:1540: parts = urlparse.urlparse(self.GetRemoteUrl()).netloc.split('.') ditto for GetRemoteUrl... https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1567 git_cl.py:1567: 'Either change your PRESUBIT to not use rietveld_obj.%s,\n' nit: please add 'Please' in the beginning of the sentence https://codereview.chromium.org/1805193002/diff/260001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1805193002/diff/260001/tests/git_cl_test.py#n... tests/git_cl_test.py:166: ((['git', 'symbolic-ref', 'HEAD'],), 'master'), not sure why there are so many changes in the call order, but I trust you did a good job and made sure that all of these changes are expected (due to refactoring and due to added Gerrit support) https://codereview.chromium.org/1805193002/diff/260001/tests/git_cl_test.py#n... tests/git_cl_test.py:555: ((['git', 'config', 'rietveld.server'],), ''), why is this called twice now?
Thanks for detailed review. I think I've addressed them all. PTAL Also, FTR, this is how this works end-to-end for Gerrit for Win/Linux/Mac: https://codereview.chromium.org/1662993002/#ps1070001 https://codereview.chromium.org/1805193002/diff/240001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode888 git_cl.py:888: if codereview == 'gerrit': On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > Why not use an enum? Or at least define these as constants, so that users can > autocomplete to reduce typos. because it's python, and having RIETVELD = 'rietveld' is pointless. + my editor autocompletes strings, too :) https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode893 git_cl.py:893: assert codereview in ('rietveld', 'gerrit') On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > nit: IMHO > > assert False, 'unsupported codereview' > > would be enough yes, but AssertionError is much nicer when you see what's allowed. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode903 git_cl.py:903: tmp_rietveld = self._codereview_impl # Save Rietveld object. On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > Why not check if it's gerrit first? Then you won't need to save a tmp object > here. because if Rietveld issue is defined, it should be rietveld, even if gerrit issue is also defined. This is a safeguard. Thus, doing it reverse requires having a tmp_gerrit. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode991 git_cl.py:991: 'or verify this branch is set up to track another \n' On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > nit: remove \n in the end - let the terminal break the long line where needed. well, it's apparently customary in this file to break ourselves. And IMO, in this case specifically it's nicer. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1185 git_cl.py:1185: if self._codereview_impl.GetCodereviewServer(): On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > nit: codereview_server = self._codereview_impl.GetCodereviewServer and then > re-use twice Done. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1255 git_cl.py:1255: 'If all fails, contact maruel@') % e) On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > discussed this with tandrii offline: 'maruel@' was here before, but otherwise > I'd recommend putting group address rather than direct names, e.g. > chrome-infra@... but it's up to tandrii and maruel Acknowledged. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1258 git_cl.py:1258: # Forward methods to codereview specific implementation. On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > I think this was intended to be above the def __getattr__ definition as it seems > to apply to the rest of the methods in this class Yes, though I still need specific comment for why __getattr__ is needed. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1316 git_cl.py:1316: def GetRieveldObjForPresubmit(self): On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > Would be nice to have a doc here to know what kind of object is expected and/or > what to return if the implementation can not provide one... or just mention that > this is a hack and that no-one should ever implement this in a subclass :-) Done. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1343 git_cl.py:1343: assert settings On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > nit: please add > > global settings > > on the line before... even though we don't modify it here, it's confusing for > the reader and requires knowing that it's defined somewhere on the global level good point. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1376 git_cl.py:1376: '404 (not found)\n' On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > nit: please remove \n everywhere a sentence continues. let the terminal do the > breaking... feel free to do it in another patch though see above - this seems to be the style here. And this message itself is certainly a copy-paste from before. leaving as is. that said, I actually prefer terminal breaking here. Ah, also mind that some scripts parse git cl output using regexes assuming end of line. It's bad, yes, but I don't want to revert my CL for the reason of breaking them :( https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode2119 git_cl.py:2119: # TODO(tandrii): refactor to use CLs list instead of branches list. On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > Why? Sometimes I use 'git cl status' to see if a branch has an associated CL or > not. It's true that I can do the same by checking if it's missing from the list > or not, but IMHO it's nice when a branch is listed and explicitly marked as not > having an associated CL. see line 2111 - there is a list of branches. then they get list of changes for each branch (right above) - nothing is missed, don't worry. but line below (2120) - they get branch names the way CLs see them. Now, the only reason is that they want branch names without "refs/heads" apparently. And then inside the loop, for each branch they create a new CL object, again. WHY???? So, my TODO is to rewrite loop below to be based on CL objects, not branches. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode2616 git_cl.py:2616: cl._codereview_impl.GetAuthConfig())) On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > Since this is also Rietveld-specific and accesses private field, please add a > TODO comment in the beginning of RietveldUpload: > > # TODO(tandrii): Move into implementation class. > > or better yet actually move RietveldUpload and GerritUpload into > _RietveldChangelistImpl and _GerritChangelistImpl classes respectively and wrap > both of them with a public Upload method in Changelist. In this case, get rid of > cl parameter. The methods can be left here for compatibilty: > > > def RietveldUpload(options, args, cl, change): > cl.Upload(options, args, change) > > > def GerritUpload(options, args, cl, change): > cl.Upload(options, args, change) that's exactly what I intended to do! But not in this CL :) https://codereview.chromium.org/1805193002/diff/260001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1285 git_cl.py:1285: # TODO(tandrii): maybe clean up _GerritChangelistImpl and sergiyb@ this is the ugly hack that you've missed. That said, I probably should fix it, but I'm still not 100% that tests provide good goverage. https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1514 git_cl.py:1514: def __init__(self, changelist, auth_config=None): On 2016/03/21 17:17:07, Sergiy Byelozyorov wrote: > please add a comment explaining that auth_config parameter is needed for > legacy-compatibility here good point. https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1522 git_cl.py:1522: if self.GetCodereviewServer(): On 2016/03/21 17:17:07, Sergiy Byelozyorov wrote: > did you miss 'not' here? yeah, totally buggy. Fixed in next PS. https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1530 git_cl.py:1530: if self.GetIssue(): On 2016/03/21 17:17:07, Sergiy Byelozyorov wrote: > I think GetIssue is part of the Changelist class, not _ChangelistCodereviewBase > or this class... so this will probably fail no, because of a __getattr__ in ChangeListImplBase, which forward to Changelist class. I am not changing now,though I'm considering to change this everywhere. The problem is that it's less readable, though easier to navigate code. https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1540 git_cl.py:1540: parts = urlparse.urlparse(self.GetRemoteUrl()).netloc.split('.') On 2016/03/21 17:17:07, Sergiy Byelozyorov wrote: > ditto for GetRemoteUrl... ditto. https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1567 git_cl.py:1567: 'Either change your PRESUBIT to not use rietveld_obj.%s,\n' On 2016/03/21 17:17:07, Sergiy Byelozyorov wrote: > nit: please add 'Please' in the beginning of the sentence Done. https://codereview.chromium.org/1805193002/diff/260001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1805193002/diff/260001/tests/git_cl_test.py#n... tests/git_cl_test.py:166: ((['git', 'symbolic-ref', 'HEAD'],), 'master'), On 2016/03/21 17:17:07, Sergiy Byelozyorov wrote: > not sure why there are so many changes in the call order, but I trust you did a > good job and made sure that all of these changes are expected (due to > refactoring and due to added Gerrit support) well, the basic idea for review is this: some git calls are actually mutating, such as [git config KEY VALUE]. Changes to these order or content should be looked at. Change of order of reading (e.g., [git config KEY]) is not very important. that said, i'm using "git cl" with this patch in my depot_tools for last week, and so far it works, so I'm confident it works (for i fixed a few breakages already :D). https://codereview.chromium.org/1805193002/diff/260001/tests/git_cl_test.py#n... tests/git_cl_test.py:555: ((['git', 'config', 'rietveld.server'],), ''), On 2016/03/21 17:17:07, Sergiy Byelozyorov wrote: > why is this called twice now? autoupdate + first server check is from settings loading. It was here before - see on the left. the new server check line is from loading a CL.
Patchset #6 (id:280001) has been deleted
Patchset #6 (id:300001) has been deleted
Patchset #6 (id:320001) has been deleted
lgtm https://codereview.chromium.org/1805193002/diff/240001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode888 git_cl.py:888: if codereview == 'gerrit': On 2016/03/22 17:41:52, tandrii(chromium) wrote: > On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > > Why not use an enum? Or at least define these as constants, so that users can > > autocomplete to reduce typos. > > because it's python, and having RIETVELD = 'rietveld' is pointless. > + my editor autocompletes strings, too :) Acknowledged. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode893 git_cl.py:893: assert codereview in ('rietveld', 'gerrit') On 2016/03/22 17:41:52, tandrii(chromium) wrote: > On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > > nit: IMHO > > > > assert False, 'unsupported codereview' > > > > would be enough > > yes, but AssertionError is much nicer when you see what's allowed. Acknowledged. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode903 git_cl.py:903: tmp_rietveld = self._codereview_impl # Save Rietveld object. On 2016/03/22 17:41:52, tandrii(chromium) wrote: > On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > > Why not check if it's gerrit first? Then you won't need to save a tmp object > > here. > > because if Rietveld issue is defined, it should be rietveld, even if gerrit > issue is also defined. This is a safeguard. Thus, doing it reverse requires > having a tmp_gerrit. Acknowledged. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode991 git_cl.py:991: 'or verify this branch is set up to track another \n' On 2016/03/22 17:41:52, tandrii(chromium) wrote: > On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > > nit: remove \n in the end - let the terminal break the long line where needed. > > well, it's apparently customary in this file to break ourselves. And IMO, in > this case specifically it's nicer. Acknowledged. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1258 git_cl.py:1258: # Forward methods to codereview specific implementation. On 2016/03/22 17:41:52, tandrii(chromium) wrote: > On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > > I think this was intended to be above the def __getattr__ definition as it > seems > > to apply to the rest of the methods in this class > > Yes, though I still need specific comment for why __getattr__ is needed. Acknowledged. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode1376 git_cl.py:1376: '404 (not found)\n' On 2016/03/22 17:41:52, tandrii(chromium) wrote: > some scripts parse git cl output what a can of worms... > I don't want to revert my CL for the reason of breaking them :( Ack. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode2119 git_cl.py:2119: # TODO(tandrii): refactor to use CLs list instead of branches list. On 2016/03/22 17:41:52, tandrii(chromium) wrote: > On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > > Why? Sometimes I use 'git cl status' to see if a branch has an associated CL > or > > not. It's true that I can do the same by checking if it's missing from the > list > > or not, but IMHO it's nice when a branch is listed and explicitly marked as > not > > having an associated CL. > > see line 2111 - there is a list of branches. > then they get list of changes for each branch (right above) - nothing is missed, > don't worry. > but line below (2120) - they get branch names the way CLs see them. Now, the > only reason is that they want branch names without "refs/heads" apparently. And > then inside the loop, for each branch they create a new CL object, again. > WHY???? > So, my TODO is to rewrite loop below to be based on CL objects, not branches. Acknowledged. https://codereview.chromium.org/1805193002/diff/240001/git_cl.py#newcode2616 git_cl.py:2616: cl._codereview_impl.GetAuthConfig())) On 2016/03/22 17:41:52, tandrii(chromium) wrote: > On 2016/03/21 17:17:06, Sergiy Byelozyorov wrote: > > Since this is also Rietveld-specific and accesses private field, please add a > > TODO comment in the beginning of RietveldUpload: > > > > # TODO(tandrii): Move into implementation class. > > > > or better yet actually move RietveldUpload and GerritUpload into > > _RietveldChangelistImpl and _GerritChangelistImpl classes respectively and > wrap > > both of them with a public Upload method in Changelist. In this case, get rid > of > > cl parameter. The methods can be left here for compatibilty: > > > > > > def RietveldUpload(options, args, cl, change): > > cl.Upload(options, args, change) > > > > > > def GerritUpload(options, args, cl, change): > > cl.Upload(options, args, change) > > that's exactly what I intended to do! But not in this CL :) Acknowledged. https://codereview.chromium.org/1805193002/diff/260001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1285 git_cl.py:1285: # TODO(tandrii): maybe clean up _GerritChangelistImpl and On 2016/03/22 17:41:52, tandrii(chromium) wrote: > sergiyb@ this is the ugly hack that you've missed. That said, I probably should > fix it, but I'm still not 100% that tests provide good goverage. I didn't miss it. I saw your comment and decided that it's fine. Or do you mean that I missed it with respect to some other comment? https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1530 git_cl.py:1530: if self.GetIssue(): On 2016/03/22 17:41:52, tandrii(chromium) wrote: > On 2016/03/21 17:17:07, Sergiy Byelozyorov wrote: > > I think GetIssue is part of the Changelist class, not > _ChangelistCodereviewBase > > or this class... so this will probably fail > > no, because of a __getattr__ in ChangeListImplBase, which forward to Changelist > class. I am not changing now,though I'm considering to change this everywhere. > The problem is that it's less readable, though easier to navigate code. Acknowledged. https://codereview.chromium.org/1805193002/diff/260001/git_cl.py#newcode1540 git_cl.py:1540: parts = urlparse.urlparse(self.GetRemoteUrl()).netloc.split('.') On 2016/03/22 17:41:52, tandrii(chromium) wrote: > On 2016/03/21 17:17:07, Sergiy Byelozyorov wrote: > > ditto for GetRemoteUrl... > > ditto. Acknowledged.
lgtm - only skimmed through the new implementation. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode2765 git_cl.py:2765: cl = GerritChangelist() On 2016/03/21 15:06:05, tandrii(chromium) wrote: > On 2016/03/16 09:28:19, Michael Achenbach wrote: > > I assume that's for the issue==None case? Or could somebody change the > settings > > while already having an issue and then call upload again? > I have exactly a use case for the latter, though it's complicated enough that I > might as well give up on it. For now this is as it is to stay closer to prev. > behavior. > > > Maybe consider adding this logic to the Changelist factory method and going > > through that one instead. > > I considered doing exactly that, but it'd would complicate *this* CL too much, > and I don't want to revert-reland it many times :) > I'm thus postponing this decision for later. Acknowledged. https://codereview.chromium.org/1805193002/diff/40001/git_cl.py#newcode3439 git_cl.py:3439: cl = RietveldChangelist(auth_config=auth_config) On 2016/03/21 15:06:05, tandrii(chromium) wrote: > On 2016/03/16 09:28:19, Michael Achenbach wrote: > > Does that deserve a TODO? Or is it a rietveld-only feature? > > For now, it's sooo Rietveld-specific, that TODO at this line is pointless. The > whole "git cl patch" would need Gerrit implementation from scratch = it's more > than TODO. Acknowledged. https://codereview.chromium.org/1805193002/diff/340001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1805193002/diff/340001/git_cl.py#newcode890 git_cl.py:890: self._codereview_impl = _GerritChangelistImpl(self) Why not pass kwargs here too? https://codereview.chromium.org/1805193002/diff/340001/git_cl.py#newcode900 git_cl.py:900: self._codereview_impl = _RietveldChangelistImpl(self, **kwargs) Maybe for another CL to remove technical dept: I find it bit awkward to instantiate a class in order to find out if the class is the right one to be instantiated. https://codereview.chromium.org/1805193002/diff/340001/git_cl.py#newcode907 git_cl.py:907: self._codereview_impl = _GerritChangelistImpl(self) here too https://codereview.chromium.org/1805193002/diff/340001/git_cl.py#newcode910 git_cl.py:910: # If Gerrit is set repo-wide => Gerrit. Maybe add a comment that from these lines on this is a new issue that has not been uploaded before.
https://codereview.chromium.org/1805193002/diff/340001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1805193002/diff/340001/git_cl.py#newcode890 git_cl.py:890: self._codereview_impl = _GerritChangelistImpl(self) On 2016/03/24 09:58:12, Michael Achenbach wrote: > Why not pass kwargs here too? bug, fixed. https://codereview.chromium.org/1805193002/diff/340001/git_cl.py#newcode900 git_cl.py:900: self._codereview_impl = _RietveldChangelistImpl(self, **kwargs) On 2016/03/24 09:58:12, Michael Achenbach wrote: > Maybe for another CL to remove technical dept: I find it bit awkward to > instantiate a class in order to find out if the class is the right one to be > instantiated. will do as follow up with staticmethods. https://codereview.chromium.org/1805193002/diff/340001/git_cl.py#newcode907 git_cl.py:907: self._codereview_impl = _GerritChangelistImpl(self) On 2016/03/24 09:58:12, Michael Achenbach wrote: > here too Done. https://codereview.chromium.org/1805193002/diff/340001/git_cl.py#newcode910 git_cl.py:910: # If Gerrit is set repo-wide => Gerrit. On 2016/03/24 09:58:12, Michael Achenbach wrote: > Maybe add a comment that from these lines on this is a new issue that has not > been uploaded before. Done.
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergiyb@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1805193002/#ps360001 (title: "Reivew")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805193002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805193002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Depot Tools Presubmit on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Depot%20Tools%20Presubm...)
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergiyb@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1805193002/#ps380001 (title: "sergiyb: apparently pylint doesnt like 'global x' without assignment 'x='")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805193002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805193002/380001
Message was sent while issue was closed.
Description was changed from ========== git cl: Rework Changelist class for Rietveld/Gerrit use. This adds pluggable codereview-specific implementations into Changelist class. The specific implementation is chosen at Changelist automatically, with Rietveld being default for backwards compatibility. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. Sadly, the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds a few extra call to git config in order to determine which codereview to use, but but it shouldn't have any performance impact. BUG=579160 ========== to ========== git cl: Rework Changelist class for Rietveld/Gerrit use. This adds pluggable codereview-specific implementations into Changelist class. The specific implementation is chosen at Changelist automatically, with Rietveld being default for backwards compatibility. Gerrit implementation for Gerrit is incomplete, and will be added in later CLs. However, it is sufficient to ensure current functionality of this tool is not diminished. Sadly, the base class isn't completely free from Rietveld assumptions because of presubmit_support. Apparently, PRESUBMIT scripts can make use of Rietveld instance for RPCs directly. This use doesn't make sense for Gerrit, which substitutes rietveld instance with a dummy object, which raises exception on any attribute access with a diagnostic message. This also includes refactoring of some related code which (ab)used ChangeList. Overall, this CL adds a few extra call to git config in order to determine which codereview to use, but but it shouldn't have any performance impact. BUG=579160 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299462 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:380001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=299462
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:380001) has been created in https://codereview.chromium.org/1831813003/ by tandrii@chromium.org. The reason for reverting is: Broke one project presubmit.. |
