Thanks, this is definitely useful.

I wonder if we should standardize dry-run names across the board. We have GIT_CHECKOUT_NONE for example.

I’m not sure that I love GIT_APPLY_NONE as an argument though. I don’t think it necessarily conveys the meaning of the argument. 🤔

As far as naming is concerned, I appeal to the authority that is git upstream - they call this option --check, so I called it CHECK.

I don't lIke that I expected > 1 returns to be meant as a synonym of this (as per the docs for the only two callbacks of that struct). I'd be tempted to provide that feature as a separate git_apply_check(…) function (with the same signature even), or alternatively the >1 behavior being made consistent, as it currently skips a bunch of checks that seem relevant w.r.t validity. Just my 2 API cents, thanks for tacking this @ddevault !

>1 returns from callbacks is not a synonym of this. If the hunk callback returns >1, then the hunk is not even tested for applicablility, it's just flatly skipped.

then the hunk is not even tested for applicablility, it's just flatly skipped.

Which means you could in theory build a "bad hunk" that would fail the skipped code, because the > 1 would have bypassed the rest of the code. So, in the words of the documentation, "[If git_apply_hunk_cb] returns > 0, the hunk will not be applied, but the apply process continues", you've hit a bug, as this skipped check is part of the "ongoing apply process", hence should have been done anyways.

I consider this skip (unless someone finds it an useful "feature") an honest-to-god bug, and that it should have been synonymous with --check. Does that make it clearer ? In the end, I'm just concerned about having "too many ways to dry run"…

I don't think the docs are ambiguous, though it took a careful reading for me to conclude so. That the hunk "will not be applied" does not imply that it will be tested for applicability, even if "the apply process continues". I think the use-case here is something else, akin to git commit -p.

I think the use-case here is something else, akin to git commit -p.

Yes, exactly so. The GIT_APPLY_CHECK and the callback are not synonymous and may in fact be complementary. Using GIT_APPLY_CHECK on a bad patch will return an error. Using a callback that always returns >1 on a bad patch should not error at all (there should be no validation because the hunks should not be attempted to be applied). You could of course combine the two so that only the hunks that you're interested in validating are actually validated.

It feels like there may be an opportunity to improve the docs here, though, to make that clear, but it seems like the deficiency is in the callback's docs so let's not block this change for that. (Suggestions to the contrary welcome, though.)

Using a callback that always returns >1 on a bad patch should not error at all

I don't think the callback can really know about "patch-badness" without parsing the patch itself.

If you want it, I'm fine with it going in, but I'll stand by my analysis that this addition introduces some "brittleness" — you can now ask for a CHECK run, return a > 1 and never be informed that some high-level patch had seen some corruption, aaannd it somehow feels wrong ? Maybe I should have just found that elusive hunk and filed a bug about it 😋?

you can now ask for a CHECK run, return a > 1 and never be informed that some high-level patch had seen some corruption

To me this just sounds like "if you do the wrong thing, something you don't want will happen". Like "if you go to the car wash and leave all of your windows down, you'll get wet."

If we're doing a GIT_APPLY_CHECK (or whatever we want to call it) then there are still a few places where we're manipulating the working directory that we should avoid.

For example, when using this flag we're still locking the index which we don't need to do since we're not actually applying any changes. We should also avoid the subsequent checkout steps since we shouldn't be touching the disk at all in the check case.

To clarify: by "manipulating the working directory" I don't mean that we're actually changing files, but we are locking the index and calling the checkout functionality unnecessarily.

To clarify: by "manipulating the working directory" I don't mean that we're actually changing files, but we are locking the index and calling the checkout functionality unnecessarily.

This seems like a nice-to-have, not an essential component of this patch. In other words, suitable for a separate/future patch.

Addressed the specific cases you pointed out in a second commit.

I also updated the first commit to handle two edge cases that I had missed. Note that it now errors out if you attempt to check a binary patch.

Bump?

Yep, I lost track of this, too. Thanks for the ping @ddevault. I'm 👍 on this.

Aha, looking more closely, now I understand why the binary check wasn't included. I'd actually suggest moving the tests for APPLY_CHECK out of apply_hunk and into git_apply so that we do the validation work but just don't write the results to the workdir or index. eg:

diff --git a/src/apply.c b/src/apply.c
index ab0b32423..0fcceafc7 100644
--- a/src/apply.c
+++ b/src/apply.c
@@ -254,9 +254,7 @@ static int apply_hunk(
        goto done;
    }

-   if (!(ctx->opts.flags & GIT_APPLY_CHECK)) {
-       error = update_hunk(image, line_num, &preimage, &postimage);
-   }
+   error = update_hunk(image, line_num, &preimage, &postimage);

 done:
    patch_image_free(&preimage);
@@ -415,21 +413,12 @@ int git_apply__patch(
            newfile->mode : GIT_FILEMODE_BLOB;
    }

