Don't give "dirty" warning only because of dirty submodules

Review Request #8627 — Created Jan. 17, 2017 and submitted

Information

VZ
RBTools
master
0622e67...

Reviewers

Ignore changes in submodules working directories when checking for pending
changes: it is common to have some untracked files in the submodules (e.g.
those produced during the build) and sometimes some existing files could be
modified as well.

Also, as diff doesn't include changes to the submodules in any case, it seems
logical to exclude them from the dirty check too.


I'm quite sure that the current behaviour is not ideal because a submodule with some untracked files in it really shouldn't result in

WARNING: Your working directory is not clean. Any changes which have not been committed to a branch will not be included in your review request.

as it currently does and so I think it's clear that --ignore-submodules option should be used. However I'm not sure if my patch is ideal neither: I can see arguments both in favour of changing the value of this option to untracked (because a really dirty submodule could contain some changes that were meant to be committed and forgotten) and all (because it would be consistent to ignore the submodules completely for consistency with make_diff). So while I believe that using dirty here is the happy middle ground, I would totally understand if you changed it to something else. But, again, IMO at the very least --ignore-submodules=untracked should be used because it's pretty common to have untracked files in submodules and the warning is just distracting and teaches users to simply ignore it.

To be honest, not much: just reran the same rbt post command that warned me about working directory not being clean before and confirmed that the warning didn't happen any more.

reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/git.py
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
VZ
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.7.x (ba946be)
Loading...