Message ID | 20190225231631.30507-1-t.gummerer@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | Convert "git stash" to C builtin | expand |
Hi Thomas, On Mon, 25 Feb 2019, Thomas Gummerer wrote: > As I was advocating for this series to go into 'next' without a large > refactor of this series, I'll put my money were my mouth is and try to > make the cleanups and fixes required, though without trying to avoid > further external process calls, or changing the series around too much. Thank you. > range-diff below: > > [...] > 9: f6bbd78127 ! 10: 45670448e8 stash: convert apply to builtin > @@ -17,7 +17,7 @@ > > Signed-off-by: Joel Teichroeb <joel@teichroeb.net> > Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> > - Signed-off-by: Junio C Hamano <gitster@pobox.com> > + Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > > diff --git a/.gitignore b/.gitignore > --- a/.gitignore > @@ -96,7 +96,6 @@ > + * i_tree is set to the index tree > + * u_tree is set to the untracked files tree > + */ > -+ > +struct stash_info { > + struct object_id w_commit; > + struct object_id b_commit; > @@ -357,7 +356,7 @@ > + if (refresh_cache(REFRESH_QUIET)) > + return -1; > + > -+ if (write_cache_as_tree(&c_tree, 0, NULL) || reset_tree(&c_tree, 0, 0)) > ++ if (write_cache_as_tree(&c_tree, 0, NULL)) Thank you for picking this up. > + return error(_("cannot apply a stash in the middle of a merge")); > + > + if (index) { > > [...] > 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@gmail.com> > - Signed-off-by: Junio C Hamano <gitster@pobox.com> > + Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > > 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 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. So if you manage to get this finally cooking again, all hail to you! Thanks, Dscho
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@gmail.com> > > - Signed-off-by: Junio C Hamano <gitster@pobox.com> > > + Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > > > > 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
On Tue, Feb 26 2019, Thomas Gummerer wrote: > As I was advocating for this series to go into 'next' without a large > refactor of this series, I'll put my money were my mouth is and try to > make the cleanups and fixes required, though without trying to avoid > further external process calls, or changing the series around too > much. > > One thing to consider here is that we have a GSoC project planned > based on 'git stash'. If we can't get this to 'next' soon, I'd vote > for taking that project out of this years GSoC, and maybe try again > next year, if nobody implemented the feature in the meantime. FWIW I'd like to +1 getting it into "next" so this can be given thorough testing in the 2.22 If there's still bugs or other regressions I think it's better sorted out without the cognitive load of reviewing it all again. Worst case we can always add something on top to flip the default of stash.useBuiltin.
Hi Ævar, On Tue, 26 Feb 2019, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Feb 26 2019, Thomas Gummerer wrote: > > > As I was advocating for this series to go into 'next' without a large > > refactor of this series, I'll put my money were my mouth is and try to > > make the cleanups and fixes required, though without trying to avoid > > further external process calls, or changing the series around too > > much. > > > > One thing to consider here is that we have a GSoC project planned > > based on 'git stash'. If we can't get this to 'next' soon, I'd vote > > for taking that project out of this years GSoC, and maybe try again > > next year, if nobody implemented the feature in the meantime. > > FWIW I'd like to +1 getting it into "next" so this can be given thorough > testing in the 2.22 Indeed. It is time. > If there's still bugs or other regressions I think it's better sorted > out without the cognitive load of reviewing it all again. Fully agree! > Worst case we can always add something on top to flip the default of > stash.useBuiltin. I actually don't think that will be necessary. If my track record with fixing all kinds of built-in rebase corner-case bugs leading up to v2.20.0 is any indication, the built-in stash will be cooking well. Ciao, Dscho
Thomas Gummerer <t.gummerer@gmail.com> writes: > One thing that came up in the latest reviews, was to keep the stash > script intact throughout the series, and to not re-introduce it after > deleting it. I did however not do that, as that would make the > range-diff quite a bit harder to read. Of course, if you start from a suboptimal ordering and reorder to make it right, the range-diff that tries to match and compare the steps will become larger and harder to follow. What else is new? That is refusing to think clearly hiding behind tautology. > In addition removing the > script bit by bit also allowed us to find the precise commit in which > the missing 'discard_cache()' bug was introduced, which made it a bit > easier to pinpoint where the bug comes from imo. In an ideal ordering, you would do a command line parser first and dispatch the rewritten commands piece by piece to the C reimplementation while diverting the remaining unwritten parts to scripted "legacy" one. That would allow us to find the precise commit that was faulty the same way. Having said all that. I do not care too deeply about seeing this particular series done in the right order anymore. Everybody's eyes are tired of looking at it, and I do not mind to see you guys declare "nobody can review this, so let's keep it in 'next' and patch breakages up as they are found out", which seems to be happening here. We can divert our review cycles to other topics that haven't faced reviewer fatigue; they still have chance to be done right ;-).
Thomas Gummerer <t.gummerer@gmail.com> writes: > As I was advocating for this series to go into 'next' without a large > refactor of this series, I'll put my money were my mouth is and try to > make the cleanups and fixes required, though without trying to avoid > further external process calls, or changing the series around too > much. Thanks for a well-written summary. One thing that is missing in the write-up is if you kept the same base (i.e. on top of eacdb4d24f) or rebased the series on top of somewhere else. I'd assume you built on the same base as before in the meantime (I'll know soon enough, when I sit down on a terminal and resume working on the code ;-)
On 03/03, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@gmail.com> writes: > > > As I was advocating for this series to go into 'next' without a large > > refactor of this series, I'll put my money were my mouth is and try to > > make the cleanups and fixes required, though without trying to avoid > > further external process calls, or changing the series around too > > much. > > Thanks for a well-written summary. One thing that is missing in the > write-up is if you kept the same base (i.e. on top of eacdb4d24f) or > rebased the series on top of somewhere else. I'd assume you built > on the same base as before in the meantime (I'll know soon enough, > when I sit down on a terminal and resume working on the code ;-) Right, I forgot mentioning that. Yes it is still based on eacdb4d24f, as there was no good reason to change that.