Summary

Adding uncommitted flag to support post staged but not commit changes

Review Request #6879 — Created Feb. 1, 2015 and discarded Jan. 29, 2019, 2:42 p.m.

Information

Owner

viettran1989

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

Problem:

While the common workflow is to post committed changes, some people have workflows where they want to post changes that are uncommitted, and have to resort to generating a diff manually and posting it using rbt post --diff-filename=. 
This work would add an --uncommitted flag to rbt post that would generate diffs for uncommitted changes instead.
Approach:

Adding can_diff_committed capability to all clients, and let git set that to True to support it.

This revision simulate the behavior of svn, where we specify the tip to be --rbtools-working-copy so that git client know to remove the revision range out of the git commands.

In addition, to support this flag, --update, --diff-filename, and --guess-fields can't coexists, so the error is thrown when the user specifies those commands along with --uncommitted.

In order to view staged but uncommitted files, we need to add --cached to the git diff command.

Testing Done

Write an extra unit test to check that the git post --uncommitted works.
Verify that it throws error when --uncommitted and other options like --diff-filename,

Issues

Description	From	Last Updated
Col: 28 E222 multiple spaces after operator	reviewbot	Feb. 1, 2015, 3:57 p.m.
Col: 80 E501 line too long (90 > 79 characters)	reviewbot	Feb. 1, 2015, 3:57 p.m.
Col: 43 E203 whitespace before ':'	reviewbot	Feb. 1, 2015, 3:57 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 1, 2015, 3:57 p.m.
Col: 37 E203 whitespace before ':'	reviewbot	Feb. 1, 2015, 3:57 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 1, 2015, 3:57 p.m.
Col: 80 E501 line too long (90 > 79 characters)	reviewbot	Feb. 1, 2015, 7:05 p.m.
Col: 80 E501 line too long (96 > 79 characters)	reviewbot	Feb. 1, 2015, 7:04 p.m.
Col: 77 E225 missing whitespace around operator	reviewbot	Feb. 1, 2015, 7:04 p.m.
Col: 80 E501 line too long (93 > 79 characters)	reviewbot	Feb. 1, 2015, 7:04 p.m.
Col: 60 E251 unexpected spaces around keyword / parameter equals	reviewbot	Feb. 1, 2015, 7:21 p.m.
Col: 62 E251 unexpected spaces around keyword / parameter equals	reviewbot	Feb. 1, 2015, 7:22 p.m.
Col: 80 E501 line too long (94 > 79 characters)	reviewbot	Feb. 1, 2015, 7:22 p.m.
Col: 46 E251 unexpected spaces around keyword / parameter equals	reviewbot	Feb. 1, 2015, 7:22 p.m.
Col: 48 E251 unexpected spaces around keyword / parameter equals	reviewbot	Feb. 1, 2015, 7:22 p.m.
Col: 46 E251 unexpected spaces around keyword / parameter equals	reviewbot	Feb. 1, 2015, 7:27 p.m.
Col: 48 E251 unexpected spaces around keyword / parameter equals	reviewbot	Feb. 1, 2015, 7:27 p.m.
Comments should be full sentences with proper capitalization and punctuation.	david	Feb. 6, 2015, 11:08 a.m.
Given that we now do this in both the n_revs == 0 and n_revs >= 1 cases, perhaps we should …	david	Feb. 6, 2015, 11:08 a.m.
We should probably not show this warning if using this flag.	david	Feb. 6, 2015, 11:09 a.m.
Hmm. So I can see people wanting both the diff in the working copy (unstaged) and the diff in the …	david	Feb. 7, 2015, 9:44 p.m.
Please add this line back in.	david	Feb. 6, 2015, 11:19 a.m.
Please fix this comment.	david	Feb. 6, 2015, 11:19 a.m.
Add a blank line between these two.	david	Feb. 6, 2015, 11:20 a.m.
Leftover debugging print?	david	Feb. 6, 2015, 11:20 a.m.
"GitClient". Also this test name is pretty hard to read (perhaps "Testing GitClient uncommitted diff with staged files"?) Because nose …	david	Feb. 6, 2015, 11:29 a.m.
This error message is kind of hard to read. Maybe we can have separate errors for each of the conflicting …	david	Feb. 6, 2015, 11:29 a.m.
Col: 22 W601 .has_key() is deprecated, use 'in'	reviewbot	Feb. 7, 2015, 9:46 p.m.
We should default this to None rather than the empty string.	david	Feb. 11, 2015, 1:13 p.m.
Reading this through, I think we should keep the handling of the uncommitted flag in this conditional: if uncommitted is …	david	Feb. 11, 2015, 1:13 p.m.
We're still duplicating this code. If you make the above change, you can just have these lines run all the …	david	Feb. 11, 2015, 1:14 p.m.
Typo: "Otherise". Also, add a period at the end.	david	Feb. 11, 2015, 1:14 p.m.
Add a blank line between these two.	david	Feb. 11, 2015, 1:14 p.m.
Add a period at the end.	david	Feb. 11, 2015, 1:15 p.m.
Can we test the parse_revision_spec logic here?	david	Feb. 11, 2015, 1:15 p.m.
Can we test the parse_revision_spec logic here?	david	Feb. 11, 2015, 1:15 p.m.
Remove this line.	david	Feb. 11, 2015, 1:15 p.m.
Can we specify choices in here?	david	Feb. 11, 2015, 1:15 p.m.
Typo: an -> and. You also have an extra ) in the string.	david	Feb. 11, 2015, 1:15 p.m.
Typo: an -> and. You also have an extra ) in the string.	david	Feb. 11, 2015, 1:15 p.m.
Typo: an -> and. You also have an extra ) in the string.	david	Feb. 11, 2015, 1:15 p.m.
This should probably default to None.	david	Feb. 11, 2015, 1:15 p.m.
Col: 55 E231 missing whitespace after ','	reviewbot	Feb. 11, 2015, 1:21 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Feb. 11, 2015, 1:21 p.m.
Col: 55 E231 missing whitespace after ','	reviewbot	Feb. 11, 2015, 1:21 p.m.
Do you mind sorting these in alphabetical order?	david	Feb. 11, 2015, 8:07 p.m.
Can you add blank lines around the blocks? We should also use del rather than .pop(). Please also use exact …	david	Feb. 11, 2015, 8:07 p.m.
This change shouldn't be necessary anymore because the uncommitted handling is a separate case.	david	Feb. 11, 2015, 8:07 p.m.
Remove this blank line?	david	Feb. 11, 2015, 8:07 p.m.
Col: 48 E127 continuation line over-indented for visual indent	reviewbot	Feb. 12, 2015, 12:40 p.m.
For the help, how about "Uploads a diff of the changes in either the working copy or index (Git only)."	david	Feb. 14, 2015, 11:09 a.m.
Why are these not allowed?	david	Feb. 22, 2015, 2:56 p.m.
Single quotes are preferred.	chipx86	Feb. 22, 2015, 6:25 p.m.
Minor nit: This wraps kind of early.	chipx86	Feb. 22, 2015, 6:25 p.m.
Can you flip this? Should be uncommitted == 'workingcopy'	chipx86	Feb. 22, 2015, 6:25 p.m.
Same here.	chipx86	Feb. 22, 2015, 6:32 p.m.
This should probably be given an error message indicating that uncommitted has an incorrect value?	chipx86	Feb. 22, 2015, 6:32 p.m.
There's some important stuff in git_cmd we don't want to lose, like turning off quoted paths nad colors.	chipx86	Feb. 22, 2015, 6:41 p.m.
Best to use tuples here, to avoid the overhead of a list.	chipx86	Feb. 22, 2015, 6:42 p.m.
Best to use .append() here.	chipx86	Feb. 22, 2015, 6:42 p.m.
That line shouldn't be indented.	chipx86	Feb. 22, 2015, 6:43 p.m.
Missing period.	chipx86	Feb. 22, 2015, 6:44 p.m.
Missing period.	chipx86	Feb. 22, 2015, 6:44 p.m.
Summaries are limited to one line. No wrapping.	chipx86	Feb. 22, 2015, 6:45 p.m.
No need for this line. It's implied.	chipx86	Feb. 22, 2015, 6:46 p.m.
Text must be aligned.	chipx86	Feb. 22, 2015, 6:49 p.m.
Maybe just check for uncommitted once, and then do the rest of the conditionals inside that if block.	chipx86	Feb. 22, 2015, 6:54 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Feb. 22, 2015, 11:01 p.m.
'UncommittedIncorrectValueError' imported but unused	reviewbot	Feb. 22, 2015, 11:01 p.m.
undefined name 'OptionsCheckError'	reviewbot	Feb. 22, 2015, 11:01 p.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Feb. 22, 2015, 11:01 p.m.
Col: 32 E128 continuation line under-indented for visual indent	reviewbot	Feb. 22, 2015, 11:02 p.m.
Col: 13 E303 too many blank lines (2)	reviewbot	Feb. 22, 2015, 11:04 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Feb. 22, 2015, 11:04 p.m.
This is short enough that you should be able to put the """s on the same line as the docstring …	david	Feb. 28, 2015, 12:50 p.m.
This comment isn't really useful (the entire function is figuring out tip/base/parent_base), and kind of confusing. I'd just get rid …	david	Feb. 28, 2015, 12:50 p.m.
Add a blank line between here.	david	Feb. 28, 2015, 12:50 p.m.
This comment isn't 100% accurate. Maybe say something about how we add the revision range to the diff command when …	david	Feb. 28, 2015, 12:54 p.m.
The second sentence here isn't good grammar. How about "This is because the update mechanism uses the commit message to …	david	Feb. 28, 2015, 12:56 p.m.
Please figure out why it's not supported and explain that here. You also have a typo ("exlude")	david	Feb. 28, 2015, 1:07 p.m.
This comment is very self-referential to the code, and doesn't really add a lot. How about: If the user passed …	david	March 8, 2015, 10 p.m.
Similar to the comment above, this is kind of confusing. How about: If the user asked for a diff of …	david	March 8, 2015, 10 p.m.
You should probably just use CommandError instead of this.	brennie	March 16, 2015, 10:39 a.m.
Since this doesn't actually interface with Git, I would suggest renaming this to _create_file or _write_file.	brennie	March 9, 2015, 5:23 p.m.
Maybe instead of the --uncommitted-type flag you could have a --staged flag that would work as follows: If --uncommitted flag …	brennie	March 16, 2015, 10:39 a.m.
Why this change?	brennie	March 9, 2015, 5:25 p.m.
Blank line between these.	brennie	March 9, 2015, 5:25 p.m.
Blank line between these.	brennie	March 9, 2015, 5:25 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 10, 2015, 1:34 p.m.
I'm not sure we want to pass a string for this value. It's too easy to typo, and isn't very …	chipx86	March 26, 2015, 9:27 a.m.
I don't see anywhere where this particular exception is caught. The name is a bit awkward. How about InvalidUncommittedModeError, to …	chipx86	April 5, 2015, 8:17 a.m.
This should be: if tip not in (self.REVISION_WORKING_COPY, self.REVISION_STAGED): No need to store as a list and then convert to …	chipx86	March 26, 2015, 9:27 a.m.
this seems redundant self.assertEqual(result['parent_diff'], None) implies self.assertTrue('parent_diff' in result) same for the base_commit_id one	TI tienv	March 30, 2015, 7:06 p.m.
"... in the index. Use ..." Note the period and "the".	chipx86	March 26, 2015, 9:27 a.m.
Comma should go immediately after the word it follows, within the same string. Also, should be "--staged to upload the …	chipx86	March 26, 2015, 9:27 a.m.
"... to the working copy."	chipx86	March 26, 2015, 9:27 a.m.
"and this option is specified, a diff of the staged changes will be uploaded. Otherwise, the changed in the working …	chipx86	March 26, 2015, 9:27 a.m.
This comment just reflects the code exactly, so it doesn't need to be here.	chipx86	March 26, 2015, 9:27 a.m.
No \n.	chipx86	March 26, 2015, 9:27 a.m.
Some grammatical fixes: "If the user has explicitly specified the --uncommitted option, then the --update option cannot be used."	chipx86	March 26, 2015, 9:27 a.m.
No \n.	chipx86	March 26, 2015, 9:27 a.m.
Some grammatical fixes: "If the user has explicitly specified the --uncommitted option, then the --exclude-patterns option cannot be used." The …	chipx86	April 25, 2015, 2:03 p.m.
"exclude-patterns", not "exclude_patterns".	chipx86	March 26, 2015, 9:29 a.m.
No \n.	chipx86	March 26, 2015, 9:29 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	March 30, 2015, 7:07 p.m.
Col: 54 E231 missing whitespace after ','	reviewbot	March 30, 2015, 7:07 p.m.
This comment needs to be updated to reflect the new flags.	brennie	April 25, 2015, 1:47 p.m.
Can we rename file to filename in all these instances? file is overriding a Python builtin.	brennie	April 25, 2015, 1:48 p.m.
Should --staged be shorthand for --staged --uncommitted ?	brennie	April 25, 2015, 2:03 p.m.
Add another blank line here.	david	May 21, 2015, 6:21 p.m.
This is useless (we just set result to be an empty dict above -- so there will never be a …	david	May 21, 2015, 6:21 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/clients/git.py

rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 28
 E222 multiple spaces after operator
```
rbtools/clients/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (90 > 79 characters)
```
rbtools/clients/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 43
 E203 whitespace before ':'
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 37
 E203 whitespace before ':'
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (90 > 79 characters)
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (96 > 79 characters)
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 77
 E225 missing whitespace around operator
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (93 > 79 characters)
```

Groups:: students

Commit:

8a23e1dce24b481c4ae1de5bf9f317071c2fe2a6

cd084d7d690d91dc596191387ce2adfe6fd879c8

Diff:

Revision 2 (+72 -13)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py

rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 60
 E251 unexpected spaces around keyword / parameter equals
```
rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 62
 E251 unexpected spaces around keyword / parameter equals
```
rbtools/commands/post.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (94 > 79 characters)
```
rbtools/utils/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 46
 E251 unexpected spaces around keyword / parameter equals
```
rbtools/utils/review_request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 48
 E251 unexpected spaces around keyword / parameter equals
```

Commit:

cd084d7d690d91dc596191387ce2adfe6fd879c8

9b5cc563f6ec0fb497e28d83bb1aab6b8868fc99

Diff:

Revision 3 (+73 -13)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py

rbtools/utils/review_request.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 46
 E251 unexpected spaces around keyword / parameter equals
```
rbtools/utils/review_request.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 48
 E251 unexpected spaces around keyword / parameter equals
```

Commit:

9b5cc563f6ec0fb497e28d83bb1aab6b8868fc99

ba6036b5ca5771b5f646b615aec80166d0bda79a

Diff:

Revision 4 (+73 -13)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py

```
This looks like a pretty good start!
```

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

The issue has been resolved. Show all issues

Comments should be full sentences with proper capitalization and punctuation.

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

The issue has been resolved. Show all issues

Given that we now do this in both the n_revs == 0 and n_revs >= 1 cases, perhaps we should move it out to happen just before we return result?

rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
We should probably not show this warning if using this flag.
```

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

The issue has been resolved. Show all issues

Hmm. So I can see people wanting both the diff in the working copy (unstaged) and the diff in the index (staged). Perhaps we should make --uncommitted be able to take an optional argument?

Please also make sure that this comment is a full sentence with proper capitalization and punctuation.

viettran1989 Feb. 6, 2015, 11:57 a.m.

Can you clarify the format of the optinal argument? Is it going to be a list of unstaged files or a boolean to show all unstaged files?

I think the former is preferable because a project might have a lot of artifact that hasn't been added to .gitignore. Some of these might not be of interest to the user, but I want to make sure I understand the requirement.

david Feb. 6, 2015, 4:56 p.m.

Well, I'm thinking maybe it can be an enumerated value rather than boolean. I can see perhaps wanting --uncommitted=workingcopy and --uncommitted=staged (and maybe have it so --uncommitted without a value defaults to workingcopy and the lack of --uncommitted makes it use the current behavior).

viettran1989 Feb. 7, 2015, 9:44 p.m.

I'm not sure there is a clean way to default to workingcopy, so I let the user specify this option either with workingcopy or staged. I add the appropriate test for it.

rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Please add this line back in.
```
rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Please fix this comment.
```
rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Add a blank line between these two.
```
rbtools/clients/git.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Leftover debugging print?
```

rbtools/clients/tests.py (Diff revision 4)

The issue has been resolved. Show all issues

"GitClient". Also this test name is pretty hard to read (perhaps "Testing GitClient uncommitted diff with staged files"?) Because nose will append an ellipsis, we don't put periods at the end of test case docstrings.

rbtools/commands/post.py (Diff revision 4)

The issue has been resolved. Show all issues

This error message is kind of hard to read. Maybe we can have separate errors for each of the conflicting options?

Commit:

ba6036b5ca5771b5f646b615aec80166d0bda79a

eeb504cae5ea00dc24c10449de5f32dda013d6f2

Diff:

Revision 5 (+84 -13)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py

Commit:

eeb504cae5ea00dc24c10449de5f32dda013d6f2

f1b575d90906e515ac35624c7243bc53bf41ea86

Diff:

Revision 6 (+117 -14)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py

rbtools/clients/git.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 22
 W601 .has_key() is deprecated, use 'in'
```

Commit:

f1b575d90906e515ac35624c7243bc53bf41ea86

0ddb7414d3ceb1daaace06e0ec6fe1238bef894b

Diff:

Revision 7 (+117 -14)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py

Commit:

0ddb7414d3ceb1daaace06e0ec6fe1238bef894b

1cf5a751b7c96556647b99fbcff19faf38e6fe22

Diff:

Revision 8 (+117 -12)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py

rbtools/clients/git.py (Diff revision 8)
The issue has been resolved. Show all issues
```
We should default this to None rather than the empty string.
```

rbtools/clients/git.py (Diff revision 8)

The issue has been resolved. Show all issues

Reading this through, I think we should keep the handling of the uncommitted flag in this conditional:

if uncommitted is not None:
    del result['commit_id']
    result['base'] = self._rev_parse(self.get_head_ref())[0]

    if uncommitted == 'workingcopy':
        result['tip'] = self.REVISION_WORKING_COPY
    elif uncommitted == 'staged':
        result['tip'] = self.REVISION_STAGED
    else:
        assertion?
elif n_revs == 0:
    ...

rbtools/clients/git.py (Diff revision 8)

The issue has been resolved. Show all issues

We're still duplicating this code. If you make the above change, you can just have these lines run all the time before return result

rbtools/clients/git.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Typo: "Otherise". Also, add a period at the end.
```
rbtools/clients/git.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Add a blank line between these two.
```
rbtools/clients/git.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Add a period at the end.
```
rbtools/clients/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Can we test the parse_revision_spec logic here?
```
rbtools/clients/tests.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Can we test the parse_revision_spec logic here?
```
rbtools/commands/post.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Remove this line.
```
rbtools/commands/post.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Can we specify choices in here?
```
rbtools/commands/post.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Typo: an -> and. You also have an extra ) in the string.
```
rbtools/commands/post.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Typo: an -> and. You also have an extra ) in the string.
```
rbtools/commands/post.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Typo: an -> and. You also have an extra ) in the string.
```
rbtools/utils/review_request.py (Diff revision 8)
The issue has been resolved. Show all issues
```
This should probably default to None.
```

Commit:

1cf5a751b7c96556647b99fbcff19faf38e6fe22

d61a19ee364c9f57022d807727996fd48e7c09b6

Diff:

Revision 9 (+126 -18)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py

rbtools/clients/tests.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 55
 E231 missing whitespace after ','
```
rbtools/clients/tests.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
rbtools/clients/tests.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 55
 E231 missing whitespace after ','
```

Commit:

d61a19ee364c9f57022d807727996fd48e7c09b6

0d834b032b17a006130018518b0366a00ad7e57e

Diff:

Revision 10 (+127 -18)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Groups:: students

rbtools/clients/git.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Do you mind sorting these in alphabetical order?
```

rbtools/clients/git.py (Diff revision 10)

The issue has been resolved. Show all issues

Can you add blank lines around the blocks? We should also use del rather than .pop().
Please also use exact comparisons rather than 'in' (which does a substring match, which is slower and imprecise).

if 'commit_id' in result:
    del result['commit_id']

result['base'] = self._rev_parse(self.get_head_red())[0]

if uncommitted == 'workingcopy':
    ...

rbtools/clients/git.py (Diff revision 10)

The issue has been resolved. Show all issues

This change shouldn't be necessary anymore because the uncommitted handling is a separate case.

rbtools/clients/git.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Remove this blank line?
```

Commit:

0d834b032b17a006130018518b0366a00ad7e57e

68a9e62698d0fd0b412d6bea6d4a2c405d4ac79a

Diff:

Revision 11 (+128 -19)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py

rbtools/commands/post.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Col: 48
 E127 continuation line over-indented for visual indent
```

Commit:

68a9e62698d0fd0b412d6bea6d4a2c405d4ac79a

a4f3b2ccd8d0d8ed8e4c89be1e14e747e5476ace

Diff:

Revision 12 (+128 -19)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py

rbtools/commands/post.py (Diff revision 12)

The issue has been resolved. Show all issues

For the help, how about "Uploads a diff of the changes in either the working copy or index (Git only)."

rbtools/commands/post.py (Diff revision 12)

The issue has been resolved. Show all issues

Why are these not allowed?

viettran1989 Feb. 12, 2015, 4:58 p.m.

I think we discussed this the last time about update, but I don't remember exactly the reason. For the exclude_pattern, it's currently not supported because there is not a clean way to support it based on my discussion with Barret

david Feb. 16, 2015, 2:52 p.m.

Update is probably because it guesses based on the commit message (and of course there isn't one). We should probably make it so that update can work without a commit message and just try guessing all open review requests in turn.

That said, the comments next to these shouldn't say what the code does (the code is very simple--I can read it easily), but rather, answer this question (why). That way if circumstances change later, we will know that we can change the restrictions here.

viettran1989 Feb. 18, 2015, 9:48 p.m.

So for the guesses, do you want me to support that in this feature or I should open it as a seperate issue?

david Feb. 19, 2015, 7:32 p.m.

I think we should get this change ready as-is, and you can work on supporting -u in a separate review request.

Commit:

a4f3b2ccd8d0d8ed8e4c89be1e14e747e5476ace

23069b4af8a86bb61ab2895321ae9c1ca028acd5

Diff:

Revision 13 (+130 -20)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py

Commit:

23069b4af8a86bb61ab2895321ae9c1ca028acd5

496774e25171537b1ee90b2cf9cae2c3c596789e

Diff:

Revision 14 (+131 -20)

Show changes

	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/utils/review_request.py
    rbtools/clients/git.py

So my main concern is that passing --uncommitted=staged or --uncommitted=workingcopy is a lot to type every time, and perhaps is confusing to people without reading docs. How about instead having --uncommitted as a boolean flag, and then --uncommitted-type=[staged|workingcopy], with the default of "staged"?

The pro being that, in the common case of posting changes scheduled for commit but not yet committed, it's quicker to type and post. Also, --uncommitted-type could be a configurable option, so that the user can specify which mode they prefer to work with in .reviewboardrc without triggering uncommitted reviews for every change.

Downside being that, in the uncommon case, it's more to type, but I think most people just want to post changes scheduled for commit without actually committing. However, with the option, that shouldn't be as big a deal.

rbtools/clients/git.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Single quotes are preferred.
```
rbtools/clients/git.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Minor nit: This wraps kind of early.
```
rbtools/clients/git.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Can you flip this? Should be uncommitted == 'workingcopy'
```
rbtools/clients/git.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Same here.
```

rbtools/clients/git.py (Diff revision 14)

The issue has been resolved. Show all issues

This should probably be given an error message indicating that uncommitted has an incorrect value?

rbtools/clients/git.py (Diff revision 14)

The issue has been resolved. Show all issues

There's some important stuff in git_cmd we don't want to lose, like turning off quoted paths nad colors.

rbtools/clients/git.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Best to use tuples here, to avoid the overhead of a list.
```
rbtools/clients/git.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Best to use .append() here.
```
rbtools/clients/git.py (Diff revision 14)
The issue has been resolved. Show all issues
```
That line shouldn't be indented.
```
rbtools/clients/tests.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Missing period.
```
rbtools/clients/tests.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Missing period.
```
rbtools/clients/tests.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Summaries are limited to one line. No wrapping.
```
rbtools/commands/post.py (Diff revision 14)
The issue has been resolved. Show all issues
```
No need for this line. It's implied.
```
rbtools/commands/post.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Text must be aligned.
```

rbtools/commands/post.py (Diff revision 14)

The issue has been resolved. Show all issues

Maybe just check for uncommitted once, and then do the rest of the conditionals inside that if block.

Commit:

496774e25171537b1ee90b2cf9cae2c3c596789e

d0a133342ae69ed4c304f29aeca0f27f3702e34e

Diff:

Revision 15 (+144 -18)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py

rbtools/clients/errors.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
rbtools/clients/git.py (Diff revision 15)
The issue has been resolved. Show all issues
```
 'UncommittedIncorrectValueError' imported but unused
```
rbtools/clients/git.py (Diff revision 15)
The issue has been resolved. Show all issues
```
 undefined name 'OptionsCheckError'
```
rbtools/clients/git.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (84 > 79 characters)
```
rbtools/commands/post.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 32
 E128 continuation line under-indented for visual indent
```
rbtools/commands/post.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 13
 E303 too many blank lines (2)
```
rbtools/commands/post.py (Diff revision 15)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```

Commit:

d0a133342ae69ed4c304f29aeca0f27f3702e34e

a8afd717ce5d166cbfef0e1ec3c733f1a0295152

Diff:

Revision 16 (+148 -19)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py

rbtools/clients/errors.py (Diff revision 16)

The issue has been resolved. Show all issues

This is short enough that you should be able to put the """s on the same line as the docstring itself. There should also be a period at the end.

