RFE: download patch between arbitrary revisions

RFE: download patch between arbitrary revisions

[mricon]

Hi, all:

There is currently a way to render a diff between two arbitrary objects,
e.g.:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/diff/?id=v3.10-rc4&id2=v3.10-rc3

However, there doesn't appear to be a way to download a patch in the
same way -- it will only make patch against id's parent. E.g.:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/patch/?id=v3.10-rc4&id2=v3.10-rc3

Any way we can make the behaviour of patch match that of diff?

Best,
-- 
Konstantin Ryabitsev
Senior Systems Administrator
Linux Foundation Collab Projects
Montr?al, Qu?bec

RFE: download patch between arbitrary revisions

[john]

On Sun, Jun 02, 2013 at 09:19:11PM -0400, Konstantin Ryabitsev wrote:
> There is currently a way to render a diff between two arbitrary objects,
> e.g.:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/diff/?id=v3.10-rc4&id2=v3.10-rc3
> 
> However, there doesn't appear to be a way to download a patch in the
> same way -- it will only make patch against id's parent. E.g.:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/patch/?id=v3.10-rc4&id2=v3.10-rc3
> 
> Any way we can make the behaviour of patch match that of diff?

I had a quick look at this and changing the patch output is really easy,
but the top of the output is now completely wrong since it displays the
message from the "id" commit.  I'm not sure what to do about this;
clearly it is useful to be able to get the patch output between two
arbitrary points in a raw format (not HTML) but I don't know what to do
about the commit message and headers.  Perhaps we can do something like:

    if "id2" is specified:
        print shortlog output
    else:
        do what we currently do

but I don't know how much code would be needed for that.

Here's the patch in case anyone wants to try it (slightly bigger than
strictly necessary because I renamed hex -> new_rev for consistency with
cgit_print_diff):

-- >8 --
diff --git a/cmd.c b/cmd.c
index abe8e46..7e8b168 100644
--- a/cmd.c
+++ b/cmd.c
@@ -93,7 +93,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 fbb92cc..ff8ee34 100644
--- a/ui-patch.c
+++ b/ui-patch.c
@@ -83,28 +83,30 @@ static void filepair_cb(struct diff_filepair *pair)
 		html("Binary files differ\n");
 }
 
-void cgit_print_patch(char *hex, const char *prefix)
+void cgit_print_patch(const char *new_rev, const char *old_rev, const char *prefix)
 {
 	struct commit *commit;
 	struct commitinfo *info;
 	unsigned char sha1[20], old_sha1[20];
 	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, sha1)) {
+		cgit_print_error("Bad object id: %s", new_rev);
 		return;
 	}
 	commit = lookup_commit_reference(sha1);
 	if (!commit) {
-		cgit_print_error("Bad commit reference: %s", hex);
+		cgit_print_error("Bad commit reference: %s", new_rev);
 		return;
 	}
 	info = cgit_parse_commit(commit);
 
-	if (commit->parents && commit->parents->item)
+	if (old_rev)
+		get_sha1(old_rev, old_sha1);
+	else if (commit->parents && commit->parents->item)
 		hashcpy(old_sha1, commit->parents->item->object.sha1);
 	else
 		hashclr(old_sha1);
diff --git a/ui-patch.h b/ui-patch.h
index 1641cea..e833042 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(const char *new_rev, const char *old_rev, const char *prefix);
 
 #endif /* UI_PATCH_H */

RFE: download patch between arbitrary revisions

[cgit]

On Mon, Jun 03, 2013 at 07:49:53PM +0100, John Keeping wrote:
> On Sun, Jun 02, 2013 at 09:19:11PM -0400, Konstantin Ryabitsev wrote:
> > There is currently a way to render a diff between two arbitrary objects,
> > e.g.:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/diff/?id=v3.10-rc4&id2=v3.10-rc3
> > 
> > However, there doesn't appear to be a way to download a patch in the
> > same way -- it will only make patch against id's parent. E.g.:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/patch/?id=v3.10-rc4&id2=v3.10-rc3
> > 
> > Any way we can make the behaviour of patch match that of diff?
> 
> I had a quick look at this and changing the patch output is really easy,
> but the top of the output is now completely wrong since it displays the
> message from the "id" commit.  I'm not sure what to do about this;
> clearly it is useful to be able to get the patch output between two
> arbitrary points in a raw format (not HTML) but I don't know what to do
> about the commit message and headers.  Perhaps we can do something like:
> 
>     if "id2" is specified:
>         print shortlog output
>     else:
>         do what we currently do
> 
> but I don't know how much code would be needed for that.

