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

[Dmitry Gutov <dgutov@yandex.ru>]

On 12.09.2022 21:19, Juri Linkov wrote:
>>> +              (if (string-search file-diff vc-git-patch-string)
>>> +                  (setq vc-git-patch-string
>>> +                        (string-replace file-diff "" vc-git-patch-string))
>>
>> Not sure I understand this part. We're replacing some text (the diff file
>> header) with empty text in the patch string? But not the whole hunk?
> 
> Actually it replaces the whole new/deleted file differences.
> 
>> It might work if we removed the whole hunk as well, but we need to make
>> sure that the staged contents for that file are exactly the same as what is
>> contained for that file in vc-git-patch-string.
> 
> This is exactly what this patch does.

Right, sorry, I see it now.

Do you know if Git use a stable ordering for files?

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.

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.

>> Perhaps if we called diff-file-next in addition to (move-beginning-of-line
>> 1)?
> 
> diff-file-next doesn't work: it stops in the middle of the diff file header.
> Therefore the patch navigates git diff file headers explicitly.
> 
>> But we need to save both strings: and ensure that if there is a match
>> for that file header, then all hunks are contained in the patch as well
>> (and nothing extra, for that file).
> 
> This is implemented by the patch.  Maybe it could help you to see how it works
> by running it under debugger and stepping through it.
> 
>> 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.