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@xxxxxx> 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").