Well, the problem, obviously, is that we are currently using
git-format-patch(1)-style patches for single commits. There are three
possible solutions that come into my mind: 

1. Use a plain diff for multiple patches (like `git diff HEAD~10..`).

2. Create a patch for a squash commit (like `git merge --squash`
   followed by `git format-patch`). Basically what John suggested above.

3. Create a bunch of `git am` compatible patches (like `git format-patch
   --stdout HEAD~10..`).

I personally think that it might be best to create a new parameter (or
an entirely new URL) to switch between creating plain diffs (see 1) and
git-format-patch(1)-like patches (see 3). That way, we would be able to
provide both patches that mirror exactly what is in the repos and simple
patches that summarize a couple of commits.

> 
> Here's the patch in case anyone wants to try it (slightly bigger than
> strictly necessary because I renamed hex -> new_rev for consistency with
> cgit_print_diff):
> 
> [...]

RFE: download patch between arbitrary revisions

[john]

On Thu, Jun 13, 2013 at 11:58:10PM +0200, Lukas Fleischer wrote:
> On Mon, Jun 03, 2013 at 07:49:53PM +0100, John Keeping wrote:
> > On Sun, Jun 02, 2013 at 09:19:11PM -0400, Konstantin Ryabitsev wrote:
> > > There is currently a way to render a diff between two arbitrary objects,
> > > e.g.:
> > > 
> > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/diff/?id=v3.10-rc4&id2=v3.10-rc3
> > > 
> > > However, there doesn't appear to be a way to download a patch in the
> > > same way -- it will only make patch against id's parent. E.g.:
> > > 
> > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/patch/?id=v3.10-rc4&id2=v3.10-rc3
> > > 
> > > Any way we can make the behaviour of patch match that of diff?
> > 
> > I had a quick look at this and changing the patch output is really easy,
> > but the top of the output is now completely wrong since it displays the
> > message from the "id" commit.  I'm not sure what to do about this;
> > clearly it is useful to be able to get the patch output between two
> > arbitrary points in a raw format (not HTML) but I don't know what to do
> > about the commit message and headers.  Perhaps we can do something like:
> > 
> >     if "id2" is specified:
> >         print shortlog output
> >     else:
> >         do what we currently do
> > 
> > but I don't know how much code would be needed for that.
> 
> Well, the problem, obviously, is that we are currently using
> git-format-patch(1)-style patches for single commits. There are three
> possible solutions that come into my mind: 
> 
> 1. Use a plain diff for multiple patches (like `git diff HEAD~10..`).
> 
> 2. Create a patch for a squash commit (like `git merge --squash`
>    followed by `git format-patch`). Basically what John suggested above.
> 
> 3. Create a bunch of `git am` compatible patches (like `git format-patch
>    --stdout HEAD~10..`).
> 
> I personally think that it might be best to create a new parameter (or
> an entirely new URL) to switch between creating plain diffs (see 1) and
> git-format-patch(1)-like patches (see 3). That way, we would be able to
> provide both patches that mirror exactly what is in the repos and simple
> patches that summarize a couple of commits.

Sounds sensible.  How about making "patch" do the "format-patch
--stdout" behaviour and add a new "raw diff" command that just shows the
diff?

So:

    /patch/?id=v1.8.3.1&id2=v1.8.3

generates something like "git format-patch v1.8.3..v1.8.3.1".  And

    /rawdiff/?id=v1.8.3.1&id2=v1.8.3

generates the diff with no HTML around it.

The latter could be a query parameter instead ("raw=1"?).  Having not
investigated the impact on the code, I have no preference for one over
the other.

RFE: download patch between arbitrary revisions

[cgit]

