Re: [PATCH v1 07/19] rebase -i: log the replay of root commits
On Mon, Aug 04, 2014 at 11:21:41PM +0200, Fabian Ruch wrote: Thanks for laying out the differences in the user visible output. With stock git we are seeing summaries for other commits, but not root commits, _with the --verbose option_. It's the fault of my patch to show the summary even in non-verbose mode. This is now fixed by wrapping the relevant command in 'output', a shell function defined in git-rebase.sh as follows: Ah, OK. That makes a lot more sense, then. output=$($@ 21 ) status=$? test $status != 0 printf %s\n $output return $status The problem that git-rebase--interactive has is that the redirection of stdin to a variable (or a file) does not work reliably with commands that invoke the log message editor, that is 'reword' and 'squash' produce their output both in verbose and non-verbose mode. I wouldn't know how to fix this but 1) invoking $GIT_EDITOR from git-rebase--interactive.sh, but to make this right there should be a built-in command for shell scripts and an interface for git-commit that prepare the editor contents like git-commit does now, or 2) forcing $GIT_EDITOR and git-commit to print on distinct file descriptors, which would involve temporarily wrapping the $GIT_EDITOR call in a shell script that redirects stdin to some other file descriptor similar to what t/test-lib.sh does, or Hmm. In the test scripts, we send stdout and stderr for sub-commands to fds 3 and 4 respectively. And then we either point those at /dev/null or to 1 and 2, depending on whether $verbose mode was specified. I don't think that will work here, though. You literally have one git commit command to run, and you want its stderr/stdout to go somewhere different than the $GIT_EDITOR it will invoke. Your (2) makes some sense to me. Something like: GIT_EDITOR=$(shellquote $(git var GIT_EDITOR)) 3 24 \ git commit ... 31 42 wherever 21 Or we could just point it at /dev/tty, though I guess that may open another can of worms (systems without /dev/tty, what happens when you do not have a terminal, etc). 3) passing the --quiet option in non-verbose mode and omitting it in verbose mode, which would cover the '$status != 0' above for if a command fails, it should indicate its error status despite being asked to be silent. Yeah, that is probably the cleanest option if it works. I would just worry that it is not as complete. It works for git commit, but are there are other commands wrapped in the verbose output that would want the same treatment (that might not know about --quiet)? Your paragraph below says it would not be that big a deal, so as long as we don't plan to add anything in the future that could not handle the requirement, that may be enough. -Peff -- 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 v1 07/19] rebase -i: log the replay of root commits
Hi, Jeff King writes: On Tue, Jul 29, 2014 at 01:18:07AM +0200, Fabian Ruch wrote: The command line used to recreate root commits specifies the option `-q` which suppresses the commit summary message. However, git-rebase--interactive tends to tell the user about the commits it creates in the final history, if she wishes (cf. command line option `--verbose`). The code parts handling non-root commits and squash commits all output commit summary messages. Do not make the replay of root commits an exception. Remove the option to make the report of the rebased history complete. It is OK that the commit summary is still suppressed when git-commit is used to initialize the authorship of the sentinel commit because this additional commit is an implementation detail hidden from the final history. The removed `-q` option was probably introduced as a copy-and-paste error stemming from that part of the root commit handling code. I'm confused. This implies that we should be seeing summaries for other commits, but not root commits, and this patch is bring them into harmony. But if I have a repo like this: git init -q repo cd repo for i in one two; do echo $i file git add file git commit -q -m $i done then using stock git gives me this: $ GIT_EDITOR=true git rebase -i --root 21 | perl -pe 's/\r/\\r\n/g' Rebasing (1/2)\r Rebasing (2/2)\r Successfully rebased and updated refs/heads/master. but with your patch, I get: $ GIT_EDITOR=true git.compile rebase -i --root 21 | perl -pe 's/\r/\\r\n/g' Rebasing (1/2)\r [detached HEAD 60834b3] one Date: Fri Aug 1 20:00:05 2014 -0400 1 file changed, 1 insertion(+) create mode 100644 file Rebasing (2/2)\r Successfully rebased and updated refs/heads/master. Am I misunderstanding the purpose of the patch? Thanks for laying out the differences in the user visible output. With stock git we are seeing summaries for other commits, but not root commits, _with the --verbose option_. It's the fault of my patch to show the summary even in non-verbose mode. This is now fixed by wrapping the relevant command in 'output', a shell function defined in git-rebase.sh as follows: output=$($@ 21 ) status=$? test $status != 0 printf %s\n $output return $status The problem that git-rebase--interactive has is that the redirection of stdin to a variable (or a file) does not work reliably with commands that invoke the log message editor, that is 'reword' and 'squash' produce their output both in verbose and non-verbose mode. I wouldn't know how to fix this but 1) invoking $GIT_EDITOR from git-rebase--interactive.sh, but to make this right there should be a built-in command for shell scripts and an interface for git-commit that prepare the editor contents like git-commit does now, or 2) forcing $GIT_EDITOR and git-commit to print on distinct file descriptors, which would involve temporarily wrapping the $GIT_EDITOR call in a shell script that redirects stdin to some other file descriptor similar to what t/test-lib.sh does, or cat $state_dir/editor.sh EOF #!/bin/sh $(git var GIT_EDITOR) \$* 3 EOF chmod +x $state_dir/editor.sh ( export GIT_EDITOR=$state_dir/editor.sh $@ 31 $state_dir/output 21 ) 3) passing the --quiet option in non-verbose mode and omitting it in verbose mode, which would cover the '$status != 0' above for if a command fails, it should indicate its error status despite being asked to be silent. Options 2) and 3) seem attainable within this patch series and 3) sounds like the cleanest option but I'm uncertain if I'm missing something here. The only command line that is wrapped in 'output' and that doesn't support a --quiet option seems to be a 'warn' line which could simply be skipped in non-verbose mode. (Johannes Schindelin is cc'd as the original author of git-rebase--interactive.sh and 'output' in particular). Fabian -- 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 v1 07/19] rebase -i: log the replay of root commits
On Tue, Jul 29, 2014 at 01:18:07AM +0200, Fabian Ruch wrote: The command line used to recreate root commits specifies the option `-q` which suppresses the commit summary message. However, git-rebase--interactive tends to tell the user about the commits it creates in the final history, if she wishes (cf. command line option `--verbose`). The code parts handling non-root commits and squash commits all output commit summary messages. Do not make the replay of root commits an exception. Remove the option to make the report of the rebased history complete. It is OK that the commit summary is still suppressed when git-commit is used to initialize the authorship of the sentinel commit because this additional commit is an implementation detail hidden from the final history. The removed `-q` option was probably introduced as a copy-and-paste error stemming from that part of the root commit handling code. I'm confused. This implies that we should be seeing summaries for other commits, but not root commits, and this patch is bring them into harmony. But if I have a repo like this: git init -q repo cd repo for i in one two; do echo $i file git add file git commit -q -m $i done then using stock git gives me this: $ GIT_EDITOR=true git rebase -i --root 21 | perl -pe 's/\r/\\r\n/g' Rebasing (1/2)\r Rebasing (2/2)\r Successfully rebased and updated refs/heads/master. but with your patch, I get: $ GIT_EDITOR=true git.compile rebase -i --root 21 | perl -pe 's/\r/\\r\n/g' Rebasing (1/2)\r [detached HEAD 60834b3] one Date: Fri Aug 1 20:00:05 2014 -0400 1 file changed, 1 insertion(+) create mode 100644 file Rebasing (2/2)\r Successfully rebased and updated refs/heads/master. Am I misunderstanding the purpose of the patch? -Peff -- 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 v1 07/19] rebase -i: log the replay of root commits
The command line used to recreate root commits specifies the option
`-q` which suppresses the commit summary message. However,
git-rebase--interactive tends to tell the user about the commits it
creates in the final history, if she wishes (cf. command line option
`--verbose`). The code parts handling non-root commits and squash
commits all output commit summary messages. Do not make the replay of
root commits an exception. Remove the option to make the report of
the rebased history complete.
It is OK that the commit summary is still suppressed when git-commit
is used to initialize the authorship of the sentinel commit because
this additional commit is an implementation detail hidden from the
final history. The removed `-q` option was probably introduced as a
copy-and-paste error stemming from that part of the root commit
handling code.
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6e2c429..576e0b1 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -511,7 +511,7 @@ do_pick () {
--no-post-rewrite -n -q -C $1
pick_one -n $1
git commit --allow-empty --allow-empty-message \
- --amend --no-post-rewrite -n -q -C $1 \
+ --amend --no-post-rewrite -n -C $1 \
${gpg_sign_opt:+$gpg_sign_opt} ||
die_with_patch $1 Could not apply $1... $2
else
--
2.0.1
--
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
