Phabric IDs / URLs in commits
Baptiste Daroussin
bapt at freebsd.org
Wed Jul 16 16:38:08 UTC 2014
On Wed, Jul 16, 2014 at 11:41:34AM -0400, John Baldwin wrote:
> 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
Btw the template is deeply related to phabricator, making a freebsd specific
template will be hard.
Over to volunteer :)
regards,
Bapt
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20140716/9c1a5585/attachment.sig>
More information about the svn-src-all
mailing list