Re: [PATCH v4 31/45] rebase: trivial cleanup
Fredrik Gustafsson iv...@iveqy.com writes: Here we have two sh-scripts (git rebase and git am) interacting with each other. Both uses GIT_QUIET, Correct. so if GIT_QUIET already is set by the caller (git rebase) the callee doesn't have to set it to. Incorrect. git rebase invokes git am, not dot-sources it, so it does not propagate. Both do dot-source git-sh-setup, which clears GIT_QUIET pretty early, so even GIT_QUIET exported into the environment by mistake will not affect them. However GIT_QUIET is undocumented for git-am (in Git Manual). If we translate git am to C/ruby/python/perl/etc. will we catch this? I think it is irrelevant. git-am uses many shell variables, e.g. HAS_HEAD, interactive, abort_safety, etc. but they are implementation details of the program and there is no point documenting it, and somebody rewriting it in C does not have to stick to the same name. What may deserve to be documented in Documentation/technical/ somewhere is the way how some shell variables (OPTIONS_SPEC, OPTIONS_KEEPDASHDASH, LONG_USAGE, USAGE, GIT_REFLOG_ACTION, etc.) are used in git-sh-setup, which is a collection of helper shell functions for use in scripted Porcelains that dot-source it. It allows the scripted Porcelains to say things like: die message say message and the latter pays attention to GIT_QUIET. This raises a few more generall questions: do we already pass information between processes(!) with enviroment variables? And is this documented the way it should be? There are a few used as implementation details, I think. GIT_QUIET is not among them. -- 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 v4 31/45] rebase: trivial cleanup
On Tue, Jun 11, 2013 at 12:09:32PM -0500, Felipe Contreras wrote: It's not removed. It's simply moved. Sorry about that, I wasn't paying enough attention. But why are you moving it? All other arguments to git am is set in git-rebase.sh, why just set -q just before the invokation in git-rebase--am.sh? -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- 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 v4 31/45] rebase: trivial cleanup
On Tue, Jun 11, 2013 at 12:24 PM, Fredrik Gustafsson iv...@iveqy.com wrote: On Tue, Jun 11, 2013 at 12:09:32PM -0500, Felipe Contreras wrote: It's not removed. It's simply moved. Sorry about that, I wasn't paying enough attention. But why are you moving it? All other arguments to git am is set in git-rebase.sh, why just set -q just before the invokation in git-rebase--am.sh? Because the next patch checks if there's any arguments meant for 'git am' to switch to am rebase mode. We shouldn't switch to that mode if the only argument to 'git am' is going to be -q. -- Felipe Contreras -- 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 v4 31/45] rebase: trivial cleanup
On Tue, Jun 11, 2013 at 12:26:42PM -0500, Felipe Contreras wrote: On Tue, Jun 11, 2013 at 12:24 PM, Fredrik Gustafsson iv...@iveqy.com wrote: On Tue, Jun 11, 2013 at 12:09:32PM -0500, Felipe Contreras wrote: It's not removed. It's simply moved. Sorry about that, I wasn't paying enough attention. But why are you moving it? All other arguments to git am is set in git-rebase.sh, why just set -q just before the invokation in git-rebase--am.sh? Because the next patch checks if there's any arguments meant for 'git am' to switch to am rebase mode. We shouldn't switch to that mode if the only argument to 'git am' is going to be -q. Okay, that make sense. How about rephrase the commit message and add this explanation. It's really not a cleanup but a preparation for the next patch. If I was a maintainer and only got this patch I would reject it. Every patch in a patch serie should be justified to be applied as a single patch, yes? -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- 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 v4 31/45] rebase: trivial cleanup
On Tue, Jun 11, 2013 at 12:41 PM, Fredrik Gustafsson iv...@iveqy.com wrote: On Tue, Jun 11, 2013 at 12:26:42PM -0500, Felipe Contreras wrote: On Tue, Jun 11, 2013 at 12:24 PM, Fredrik Gustafsson iv...@iveqy.com wrote: On Tue, Jun 11, 2013 at 12:09:32PM -0500, Felipe Contreras wrote: It's not removed. It's simply moved. Sorry about that, I wasn't paying enough attention. But why are you moving it? All other arguments to git am is set in git-rebase.sh, why just set -q just before the invokation in git-rebase--am.sh? Because the next patch checks if there's any arguments meant for 'git am' to switch to am rebase mode. We shouldn't switch to that mode if the only argument to 'git am' is going to be -q. Okay, that make sense. How about rephrase the commit message and add this explanation. It's really not a cleanup but a preparation for the next patch. If I was a maintainer and only got this patch I would reject it. Every patch in a patch serie should be justified to be applied as a single patch, yes? Yes, but it doesn't matter, because the maintainer has made it clear he is not going to even read this patch series. -- Felipe Contreras -- 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
[PATCH v4 31/45] rebase: trivial cleanup
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- git-rebase--am.sh | 1 + git-rebase.sh | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/git-rebase--am.sh b/git-rebase--am.sh index 6460028..2ce7570 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -51,6 +51,7 @@ then return $? fi +test -n $GIT_QUIET git_am_opt=$git_am_opt -q git am $git_am_opt --rebasing --resolvemsg=$resolvemsg $GIT_DIR/rebased-patches ret=$? diff --git a/git-rebase.sh b/git-rebase.sh index 0937e2c..6be247d 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -285,7 +285,6 @@ do ;; -q) GIT_QUIET=t - git_am_opt=$git_am_opt -q verbose= diffstat= ;; -- 1.8.3.698.g079b096 -- 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 v4 31/45] rebase: trivial cleanup
On Sun, Jun 09, 2013 at 11:40:43AM -0500, Felipe Contreras wrote: - git_am_opt=$git_am_opt -q This one makes me wonder a bit. I'm not sure about how this works in git, so I would appriciate if someone can explain. I might also have misunderstood something. Here we have two sh-scripts (git rebase and git am) interacting witch eachother. Both uses GIT_QUIET, so if GIT_QUIET already is set by the caller (git rebase) the callee doesn't have to set it to. However GIT_QUIET is undocumented for git-am (in Git Manual). If we translate git am to C/ruby/python/perl/etc. will we catch this? This raises a few more generall questions: do we already pass information between processes(!) with enviroment variables? And is this documented the way it should be? -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- 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
