{"thread":{"id":"61960","subject":"[PATCH] rebase -x: don't print \"Executing:\" msgs with --quiet","startedAt":"2024-08-16T03:26:30Z","lastAt":"2024-08-21T16:00:59Z","messageCount":13,"participants":["Matheus Tavares","Elijah Newren","Patrick Steinhardt","Junio C Hamano","Matheus Tavares Bernardino","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"501083","messageId":"767ea219e3365303535c8b5f0d8eadb28b5e872e.1723778779.git.matheus.tavb@gmail.com","threadId":"61960","inReplyTo":null,"subject":"[PATCH] rebase -x: don't print \"Executing:\" msgs with --quiet","fromName":"Matheus Tavares","fromEmail":"matheus.tavb@gmail.com","sentAt":"2024-08-16T03:26:19Z","receivedAt":"2024-08-16T03:26:30Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"`rebase --exec` doesn't obey --quiet and end up printing a few messages\nabout the cmd being executed:\n\n git rebase HEAD~3 --quiet --exec \"printf foo >/dev/null\"\n Executing: printf foo >/dev/null\n Executing: printf foo >/dev/null\n Executing: printf foo >/dev/null\n\nLet's fix that.\n\nSuggested-by: Rodrigo Siqueira \nSigned-off-by: Matheus Tavares \n---\n sequencer.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0291920f0b..d5824b41c1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3793,12 +3793,14 @@ static int error_failed_squash(struct repository *r,\n \treturn error_with_patch(r, commit, subject, subject_len, opts, 1, 0);\n }\n \n-static int do_exec(struct repository *r, const char *command_line)\n+static int do_exec(struct repository *r, const char *command_line, int quiet)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint dirty, status;\n \n-\tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n+\tif (!quiet) {\n+\t\tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n+\t}\n \tcmd.use_shell = 1;\n \tstrvec_push(&cmd.args, command_line);\n \tstrvec_push(&cmd.env, \"GIT_CHERRY_PICK_HELP\");\n@@ -5013,7 +5015,7 @@ static int pick_commits(struct repository *r,\n \t\t\tif (!opts->verbose)\n \t\t\t\tterm_clear_line();\n \t\t\t*end_of_arg = '\\0';\n-\t\t\tres = do_exec(r, arg);\n+\t\t\tres = do_exec(r, arg, opts->quiet);\n \t\t\t*end_of_arg = saved;\n \n \t\t\tif (res) {\n-- \n2.46.0\n\n"},{"id":"501086","messageId":"CABPp-BEd8LvpMMf_sT5zvYrxNVe-Q=oUX7ANQa1f27GmM4=crw@mail.gmail.com","threadId":"61960","inReplyTo":"767ea219e3365303535c8b5f0d8eadb28b5e872e.1723778779.git.matheus.tavb@gmail.com","subject":"Re: [PATCH] rebase -x: don't print \"Executing:\" msgs with --quiet","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-08-16T06:20:29Z","receivedAt":"2024-08-16T06:20:42Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 15, 2024 at 8:26 PM Matheus Tavares wrote:\n>\n> `rebase --exec` doesn't obey --quiet and end up printing a few messages\n> about the cmd being executed:\n>\n> git rebase HEAD~3 --quiet --exec \"printf foo >/dev/null\"\n> Executing: printf foo >/dev/null\n> Executing: printf foo >/dev/null\n> Executing: printf foo >/dev/null\n>\n> Let's fix that.\n>\n> Suggested-by: Rodrigo Siqueira \n> Signed-off-by: Matheus Tavares \n> ---\n> sequencer.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 0291920f0b..d5824b41c1 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3793,12 +3793,14 @@ static int error_failed_squash(struct repository *r,\n> return error_with_patch(r, commit, subject, subject_len, opts, 1, 0);\n> }\n>\n> -static int do_exec(struct repository *r, const char *command_line)\n> +static int do_exec(struct repository *r, const char *command_line, int quiet)\n> {\n> struct child_process cmd = CHILD_PROCESS_INIT;\n> int dirty, status;\n>\n> - fprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n> + if (!quiet) {\n> + fprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n> + }\n> cmd.use_shell = 1;\n> strvec_push(&cmd.args, command_line);\n> strvec_push(&cmd.env, \"GIT_CHERRY_PICK_HELP\");\n> @@ -5013,7 +5015,7 @@ static int pick_commits(struct repository *r,\n> if (!opts->verbose)\n> term_clear_line();\n> *end_of_arg = '\\0';\n> - res = do_exec(r, arg);\n> + res = do_exec(r, arg, opts->quiet);\n> *end_of_arg = saved;\n>\n> if (res) {\n> --\n> 2.46.0\n\nMakes sense and looks good to me. It's kind surprising just how many\nplaces we've ignored --quiet over the years...anyway, thanks for\nfixing another one of them.\n"},{"id":"501104","messageId":"Zr8NOh-gMuhp-p0M@tanuki","threadId":"61960","inReplyTo":"767ea219e3365303535c8b5f0d8eadb28b5e872e.1723778779.git.matheus.tavb@gmail.com","subject":"Re: [PATCH] rebase -x: don't print \"Executing:\" msgs with --quiet","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-16T08:26:34Z","receivedAt":"2024-08-16T08:26:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 16, 2024 at 12:26:19AM -0300, Matheus Tavares wrote:\n> `rebase --exec` doesn't obey --quiet and end up printing a few messages\n\ns/end/ends/\n\n> about the cmd being executed:\n\ns/cmd/command/\n\n> git rebase HEAD~3 --quiet --exec \"printf foo >/dev/null\"\n> Executing: printf foo >/dev/null\n> Executing: printf foo >/dev/null\n> Executing: printf foo >/dev/null\n> \n> Let's fix that.\n> \n> Suggested-by: Rodrigo Siqueira \n> Signed-off-by: Matheus Tavares \n> ---\n> sequencer.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 0291920f0b..d5824b41c1 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3793,12 +3793,14 @@ static int error_failed_squash(struct repository *r,\n> \treturn error_with_patch(r, commit, subject, subject_len, opts, 1, 0);\n> }\n> \n> -static int do_exec(struct repository *r, const char *command_line)\n> +static int do_exec(struct repository *r, const char *command_line, int quiet)\n> {\n> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n> \tint dirty, status;\n> \n> -\tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n> +\tif (!quiet) {\n> +\t\tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n> +\t}\n\nWe don't typically use braces around single-line statements, so they\nshould be removed here.\n\n> \tcmd.use_shell = 1;\n> \tstrvec_push(&cmd.args, command_line);\n> \tstrvec_push(&cmd.env, \"GIT_CHERRY_PICK_HELP\");\n> @@ -5013,7 +5015,7 @@ static int pick_commits(struct repository *r,\n> \t\t\tif (!opts->verbose)\n> \t\t\t\tterm_clear_line();\n> \t\t\t*end_of_arg = '\\0';\n> -\t\t\tres = do_exec(r, arg);\n> +\t\t\tres = do_exec(r, arg, opts->quiet);\n> \t\t\t*end_of_arg = saved;\n> \n> \t\t\tif (res) {\n\nDo we also want to add a test for this fix?\n\nOther than those nits the fix looks obviously correct to me, thanks!\n\nPatrick\n"},{"id":"501146","messageId":"xmqqa5hcl6cy.fsf@gitster.g","threadId":"61960","inReplyTo":"Zr8NOh-gMuhp-p0M@tanuki","subject":"Re: [PATCH] rebase -x: don't print \"Executing:\" msgs with --quiet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-16T17:34:05Z","receivedAt":"2024-08-16T17:34:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> \t\t\tif (res) {\n>\n> Do we also want to add a test for this fix?\n>\n> Other than those nits the fix looks obviously correct to me, thanks!\n\nThanks for a review. I agree that this is almost there but would\nwant a test.\n\n"},{"id":"501164","messageId":"be3c968b0d9085843cd9ce67e85aadfaaafa69c8.1723848510.git.matheus.tavb@gmail.com","threadId":"61960","inReplyTo":"767ea219e3365303535c8b5f0d8eadb28b5e872e.1723778779.git.matheus.tavb@gmail.com","subject":"[PATCH v2] rebase -x: don't print \"Executing:\" msgs with --quiet","fromName":"Matheus Tavares","fromEmail":"matheus.tavb@gmail.com","sentAt":"2024-08-16T22:48:30Z","receivedAt":"2024-08-16T22:51:31Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"`rebase --exec` doesn't obey --quiet and ends up printing a few messages\nabout the command being executed:\n\n git rebase HEAD~3 --quiet --exec \"printf foo >/dev/null\"\n Executing: printf foo >/dev/null\n Executing: printf foo >/dev/null\n Executing: printf foo >/dev/null\n\nLet's fix that.\n\nReported-by: Lincoln Yuji \nReported-by: Rodrigo Siqueira \nSigned-off-by: Matheus Tavares \n---\nChanges in v2:\n- Applied commit message fixes by Patrick.\n- Fixed codestyle.\n- Added regression test.\n- Also checked \"!opt->quiet\" before calling term_clear_line() (this\n would only print whitspaces, so no direct impact for users, but the\n bytes are still there when the output is captured by scripts, like the\n test script :)\n- Added Lincoln as one of the reporters.\n\n sequencer.c | 13 +++++++------\n t/t3400-rebase.sh | 7 +++++++\n 2 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0291920f0b..79d577e676 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3793,12 +3793,13 @@ static int error_failed_squash(struct repository *r,\n \treturn error_with_patch(r, commit, subject, subject_len, opts, 1, 0);\n }\n \n-static int do_exec(struct repository *r, const char *command_line)\n+static int do_exec(struct repository *r, const char *command_line, int quiet)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint dirty, status;\n \n-\tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n+\tif (!quiet)\n+\t\tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n \tcmd.use_shell = 1;\n \tstrvec_push(&cmd.args, command_line);\n \tstrvec_push(&cmd.env, \"GIT_CHERRY_PICK_HELP\");\n@@ -4902,7 +4903,7 @@ static int pick_one_commit(struct repository *r,\n \tif (item->command == TODO_EDIT) {\n \t\tstruct commit *commit = item->commit;\n \t\tif (!res) {\n-\t\t\tif (!opts->verbose)\n+\t\t\tif (!opts->quiet && !opts->verbose)\n \t\t\t\tterm_clear_line();\n \t\t\tfprintf(stderr, _(\"Stopped at %s... %.*s\\n\"),\n \t\t\t\tshort_commit_name(r, commit), item->arg_len, arg);\n@@ -4994,7 +4995,7 @@ static int pick_commits(struct repository *r,\n \t\t\t\t\tNULL, REF_NO_DEREF);\n \n \t\t\tif (item->command == TODO_BREAK) {\n-\t\t\t\tif (!opts->verbose)\n+\t\t\t\tif (!opts->quiet && !opts->verbose)\n \t\t\t\t\tterm_clear_line();\n \t\t\t\treturn stopped_at_head(r);\n \t\t\t}\n@@ -5010,10 +5011,10 @@ static int pick_commits(struct repository *r,\n \t\t\tchar *end_of_arg = (char *)(arg + item->arg_len);\n \t\t\tint saved = *end_of_arg;\n \n-\t\t\tif (!opts->verbose)\n+\t\t\tif (!opts->quiet && !opts->verbose)\n \t\t\t\tterm_clear_line();\n \t\t\t*end_of_arg = '\\0';\n-\t\t\tres = do_exec(r, arg);\n+\t\t\tres = do_exec(r, arg, opts->quiet);\n \t\t\t*end_of_arg = saved;\n \n \t\t\tif (res) {\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex ae34bfad60..15b3228c6e 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -235,6 +235,13 @@ test_expect_success 'rebase --merge -q is quiet' '\n \ttest_must_be_empty output.out\n '\n \n+test_expect_success 'rebase --exec -q is quiet' '\n+\tgit checkout -B quiet topic &&\n+\tgit rebase --exec true -q main >output.out 2>&1 &&\n+\ttest_must_be_empty output.out\n+\t\n+'\n+\n test_expect_success 'Rebase a commit that sprinkles CRs in' '\n \t(\n \t\techo \"One\" &&\n-- \n2.46.0\n\n"},{"id":"501221","messageId":"xmqq34n3jswh.fsf@gitster.g","threadId":"61960","inReplyTo":"be3c968b0d9085843cd9ce67e85aadfaaafa69c8.1723848510.git.matheus.tavb@gmail.com","subject":"Re: [PATCH v2] rebase -x: don't print \"Executing:\" msgs with --quiet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-17T11:22:22Z","receivedAt":"2024-08-17T11:23:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares writes:\n\n> `rebase --exec` doesn't obey --quiet and ends up printing a few messages\n> about the command being executed:\n> ...\n> -static int do_exec(struct repository *r, const char *command_line)\n> +static int do_exec(struct repository *r, const char *command_line, int quiet)\n> {\n> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n> \tint dirty, status;\n> \n> -\tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n> +\tif (!quiet)\n> +\t\tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n\nThis is very much understandable and match what the proposed log\nmessage explained.\n\n> @@ -4902,7 +4903,7 @@ static int pick_one_commit(struct repository *r,\n> \tif (item->command == TODO_EDIT) {\n> \t\tstruct commit *commit = item->commit;\n> \t\tif (!res) {\n> -\t\t\tif (!opts->verbose)\n> +\t\t\tif (!opts->quiet && !opts->verbose)\n> \t\t\t\tterm_clear_line();\n\nThis is not, though. The original says \"if not verbose, clear the\nline\", so presumably calling the term_clear_line() makes it _less_\nverbose. The reasoning needs to be explained. \n\nI actually would have expected that this message ...\n\n> \t\t\tfprintf(stderr, _(\"Stopped at %s... %.*s\\n\"),\n> \t\t\t\tshort_commit_name(r, commit), item->arg_len, arg);\n\n... goes away when opts->quiet is in effect ;-).\n\nAnother thing, if _all_ calls to term_clear_line() is done under the\nsame \"not quiet, and not verbose\" condition, perhaps it is easier to\nfollow the resulting code if a helper function that takes a single\nargument, opts, and does eomthing like:\n\n\tstatic void helper(struct replay_opts *opts)\n\t{\n\t\t/* \n * explain why we shouldn't call term_clear_line()\n * under opts->quiet or opts->verbose here.\n\t\t */\n\t\tif (opts->quiet || opts->verbose)\n\t\t\treturn;\n\t\tterm_clear_line();\n\t}\n\nOnce we understand why it makes sense to treat quiet and verbose the\nsame way with repect to clearing the line, we can properly fill the\n\"explain\" above, and give an intuitive name to the helper, which will\nhelp readers understand the callers, too.\n\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index ae34bfad60..15b3228c6e 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -235,6 +235,13 @@ test_expect_success 'rebase --merge -q is quiet' '\n> \ttest_must_be_empty output.out\n> '\n> \n> +test_expect_success 'rebase --exec -q is quiet' '\n> +\tgit checkout -B quiet topic &&\n> +\tgit rebase --exec true -q main >output.out 2>&1 &&\n> +\ttest_must_be_empty output.out\n> +\t\n> +'\n\nThanks.\n"},{"id":"501234","messageId":"CAGdrTFhZ6KeDPDUoCsV3h5myPuoYf7RR8eFdbFFXGrUGCdEkEw@mail.gmail.com","threadId":"61960","inReplyTo":"xmqq34n3jswh.fsf@gitster.g","subject":"Re: [PATCH v2] rebase -x: don't print \"Executing:\" msgs with --quiet","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.tavb@gmail.com","sentAt":"2024-08-18T13:03:45Z","receivedAt":"2024-08-18T13:03:58Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Aug 17, 2024 at 8:22 AM Junio C Hamano wrote:\n>\n> Matheus Tavares writes:\n>\n> >\n> > - fprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n> > + if (!quiet)\n> > + fprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n>\n> This is very much understandable and match what the proposed log\n> message explained.\n>\n> > @@ -4902,7 +4903,7 @@ static int pick_one_commit(struct repository *r,\n> > if (item->command == TODO_EDIT) {\n> > struct commit *commit = item->commit;\n> > if (!res) {\n> > - if (!opts->verbose)\n> > + if (!opts->quiet && !opts->verbose)\n> > term_clear_line();\n>\n> This is not, though. The original says \"if not verbose, clear the\n> line\", so presumably calling the term_clear_line() makes it _less_\n> verbose. The reasoning needs to be explained.\n\nThe idea is that, when running in --quiet mode, we don't want to print\nanything, not even a line-cleaning char sequence.\n\nNonetheless, since these are invisible chars (assuming we haven't\nprinted anything to be \"cleaned\" before them), printing them doesn't\nactually make a difference to the user running rebase in the terminal,\nas they won't see the chars anyways.\n\nThe actual issue is when piping/redirecting the rebase output, which\nwill include these invisible chars... So perhaps, instead of modifying\nthe sequencer.c to use \"if (!opts->quiet && !opts->verbose)\nterm_clean_line()\", the correct approach would be to modify\n\"term_clean_line()\" to return earlier \"if (!isatty(1))\". What do you\nthink?\n\n> I actually would have expected that this message ...\n>\n> > fprintf(stderr, _(\"Stopped at %s... %.*s\\n\"),\n> > short_commit_name(r, commit), item->arg_len, arg);\n>\n> ... goes away when opts->quiet is in effect ;-).\n\nSure, I can add that :) I was mostly focused on the \"Executing ...\"\nlines, so that's why I haven't seen/touched this one.\n"},{"id":"501270","messageId":"08dc334a-e1d9-4aa1-945e-c543de549163@gmail.com","threadId":"61960","inReplyTo":"CAGdrTFhZ6KeDPDUoCsV3h5myPuoYf7RR8eFdbFFXGrUGCdEkEw@mail.gmail.com","subject":"Re: [PATCH v2] rebase -x: don't print \"Executing:\" msgs with --quiet","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-08-19T13:57:16Z","receivedAt":"2024-08-19T13:57:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Matheus\n\nOn 18/08/2024 14:03, Matheus Tavares Bernardino wrote:\n> On Sat, Aug 17, 2024 at 8:22 AM Junio C Hamano wrote:\n> The idea is that, when running in --quiet mode, we don't want to print\n> anything, not even a line-cleaning char sequence.\n> \n> Nonetheless, since these are invisible chars (assuming we haven't\n> printed anything to be \"cleaned\" before them), printing them doesn't\n> actually make a difference to the user running rebase in the terminal,\n> as they won't see the chars anyways.\n> \n> The actual issue is when piping/redirecting the rebase output, which\n> will include these invisible chars... So perhaps, instead of modifying\n> the sequencer.c to use \"if (!opts->quiet && !opts->verbose)\n> term_clean_line()\", the correct approach would be to modify\n> \"term_clean_line()\" to return earlier \"if (!isatty(1))\". What do you\n> think?\n\nOn the face of it that sounds like a good idea but I haven't thought too \nmuch about it. These messages are all going to stderr rather than \nstdout. If we do go that way we'll need to adjust \nlaunch_specified_editor() in editor.c to either suppress the hint or \nterminate it with '\\n' if stderr is not a terminal.\n\n>> I actually would have expected that this message ...\n>>\n>>> fprintf(stderr, _(\"Stopped at %s... %.*s\\n\"),\n>>> short_commit_name(r, commit), item->arg_len, arg);\n>>\n>> ... goes away when opts->quiet is in effect ;-).\n> \n> Sure, I can add that :) I was mostly focused on the \"Executing ...\"\n> lines, so that's why I haven't seen/touched this one.\n\nIf we're going to suppress this we should probably suppress the message \nabout amending the commit that gets printed after this by \nerror_with_patch(). There are a number of other places that we ignore \n\"--quiet\". stopped_at_head() prints a similar message to the one above \nwhen we stop for a \"break\" command and currently ignores \"--quiet\". \nShould the messages from \"--autostash\" be suppressed by \"--quiet\"? What \nabout when a commit is dropped because it is has become empty in \ndo_pick_commit()?\n\nThanks for working on this, it would be nice to have the sequencer \nrespect \"--quiet\" better.\n\nPhillip\n"},{"id":"501283","messageId":"xmqqwmkcfrk6.fsf@gitster.g","threadId":"61960","inReplyTo":"CAGdrTFhZ6KeDPDUoCsV3h5myPuoYf7RR8eFdbFFXGrUGCdEkEw@mail.gmail.com","subject":"Re: [PATCH v2] rebase -x: don't print \"Executing:\" msgs with --quiet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-19T15:41:45Z","receivedAt":"2024-08-19T15:41:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino writes:\n\n> Nonetheless, since these are invisible chars (assuming we haven't\n> printed anything to be \"cleaned\" before them), printing them doesn't\n> actually make a difference to the user running rebase in the terminal,\n> as they won't see the chars anyways.\n>\n> The actual issue is when piping/redirecting the rebase output, which\n> will include these invisible chars... So perhaps, instead of modifying\n> the sequencer.c to use \"if (!opts->quiet && !opts->verbose)\n> term_clean_line()\", the correct approach would be to modify\n> \"term_clean_line()\" to return earlier \"if (!isatty(1))\". What do you\n> think?\n\nSo, term_clear_line() assumes that there were something already on\nthe line, goes back to the beginning of the line and then makes what\nwas on the line invisible, either by overwriting them with enough\nspaces or with \"clear to the end of line\" sequence, and then go back\nto the beginning of the line. None of that really makes much sense\nif the output is not going to the human user sitting in front of the\nterminal, so isatty(1) (or isatty(2)[*]) based guard does sound like\nthe right thing to do. I certainly would have suggested us do so if\nwe were inventing this code anew today, and offhand my gut feeling\nis that it is unlikely if such a behaviour change causes breakage of\nany existing scripted use.\n\nBut people do \"interesting\" things, and because there are\nsufficiently large number of Git users, I would not be totally\nsurprised if there are people who \"double check\" by, say, counting\n\"Rebasing\" and \"Executing\" and making sure these match what they fed\nin the todo file---their use case will certainly be broken.\n\n>> I actually would have expected that this message ...\n>>\n>> > fprintf(stderr, _(\"Stopped at %s... %.*s\\n\"),\n>> > short_commit_name(r, commit), item->arg_len, arg);\n>>\n>> ... goes away when opts->quiet is in effect ;-).\n>\n> Sure, I can add that :) I was mostly focused on the \"Executing ...\"\n> lines, so that's why I haven't seen/touched this one.\n\nIt would make the user experience horrible if we removed this\n\"Stopped at\", especially with the \"Rebasing...\" indicator that is\ngiven at each step squelched with the \"opts->quiet\" flag, because\nthe user would totally really lose where they are if we did't give\nthis message. As it is the norm for sequencer operations to advance\nwithout human intervention, stopping at somewhere ought to be given\na bit more special status and deserves to be marked as such.\n\nWith the same yardstick, removing \"Executing:\" message while running\nunder the --quiet option, when these \"exec\" insn were automatically\ninserted via \"rebase -x\", does make sense, because it is just \"a\nstream of insns given in the todo file, we execute one step at a\ntime, and we stay quiet unless some exceptional thing happens\".\nBecause we give a warning if the execution fails or the execution\nleaves the working tree dirty, and we include what command we\nattempted to run with the \"exec\" insn, it is unlikely that users\nwill lose their place and get confused.\n\nIf a user of \"rebase -i\" inserted an \"exec\" insn at a selected place\nin the todo file, the above argument to sequelch \"Executing\" becomes\na bit weaker, but I think it still is OK.\n"},{"id":"501302","messageId":"xmqqv7zwclns.fsf@gitster.g","threadId":"61960","inReplyTo":"08dc334a-e1d9-4aa1-945e-c543de549163@gmail.com","subject":"Re: [PATCH v2] rebase -x: don't print \"Executing:\" msgs with --quiet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-19T20:17:27Z","receivedAt":"2024-08-19T20:17:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 18/08/2024 14:03, Matheus Tavares Bernardino wrote:\n>> ...\n>> term_clean_line()\", the correct approach would be to modify\n>> \"term_clean_line()\" to return earlier \"if (!isatty(1))\". What do you\n>> think?\n>\n> On the face of it that sounds like a good idea but I haven't thought\n> too much about it. These messages are all going to stderr rather than\n> stdout. If we do go that way we'll need to adjust\n> launch_specified_editor() in editor.c to either suppress the hint or\n> terminate it with '\\n' if stderr is not a terminal.\n\nRight.\n\nThe true reason why I brought it up was because (1) it looked really\nfunny to avoid doing that term_clean_line() under \"--verbose\" as\nwell as under \"--quiet\" and the code should explain what reasoning\nbacks such decision but it did not, and (2) that unexplained funny\npattern repeated, which probably was a sign that it needed to become\na small helper function with descriptive name to encapsulate the\nlogic to decide when to call and when not to call the clean-line,\nwhich as a bonus would give a central place for us to explain the\nreason behind not cleaning the line under \"--verbose\" and the same\nfor \"--quiet\" (as I suspect that these two want to omit the call for\ndifferent reasons).\n\nThanks.\n"},{"id":"501398","messageId":"CAGdrTFgo5kyObDTyhwjbnDUf8zm=-qYFbykqKv6cDG1mgSfmpg@mail.gmail.com","threadId":"61960","inReplyTo":"08dc334a-e1d9-4aa1-945e-c543de549163@gmail.com","subject":"Re: [PATCH v2] rebase -x: don't print \"Executing:\" msgs with --quiet","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.tavb@gmail.com","sentAt":"2024-08-20T22:23:57Z","receivedAt":"2024-08-20T22:24:10Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, Aug 19, 2024 at 10:57 AM Phillip Wood wrote:\n>\n> Hi Matheus\n>\n> On 18/08/2024 14:03, Matheus Tavares Bernardino wrote:\n> > On Sat, Aug 17, 2024 at 8:22 AM Junio C Hamano wrote:\n> > The idea is that, when running in --quiet mode, we don't want to print\n> > anything, not even a line-cleaning char sequence.\n> >\n> > Nonetheless, since these are invisible chars (assuming we haven't\n> > printed anything to be \"cleaned\" before them), printing them doesn't\n> > actually make a difference to the user running rebase in the terminal,\n> > as they won't see the chars anyways.\n> >\n> > The actual issue is when piping/redirecting the rebase output, which\n> > will include these invisible chars... So perhaps, instead of modifying\n> > the sequencer.c to use \"if (!opts->quiet && !opts->verbose)\n> > term_clean_line()\", the correct approach would be to modify\n> > \"term_clean_line()\" to return earlier \"if (!isatty(1))\". What do you\n> > think?\n>\n> On the face of it that sounds like a good idea but I haven't thought too\n> much about it. These messages are all going to stderr rather than\n> stdout.\n\nOh, good point. So `isatty(2)`, actually.\n\n> If we do go that way we'll need to adjust\n> launch_specified_editor() in editor.c to either suppress the hint or\n> terminate it with '\\n' if stderr is not a terminal.\n\nHmm, isn't that what we do already? The hint printing is conditional\non `print_waiting_for_editor` which, in turn, is conditional on\n`isatty(2)`.\n"},{"id":"501401","messageId":"f105b34b8e6b33448f4d0ef07d51b7bbf4e71aaa.1724203912.git.matheus.tavb@gmail.com","threadId":"61960","inReplyTo":"be3c968b0d9085843cd9ce67e85aadfaaafa69c8.1723848510.git.matheus.tavb@gmail.com","subject":"[PATCH v3] rebase --exec: respect --quiet","fromName":"Matheus Tavares","fromEmail":"matheus.tavb@gmail.com","sentAt":"2024-08-21T01:31:52Z","receivedAt":"2024-08-21T01:33:33Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"rebase --exec doesn't obey --quiet and ends up printing messages about\nthe command being executed:\n\n git rebase HEAD~3 --quiet --exec true\n Executing: true\n Executing: true\n Executing: true\n\nLet's fix that by omitting the \"Executing\" messages when using --quiet.\n\nFurthermore, the sequencer code includes a few calls to\nterm_clear_line(), which prints a special character sequence to erase\nthe previous line displayed on stderr (even when nothing was printed\nyet). For an user running the command interactively, the net effect of\ncalling this function with or without --quiet is the same as the\ncharacters are invisible in the terminal. However, when redirecting the\noutput to a file or piping to another command, the presence of these\ninvisible characters is noticeable, and it may break user expectation as\n--quiet is not being respected.\n\nWe could skip the term_clear_line() calls when --quiet is used, like we\nare doing with the \"Executing\" messages, but it makes much more sense to\ncondition the line cleaning upon stderr being TTY, since these\ncharacters are really only useful for TTY outputs.\n\nThe added test checks for both these two changes.\n\nReported-by: Lincoln Yuji \nReported-by: Rodrigo Siqueira \nSigned-off-by: Matheus Tavares \n---\n\nChanges in v3:\n- Skipped term_clean_line() when stderr is not a TTY.\n- Removed the !opts->quiet condition when calling term_clean_line().\n- Reworded commit message to better explain the proposed changes.\n\n pager.c | 2 ++\n sequencer.c | 7 ++++---\n t/t3400-rebase.sh | 6 ++++++\n 3 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 896f40fcd2..9c24ce6263 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -234,6 +234,8 @@ int term_columns(void)\n */\n void term_clear_line(void)\n {\n+\tif (!isatty(2))\n+\t\treturn;\n \tif (is_terminal_dumb())\n \t\t/*\n \t\t * Fall back to print a terminal width worth of space\ndiff --git a/sequencer.c b/sequencer.c\nindex 0291920f0b..65c485d783 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3793,12 +3793,13 @@ static int error_failed_squash(struct repository *r,\n \treturn error_with_patch(r, commit, subject, subject_len, opts, 1, 0);\n }\n \n-static int do_exec(struct repository *r, const char *command_line)\n+static int do_exec(struct repository *r, const char *command_line, int quiet)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint dirty, status;\n \n-\tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n+\tif (!quiet)\n+\t\tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n \tcmd.use_shell = 1;\n \tstrvec_push(&cmd.args, command_line);\n \tstrvec_push(&cmd.env, \"GIT_CHERRY_PICK_HELP\");\n@@ -5013,7 +5014,7 @@ static int pick_commits(struct repository *r,\n \t\t\tif (!opts->verbose)\n \t\t\t\tterm_clear_line();\n \t\t\t*end_of_arg = '\\0';\n-\t\t\tres = do_exec(r, arg);\n+\t\t\tres = do_exec(r, arg, opts->quiet);\n \t\t\t*end_of_arg = saved;\n \n \t\t\tif (res) {\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex ae34bfad60..bd8bcc381a 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -235,6 +235,12 @@ test_expect_success 'rebase --merge -q is quiet' '\n \ttest_must_be_empty output.out\n '\n \n+test_expect_success 'rebase --exec -q is quiet' '\n+\tgit checkout -B quiet topic &&\n+\tgit rebase --exec true -q main >output.out 2>&1 &&\n+\ttest_must_be_empty output.out\n+'\n+\n test_expect_success 'Rebase a commit that sprinkles CRs in' '\n \t(\n \t\techo \"One\" &&\n-- \n2.46.0\n\n"},{"id":"501437","messageId":"xmqq7cc93lxj.fsf@gitster.g","threadId":"61960","inReplyTo":"f105b34b8e6b33448f4d0ef07d51b7bbf4e71aaa.1724203912.git.matheus.tavb@gmail.com","subject":"Re: [PATCH v3] rebase --exec: respect --quiet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-21T16:00:56Z","receivedAt":"2024-08-21T16:00:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares writes:\n\n> rebase --exec doesn't obey --quiet and ends up printing messages about\n> the command being executed:\n> ...\n> Changes in v3:\n> - Skipped term_clean_line() when stderr is not a TTY.\n> - Removed the !opts->quiet condition when calling term_clean_line().\n> - Reworded commit message to better explain the proposed changes.\n\nThe updated title and log message, and the new approach to squelch\nterm_clear_line() when it is not needed. Both look quite sensible.\n\nWill replace. Thanks.\n\n"}]}