| Message ID | pull.1528.git.git.1686913210137.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add 'preserve' subcommand to 'git stash' | expand |
"Nadav Goldstein via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Nadav Goldstein <nadav.goldstein@blazepod.co> > > In this patch, we introduce a new subcommand preserve to > git stash. The purpose of this subcommand is to save the > current changes into the stash and then immediately re-apply > those changes to the working directory. Why a new subcommand, not a new option to "push"? Adding a new subcommand would mean it would be another unfamiliar thing users need to learn, as opposed to a slight variation of what they are already familiar with. > Implementation-wise, this is achieved by adding a new branch > to the conditional in the cmd_stash function, where we check > if argv[0] is "preserve". If it is, we push_stash with the > new argument that we added to it preserve=1. > In all other cases we call push_stack/do_push_stack preserve=0 The proposed log message is lacking "why". What problem does it want to solve? What are the things you cannot do without the feature, and what does the new feature allow you to do that you couldn't do before? On the other hand, the implementation detail, unless it is tricky to read from the patch, does not have to be repeated here---please aim to make the patch and new code clear enough not to require such commentary. More on this in "Describe your changes well" section in the Documentation/SubmittingPatches document. > Signed-off-by: Nadav Goldstein <nadav.goldstein96@gmail.com> > --- > Add 'preserve' subcommand to 'git stash' > > In this patch, we introduce a new subcommand preserve to git stash. The > purpose of this subcommand is to save the current changes into the stash > and then immediately re-apply those changes to the working directory. > > Implementation-wise, this is achieved by adding a new branch to the > conditional in the cmd_stash function, where we check if argv[0] is > "preserve". If it is, we push_stash with the new argument that we added > to it preserve=1. In all other cases we call push_stack/do_push_stack > preserve=0 Please do not repeat what you already said in your log message here. This is a place to give additional message that would help only during the review. > If the community will approve, I will modify the patch to include help > messages for the new subcommand Please don't think this way. If the new feature is not worth completing to document and tests for your own use, it is not worth community's time to review or "approve" it. Instead, we try to send a patch that is already perfected, with tests and docs, in order to improve the chance reviewers will understand the new feature and its motivation better when they review the patch. > static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int quiet, > - int keep_index, int patch_mode, int include_untracked, int only_staged) > + int keep_index, int patch_mode, int include_untracked, int only_staged, int preserve) > { > int ret = 0; > struct stash_info info = STASH_INFO_INIT; > @@ -1643,7 +1643,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q > ret = -1; > goto done; > } > - } else { > + } else if (!preserve) { > struct child_process cp = CHILD_PROCESS_INIT; > cp.git_cmd = 1; > /* BUG: this nukes untracked files in the way */ While this does skip the "reset --hard" step, * With --include-untracked and without a pathspec, we have run "clean --force --quiet" already, removing the untracked files that have been stored in the stash. Wasn't the objective of the change to ensure that the working tree files are unaffected? * When run with a pathspec, this change has no effect, no? We've added updated files with "add -u" to the index, compared the contents between HEAD and what was added with "add -u" and applied the reverse of it to both the index and the working tree, which amounts to reverting the changes to the paths that match the pathspec. * When run with --keep-index, there is another block after this part that uses "checkout --no-overlay" to ensure that the contents of the working tree matches what is in the index. * When the outermost "if (!(patch_mode || only_staged))" block is not entered, we'd run "apply -R" to revert the changes in the patch, whose point is to update the working tree contents. What the patch does seem to fall far short of "preserving" in many cases. In some modes, I suspect that it might not make sense to use "preserve" (for example, it could be that "--keep-index" may be totally incompatible with "--preserve"), but that is hard to guess without knowing _why_ you wanted to this new feature in the first place. Adding a separate subcommand that gives users no access to these modes would be an easy way to avoid having to think about these issues, but may close too many doors, including ones those that do not have to be closed if this feature is done as a new option to existing "push" command. I dunno. Thanks.
On Fri, Jun 16, 2023 at 09:42:41AM -0700, Junio C Hamano wrote: >"Nadav Goldstein via GitGitGadget" <gitgitgadget@gmail.com> writes: >> In this patch, we introduce a new subcommand preserve to >> git stash. The purpose of this subcommand is to save the >> current changes into the stash and then immediately re-apply >> those changes to the working directory. > >Why a new subcommand, not a new option to "push"? Adding a new >subcommand would mean it would be another unfamiliar thing users >need to learn, as opposed to a slight variation of what they are >already familiar with. > to be fair, there's also `apply` and not `pop --keep`. of course, `preserve` seems a bit unspecific, but `save` and `create` are already taken. >> If the community will approve, I will modify the patch to include >> help >> messages for the new subcommand > >Please don't think this way. If the new feature is not worth >completing to document and tests for your own use, it is not worth >community's time to review or "approve" it. > for one's own use, one usually wouldn't do the polishing. >Instead, we try to send a patch that is already perfected, with tests >and docs, > it's nice when "we" do that, but i think that this is a somewhat too one-sided committment to *ask* for. >in order to improve the chance reviewers will understand the new >feature and its motivation better when they review the patch. > i think one can achieve that without doing the full monty. that's what the design-driven process is for, after all. the crux is at what contribution size one considers it "worth it", but you can be sure that drive-by contributors have a significantly lower threshold than regulars. i'm not saying that nadav succeeded, but a focus on final artifacts alone is unlikely to have changed anything. regards, ossi
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: >>Why a new subcommand, not a new option to "push"? Adding a new >>subcommand would mean it would be another unfamiliar thing users >>need to learn, as opposed to a slight variation of what they are >>already familiar with. >> > to be fair, there's also `apply` and not `pop --keep`. I do not care all that much if that is fair, but I do not think it is a meaningful comparison. "stash apply" is merely exposing the first half (the other half is "stash drop") of a two step operation that is "stash pop".
On Fri, Jun 16, 2023 at 01:11:58PM -0700, Junio C Hamano wrote: >Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > >>>Why a new subcommand, not a new option to "push"? Adding a new >>>subcommand would mean it would be another unfamiliar thing users >>>need to learn, as opposed to a slight variation of what they are >>>already familiar with. >>> >> to be fair, there's also `apply` and not `pop --keep`. > >I do not care all that much if that is fair, but I do not think it >is a meaningful comparison. "stash apply" is merely exposing the >first half (the other half is "stash drop") of a two step operation >that is "stash pop". > i may be totally wrong about it (because i don't understand the motivation behind this feature, either), but i think the _intent_ of nadav's patch is to merely expose the first half of "stash push" (the other half is the implicit "reset --hard"). it may not be a sufficiently good one, but there is clearly an analogy here. regards, ossi
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > i may be totally wrong about it (because i don't understand the > motivation behind this feature, either), but i think the _intent_ of > nadav's patch is to merely expose the first half of "stash push" (the > other half is the implicit "reset --hard"). it may not be a > sufficiently good one, but there is clearly an analogy here. I do agree that it would be reasonable to want to expose the first half (the other half is "now the local mod got saved in a stash, adjust the working tree and/or the index"), but then that means the code should cover the various operating modes we have, and let the users perform their first half, so that the second half (which by the way needs to be exposed by another series later) can be used on top of the result to emulate as if the combined two (i.e. "stash save/push") have been run, for the feature to be complete, no? Lack of the second half can be excused away with "let's do these one step at a time", but the analogy fails to hold with an incomplete coverage of even the first half, I am afraid. But as you said, I think the lack of concrete "here is how this feature is expected to be used and why it is useful because it allows us to do X that we haven't been able to before" is the largest first issue in the posted patch, as that leaves reviewers guessing without feeling they "understand the motivation behind" the feature. Such an understanding would help us to tell where to stop (maybe in certain modes doing only the "first half" does not make sense because the corresponding "second half" inherently does not exist for some reason, in which case it is fine not to support such a mode that is supported by "stash push").
Hi, Thanks for the feedback, and I totally agree I was very vague in my description, and I'm sorry for that. Let me try to explain my motivation: I heavily use stash to set quick points in my code so I could go back to them (during thought process), and I want to store my changes quickly and continue from there). Currently doing git stash discard the current working tree, so I need to perform git stash apply 0 to restore it, so my new sub-command is aiming to replace doing: `git stash git stash apply 0` with just `git stash preserve` Regarding using it as a flag in the stash push, I went to this direction initially, but stopped because of all of the flags you mentioned (keep-index, include-untracked etc...), I wanted a clean slate, and to avoid using the push flags that seems overkill in this phase (they can be supported later if users requested it in forums, wanted to keep it simple). If I understand correctly, the problem is my subcommand behind the scenes still support the push flags because they use the same method (do_push_stash). Do you have any idea how to disable those flags in the new subcommand only? And do you still think it should be a flag? Also what do you think regarding the way I choose to implement it? (Adding the extra argument to do_push_stash) Thanks, Nadav On 17/06/2023 14:21, Junio C Hamano wrote: > Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > >> i may be totally wrong about it (because i don't understand the >> motivation behind this feature, either), but i think the _intent_ of >> nadav's patch is to merely expose the first half of "stash push" (the >> other half is the implicit "reset --hard"). it may not be a >> sufficiently good one, but there is clearly an analogy here. > I do agree that it would be reasonable to want to expose the first > half (the other half is "now the local mod got saved in a stash, > adjust the working tree and/or the index"), but then that means the > code should cover the various operating modes we have, and let the > users perform their first half, so that the second half (which by > the way needs to be exposed by another series later) can be used on > top of the result to emulate as if the combined two (i.e. "stash > save/push") have been run, for the feature to be complete, no? > > Lack of the second half can be excused away with "let's do these one > step at a time", but the analogy fails to hold with an incomplete > coverage of even the first half, I am afraid. > > But as you said, I think the lack of concrete "here is how this > feature is expected to be used and why it is useful because it > allows us to do X that we haven't been able to before" is the > largest first issue in the posted patch, as that leaves reviewers > guessing without feeling they "understand the motivation behind" the > feature. Such an understanding would help us to tell where to stop > (maybe in certain modes doing only the "first half" does not make > sense because the corresponding "second half" inherently does not > exist for some reason, in which case it is fine not to support such > a mode that is supported by "stash push"). > > > >
On Sun, Jun 18, 2023 at 12:05:21PM +0300, Nadav Goldstein wrote: >Let me try to explain my motivation: >I heavily use stash to set quick points in my code so I could go back to >them (during thought process), and I want to store my changes quickly >and continue from there. > so why are you (ab-)using stash for that, rather than just committing each time, and later cleaning it by using `reset [--mixed]` and re-committing (or using `rebase --interactive`)? the reflog holds information about "lost" commits (the stash is just a somewhat special reflog, too). regards, ossi
I see your point, but commits force me to detail the changes (and I know it's good practice). I just want to give the user another option rather than committing their changes. If we talking about commits, why use stash at all? why not just commit, push it/create and switch branch, and go back? I know my argument is not so smart, but I just want to highlight that everything can be done with commits, as this what makes git so powerful :) Thanks for you reply! Nadav On 18/06/2023 12:47, Oswald Buddenhagen wrote: > On Sun, Jun 18, 2023 at 12:05:21PM +0300, Nadav Goldstein wrote: >> Let me try to explain my motivation: >> I heavily use stash to set quick points in my code so I could go back >> to them (during thought process), and I want to store my changes >> quickly and continue from there. >> > so why are you (ab-)using stash for that, rather than just committing > each time, and later cleaning it by using `reset [--mixed]` and > re-committing (or using `rebase --interactive`)? the reflog holds > information about "lost" commits (the stash is just a somewhat special > reflog, too). > > regards, > ossi > >
Nadav Goldstein <nadav.goldstein96@gmail.com> writes: > I heavily use stash to set quick points in my code so I could go back > to them (during thought process), and I want to store my changes > quickly and continue from there). So it is more like "take a series of snapshots". I do not think "stash" is a good match for that, and I do not know how to bend "stash" to make it a good match for that purpose while not hurting the original use of the "stash" command. Surely, the first part of "stash create" is a good way to take a snapshot, but after creating a series of stash entries, we cannot use them effectively as "snapshot we can go back to", and half the problem lies in the application side ("stash apply") and the other half comes from your design to leave the local changes in the working tree and the index. Let's imagine that while we are exploring, we came up with an idea, wrote code and did a "git stash push && git stash apply" (or "git stash snapshot"). We continue, and we do more modification, and do another "git stash snapshot". We do that again and create another snapshot. In total, on top of the HEAD that hasn't changed, we now have three snapshots stash@{2}, stash@{1}, and stash@{0}. Let's further imagine that the earliest first step was good (i.e. the change stored in stash@{2}^..stash@{2} and the tree object recorded in stash@{2}^{tree} are both good). Also, the latest change we made since we took the second snapshot (i.e. diff between stash@{1} and stash@{0}) is good. But the changes made in the second part was totally misdesigned and beyond salvaging. It has to be discarded and reworked from scratch. Using stash@{2} to go back to the first snapshot may be trivial. We'd do something like $ git reset --hard stash@{2} && git reset --soft HEAD^ But how would we salvage the latest part of the work on top of this state, while excluding the crap we made before we took the second snapshot? Mostly because the main motivation behind "git stash" is to preserve the change between HEAD and the current state while preparing a clean slate to work on something totally unrelated for us, its application side is geared towards applying the preserved change (i.e. not the tree state of the stash entry, but the differences between the tree state of the parent of the stash entry and the tree state of the stash entry). Asking "please apply the difference recorded in the stash@{0}" would give us changes we made while taking all three snapshots, which is bad for two reasons, (1) because we have already recovered the changes we made before taking the first snapshot above but stash@{0} contains that change redundantly, and (2) stash@{0} also contains the change we made between the first and the second snapshot, the crappy one we want to discard. We end up doing something unwierdy like this, perhaps? $ git diff stash@{1} stash@{0} | git apply Besides, new users will be utterly confused if they realize that stash@{0} would contain changes made in the all three phases and their "stash pop/apply" on it would resurrect all of them. So we need to educate users with "stash entries created with 'stash preserve' cannot be applied with 'stash pop' without care". If we instead used normal commits during our exploring phase of the development, this would have been much easier and cleaner: $ work work initial work $ git commit -a -m 'snapshot #1' $ work work more work $ git commit -a -m 'snapshot #2' $ work work great work $ git commit -a -m 'snapshot #3' We only discover that earlier work was crap after trying to build on top and the end result does not work well. So the "great work" part may have been very good, "initial work" part may be OK, but the other part, i.e. "more work", while it may have looked like a good idea, turns out to be unusable. Fortunately, how to clean up such a history is well established. With "git rebase -i HEAD~3", we can easily discard the "more work" part and preserve both "initial" and "great" work in the result. In short, we do not have to abuse stash to make a poor imitation of already well understood workflow, and even if it were a good idea, the "stash preserve" proposed in the thread would not be a good ingredient to build something like that, because of its design, i.e. the snapshots are not incremental and it is always relative to the unmoving HEAD, hence recording older changes redundantly. > Regarding using it as a flag in the stash push, I went to this > direction initially, but stopped because of all of the flags you > mentioned (keep-index, include-untracked etc...),... As shown above, "stash preserve" is not a good alternative to "commits then rebase". In short, if you create N snapshots in a row, if your changes between M-th snapshot and M+1-th snapshot were so bad that they need to be discarded, it would make all of your later snapshot hard to salvage. Now let's see if we can salvage it for single-use snapshot that cannot be combined. The limitation is that you can only go back to the state of a single stash entry, as if running "reset --hard". Would the "stash preserve" with such a limited application still be useful? Even then lack of "include untracked" would probably be a show-stopper for those people who create new files while "during thought process", and here is why. $ work work work $ modify files A, B, C, D $ create new file E, F $ git stash snapshot $ work work more work $ git stash snapshot $ work work even more work $ git stash snapshot $ work work even more work $ ... Now, if realize that these later work after the snapshot were all bad, can we go back to the initial snapshot? If you do not allow recording what was in the untracked files, the contents of new files E and F may have been modified while you were butchering your somewhat good initial step you recorded in your first snapshot. Not allowing pathspec would have similar issues. While doing exploratory programming, you may realize that changes you made to some of the files are in good shape and worth including in a snapshot while the state of other files are no better than what you had in your previous snapshot, and you would want to be able to use "git stash snapshot -- <pathspec>" to make the resulting snapshot usable. So whether it is done as a separate command "git stash preserve", or it is done as an option to "git stash push --snapshot-only", the users will need access to (some of) the options they expect to be able to use the usual "stash push". It may not have to support all the options and operating modes from day one, but if it will have to eventually learn many of them when it becomes complete, I do not see a strong reason to have it as a separate command. It is fine if some of the options are inherently incompatible with "--snapshot-only" mode. There are precedence in the code you can find and mimic, like "--patch" and "--include-untracked" options are marked to be incompatible. But as I already said, I am not sure if it is worth pursuing this route, as "commits then rebase" is well understood and established workflow to help us during our exploratory programming use case. Thanks.
diff --git a/builtin/stash.c b/builtin/stash.c index a7e17ffe384..88abf4cc19c 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1498,7 +1498,7 @@ static int create_stash(int argc, const char **argv, const char *prefix UNUSED) } static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int quiet, - int keep_index, int patch_mode, int include_untracked, int only_staged) + int keep_index, int patch_mode, int include_untracked, int only_staged, int preserve) { int ret = 0; struct stash_info info = STASH_INFO_INIT; @@ -1643,7 +1643,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q ret = -1; goto done; } - } else { + } else if (!preserve) { struct child_process cp = CHILD_PROCESS_INIT; cp.git_cmd = 1; /* BUG: this nukes untracked files in the way */ @@ -1709,7 +1709,7 @@ done: } static int push_stash(int argc, const char **argv, const char *prefix, - int push_assumed) + int push_assumed, int preserve) { int force_assume = 0; int keep_index = -1; @@ -1780,14 +1780,19 @@ static int push_stash(int argc, const char **argv, const char *prefix, } ret = do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode, - include_untracked, only_staged); + include_untracked, only_staged, preserve); clear_pathspec(&ps); return ret; } static int push_stash_unassumed(int argc, const char **argv, const char *prefix) { - return push_stash(argc, argv, prefix, 0); + return push_stash(argc, argv, prefix, 0, 0); +} + +static int preserve_stash(int argc, const char **argv, const char *prefix) +{ + return push_stash(argc, argv, prefix, 0, 1); } static int save_stash(int argc, const char **argv, const char *prefix) @@ -1827,7 +1832,7 @@ static int save_stash(int argc, const char **argv, const char *prefix) memset(&ps, 0, sizeof(ps)); ret = do_push_stash(&ps, stash_msg, quiet, keep_index, - patch_mode, include_untracked, only_staged); + patch_mode, include_untracked, only_staged, 0); strbuf_release(&stash_msg_buf); return ret; @@ -1850,6 +1855,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix) OPT_SUBCOMMAND("store", &fn, store_stash), OPT_SUBCOMMAND("create", &fn, create_stash), OPT_SUBCOMMAND("push", &fn, push_stash_unassumed), + OPT_SUBCOMMAND("preserve", &fn, preserve_stash), OPT_SUBCOMMAND_F("save", &fn, save_stash, PARSE_OPT_NOCOMPLETE), OPT_END() }; @@ -1876,5 +1882,5 @@ int cmd_stash(int argc, const char **argv, const char *prefix) /* Assume 'stash push' */ strvec_push(&args, "push"); strvec_pushv(&args, argv); - return !!push_stash(args.nr, args.v, prefix, 1); + return !!push_stash(args.nr, args.v, prefix, 1, 0); }