Re: [PATCH v4 12/12] sequencer: notify user of --update-refs activity

From: Derrick Stolee <derrickstolee@github.com>
To: phillip.wood@dunelm.org.uk,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: gitster@pobox.com, johannes.schindelin@gmx.de, me@ttaylorr.com,
	Jeff Hostetler <git@jeffhostetler.com>,
	Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH v4 12/12] sequencer: notify user of --update-refs activity
Date: Fri, 15 Jul 2022 09:20:05 -0400	[thread overview]
Message-ID: <fc2e8f9d-0232-4c19-2e4d-9ba6c9b17ab2@github.com> (raw)
In-Reply-To: <11ff21b9-7169-00c5-95c0-130675ffe2ae@gmail.com>

On 7/15/2022 6:12 AM, Phillip Wood wrote:
> On 12/07/2022 14:07, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> When the user runs 'git rebase -i --update-refs', the end message still
>> says only
>>
>>    Successfully rebased and updated <HEAD-ref>.
>>
>> Update the sequencer to collect the successful (and unsuccessful) ref
>> updates due to the --update-refs option, so the end message now says
>>
>>    Successfully rebased and updated <HEAD-ref>.
>>    Updated the following refs with --update-refs:
>>     refs/heads/first
>>     refs/heads/third
>>    Failed to update the following refs with --update-refs:
>>     refs/heads/second
>>
>> To test this output, we need to be very careful to format the expected
>> error to drop the leading tab characters. Also, we need to be aware that
>> the verbose output from 'git rebase' is writing progress lines which
> 
> s/is writing/writes/ ?

That is an improvement. Thanks.

>> don't use traditional newlines but clear the line after every progress
>> item is complete.
> 
> I was a bit confused by the reference to "verbose output" in this paragraph. When the user passes --verbose then we do actually use NL, it is when the user does not pass verbose that we use CR instead.

I was mostly thinking that the summary message at the end is only shown
when the command has verbose output (which really means the default of
--no-quiet). There is no current way to have --no-progress --no-quiet
to keep the progress lines out of the output but still have the summary.

>> When opening the error file in an editor, these lines
>> are visible, but when looking at the diff in a terminal those lines
>> disappear because of the characters that delete the previous characters.
>> Use 'sed' to clear those progress lines and clear the tabs so we can get
>> an exact match on our expected output.
> 
> Thanks for the comprehensive commit message and for implementing an excellent suggestion from Elijah. I wonder if it makes sense to distinguish between the current branch and all the others when writing the update message as we do here or if all the refs should just be in a single list. I also think it doesn't matter much and we can change it later if we want.

I'm definitely open to suggestions, but I also think we should start
somewhere and see what users think. Since the mechanisms for updating the
refs are different, I felt it was appropriate to have different error
messages.

>> +    if (!quiet &&
> 
> As you skip adding anything to the strbufs when quiet is true you don't really need this test
> 
>> +        (update_msg.len || error_msg.len)) {
>> +        fprintf(stderr,
>> +            _("Updated the following refs with %s:\n%s"),
>> +            "--update-refs",
>> +            update_msg.buf);
> 
> This will be printed even if all the updates falied

These are good points! Instead, I should arrange this as

	if (update_msg.len)
		/* send the success message. */
	if (error_msg.len)
		/* send the failure message. */

>> +    cat >expect <<-\EOF &&
>> +    Updated the following refs with --update-refs:
>> +        refs/heads/first
>> +        refs/heads/no-conflict-branch
>> +        refs/heads/third
>> +    Failed to update the following refs with --update-refs:
>> +        refs/heads/second
>> +    EOF

(copying this message from earlier in your message for context)

> From the last test it looks like we are already printing something when we fail to update a ref (possibly this comes from the refs backend code) I don't think it hurts to print a summary of them all after that though.

>> +    # Clear "Rebasing (X/Y)" progress lines and drop leading tabs.
>> +    tail -n 6 err >err.last &&
> 
> I'm curious as to why we need tail here but not in the test above.

I think I was trying to avoid testing the error messages from the
ref backend code. I bet that since this tail is here, the 'sed' is
no longer required.

>> +    sed -e "s/Rebasing.*Successfully/Successfully/g" -e "s/^\t//g" \
>> +        <err.last >err.trimmed &&
>> +    test_cmp expect err.trimmed

Thanks!
-Stolee