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 <ungureanupaulsebastian@xxxxxxxxx> > > - Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> > > + Signed-off-by: Thomas Gummerer <t.gummerer@xxxxxxxxx> > > > > 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