rbtools/clients/git.py (Diff revision 16)

The issue has been resolved. Show all issues

This comment isn't really useful (the entire function is figuring out tip/base/parent_base), and kind of confusing. I'd just get rid of it.

rbtools/clients/git.py (Diff revision 16)
The issue has been resolved. Show all issues
```
Add a blank line between here.
```

rbtools/clients/git.py (Diff revision 16)

The issue has been resolved. Show all issues

This comment isn't 100% accurate. Maybe say something about how we add the revision range to the diff command when posting committed changes?

rbtools/commands/post.py (Diff revision 16)

The issue has been resolved. Show all issues

The second sentence here isn't good grammar. How about "This is because the update mechanism uses the commit message to find the review request, and uncommitted changes don't have a commit message yet."

rbtools/commands/post.py (Diff revision 16)

The issue has been resolved. Show all issues

Please figure out why it's not supported and explain that here. You also have a typo ("exlude")

Commit:

a8afd717ce5d166cbfef0e1ec3c733f1a0295152

9fa7f027d05bab2b454a1dc4d45b94b3066d0f62

Diff:

Revision 17 (+149 -19)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py

Your description should be updated to talk about --uncommitted-type. You also have a typo ("Mercurila").

rbtools/clients/git.py (Diff revision 17)

The issue has been resolved. Show all issues

