Re: svn commit: r1922647 - in /subversion/trunk/subversion: include/private/svn_client_private.h svn/shelf-cmd.c svn/shelf2-cmd.c

On Mon, Dec 23, 2024 at 6:08 PM Daniel Sahlberg <[email protected]>
wrote:

> Hi,
>
> Den mån 23 dec. 2024 kl 15:42 skrev <[email protected]>:
>
>> Author: rinrab
>> Date: Mon Dec 23 14:42:38 2024
>> New Revision: 1922647
>>
>> URL: http://svn.apache.org/viewvc?rev=1922647&view=rev
>> Log:
>> Follow-up to r1921602: Fix argument sequence of
>> svn_client__get_diff_writer_svn
>> to much its declaration with its implementation.
>>
>
> [...]
>
> * subversion/svn/shelf-cmd.c
>>   (shelf_diff): Update the usage to fit with the updated function.
>> * subversion/svn/shelf2-cmd.c
>>   (shelf_diff): Ditto.
>>
>> Modified:
>>     subversion/trunk/subversion/include/private/svn_client_private.h
>>     subversion/trunk/subversion/svn/shelf-cmd.c
>>     subversion/trunk/subversion/svn/shelf2-cmd.c
>>
>>
> [...]
>
>
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/shelf-cmd.c?rev=1922647&r1=1922646&r2=1922647&view=diff
>>
>> ==============================================================================
>> --- subversion/trunk/subversion/svn/shelf-cmd.c (original)
>> +++ subversion/trunk/subversion/svn/shelf-cmd.c Mon Dec 23 14:42:38 2024
>> @@ -769,8 +769,8 @@ shelf_diff(const char *name,
>>                  FALSE /*ignore_content_type*/,
>>                  FALSE /*ignore_properties*/,
>>                  FALSE /*properties_only*/,
>> -                TRUE /*pretty_print_mergeinfo*/,
>>                  FALSE /*use_git_diff_format*/,
>> +                TRUE /*pretty_print_mergeinfo*/,
>>                  svn_cmdline_output_encoding(scratch_pool),
>>                  stream, errstream,
>>                  ctx, scratch_pool));
>>
>> Modified: subversion/trunk/subversion/svn/shelf2-cmd.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/shelf2-cmd.c?rev=1922647&r1=1922646&r2=1922647&view=diff
>>
>> ==============================================================================
>> --- subversion/trunk/subversion/svn/shelf2-cmd.c (original)
>> +++ subversion/trunk/subversion/svn/shelf2-cmd.c Mon Dec 23 14:42:38 2024
>> @@ -769,8 +769,8 @@ shelf_diff(const char *name,
>>                  FALSE /*ignore_content_type*/,
>>                  FALSE /*ignore_properties*/,
>>                  FALSE /*properties_only*/,
>> -                TRUE /*pretty_print_mergeinfo*/,
>>                  FALSE /*use_git_diff_format*/,
>> +                TRUE /*pretty_print_mergeinfo*/,
>>                  svn_cmdline_output_encoding(scratch_pool),
>>                  stream, errstream,
>>                  ctx, scratch_pool));
>>
>>
>>
> Won't these two changes affect how the patch files look, when using the
> x-shelf-diff command? Previously we created a patch in the git-diff format,
> but now we won't.
>
> It seems the intention was always to create a non-git-diff format patch,
> but given that the arguments were reversed in the declaration (now
> corrected, good!) and then used in the wrong order here.
>
> I don't have the differences between git-diff and non-git-diff's on the
> top of my head so I can't judge how much of a change this will be and the
> x- commands are explicitly noted as "no promise of backward compatibility".
> But still I think we should agree (at least with lazy consensus) that:
> 1. We should change to the intended implementation (as above)
>   A. In that case, if we should mention this in CHANGES and/or release
> notes
> 2. Or if we should say that the previous implementation should stay, in
> which case the above two changes should be reverted and we should "only"
> change the comments (something like:
> [[[
> - TRUE /*pretty_print_mergeinfo*/,
> +TRUE /*use_git_diff_format*/,
> -FALSE /*use_git_diff_format*/,
> +FALSE /*pretty_print_merge_info*/,
> ]]]
>
> I don't really have a strong opinion about either of these, only that if
> we do 1. above, we should also do A.
>
> Cheers,
> Daniel
>
>
Hi Daniel,

Thanks for this feedback, but probably you didn't fully get this commit.
This is a fix to a stupid bug that I made in the commit, that I follow-up
to. In r1921602 I've introduced a new argument to the function
(use_git_diff_format). Before, it was always defaulting to FALSE. In that
revision, you may notice a change, presented below in this function:

[[[
   SVN_ERR(get_diff_processor(diff_processor, &ddi,
                              options,
                              relative_to_dir,
                              no_diff_added,
                              no_diff_deleted,
                              show_copies_as_adds,
                              ignore_content_type,
                              ignore_properties,
                              properties_only,
-                             FALSE /*use_git_diff_format*/,
+                             use_git_diff_format,
                              pretty_print_mergeinfo,
                              header_encoding,
                              outstream, errstream,
]]]

This means that we need to pass FALSE to the use_git_diff_format argument
in order to prevent any functional changes (in some places we'll specify a
different value, for example, in svn_client_diff* APIs. These functions
have a flag passed from the caller's side).

I mean that in r1921602 I've introduced a stupid bug, fixed in the current
revision. This follows that no functional changes were made with these two
commits in total.

Wish you Merry Christmas!

-- 
Timofei Zhakov