<p style="white-space: pre-wrap; word-wrap: break-word;">patch set coming up...</p><p><a href="https://gerrit.osmocom.org/11560">View Change</a></p><p>13 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@2">Patch Set #4, Line 2:</a> <code style="font-family:monospace,monospace">argparse</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">argparse is not used. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">the reason why argparse will not work so well is passing arguments to git. consider:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  gits clean -dxf</pre><p style="white-space: pre-wrap; word-wrap: break-word;">then 'gits' will argparse the -dxf arguments, but they were intended to be passed on to each 'cd subdir/; git clean -dxf' cmdline.</p><p style="white-space: pre-wrap; word-wrap: break-word;">good catch about -h</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@11">Patch Set #4, Line 11:</a> <code style="font-family:monospace,monospace">re_status_mods = re.compile('^\t(modified|deleted):.*')</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This is only used once in the code. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think I'd rather precompile all others here as well</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">How about differentiating the command from the output it produces? For example with a leading '>' ch […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">can do. I think I'd use the '+ ' ala 'set -x'</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@37">Patch Set #4, Line 37:</a> <code style="font-family:monospace,monospace">    sys.stderr.flush()</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why flush the output only when the section_marker is needed?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">not sure. probably came from previous code... should probably flush above 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@45">Patch Set #4, Line 45:</a> <code style="font-family:monospace,monospace">m</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">how about "branch" as variable name?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">it's a re "match object"</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@48">Patch Set #4, Line 48:</a> <code style="font-family:monospace,monospace">  return m.group(1)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this one is the branch</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><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">With git 2.11.0 I get: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why make a list first, and then join it?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">In this instance it was about conditionally adding strings. a bit overboard, yes</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If we print out something like "> cd %s && %s" before the output of the command, it would be consist […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I like more visible markers in this case</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">How about "SkipThisRepo"? repo is commonly used to abbreviate repository, so when reading "repos" I' […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I've always thought of "repos", not "repo", had the same "conflict" with stsp many years ago... :)</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">how about: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I see, but I prefer listing each valid_answers case in its separate 'if'</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'm not sure if it's a good idea to perform "do" by default - whenever there's a typo in any of the  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">but I want to write 'gits fetch' :)<br>and don't want to write a shim for each and every one of those...</p><p style="white-space: pre-wrap; word-wrap: break-word;">you're right of course, maybe a shim then.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Since there are not Python coding guidelines in the wiki yet: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">wow, you're just as obsessed with details like I am!</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: Tue, 06 Nov 2018 15:31:09 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>