Summary:

rbtools support diff filename instead of calling scm to generate diff

Review Request #1197 — Created Nov. 5, 2009 and submitted

Information
Owner:	clach04
Repository:	RBTools
Branch:
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:	chipx86

Description

Added new flag "--diff-filename" that specifies the name of the file where diffs should be read from instead of using SCM client to generate diff.

Due to the way some (git) SCMClient subclasses change the current directory the new code has to take a backup of the starting directory if diff filename is not fully specified/qualified.

NOTE at this time only reading from a file is supported.

Testing Done

git diff --full-index HEAD >mydiff
./postreview.py --server=http://reviews.reviewboard.org/ --diff-filename mydiff -r 1197

I.e. this review was posted with the new code

We could potentially change the GitClient to not chdir. I'm not sure what the comment about not doing it from the top level directory is referring to anyway. With pure git there's no reason we can't diff from a subdirectory. Perhaps there is an implication for git-svn though.

clach04 Nov. 6, 2009, 5:57 a.m.

I'm a complete newbie with git so I can't comment either way on potential changes to GitClient. If Git Client does change I think it should be done in a separate change, this change is only for specifying a diff filename.

Is there any chance this can be accepted as-is (without "-" support for the filename)?

chipx86 Nov. 10, 2009, 6:51 p.m.

I think that's fine. I still have to review the code (been on vacation, will get to it soon), but changing it to not chdir and adding "-" support can be done separately.

rbtools/postreview.py (Diff revision 1)

I'd prefer a simpler comment: "upload an existing diff file, instead of generating a new diff"

rbtools/postreview.py (Diff revision 1)

Combine these.

Blank line after these and before the if statement.

rbtools/postreview.py (Diff revision 1)

This can be simplified a little, and also needs to close the file handle. Should look more like:


if options.diff_filename == '-':
    raise NotImplemented(...)

parent_diff = None

try:
    fp = open(os.path.join(origcwd, options.diff_filename), 'r')
    diff = fp.read()
    fp.close()
except IOError, e:
    die("Unable to open diff filename: %s" % e)

Change Summary:

Style change to file IO along with help text update based on Christian's feedback.

Testing Done:

		git diff --full-index HEAD >mydiff
~		./postreview.py --server=http://reviews.reviewboard.org/ --diff-filename mydiff
	~	./postreview.py --server=http://reviews.reviewboard.org/ --diff-filename mydiff -r 1197
	+
	+	I.e. this review was posted with the new code

Diff:

Revision 2 (+19 -1)

Show changes

rbtools/postreview.py

rbtools/postreview.py (Diff revision 2)

You could combine these 2 lines into
elif options.diff_filename:
to be consistent with the previous elif.

But then, the else: on line 2663 needs to be properly indented.

clach04 Jan. 11, 2010, 7:09 a.m.

Agreed merging line 2650 and 2651 into an elif and then de-indenting what is in the proposed diff is line 2663 (and the line below) would be cleaner.

I'm probably not going to post another review for this though as it looks like interest for this may have dropped off. I've updated my local copy though! Thanks for the feedback.

Chris

chipx86 Jan. 11, 2010, 5:41 p.m.

I think it's still a useful change to get in, especially for automation.

Change Summary:

Based on feedback, merged if/else (reduces line count and simplifies). Also added commented out code for "-" support. Again this change was posted using the new feature:

    git diff --full-index HEAD >mynewdiff
    ./postreview.py --server=http://reviews.reviewboard.org/ --diff-filename  mynewdiff -r 1197

Diff:

Revision 3 (+17)

Show changes

rbtools/postreview.py

Change Summary:

Essentially the same code as the last posted diff, but this is after a "git pull/merge" to latest code as of 2009-01-12. I.e. fully merged code with headrevs.

Diff:

Revision 4 (+17)

Show changes

rbtools/postreview.py

Looks great.  I implemented locally a similar change, so it'll be useful if yours goes into the tree.  
Many thanks.

Ship it!

Thanks! Committed as r368f1f4.

Also made a commit with support for --diff-filename=-, which prevents usage of raw_input and stuff.