On Thu, Jun 13, 2013 at 11:21:26PM +0100, John Keeping wrote:
> On Thu, Jun 13, 2013 at 11:58:10PM +0200, Lukas Fleischer wrote:
> > On Mon, Jun 03, 2013 at 07:49:53PM +0100, John Keeping wrote:
> > > On Sun, Jun 02, 2013 at 09:19:11PM -0400, Konstantin Ryabitsev wrote:
> > > > There is currently a way to render a diff between two arbitrary objects,
> > > > e.g.:
> > > > 
> > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/diff/?id=v3.10-rc4&id2=v3.10-rc3
> > > > 
> > > > However, there doesn't appear to be a way to download a patch in the
> > > > same way -- it will only make patch against id's parent. E.g.:
> > > > 
> > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/patch/?id=v3.10-rc4&id2=v3.10-rc3
> > > > 
> > > > Any way we can make the behaviour of patch match that of diff?
> > > 
> [...]
> 
> So:
> 
>     /patch/?id=v1.8.3.1&id2=v1.8.3
> 
> generates something like "git format-patch v1.8.3..v1.8.3.1".  And
> 
>     /rawdiff/?id=v1.8.3.1&id2=v1.8.3
> 
> generates the diff with no HTML around it.
> 
> The latter could be a query parameter instead ("raw=1"?).  Having not
> investigated the impact on the code, I have no preference for one over
> the other.

Sounds good to me either way. Jason?

Also, I just wondered whether this could be used to DoS servers when
requesting a series of patches for a huge range of revisions... Maybe
there should be some kind of limit?

RFE: download patch between arbitrary revisions

[john]

On Sun, Jun 16, 2013 at 09:56:55AM +0200, Lukas Fleischer wrote:
> On Thu, Jun 13, 2013 at 11:21:26PM +0100, John Keeping wrote:
> > On Thu, Jun 13, 2013 at 11:58:10PM +0200, Lukas Fleischer wrote:
> > > On Mon, Jun 03, 2013 at 07:49:53PM +0100, John Keeping wrote:
> > > > On Sun, Jun 02, 2013 at 09:19:11PM -0400, Konstantin Ryabitsev wrote:
> > > > > There is currently a way to render a diff between two arbitrary objects,
> > > > > e.g.:
> > > > > 
> > > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/diff/?id=v3.10-rc4&id2=v3.10-rc3
> > > > > 
> > > > > However, there doesn't appear to be a way to download a patch in the
> > > > > same way -- it will only make patch against id's parent. E.g.:
> > > > > 
> > > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/patch/?id=v3.10-rc4&id2=v3.10-rc3
> > > > > 
> > > > > Any way we can make the behaviour of patch match that of diff?
> > > > 
> > [...]
> > 
> > So:
> > 
> >     /patch/?id=v1.8.3.1&id2=v1.8.3
> > 
> > generates something like "git format-patch v1.8.3..v1.8.3.1".  And
> > 
> >     /rawdiff/?id=v1.8.3.1&id2=v1.8.3
> > 
> > generates the diff with no HTML around it.
> > 
> > The latter could be a query parameter instead ("raw=1"?).  Having not
> > investigated the impact on the code, I have no preference for one over
> > the other.
> 
> Sounds good to me either way. Jason?
> 
> Also, I just wondered whether this could be used to DoS servers when
> requesting a series of patches for a huge range of revisions... Maybe
> there should be some kind of limit?

The "diff" version costs about the same regardless of how many revisions
are between the endpoints - it's purely a function of the size of the
tree (and the number of changed files which probably is affected by how
far apart the commits are, but it's still bounded by the size of the
tree).

I can see it potentially being a problem in the "format-patch" case, so
perhaps we want to add a "max-patches-per-request" variable if we do
implement that.

RFE: download patch between arbitrary revisions

[cgit]

