Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11560 )
Change subject: replace src/* git scripts with a single src/gits ...................................................................... Patch Set 4: (13 comments) patch set coming up... https://gerrit.osmocom.org/#/c/11560/4/src/gits File src/gits: https://gerrit.osmocom.org/#/c/11560/4/src/gits@2 PS4, Line 2: argparse > argparse is not used. […] the reason why argparse will not work so well is passing arguments to git. consider: gits clean -dxf then 'gits' will argparse the -dxf arguments, but they were intended to be passed on to each 'cd subdir/; git clean -dxf' cmdline. good catch about -h https://gerrit.osmocom.org/#/c/11560/4/src/gits@11 PS4, Line 11: re_status_mods = re.compile('^\t(modified|deleted):.*') > This is only used once in the code. […] I think I'd rather precompile all others here as well https://gerrit.osmocom.org/#/c/11560/4/src/gits@29 PS4, Line 29: print(' '.join(cmd)) > How about differentiating the command from the output it produces? For > example with a leading '>' ch […] can do. I think I'd use the '+ ' ala 'set -x' https://gerrit.osmocom.org/#/c/11560/4/src/gits@37 PS4, Line 37: sys.stderr.flush() > why flush the output only when the section_marker is needed? not sure. probably came from previous code... should probably flush above instead https://gerrit.osmocom.org/#/c/11560/4/src/gits@45 PS4, Line 45: m > how about "branch" as variable name? it's a re "match object" https://gerrit.osmocom.org/#/c/11560/4/src/gits@48 PS4, Line 48: return m.group(1) this one is the branch https://gerrit.osmocom.org/#/c/11560/4/src/gits@68 PS4, Line 68: up to date > With git 2.11.0 I get: […] yeah, that's the main ultra drawback of this script so far. I really really shouldn't use porcelain. I'm not sure how to use plumbing though, so far it's "works-for-me" -- do you know how to fix that? https://gerrit.osmocom.org/#/c/11560/4/src/gits@122 PS4, Line 122: strs.append(''.join(branch_info)) > why make a list first, and then join it? that's my common string composition pattern. If I do "str = str + more", then I create an ever growing string in a loop. If I append to a list and later join, I only create one large string in the end. Well, it's premature optimization, but I prefer that style. In this instance it was about conditionally adding strings. a bit overboard, yes https://gerrit.osmocom.org/#/c/11560/4/src/gits@194 PS4, Line 194: print('\n===== %s =====' % git_dir) > If we print out something like "> cd %s && %s" before the output of the > command, it would be consist […] I like more visible markers in this case https://gerrit.osmocom.org/#/c/11560/4/src/gits@200 PS4, Line 200: SkipThisRepos > How about "SkipThisRepo"? repo is commonly used to abbreviate repository, so > when reading "repos" I' […] I've always thought of "repos", not "repo", had the same "conflict" with stsp many years ago... :) https://gerrit.osmocom.org/#/c/11560/4/src/gits@223 PS4, Line 223: if v == answer: : return answer : if v == '*': : return answer > how about: […] I see, but I prefer listing each valid_answers case in its separate 'if' https://gerrit.osmocom.org/#/c/11560/4/src/gits@359 PS4, Line 359: cmd_do(sys.argv[1:]) > I'm not sure if it's a good idea to perform "do" by default - whenever > there's a typo in any of the […] but I want to write 'gits fetch' :) and don't want to write a shim for each and every one of those... you're right of course, maybe a shim then. https://gerrit.osmocom.org/#/c/11560/4/src/gits@362 PS4, Line 362: # vim: shiftwidth=2 expandtab tabstop=2 > Since there are not Python coding guidelines in the wiki yet: […] wow, you're just as obsessed with details like I am! -- To view, visit https://gerrit.osmocom.org/11560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-dev Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I579e7af26d76d5c5d83b2349695456bc7b54f5a2 Gerrit-Change-Number: 11560 Gerrit-PatchSet: 4 Gerrit-Owner: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: osmith <osm...@sysmocom.de> Gerrit-Comment-Date: Tue, 06 Nov 2018 15:31:09 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No