Phabric IDs / URLs in commits
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.
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'.
So what do folks prefer for 1) and 2)?
--
John Baldwin
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: Phabric IDs / URLs in commits
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/ 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.)
--
John Baldwin
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: Phabric IDs / URLs in commits
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/ 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: 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.
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: Phabric IDs / URLs in commits
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/ 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: 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/' ala 'review.freebsd.org/' or the like.
--
John Baldwin
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: Phabric IDs / URLs in commits
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/ 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: 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/' ala 'review.freebsd.org/' 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
pgpK6JQuQGz0X.pgp
Description: PGP signature
Re: Phabric IDs / URLs in commits
On 11 July 2014 12:38, John Baldwin wrote: >> 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. FYI, Phabricator's canonical format for this is: Differential Revision: http://phabric.freebsd.org/D312 It will then automatically associate the commit with the review and close the review. See for example LLVM review D4563: Review: http://reviews.llvm.org/D4563 Commit: http://reviews.llvm.org/rL213304 (from LLVM's Phabricator repo browser; LLVM's viewvc seems to be down at the moment.) "Differential Revision" seems a bit unwieldy and "CR" does fit in better with our other tags. But we'll want to teach Phabricator to parse our custom tag if we go that way. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: Phabric IDs / URLs in commits
On 7/11/2014 11:38 AM, 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.
>
> 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'.
>
> So what do folks prefer for 1) and 2)?
>
FYI Ports has been using the convention: "Phabric\tDXXX"
--
Regards,
Bryan Drewery
signature.asc
Description: OpenPGP digital signature
Re: Phabric IDs / URLs in commits
On 7/11/2014 11:47 AM, Bryan Drewery wrote:
> On 7/11/2014 11:38 AM, 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.
>>
>> 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'.
>>
>> So what do folks prefer for 1) and 2)?
>>
>
> FYI Ports has been using the convention: "Phabric\tDXXX"
>
'Phabric:\tDXXX' with : of course.
--
Regards,
Bryan Drewery
signature.asc
Description: OpenPGP digital signature
Re: Phabric IDs / URLs in commits
I don't know the ins and outs of arcanist but at Facebook we use 'arc
amend' to update the local commit message with whatever is in phabricator.
This includes the name of the reviewer.
Florent
On Fri, Jul 11, 2014 at 9:38 AM, 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.
>
> 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'.
>
> So what do folks prefer for 1) and 2)?
>
> --
> John Baldwin
> ___
> svn-src-all@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/svn-src-all
> To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
>
--
Florent
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: Phabric IDs / URLs in commits
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/ that also works and it is
short :)
>
> 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.
regards,
Bapt
pgp5AuGPvSUIi.pgp
Description: PGP signature
Re: Phabric IDs / URLs in commits
On Fri, Jul 11, 2014 at 10:51:32AM -0700, Florent Thoumie wrote: > I don't know the ins and outs of arcanist but at Facebook we use 'arc > amend' to update the local commit message with whatever is in phabricator. > This includes the name of the reviewer. That only works because you are using a dvcs. regards, Bapt pgp4Dyk554r5V.pgp Description: PGP signature
Re: Phabric IDs / URLs in commits
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/ 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
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: Phabric IDs / URLs in commits
On Fri, Jul 11, 2014 at 11:06 AM, Baptiste Daroussin wrote: > On Fri, Jul 11, 2014 at 10:51:32AM -0700, Florent Thoumie wrote: > > I don't know the ins and outs of arcanist but at Facebook we use 'arc > > amend' to update the local commit message with whatever is in > phabricator. > > This includes the name of the reviewer. > > That only works because you are using a dvcs. > At least one of the repos is svn-backed (but yeah we do have a git-svn bridge, I haven't used arc with pure svn). -- Florent ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
