Subject: Re: [PATCH v13 00/27] Convert "git stash" to C builtin

From: Thomas Gummerer <[email-masked]>
Date: Tue, 26 Feb 2019 20:48:28 +0000

On 02/26, Johannes Schindelin wrote:
> Hi Thomas,
> 
> On Mon, 25 Feb 2019, Thomas Gummerer wrote:
> > 22:  51809c70ca ! 23:  56a5ce2aeb stash: convert `stash--helper.c` into `stash.c`
> >     @@ -13,7 +13,7 @@
> >          called directly and not by a shell script.
> >      
> >          Signed-off-by: Paul-Sebastian Ungureanu <[email-masked]>
> >     -    Signed-off-by: Junio C Hamano <[email-masked]>
> >     +    Signed-off-by: Thomas Gummerer <[email-masked]>
> >      
> >       diff --git a/.gitignore b/.gitignore
> >       --- a/.gitignore
> >     @@ -273,9 +273,11 @@
> >       		return 0;
> >       
> >      -	strbuf_addstr(&stash_msg_buf, stash_msg);
> >     - 	if (!(ret = do_create_stash(ps, &stash_msg_buf, 0, 0, &info, NULL, 0)))
> >     +-	ret = do_create_stash(ps, &stash_msg_buf, include_untracked, 0, &info,
> >     ++	ret = do_create_stash(ps, &stash_msg_buf, 0, 0, &info,
> >     + 			      NULL, 0);
> 
> I do not quite understand this change, though. The end result seems to
> look similar enough to the previous iteration (except for the kept line
> wrapping which I would have undone in this patch), but how come that the
> `include_untracked` crept into some earlier patch in this iteration?

The reason for this is that previously we stopped passing
include_untracked through in patch 17.  As the option parsing in the
create_stash function was still in place at that point, it felt
incorrect to not pass it through to 'do_create_stash()'.

None of the tests failed at that point though, as nothing was actually
passing the command line options through anymore.  The last user of
that was removed in the "convert save to builtin" patch.

Arguably that would also be the right place to remove the option
parsing, rather than in this patch (convert `stash--helper.c` into
`stash.c`), but I tried to refrain from too much re-ordering to make
the changes more easily reviewable through range-diff.

Maybe I should have held back on these changes as well, or made the
other changes, dunno.  I wasn't always sure where to draw the line
between changing too much, which might hurt the chances of advancing
to next, because of the review work required, or doing too little,
which might hurt the chances because the series is not deemed in good
enough shape overall.

So what I ended up with is what I felt were the changes necessary to
advance the series, so we can start working on top, without the fear
of stepping on others peoples toes as much.

> The rest was obvious enough, thank you.
> 
> As I said before, I was very much in favor of getting this `stash-in-c`
> business moving again by advancing it to `next`.
> 
> From my perspective (which is not informed by any official statement about
> the role of `pu` or `next` because there is none as far as I know), the
> purpose of `pu` is to get patches into a shape where the original author
> is no longer the only one working on them. In my mind, that moment has
> long passed, and the fact that `ps/stash-in-c` is still stuck in `pu`
> really held me back on working more on this.

I mainly held back on doing much with it, because I didn't want to
step on Paul-Sebastians toes (and I thought we'd be okay with
advancing the series with the fixups on top).  But hopefully this is
now getting us into a state where we get this to next, and get further
work on top unblocked.

> So if you manage to get this finally cooking again, all hail to you!

I hope we can :)

> Thanks,
> Dscho