Phabric IDs / URLs in commits

[John Baldwin]

On Wednesday, July 16, 2014 09:27:57 AM Kubilay Kocak wrote:
> On 16/07/2014 1:12 AM, John Baldwin wrote:
> > 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 :)
> > 
> > Ok, so Bryan said ports uses 'Phabric: Dxxx' and I read Baptiste's e-mail
> > as a preference for the URL itself (no preference on the prefix though?) 
> > Any other thoughts?  I probably lean towards the full URL personally
> > since it requires less work (no hacking on svnweb) and works
> > out-of-the-box in more forums (e-mail, etc.)
> +100 on CR: <ID-including-D-prefix> without URL's to keep them decoupled
> and forever valid in the (probably very likely) case we change
> hostnames/urls.
> 
> I'm liking phabric so far, but would opt for a more concrete
> review.freebsd.org if I had the choice (and when it's ready). This way
> our "review" processes and workflows can be extended or modified
> orthogonal to the tool in use.

Note that we could choose a "canonical" URL similar to how we have done
for 'bugs.freebsd.org/<ID>' ala 'review.freebsd.org/<ID>' or the like.

-- 
John Baldwin