Message ID | 0374dde9-0b49-0af4-c718-e894b59e680f@polymtl.ca |
---|---|
State | Not applicable |
Headers |
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: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> 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 <joseph@codesourcery.com> Cc: Sergio Durigan Junior <sergiodj@redhat.com>, "Carlos O'Donell" <carlos@redhat.com>, libc-alpha <libc-alpha@sourceware.org>, Jonathan Nieder <jrnieder@gmail.com> References: <2e93ece9-386b-c587-9355-33a4695a3f02@redhat.com> <alpine.DEB.2.21.1910242122100.22279@digraph.polyomino.org.uk> <8b64d48f-3fd6-fa06-5b33-8daa97661fef@redhat.com> <87tv7xvaw7.fsf@redhat.com> <abf81e1a-3920-ef34-62b2-a582cbdc5f1c@polymtl.ca> <alpine.DEB.2.21.1910251438220.20732@digraph.polyomino.org.uk> From: Simon Marchi <simon.marchi@polymtl.ca> 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: <alpine.DEB.2.21.1910251438220.20732@digraph.polyomino.org.uk> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit |
Commit Message
Simon Marchi
Oct. 25, 2019, 3:48 p.m. UTC
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
Comments
On Fri, 25 Oct 2019, Simon Marchi wrote: > > 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: Yes. It's a reasonable heuristic that works in many common cases (and can be configured with a diff attribute in .gitattributes and a corresponding diff.<language>.xfuncname setting in .git/config, if desired). Sometimes the default amount of context in a diff isn't enough either, and sometimes code is rearranged in a way that makes diff output unhelpful. I'd like reasonable heuristics to be used in the gerrit messages so that what they show is sufficient in most cases for it to be possible to fully follow and contribute to the discussion via email without needing to go to the website. Showing the diff hunk, with default settings (so the same logic as used by default by git diff to identify a function name) seems like a reasonable heuristic for that purpose. > > 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 If using a topic were to mark the patches as a patch series (including marking email subjects and threading them accordingly), that would be fine. I don't think it's necessary for simply pushing multiple changes at once to be the way you cause something to be handled as a patch series, if topics are the idiomatic concept in gerrit that corresponds to a patch series as generated by git-format-patch. > > 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. Well, there would be issues if you wanted gerrit to push changes itself. And how does the "identify this as being the same change" system (for knowing when something has been committed) work if multiple separately reviewed patches are squashed into one commit? (Is the valid syntax for Change-Ids documented anywhere? In particular, is it OK to write a human-readable Change-Id oneself?)
On 2019-10-25 12:10 p.m., Joseph Myers wrote: >>> 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 > > If using a topic were to mark the patches as a patch series (including > marking email subjects and threading them accordingly), that would be > fine. I don't think it's necessary for simply pushing multiple changes at > once to be the way you cause something to be handled as a patch series, if > topics are the idiomatic concept in gerrit that corresponds to a patch > series as generated by git-format-patch. I don't believe the topic is included in the email (at least not the subject), but that is likely possible to do. For example, including it in the prefix tag: [review my-topic-name] I think that would help, but we would still be missing the information about the order of the patches. Any ideas for that? >>> 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. > > Well, there would be issues if you wanted gerrit to push changes itself. > And how does the "identify this as being the same change" system (for > knowing when something has been committed) work if multiple separately > reviewed patches are squashed into one commit? In that case, I would keep in the squashed commit the Change-Id of the first commit (i.e. the commit that contains the logical change, not the generated stuff). When pushing that commit, it will auto-close the first change. The last/final version of that change would therefore be the full thing, including the generated stuff. I would then need to abandon the second commit. In the reason field (when you click Abandon) I would say that since the content has been squashed in the parent change, which is now merged, this one is no longer needed. > (Is the valid syntax for Change-Ids documented anywhere? In particular, > is it OK to write a human-readable Change-Id oneself?) It's not mentioned here [1] so I suppose it's not documented. We would have to look in the source code (or just try it) to see what's supported. [1] https://gerrit-review.googlesource.com/Documentation/user-changeid.html 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);