This comment is very self-referential to the code, and doesn't really add a lot. How about:

If the user passed --uncommitted-type=staged, add --cached to the 'git diff' command.

rbtools/clients/git.py (Diff revision 17)

The issue has been resolved. Show all issues

Similar to the comment above, this is kind of confusing. How about:

If the user asked for a diff of the working copy, we do not pass any revisions to the 'git diff' command. Otherwise, add the revision range.

Commit:

9fa7f027d05bab2b454a1dc4d45b94b3066d0f62

0a434c24773633ced0b02d85d04d3a9ccc5f181f

Diff:

Revision 18 (+150 -19)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py



Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py

rbtools/clients/errors.py (Diff revision 18)

The issue has been resolved. Show all issues

You should probably just use CommandError instead of this.

viettran1989 March 9, 2015, 7:59 p.m.

Is it too generic though? The OptionCheckError in the same file seems to be a CommandError but it has its own definition.

brennie March 9, 2015, 8:38 p.m.

The OptionCheckError would be fine, too.

rbtools/clients/tests.py (Diff revision 18)

The issue has been resolved. Show all issues

Since this doesn't actually interface with Git, I would suggest renaming this to _create_file or _write_file.

rbtools/commands/post.py (Diff revision 18)

The issue has been resolved. Show all issues