On Sun, Jun 16, 2013 at 11:11:22AM +0100, John Keeping wrote:
> On Sun, Jun 16, 2013 at 09:56:55AM +0200, Lukas Fleischer wrote:
> > On Thu, Jun 13, 2013 at 11:21:26PM +0100, John Keeping wrote:
> > > On Thu, Jun 13, 2013 at 11:58:10PM +0200, Lukas Fleischer wrote:
> > > > On Mon, Jun 03, 2013 at 07:49:53PM +0100, John Keeping wrote:
> > > > > On Sun, Jun 02, 2013 at 09:19:11PM -0400, Konstantin Ryabitsev wrote:
> > > > > > There is currently a way to render a diff between two arbitrary objects,
> > > > > > e.g.:
> > > > > > 
> > > > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/diff/?id=v3.10-rc4&id2=v3.10-rc3
> > > > > > 
> > > > > > However, there doesn't appear to be a way to download a patch in the
> > > > > > same way -- it will only make patch against id's parent. E.g.:
> > > > > > 
> > > > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/patch/?id=v3.10-rc4&id2=v3.10-rc3
> > > > > > 
> > > > > > Any way we can make the behaviour of patch match that of diff?
> > > > > 
> > > [...]
> > > 
> > > So:
> > > 
> > >     /patch/?id=v1.8.3.1&id2=v1.8.3
> > > 
> > > generates something like "git format-patch v1.8.3..v1.8.3.1".  And
> > > 
> > >     /rawdiff/?id=v1.8.3.1&id2=v1.8.3
> > > 
> > > generates the diff with no HTML around it.
> > > 
> > > The latter could be a query parameter instead ("raw=1"?).  Having not
> > > investigated the impact on the code, I have no preference for one over
> > > the other.
> > 
> > Sounds good to me either way. Jason?
> > 
> > Also, I just wondered whether this could be used to DoS servers when
> > requesting a series of patches for a huge range of revisions... Maybe
> > there should be some kind of limit?
> 
> The "diff" version costs about the same regardless of how many revisions
> are between the endpoints - it's purely a function of the size of the
> tree (and the number of changed files which probably is affected by how
> far apart the commits are, but it's still bounded by the size of the
> tree).
> 
> I can see it potentially being a problem in the "format-patch" case, so
> perhaps we want to add a "max-patches-per-request" variable if we do
> implement that.

Yeah, the `git format-patch` case is what I meant with "requesting a
series of patches". Limiting the number of patches also sounds good to
me.

I will go ahead and implement this if Jason likes the idea :)

RFE: download patch between arbitrary revisions

[mricon]

On 16/06/13 03:18 PM, Lukas Fleischer wrote:
>>>>>>> There is currently a way to render a diff between two arbitrary objects,
>>>>>>> e.g.:
>>>>>>>
>>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/diff/?id=v3.10-rc4&id2=v3.10-rc3
>>>>>>>
>>>>>>> However, there doesn't appear to be a way to download a patch in the
>>>>>>> same way -- it will only make patch against id's parent. E.g.:
>>>>>>>
>>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/patch/?id=v3.10-rc4&id2=v3.10-rc3
>>>>>>>
>>>>>>> Any way we can make the behaviour of patch match that of diff?
>>>>>>
>>>> [...]
>>>>
>>>> So:
>>>>
>>>>     /patch/?id=v1.8.3.1&id2=v1.8.3
>>>>
>>>> generates something like "git format-patch v1.8.3..v1.8.3.1".  And
>>>>
>>>>     /rawdiff/?id=v1.8.3.1&id2=v1.8.3
>>>>
>>>> generates the diff with no HTML around it.
>>>>
>>>> The latter could be a query parameter instead ("raw=1"?).  Having not
>>>> investigated the impact on the code, I have no preference for one over
>>>> the other.
>>>
>>> Sounds good to me either way. Jason?
>>>
>>> Also, I just wondered whether this could be used to DoS servers when
>>> requesting a series of patches for a huge range of revisions... Maybe
>>> there should be some kind of limit?
>>
>> The "diff" version costs about the same regardless of how many revisions
>> are between the endpoints - it's purely a function of the size of the
>> tree (and the number of changed files which probably is affected by how
>> far apart the commits are, but it's still bounded by the size of the
>> tree).
>>
>> I can see it potentially being a problem in the "format-patch" case, so
>> perhaps we want to add a "max-patches-per-request" variable if we do
>> implement that.
> 
> Yeah, the `git format-patch` case is what I meant with "requesting a
> series of patches". Limiting the number of patches also sounds good to
> me.
> 
> I will go ahead and implement this if Jason likes the idea :)

Hi, all:

We keep getting requests for this feature, so I'm going to bump this in
hopes that this gets implemented in the next release. :)

Best,
-- 
Konstantin Ryabitsev
Senior Systems Administrator
Linux Foundation Collab Projects
Montr?al, Qu?bec

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 730 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20130806/40433f17/attachment.asc>

RFE: download patch between arbitrary revisions

[cgit]

