bug#52349: 29.0.50; vc-git and diff-mode: stage hunks

[Dmitry Gutov <dgutov@yandex.ru>]

On 19.09.2022 09:50, Juri Linkov wrote:
>> Do you know if Git use a stable ordering for files?
> 
> It seems the ordering is stable unless it's changed explicitly
> by command line options.
> 
>> If not, here's where the proposed implementation might fail:
>>
>> Suppose we have the staging area with contents
>>
>> a/bar b/bar
>> +bbb
>> a/foo b/foo
>> +aaa
>> +ccc
>>
>> and the diff to check in with contents
>>
>> a/foo b/foo
>> +aaa
>> a/bar b/bar
>> +bbb
>> +ccc
>>
>> ...then the check will succeed, I think.
> 
> This check is intended to detect only added/deleted files
> that get into the staging area.

It doesn't seem like it's going to differentiate between "add whole 
file" chunks and any other kinds of chunks.

Which is not a bad thing by itself, probably. But can increase the odds 
of something like the above happening.

>> Even if Git always sorts them the same, I suppose an externally-produced
>> diff could trigger this scenario. It should be a pretty rare case, though,
>> so it's probably fine.
>>
>> A tweak like the following could fix it, though: instead of replacing the
>> chunks with "", maybe keep the file headers. Then the remaining contents
>> for the same file in vc-git-patch-string wouldn't be able to stick to the
>> previous file's chunks.
> 
> This looks too complicated.  And indeed, this is a rare case, so maybe
> something like this could be added when this will became a real problem.

Sure, sounds good to me.

>>>> It's complex logic, so if you manage to write a test as well, that would be
>>>> excellent.
>>> A test could written when someone will create infrastructure for testing
>>> git commands with helpers to create a git repository and checking its content.
>>
>> test/lisp/vc/vc-tests.el actually contains a helper like this.
>>
>> Every scenario starts with calling vc-test--create-repo-function, and there
>> are tests for 'version-diff' at the very end of the file. It's somewhat
>> convoluted, so I don't blame you for missing it.
> 
> Actually, I looked into test/lisp/vc/vc-git-tests.el that is almost empty.
> I expected that since this check is git-specific, it should be in
> vc-git-tests.el.

Not sure how to best share the setup/teardown logic between vc-tests.el 
and vc-git-test.el.