All of lore.kernel.org
 help / color / mirror / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Thomas Gummerer" <t.gummerer@gmail.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Paul-Sebastian Ungureanu" <ungureanupaulsebastian@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v12 18/26] stash: convert push to builtin
Date: Wed, 20 Feb 2019 22:10:01 +0100 (STD)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1902202205470.41@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <xmqqmumrvzwk.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Tue, 19 Feb 2019, Junio C Hamano wrote:

> 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. He put in quite a few dozen hours of work *after* GSoC.

> 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.
> 
> 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..

My throwing hands into the air and giving up on advancing it in the
current form to `next` was based on its rotting away in `pu`...

If it had been in `next` while Hannes, others and I found and fixed bugs,
then it would truly be cooking, but in `pu`? It kind of stopped pretty
much all the development around it.

And that's the only reason why I came up with that plan (that will require
at least 50 hours from my side, I know that): because I thought that your
not advancing the patches to `next` meant that this extra work was exactly
what you were expecting.

> > 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.
> 
> 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.

Yes, you are right. This is on me. Guilty as charged.

And I don't mean this flippantly. I thought long and hard about this, and
I've come to the conclusion that I will not offer to mentor this round of
GSoC as a consequence.

Ciao,
Dscho

> 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.
> 
>     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.
> 

  reply	other threads:[~2019-02-20 21:10 UTC|newest]

