Is it possible/easy to audit whole files?


#1

I really want a workflow not supported by most review tools: the ability to review whole files or sets of files, rather than commits. When a file has a lot of history behind it, it is often important to be able to review and comment on details about the current state of the file, without digging out the commits that may have last touched individual lines. I can’t tell from the documentation whether Phabricator’s Audit tool (or any other Phabricator tool) supports that workflow or not. Can anyone shed light?

Thanks,
Dave


#2

Not current way to do that. See https://secure.phabricator.com/T4348.


#3

Hm, thanks. I would love to be able to address the potential usefulness concern in the ticket, but unfortunately I don’t seem to be able to get an account set up, so I’ll write it here:

I don’t know what Phabricator’s mission is, but if it was my company, part of it would be to enable and promote good coding practices. Most projects start out without code review processes in place, on the theory that it would slow down development too much. The problem is, they stay that way. Not being able to efficiently get an existing codebase into “completely reviewed” state is a huge barrier to entry for such projects, which as I claim is most of them. Further, I can guarantee that I would have taken several large projects through the process to get into that state if the tool was available. The fact that some “large ideas that span commits and files” could be overlooked in such a process doesn’t in any way negate the value of doing it. It’s much more likely that they will be caught if it is done than if it is not done.

Finally, this is not only of value to whole codebases. It is also very common that individual source files, documents, and proposals under development undergo many revisions before someone with useful feedback gets to review them. What’s important at that point is not how the state was arrived at, but that it makes sense as a whole. It’s also very common that I’m looking at the latest revision of a source file and I notice one or more things that look wrong that I want to raise as issues. At that point, I want to be able to launch an audit of that file (and maybe a few other related ones). Having to dig through revision history just to figure out where the relevant changes are is prohibitive, and since the changes might be disjoint there’s often no good way to create such a review.

I hope I’ve made the case for whole-file review in a useful (and convincing!) way. Thanks for listening.


#4

Actually on re-reading the ticket, I’m not even sure I’m making that same request. It’s much more important to be able to do this with a file or subset of files than to try to ingest the whole codebase at once. The only reasonable way to get to “completely audited” is to bite off logical chunks, and AFAICT that will always be done on a fileset or directory granularity.


#5

I think your use-case falls within T4348. Anyway, that ticket is under “will happen one day”, except that it’s low priority, and the upstream bandwidth is very limited, so it can stay there for a (many) years.

I’m afraid there is no good answer for this, other than “if you really want it, try building it yourself”.

(There’s a way to ask for an invite to the upstream phabricator, but I don’t know what it is; “Support” and “community” flows are being shuffled right now).


#6

I see. In that case would you mind adding a link in the ticket to Is it possible/easy to audit whole files??

When the ticket is considered again, I’d hate for the last word to be, “I’m not really sure this is useful,” with no rebuttal.


#7

Already linked :slight_smile:


#8

Bump!

I’m trying to introduce code review tools in a company that has lots of un-reviewed source code. The current project I’m on is written in Java and is a tangle of many classes. I want to pick a file, or handful of files and create an audit task on those files which would result in a code review.

What would be really cool is if Phabricator itself could tell me the audit state of the project. The thought is after a project is created/linked (we are using external git repositories until I can get buy in from the team) to the source code, one could see a list of audit states and set a flag to track file review status. Files could be selected and then an audit created for that file and pinned to the latest commit record in the tree at the time of audit. Alternatively, files that are in an un-reviewed state could be set to require a full code review before any changes could be committed and the differential screen would show the entire context of the source code by default.

Anyway, just a thought. If anyone could suggest a way of allowing an audit on a file without having to make random whitespace changes, I’d be appreciative.

Russ


#9

Its not a complete solution but its what we do…

when making our diffs for review add more diff context

git diff -U9999999

Then in the review you can see

image

This means every diff has the entire contents of the files that are changing and you can leave review comments everywhere


#10

Hey, thanks for the response. That might help. What I’m really after is for use with regulated software. If I have to pull in a bunch of external code, how do I verify any of it? What if we have a project that was used in a low criticality software that I want to import into a higher criticality software system/unit?

Again, the use case would be to import or link to a repository and then create a list of files that have been audited. Reviewers could then be assigned and progress tracked against that repository. Other features I could see:

  • Allowing for different types of audits such as Risk Mitigation, Code Quality, Problem Resolution Verification
  • Track the audit coverage for each file for the life of the project. This would be useful when post commit is performed and then pre-commit is turned on. As each file changes, the level of verify-ability goes down (by perhaps a percentage of lines changed?) until a full file audit is performed again.

#11

I think too that a file based audit process is a better approach. But I fear that it is not going to be implemented…