On Tue, Aug 06, 2013 at 03:23:29PM -0400, Konstantin Ryabitsev wrote:
> On 16/06/13 03:18 PM, Lukas Fleischer wrote:
> >>>>>>> There is currently a way to render a diff between two arbitrary objects,
> >>>>>>> e.g.:
> >>>>>>>
> >>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/diff/?id=v3.10-rc4&id2=v3.10-rc3
> >>>>>>>
> >>>>>>> However, there doesn't appear to be a way to download a patch in the
> >>>>>>> same way -- it will only make patch against id's parent. E.g.:
> >>>>>>>
> >>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/patch/?id=v3.10-rc4&id2=v3.10-rc3
> >>>>>>>
> >>>>>>> Any way we can make the behaviour of patch match that of diff?
> >>>>>>
> >>>> [...]
> >>>>
> >>>> So:
> >>>>
> >>>>     /patch/?id=v1.8.3.1&id2=v1.8.3
> >>>>
> >>>> generates something like "git format-patch v1.8.3..v1.8.3.1".  And
> >>>>
> >>>>     /rawdiff/?id=v1.8.3.1&id2=v1.8.3
> >>>>
> >>>> generates the diff with no HTML around it.
> >>>>
> >>>> The latter could be a query parameter instead ("raw=1"?).  Having not
> >>>> investigated the impact on the code, I have no preference for one over
> >>>> the other.
> >>>
> >>> Sounds good to me either way. Jason?
> >>>
> >>> Also, I just wondered whether this could be used to DoS servers when
> >>> requesting a series of patches for a huge range of revisions... Maybe
> >>> there should be some kind of limit?
> >>
> >> The "diff" version costs about the same regardless of how many revisions
> >> are between the endpoints - it's purely a function of the size of the
> >> tree (and the number of changed files which probably is affected by how
> >> far apart the commits are, but it's still bounded by the size of the
> >> tree).
> >>
> >> I can see it potentially being a problem in the "format-patch" case, so
> >> perhaps we want to add a "max-patches-per-request" variable if we do
> >> implement that.
> > 
> > Yeah, the `git format-patch` case is what I meant with "requesting a
> > series of patches". Limiting the number of patches also sounds good to
> > me.
> > 
> > I will go ahead and implement this if Jason likes the idea :)
> 
> Hi, all:
> 
> We keep getting requests for this feature, so I'm going to bump this in
> hopes that this gets implemented in the next release. :)

Yeah, I already started writing a patch -- just waiting for Jason's
approval since it might take some time to polish it and make it ready
for submission.

> 
> Best,
> -- 
> Konstantin Ryabitsev
> Senior Systems Administrator
> Linux Foundation Collab Projects
> Montr?al, Qu?bec
> 

RFE: download patch between arbitrary revisions

[Jason]

This is a wonderful idea.

The sequence of patches provides a somewhat ridiculous way of
reassembling a git repository from cgit without using the proper http
clone endpoints; I wonder how long it will be before somebody winds up
(ab)using this for this purpose. A max-patches-per-request could
really help to prevent the DoS. On the otherhand, aren't there are a
few other endpoints that can cause similar DoS? If so, maybe we ought
to save max-patches-per-request for another commit that handles other
DoS cases too, and perhaps if we're lucky then be able to consolidate
them into one configuration setting at that point --
max-operations-per-request.

Anyway, go ahead and implement it --

    * /patch/ will support id2 param and return format-patch --stdout id1..id2
    * /diff/ will be unchanged
    * /diff/?raw=1 will spit out the raw output

Sound good?

RFE: download patch between arbitrary revisions

[cgit]

On Mon, Aug 12, 2013 at 01:05:18PM -0600, Jason A. Donenfeld wrote:
> This is a wonderful idea.
> 
> The sequence of patches provides a somewhat ridiculous way of
> reassembling a git repository from cgit without using the proper http
> clone endpoints; I wonder how long it will be before somebody winds up
> (ab)using this for this purpose. A max-patches-per-request could
> really help to prevent the DoS. On the otherhand, aren't there are a
> few other endpoints that can cause similar DoS? If so, maybe we ought
> to save max-patches-per-request for another commit that handles other
> DoS cases too, and perhaps if we're lucky then be able to consolidate
> them into one configuration setting at that point --
> max-operations-per-request.
> 
> Anyway, go ahead and implement it --
> 
>     * /patch/ will support id2 param and return format-patch --stdout id1..id2
>     * /diff/ will be unchanged
>     * /diff/?raw=1 will spit out the raw output

I implemented it as /rawdiff/ for now since /diff/?raw=1 turned out to
be a bit more involved to implement. Up to now, the decision on whether
to include the HTML layout (including the header, navigation bar) or not
is solely based on the command ("patch", "diff", ...) that is used.

I can try to change this if you feel strongly about it.

> 
> Sound good?