Maybe instead of the --uncommitted-type flag you could have a --staged flag that would work as follows:

If --uncommitted flag is specified and --staged isn't specified it will use the working copy changes; otherwise if --staged is specified, it will use the staged changes.

This seems like a clearer way to go about it (and will be easier to describe).

david March 9, 2015, 4:59 p.m.

Having these two flags was Christian's suggestion. How about you and he hash it out, so we don't have Tran thrash around too much between different options?

viettran1989 March 9, 2015, 7:59 p.m.

I just followed what Christian suggested in previous post which I quoted "So my main concern is that passing --uncommitted=staged or --uncommitted=workingcopy is a lot to type every time, and perhaps is confusing to people without reading docs. How about instead having --uncommitted as a boolean flag, and then --uncommitted-type=[staged|workingcopy], with the default of "staged"?" . I get your point, but I feel like I keep going back and forth fixing cosmetic issues everytime someone looks at it which not really help me learn. Is there a guideline or standard where I could refer to when applying my changes?

brennie March 9, 2015, 8:38 p.m.

This is just a suggesstion, so feel free to drop it if you prefer Christian's suggestion. Either way is fine for me.

chipx86 March 9, 2015, 10:47 p.m.

I'm happy enough with Barret's suggestion. It's cleaner.

rbtools/commands/post.py (Diff revision 18)
The issue has been resolved. Show all issues
```
Why this change?
```
rbtools/commands/post.py (Diff revision 18)
The issue has been resolved. Show all issues
```
Blank line between these.
```
rbtools/commands/post.py (Diff revision 18)
The issue has been resolved. Show all issues
```
Blank line between these.
```