-   if (patch->delta->flags & GIT_DIFF_FLAG_BINARY) {
-       if (!(ctx.opts.flags & GIT_APPLY_CHECK)) {
-           error = apply_binary(contents_out, source, source_len, patch);
-       } else {
-           // TODO
-           error = apply_err("--check with binary patches is unsupported");
-       }
-   }
+   if (patch->delta->flags & GIT_DIFF_FLAG_BINARY)
+       error = apply_binary(contents_out, source, source_len, patch);
    else if (patch->hunks.size)
        error = apply_hunks(contents_out, source, source_len, patch, &ctx);
-   else {
-       if (!(ctx.opts.flags & GIT_APPLY_CHECK)) {
-           error = git_buf_put(contents_out, source, source_len);
-       } /* else { always applicable } */
-   }
+   else
+       error = git_buf_put(contents_out, source, source_len);

    if (error)
        goto done;
@@ -864,6 +853,9 @@ int git_apply(
    if ((error = apply_deltas(repo, pre_reader, preimage, post_reader, postimage, diff, &opts)) < 0)
        goto done;

+   if (opts.flags & GIT_APPLY_CHECK)
+       goto done;
+
    switch (location) {
    case GIT_APPLY_LOCATION_BOTH:
        error = git_apply__to_workdir(repo, diff, preimage, postimage, location, &opts);
@@ -881,8 +873,7 @@ int git_apply(
    if (error < 0)
        goto done;

-   if (!(opts.flags & GIT_APPLY_CHECK))
-       error = git_indexwriter_commit(&indexwriter);
+   error = git_indexwriter_commit(&indexwriter);

 done:
    git_indexwriter_cleanup(&indexwriter);

Then we can check binary files:

diff --git a/tests/apply/apply_helpers.h b/tests/apply/apply_helpers.h
index 364daf49d..be5bd9687 100644
--- a/tests/apply/apply_helpers.h
+++ b/tests/apply/apply_helpers.h
@@ -21,6 +21,27 @@
    "-longer. take out the slices of ham, and skim off the grease if any\n" \
    "+longer; take out the slices of ham, and skim off the grease if any\n"

+/* This is the binary equivalent of DIFF_MODIFY_TWO_FILES */
+#define DIFF_MODIFY_TWO_FILES_BINARY \
+   "diff --git a/asparagus.txt b/asparagus.txt\n" \
+   "index f51658077d85f2264fa179b4d0848268cb3475c3..ffb36e513f5fdf8a6ba850a20142676a2ac4807d 100644\n" \
+   "GIT binary patch\n" \
+   "delta 24\n" \
+   "fcmX@ja+-zTF*v|6$k9DCSRvRyG(c}7zYP-rT_OhP\n" \
+   "\n" \
+   "delta 24\n" \
+   "fcmX@ja+-zTF*v|6$k9DCSRvRyG(d49zYP-rT;T@W\n" \
+   "\n" \
+   "diff --git a/veal.txt b/veal.txt\n" \
+   "index 94d2c01087f48213bd157222d54edfefd77c9bba..a7b066537e6be7109abfe4ff97b675d4e077da20 100644\n" \
+   "GIT binary patch\n" \
+   "delta 26\n" \
+   "hcmX@kah!uI%+=9HA=p1OKyM?L03)OIW@$zpW&mXg25bNT\n" \
+   "\n" \
+   "delta 26\n" \
+   "hcmX@kah!uI%+=9HA=p1OKyf3N03)N`W@$zpW&mU#22ub3\n" \
+   "\n"
+
 #define DIFF_DELETE_FILE \
    "diff --git a/gravy.txt b/gravy.txt\n" \
    "deleted file mode 100644\n" \
diff --git a/tests/apply/check.c b/tests/apply/check.c
index f83cd9d3d..2c0e53ceb 100644
--- a/tests/apply/check.c
+++ b/tests/apply/check.c
@@ -70,6 +70,23 @@ void test_apply_check__parsed_diff(void)
    git_diff_free(diff);
 }

+void test_apply_check__binary(void)
+{
+   git_diff *diff;
+   git_apply_options opts = GIT_APPLY_OPTIONS_INIT;
+
+   opts.flags |= GIT_APPLY_CHECK;
+   cl_git_pass(git_diff_from_buffer(&diff,
+       DIFF_MODIFY_TWO_FILES_BINARY,
+       strlen(DIFF_MODIFY_TWO_FILES_BINARY)));
+   cl_git_pass(git_apply(repo, diff, GIT_APPLY_LOCATION_INDEX, &opts));
+
+   validate_index_unchanged(repo);
+   validate_workdir_unchanged(repo);
+
+   git_diff_free(diff);
+}
+
 void test_apply_check__does_not_apply(void)
 {
    git_diff *diff;

Those are great suggestions! I've updated the patch accordingly.

Thanks so much @ddevault! I'm excited to have this functionality.

I'm +1 on this. @pks-t, any final review?

/rebuild

Sorry @pks-t, an error occurred while trying to requeue the build.

sigh I'm 👍, and the CI error is unrelated. I'll manually queue another build.

Thanks a lot, @ddevault!

Thanks folks!