{"thread":{"id":"59875","subject":"[PATCH] Add 'preserve' subcommand to 'git stash'","startedAt":"2023-06-16T11:06:05Z","lastAt":"2023-06-19T01:42:16Z","messageCount":10,"participants":["Nadav Goldstein via GitGitGadget","Junio C Hamano","Oswald Buddenhagen","Nadav Goldstein"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478465","messageId":"pull.1528.git.git.1686913210137.gitgitgadget@gmail.com","threadId":"59875","inReplyTo":null,"subject":"[PATCH] Add 'preserve' subcommand to 'git stash'","fromName":"Nadav Goldstein via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-16T11:00:09Z","receivedAt":"2023-06-16T11:06:05Z","isPatch":true,"sender":{"key":"nadav.goldstein96@gmail.com","avatar":"https://gravatar.com/avatar/a7283275addeeb7e83e72cdc2cff94d804f2621e4965cf7cf680dfb118ca1bcb?d=mp&s=160"},"body":"From: Nadav Goldstein <nadav.goldstein@blazepod.co>\n\nIn this patch, we introduce a new subcommand preserve to\ngit stash. The purpose of this subcommand is to save the\ncurrent changes into the stash and then immediately re-apply\nthose changes to the working directory.\n\nImplementation-wise, this is achieved by adding a new branch\nto the conditional in the cmd_stash function, where we check\nif argv[0] is \"preserve\". If it is, we push_stash with the\nnew argument that we added to it preserve=1.\nIn all other cases we call push_stack/do_push_stack preserve=0\n\nSigned-off-by: Nadav Goldstein <nadav.goldstein96@gmail.com>\n---\n    Add 'preserve' subcommand to 'git stash'\n    \n    In this patch, we introduce a new subcommand preserve to git stash. The\n    purpose of this subcommand is to save the current changes into the stash\n    and then immediately re-apply those changes to the working directory.\n    \n    Implementation-wise, this is achieved by adding a new branch to the\n    conditional in the cmd_stash function, where we check if argv[0] is\n    \"preserve\". If it is, we push_stash with the new argument that we added\n    to it preserve=1. In all other cases we call push_stack/do_push_stack\n    preserve=0\n    \n    If the community will approve, I will modify the patch to include help\n    messages for the new subcommand\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1528%2Fnadav96%2Fstash_preserve-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1528/nadav96/stash_preserve-v1\nPull-Request: https://github.com/git/git/pull/1528\n\n builtin/stash.c | 20 +++++++++++++-------\n 1 file changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex a7e17ffe384..88abf4cc19c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1498,7 +1498,7 @@ static int create_stash(int argc, const char **argv, const char *prefix UNUSED)\n }\n \n static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int quiet,\n-\t\t\t int keep_index, int patch_mode, int include_untracked, int only_staged)\n+\t\t\t int keep_index, int patch_mode, int include_untracked, int only_staged, int preserve)\n {\n \tint ret = 0;\n \tstruct stash_info info = STASH_INFO_INIT;\n@@ -1643,7 +1643,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \t\t\t\tret = -1;\n \t\t\t\tgoto done;\n \t\t\t}\n-\t\t} else {\n+\t\t} else if (!preserve) {\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \t\t\tcp.git_cmd = 1;\n \t\t\t/* BUG: this nukes untracked files in the way */\n@@ -1709,7 +1709,7 @@ done:\n }\n \n static int push_stash(int argc, const char **argv, const char *prefix,\n-\t\t      int push_assumed)\n+\t\t      int push_assumed, int preserve)\n {\n \tint force_assume = 0;\n \tint keep_index = -1;\n@@ -1780,14 +1780,19 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n \t}\n \n \tret = do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode,\n-\t\t\t    include_untracked, only_staged);\n+\t\t\t    include_untracked, only_staged, preserve);\n \tclear_pathspec(&ps);\n \treturn ret;\n }\n \n static int push_stash_unassumed(int argc, const char **argv, const char *prefix)\n {\n-\treturn push_stash(argc, argv, prefix, 0);\n+\treturn push_stash(argc, argv, prefix, 0, 0);\n+}\n+\n+static int preserve_stash(int argc, const char **argv, const char *prefix)\n+{\n+\treturn push_stash(argc, argv, prefix, 0, 1);\n }\n \n static int save_stash(int argc, const char **argv, const char *prefix)\n@@ -1827,7 +1832,7 @@ static int save_stash(int argc, const char **argv, const char *prefix)\n \n \tmemset(&ps, 0, sizeof(ps));\n \tret = do_push_stash(&ps, stash_msg, quiet, keep_index,\n-\t\t\t    patch_mode, include_untracked, only_staged);\n+\t\t\t    patch_mode, include_untracked, only_staged, 0);\n \n \tstrbuf_release(&stash_msg_buf);\n \treturn ret;\n@@ -1850,6 +1855,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)\n \t\tOPT_SUBCOMMAND(\"store\", &fn, store_stash),\n \t\tOPT_SUBCOMMAND(\"create\", &fn, create_stash),\n \t\tOPT_SUBCOMMAND(\"push\", &fn, push_stash_unassumed),\n+\t\tOPT_SUBCOMMAND(\"preserve\", &fn, preserve_stash),\n \t\tOPT_SUBCOMMAND_F(\"save\", &fn, save_stash, PARSE_OPT_NOCOMPLETE),\n \t\tOPT_END()\n \t};\n@@ -1876,5 +1882,5 @@ int cmd_stash(int argc, const char **argv, const char *prefix)\n \t/* Assume 'stash push' */\n \tstrvec_push(&args, \"push\");\n \tstrvec_pushv(&args, argv);\n-\treturn !!push_stash(args.nr, args.v, prefix, 1);\n+\treturn !!push_stash(args.nr, args.v, prefix, 1, 0);\n }\n\nbase-commit: d7d8841f67f29e6ecbad85a11805c907d0f00d5d\n-- \ngitgitgadget\n"},{"id":"478470","messageId":"xmqqjzw3qry6.fsf@gitster.g","threadId":"59875","inReplyTo":"pull.1528.git.git.1686913210137.gitgitgadget@gmail.com","subject":"Re: [PATCH] Add 'preserve' subcommand to 'git stash'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-16T16:42:41Z","receivedAt":"2023-06-16T16:42:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nadav Goldstein via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Nadav Goldstein <nadav.goldstein@blazepod.co>\n>\n> In this patch, we introduce a new subcommand preserve to\n> git stash. The purpose of this subcommand is to save the\n> current changes into the stash and then immediately re-apply\n> those changes to the working directory.\n\nWhy a new subcommand, not a new option to \"push\"?  Adding a new\nsubcommand would mean it would be another unfamiliar thing users\nneed to learn, as opposed to a slight variation of what they are\nalready familiar with.\n\n> Implementation-wise, this is achieved by adding a new branch\n> to the conditional in the cmd_stash function, where we check\n> if argv[0] is \"preserve\". If it is, we push_stash with the\n> new argument that we added to it preserve=1.\n> In all other cases we call push_stack/do_push_stack preserve=0\n\nThe proposed log message is lacking \"why\".  What problem does it\nwant to solve?  What are the things you cannot do without the\nfeature, and what does the new feature allow you to do that you\ncouldn't do before?  On the other hand, the implementation detail,\nunless it is tricky to read from the patch, does not have to be\nrepeated here---please aim to make the patch and new code clear\nenough not to require such commentary.\n\nMore on this in \"Describe your changes well\" section in the\nDocumentation/SubmittingPatches document.\n\n> Signed-off-by: Nadav Goldstein <nadav.goldstein96@gmail.com>\n> ---\n>     Add 'preserve' subcommand to 'git stash'\n>     \n>     In this patch, we introduce a new subcommand preserve to git stash. The\n>     purpose of this subcommand is to save the current changes into the stash\n>     and then immediately re-apply those changes to the working directory.\n>     \n>     Implementation-wise, this is achieved by adding a new branch to the\n>     conditional in the cmd_stash function, where we check if argv[0] is\n>     \"preserve\". If it is, we push_stash with the new argument that we added\n>     to it preserve=1. In all other cases we call push_stack/do_push_stack\n>     preserve=0\n\nPlease do not repeat what you already said in your log message here.\nThis is a place to give additional message that would help only\nduring the review.\n\n>     If the community will approve, I will modify the patch to include help\n>     messages for the new subcommand\n\nPlease don't think this way.  If the new feature is not worth\ncompleting to document and tests for your own use, it is not worth\ncommunity's time to review or \"approve\" it.  Instead, we try to send\na patch that is already perfected, with tests and docs, in order to\nimprove the chance reviewers will understand the new feature and its\nmotivation better when they review the patch.\n\n>  static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int quiet,\n> -\t\t\t int keep_index, int patch_mode, int include_untracked, int only_staged)\n> +\t\t\t int keep_index, int patch_mode, int include_untracked, int only_staged, int preserve)\n>  {\n>  \tint ret = 0;\n>  \tstruct stash_info info = STASH_INFO_INIT;\n> @@ -1643,7 +1643,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n>  \t\t\t\tret = -1;\n>  \t\t\t\tgoto done;\n>  \t\t\t}\n> -\t\t} else {\n> +\t\t} else if (!preserve) {\n>  \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n>  \t\t\tcp.git_cmd = 1;\n>  \t\t\t/* BUG: this nukes untracked files in the way */\n\nWhile this does skip the \"reset --hard\" step,\n\n * With --include-untracked and without a pathspec, we have run \"clean\n   --force --quiet\" already, removing the untracked files that have\n   been stored in the stash.  Wasn't the objective of the change to\n   ensure that the working tree files are unaffected?\n\n * When run with a pathspec, this change has no effect, no?  We've\n   added updated files with \"add -u\" to the index, compared the\n   contents between HEAD and what was added with \"add -u\" and\n   applied the reverse of it to both the index and the working tree,\n   which amounts to reverting the changes to the paths that match\n   the pathspec.\n\n * When run with --keep-index, there is another block after this\n   part that uses \"checkout --no-overlay\" to ensure that the\n   contents of the working tree matches what is in the index.\n\n * When the outermost \"if (!(patch_mode || only_staged))\" block is\n   not entered, we'd run \"apply -R\" to revert the changes in the\n   patch, whose point is to update the working tree contents.\n\nWhat the patch does seem to fall far short of \"preserving\" in many\ncases.  In some modes, I suspect that it might not make sense to use\n\"preserve\" (for example, it could be that \"--keep-index\" may be\ntotally incompatible with \"--preserve\"), but that is hard to guess\nwithout knowing _why_ you wanted to this new feature in the first\nplace.  Adding a separate subcommand that gives users no access to\nthese modes would be an easy way to avoid having to think about\nthese issues, but may close too many doors, including ones those\nthat do not have to be closed if this feature is done as a new\noption to existing \"push\" command.  I dunno.\n\nThanks.\n"},{"id":"478475","messageId":"ZIzALOe8GBsNGIhR@ugly","threadId":"59875","inReplyTo":"xmqqjzw3qry6.fsf@gitster.g","subject":"Re: [PATCH] Add 'preserve' subcommand to 'git stash'","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-06-16T20:03:56Z","receivedAt":"2023-06-16T20:04:02Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Fri, Jun 16, 2023 at 09:42:41AM -0700, Junio C Hamano wrote:\n>\"Nadav Goldstein via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>> In this patch, we introduce a new subcommand preserve to\n>> git stash. The purpose of this subcommand is to save the\n>> current changes into the stash and then immediately re-apply\n>> those changes to the working directory.\n>\n>Why a new subcommand, not a new option to \"push\"?  Adding a new\n>subcommand would mean it would be another unfamiliar thing users\n>need to learn, as opposed to a slight variation of what they are\n>already familiar with.\n>\nto be fair, there's also `apply` and not `pop --keep`.\n\nof course, `preserve` seems a bit unspecific, but `save` and `create` \nare already taken.\n\n>>     If the community will approve, I will modify the patch to include \n>>     help\n>>     messages for the new subcommand\n>\n>Please don't think this way.  If the new feature is not worth\n>completing to document and tests for your own use, it is not worth\n>community's time to review or \"approve\" it.\n>\nfor one's own use, one usually wouldn't do the polishing.\n\n>Instead, we try to send a patch that is already perfected, with tests \n>and docs,\n>\nit's nice when \"we\" do that, but i think that this is a somewhat too \none-sided committment to *ask* for.\n\n>in order to improve the chance reviewers will understand the new \n>feature and its motivation better when they review the patch.\n>\ni think one can achieve that without doing the full monty.\nthat's what the design-driven process is for, after all. the crux is at \nwhat contribution size one considers it \"worth it\", but you can be sure \nthat drive-by contributors have a significantly lower threshold than \nregulars.\n\ni'm not saying that nadav succeeded, but a focus on final artifacts \nalone is unlikely to have changed anything.\n\nregards,\nossi\n"},{"id":"478476","messageId":"xmqqv8fnrwtt.fsf@gitster.g","threadId":"59875","inReplyTo":"ZIzALOe8GBsNGIhR@ugly","subject":"Re: [PATCH] Add 'preserve' subcommand to 'git stash'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-16T20:11:58Z","receivedAt":"2023-06-16T20:12:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:\n\n>>Why a new subcommand, not a new option to \"push\"?  Adding a new\n>>subcommand would mean it would be another unfamiliar thing users\n>>need to learn, as opposed to a slight variation of what they are\n>>already familiar with.\n>>\n> to be fair, there's also `apply` and not `pop --keep`.\n\nI do not care all that much if that is fair, but I do not think it\nis a meaningful comparison.  \"stash apply\" is merely exposing the\nfirst half (the other half is \"stash drop\") of a two step operation\nthat is \"stash pop\".\n"},{"id":"478511","messageId":"ZI1xLwemOs9Vxorf@ugly","threadId":"59875","inReplyTo":"xmqqv8fnrwtt.fsf@gitster.g","subject":"Re: [PATCH] Add 'preserve' subcommand to 'git stash'","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-06-17T08:39:11Z","receivedAt":"2023-06-17T08:39:18Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Fri, Jun 16, 2023 at 01:11:58PM -0700, Junio C Hamano wrote:\n>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:\n>\n>>>Why a new subcommand, not a new option to \"push\"?  Adding a new\n>>>subcommand would mean it would be another unfamiliar thing users\n>>>need to learn, as opposed to a slight variation of what they are\n>>>already familiar with.\n>>>\n>> to be fair, there's also `apply` and not `pop --keep`.\n>\n>I do not care all that much if that is fair, but I do not think it\n>is a meaningful comparison.  \"stash apply\" is merely exposing the\n>first half (the other half is \"stash drop\") of a two step operation\n>that is \"stash pop\".\n>\ni may be totally wrong about it (because i don't understand the \nmotivation behind this feature, either), but i think the _intent_ of \nnadav's patch is to merely expose the first half of \"stash push\" (the \nother half is the implicit \"reset --hard\"). it may not be a sufficiently \ngood one, but there is clearly an analogy here.\n\nregards,\nossi\n"},{"id":"478513","messageId":"xmqqwn02qqp4.fsf@gitster.g","threadId":"59875","inReplyTo":"ZI1xLwemOs9Vxorf@ugly","subject":"Re: [PATCH] Add 'preserve' subcommand to 'git stash'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-17T11:21:59Z","receivedAt":"2023-06-17T11:22:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:\n\n> i may be totally wrong about it (because i don't understand the\n> motivation behind this feature, either), but i think the _intent_ of\n> nadav's patch is to merely expose the first half of \"stash push\" (the\n> other half is the implicit \"reset --hard\"). it may not be a\n> sufficiently good one, but there is clearly an analogy here.\n\nI do agree that it would be reasonable to want to expose the first\nhalf (the other half is \"now the local mod got saved in a stash,\nadjust the working tree and/or the index\"), but then that means the\ncode should cover the various operating modes we have, and let the\nusers perform their first half, so that the second half (which by\nthe way needs to be exposed by another series later) can be used on\ntop of the result to emulate as if the combined two (i.e. \"stash\nsave/push\") have been run, for the feature to be complete, no?\n\nLack of the second half can be excused away with \"let's do these one\nstep at a time\", but the analogy fails to hold with an incomplete\ncoverage of even the first half, I am afraid.\n\nBut as you said, I think the lack of concrete \"here is how this\nfeature is expected to be used and why it is useful because it\nallows us to do X that we haven't been able to before\" is the\nlargest first issue in the posted patch, as that leaves reviewers\nguessing without feeling they \"understand the motivation behind\" the\nfeature.  Such an understanding would help us to tell where to stop\n(maybe in certain modes doing only the \"first half\" does not make\nsense because the corresponding \"second half\" inherently does not\nexist for some reason, in which case it is fine not to support such\na mode that is supported by \"stash push\").\n\n\n"},{"id":"478520","messageId":"f6a510f8-f569-6b1e-a74d-cdd103b39c10@gmail.com","threadId":"59875","inReplyTo":"xmqqwn02qqp4.fsf@gitster.g","subject":"Re: [PATCH] Add 'preserve' subcommand to 'git stash'","fromName":"Nadav Goldstein","fromEmail":"nadav.goldstein96@gmail.com","sentAt":"2023-06-18T09:05:21Z","receivedAt":"2023-06-18T09:05:35Z","isPatch":true,"sender":{"key":"nadav.goldstein96@gmail.com","avatar":"https://gravatar.com/avatar/a7283275addeeb7e83e72cdc2cff94d804f2621e4965cf7cf680dfb118ca1bcb?d=mp&s=160"},"body":"Hi,\nThanks for the feedback, and I totally agree I was very vague in my \ndescription, and I'm sorry for that.\n\nLet me try to explain my motivation:\nI heavily use stash to set quick points in my code so I could go back to \nthem (during thought process), and I want to store my changes quickly \nand continue from there).\n\nCurrently doing git stash discard the current working tree, so I need to \nperform git stash apply 0 to restore it, so my new sub-command is aiming \nto replace doing:\n`git stash\ngit stash apply 0`\n\nwith just `git stash preserve`\n\nRegarding using it as a flag in the stash push, I went to this direction \ninitially, but stopped because of all of the flags you mentioned \n(keep-index, include-untracked etc...), I wanted a clean slate, and to \navoid using the push flags that seems overkill in this phase (they can \nbe supported later if users requested it in forums, wanted to keep it \nsimple).\n\nIf I understand correctly, the problem is my subcommand behind the \nscenes still support the push flags because they use the same method \n(do_push_stash).\n\nDo you have any idea how to disable those flags in the new subcommand \nonly? And do you still think it should be a flag?\n\nAlso what do you think regarding the way I choose to implement it? \n(Adding the extra argument to do_push_stash)\n\nThanks,\nNadav\n\nOn 17/06/2023 14:21, Junio C Hamano wrote:\n> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:\n>\n>> i may be totally wrong about it (because i don't understand the\n>> motivation behind this feature, either), but i think the _intent_ of\n>> nadav's patch is to merely expose the first half of \"stash push\" (the\n>> other half is the implicit \"reset --hard\"). it may not be a\n>> sufficiently good one, but there is clearly an analogy here.\n> I do agree that it would be reasonable to want to expose the first\n> half (the other half is \"now the local mod got saved in a stash,\n> adjust the working tree and/or the index\"), but then that means the\n> code should cover the various operating modes we have, and let the\n> users perform their first half, so that the second half (which by\n> the way needs to be exposed by another series later) can be used on\n> top of the result to emulate as if the combined two (i.e. \"stash\n> save/push\") have been run, for the feature to be complete, no?\n>\n> Lack of the second half can be excused away with \"let's do these one\n> step at a time\", but the analogy fails to hold with an incomplete\n> coverage of even the first half, I am afraid.\n>\n> But as you said, I think the lack of concrete \"here is how this\n> feature is expected to be used and why it is useful because it\n> allows us to do X that we haven't been able to before\" is the\n> largest first issue in the posted patch, as that leaves reviewers\n> guessing without feeling they \"understand the motivation behind\" the\n> feature.  Such an understanding would help us to tell where to stop\n> (maybe in certain modes doing only the \"first half\" does not make\n> sense because the corresponding \"second half\" inherently does not\n> exist for some reason, in which case it is fine not to support such\n> a mode that is supported by \"stash push\").\n>\n>\n>\n>\n"},{"id":"478521","messageId":"ZI7SzSuT26XXKzah@ugly","threadId":"59875","inReplyTo":"f6a510f8-f569-6b1e-a74d-cdd103b39c10@gmail.com","subject":"Re: [PATCH] Add 'preserve' subcommand to 'git stash'","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-06-18T09:47:57Z","receivedAt":"2023-06-18T09:48:03Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Sun, Jun 18, 2023 at 12:05:21PM +0300, Nadav Goldstein wrote:\n>Let me try to explain my motivation:\n>I heavily use stash to set quick points in my code so I could go back to \n>them (during thought process), and I want to store my changes quickly \n>and continue from there.\n>\nso why are you (ab-)using stash for that, rather than just committing \neach time, and later cleaning it by using `reset [--mixed]` and \nre-committing (or using `rebase --interactive`)? the reflog holds \ninformation about \"lost\" commits (the stash is just a somewhat special \nreflog, too).\n\nregards,\nossi\n"},{"id":"478522","messageId":"8c3ca631-a33a-a64c-5152-249f6182a5c7@gmail.com","threadId":"59875","inReplyTo":"ZI7SzSuT26XXKzah@ugly","subject":"Re: [PATCH] Add 'preserve' subcommand to 'git stash'","fromName":"Nadav Goldstein","fromEmail":"nadav.goldstein96@gmail.com","sentAt":"2023-06-18T10:57:02Z","receivedAt":"2023-06-18T10:57:11Z","isPatch":true,"sender":{"key":"nadav.goldstein96@gmail.com","avatar":"https://gravatar.com/avatar/a7283275addeeb7e83e72cdc2cff94d804f2621e4965cf7cf680dfb118ca1bcb?d=mp&s=160"},"body":"I see your point, but commits force me to detail the changes (and I know \nit's good practice).\n\nI just want to give the user another option rather than committing their \nchanges.\n\n\nIf we talking about commits, why use stash at all? why not just commit, \npush it/create and switch branch, and go back?\n\n\nI know my argument is not so smart, but I just want to highlight that \neverything can be done with commits, as this what makes git so powerful :)\n\n\nThanks for you reply!\n\nNadav\n\n\nOn 18/06/2023 12:47, Oswald Buddenhagen wrote:\n> On Sun, Jun 18, 2023 at 12:05:21PM +0300, Nadav Goldstein wrote:\n>> Let me try to explain my motivation:\n>> I heavily use stash to set quick points in my code so I could go back \n>> to them (during thought process), and I want to store my changes \n>> quickly and continue from there.\n>>\n> so why are you (ab-)using stash for that, rather than just committing \n> each time, and later cleaning it by using `reset [--mixed]` and \n> re-committing (or using `rebase --interactive`)? the reflog holds \n> information about \"lost\" commits (the stash is just a somewhat special \n> reflog, too).\n>\n> regards,\n> ossi\n>\n>\n"},{"id":"478523","messageId":"xmqqwn00p6s2.fsf@gitster.g","threadId":"59875","inReplyTo":"f6a510f8-f569-6b1e-a74d-cdd103b39c10@gmail.com","subject":"Re: [PATCH] Add 'preserve' subcommand to 'git stash'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-19T01:42:05Z","receivedAt":"2023-06-19T01:42:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nadav Goldstein <nadav.goldstein96@gmail.com> writes:\n\n> I heavily use stash to set quick points in my code so I could go back\n> to them (during thought process), and I want to store my changes\n> quickly and continue from there).\n\nSo it is more like \"take a series of snapshots\".  I do not think\n\"stash\" is a good match for that, and I do not know how to bend\n\"stash\" to make it a good match for that purpose while not hurting\nthe original use of the \"stash\" command.\n\nSurely, the first part of \"stash create\" is a good way to take a\nsnapshot, but after creating a series of stash entries, we cannot\nuse them effectively as \"snapshot we can go back to\", and half the\nproblem lies in the application side (\"stash apply\") and the other\nhalf comes from your design to leave the local changes in the\nworking tree and the index.\n\nLet's imagine that while we are exploring, we came up with an idea,\nwrote code and did a \"git stash push && git stash apply\" (or \"git\nstash snapshot\").  We continue, and we do more modification, and do\nanother \"git stash snapshot\".  We do that again and create another\nsnapshot.  In total, on top of the HEAD that hasn't changed, we now\nhave three snapshots stash@{2}, stash@{1}, and stash@{0}.\n\nLet's further imagine that the earliest first step was good\n(i.e. the change stored in stash@{2}^..stash@{2} and the tree object\nrecorded in stash@{2}^{tree} are both good).  Also, the latest\nchange we made since we took the second snapshot (i.e. diff between\nstash@{1} and stash@{0}) is good.\n\nBut the changes made in the second part was totally misdesigned and\nbeyond salvaging.  It has to be discarded and reworked from scratch.\n\nUsing stash@{2} to go back to the first snapshot may be trivial.\nWe'd do something like\n\n    $ git reset --hard stash@{2} && git reset --soft HEAD^\n\nBut how would we salvage the latest part of the work on top of this\nstate, while excluding the crap we made before we took the second\nsnapshot?  Mostly because the main motivation behind \"git stash\" is\nto preserve the change between HEAD and the current state while\npreparing a clean slate to work on something totally unrelated for\nus, its application side is geared towards applying the preserved\nchange (i.e. not the tree state of the stash entry, but the\ndifferences between the tree state of the parent of the stash entry\nand the tree state of the stash entry).  Asking \"please apply the\ndifference recorded in the stash@{0}\" would give us changes we made\nwhile taking all three snapshots, which is bad for two reasons, (1)\nbecause we have already recovered the changes we made before taking\nthe first snapshot above but stash@{0} contains that change\nredundantly, and (2) stash@{0} also contains the change we made\nbetween the first and the second snapshot, the crappy one we want to\ndiscard.  We end up doing something unwierdy like this, perhaps?\n\n    $ git diff stash@{1} stash@{0} | git apply\n\nBesides, new users will be utterly confused if they realize that\nstash@{0} would contain changes made in the all three phases and\ntheir \"stash pop/apply\" on it would resurrect all of them.  So we\nneed to educate users with \"stash entries created with 'stash\npreserve' cannot be applied with 'stash pop' without care\".\n\nIf we instead used normal commits during our exploring phase of the\ndevelopment, this would have been much easier and cleaner:\n\n    $ work work initial work\n    $ git commit -a -m 'snapshot #1'\n    $ work work more work\n    $ git commit -a -m 'snapshot #2'\n    $ work work great work\n    $ git commit -a -m 'snapshot #3'\n\nWe only discover that earlier work was crap after trying to build on\ntop and the end result does not work well.  So the \"great work\" part\nmay have been very good, \"initial work\" part may be OK, but the\nother part, i.e. \"more work\", while it may have looked like a good\nidea, turns out to be unusable.  Fortunately, how to clean up such a\nhistory is well established.  With \"git rebase -i HEAD~3\", we can\neasily discard the \"more work\" part and preserve both \"initial\" and\n\"great\" work in the result.\n\nIn short, we do not have to abuse stash to make a poor imitation of\nalready well understood workflow, and even if it were a good idea,\nthe \"stash preserve\" proposed in the thread would not be a good\ningredient to build something like that, because of its design,\ni.e. the snapshots are not incremental and it is always relative to\nthe unmoving HEAD, hence recording older changes redundantly.\n\n> Regarding using it as a flag in the stash push, I went to this\n> direction initially, but stopped because of all of the flags you\n> mentioned (keep-index, include-untracked etc...),...\n\nAs shown above, \"stash preserve\" is not a good alternative to\n\"commits then rebase\".  In short, if you create N snapshots in a\nrow, if your changes between M-th snapshot and M+1-th snapshot were\nso bad that they need to be discarded, it would make all of your\nlater snapshot hard to salvage.\n\nNow let's see if we can salvage it for single-use snapshot that\ncannot be combined.  The limitation is that you can only go back to\nthe state of a single stash entry, as if running \"reset --hard\".\nWould the \"stash preserve\" with such a limited application still be\nuseful?  Even then lack of \"include untracked\" would probably be a\nshow-stopper for those people who create new files while \"during\nthought process\", and here is why.\n\n    $ work work work\n    $ modify files A, B, C, D\n    $ create new file E, F\n    $ git stash snapshot\n    $ work work more work\n    $ git stash snapshot\n    $ work work even more work\n    $ git stash snapshot\n    $ work work even more work\n    $ ...\n\nNow, if realize that these later work after the snapshot were all\nbad, can we go back to the initial snapshot?\n\nIf you do not allow recording what was in the untracked files, the\ncontents of new files E and F may have been modified while you were\nbutchering your somewhat good initial step you recorded in your\nfirst snapshot.\n\nNot allowing pathspec would have similar issues.  While doing\nexploratory programming, you may realize that changes you made to\nsome of the files are in good shape and worth including in a\nsnapshot while the state of other files are no better than what you\nhad in your previous snapshot, and you would want to be able to use\n\"git stash snapshot -- <pathspec>\" to make the resulting snapshot\nusable.\n\nSo whether it is done as a separate command \"git stash preserve\", or\nit is done as an option to \"git stash push --snapshot-only\", the\nusers will need access to (some of) the options they expect to be\nable to use the usual \"stash push\".  It may not have to support all\nthe options and operating modes from day one, but if it will have to\neventually learn many of them when it becomes complete, I do not see\na strong reason to have it as a separate command.\n\nIt is fine if some of the options are inherently incompatible with\n\"--snapshot-only\" mode.  There are precedence in the code you can\nfind and mimic, like \"--patch\" and \"--include-untracked\" options\nare marked to be incompatible.\n\nBut as I already said, I am not sure if it is worth pursuing this\nroute, as \"commits then rebase\" is well understood and established\nworkflow to help us during our exploratory programming use case.\n\nThanks.\n\n"}]}