Commit:

0a434c24773633ced0b02d85d04d3a9ccc5f181f

6162ed298eac495f0d911aef30d0af8c732a0a00

Diff:

Revision 19 (+151 -18)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py

Description:: Problem:
While the common workflow is to post committed changes, some people have workflows where they want to post changes that are uncommitted, and have to resort to generating a diff manually and posting it using rbt post --diff-filename=.

This work would add an --uncommitted flag to rbt post that would generate diffs for uncommitted changes instead.

Approach:
Adding can_diff_committed capability to all clients, and let git set that to True to support it.
This revision simulate the behavior of svn, where we specify the tip to be --rbtools-working-copy so that git client know to remove the revision range out of the git commands.
In addition, to support this flag, --update, --diff-filename, and --guess-fields can't coexists, so the error is thrown when the user specifies those commands along with --uncommitted.
In order to view staged but uncommitted files, we need to add --cached to the git diff command.
-
-
To Do:
- - Support Merculia
- - Support exclude pattern in which the user can exclude the staged files to be posted. Currently, it will throw un error if --excluded-pattern is specified along with --uncommitted.

Commit:

6162ed298eac495f0d911aef30d0af8c732a0a00

7360845ec538d50588596b6d2a4a374f491fc96c

Diff:

Revision 20 (+151 -18)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py

