* [PATCH] gitweb: Do not show 'patch' link in 'commit' view for merges
@ 2009-09-30 20:21 Jakub Narebski
2009-10-01 3:11 ` Jeff King
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Narebski @ 2009-09-30 20:21 UTC (permalink / raw)
To: git
Show 'patch' link in the 'commit' view only for commits which have
exactly one parent, otherwise call to git-format-patch would fail for
the hyperlinked 'patch' action.
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Just noticed this issue, thanks indirectly to
"Re: How can I download a git commit as a diff patch?"
thread.
This is conservative change: perhaps we could extend 'patch' view to
work also for root commit...
gitweb/gitweb.perl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 24b2193..fb045a1 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5328,7 +5328,7 @@ sub git_commit {
} @$parents ) .
')';
}
- if (gitweb_check_feature('patches')) {
+ if (gitweb_check_feature('patches') && @$parents == 1) {
$formats_nav .= " | " .
$cgi->a({-href => href(action=>"patch", -replay=>1)},
"patch");
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] gitweb: Do not show 'patch' link in 'commit' view for merges
2009-09-30 20:21 [PATCH] gitweb: Do not show 'patch' link in 'commit' view for merges Jakub Narebski
@ 2009-10-01 3:11 ` Jeff King
2009-10-01 7:36 ` Jakub Narebski
0 siblings, 1 reply; 10+ messages in thread
From: Jeff King @ 2009-10-01 3:11 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
On Wed, Sep 30, 2009 at 10:21:53PM +0200, Jakub Narebski wrote:
> Show 'patch' link in the 'commit' view only for commits which have
> exactly one parent, otherwise call to git-format-patch would fail for
> the hyperlinked 'patch' action.
Fail in what way? From my cursory reading of the code, it looks like the
'patch' action calls into git_commitdiff, which handles the multi-parent
case.
I assume I'm reading wrong, since you obviously know gitweb much better
than I do. :) But can you expand a little on the nature of the problem
in the commit message?
-Peff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gitweb: Do not show 'patch' link in 'commit' view for merges
2009-10-01 3:11 ` Jeff King
@ 2009-10-01 7:36 ` Jakub Narebski
2009-10-01 7:55 ` Jeff King
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Narebski @ 2009-10-01 7:36 UTC (permalink / raw)
To: Jeff King; +Cc: git
Dnia czwartek 1. października 2009 05:11, Jeff King napisał:
> On Wed, Sep 30, 2009 at 10:21:53PM +0200, Jakub Narebski wrote:
>
> > Show 'patch' link in the 'commit' view only for commits which have
> > exactly one parent, otherwise call to git-format-patch would fail for
> > the hyperlinked 'patch' action.
>
> Fail in what way? From my cursory reading of the code, it looks like the
> 'patch' action calls into git_commitdiff, which handles the multi-parent
> case.
>
> I assume I'm reading wrong, since you obviously know gitweb much better
> than I do. :) But can you expand a little on the nature of the problem
> in the commit message?
Well, from the point of view of behavior, 'patch' link in 'commit' view
for a merge commit, e.g.
gitweb.cgi/git.git/commit/833423ae071ffedb7fbca39789f14f9a45a3d1c4
leads to the 'patch' view
git.git/patch/833423ae071ffedb7fbca39789f14f9a45a3d1c4
which leads to 'text/plain' output with the following contents:
Reading git-format-patch failed
From the point of view of code, 'patch' view is handled by git_patch()
subroutine, which in turn calls git_commitdiff(-format => 'patch', -single=> 1);
git_commitdiff checks if 'patch' view is enabled feature, and then
composes and calls the following command (I have skipped --git-dir=...):
git format-patch --encoding=utf8 --stdout -1 --root <commit-id>
And git-format-patch produces no output for merge commit. Then
git_commitdiff dumps output of git-format-patch
local $/ = undef;
print <$fd>;
and somehow fails on closing filehandle
close $fd
or print "Reading git-format-patch failed\n";
Even if 'patch' view didn't fail, it is not a good idea to have link
to an empty page (or page with only error message). Though probably
git_commitdiff could check if it is used for a merge commit...
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gitweb: Do not show 'patch' link in 'commit' view for merges
2009-10-01 7:36 ` Jakub Narebski
@ 2009-10-01 7:55 ` Jeff King
2009-10-01 9:18 ` Jakub Narebski
0 siblings, 1 reply; 10+ messages in thread
From: Jeff King @ 2009-10-01 7:55 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
On Thu, Oct 01, 2009 at 09:36:23AM +0200, Jakub Narebski wrote:
> From the point of view of code, 'patch' view is handled by git_patch()
> subroutine, which in turn calls git_commitdiff(-format => 'patch', -single=> 1);
> git_commitdiff checks if 'patch' view is enabled feature, and then
> composes and calls the following command (I have skipped --git-dir=...):
>
> git format-patch --encoding=utf8 --stdout -1 --root <commit-id>
>
> And git-format-patch produces no output for merge commit. Then
> git_commitdiff dumps output of git-format-patch
Ah, OK, I see the code path you are talking about now. My first thought
was "shouldn't it be handling the merge case?" but that really doesn't
make any sense. The "patch" view is just about something that can be
given to "git am", and it doesn't understand merges anyway. And the
"commitdiff" and "html" formats already handle it appropriately.
So I think your patch is the right thing to do. Thanks for the
explanation.
-Peff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gitweb: Do not show 'patch' link in 'commit' view for merges
2009-10-01 7:55 ` Jeff King
@ 2009-10-01 9:18 ` Jakub Narebski
2009-10-09 12:10 ` [PATCH v2] gitweb: Do not show 'patch' link for merge commits Jakub Narebski
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Narebski @ 2009-10-01 9:18 UTC (permalink / raw)
To: Jeff King; +Cc: git
Dnia czwartek 1. października 2009 09:55, Jeff King napisał:
> On Thu, Oct 01, 2009 at 09:36:23AM +0200, Jakub Narebski wrote:
>
> > From the point of view of code, 'patch' view is handled by git_patch()
> > subroutine, which in turn calls git_commitdiff(-format => 'patch', -single=> 1);
> > git_commitdiff checks if 'patch' view is enabled feature, and then
> > composes and calls the following command (I have skipped --git-dir=...):
> >
> > git format-patch --encoding=utf8 --stdout -1 --root <commit-id>
> >
> > And git-format-patch produces no output for merge commit. Then
> > git_commitdiff dumps output of git-format-patch
>
> Ah, OK, I see the code path you are talking about now. My first thought
> was "shouldn't it be handling the merge case?" but that really doesn't
> make any sense. The "patch" view is just about something that can be
> given to "git am", and it doesn't understand merges anyway. And the
> "commitdiff" and "html" formats already handle it appropriately.
>
> So I think your patch is the right thing to do. Thanks for the
> explanation.
I'll enhance commit message to talk about issues touched here (that
'patch' is for git-am and doesn't make sense for merges, regardless
of possible bug in 'patch' view when used for merge commits), and
check if this correction isn't needed also elsewhere.
I'll send v2 of this patch in a bit.
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] gitweb: Do not show 'patch' link for merge commits
2009-10-01 9:18 ` Jakub Narebski
@ 2009-10-09 12:10 ` Jakub Narebski
2009-10-09 12:23 ` Jakub Narebski
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Narebski @ 2009-10-09 12:10 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio Hamano
The 'patch' view is about generating text/plain patch that can be
given to "git am", and "git am" doesn't understand merges anyway.
Therefore link to 'patch' view should not be shown for merge commits.
Also call to git-format-patch inside the 'patch' action would fail
when 'patch' action is called for a merge commit, with "Reading
git-format-patch failed" text as 'patch' view body.
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
On Thu, 1 Oct 2009, Jakub Narebski wrote:
> I'll send v2 of this patch in a bit.
Here it is.
Changes since v1:
* Do not show 'patch' link for merge commits not only in 'commit'
view, but also in 'commitdiff' view (more complete)
* 'patch' link is shown also for root (parentless) commits; it
works correctly thanks to passing '--root' option to git-format-patch
(remove unnecessary restriction)
* better commit message thanks to discussion with Jeff King
gitweb/gitweb.perl | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 24b2193..14f31dc 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5328,7 +5328,7 @@ sub git_commit {
} @$parents ) .
')';
}
- if (gitweb_check_feature('patches')) {
+ if (gitweb_check_feature('patches') && @$parents <= 1) {
$formats_nav .= " | " .
$cgi->a({-href => href(action=>"patch", -replay=>1)},
"patch");
@@ -5616,7 +5616,7 @@ sub git_commitdiff {
$formats_nav =
$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
"raw");
- if ($patch_max) {
+ if ($patch_max && && @{$co{'parents'}} <= 1) {
$formats_nav .= " | " .
$cgi->a({-href => href(action=>"patch", -replay=>1)},
"patch");
@@ -5824,7 +5824,7 @@ sub git_commitdiff_plain {
# format-patch-style patches
sub git_patch {
- git_commitdiff(-format => 'patch', -single=> 1);
+ git_commitdiff(-format => 'patch', -single => 1);
}
sub git_patches {
--
1.6.4.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] gitweb: Do not show 'patch' link for merge commits
2009-10-09 12:10 ` [PATCH v2] gitweb: Do not show 'patch' link for merge commits Jakub Narebski
@ 2009-10-09 12:23 ` Jakub Narebski
2009-10-09 12:26 ` [PATCH v2 (amend)] " Jakub Narebski
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Narebski @ 2009-10-09 12:23 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio Hamano
Jakub Narebski wrote:
> @@ -5616,7 +5616,7 @@ sub git_commitdiff {
> $formats_nav =
> $cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
> "raw");
> - if ($patch_max) {
> + if ($patch_max && && @{$co{'parents'}} <= 1) {
Gaaaah. It should be "&&", not "&& &&". I'm extremely sorry!
> $formats_nav .= " | " .
> $cgi->a({-href => href(action=>"patch", -replay=>1)},
> "patch");
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 (amend)] gitweb: Do not show 'patch' link for merge commits
2009-10-09 12:23 ` Jakub Narebski
@ 2009-10-09 12:26 ` Jakub Narebski
2009-10-09 19:19 ` Jeff King
2009-10-10 0:56 ` Junio C Hamano
0 siblings, 2 replies; 10+ messages in thread
From: Jakub Narebski @ 2009-10-09 12:26 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio Hamano
The 'patch' view is about generating text/plain patch that can be
given to "git am", and "git am" doesn't understand merges anyway.
Therefore link to 'patch' view should not be shown for merge commits.
Also call to git-format-patch inside the 'patch' action would fail
when 'patch' action is called for a merge commit, with "Reading
git-format-patch failed" text as 'patch' view body.
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Changes since v1:
* Do not show 'patch' link for merge commits not only in 'commit'
view, but also in 'commitdiff' view (more complete)
* 'patch' link is shown also for root (parentless) commits; it
works correctly thanks to passing '--root' option to git-format-patch
(remove unnecessary restriction)
* better commit message thanks to discussion with Jeff King
gitweb/gitweb.perl | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 24b2193..14f31dc 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5328,7 +5328,7 @@ sub git_commit {
} @$parents ) .
')';
}
- if (gitweb_check_feature('patches')) {
+ if (gitweb_check_feature('patches') && @$parents <= 1) {
$formats_nav .= " | " .
$cgi->a({-href => href(action=>"patch", -replay=>1)},
"patch");
@@ -5616,7 +5616,7 @@ sub git_commitdiff {
$formats_nav =
$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
"raw");
- if ($patch_max) {
+ if ($patch_max && @{$co{'parents'}} <= 1) {
$formats_nav .= " | " .
$cgi->a({-href => href(action=>"patch", -replay=>1)},
"patch");
@@ -5824,7 +5824,7 @@ sub git_commitdiff_plain {
# format-patch-style patches
sub git_patch {
- git_commitdiff(-format => 'patch', -single=> 1);
+ git_commitdiff(-format => 'patch', -single => 1);
}
sub git_patches {
--
1.6.4.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 (amend)] gitweb: Do not show 'patch' link for merge commits
2009-10-09 12:26 ` [PATCH v2 (amend)] " Jakub Narebski
@ 2009-10-09 19:19 ` Jeff King
2009-10-10 0:56 ` Junio C Hamano
1 sibling, 0 replies; 10+ messages in thread
From: Jeff King @ 2009-10-09 19:19 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Junio Hamano
On Fri, Oct 09, 2009 at 02:26:44PM +0200, Jakub Narebski wrote:
> Changes since v1:
> * Do not show 'patch' link for merge commits not only in 'commit'
> view, but also in 'commitdiff' view (more complete)
> * 'patch' link is shown also for root (parentless) commits; it
> works correctly thanks to passing '--root' option to git-format-patch
> (remove unnecessary restriction)
> * better commit message thanks to discussion with Jeff King
Thanks. I am not a gitweb expert, but those changes look good to me.
-Peff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 (amend)] gitweb: Do not show 'patch' link for merge commits
2009-10-09 12:26 ` [PATCH v2 (amend)] " Jakub Narebski
2009-10-09 19:19 ` Jeff King
@ 2009-10-10 0:56 ` Junio C Hamano
1 sibling, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2009-10-10 0:56 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Jeff King, Junio Hamano
Thanks.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2009-10-10 1:02 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-09-30 20:21 [PATCH] gitweb: Do not show 'patch' link in 'commit' view for merges Jakub Narebski
2009-10-01 3:11 ` Jeff King
2009-10-01 7:36 ` Jakub Narebski
2009-10-01 7:55 ` Jeff King
2009-10-01 9:18 ` Jakub Narebski
2009-10-09 12:10 ` [PATCH v2] gitweb: Do not show 'patch' link for merge commits Jakub Narebski
2009-10-09 12:23 ` Jakub Narebski
2009-10-09 12:26 ` [PATCH v2 (amend)] " Jakub Narebski
2009-10-09 19:19 ` Jeff King
2009-10-10 0:56 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).