[PATCH v2 0/4] `git format-patch --stdout id2..id`

[PATCH v2 0/4] `git format-patch --stdout id2..id`

[cgit]

Fixed two issues spotted by Jason's and John's. Still based on lf/diffs.

Lukas Fleischer (4):
  ui-diff: Check the return value of get_sha1()
  ui-patch.c: Use log_tree_commit() to generate diffs
  Allow for creating patch series
  ui-patch: Rename variables

 cmd.c      |  2 +-
 ui-diff.c  | 23 +++++++-----------
 ui-patch.c | 80 +++++++++++++++++++++++++++++++++++---------------------------
 ui-patch.h |  3 ++-
 4 files changed, 57 insertions(+), 51 deletions(-)

-- 
1.8.4.rc2.477.g1da3ebd

[PATCH v2 1/4] ui-diff: Check the return value of get_sha1()

[cgit]

Sync with what we do everywhere else and check the return value of
get_sha1() instead of calling sha1_object_info() to validate the object.
Note that we later call lookup_commit_reference(), which checks that
both SHA1 values refer to commits, anyway.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 ui-diff.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/ui-diff.c b/ui-diff.c
index 838db8c..1209c47 100644
--- a/ui-diff.c
+++ b/ui-diff.c
@@ -360,15 +360,11 @@ void cgit_print_diff_ctrls()
 void cgit_print_diff(const char *new_rev, const char *old_rev,
 		     const char *prefix, int show_ctrls, int raw)
 {
-	enum object_type type;
-	unsigned long size;
 	struct commit *commit, *commit2;
 
 	if (!new_rev)
 		new_rev = ctx.qry.head;
-	get_sha1(new_rev, new_rev_sha1);
-	type = sha1_object_info(new_rev_sha1, &size);
-	if (type == OBJ_BAD) {
+	if (get_sha1(new_rev, new_rev_sha1)) {
 		cgit_print_error("Bad object name: %s", new_rev);
 		return;
 	}
@@ -378,19 +374,18 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 		return;
 	}
 
-	if (old_rev)
-		get_sha1(old_rev, old_rev_sha1);
-	else if (commit->parents && commit->parents->item)
+	if (old_rev) {
+		if (get_sha1(old_rev, old_rev_sha1)) {
+			cgit_print_error("Bad object name: %s", old_rev);
+			return;
+		}
+	} else if (commit->parents && commit->parents->item) {
 		hashcpy(old_rev_sha1, commit->parents->item->object.sha1);
-	else
+	} else {
 		hashclr(old_rev_sha1);
+	}
 
 	if (!is_null_sha1(old_rev_sha1)) {
-		type = sha1_object_info(old_rev_sha1, &size);
-		if (type == OBJ_BAD) {
-			cgit_print_error("Bad object name: %s", sha1_to_hex(old_rev_sha1));
-			return;
-		}
 		commit2 = lookup_commit_reference(old_rev_sha1);
 		if (!commit2 || parse_commit(commit2)) {
 			cgit_print_error("Bad commit: %s", sha1_to_hex(old_rev_sha1));
-- 
1.8.4.rc2.477.g1da3ebd

[PATCH v2 2/4] ui-patch.c: Use log_tree_commit() to generate diffs

[cgit]

Instead of using our own formatting, use log_tree_commit() from Git to
create patches. This removes unnecessary duplicate code and also fixes a
bug with e-mail address formatting that existed in our own
implementation.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 ui-patch.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/ui-patch.c b/ui-patch.c
index 3922a44..e205fda 100644
--- a/ui-patch.c
+++ b/ui-patch.c
@@ -13,9 +13,11 @@
 
 void cgit_print_patch(char *hex, const char *prefix)
 {
+	struct rev_info rev;
 	struct commit *commit;
-	struct commitinfo *info;
 	unsigned char sha1[20], old_sha1[20];
+	char rev_range[2 * 40 + 3];
+	char *rev_argv[] = { NULL, "--reverse", rev_range };
 	char *patchname;
 
 	if (!hex)
@@ -30,36 +32,32 @@ void cgit_print_patch(char *hex, const char *prefix)
 		cgit_print_error("Bad commit reference: %s", hex);
 		return;
 	}
-	info = cgit_parse_commit(commit);
 
 	if (commit->parents && commit->parents->item)
 		hashcpy(old_sha1, commit->parents->item->object.sha1);
 	else
 		hashclr(old_sha1);
 
+	sprintf(rev_range, "%s..%s", sha1_to_hex(old_sha1), sha1_to_hex(sha1));
+
 	patchname = fmt("%s.patch", sha1_to_hex(sha1));
 	ctx.page.mimetype = "text/plain";
 	ctx.page.filename = patchname;
 	cgit_print_http_headers(&ctx);
-	htmlf("From %s Mon Sep 17 00:00:00 2001\n", sha1_to_hex(sha1));
-	htmlf("From: %s", info->author);
-	if (!ctx.cfg.noplainemail) {
-		htmlf(" %s", info->author_email);
-	}
-	html("\n");
-	html("Date: ");
-	cgit_print_date(info->author_date, "%a, %d %b %Y %H:%M:%S %z%n", ctx.cfg.local_time);
-	htmlf("Subject: %s\n\n", info->subject);
-	if (info->msg && *info->msg) {
-		htmlf("%s", info->msg);
-		if (info->msg[strlen(info->msg) - 1] != '\n')
-			html("\n");
+
+	init_revisions(&rev, NULL);
+	rev.abbrev = DEFAULT_ABBREV;
+	rev.commit_format = CMIT_FMT_EMAIL;
+	rev.verbose_header = 1;
+	rev.diff = 1;
+	rev.show_root_diff = 0;
+	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
+	setup_revisions(ARRAY_SIZE(rev_argv), (const char **)rev_argv, &rev,
+			NULL);
+	prepare_revision_walk(&rev);
+
+	while ((commit = get_revision(&rev)) != NULL) {
+		log_tree_commit(&rev, commit);
+		printf("--\ncgit %s\n", cgit_version);
 	}
-	html("---\n");
-	if (prefix)
-		htmlf("(limited to '%s')\n\n", prefix);
-	cgit_diff_tree(old_sha1, sha1, filepair_cb_raw, prefix, 0);
-	html("--\n");
-	htmlf("cgit %s\n", cgit_version);
-	cgit_free_commitinfo(info);
 }
-- 
1.8.4.rc2.477.g1da3ebd

[PATCH v2 3/4] Allow for creating patch series

[cgit]

This allows for specifying a revision range using the id2 parameter of
/patch/. The output that is produced is similar to

    $ git format-patch --stdout id2..id

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 cmd.c      |  2 +-
 ui-patch.c | 16 +++++++++++++---
 ui-patch.h |  2 +-
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/cmd.c b/cmd.c
index 9893710..0202917 100644
--- a/cmd.c
+++ b/cmd.c
@@ -98,7 +98,7 @@ static void repolist_fn(struct cgit_context *ctx)
 
 static void patch_fn(struct cgit_context *ctx)
 {
-	cgit_print_patch(ctx->qry.sha1, ctx->qry.path);
+	cgit_print_patch(ctx->qry.sha1, ctx->qry.sha2, ctx->qry.path);
 }
 
 static void plain_fn(struct cgit_context *ctx)
diff --git a/ui-patch.c b/ui-patch.c
index e205fda..79a6fa7 100644
--- a/ui-patch.c
+++ b/ui-patch.c
@@ -11,7 +11,7 @@
 #include "html.h"
 #include "ui-shared.h"
 
-void cgit_print_patch(char *hex, const char *prefix)
+void cgit_print_patch(char *hex, const char *old_rev, const char *prefix)
 {
 	struct rev_info rev;
 	struct commit *commit;
@@ -33,10 +33,20 @@ void cgit_print_patch(char *hex, const char *prefix)
 		return;
 	}
 
-	if (commit->parents && commit->parents->item)
+	if (old_rev) {
+		if (get_sha1(old_rev, old_sha1)) {
+			cgit_print_error("Bad object id: %s", old_rev);
+			return;
+		}
+		if (!lookup_commit_reference(old_sha1)) {
+			cgit_print_error("Bad commit reference: %s", old_rev);
+			return;
+		}
+	} else if (commit->parents && commit->parents->item) {
 		hashcpy(old_sha1, commit->parents->item->object.sha1);
-	else
+	} else {
 		hashclr(old_sha1);
+	}
 
 	sprintf(rev_range, "%s..%s", sha1_to_hex(old_sha1), sha1_to_hex(sha1));
 
diff --git a/ui-patch.h b/ui-patch.h
index 1641cea..acd37fd 100644
--- a/ui-patch.h
+++ b/ui-patch.h
@@ -1,6 +1,6 @@
 #ifndef UI_PATCH_H
 #define UI_PATCH_H
 
-extern void cgit_print_patch(char *hex, const char *prefix);
+extern void cgit_print_patch(char *hex, const char *old_rev, const char *prefix);
 
 #endif /* UI_PATCH_H */
-- 
1.8.4.rc2.477.g1da3ebd

[PATCH v2 4/4] ui-patch: Rename variables

[cgit]

Rename parameters and local variables to match those from ui-diff. Also,
convert a "char *" to "const char *".

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 ui-patch.c | 30 ++++++++++++++++--------------
 ui-patch.h |  3 ++-
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/ui-patch.c b/ui-patch.c
index 79a6fa7..eabea92 100644
--- a/ui-patch.c
+++ b/ui-patch.c
@@ -11,46 +11,48 @@
 #include "html.h"
 #include "ui-shared.h"
 
-void cgit_print_patch(char *hex, const char *old_rev, const char *prefix)
+void cgit_print_patch(const char *new_rev, const char *old_rev,
+		      const char *prefix)
 {
 	struct rev_info rev;
 	struct commit *commit;
-	unsigned char sha1[20], old_sha1[20];
+	unsigned char new_rev_sha1[20], old_rev_sha1[20];
 	char rev_range[2 * 40 + 3];
 	char *rev_argv[] = { NULL, "--reverse", rev_range };
 	char *patchname;
 
-	if (!hex)
-		hex = ctx.qry.head;
+	if (!new_rev)
+		new_rev = ctx.qry.head;
 
-	if (get_sha1(hex, sha1)) {
-		cgit_print_error("Bad object id: %s", hex);
+	if (get_sha1(new_rev, new_rev_sha1)) {
+		cgit_print_error("Bad object id: %s", new_rev);
 		return;
 	}
-	commit = lookup_commit_reference(sha1);
+	commit = lookup_commit_reference(new_rev_sha1);
 	if (!commit) {
-		cgit_print_error("Bad commit reference: %s", hex);
+		cgit_print_error("Bad commit reference: %s", new_rev);
 		return;
 	}
 
 	if (old_rev) {
-		if (get_sha1(old_rev, old_sha1)) {
+		if (get_sha1(old_rev, old_rev_sha1)) {
 			cgit_print_error("Bad object id: %s", old_rev);
 			return;
 		}
-		if (!lookup_commit_reference(old_sha1)) {
+		if (!lookup_commit_reference(old_rev_sha1)) {
 			cgit_print_error("Bad commit reference: %s", old_rev);
 			return;
 		}
 	} else if (commit->parents && commit->parents->item) {
-		hashcpy(old_sha1, commit->parents->item->object.sha1);
+		hashcpy(old_rev_sha1, commit->parents->item->object.sha1);
 	} else {
-		hashclr(old_sha1);
+		hashclr(old_rev_sha1);
 	}
 
-	sprintf(rev_range, "%s..%s", sha1_to_hex(old_sha1), sha1_to_hex(sha1));
+	sprintf(rev_range, "%s..%s", sha1_to_hex(old_rev_sha1),
+		sha1_to_hex(new_rev_sha1));
 
-	patchname = fmt("%s.patch", sha1_to_hex(sha1));
+	patchname = fmt("%s.patch", sha1_to_hex(new_rev_sha1));
 	ctx.page.mimetype = "text/plain";
 	ctx.page.filename = patchname;
 	cgit_print_http_headers(&ctx);
diff --git a/ui-patch.h b/ui-patch.h
index acd37fd..7a6cacd 100644
--- a/ui-patch.h
+++ b/ui-patch.h
@@ -1,6 +1,7 @@
 #ifndef UI_PATCH_H
 #define UI_PATCH_H
 
-extern void cgit_print_patch(char *hex, const char *old_rev, const char *prefix);
+extern void cgit_print_patch(const char *new_rev, const char *old_rev,
+			     const char *prefix);
 
 #endif /* UI_PATCH_H */
-- 
1.8.4.rc2.477.g1da3ebd

[PATCH v2 2/4] ui-patch.c: Use log_tree_commit() to generate diffs

[Jason]

On Thu, Aug 15, 2013 at 4:01 PM, Lukas Fleischer <cgit at cryptocrack.de>wrote:

> -       if (!ctx.cfg.noplainemail) {
> -               htmlf(" %s", info->author_email);
> -       }
>

Unfortunately this kills the noplainemail functionality:

> noplainemail::
>         If set to "1" showing full author email addresses will be disabled.
>         Default value: "0".


What to do here?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20130816/feccba66/attachment.html>

[PATCH v2 2/4] ui-patch.c: Use log_tree_commit() to generate diffs

[cgit]

On Fri, Aug 16, 2013 at 01:16:12PM -0600, Jason A. Donenfeld wrote:
> On Thu, Aug 15, 2013 at 4:01 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> 
>     -       if (!ctx.cfg.noplainemail) {
>     -               htmlf(" %s", info->author_email);
>     -       }
> 
> 
> Unfortunately this kills the noplainemail functionality:
> 
>     noplainemail::
>             If set to "1" showing full author email addresses will be disabled.
>             Default value: "0".
> 
> 
> What to do here? 

Not sure, but note that we didn't do this properly anyway. Even with
noplainemail enabled, we did not hide any email addresses appearing in
the commit message -- and a lot of projects use sign-offs. The whole
option makes no sense if we mask the author email addresses but keep
"Signed-off-by:" lines.

To come back to your question, I think we could probably use pretty
formats to achieve that. Other possibilities would involve manipulating
each commit buffer before calling log_tree_commit() or patching Git to
have an internal option to hide e-mail addresses. Not very elegant :)

> 
> 
> 

[PATCH v3 2/4] ui-patch.c: Use log_tree_commit() to generate diffs

[cgit]

Instead of using our own formatting, use log_tree_commit() from Git to
create patches. This removes unnecessary duplicate code and also fixes a
bug with e-mail address formatting that existed in our own
implementation.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
This should work with noplainemail. We should think about improving (or
dropping) noplainemail later but I think it is best not to change its
behavior in this patch series.

 ui-patch.c | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/ui-patch.c b/ui-patch.c
index 3922a44..c46a53e 100644
--- a/ui-patch.c
+++ b/ui-patch.c
@@ -13,9 +13,11 @@
 
 void cgit_print_patch(char *hex, const char *prefix)
 {
+	struct rev_info rev;
 	struct commit *commit;
-	struct commitinfo *info;
 	unsigned char sha1[20], old_sha1[20];
+	char rev_range[2 * 40 + 3];
+	char *rev_argv[] = { NULL, "--reverse", "--format=email", rev_range };
 	char *patchname;
 
 	if (!hex)
@@ -30,36 +32,37 @@ void cgit_print_patch(char *hex, const char *prefix)
 		cgit_print_error("Bad commit reference: %s", hex);
 		return;
 	}
-	info = cgit_parse_commit(commit);
 
 	if (commit->parents && commit->parents->item)
 		hashcpy(old_sha1, commit->parents->item->object.sha1);
 	else
 		hashclr(old_sha1);
 
+	sprintf(rev_range, "%s..%s", sha1_to_hex(old_sha1), sha1_to_hex(sha1));
+
 	patchname = fmt("%s.patch", sha1_to_hex(sha1));
 	ctx.page.mimetype = "text/plain";
 	ctx.page.filename = patchname;
 	cgit_print_http_headers(&ctx);
-	htmlf("From %s Mon Sep 17 00:00:00 2001\n", sha1_to_hex(sha1));
-	htmlf("From: %s", info->author);
-	if (!ctx.cfg.noplainemail) {
-		htmlf(" %s", info->author_email);
+
+	if (ctx.cfg.noplainemail) {
+		rev_argv[2] = "--format=format:From %H Mon Sep 17 00:00:00 "
+			      "2001%nFrom: %an%nDate: %aD%n%w(78,0,1)Subject: "
+			      "%s%n%n%w(0)%b";
 	}
-	html("\n");
-	html("Date: ");
-	cgit_print_date(info->author_date, "%a, %d %b %Y %H:%M:%S %z%n", ctx.cfg.local_time);
-	htmlf("Subject: %s\n\n", info->subject);
-	if (info->msg && *info->msg) {
-		htmlf("%s", info->msg);
-		if (info->msg[strlen(info->msg) - 1] != '\n')
-			html("\n");
+
+	init_revisions(&rev, NULL);
+	rev.abbrev = DEFAULT_ABBREV;
+	rev.verbose_header = 1;
+	rev.diff = 1;
+	rev.show_root_diff = 0;
+	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
+	setup_revisions(ARRAY_SIZE(rev_argv), (const char **)rev_argv, &rev,
+			NULL);
+	prepare_revision_walk(&rev);
+
+	while ((commit = get_revision(&rev)) != NULL) {
+		log_tree_commit(&rev, commit);
+		printf("--\ncgit %s\n", cgit_version);
 	}
-	html("---\n");
-	if (prefix)
-		htmlf("(limited to '%s')\n\n", prefix);
-	cgit_diff_tree(old_sha1, sha1, filepair_cb_raw, prefix, 0);
-	html("--\n");
-	htmlf("cgit %s\n", cgit_version);
-	cgit_free_commitinfo(info);
 }
-- 
1.8.4.rc2.477.g1da3ebd

[PATCH v3 2/4] ui-patch.c: Use log_tree_commit() to generate diffs

[Jason]

Great. This series is now in wip.

Test test test!

[PATCH v3 2/4] ui-patch.c: Use log_tree_commit() to generate diffs

[Jason]

http://git.zx2c4.com/cgit/patch/?id=wip&id2=master

This appears to work!


Apparently some tests need to be updated:

not ok 7 - generate patch for initial commit
#
#               cgit_query "url=foo/patch&id=$root" >tmp
#
not ok 8 - find `cgit` signature
#
#               tail -1 tmp | grep "^cgit"
#
# failed 2 among 8 test(s)