On Mon, Dec 23, 2024 at 6:08 PM Daniel Sahlberg <daniel.l.sahlb...@gmail.com> wrote:
> Hi, > > Den mån 23 dec. 2024 kl 15:42 skrev <rin...@apache.org>: > >> 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
