Re: [PATCH v12 18/26] stash: convert push to builtin

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Feb 20, 2019, 22:30 UTC
Message-ID: <20190220223028.GN6085@hank.intra.tgummerer.com>
In-Reply-To: <xmqqmumrvzwk.fsf@gitster-ct.c.googlers.com>

On 02/19, Junio C Hamano wrote:

Show 17 quoted lines

> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> >> Now, I seriously believe that we missed the best time to move
> >> ps/stash-in-c into `next` for cooking. The best time would have been just
> >> ...
> >> Anyway, that's my plan for now.
> >
> > I must say I am not very happy about this plan.  The series has been
> > marked as "Will merge to 'next'" in previous iterations, but then we
> > found some issues that prevented that.  However I thought we were fine
> > fixing those on top at this point, rather than starting with a new
> > iteration again.
> 
> First before going into anything else, let me thank, and let me
> invite readers of this thread to join me thanking, Paul (Sebi) for
> sticking with this topic for this long.  It is above and beyond what
> GSoC calls for.

Indeed, thanks for all your work on this Paul-Sebastian!

Show 12 quoted lines

> Having said that.
> 
> I too was somehow led to believe that the topic was in a good enough
> shape, with some room for clean-up by reordering the patches to make
> them into a more logical progression and squashing an existing and
> recently figured out "oops, that was wrong" fixes into the patches
> where the breakages originate.
> 
> And that was where the "Will merge to" originally came from.  Thanks
> to tools like range-diff, a topic that goes through such reordering
> and squashing of patches should not have to "waste" a lot of review
> cycles out of those who have seen the previous round.

Right, I had the impression that we were okay with doing the cleanups on top of what is already in 'pu'. Especially since the topic with Johannes Sixt's patch on top was marked as "Will merge to 'next'" at one point if I remember correctly. I didn't think that it being in 'pu' vs. it being in 'next' would make too much of a difference there, and that it's just a by-product of where in the development cycle we are rather than an indication of which way we are taking the branch.

Show 7 quoted lines

> It however is a totally different matter if the topic was so
> unsalvageable that it needs a total rewrite---that would need
> another round of careful review, of course, and it would be
> irresponsive to merge a topic in such a messy state to 'next'.  But
> my impression was that the topic was not _that_ bad, so Dscho's
> message and the plan were something that was totally unexpected to
> me, too..

Indeed, the topic did not get any worse over the time it was in 'pu', indeed it got a couple of fixes on top. And my impression was that Dscho still would have wanted to get the topic merged to next much earlier, so I don't quite understand what changed since then, other than getting a few fixes on top.

Show 14 quoted lines

> > I was always under the impression that once the problem that was
> > discovered here was fixed we'd advance the series to 'next' with the
> > patch that comes out of this discussion on top.  Whether it's in next
> > shortly before 2.21 or not doesn't seem to make much of a difference
> > to me, as this wasn't going to make the 2.21 release anyway.  My hope
> > was that we could get it into 'next' shortly after 2.21 is released to
> > get the series some further exposure (which may well turn up some
> > other issues that we are not aware of yet, but such is the life of
> > software).
> 
> I was hoping similar, but also was hoping that people would use the
> time wisely while waiting for the next cycle to polish the topic with
> reordering and squashing, so that it can hit 'next' early once the
> tree opens.

I'd be happy to do this myself, but as mentioned above I thought we were okay with just having the patches on top, and leave the work Paul-Sebastian sent until now as it was. If that impression was wrong I'm happy to put in some work to help with the cleanup.

Show 26 quoted lines

> Anyway.
> 
> I actually have a different issue with this topic, though.  It is
> wonderful to see a GSoC student's continued involvement in the
> project, but it is not healthy that we need so much work on top of
> what was marked "done" at the end of the GSoC period.  Especially
> the impression I am getting for the post GSoC work of this topic is
> not "we are already done converting to built-in during GSoC, and now
> we are extending the command", but "we ran out of time during GSoC;
> here is what we would have seen at the end of GSoC in an ideal
> world."
> 
> I wonder if this is an unfortunate indication that our expectation
> is unrealistically high when we accept students' applications.
> Being overly ambitious is *not* students' fault, but those of us on
> the list, especially those who mentor, have far deeper experience
> with how our code and project is structured than any students do.
> We should be able to, and should not hesitate to, say things like
> "that's overly ambitious---for such and such, you'd need to even
> invent an internal API---can we reduce the scope and still produce a
> useful end result?"
> 
> One suggestion I have is to have success criteria (e.g. "gets merged
> to 'master' before GSoC ends" [*1*]) clearly spelled out in the
> application.  Something like that would help managing the
> expectation and biting way too much for a summer, I'd hope.

[Adding Christian and Olga to cc here as this discussion should be interesting to them as well as GSoC mentors]

I think one thing we underestimated at least here is how long it takes from "everything that we intended to do is done" to "this is reviewed and ready to merge into 'next' and 'master'". This is partly my fault as well because it took me quite a while to review the series on the list, which certainly didn't help in moving things along.

While having set criteria is a good idea, I think we should still give some leeway to the mentors in terms of the actual success/fail rating in the program. Not getting things merged is definitely not a good experience, but it would be much worse to not get it merged, and also fail GSoC and not getting paid for the efforts over the summer. We shouldn't punish students for the failure of the mentors to estimate projects correctly.

One thing we should do I think is to say the project should be "complete" at least a month before the end of GSoC, and that the last month should only be dedicated to polishing the patches. I don't know how to make sure the students still have enough work to do during that last period while they are just waiting for reviewers.

An other alternative (just thinking out loud here) is to make sure the project has a few deliverables that can and will be merged individually, even if they later build on top of eachother. This would mean getting students to send multiple series, when the patches may normally go better in a single series, but I think it would help moving the review cycle along faster.

Dunno, maybe there are other alternatives that I'm missing as well.

Show 9 quoted lines

>     Side note *1*.  Of course, depending on the alignment of the
>     stars ^W our ~10-12 week development cycle and the end of GSoC,
>     getting merged to 'master' might become impossible if it
>     coincides with the pre-release freeze period.  But we on the
>     list and the mentors know how the project works, and can help
>     stating a more realistic success criterion if the development
>     cycle and other quirks specific to this project gets in the way.
> 
> Thanks.