Date: Mon, 8 Jan 2018 18:08:18 -0800 (PST) From: "Rodney W. Grimes" <freebsd@pdx.rh.CN85.dnsmgr.net> To: Pedro Giffuni <pfg@freebsd.org> Cc: cem@freebsd.org, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r327699 - head/sys/sys Message-ID: <201801090208.w0928IvH024760@pdx.rh.CN85.dnsmgr.net> In-Reply-To: <89964e3f-a982-f0e5-a7ff-9c13a5ebe61c@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[ Charset UTF-8 unsupported, converting... ] > Hello; > > On 08/01/2018 15:27, Conrad Meyer wrote: > > I'm (again, atypically) with rgrimes here. Revert has a specific > > meaning and shouldn't be used like this. Making a commit with the > > subject "Revert rXXX" that does more than just "svn revert rXXX" makes > > life harder for developers looking at code history after you. Making > > two separate commits for two different changes (revert, then add the > > annotation) is not burdensome. > > > > Best, > > Conrad > > Yeah, I understand where that comes from and I will take it into account > for future commits, but I think it should be *documented* and not assume > that everybody?s I agree, we should have a section in the commiters guide on doing reverts. We should also have one on how to merge code to other branches and how to format commit messages for merged code. Most of these things should be taught during mentorship, perhaps even a check list for mentor/mentee to make sure they have done most, if not all, of some specific set of things before they are releaesed. I would also like to see a standardize of the revert log format by adding a Revert: line to the template so that it is there in a very standard form that this reverts revision rXXXXXX > thinks that is the way version control is supposed to be > used. My background taught me that this is how document control should be used. Vcs is just an automated form of document control. We should also try to do more seperatation of style changes vs functional changes. It just makes things easier to both review and understand. Same with a function add done with a refactor of code, mixing the two in one commit, even in one differential review, makes it harder to understand what is just simply a refactor, and what is the new code. Nothing wrong with making 3 commits in a row to the same area, style, refactor, functional change. This makes it very easy to analyze the changes and IMHO could reduce the rate of breakage. I believe phabricator has the tooling to stack these as well? Dependent diffs or some such thing? > Pedro. > > > On Mon, Jan 8, 2018 at 12:18 PM, Pedro Giffuni <pfg@freebsd.org> wrote: > >> On 08/01/2018 11:09, Rodney W. Grimes wrote: > >>>> Author: pfg > >>>> Date: Mon Jan 8 15:54:29 2018 > >>>> New Revision: 327699 > >>>> URL: https://svnweb.freebsd.org/changeset/base/327699 > >>>> > >>>> Log: > >>>> Revert r327697: > >>>> malloc(9): drop the __result_use_check attribute for the kernel > >>>> allocator. > >>>> My bad: __result_use_check just checks the for the general and we > >>>> always > >>>> want to make sure allocated memory is used, not only checked for > >>>> nullness. > >>>> Add it to reallocf since that was missing. > >>> Please try not to combine a revert with an add, it makes it messy > >>> to try and figure out things in the future when only the svn log > >>> is being used to analyze stuff as digging in mail archives becomes > >>> painful. > >>> > >>> Revert, then commit the add standalone, is the better sequence in > >>> this type of situation. > >> > >> Not that any of this is defined in our committers guide but IMHO "svn > >> revert" is just a tool, pretty much as "svn move" and "svn copy". The idea > >> is to make a committers' life easier: making two commits for such a simple > >> change is not "easier". > >> > >> In this case the change is rather consistent: I added __result_use_check to > >> the three functions that needed it. > >> The commit log is not only clear on why the revert happened but also > >> explains the additional one line change. > >> > >> When I MFC it, I will merge both changes for repository consistency but the > >> log will basically mention that I am adding __result_use_check to > >> reallocf(). > >> > >> > >> Pedro. > >> > >>>> Modified: > >>>> head/sys/sys/malloc.h > >>>> > >>>> Modified: head/sys/sys/malloc.h > >>>> > >>>> ============================================================================== > >>>> --- head/sys/sys/malloc.h Mon Jan 8 15:41:49 2018 (r327698) > >>>> +++ head/sys/sys/malloc.h Mon Jan 8 15:54:29 2018 (r327699) > >>>> @@ -176,7 +176,7 @@ void *contigmalloc(unsigned long size, struct > >>>> malloc_t > >>>> __alloc_size(1) __alloc_align(6); > >>>> void free(void *addr, struct malloc_type *type); > >>>> void *malloc(unsigned long size, struct malloc_type *type, int flags) > >>>> - __malloc_like __alloc_size(1); > >>>> + __malloc_like __result_use_check __alloc_size(1); > >>>> void *mallocarray(size_t nmemb, size_t size, struct malloc_type *type, > >>>> int flags) __malloc_like __result_use_check > >>>> __alloc_size(1) __alloc_size(2); > >>>> @@ -187,9 +187,9 @@ void malloc_type_freed(struct malloc_type > >>>> *type, unsig > >>>> void malloc_type_list(malloc_type_list_func_t *, void *); > >>>> void malloc_uninit(void *); > >>>> void *realloc(void *addr, unsigned long size, struct malloc_type > >>>> *type, > >>>> - int flags) __alloc_size(2); > >>>> + int flags) __result_use_check __alloc_size(2); > >>>> void *reallocf(void *addr, unsigned long size, struct malloc_type > >>>> *type, > >>>> - int flags) __alloc_size(2); > >>>> + int flags) __result_use_check __alloc_size(2); > >>>> struct malloc_type *malloc_desc2type(const char *desc); > >>>> #endif /* _KERNEL */ > >>>> > >>>> > >> Pedro. > >> > > > -- Rod Grimes rgrimes@freebsd.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201801090208.w0928IvH024760>