When we submit diffs via the web interface (Differential --> Create Diff --> Raw Diff field), we don't get context. Typically folks here are using TortoiseSVN --> "Create Patch" to generate the diff. I realize these diffs have only relative paths in them, however TortoiseSVN itself is smart enough to figure out (when you use "Apply Patch") if there is some path in the source tree which might possibly match the diff (it even prompts you to confirm). A similar feature for Phabricator would be awesome, since not having context makes review using differential kind of sub-par.
Description
Description
| Status | Assigned | Task | ||
|---|---|---|---|---|
| Wontfix | epriestley | T5274 Herald Rules - Trigger when Diff content contains | ||
| Open | None | T5029 Fill in missing context for low-context diffs against tracked repositories |
Event Timeline
Comment Actions
If you generate a diff with full context, we'll handle it properly -- I'm not sure if there's a way to do this in TortiseSVN or not, but from the command line it's something like svn diff --diff-cmd diff -x -U99999.
It would ideally be nice to fill context in from the repository, but it's complex. For example, we may not have any idea what branch you're on, and potentially need to search thousands of branches -- when applying a patch, TortiseSVN only needs to look through the working copy, which usually does not have every branch in it. We could add more configuration to Diffusion, like "diffs with no context probably came from directories x/ or y/", but this is a lot of work.
If you use arc, we'll also automatically generate full-context diffs for you. However, we don't currently have GUI bindings for arc.
Comment Actions
Thanks for wishlisting this, it would be great for shops like us that are Windows-based (and where it's hard enough to get buy-in to using a code review tool, let alone convince everyone to use arc instead of TortoiseSVN).
I'm probably not understanding/oversimplifying, but the patch that TortoiseSVN generates has lines like:
Index: CPCAPI2/impl/call/ConversationManagerInterface.cpp =================================================================== --- CPCAPI2/impl/call/ConversationManagerInterface.cpp (revision 108901) +++ CPCAPI2/impl/call/ConversationManagerInterface.cpp (working copy)
... so wouldn't the revision itself tell you what branch?
Comment Actions
The same problem exists in git and I have the same issue over acceptability of my users who happen to be familiar with ReviewBoard.
With git you have the commit hash to save you having to look for the repo/branch. Use of --full-index (to give the full hash) would give it to you completely unambiguously across any number of repos being managed by phabricator.
The Web UI Submission needs to be extended to accept patches created with "git format-patch" too as well as diffs. Currently it fails with an ugly exception but this would also provide the same contextual hook needed to allow a full display of the surrounding code once submitted.
Comment Actions
Another +1 from me. Would it make sense to just ask the submitter for a revision to use as the base?
Comment Actions
+1 from me.
Won't patches with full context create issues in SVN. For example I've changed single file in 2 patches. Then after 1st patch is applied the 2nd patch can't be applied. Or the patch command can take care of it automatically?
This comment was removed by tao_qiping.
Comment Actions
In the Google codereview app when it used to be possible to submit a raw diff for submitting code for review, it was quite simple to do that. The main issue is not the revision, the main issue is the path. So simply add a field in the submission form to provide the path to the root of the patch.
In other words, if you have a repository and you do a diff in A/B/C, meaning that in the diff you won't see A/B/C, just have a field in the form to specify A/B/C. Then there is no ambiguity. If the field is empty, then you simply assume it is the root.
Comment Actions
We've had some people in KDE kick up some fuss about this.
Are there any plans to look into this / how complicated would it be to implement? (Assume interest in just Git for now, and that diffs would have the commit hashes in them)
Comment Actions
Chances are we're going to get complaints that go either way. Myself, i'd tend to prefer T5000.
Comment Actions
I'm just curious how people are generating diffs, just command line then cut and paste? I would presume those users would be fine with pushing a branch up for review. Are there other methods we're not considering, like IDEs?
Not sure where T5000 is on the current roadmap, but I think it's part of the audit/diffusion/arc overhaul.
Comment Actions
I would imagine they're using something along those lines yes. I think we'll probably keep a bunch of people happy if pushing Git refs up were supported.
People could theoretically be using IDE's such as KDevelop or QtCreator, but I don't think either of those offer the ability to view a diff for uploading to a code review system (although they do have specialised functionality to upload patches to some code review systems I believe).
Comment Actions
People could theoretically be using IDE's such as KDevelop or QtCreator, but I don't think either of those offer the ability to view a diff for uploading to a code review system (although they do have specialised functionality to upload patches to some code review systems I believe).
KDevelop has long had the possibility to view local differences obtained from the VCS in use and then upload those to ReviewBoard. Those differences are generated using git diff or whatever the equivalent for the current VCS system, and it probably shouldn't be an issue to add --full-index to the git diff arguments. That means the patch should contain all required information to show all available context once you know the repository and branch it applies to. One already has to tell Phab what repository a diff applies to when creating a revision, would it be very complex to add a branch selection drop-down (which defaults to the master branch)? Even if it is it might be useful to be able to register the target branch as a kind of simple tag instead of providing that information in the summary text.
I see a suggestion to generate the diff with -U<hugeVal> . Does that mean that in terms of context revisions created from such diffs do not lack anything compared to revisions created from locally committed diffs?