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

[Dmitry Gutov]

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.