rbtools/commands/post.py (Diff revision 20)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Commit:

7360845ec538d50588596b6d2a4a374f491fc96c

5cc36ea187f9acf4e66ad6d1ffb25933a7260372

Diff:

Revision 21 (+151 -18)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py

rbtools/clients/git.py (Diff revision 21)

The issue has been resolved. Show all issues

I'm not sure we want to pass a string for this value. It's too easy to typo, and isn't very obvious to the caller.
What I'd suggest instead is a class that works as an enum for storing the possible values. For instance:

class UncommittedMode(object):
    WORKING_COPY = 1
    STAGED = 2


Then it's easy to check.

rbtools/clients/git.py (Diff revision 21)

The issue has been dropped. Show all issues

I don't see anywhere where this particular exception is caught.
The name is a bit awkward. How about InvalidUncommittedModeError, to match the suggested class name?

viettran1989 March 26, 2015, 9:29 a.m.

"I don't see anywhere where this particular exception is caught" > Can you elaborate on this? Are you talking about trying to catch it somewhere so that the user can continue?

tienv March 27, 2015, 5:42 p.m.

I think what he means is, in commands/diff.py

    try:
        revisions = tool.parse_revision_spec(args)
        extra_args = None
    except InvalidRevisionSpecError:

there should be another 'except' to catch the UncommittedIncorrectValueError

