From patchwork Fri Oct 25 15:48:05 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 35342 Received: (qmail 41147 invoked by alias); 25 Oct 2019 15:48:44 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 40956 invoked by uid 89); 25 Oct 2019 15:48:25 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.1 spammy=mails, UD:a.m, am, a.m X-HELO: smtp.polymtl.ca DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca x9PFm6PZ032010 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1572018493; bh=H5KyIycZQrksFkVp2TeLvEalbinVj5Tisq8dRctjY+s=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=NC+CLne4T+5k/AsqWywlIXxHffP4WwYw6egivYHYZHgDpv3XEUY4/dbBwdT0YBEer 37hKxplLsctdbwxAOD0rk7WdIX2TVXzk/6mCncp+1kIPp9+cEJaDd5k8jAXcWUXzZ1 j7hoHOgWqo25/ynDVbqS8ty00xojKX0h8HuSwhKQ= Subject: Re: Setup non-pushing gerrit instance for glibc. To: Joseph Myers Cc: Sergio Durigan Junior , "Carlos O'Donell" , libc-alpha , Jonathan Nieder References: <2e93ece9-386b-c587-9355-33a4695a3f02@redhat.com> <8b64d48f-3fd6-fa06-5b33-8daa97661fef@redhat.com> <87tv7xvaw7.fsf@redhat.com> From: Simon Marchi Message-ID: <0374dde9-0b49-0af4-c718-e894b59e680f@polymtl.ca> Date: Fri, 25 Oct 2019 11:48:05 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: On 2019-10-25 10:48 a.m., Joseph Myers wrote: > On Thu, 24 Oct 2019, Simon Marchi wrote: > >> We'll maybe manage to do something a bit smarter, but I don't think we'll >> easily be able to quote the _hunk_. This is where the mail is generated: > > I think the hunk - not just the new version of the code on its own - is > critical information if someone is commenting on a particular part of a > change, needed for such comments to be properly readable in context. I understand that, I'm just saying I don't think I'll be able to do this change (maybe an experienced Gerrit developer could). >> Also, remember that you can go put a comment on an unchanged section on the >> file (e.g. to say "you forgot to update this call"), so there would be no >> diff hunk to show for that comment anyway. > > In that case it should at least show a reasonable amount of context, with > some indication of what function it is in, like in the diff hunk header. From my experience, git diff doesn't really get the function name, it just gives you the first preceding line that starts with a non-whitespace. It often "breaks" when you have labels or preprocessor directives at column 0, for example: Since it's pretty basic, I could probably replicate that method to include a few lines before the line of interest, as well as the first preceding line that starts with a non-whitespace. >> They are naturally related due to them being git commits and having a >> parent-child relationship. If I check out C, I'll automatically check >> out its parent B and C. Gerrit doesn't see that as a series, just as >> individual changes that are dependent on one another. > > There is a difference of *intent* between changes that depend on one > another and a patch series. A patch series is saying: > > * there is a common purpose motivating the patches; and In Gerrit, you'd probably use topics to indicate that many changes have a common purpose (whether they are interdependent or not). https://gerrit-review.googlesource.com/Documentation/intro-user.html#topics > * some patches may best be understood by reference to ones later in the > series (if e.g. one patch adds an interface that a later one adds users > for), so the link to those later patches is important for review purposes. For that second point, I believe that a preparatory patch should mention in its commit message that it's adding something not used yet, with the intent of using it in a later patch. This way, somebody browsing the git (without access to the cover letter) tree will understand why this particular commit adds a function that's not used. So that is clear to the reviewer anyway. As long as Gerrit lets you find those later patches by showing you the relation chain, I think it's clear. > And so a patch series should be sent out to the list for review as such. > There may be comments on the series as a whole, or on individual patches > in it. > > There's also the variant of patch series that are explained in the cover > letter (or in other text not intended for the final commit) as being split > only for review purposes and intended to be squashed for commit, if the > pieces most convenient for review don't form bisectable commits. I'm not > sure how much any review system needs to know about the distinction > between the two kinds of patch series if it's not pushing the commits > itself. I don't think there would be a problem with that, except that the two emails on the mailing would not be threaded together. More thoughts about that below. >> Let's say I push a new version of C. A and B are still at their v1, >> while C is at its v2. Then, if I push D on top of that, D will be at >> its v1. >> >> Then, I (or even you!) could make a new change E on top of A, where E >> would be unrelated to B, C and D (other than sharing A in their >> ancestry). >> >> I can then decide to rebase A by itself (because master has moved on), >> which will create a v2 of A. All the other changes will still be based >> on the v1 of A. I will be able to rebase the other changes later on the >> latest version of A, if I want to. > > All of this is perfectly meaningful for patch series - you can do [PATCHv2 > 3/3] and then potentially [PATCH 4/3] for D if it's intended as part of > the series (or just a separate submission for D if it's not intended as > part of the series). You can revise patch 1/3 with or without refreshing > the whole series. I understand that we can do that with patch series. My point is that I don't see an easy way of translating changes from Gerrit's (current) model - i.e. git branches - into a patch series model. About cover letters: Sergio mentioned that a hacky way of doing a cover letter would be to add an empty commit, whose commit message is the cover letter. At first I thought this commit would come before the actual commits. But if we put it at the end of the chain, maybe it works. Let's say you have A: Preparatory patch to add function foo B: Preparatory patch to add function bar C: Big feature that uses foo and bar D: Cover letter that explains my series With this relationship: A <- B <- C <- D Then when you checkout out the cover letter, you check out the whole series. If you modify C and push again, both C and D will be at v2. So essentially, the version of D kind of becomes the version of the series. And when you actually push the series, you don't push D. I understand that there is still the downside that if you are following only from the mailing list, these mails are not threaded together. Perhaps that can be improved, like if I open a new change that depends on another change that is still open (not merged yet, still up for review), then we send the mail in reply to the mail of that other change. Simon diff --git a/src/plugins/ctf/common/metadata/decoder.c b/src/plugins/ctf/common/metadata/decoder.c index a62beebfcd99..5e4facb4ff27 100644 --- a/src/plugins/ctf/common/metadata/decoder.c +++ b/src/plugins/ctf/common/metadata/decoder.c @@ -351,6 +351,7 @@ end: "mdec-addr=%p", mdec); } } + hello free(buf);