Thread overview: 106+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <https://public-inbox.org/git/cover.1542925164.git.ungureanupaulsebastian@gmail.com/>
2018-12-20 19:44 ` [PATCH v12 00/26] Convert "git stash" to C builtin Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 01/26] sha1-name.c: add `get_oidf()` which acts like `get_oid()` Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 02/26] strbuf.c: add `strbuf_join_argv()` Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 03/26] strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()` Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 04/26] ident: add the ability to provide a "fallback identity" Paul-Sebastian Ungureanu
2018-12-26 21:21     ` Junio C Hamano
2018-12-27 21:24       ` Johannes Schindelin
2018-12-28 19:40         ` Junio C Hamano
2018-12-20 19:44   ` [PATCH v12 05/26] stash: improve option parsing test coverage Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 06/26] t3903: modernize style Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 07/26] stash: rename test cases to be more descriptive Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 08/26] stash: add tests for `git stash show` config Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 09/26] stash: mention options in `show` synopsis Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 10/26] stash: convert apply to builtin Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 11/26] stash: convert drop and clear " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 12/26] stash: convert branch " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 13/26] stash: convert pop " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 14/26] stash: convert list " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 15/26] stash: convert show " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 16/26] stash: convert store " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 17/26] stash: convert create " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 18/26] stash: convert push " Paul-Sebastian Ungureanu
2019-02-08 11:30     ` SZEDER Gábor
2019-02-10 22:17       ` Thomas Gummerer
2019-02-11  1:13         ` SZEDER Gábor
2019-02-12 23:18           ` Thomas Gummerer
2019-02-19  0:23             ` SZEDER Gábor
2019-02-19 10:47               ` Johannes Schindelin
2019-02-19 19:59                 ` Junio C Hamano
2019-02-20 21:01                   ` Johannes Schindelin
2019-02-19 23:59                 ` Thomas Gummerer
2019-02-20  4:37                   ` Junio C Hamano
2019-02-20 21:10                     ` Johannes Schindelin [this message]
2019-02-20 22:30                     ` Thomas Gummerer
2019-02-25 23:16                 ` [PATCH v13 00/27] Convert "git stash" to C builtin Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 01/27] sha1-name.c: add `get_oidf()` which acts like `get_oid()` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 02/27] strbuf.c: add `strbuf_join_argv()` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 03/27] strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 04/27] ident: add the ability to provide a "fallback identity" Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 05/27] stash: improve option parsing test coverage Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 06/27] t3903: modernize style Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 07/27] t3903: add test for --intent-to-add file Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 08/27] stash: rename test cases to be more descriptive Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 09/27] stash: add tests for `git stash show` config Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 10/27] stash: mention options in `show` synopsis Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 11/27] stash: convert apply to builtin Thomas Gummerer
2019-03-14 13:19                     ` regression in new built-in stash + fsmonitor (was Re: [PATCH v13 11/27] stash: convert apply to builtin) Ævar Arnfjörð Bjarmason
2019-03-14 15:20                       ` Johannes Schindelin
2019-03-14 15:40                         ` Ævar Arnfjörð Bjarmason
2019-03-14 22:45                           ` Johannes Schindelin
2019-03-14 23:39                             ` Ævar Arnfjörð Bjarmason
2019-03-15  2:23                               ` Ben Peart
2019-02-25 23:16                   ` [PATCH v13 12/27] stash: convert drop and clear to builtin Thomas Gummerer
2019-03-07 19:15                     ` Jeff King
2019-03-09 18:30                       ` Thomas Gummerer
2019-03-10 23:26                         ` Jeff King
2019-03-11  1:40                         ` Junio C Hamano
2019-03-11 21:40                           ` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 13/27] stash: convert branch " Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 14/27] stash: convert pop " Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 15/27] stash: convert list " Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 16/27] stash: convert show " Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 17/27] stash: convert store " Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 18/27] stash: convert create " Thomas Gummerer
2019-03-07 19:18                     ` Jeff King
2019-03-08 15:30                       ` Johannes Schindelin
2019-03-09 18:26                         ` Thomas Gummerer
2019-03-11  1:47                           ` Junio C Hamano
2019-03-11  7:30                             ` Junio C Hamano
2019-03-11 21:42                               ` Thomas Gummerer
2019-03-11 22:16                                 ` [PATCH v2] stash: pass pathspec as pointer Thomas Gummerer
2019-03-12  6:50                                   ` Junio C Hamano
2019-03-12 22:35                                   ` Johannes Schindelin
2019-03-12 23:40                                     ` Thomas Gummerer
2019-03-13  1:47                                       ` Junio C Hamano
2019-03-13 22:14                                       ` Johannes Schindelin
2019-03-15 22:33                                         ` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 19/27] stash: convert push to builtin Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 20/27] stash: make push -q quiet Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 21/27] stash: convert save to builtin Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 22/27] stash: optimize `get_untracked_files()` and `check_changes()` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 23/27] stash: replace all `write-tree` child processes with API calls Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 24/27] stash: convert `stash--helper.c` into `stash.c` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 25/27] stash: add back the original, scripted `git stash` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 26/27] stash: optionally use the scripted version again Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 27/27] tests: add a special setup where stash.useBuiltin is off Thomas Gummerer
2019-02-26 12:40                   ` [PATCH v13 00/27] Convert "git stash" to C builtin Johannes Schindelin
2019-02-26 20:48                     ` Thomas Gummerer
2019-02-26 21:45                   ` Ævar Arnfjörð Bjarmason
2019-02-26 22:37                     ` Johannes Schindelin
2019-03-03  1:24                   ` Junio C Hamano
2019-03-03  1:25                   ` Junio C Hamano
2019-03-03 10:03                     ` Thomas Gummerer
2018-12-20 19:44   ` [PATCH v12 19/26] stash: make push -q quiet Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 20/26] stash: convert save to builtin Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 21/26] stash: optimize `get_untracked_files()` and `check_changes()` Paul-Sebastian Ungureanu
2019-01-06 22:47     ` Thomas Gummerer
2018-12-20 19:44   ` [PATCH v12 22/26] stash: replace all `write-tree` child processes with API calls Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 23/26] stash: convert `stash--helper.c` into `stash.c` Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 24/26] stash: add back the original, scripted `git stash` Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 25/26] stash: optionally use the scripted version again Paul-Sebastian Ungureanu
2019-01-06 22:59     ` Thomas Gummerer
2018-12-20 19:44   ` [PATCH v12 26/26] tests: add a special setup where stash.useBuiltin is off Paul-Sebastian Ungureanu
2019-01-03 23:39   ` [PATCH v12 00/26] Convert "git stash" to C builtin Junio C Hamano
2019-01-18 12:06     ` Johannes Schindelin
2019-01-18 17:49       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.1902202205470.41@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=szeder.dev@gmail.com \
    --cc=t.gummerer@gmail.com \
    --cc=ungureanupaulsebastian@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.