Re: [PATCH 4/6] stash: introduce 'git stash store'
Ramkumar Ramachandra artag...@gmail.com writes: diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt index 05e462b..e58ab74 100644 --- a/Documentation/git-stash.txt +++ b/Documentation/git-stash.txt @@ -17,6 +17,7 @@ SYNOPSIS [-u|--include-untracked] [-a|--all] [message]] 'git stash' clear 'git stash' create [message [include-untracked-p]] +'git stash' store commit message Two points: - We should not advertise store (and create for that matter) in the end-user facing documentation. IIRC, git stash -h deliberately omits 'create'; having it in the documentation is unavoidable, but it was a mistake that it was not marked with this is most likely not what you want to use; see 'save'. It may even be better with a leading underscore or two in the name that clearly marks it as not meant for direct end-user consumption. - The error message store_stash() gives should not be hardcoded in that function. Save-stash may want to keep saying 'the current status' as it said before, but a caller like your rebase-autostash will not be saving the current status and would want to have a different message. Other than that, the overfall structure of the patch looks OK to me. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] stash: introduce 'git stash store'
Junio C Hamano wrote: - The error message store_stash() gives should not be hardcoded in that function. Save-stash may want to keep saying 'the current status' as it said before, but a caller like your rebase-autostash will not be saving the current status and would want to have a different message. Makes sense. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] stash: introduce 'git stash store'
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: - The error message store_stash() gives should not be hardcoded in that function. Save-stash may want to keep saying 'the current status' as it said before, but a caller like your rebase-autostash will not be saving the current status and would want to have a different message. Makes sense. I think it is fine to have a default message to be used when store_stash shell function is not given an optional error message. The command line for it may become: git stash store [-m message] [-e error message] $stash_sha1 or something like that. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] stash: introduce 'git stash store'
Junio C Hamano wrote: git stash store [-m message] [-e error message] $stash_sha1 1. The message is NOT optional. If the user says 'git stash store b8bb3fe9', what default message can we possibly put in? There is absolutely no context: no branch name, nothing. So, the best we can do is generic WIP. What is the point of putting in such a useless message? 2. We have already determined that the command is NOT for end user interactive use. So, why do we need a default error message at all? The last statement is an update-ref, and you can infer whether it succeeded or failed from the exit status. 3. Why are we designing a command-line interface? git stash store $stash_sha1 $message is sufficient for scripts, and there is absolutely no point in parsing '-m', '-e', or any such thing. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] stash: introduce 'git stash store'
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: git stash store [-m message] [-e error message] $stash_sha1 ... 3. Why are we designing a command-line interface? git stash store $stash_sha1 $message is sufficient for scripts, and there is absolutely no point in parsing '-m', '-e', or any such thing. git stash store $stash_sha1 $message [ $error_message ] is adequate an internal API _for now_. I however suspect that you would regret later when you need more customization. It already happened once for git merge when it was an internal API for git pull and it was painful to support saner interface and the traditional one at the same time [*1*]. [Footnote] *1* And no, don't even try to rewrite git merge call inside git pull to use the modern style with -m message; you will likely break it (I've tried once and decided it was not worth the hassle). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] stash: introduce 'git stash store'
Junio C Hamano wrote: I however suspect that you would regret later when you need more customization. It already happened once for git merge when it was an internal API for git pull and it was painful to support saner interface and the traditional one at the same time [*1*]. Oh god. git-merge --stat --progress $merge_name HEAD 04c5b83c46760573 We made a design mistake at the command-level in merge. This is at a subcommand-level. 1. Will git stash store ever be more than a one-liner? Can you think of how this function could be larger? 2. Will git stash store ever become an interactive command? Isn't the whole point of interactive stash something that operates on a worktree? Why will I ever want to operate on a commit with stash, interactively? While it is absolutely necessary to avoid calamities like the merge invocation in git-pull.sh, we shouldn't be over-engineering either. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] stash: introduce 'git stash store'
Junio C Hamano wrote: *1* And no, don't even try to rewrite git merge call inside git pull to use the modern style with -m message; you will likely break it (I've tried once and decided it was not worth the hassle). This falls in my basket of nice theoretical exercise: a lot of work for no tangible benefit ;) Footnote: I'm not saying that code is not important; you've seen me arguing for beautiful implementations several times before, against all odds. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] stash: introduce 'git stash store'
Junio C Hamano wrote: That is not a valid question. I was just asking to see if you could think of something. I just did: named stashes (each one has a different ref/ reflog) for internal use. Sure, we'll go with the -m -e approach. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] stash: introduce 'git stash store'
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: That is not a valid question. I was just asking to see if you could think of something. I just did: named stashes (each one has a different ref/ reflog) for internal use. Sure, we'll go with the -m -e approach. The whole point of my response is that it is not a valid approach to decide to add (or not to add) a reasonable enhancement mechanism built in from the beginning by asking what future enhancement do you foresee today?. It is unclear if you got that point. As long as the end result is to have something that does not paint us into a corner from which it will be painful to escape, I'll be happy anyway. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] stash: introduce 'git stash store'
Junio C Hamano wrote: The whole point of my response is that it is not a valid approach to decide to add (or not to add) a reasonable enhancement mechanism built in from the beginning by asking what future enhancement do you foresee today?. It is unclear if you got that point. Right, got it. The fact that we weren't able to foresee the merge UI problem tells us that we're capable of repeating the mistake no matter how much we think we've thought about it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