viettran1989 March 30, 2015, 7:29 p.m.

Well, that makes sense, but what protocol to do here except raise the exception again? I don't see any particular way to handle this exception since the revisions variables is not complete.

rbtools/clients/git.py (Diff revision 21)

The issue has been resolved. Show all issues

This should be:

if tip not in (self.REVISION_WORKING_COPY, self.REVISION_STAGED):

No need to store as a list and then convert to a tuple.

rbtools/commands/post.py (Diff revision 21)
The issue has been resolved. Show all issues
```
"... in the index. Use ..."
Note the period and "the".
```

rbtools/commands/post.py (Diff revision 21)

The issue has been resolved. Show all issues

Comma should go immediately after the word it follows, within the same string.

Also, should be "--staged to upload the staged changes."

rbtools/commands/post.py (Diff revision 21)
The issue has been resolved. Show all issues
```
"... to the working copy."
```

rbtools/commands/post.py (Diff revision 21)

The issue has been resolved. Show all issues

"and this option is specified, a diff of the staged changes will be uploaded. Otherwise, the changed in the working copy are uploaded."

rbtools/commands/post.py (Diff revision 21)

The issue has been resolved. Show all issues

This comment just reflects the code exactly, so it doesn't need to be here.

rbtools/commands/post.py (Diff revision 21)
The issue has been resolved. Show all issues
```
No \n.
```

