Fix a bug with finding Git remotes when not set on a branch.

Review Request #10773 — Created Nov. 1, 2019 and updated

solarmist
RBTools
master
894b290...
rbtools

Fix bug in finding remotes where _find_remote would return the remote name including the newline character which made self._rev_list_youngest_remote_ancestor fail.

This is due to str.splitlines not dropping the '\n' after splitting the lines.

Bug in _find_remote

rbt post -u --parent 9902a1599041080763bcf0d3fe610019a56d2c6f -r 1863684
> /Users/jolson/repos/svin/build/svin/venv/lib/python3.7/site-packages/rbtools/clients/git.py(1285)_find_remote()
-> if len(all_remotes) >= 1:
(Pdb) all_remotes
['origin\n']
(Pdb) self._execute(['git', 'remote'])
'origin\n'
(Pdb) self._execute(['git', 'remote'], split_lines=True)
['origin\n']
Description From Last Updated

Review request summaries and descriptions need to be informative enough to let us easily understand the purpose of the change ...

chipx86chipx86

Can you put this blank line back? It's not relevant for the change, and we always ensure a blank line ...

chipx86chipx86

E501 line too long (103 > 79 characters)

reviewbotreviewbot

F812 list comprehension redefines 'remote' from line 1447

reviewbotreviewbot

As Review Bot notes, this line is too long and overrides a variable we just used. This would be better ...

chipx86chipx86

E128 continuation line under-indented for visual indent

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

solarmist
chipx86
  1. 
      
  2. Review request summaries and descriptions need to be informative enough to let us easily understand the purpose of the change even years from now without having to dig into the code. This is an important part of how we maintain the codebase.

    For instance, the summary would benefit, at a minimum, from saying that this is a problem with finding Git remotes. It'd also be helpful to have a hint as to when this occurs. So: Fix a bug with finding Git remotes when not set on a branch.

    The description should go over what went wrong, why, and how it was fixed.

    You can see plenty of examples in the history of any of our trees, but we also go over this in our guide on Writing Good Change Descriptions.

    1. Sure. Updated the summary and description.

  3. rbtools/clients/git.py (Diff revision 1)
     
     
     
     

    Can you put this blank line back? It's not relevant for the change, and we always ensure a blank line before/after new blocks.

    1. Yup, I thought I did that before I published it.

  4. rbtools/clients/git.py (Diff revision 1)
     
     

    As Review Bot notes, this line is too long and overrides a variable we just used. This would be better written as:

    all_remotes = [
        _remote.strip()
        for _remote in self._execute(['git', 'remote'],
                                     split_lines=True)
    ]
    
    1. I saw that. Fixed.

    2. Looks much, better but not quite there. Can you use the _ as the prefix instead of suffix (it's more visible that way, and designates an intent to be private within the statement it's in rather than a sort of "varname2")? We also style our list/dictionary/etc. comprehensions in the form of:

      result = [
          iter_value
          iteration
          conditions
      ]
      

      This helps keep comprehensions uniform and makes them easier to expand without rewriting any part of the statement.

    3. Fixed.

  5. 
      
solarmist
solarmist
Review request changed

Commit:

-a1d26ea79525e1780323985e3695f11f22a02e99
+b2ee75a90a127e779e11a47e14d361c9379c8583

Diff:

Revision 3 (+4 -1)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

solarmist
Review request changed

Commit:

-b2ee75a90a127e779e11a47e14d361c9379c8583
+894b290ecaa6ee8c555f090068f48e06dcf9dd8b

Diff:

Revision 4 (+4 -1)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...