<p>Patch set 4:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/11560">View Change</a></p><p>19 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits">File src/gits:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@1">Patch Set #4, Line 1:</a> <code style="font-family:monospace,monospace">#!/usr/bin/env python3</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add the copyright and SPDX license lines here?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@2">Patch Set #4, Line 2:</a> <code style="font-family:monospace,monospace">argparse</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">argparse is not used.</p><p style="white-space: pre-wrap; word-wrap: break-word;">But I would highly recommend to use it:</p><ul><li>the help output would look like in any other python program</li><li>right now "./gits -h" runs "git -h" in every directory instead of printing the help output</li><li>no wheel-reinventing for checking the arguments, cmd_help(), having aliases</li></ul></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@11">Patch Set #4, Line 11:</a> <code style="font-family:monospace,monospace">re_status_mods = re.compile('^\t(modified|deleted):.*')</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is only used once in the code. There are other regex patterns, that are not global variables in this file. So I'd recommend to follow that pattern and move this to the function where it is needed.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@29">Patch Set #4, Line 29:</a> <code style="font-family:monospace,monospace">    print(' '.join(cmd))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">How about differentiating the command from the output it produces? For example with a leading '>' character:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  > git -C osmo-mgw status<br>  On branch master<br>  Your branch is up-to-date with 'origin/master'.<br>  nothing to commit, working dir clean</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@37">Patch Set #4, Line 37:</a> <code style="font-family:monospace,monospace">    sys.stderr.flush()</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">why flush the output only when the section_marker is needed?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@45">Patch Set #4, Line 45:</a> <code style="font-family:monospace,monospace">m</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">how about "branch" as variable name?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@68">Patch Set #4, Line 68:</a> <code style="font-family:monospace,monospace">up to date</code></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">With git 2.11.0 I get:<br>  unknown status str: "Your branch is up-to-date with 'origin/master'."</pre><p style="white-space: pre-wrap; word-wrap: break-word;">(There's "git status --porcelain" which is meant to be parsed, but it doesn't print the branch line - maybe there's an appropriate git plumbing command with stable output that can be used instead?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@79">Patch Set #4, Line 79:</a> <code style="font-family:monospace,monospace">m</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">How about "status"?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@87">Patch Set #4, Line 87:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">here are two empty lines after the function, mostly this file has one empty line instead. PEP8 would be using two empty lines between each function.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@89">Patch Set #4, Line 89:</a> <code style="font-family:monospace,monospace">  '''return a list of strings: [branchname, info0, info1,...]'''</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">it's not clear what info0, info1 etc. would be. Maybe add examples with actual strings?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@91">Patch Set #4, Line 91:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">interesting_branch_names = [ 'master' ]<br><br>  strs = [git_dir,]<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(inconsistent: [ text ] vs [text,])</p><p style="white-space: pre-wrap; word-wrap: break-word;">How about renaming "strs" to "ret" to indicate that this will be returned?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@122">Patch Set #4, Line 122:</a> <code style="font-family:monospace,monospace">    strs.append(''.join(branch_info))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">why make a list first, and then join it?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@194">Patch Set #4, Line 194:</a> <code style="font-family:monospace,monospace">    print('\n===== %s =====' % git_dir)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If we print out something like "> cd %s && %s" before the output of the command, it would be consistent with executing git in every folder.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@200">Patch Set #4, Line 200:</a> <code style="font-family:monospace,monospace">SkipThisRepos</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">How about "SkipThisRepo"? repo is commonly used to abbreviate repository, so when reading "repos" I'm thinking this is a plural form.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@207">Patch Set #4, Line 207:</a> <code style="font-family:monospace,monospace">repos</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"repo"?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@223">Patch Set #4, Line 223:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">      if v == answer:<br>        return answer<br>      if v == '*':<br>        return answer<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">how about:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  if v in (answer, "*"):<br>    return answer</pre><p style="white-space: pre-wrap; word-wrap: break-word;">(somehow gerrit puts this comment in a single line without writing something below the code block)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@227">Patch Set #4, Line 227:</a> <code style="font-family:monospace,monospace">      if v == '+' and len(answer) > 0:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(the > 0 is not needed)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@359">Patch Set #4, Line 359:</a> <code style="font-family:monospace,monospace">    cmd_do(sys.argv[1:])</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm not sure if it's a good idea to perform "do" by default - whenever there's a typo in any of the actions, it will be executed in every directory.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11560/4/src/gits@362">Patch Set #4, Line 362:</a> <code style="font-family:monospace,monospace"># vim: shiftwidth=2 expandtab tabstop=2</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since there are not Python coding guidelines in the wiki yet:<br>https://osmocom.org/projects/cellular-infrastructure/wiki/Coding_standards</p><p style="white-space: pre-wrap; word-wrap: break-word;">How about following PEP8 (Style Guide for Python Code) instead?<br>https://www.python.org/dev/peps/pep-0008/</p><p style="white-space: pre-wrap; word-wrap: break-word;">"Use 4 spaces per indentation level."</p><p style="white-space: pre-wrap; word-wrap: break-word;">There are nice tools like "flake8" which automatically check the formatting, and also auto formatting tools like "autopep8".</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/11560">change 11560</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/11560"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-dev </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I579e7af26d76d5c5d83b2349695456bc7b54f5a2 </div>
<div style="display:none"> Gerrit-Change-Number: 11560 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 05 Nov 2018 11:31:27 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>