git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood123@gmail.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Phillip Wood <phillip.wood@dunelm.org.uk>,
	Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v2 3/4] rebase: fix garbled progress display with '-x'
Date: Tue, 25 Jun 2019 19:00:42 +0100	[thread overview]
Message-ID: <573b9280-939b-879e-6e5e-b63779836740@gmail.com> (raw)
In-Reply-To: <20190625113103.GB10853@szeder.dev>

Hi Gábor and Dscho
On 25/06/2019 12:31, SZEDER Gábor wrote:
> On Tue, Jun 25, 2019 at 11:08:04AM +0100, Phillip Wood wrote:
>>>> Rebasing (1/4)QRebasing (2/4)QRebasing (3/4)QRebasing (4/4)QQ                                                                                QSuccessfully rebased and updated refs/heads/missing-commit.
> 
> 
>>>> May I please *strongly* suggest to fix this first? It should
>>>>
>>>> - completely lose that last line,
>>>> - be inserted into the test case itself so that e.g. disk full problems are
>>>>   caught and logged properly, and
>>>> - the `test_i18ncmp expect actual` call in the test case should be replaced
>>>>   by something like:
>>>>
>>>> 	sed "\$d" <actual >actual-skip-progress &&
>>>> 	test_i18ncmp expect actual-skip-progress
>>>>
>>>> This should obviously be made as a separate, introductory patch (probably with
>>>> a less scathing commit message than my comments above would suggest).
>>>>
>>>> And that would also remove the double-yuck.
>>>
>>> Unfortunately, this addresses only one of the "Yuck"s; see v3 of this
>>> patch series [1].
>>>
>>> The other yucks affect the following four tests in
>>> 't3420-rebase-autostash.sh':
>>>
>>>   16 - rebase --merge --autostash: check output
>>>   23 - rebase --merge: check output with conflicting stash
>>>   26 - rebase --interactive --autostash: check output
>>>   33 - rebase --interactive: check output with conflicting stash
>>>
>>> These tests come from commits b76aeae553 (rebase: add regression tests
>>> for console output, 2017-06-19) and 7d70e6b902 (rebase: add more
>>> regression tests for console output, 2017-06-19), and are specifically
>>> about checking the (whole) console output of 'git rebase', so I left
>>> the updates to them as they were.
>>>
>>> In any case, Cc-ing Phillip to discuss whether something could be done
>>> about them (now perhaps preferably (for me :) as a follow-up, and not
>>> another preparatory patches).
>>
>> Those tests were added to check that `git stash` was being silenced (see
>> 79a6226981 ("rebase -i: silence stash apply", 2017-05-18)).
> 
> Hmm, so would it then be possible/sensible to do something like this
> instead:
> 
>   git rebase --options... >out &&
>   test_i18ngrep ! "<something specific that only 'git stash' would print if it wasn't silenced>" out
> 
>> I can have a
>> think about a better way to do that, but is it still a problem? I just
>> tried to take a look at your CI output and
>> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11406
>> seems to be all green - have I missed something or has Gábor fixed the
>> issue?
> 
> No, it was Dscho who fixed the Azure CI issue, see
> 
>   https://public-inbox.org/git/nycvar.QRO.7.76.6.1906141356140.44@tvgsbejvaqbjf.bet/
> 
> elsewhere in this thread (it's a long one, but well worth the read
> IMO).
> 
> However, the point here is not that Azure CI failure, but rather the
> fact that tests had to be updated to include the new line clearing
> sequence in the expected output, and that "Q<...80 spaces...>Q" looks
> yuck indeed.

I've come up with a sed based solution to remove the progress
notifications from the output - see
https://github.com/gitgitgadget/git/pull/276 If you're both happy with
the basic idea I'll clean it up and submit it (I've just noticed I left
some debugging bits in one of the tests). I was worried using "\r" in a
sed expression wouldn't be portable but the tests pass on gitgitgadget.
If it proves to be a problem on other platforms we could use always tr
to convert "\r" to some unique string and use that string in the sed
expression.

Best Wishes

Phillip


  parent reply	other threads:[~2019-06-25 18:00 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-30 14:25 [PATCH] rebase: fix garbled progress display with '-x' SZEDER Gábor
2019-04-30 22:25 ` Johannes Schindelin
2019-05-01 23:16   ` SZEDER Gábor
2019-05-03  8:41     ` Johannes Schindelin
2019-06-11 12:38     ` SZEDER Gábor
2019-06-11 13:03 ` [PATCH v2 0/4] rebase/progress: add and use term_clear_line() SZEDER Gábor
2019-06-11 13:03   ` [PATCH v2 1/4] t3404-rebase-interactive: use the 'q_to_cr' helper SZEDER Gábor
2019-06-11 13:03   ` [PATCH v2 2/4] pager: add a helper function to clear the last line in the terminal SZEDER Gábor
2019-06-11 13:03   ` [PATCH v2 3/4] rebase: fix garbled progress display with '-x' SZEDER Gábor
2019-06-11 20:36     ` Junio C Hamano
2019-06-11 21:11       ` SZEDER Gábor
2019-06-12 19:14         ` Johannes Schindelin
2019-06-12 19:41           ` SZEDER Gábor
2019-06-13  7:54             ` Johannes Schindelin
2019-06-14 18:42             ` Johannes Schindelin
2019-06-17 18:40               ` SZEDER Gábor
2019-06-17 19:13               ` SZEDER Gábor
2019-06-17 19:25                 ` SZEDER Gábor
2019-06-24 18:39           ` SZEDER Gábor
2019-06-25 10:08             ` Phillip Wood
2019-06-25 11:31               ` SZEDER Gábor
2019-06-25 13:33                 ` Phillip Wood
2019-06-25 18:00                 ` Phillip Wood [this message]
2019-06-25 11:38               ` Johannes Schindelin
2019-06-25 13:35                 ` Phillip Wood
2019-06-11 20:48     ` Junio C Hamano
2019-06-11 23:50       ` SZEDER Gábor
2019-06-12 16:21       ` Junio C Hamano
2019-06-11 13:03   ` [PATCH v2 4/4] progress: use term_clear_line() SZEDER Gábor
2019-06-12 20:00     ` Johannes Schindelin
2019-06-24 18:13   ` [PATCH v3 0/5] rebase/progress: add and " SZEDER Gábor
2019-06-24 18:13     ` [PATCH v3 1/5] t3404: modernize here doc style SZEDER Gábor
2019-06-25  9:06       ` Johannes Schindelin
2019-06-24 18:13     ` [PATCH v3 2/5] t3404: make the 'rebase.missingCommitsCheck=ignore' test more focused SZEDER Gábor
2019-06-25  9:07       ` Johannes Schindelin
2019-06-24 18:13     ` [PATCH v3 3/5] pager: add a helper function to clear the last line in the terminal SZEDER Gábor
2019-06-24 18:13     ` [PATCH v3 4/5] rebase: fix garbled progress display with '-x' SZEDER Gábor
2019-06-27 13:42       ` [PATCH v3.1 " SZEDER Gábor
2019-06-24 18:13     ` [PATCH v3 5/5] progress: use term_clear_line() SZEDER Gábor
2019-06-25  9:10       ` Johannes Schindelin
2019-06-25 19:44         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=573b9280-939b-879e-6e5e-b63779836740@gmail.com \
    --to=phillip.wood123@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).