rbtools/commands/post.py (Diff revision 21)

The issue has been resolved. Show all issues

Some grammatical fixes:

"If the user has explicitly specified the --uncommitted option, then the --update option cannot be used."

rbtools/commands/post.py (Diff revision 21)
The issue has been resolved. Show all issues
```
No \n.
```

rbtools/commands/post.py (Diff revision 21)

The issue has been dropped. Show all issues

Some grammatical fixes:

"If the user has explicitly specified the --uncommitted option, then the --exclude-patterns option cannot be used."

The note about diff-tree is a Git-specific implementation detail, so we shouldn't mention it here.

viettran1989 March 26, 2015, 9:29 a.m.

Previous mentors mentioned that I need to indicate the reason for not supporting this and the main reason is to find an alternative to diff-tree command for git. If we remove it, how should I rephrase this to satisfy the requirement of the comment?

rbtools/commands/post.py (Diff revision 21)
The issue has been resolved. Show all issues
```
"exclude-patterns", not "exclude_patterns".
```
rbtools/commands/post.py (Diff revision 21)
The issue has been resolved. Show all issues
```
No \n.
```

rbtools/clients/tests.py (Diff revision 21)

The issue has been dropped. Show all issues

this seems redundant
self.assertEqual(result['parent_diff'], None)
implies
self.assertTrue('parent_diff' in result)
same for the base_commit_id one

viettran1989 April 2, 2015, 12:40 p.m.

I don't think this comment is valid though, if you try to do "self.assertEqual(result['parent_diff'], None)" without making sure that result actually has 'parent_diff', you will have ValueError exception because you try to access an item that not in the dictionary yet.

tienv April 2, 2015, 1:48 p.m.

yeah, I was thinking that if the key wasn't in the dict, the test will fail anyway. but an exception is probably not as good as an explicit assertion

Commit:

5cc36ea187f9acf4e66ad6d1ffb25933a7260372

38b86084047b624629e126320c150ed1f733e1de

Diff:

Revision 22 (+152 -18)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py

rbtools/clients/git.py (Diff revision 22)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
rbtools/clients/git.py (Diff revision 22)
The issue has been resolved. Show all issues
```
Col: 54
 E231 missing whitespace after ','
```

Commit:

38b86084047b624629e126320c150ed1f733e1de

3860f9757e56c95feeceba83acff9f004c6f950c

Diff:

Revision 23 (+153 -18)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py

rbtools/clients/git.py (Diff revision 23)
The issue has been resolved. Show all issues
```
This comment needs to be updated to reflect the new flags.
```

rbtools/clients/tests.py (Diff revision 23)

The issue has been resolved. Show all issues

Can we rename file to filename in all these instances? file is overriding a Python builtin.

rbtools/commands/post.py (Diff revision 23)

The issue has been dropped. Show all issues

Should --staged be shorthand for --staged --uncommitted ?

viettran1989 April 25, 2015, 1:54 p.m.

In the previous comment, you mentioned that I should use --uncommitted to indicate changes in working copy and --uncommitted --staged to indicate staged change. For me, it makes senses that way because it's part of supporting uncommitted flag.

Commit:

3860f9757e56c95feeceba83acff9f004c6f950c

756835ef2152429dc74743f955cb274bf64e9b1a

Diff:

Revision 24 (+154 -20)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/git.py
	rbtools/clients/tests.py
	rbtools/commands/post.py
	rbtools/utils/review_request.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/tests.py
    rbtools/commands/post.py
    rbtools/utils/review_request.py
    rbtools/clients/errors.py
    rbtools/clients/git.py

Fix it!

rbtools/clients/git.py (Diff revision 24)
An issue was opened. Show all issues
```
Add another blank line here.
```

rbtools/clients/git.py (Diff revision 24)

An issue was opened. Show all issues

This is useless (we just set result to be an empty dict above -- so there will never be a commit_id in it).

Status:: Discarded

		Problem:
		While the common workflow is to post committed changes, some people have workflows where they want to post changes that are uncommitted, and have to resort to generating a diff manually and posting it using `rbt post --diff-filename=`.

		This work would add an `--uncommitted` flag to `rbt post` that would generate diffs for uncommitted changes instead.

		Approach:
		Adding can_diff_committed capability to all clients, and let git set that to True to support it.
		This revision simulate the behavior of svn, where we specify the tip to be --rbtools-working-copy so that git client know to remove the revision range out of the git commands.
		In addition, to support this flag, --update, --diff-filename, and --guess-fields can't coexists, so the error is thrown when the user specifies those commands along with --uncommitted.
		In order to view staged but uncommitted files, we need to add --cached to the git diff command.
-
-		To Do:
-		- Support Merculia
-		- Support exclude pattern in which the user can exclude the staged files to be posted. Currently, it will throw un error if --excluded-pattern is specified along with --uncommitted.