Support for format-patch

[jacquesg]

This PR adds support for git format-patch behaviour. This is something we need to have for rebases #2253 and core git compatibility (git rebase uses git am).

It currently cannot generate a "patch" for merge commits (depends on #1965) or binary files.

[jiripospisil]

This seems to be missing static, same for the function below.

[jacquesg]

Fixed.

[ethomson]

We try to have an object-like namespace. (Eg, git_blob is an object and git_blob_foo is a foo on a blob.) With that in mind, I think that git_patch_format would be more appropriate as a name.

[jacquesg]

I had the same idea. But ...

We already have the concept of a git_patch, which is not quite the same. This is more a collection of patches.

[ethomson]

By the way, I am 👍 👍 👍 on the thorough test cases that you're including in your PRs. Thanks!

[jacquesg]

When #1965 is complete @arbee 😉, they will have to be extended in order to cover merge commits.

[arrbee]

Regarding the naming of the function, to me this feels like it is a flattening of git_diff_commit() to generate a git_diff for the commit (a git_diff is more-or-less a collection of patches, after all) and then a special formatting function for the git_diff that knows how to do all the extra things that git format-patch does.

We could shoehorn the special formatting into git_diff_print(), but I think that may not be a good idea. Instead, can we just have a git_diff_format_email that takes a git_diff plus extra parameters to generate proper email headers (i.e. flag for [PATCH] plus values for patch sequence numbers)?

Instead of writing:

git_commit_lookup(&commit, repo, &oid);
git_format_patch(&buf, repo, commit, 1, 1, NULL);

you would write the slightly more verbose:

git_diff_format_email_options email_opts;

git_commit_lookup(&commit, repo, &oid);

email_opts.patch_no = 1;
email_opts.total_patches = 1;
email_opts.subject_prefix = NULL; /* default to "[PATCH]" */
email_opts.signature = git_commit_author(commit);
email_opts.message = git_commit_summary(commit);

git_diff_commit(&diff, commit, NULL);
git_diff_format_email(&buf, diff, &email_opts);
git_diff_free(diff);

To me, this second alternative seems to fix more neatly into the existing namespaces. However, I recognize that it is much more verbose and there is some messiness in setting up the email options correctly in order to generate appropriate patches.

One thought, depending on whether you like or hate this alternative proposal would be to break things down a la the second proposal (which would give libgit2 users more flexibility in formatting various diffs for email, not just single commits), and also provide a git_diff_commit_as_email API that wrapped the 10 lines of code into a single convenience API.

[jacquesg]

Messiness is an understatement 😉. The part that took the longest was to print all the prettiness that core git does, took pretty much 85% of the time.

Moving it to git_diff_format_email sound good, tried to not "rock the boat" too much. The only thing about the above git_diff_format_email_options.subject_prefix that won't quite work is that depending on the patch count, it could be [PATCH] or [PATCH 1/2]. Leaving it as a boolean is more robust. Thoughts?

[jacquesg]

What I meant by it won't work is that it will be hard to inject the patch number and count if the prefix is provided by the user.

[arrbee]

Leaving it as a boolean is more robust. Thoughts?

Oh, that was just my overly quick reading of the meaning of the flag. Sorry! Keeping it a boolean is perfectly fine.

At a basic level, does this restructuring of the API otherwise seem to make sense? I have three motivations for proposing it:

1. It allows the user to more naturally pass options to the diff generation phase (which covers a ton of the command line options to git format-patch, such as running rename detection, etc.)
2. To me, it feels slightly less redundant with the (TBD) git_diff_commit API
3. Users could generate pretty emails for other diffs, such as collapsing a series of commits by generating the diff from an arbitrary pair of trees

By adding the email formatting options structure, we can (incrementally) add flags / fields to support --no-stat, --attach, --thread, etc. Obviously, the API doesn't require my proposed restructuring to achieve this, but it does distinguish between where we would use the existing git_diff_options and a new options structure with email details.

[jacquesg]

Genius! I'll make the changes. Maybe I got a little bit blinded by how close I am to this.

[jacquesg]

@arrbee Is this what you meant? Note that I have added git_diff__commit internally. It will return an error when encountering a merge commit which should be ok for now.

[jacquesg]

@arrbee So I'm trying to make this a little bit more generic, i.e. get --stat, --shortstat and --numstat information not only for format_email.

I've added a test case for a git_diff on which git_diff_find_similar has been invoked. What I'm seeing as that there are no hunks if the file was only renamed (no content change). The problem I've found is that its not really possible to report the line number statistics for these files. If the content has also changed, its not a problem.

Any suggestions or advice?

[jacquesg]

So I got this to do the right thing (still a bit of refactoring todo), except for binary files.

@arrbee I've added a test case to ensure --stat functionality works even for binary files, but it seems like we don't have the actual file size for the new file. Core git reports

binary.bin | Bin 3 -> 5 bytes whereas I'm getting binary.bin | Bin 3 -> 0 bytes. I'm using the size value from git_diff_file->size.

[vmg]

git_oid_tostr does the same thing as these two lines :)

[vmg]

...and it returns a pointer to the stack buffer so you can pass it directly in the varargs in giterr_set.

char buf[GIT_OID_HEXSZ + 1];
giterr_set(GITERR_INVALID, "Commit %s is a merge commit", git_commit_tostr(buf, 41, git_commit_id(commit)));

[jacquesg]

Sweet! I'll use that instead :)

[vmg]

This is brilliant work by the way. Just that small nitpick and I think this is ready to merge. :)

[jacquesg]

I'm still refactoring slightly, as I'm understanding the code base better :) I should finish it off tonight (I hope)

[jacquesg]

I'm happy with this, feel that the test cases cover everything.

[jacquesg]

Ready to merge?

[vmg]

I think so. :)

[vmg]

Lovely job again. Can't wait to see what's next. :)

[jacquesg]

🤘

[jacquesg]

Do we auto update the gh-pages? Looks like they are a little bit stale?

[arthurschreiber]

Summoning @carlosmn is the current way to "auto update the gh-pages". 😄

[jacquesg]

Haha, maybe some incarnation of a post-receive hook would do the trick, don't know how flexible github is with it :)

[arthurschreiber]

We could also use Travis to do that. :)