Phabric IDs / URLs in commits

[John Baldwin]

On Friday, July 11, 2014 1:54:42 pm Baptiste Daroussin wrote:
> On Fri, Jul 11, 2014 at 12:38:23PM -0400, John Baldwin wrote:
> > On Friday, July 11, 2014 12:16:26 pm John Baldwin wrote:
> > > Author: jhb
> > > Date: Fri Jul 11 16:16:26 2014
> > > New Revision: 268531
> > > URL: http://svnweb.freebsd.org/changeset/base/268531
> > > 
> > > Log:
> > >   Fix some edge cases with rewinddir():
> > >   - In the unionfs case, opendir() and fdopendir() read the directory's full
> > >     contents and cache it.  This cache is not refreshed when rewinddir() is
> > >     called, so rewinddir() will not notice updates to a directory.  Fix this
> > >     by splitting the code to fetch a directory's contents out of
> > >     __opendir_common() into a new _filldir() function and call this from
> > >     rewinddir() when operating on a unionfs directory.
> > >   - If rewinddir() is called on a directory opened with fdopendir() before
> > >     any directory entries are fetched, rewinddir() will not adjust the seek
> > >     location of the backing file descriptor.  If the file descriptor passed
> > >     to fdopendir() had a non-zero offset, the rewinddir() will not rewind to
> > >     the beginning.  Fix this by always seeking back to 0 in rewinddir().
> > >     This means the dd_rewind hack can also be removed.
> > >   
> > >   While here, add missing locking to rewinddir().
> > >   
> > >   CR:   	    	https://phabric.freebsd.org/D312
> > >   Reviewed by:	jilles
> > >   MFC after:	1 week
> > 
> > Just picking my own commit here as a sample case.
> > 
> > I think we should be annotating commits with phabricator code reviews in some 
> > way when a change has gone through that review.  It is very useful to get back
> > to the review details from the commit log message in svnweb, etc.
> > 
> > I can see a number of different ways to do this, but I do think it would be
> > nice to pick a consistent way to do it.
> > 
> > Things to consider:
> > 
> > 1) The tag ("CR:" is what I used above).  I don't care, just pick one.  I
> >    chose CR since Warner used it previously.  Whatever we decide, we should
> >    add it to the template.
> > 
> > 2) ID vs full URL.  For PRs we just list the bug ID and not the full URL
> >    (same for Coverity).  I would be fine with that so long as someone hacks
> >    up svnweb to convert the IDs into links (the way it handles PR bug
> >    numbers).  OTOH, if you use the full URL you get that for free in svnweb,
> >    and you also get it in mail clients, etc.  It helps that the URL isn't but
> >    so long.
> 
> for bugs we could use http://bugs.FreeBSD.org/<number> that also works and it is
> short :)

Yeah, I thought about that to.  It would also have the same properties (works in
e-mail clients, doesn't require special hacks to svnweb).  I would not be opposed
to doing that, but that seems like a larger change.  I do think that if we want to
just put the ID we need to hack svnweb so at least in svnweb the IDs are links.

> > This is more of a pie-in-the-sky, but it would be _really_ nice if arcanist 
> > were hacked up to support our local commit template and would auto populate 
> > the 'Reviewed by' and 'CR' (or whatever it ends up being called) fields so one 
> > could use 'arc commit'.
> 
> I'm planning to work on this but I first need to finish tracking 2 bugs it has
> with svn.

Very cool!

-- 
John Baldwin