* Re: ETA for release of gjit 0.4?
From: Robin Rosenberg @ 2008-12-06 18:47 UTC (permalink / raw)
To: Farrukh Najmi; +Cc: git, Shawn O. Pearce
In-Reply-To: <49393BFC.6010606@wellfleetsoftware.com>
fredag 05 december 2008 15:34:36 skrev Farrukh Najmi:
>
> Dear colleagues,
>
> I am using jgit in my maven project. Since current version is a SNAPSHOT
> (0.4-SNAPSHOT) I cannot release my project with a SNAPSHOT dependency
> (maven does not allow it). WHat is the time line for releasing version
> 0.4 of jgit so I can plan accordingly.
> Thanks for any info.
What? Plan? :)
You could name the version something else like 0.3.1.42b27c
It was a long time since we labeled anything. Shawn, how about merging
my recent close-file-patches, reverting 3ffa47d9294086fbd1cdeb9b1564f922a23e3c6f
and e7307f14c531d52cf231c39d844841c4adaf5e5a and then just call i 0.4 ?
-- robin
# from my pu branch, the latest Tentative build in the update site. Works quite well for
me. Refactoring and
$ git log --pretty=oneline origin/master..pu
2aafc054446f9b3aecdc01687a6949e4c54be6eb Revert "Rewrite GitMoveDeleteHook to use DirCacheBuilder"
439277860e1e315b7f0cd339b2435cc8311956c1 Revert "Teach GitMoveDeleteHook how to move a folder recursively"
abc44bc6efa47c4c6e3c23f85fe9de9cd8460224 Improve closing of files in error situations.
22e2808ac2915446ed81115b663c684341ce6bdd Close a forgotten reference to the HEAD ref.
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2008, #06; Wed, 26)
From: Daniel Barkalow @ 2008-12-06 18:39 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Junio C Hamano, Shawn O. Pearce, Johannes Schindelin, git
In-Reply-To: <fcaeb9bf0812060926r2ee443bfl3adb3f2d1129e5b8@mail.gmail.com>
On Sun, 7 Dec 2008, Nguyen Thai Ngoc Duy wrote:
> On 12/1/08, Daniel Barkalow <barkalow@iabervon.org> wrote:
> > On Sun, 30 Nov 2008, Nguyen Thai Ngoc Duy wrote:
> >
> > > On 11/29/08, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> > > > On 11/29/08, Daniel Barkalow <barkalow@iabervon.org> wrote:
> > > > > If there's any need for this to be distinguished from "assume unchanged",
> > > > > I think it should be used with, not instead of, the CE_VALID bit; and it
> > > > > could probably use some bit in the stat info section, since we don't need
> > > > > stat info if we know by assumption that the entry is valid.
> > > >
> > > >
> > > > Interesting. I'll think more about this.
> > > >
> > >
> > > As I said, CE_VALID implies all files are present.
> >
> >
> > My first question is whether this actually should be true. Going back to
> > the message for 5f73076c1a9b4b8dc94f77eac98eb558d25e33c0, it sounds like
> > the CE_VALID code is designed to be safe and sort of correct even if the
> > files are not actually unchanged; I don't think it would be out-of-spec
> > for CE_VALID to (1) always produce output as if the working tree contained
> > what the index contains, while (2) refusing to make any changes to working
> > tree files that do not actually match the index. As it is now, (2) is
> > explicitly true, but (1) is left vague-- commands may fail entirely or
> > produce different output if CE_VALID is set in the index for a file that
> > has changes in the working tree, but not in any particular way.
>
> (1) is not always true. For example diff machinary may examine
> worktree files regardless CE_VALID, which is updated for
> CE_NO_CHECKOUT in d9f8fca (Prevent diff machinery from examining
> worktree outside sparse checkout)
I know (1) is not always true in the current implementation. What I'm
getting at is to ask (a) whether our documented behavior ever violates (1)
and (b) whether enforcing (1) would be an improvement.
I suspect that enforcing (1) would be less surprising to users in the
situation where the assumption is false that worktree files match the
index when CE_VALID is set. As it is, the diff machinery does surprising
things in this situation, and I think we just say "Nasal Monkey Territory"
(that is, we tell the user, "if you don't want git to do surprising
things, don't get into this situation"). If that is the case, we should be
fine changing it to match your CE_NO_CHECKOUT behavior, and it wouldn't be
worse for CE_VALID and might be better.
> > Now, it might be necessary for CE_NO_CHECKOUT to differ from CE_VALID in
> > some ways in (2): if a file is CE_NO_CHECKOUT and absent, code which would
> > modify it could probably just report sucess, while CE_VALID on a file
> > with changes should probably report failure. On the other hand, that could
> > just as easily be at the porcelain layer, with the porcelain instructing
> > the plumbing to change the index without changing the working tree for
> > those files outside the sparse checkout, and the plumbing would report
> > errors if the porcelain did not do this.
>
> That's right. Much of work in the last half of the series is on
> porcelain layer. "git grep" fix is the only porcelain that gets fixed
> in this series.
I haven't gotten to reading the last half yet, so I'll avoid taking a
specific position on how it should work, and just make unsupported
suggestions.
> > > I could make CE_NO_CHECKOUT to be used with CE_VALID, but I would need
> > > to check all CE_VALID code path to make sure the behaviour remains if
> > > CE_NO_CHECKOUT is absent. It's just more intrusive.
> >
> >
> > I would expect all code that has a CE_VALID path to do something actually
> > wrong if it took the non-CE_VALID code path on CE_NO_CHECKOUT and there
> > was no CE_NO_CHECKOUT code path. So I'd expect that your patch is
> > insufficient to the extent that CE_NO_CHECKOUT doesn't imply CE_VALID
> > (since there is very little in the way of CE_NO_CHECKOUT-specific
> > handling in your patch).
>
> I read the code again. CE_NO_CHECKOUT should follow CE_VALID code path
> (which was extended to CE_VALID_MASK to have both flags). That means
> CE_NO_CHECKOUT is treated as same as CE_VALID. The only difference
> here is CE_VALID is set/unset by "git update-index --really-refresh"
> and core.ingorestat while CE_NO_CHECKOUT has its own way to set/unset.
I think, then, that it would be cleaner to have CE_VALID instead of
CE_VALID_MASK, and have the things that care test CE_NO_CHECKOUT or
!CE_NO_CHECKOUT. This wouldn't be all that different, except that it would
mean that distinguishing them appears as a special case, which in turn
makes it easier to question whether they should differ.
> There is not much work for CE_NO_CHECKOUT on plumbling level except
> some fixes. The last half of the series, for porcelain level, you will
> see more.
For the porcelain level, do we need the difference to be in the index? If
the porcelain knows the sparse checkout area and can instruct the plumbing
appropriately, the information shouldn't need to be stored in the index
unless it's ever important to remember whether an entry is CE_VALID due to
having been outside the checkout when the index was written, even though
the checkout area now includes it. I don't have a good intuition as to
what ought to happen if the user manually changes what's specified for
checkout without actually updating the index and working tree.
> > The only case I can think of where NO_CHECKOUT is more like !VALID than
> > VALID is with respect to whether we can report the content in the index by
> > looking in the filesystem instead of in the database; I don't think this
> > is an intentional optimization anywhere, and I think it would be a likely
> > source of bugs if it were (e.g., it would have to know about files which
> > are up-to-date with respect to stat info, but which have been "smudged" on
> > disk and therefore don't match byte-for-byte with the database).
>
> There is worktree file reuse in diff code somewhere IIRC. Yes, this
> should be checked.
>
> > Actually,
> > it might be most accurate to treat --no-checkout as being CE_VALID with a
> > smudge filter of "rm". If the combination of CE_VALID and on-disk
> > conversion works (which is likely to be the common pattern for Windows
> > users, who need autocrlf and have a slow lstat(), and is therefore
> > maintained), surely this combination would work for CE_NO_CHECKOUT.
>
> Very interesting.
>
> > > I have nothing against storing CE_NO_CHECKOUT in stat info except that
> > > it seems inappropriate/hidden place to do. ce_flags is more obvious
> > > choice. I haven't looked closely to stat info code in read-cache.c
> > > though.
> >
> >
> > It should be pretty clean to check CE_VALID when reading an entry from
> > disk and remap bits from it to additional flags in memory. I wouldn't
> > suggest overlaying them in memory, but there's also no shortage of space
> > for flags in memory.
>
> I see. Still I prefer the current approach, less headache to decide
> what bit to take from stat info ;-)
True. But it would be even easier if it didn't have to be marked in the
index at all.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2008, #06; Wed, 26)
From: Nguyen Thai Ngoc Duy @ 2008-12-06 17:26 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: Junio C Hamano, Shawn O. Pearce, Johannes Schindelin, git
In-Reply-To: <alpine.LNX.1.00.0811301509070.19665@iabervon.org>
On 12/1/08, Daniel Barkalow <barkalow@iabervon.org> wrote:
> On Sun, 30 Nov 2008, Nguyen Thai Ngoc Duy wrote:
>
> > On 11/29/08, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> > > On 11/29/08, Daniel Barkalow <barkalow@iabervon.org> wrote:
> > > > If there's any need for this to be distinguished from "assume unchanged",
> > > > I think it should be used with, not instead of, the CE_VALID bit; and it
> > > > could probably use some bit in the stat info section, since we don't need
> > > > stat info if we know by assumption that the entry is valid.
> > >
> > >
> > > Interesting. I'll think more about this.
> > >
> >
> > As I said, CE_VALID implies all files are present.
>
>
> My first question is whether this actually should be true. Going back to
> the message for 5f73076c1a9b4b8dc94f77eac98eb558d25e33c0, it sounds like
> the CE_VALID code is designed to be safe and sort of correct even if the
> files are not actually unchanged; I don't think it would be out-of-spec
> for CE_VALID to (1) always produce output as if the working tree contained
> what the index contains, while (2) refusing to make any changes to working
> tree files that do not actually match the index. As it is now, (2) is
> explicitly true, but (1) is left vague-- commands may fail entirely or
> produce different output if CE_VALID is set in the index for a file that
> has changes in the working tree, but not in any particular way.
(1) is not always true. For example diff machinary may examine
worktree files regardless CE_VALID, which is updated for
CE_NO_CHECKOUT in d9f8fca (Prevent diff machinery from examining
worktree outside sparse checkout)
> Now, it might be necessary for CE_NO_CHECKOUT to differ from CE_VALID in
> some ways in (2): if a file is CE_NO_CHECKOUT and absent, code which would
> modify it could probably just report sucess, while CE_VALID on a file
> with changes should probably report failure. On the other hand, that could
> just as easily be at the porcelain layer, with the porcelain instructing
> the plumbing to change the index without changing the working tree for
> those files outside the sparse checkout, and the plumbing would report
> errors if the porcelain did not do this.
That's right. Much of work in the last half of the series is on
porcelain layer. "git grep" fix is the only porcelain that gets fixed
in this series.
> > I could make CE_NO_CHECKOUT to be used with CE_VALID, but I would need
> > to check all CE_VALID code path to make sure the behaviour remains if
> > CE_NO_CHECKOUT is absent. It's just more intrusive.
>
>
> I would expect all code that has a CE_VALID path to do something actually
> wrong if it took the non-CE_VALID code path on CE_NO_CHECKOUT and there
> was no CE_NO_CHECKOUT code path. So I'd expect that your patch is
> insufficient to the extent that CE_NO_CHECKOUT doesn't imply CE_VALID
> (since there is very little in the way of CE_NO_CHECKOUT-specific
> handling in your patch).
I read the code again. CE_NO_CHECKOUT should follow CE_VALID code path
(which was extended to CE_VALID_MASK to have both flags). That means
CE_NO_CHECKOUT is treated as same as CE_VALID. The only difference
here is CE_VALID is set/unset by "git update-index --really-refresh"
and core.ingorestat while CE_NO_CHECKOUT has its own way to set/unset.
There is not much work for CE_NO_CHECKOUT on plumbling level except
some fixes. The last half of the series, for porcelain level, you will
see more.
> The only case I can think of where NO_CHECKOUT is more like !VALID than
> VALID is with respect to whether we can report the content in the index by
> looking in the filesystem instead of in the database; I don't think this
> is an intentional optimization anywhere, and I think it would be a likely
> source of bugs if it were (e.g., it would have to know about files which
> are up-to-date with respect to stat info, but which have been "smudged" on
> disk and therefore don't match byte-for-byte with the database).
There is worktree file reuse in diff code somewhere IIRC. Yes, this
should be checked.
> Actually,
> it might be most accurate to treat --no-checkout as being CE_VALID with a
> smudge filter of "rm". If the combination of CE_VALID and on-disk
> conversion works (which is likely to be the common pattern for Windows
> users, who need autocrlf and have a slow lstat(), and is therefore
> maintained), surely this combination would work for CE_NO_CHECKOUT.
Very interesting.
> > I have nothing against storing CE_NO_CHECKOUT in stat info except that
> > it seems inappropriate/hidden place to do. ce_flags is more obvious
> > choice. I haven't looked closely to stat info code in read-cache.c
> > though.
>
>
> It should be pretty clean to check CE_VALID when reading an entry from
> disk and remap bits from it to additional flags in memory. I wouldn't
> suggest overlaying them in memory, but there's also no shortage of space
> for flags in memory.
I see. Still I prefer the current approach, less headache to decide
what bit to take from stat info ;-)
--
Duy
^ permalink raw reply
* [PATCH (GIT-GUI FIX)] git-gui: Fix commit encoding handling.
From: Alexander Gavrilov @ 2008-12-06 17:24 UTC (permalink / raw)
To: Git Mailing List; +Cc: Shawn O. Pearce
Commits without an encoding header are supposed to
be encoded in utf8. While this apparently hasn't always
been the case, currently it is the active convention, so
it is better to follow it; otherwise people who have to
use commitEncoding on their machines are unable to read
utf-8 commits made by others.
I also think that it is preferrable to display the warning
about an unsupported value of commitEncoding more prominently,
because this condition may lead to surprising behavior and,
eventually, to loss of data.
Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
lib/blame.tcl | 5 ++---
lib/commit.tcl | 7 +++----
2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/lib/blame.tcl b/lib/blame.tcl
index c1cd7f3..1f3b08f 100644
--- a/lib/blame.tcl
+++ b/lib/blame.tcl
@@ -940,9 +940,8 @@ method _showcommit {cur_w lno} {
catch {
set fd [git_read cat-file commit $cmit]
fconfigure $fd -encoding binary -translation lf
- if {[catch {set enc $repo_config(i18n.commitencoding)}]} {
- set enc utf-8
- }
+ # By default commits are assumed to be in utf-8
+ set enc utf-8
while {[gets $fd line] > 0} {
if {[string match {encoding *} $line]} {
set enc [string tolower [string range $line 9 end]]
diff --git a/lib/commit.tcl b/lib/commit.tcl
index 3345149..9cc8410 100644
--- a/lib/commit.tcl
+++ b/lib/commit.tcl
@@ -27,9 +27,8 @@ You are currently in the middle of a merge that has not been fully completed. Y
if {[catch {
set fd [git_read cat-file commit $curHEAD]
fconfigure $fd -encoding binary -translation lf
- if {[catch {set enc $repo_config(i18n.commitencoding)}]} {
- set enc utf-8
- }
+ # By default commits are assumed to be in utf-8
+ set enc utf-8
while {[gets $fd line] > 0} {
if {[string match {parent *} $line]} {
lappend parents [string range $line 7 end]
@@ -208,7 +207,7 @@ A good commit message has the following format:
if {$use_enc ne {}} {
fconfigure $msg_wt -encoding $use_enc
} else {
- puts stderr [mc "warning: Tcl does not support encoding '%s'." $enc]
+ error_popup [mc "warning: Tcl does not support encoding '%s'." $enc]
fconfigure $msg_wt -encoding utf-8
}
puts $msg_wt $msg
--
1.6.0.4.30.gf4240
^ permalink raw reply related
* [PATCH (GIT-GUI FIX)] git-gui: Fix handling of relative paths in blame.
From: Alexander Gavrilov @ 2008-12-06 17:21 UTC (permalink / raw)
To: Git Mailing List; +Cc: Shawn O. Pearce, Paul Mackerras
Currently using '..' or '.' in the file path for gui blame
causes it to break, because the path is passed inside the
SHA:PATH spec to cat-file, which apparently does not understand
such items. As a result, cat-file returns nothing, and the
viewer crashes because of an "index out of range" error.
This commit adds a simple function that normalizes such paths.
I choose not to use [file normalize], because it uses some data
from the file system, e.g. dereferences symlinks, and creates
an absolute path, while blame may be used to inspect historical
information that bears no relation to the current filesystem state.
Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
On Saturday 06 December 2008 13:56:45 Paul Mackerras wrote:
> I have checked in a fix into my gitk.git repository at
> git://git.kernel.org/pub/scm/gitk/gitk.git. However, there seems to
> be a bug in git gui blame; it gives a "list index out of range" error
> sometimes.
>
> Shawn, to see an example of the error, get a current kernel tree and
> do:
>
> $ cd arch/powerpc
> $ git gui blame --line=1183 2c5e76158fcea6e3b9536a74efa7b5e2e846d374 \
> ../../net/sunrpc/svcsock.c
git-gui.sh | 18 ++++++++++++++++--
1 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/git-gui.sh b/git-gui.sh
index 8a4b42d..65dacf9 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2630,6 +2630,20 @@ proc usage {} {
exit 1
}
+proc normalize_relpath {path} {
+ set elements {}
+ foreach item [file split $path] {
+ if {$item eq {.}} continue
+ if {$item eq {..} && [llength $elements] > 0
+ && [lindex $elements end] ne {..}} {
+ set elements [lrange $elements 0 end-1]
+ continue
+ }
+ lappend elements $item
+ }
+ return [eval file join $elements]
+}
+
# -- Not a normal commit type invocation? Do that instead!
#
switch -- $subcommand {
@@ -2648,7 +2662,7 @@ blame {
foreach a $argv {
if {$is_path || [file exists $_prefix$a]} {
if {$path ne {}} usage
- set path $_prefix$a
+ set path [normalize_relpath $_prefix$a]
break
} elseif {$a eq {--}} {
if {$path ne {}} {
@@ -2671,7 +2685,7 @@ blame {
unset is_path
if {$head ne {} && $path eq {}} {
- set path $_prefix$head
+ set path [normalize_relpath $_prefix$head]
set head {}
}
--
1.6.0.4.30.gf4240
^ permalink raw reply related
* Re: [RFCv3 2/2] gitweb: links to patch action in commitdiff and shortlog view
From: Jakub Narebski @ 2008-12-06 15:25 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <cb7bb73a0812060525k65a7f549l2cce5f0dae9fc76c@mail.gmail.com>
Dnia sobota 6. grudnia 2008 14:25, Giuseppe Bilotta napisał:
> On Sat, Dec 6, 2008 at 1:53 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>>
>>> In shortlog view, a link to the patchset is only offered when the number
>>> of commits shown is less than the allowed maximum number of patches.
>>>
>>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>>> + if (gitweb_check_feature('patches')) {
>>> + $formats_nav .= " | " .
>>> + $cgi->a({-href => href(action=>"patch", -replay=>1)},
>>> + "patch");
>>> + }
>>>
>>> if (!defined $parent) {
>>> $parent = "--root";
>>> @@ -5415,6 +5420,11 @@ sub git_commitdiff {
>>> $formats_nav =
>>> $cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
>>> "raw");
>>> + if ($patch_max) {
>>> + $formats_nav .= " | " .
>>> + $cgi->a({-href => href(action=>"patch", -replay=>1)},
>>> + "patch");
>>> + }
>>>
>>> if (defined $hash_parent &&
>>> $hash_parent ne '-c' && $hash_parent ne '--cc') {
>>
>> In the above two hunks 'patch' view functions as "git show --pretty=email"
>> text/plain equivalent, but this duplicates a bit 'commitdiff_plain'
>> functionality. Well, 'commitdiff_plain' has currently some errors,
>> but...
>
> All things considered, for single commit view there is (modulo bugs)
> no factual difference between plain diff and patch view.
>
> Although we could merge them, I'm thinking that the plain diff view
> has somewhat too much information in it. For backwards compatibility
> it's probably not wise to change it, but we should consider it for the
> next major version, honestly.
I'm just wondering if we should add 'patch' link to 'commit' and
'commitdiff' views (as alternate view) at all...
>>> @@ -5949,6 +5959,14 @@ sub git_shortlog {
>>> $cgi->a({-href => href(-replay=>1, page=>$page+1),
>>> -accesskey => "n", -title => "Alt-n"}, "next");
>>> }
>>> + my $patch_max = gitweb_check_feature('patches');
>>> + if ($patch_max) {
>>> + if ($patch_max < 0 || @commitlist <= $patch_max) {
>>> + $paging_nav .= " ⋅ " .
>>> + $cgi->a({-href => href(action=>"patch", -replay=>1)},
>>> + @commitlist> 1 ? "patchset" : "patch");
>>> + }
>>> + }
>>
>> Here 'patch' view functions as "git format-patch", able to be downloaded
>> and fed to git-am. Perhaps the action should also be named 'patches'
>> here?; it could lead to the same function.
>
> I had half an idea to do so. 'patches' or 'patchset'?
Hmmm... I think 'patches'.
>> By the way, there is subtle bug in above link. If shortlog view is less
>> than $patch_max commits long, but it is because the history for a given
>> branch (or starting from given commit) is so short, and not because
>> there is cutoff $hash_parent set, the 'patchset' view wouldn't display
>> plain text equivalent view, but only patch for top commit.
>
> Ah, good point.
>
> Hm, not easy to solve. One way could be to add the hash_parent
> manually. Or we could make the 'patches' view different from the
> 'patch' view in the way it handles refspecs without ranges. I'm
> leaning towards the latter. What's your opinion?
I think simplest solution would be to add $hash_parent if it is not
set from the last commit, i.e. $commitlist[-1]{'id'}
>> I assume that the link is only for 'shortlog' view, and not also for
>> 'log' and 'history' views because 'shortlog' is the only log-like view
>> which support $hash_parent?
>
> The actual reason is that I never use log nor history view, but since
> they don't support hash_parent because of this (I was the one who sent
> the patch to support hash_parent in shortlog view) you could
> paralogistically say that's the reason ;-)
>
> I'm not sure about history view, but for log view I'm considering
> addiong also a 'patch' link next to each commit. I'll think about it.
Well, you can add it only for 'shortlog' view, and when the code for
all log-like views would get consolidated, you will get link to 'patches'
view automatically :-)
--
Jakub Narebski
Poland
^ permalink raw reply
* [RFCv4 3/3] gitweb: link to patch(es) view from commit and log views
From: Giuseppe Bilotta @ 2008-12-06 15:02 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Petr Baudis, Junio C Hamano, Giuseppe Bilotta
In-Reply-To: <1228575755-13432-3-git-send-email-giuseppe.bilotta@gmail.com>
We link to patch view in commit and commitdiff view, and to patches view
in log and shortlog view.
In (short)log view, the link is only offered when the number of commits
shown is no more than the allowed maximum number of patches.
Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
gitweb/gitweb.perl | 30 ++++++++++++++++++++++++++++--
1 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index dfc7128..8198875 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5019,6 +5019,15 @@ sub git_log {
my $paging_nav = format_paging_nav('log', $hash, $head, $page, $#commitlist >= 100);
+ my $patch_max = gitweb_check_feature('patches');
+ if ($patch_max) {
+ if ($patch_max < 0 || @commitlist <= $patch_max) {
+ $paging_nav .= " ⋅ " .
+ $cgi->a({-href => href(action=>"patches", -replay=>1)},
+ @commitlist > 1 ? "patchset" : "patch");
+ }
+ }
+
git_header_html();
git_print_page_nav('log','', $hash,undef,undef, $paging_nav);
@@ -5098,6 +5107,11 @@ sub git_commit {
} @$parents ) .
')';
}
+ if (gitweb_check_feature('patches')) {
+ $formats_nav .= " | " .
+ $cgi->a({-href => href(action=>"patch", -replay=>1)},
+ "patch");
+ }
if (!defined $parent) {
$parent = "--root";
@@ -5413,9 +5427,8 @@ sub git_commitdiff {
# if only a single commit is passed?
my $single_patch = shift && 1;
- my $patch_max;
+ my $patch_max = gitweb_check_feature('patches');
if ($format eq 'patch') {
- $patch_max = gitweb_check_feature('patches');
die_error(403, "Patch view not allowed") unless $patch_max;
}
@@ -5433,6 +5446,11 @@ sub git_commitdiff {
$formats_nav =
$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
"raw");
+ if ($patch_max) {
+ $formats_nav .= " | " .
+ $cgi->a({-href => href(action=>"patch", -replay=>1)},
+ "patch");
+ }
if (defined $hash_parent &&
$hash_parent ne '-c' && $hash_parent ne '--cc') {
@@ -5989,6 +6007,14 @@ sub git_shortlog {
$cgi->a({-href => href(-replay=>1, page=>$page+1),
-accesskey => "n", -title => "Alt-n"}, "next");
}
+ my $patch_max = gitweb_check_feature('patches');
+ if ($patch_max) {
+ if ($patch_max < 0 || @commitlist <= $patch_max) {
+ $paging_nav .= " ⋅ " .
+ $cgi->a({-href => href(action=>"patches", -replay=>1)},
+ @commitlist > 1 ? "patchset" : "patch");
+ }
+ }
git_header_html();
git_print_page_nav('shortlog','', $hash,$hash,$hash, $paging_nav);
--
1.5.6.5
^ permalink raw reply related
* [RFCv4 1/3] gitweb: add patch view
From: Giuseppe Bilotta @ 2008-12-06 15:02 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Petr Baudis, Junio C Hamano, Giuseppe Bilotta
In-Reply-To: <1228575755-13432-1-git-send-email-giuseppe.bilotta@gmail.com>
The output of commitdiff_plain is not intended for git-am:
* when given a range of commits, commitdiff_plain publishes a single
patch with the message from the first commit, instead of a patchset
* the hand-built email format replicates the commit summary both as
email subject and as first line of the email itself, resulting in
a duplication if the output is used with git-am.
We thus create a new view that can be fed to git-am directly, allowing
patch exchange via gitweb. The new view exposes the output of git
format-patch directly, limiting it to a single patch in the case of a
single commit.
A configurable upper limit is imposed on the number of commits which
will be included in a patchset, to prevent DoS attacks on the server.
Setting the limit to 0 will disable the patch view, setting it to a
negative number will remove the limit.
Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
gitweb/gitweb.perl | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 64 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 95988fb..71d5af4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -329,6 +329,14 @@ our %feature = (
'ctags' => {
'override' => 0,
'default' => [0]},
+
+ # The maximum number of patches in a patchset generated in patch
+ # view. Set this to 0 or undef to disable patch view, or to a
+ # negative number to remove any limit.
+ 'patches' => {
+ 'sub' => \&feature_patches,
+ 'override' => 0,
+ 'default' => [16]},
);
sub gitweb_get_feature {
@@ -410,6 +418,19 @@ sub feature_pickaxe {
return ($_[0]);
}
+sub feature_patches {
+ my @val = (git_get_project_config('patches', '--int'));
+
+ # if @val is empty, the config is not (properly)
+ # overriding the feature, so we return the default,
+ # otherwise we pick the override
+ if (@val) {
+ return @val;
+ }
+
+ return ($_[0]);
+}
+
# checking HEAD file with -e is fragile if the repository was
# initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
# and then pruned.
@@ -503,6 +524,7 @@ our %actions = (
"heads" => \&git_heads,
"history" => \&git_history,
"log" => \&git_log,
+ "patch" => \&git_patch,
"rss" => \&git_rss,
"atom" => \&git_atom,
"search" => \&git_search,
@@ -5386,6 +5408,13 @@ sub git_blobdiff_plain {
sub git_commitdiff {
my $format = shift || 'html';
+
+ my $patch_max;
+ if ($format eq 'patch') {
+ $patch_max = gitweb_check_feature('patches');
+ die_error(403, "Patch view not allowed") unless $patch_max;
+ }
+
$hash ||= $hash_base || "HEAD";
my %co = parse_commit($hash)
or die_error(404, "Unknown commit object");
@@ -5483,7 +5512,23 @@ sub git_commitdiff {
open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
'-p', $hash_parent_param, $hash, "--"
or die_error(500, "Open git-diff-tree failed");
-
+ } elsif ($format eq 'patch') {
+ # For commit ranges, we limit the output to the number of
+ # patches specified in the 'patches' feature.
+ # For single commits, we limit the output to a single patch,
+ # diverging from the git format-patch default.
+ my @commit_spec = ();
+ if ($hash_parent) {
+ if ($patch_max > 0) {
+ push @commit_spec, "-$patch_max";
+ }
+ push @commit_spec, '-n', "$hash_parent..$hash";
+ } else {
+ push @commit_spec, '-1', '--root', $hash;
+ }
+ open $fd, "-|", git_cmd(), "format-patch", '--encoding=utf8',
+ '--stdout', @commit_spec
+ or die_error(500, "Open git-format-patch failed");
} else {
die_error(400, "Unknown commitdiff format");
}
@@ -5532,6 +5577,14 @@ sub git_commitdiff {
print to_utf8($line) . "\n";
}
print "---\n\n";
+ } elsif ($format eq 'patch') {
+ my $filename = basename($project) . "-$hash.patch";
+
+ print $cgi->header(
+ -type => 'text/plain',
+ -charset => 'utf-8',
+ -expires => $expires,
+ -content_disposition => 'inline; filename="' . "$filename" . '"');
}
# write patch
@@ -5553,6 +5606,11 @@ sub git_commitdiff {
print <$fd>;
close $fd
or print "Reading git-diff-tree failed\n";
+ } elsif ($format eq 'patch') {
+ local $/ = undef;
+ print <$fd>;
+ close $fd
+ or print "Reading git-format-patch failed\n";
}
}
@@ -5560,6 +5618,11 @@ sub git_commitdiff_plain {
git_commitdiff('plain');
}
+# format-patch-style patches
+sub git_patch {
+ git_commitdiff('patch');
+}
+
sub git_history {
if (!defined $hash_base) {
$hash_base = git_get_head_hash($project);
--
1.5.6.5
^ permalink raw reply related
* [RFCv4 2/3] gitweb: add patches view
From: Giuseppe Bilotta @ 2008-12-06 15:02 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Petr Baudis, Junio C Hamano, Giuseppe Bilotta
In-Reply-To: <1228575755-13432-2-git-send-email-giuseppe.bilotta@gmail.com>
The only difference between patch and patches view is in the treatement
of single commits: the former only displays a single patch, whereas the
latter displays a patchset leading to the specified commit.
---
gitweb/gitweb.perl | 20 ++++++++++++++++++--
1 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 71d5af4..dfc7128 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -525,6 +525,7 @@ our %actions = (
"history" => \&git_history,
"log" => \&git_log,
"patch" => \&git_patch,
+ "patches" => \&git_patches,
"rss" => \&git_rss,
"atom" => \&git_atom,
"search" => \&git_search,
@@ -5408,6 +5409,9 @@ sub git_blobdiff_plain {
sub git_commitdiff {
my $format = shift || 'html';
+ # for patch view: should we limit ourselves to a single patch
+ # if only a single commit is passed?
+ my $single_patch = shift && 1;
my $patch_max;
if ($format eq 'patch') {
@@ -5524,7 +5528,15 @@ sub git_commitdiff {
}
push @commit_spec, '-n', "$hash_parent..$hash";
} else {
- push @commit_spec, '-1', '--root', $hash;
+ if ($single_patch) {
+ push @commit_spec, '-1';
+ } else {
+ if ($patch_max > 0) {
+ push @commit_spec, "-$patch_max";
+ }
+ push @commit_spec, "-n";
+ }
+ push @commit_spec, '--root', $hash;
}
open $fd, "-|", git_cmd(), "format-patch", '--encoding=utf8',
'--stdout', @commit_spec
@@ -5620,7 +5632,11 @@ sub git_commitdiff_plain {
# format-patch-style patches
sub git_patch {
- git_commitdiff('patch');
+ git_commitdiff('patch', 1);
+}
+
+sub git_patches {
+ git_commitdiff('patch', 0);
}
sub git_history {
--
1.5.6.5
^ permalink raw reply related
* [RFCv4 0/3] gitweb: patch view
From: Giuseppe Bilotta @ 2008-12-06 15:02 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Petr Baudis, Junio C Hamano, Giuseppe Bilotta
Fourth iteration of the patch view feature, that exposes git
format-patch output in gitweb. The most significant different from the
previous revision is the introduction of the 'patches' view, that only
differs from 'patch' view in the treatment of single commits: 'patch'
view only displays the patch for that specific commit, whereas 'patches'
follows the git format-patch M.O.
Giuseppe Bilotta (3):
gitweb: add patch view
gitweb: add patches view
gitweb: link to patch(es) view from commit and log views
gitweb/gitweb.perl | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 106 insertions(+), 1 deletions(-)
^ permalink raw reply
* Re: Git Books
From: nadim khemir @ 2008-12-06 14:38 UTC (permalink / raw)
To: git list
In-Reply-To: <m34p1hihx4.fsf@localhost.localdomain>
On Saturday 06 December 2008 13.54.06 Jakub Narebski wrote:
> "Scott Chacon" <schacon@gmail.com> writes:
> > I have been talked into helping write a real, paper-based book on Git
> > ...
>
> What I really would like to see in a paper book is _diagrams_, in the
> form of simple graphs (and not UML-like diagrams, of flow-control like
> diagrams).
I was thinking about buying the pdf below. The little I can see looks like
there are a bunch of diagrams in it.
http://peepcode.com/products/git-internals-pdf
> You can find them in various slides for presentations
> (among others Junio's talks), and sometimes in blog posts[1], but
> usually only as ASCII-diagrams[2] in git documentation. (And the
> examples in"The Git Comminity Book" I've seen so far are a bit too
> complicated).
I like doing my ASCII-diagrams with Asciio, unsurprizingly.
********
* HEAD *
********
|
v
.-----. .--------.
| tag | | branch |
'-----' '--------'
| |
v v
.......... .......... ..........
. commit .<--------------. commit .<----. commit .
.......... .......... ..........
| | |
v v v
.------. .------. .------.
| tree |--------.--------| tree | | tree |
'------' | '------' '------'
| v | |
v .------. v v
.------. | blob | .------. .------.
| tree |--. '------' .--| tree | | blob |
'------' | | '------' '------'
'-----.-----' |
| v
v .------.
.------. | tree |
| blob | '------'
'------' |
v
.------.
| blob |
'------'
cheers, Nadim.
^ permalink raw reply
* Re: [RFCv3 1/2] gitweb: add patch view
From: Giuseppe Bilotta @ 2008-12-06 13:46 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <200812061409.02730.jnareb@gmail.com>
On Sat, Dec 6, 2008 at 2:09 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, Dec 6, 2008 at 13:34, Giuseppe Bilotta wrote:
>> On Sat, Dec 6, 2008 at 1:34 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>>>
>>>> The manually-built email format in commitdiff_plain output is not
>>>> appropriate for feeding git-am, because of two limitations:
>>>> * when a range of commits is specified, commitdiff_plain publishes a
>>>> single patch with the message from the first commit, instead of a
>>>> patchset,
>>>
>>> This is because 'commitdiff_plain' wasn't _meant_ as patch series view,
>>> to be fed to git-am. Actually it is a bit cross between "git show"
>>> result with '--pretty=email' format, and "git diff" between two commits,
>>> to be fed to git-apply or GNU patch.
>>>
>>> Nevertheless the above reasoning doesn't need to be put in a commit
>>> message. But it explains why new 'patch' / 'patchset' view is needed:
>>> because there was no equivalent.
>>
>> I'll remove it.
>
> Errr... sorry, I haven't made myself clear. I meant here that *my*
> comment should not be added; your explanation about adding 'patch'
> view should IMHO stay, perhaps reworked a bit: commitdiff is not
> about generating patch series.
Oops, ok, I'll just rewrite it better 8-)
>>>> * in either case, the patch summary is replicated both as email subject
>>>> and as first line of the email itself, resulting in a doubled summary
>>>> if the output is fed to git-am.
>>>
>>> This is independent issue which is I think worth correcting anyway,
>>> unless we decide to scrap 'commitdiff_plain' view altogether.
>>> But I think we would want some text/plain patch view to be applied
>>> by GNU patch (for example in RPM .spec file).
>>
>> I don't think we should scrap commitdiff either, but the subject
>> replication is not really an issue if the view is not fed to git am.
>
> Well, there is it.
Considering the comments on the second patch too, I've been thinking
that it might worth it to merge commitdiff_plain and patch view for
single commits. Some changes for multi-commits commitdiff_plain can be
considered too, but as I mentioned this is a major changes and should
be dealt with on its own.
>>>> + # The maximum number of patches in a patchset generated in patch
>>>> + # view. Set this to 0 or undef to disable patch view, or to a
>>>> + # negative number to remove any limit.
>>>> + 'patches' => {
>>>> + 'override' => 1,
>>>> + 'default' => [16]},
>>>> );
>>>
>>> You need to set "'sub' => \&feature_patches" for feature to be
>>> override-able at all. Also features are usually not overridable
>>> by default, which reduces load a tiny bit (by _possibly_ not reading
>>> config, although that shouldn't matter much now with reading whole
>>> commit using single call to git-config, and not one call per variable).
>>
>> I think I'll make the feature non-overridable. I'll also make it
>> default to disabled, although I'm not particularly happy with the
>> choice.
>
> I think that having 'patches' feature to be able to override is a good
> idea, as limit on number of patches might depend on _repository_, for
> example being lower for repository with large commits (large in sense
> of diff size).
>
> Default with override unset seems to be precedence... as to having it
> disabled by default: having feature which require configuration enabled
> by default serves as an example of configuration; on the other hand the
> example might be in comments, like for 'actions' %feature.
I've added the feature_patches sub. Also, considering that there are
now basically 3 votes against 1 for enabling the feature by default,
I'll keep it enabled.
>>> And I think the default might be set larger: 'log' view generates
>>> as big if not bigger load, and it is split into 100 commits long
>>> pages.
>>
>> Hm, I would say the load of patch view is much higher than the load of
>> log view, both in terms of bandwidth and in terms of load on the
>> server, because of the diffs.
>
> Ah, I forgot about that. Limit to 16-25 seems to be reasonable, then.
I'll go with 16 for the time being, I think it's large enough to
accomodate most patchsets.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: Git Books
From: Dilip M @ 2008-12-06 13:39 UTC (permalink / raw)
To: Christian MICHON; +Cc: Scott Chacon, git list
In-Reply-To: <46d6db660812060427h3c97efd9ie4aa604133aa3583@mail.gmail.com>
On Sat, Dec 6, 2008 at 5:57 PM, <christian.michon@gmail.com> wrote:
> workflows, workflows, workflows...
>
> gitconfig/aliases best practices
>
> tips and tricks in branches (ex: parentless...) and in setting up git servers
Work flows, setting up the GIT server, publishing codes,....would be
_really_ helpful to great extent....and help a lot in deploying GIT
The work flow something like...http://source.android.com/submit-patches/workflow
--
Dilip
^ permalink raw reply
* Re: [RFCv3 2/2] gitweb: links to patch action in commitdiff and shortlog view
From: Giuseppe Bilotta @ 2008-12-06 13:25 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <200812060153.52947.jnareb@gmail.com>
On Sat, Dec 6, 2008 at 1:53 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>
>> In shortlog view, a link to the patchset is only offered when the number
>> of commits shown is less than the allowed maximum number of patches.
>>
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>> + if (gitweb_check_feature('patches')) {
>> + $formats_nav .= " | " .
>> + $cgi->a({-href => href(action=>"patch", -replay=>1)},
>> + "patch");
>> + }
>>
>> if (!defined $parent) {
>> $parent = "--root";
>> @@ -5415,6 +5420,11 @@ sub git_commitdiff {
>> $formats_nav =
>> $cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
>> "raw");
>> + if ($patch_max) {
>> + $formats_nav .= " | " .
>> + $cgi->a({-href => href(action=>"patch", -replay=>1)},
>> + "patch");
>> + }
>>
>> if (defined $hash_parent &&
>> $hash_parent ne '-c' && $hash_parent ne '--cc') {
>
> In the above two hunks 'patch' view functions as "git show --pretty=email"
> text/plain equivalent, but this duplicates a bit 'commitdiff_plain'
> functionality. Well, 'commitdiff_plain' has currently some errors,
> but...
All things considered, for single commit view there is (modulo bugs)
no factual difference between plain diff and patch view.
Although we could merge them, I'm thinking that the plain diff view
has somewhat too much information in it. For backwards compatibility
it's probably not wise to change it, but we should consider it for the
next major version, honestly.
>> @@ -5949,6 +5959,14 @@ sub git_shortlog {
>> $cgi->a({-href => href(-replay=>1, page=>$page+1),
>> -accesskey => "n", -title => "Alt-n"}, "next");
>> }
>> + my $patch_max = gitweb_check_feature('patches');
>> + if ($patch_max) {
>> + if ($patch_max < 0 || @commitlist <= $patch_max) {
>> + $paging_nav .= " ⋅ " .
>> + $cgi->a({-href => href(action=>"patch", -replay=>1)},
>> + @commitlist > 1 ? "patchset" : "patch");
>> + }
>> + }
>
> Here 'patch' view functions as "git format-patch", able to be downloaded
> and fed to git-am. Perhaps the action should also be named 'patches'
> here?; it could lead to the same function.
I had half an idea to do so. 'patches' or 'patchset'?
> By the way, there is subtle bug in above link. If shortlog view is less
> than $patch_max commits long, but it is because the history for a given
> branch (or starting from given commit) is so short, and not because
> there is cutoff $hash_parent set, the 'patchset' view wouldn't display
> plain text equivalent view, but only patch for top commit.
Ah, good point.
Hm, not easy to solve. One way could be to add the hash_parent
manually. Or we could make the 'patches' view different from the
'patch' view in the way it handles refspecs without ranges. I'm
leaning towards the latter. What's your opinion?
> I assume that the link is only for 'shortlog' view, and not also for
> 'log' and 'history' views because 'shortlog' is the only log-like view
> which support $hash_parent?
The actual reason is that I never use log nor history view, but since
they don't support hash_parent because of this (I was the one who sent
the patch to support hash_parent in shortlog view) you could
paralogistically say that's the reason ;-)
I'm not sure about history view, but for log view I'm considering
addiong also a 'patch' link next to each commit. I'll think about it.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: [RFCv3 1/2] gitweb: add patch view
From: Petr Baudis @ 2008-12-06 13:10 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: Junio C Hamano, Jakub Narebski, git
In-Reply-To: <cb7bb73a0812060501s6a3faea7l74e81c2a591ba2a7@mail.gmail.com>
On Sat, Dec 06, 2008 at 02:01:09PM +0100, Giuseppe Bilotta wrote:
> On Sat, Dec 6, 2008 at 1:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Some things, like sitewide default css changes, cannot be made disabled
> > by default. But a new feature can easily be kept disabled by default not
> > to cause needless surprises.
>
> In the eternal war between making feature easily available and not
> disturbing the user too much between upgrades, I'm more on the 'making
> available' field, especially when the features are not particularly
> invasive (e.g., the patch view only adds one single link, on the
> navigation bar, and only in some views).
>
> It should also be noted that if the sysadmin deploys a new software
> version without telling its users, there's an obvious communication
> problem between the sysadmin and its users, but that's not something
> the tool developers should try to work around. Otherwise we'd still be
> offering the dash version of the commands by default.
I feel exactly the same.
Acked-by: Petr Baudis <pasky@suse.cz>
Same goes for the patches, I haven't had time to examine them in detail
but the general shape looks fine.
^ permalink raw reply
* Re: [RFCv3 1/2] gitweb: add patch view
From: Jakub Narebski @ 2008-12-06 13:09 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <cb7bb73a0812060434r2b48fd1ev359020fb1e0d1603@mail.gmail.com>
On Sat, Dec 6, 2008 at 13:34, Giuseppe Bilotta wrote:
> On Sat, Dec 6, 2008 at 1:34 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>>
>>> The manually-built email format in commitdiff_plain output is not
>>> appropriate for feeding git-am, because of two limitations:
>>> * when a range of commits is specified, commitdiff_plain publishes a
>>> single patch with the message from the first commit, instead of a
>>> patchset,
>>
>> This is because 'commitdiff_plain' wasn't _meant_ as patch series view,
>> to be fed to git-am. Actually it is a bit cross between "git show"
>> result with '--pretty=email' format, and "git diff" between two commits,
>> to be fed to git-apply or GNU patch.
>>
>> Nevertheless the above reasoning doesn't need to be put in a commit
>> message. But it explains why new 'patch' / 'patchset' view is needed:
>> because there was no equivalent.
>
> I'll remove it.
Errr... sorry, I haven't made myself clear. I meant here that *my*
comment should not be added; your explanation about adding 'patch'
view should IMHO stay, perhaps reworked a bit: commitdiff is not
about generating patch series.
>>> * in either case, the patch summary is replicated both as email subject
>>> and as first line of the email itself, resulting in a doubled summary
>>> if the output is fed to git-am.
>>
>> This is independent issue which is I think worth correcting anyway,
>> unless we decide to scrap 'commitdiff_plain' view altogether.
>> But I think we would want some text/plain patch view to be applied
>> by GNU patch (for example in RPM .spec file).
>
> I don't think we should scrap commitdiff either, but the subject
> replication is not really an issue if the view is not fed to git am.
Well, there is it.
>>> + # The maximum number of patches in a patchset generated in patch
>>> + # view. Set this to 0 or undef to disable patch view, or to a
>>> + # negative number to remove any limit.
>>> + 'patches' => {
>>> + 'override' => 1,
>>> + 'default' => [16]},
>>> );
>>
>> You need to set "'sub' => \&feature_patches" for feature to be
>> override-able at all. Also features are usually not overridable
>> by default, which reduces load a tiny bit (by _possibly_ not reading
>> config, although that shouldn't matter much now with reading whole
>> commit using single call to git-config, and not one call per variable).
>
> I think I'll make the feature non-overridable. I'll also make it
> default to disabled, although I'm not particularly happy with the
> choice.
I think that having 'patches' feature to be able to override is a good
idea, as limit on number of patches might depend on _repository_, for
example being lower for repository with large commits (large in sense
of diff size).
Default with override unset seems to be precedence... as to having it
disabled by default: having feature which require configuration enabled
by default serves as an example of configuration; on the other hand the
example might be in comments, like for 'actions' %feature.
>> And I think the default might be set larger: 'log' view generates
>> as big if not bigger load, and it is split into 100 commits long
>> pages.
>
> Hm, I would say the load of patch view is much higher than the load of
> log view, both in terms of bandwidth and in terms of load on the
> server, because of the diffs.
Ah, I forgot about that. Limit to 16-25 seems to be reasonable, then.
>>> @@ -5483,7 +5498,12 @@ sub git_commitdiff {
>>> open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
>>> '-p', $hash_parent_param, $hash, "--"
>>> or die_error(500, "Open git-diff-tree failed");
>>> -
>>> + } elsif ($format eq 'patch') {
>>> + open $fd, "-|", git_cmd(), "format-patch", '--encoding=utf8',
>>> + '--stdout', $patch_max > 0 ? "-$patch_max" : (),
>>> + $hash_parent ? ('-n', "$hash_parent..$hash") :
>>> + ('--root', '-1', $hash)
>>
>> This bit makes 'patch' view behave bit differently than git-format-patch,
>> which I think is a good idea: namely it shows single patch if there is
>> no cutoff. This should be mentioned in commit message, and perhaps
>> even in a comment in code.
>>
>> Beside, if you show only single patch because $hash_parent is not set,
>> you don't need to examine $patch_max nor set limit, because you use '-1'.
>> Currently if $patch_max > 0 and !$hash_parent, you pass limit for the
>> number of patches twice. This I think is harmless but...
>
> I've reworked the code a bit, making the commit spec an array computed
> before passing it to the command line. The code is cleaner but
> obviously longer 8-)
Cleaner is good.
> The double limit worked properly, btw.
Nice to know. But should we rely on corner cases handling?
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [RFCv3 1/2] gitweb: add patch view
From: Giuseppe Bilotta @ 2008-12-06 13:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narebski, git, Petr Baudis
In-Reply-To: <7vabbaxh8y.fsf@gitster.siamese.dyndns.org>
On Sat, Dec 6, 2008 at 1:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>>> + # The maximum number of patches in a patchset generated in patch
>>> + # view. Set this to 0 or undef to disable patch view, or to a
>>> + # negative number to remove any limit.
>>> + 'patches' => {
>>> + 'override' => 1,
>>> + 'default' => [16]},
>>> );
>>
>> You need to set "'sub' => \&feature_patches" for feature to be
>> override-able at all. Also features are usually not overridable
>> by default, which reduces load a tiny bit (by _possibly_ not reading
>> config, although that shouldn't matter much now with reading whole
>> commit using single call to git-config, and not one call per variable).
>> And I think the default might be set larger: 'log' view generates
>> as big if not bigger load, and it is split into 100 commits long
>> pages.
>
> I do not think defaulting to 'no' for overridability nor defaulting a new
> feature to 'disabled' have much to do with the load, but they are more
> about the principle of least surprise. Somebody who runs gitweb in the
> playpen he was given on the server shouldn't be getting a phone call from
> his users late at night complaining that the page his gitweb serves look
> different and has one extra link per each line, only because the sysadmin
> of the server decided to update git to 1.6.1 without telling him.
>
> Once a new version capable of serving a new feature is introduced, he can
> plan, announce and deploy by switching the feature on in his gitweb
> configuration file.
>
> Some things, like sitewide default css changes, cannot be made disabled
> by default. But a new feature can easily be kept disabled by default not
> to cause needless surprises.
In the eternal war between making feature easily available and not
disturbing the user too much between upgrades, I'm more on the 'making
available' field, especially when the features are not particularly
invasive (e.g., the patch view only adds one single link, on the
navigation bar, and only in some views).
It should also be noted that if the sysadmin deploys a new software
version without telling its users, there's an obvious communication
problem between the sysadmin and its users, but that's not something
the tool developers should try to work around. Otherwise we'd still be
offering the dash version of the commands by default.
(Plus, weren't you the one suggesting that the remote heads feature
should be enabled by default? And that's an even more invasive change,
if you ask me.)
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: Git Books
From: Paolo Ciarrocchi @ 2008-12-06 12:56 UTC (permalink / raw)
To: Scott Chacon; +Cc: git list
In-Reply-To: <d411cc4a0812060358ub640ea3kd04072c5640eef68@mail.gmail.com>
On 12/6/08, Scott Chacon <schacon@gmail.com> wrote:
> Hey all,
>
> I have been talked into helping write a real, paper-based book on Git
> for a publisher big enough that you may even see it in your local
> Borders or whatnot. (And, it appears that Junio has been as well:
> http://gitster.livejournal.com/21616.html)
>
> So, since I'm near the beginning of this process, I was wondering if
> the group had any feedback as to what might be super helpful to
> include. I mean, I have a pretty good layout and all, but if you
> wanted to point me to some threads that tend to crop up in the mailing
> list and IRC channel from relative newcomers that I might be able to
> nip in the bud, I would like to. I'm addressing the stuff that _I_
> hear a lot, and I'm scanning the IRC logs and list for topics, but I
> figured many of you must answer the same questions all the time, too.
Please spend lot of words about fetch and pull. These are the most
hard to ùnderstand commands for people that are used to cvs and svn.
Btw, i would love to see a git in a nutshell book in my native
language (italian).
I'm willing to help writing and translating.
Ciao,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/
http://mypage.vodafone.it/
^ permalink raw reply
* Re: Git Books
From: Jakub Narebski @ 2008-12-06 12:54 UTC (permalink / raw)
To: Scott Chacon; +Cc: git list
In-Reply-To: <d411cc4a0812060358ub640ea3kd04072c5640eef68@mail.gmail.com>
"Scott Chacon" <schacon@gmail.com> writes:
> I have been talked into helping write a real, paper-based book on Git
> for a publisher big enough that you may even see it in your local
> Borders or whatnot. (And, it appears that Junio has been as well:
> http://gitster.livejournal.com/21616.html)
>
> So, since I'm near the beginning of this process, I was wondering if
> the group had any feedback as to what might be super helpful to
> include. I mean, I have a pretty good layout and all, but if you
> wanted to point me to some threads that tend to crop up in the mailing
> list and IRC channel from relative newcomers that I might be able to
> nip in the bud, I would like to. I'm addressing the stuff that _I_
> hear a lot, and I'm scanning the IRC logs and list for topics, but I
> figured many of you must answer the same questions all the time, too.
What I really would like to see in a paper book is _diagrams_, in the
form of simple graphs (and not UML-like diagrams, of flow-control like
diagrams). You can find them in various slides for presentations
(among others Junio's talks), and sometimes in blog posts[1], but
usually only as ASCII-diagrams[2] in git documentation. (And the
examples in"The Git Comminity Book" I've seen so far are a bit too
complicated).
For example explaining git object model, explaining refs: local
branches, remote-tracking branches and tags, explaining pulling and
pushing, explaining merging and 3-way merge algorithm are difficult to
do without diagrams; diagrams make it much easier to understand.
Others have emphasized workflows enough...
Footnotes:
==========
[1] http://www.gnome.org/~federico/news-2008-11.html#pushing-and-pulling-with-git-1
[2] This is understandable, as while AsciiDoc format makes it quite
good on promise to be easy to edit for non-tech users, AFAIK there
is no such format for diagrams and pictures. PIC and Asymptote
nonwithstanding.
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [RFCv3 1/2] gitweb: add patch view
From: Giuseppe Bilotta @ 2008-12-06 12:34 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <200812060134.22959.jnareb@gmail.com>
On Sat, Dec 6, 2008 at 1:34 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Wed, 3 Dec 2008, Giuseppe Bilotta wrote:
>
>> The manually-built email format in commitdiff_plain output is not
>> appropriate for feeding git-am, because of two limitations:
>> * when a range of commits is specified, commitdiff_plain publishes a
>> single patch with the message from the first commit, instead of a
>> patchset,
>
> This is because 'commitdiff_plain' wasn't _meant_ as patch series view,
> to be fed to git-am. Actually it is a bit cross between "git show"
> result with '--pretty=email' format, and "git diff" between two commits,
> to be fed to git-apply or GNU patch.
>
> Nevertheless the above reasoning doesn't need to be put in a commit
> message. But it explains why new 'patch' / 'patchset' view is needed:
> because there was no equivalent.
I'll remove it.
>> * in either case, the patch summary is replicated both as email subject
>> and as first line of the email itself, resulting in a doubled summary
>> if the output is fed to git-am.
>
> This is independent issue which is I think worth correcting anyway,
> unless we decide to scrap 'commitdiff_plain' view altogether.
> But I think we would want some text/plain patch view to be applied
> by GNU patch (for example in RPM .spec file).
I don't think we should scrap commitdiff either, but the subject
replication is not really an issue if the view is not fed to git am.
>> + # The maximum number of patches in a patchset generated in patch
>> + # view. Set this to 0 or undef to disable patch view, or to a
>> + # negative number to remove any limit.
>> + 'patches' => {
>> + 'override' => 1,
>> + 'default' => [16]},
>> );
>
> You need to set "'sub' => \&feature_patches" for feature to be
> override-able at all. Also features are usually not overridable
> by default, which reduces load a tiny bit (by _possibly_ not reading
> config, although that shouldn't matter much now with reading whole
> commit using single call to git-config, and not one call per variable).
I think I'll make the feature non-overridable. I'll also make it
default to disabled, although I'm not particularly happy with the
choice.
> And I think the default might be set larger: 'log' view generates
> as big if not bigger load, and it is split into 100 commits long
> pages.
Hm, I would say the load of patch view is much higher than the load of
log view, both in terms of bandwidth and in terms of load on the
server, because of the diffs.
>> sub git_commitdiff {
>> my $format = shift || 'html';
>> +
>> + my $patch_max = gitweb_check_feature('patches');
>
> Wouldn't it be better to name this variable $max_patchset_size, or
> something like that? I'm not saying that this name is bad, but I'm
> wondering if it could be better...
max_patchset_size sounds much worse than patch_max to me, which is why
I went for the latter 8-)
>> if ($format eq 'html') {
>> $formats_nav =
>> $cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
>> @@ -5483,7 +5498,12 @@ sub git_commitdiff {
>> open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
>> '-p', $hash_parent_param, $hash, "--"
>> or die_error(500, "Open git-diff-tree failed");
>> -
>> + } elsif ($format eq 'patch') {
>> + open $fd, "-|", git_cmd(), "format-patch", '--encoding=utf8',
>> + '--stdout', $patch_max > 0 ? "-$patch_max" : (),
>
>> + $hash_parent ? ('-n', "$hash_parent..$hash") :
>> + ('--root', '-1', $hash)
>
> This bit makes 'patch' view behave bit differently than git-format-patch,
> which I think is a good idea: namely it shows single patch if there is
> no cutoff. This should be mentioned in commit message, and perhaps
> even in a comment in code.
>
> Beside, if you show only single patch because $hash_parent is not set,
> you don't need to examine $patch_max nor set limit, because you use '-1'.
> Currently if $patch_max > 0 and !$hash_parent, you pass limit for the
> number of patches twice. This I think is harmless but...
I've reworked the code a bit, making the commit spec an array computed
before passing it to the command line. The code is cleaner but
obviously longer 8-)
The double limit worked properly, btw.
>> + # TODO add X-Git-Tag/X-Git-Url headers in a sensible way
>
> Sensible way would mean modifying git-format-patch to be able to add
> extra headers via command option, just like it is now possible via
> config variable format.headers, I think. Because there are no surefire
> markers of where one patch ends and another begins: commit message is
> free text, and can contain diff... although if it contains diff
> separator '---' then git-am would have problem with patch; or at least
> even assuming sane commit messages it is not easy.
>
> Also I think that only X-Git-Url makes sense, and it is per whole
> patchset (whole 'patch' view output) and not for each individual
> patch.
I've stripped this commet for the time being. I'm not sure even
X-Git-Url makes sense, and the fact that it should only attached to
the first email makes it an oddball.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: Git Books
From: Christian MICHON @ 2008-12-06 12:27 UTC (permalink / raw)
To: Scott Chacon; +Cc: git list
In-Reply-To: <d411cc4a0812060358ub640ea3kd04072c5640eef68@mail.gmail.com>
On Sat, Dec 6, 2008 at 12:58 PM, Scott Chacon <schacon@gmail.com> wrote:
> Hey all,
>
> I have been talked into helping write a real, paper-based book on Git
> for a publisher big enough that you may even see it in your local
> Borders or whatnot. (And, it appears that Junio has been as well:
> http://gitster.livejournal.com/21616.html)
>
> So, since I'm near the beginning of this process, I was wondering if
> the group had any feedback as to what might be super helpful to
> include. I mean, I have a pretty good layout and all, but if you
> wanted to point me to some threads that tend to crop up in the mailing
> list and IRC channel from relative newcomers that I might be able to
> nip in the bud, I would like to. I'm addressing the stuff that _I_
> hear a lot, and I'm scanning the IRC logs and list for topics, but I
> figured many of you must answer the same questions all the time, too.
>
> Thanks,
> Scott
> --
workflows, workflows, workflows...
gitconfig/aliases best practices
tips and tricks in branches (ex: parentless...) and in setting up git servers
my 3 cents :)
--
Christian
--
http://detaolb.sourceforge.net/, a linux distribution for Qemu with Git inside !
^ permalink raw reply
* How to clone git repository with git-svn meta-data included?
From: Grzegorz Kossakowski @ 2008-12-06 12:15 UTC (permalink / raw)
To: git
Hello,
Some folks at Apache are experimenting with Git and we are currently seeking for the git-svn
integration that fits our needs and infrastructure.
After some evaluation we decided that our setup could be described using following points:
a) our svn repository remains our main, official server where every committer is obligated to push
their changes to at some time.
b) we set up clone of svn repository using git-svn. One of our members, Jukka Zitting, maintains
such a service here[1]. Such repositories should be usable both for our committers (that have rights
to push to svn) and our contributors that want to contribute random patches
c) we want carefully track who committed/contributed what
Basically, a) implies b) and point b) looks little bit problematic right now.
Jukka has set up his hosting using method described in his e-mail[2] which basically makes use of
git svn. The major problem is that if one clones Jukka's repository then git svn information is not
being cloned so committers have no means to push their changes to main, svn server.
I've tried to play a little bit around with this issue and I tried to copy information from .git
directory found on Jukka's server. This made me able to push my changes but git svn insisted on
rebasing my repository using commits found in svn which is wrong. Basically we want such a setup
that uses git repository (Jukka's clone) for pulling changes and local git svn for pushing changes.
Git svn should never try to rebase local repository because this will lead to two different trees on
two different machines so we won't be able to exchange and merge changesets.
Is it possible with Git right now?
Another point (c) which seems to be brought a couple of times but never a definitive answer has been
given, AFAIK. Let's imagine we have committer C and two contributors A and B.
A and B start to work on some feature and C agreed to help A and B and once their work is finished
to merge their changes into his repository and eventually push them to main, svn repository. Now A
and B work on implementation and from time to time their merge changes from each other. Once they
are finished A asks C to merge their work into C's repository. Everything is fine provided we can
trust both A and B.
What if A was not fair and has rewritten a few commits coming from B so they contain malicious code?
How we can detect something like that and how C be sure that what he merges is really work
attributed by correct names?
Thanks for your answers.
[1] http://jukka.zitting.name/git/
[2] http://markmail.org/message/fzzy7nepk7olx5fl
--
Best regards,
Grzegorz Kossakowski
^ permalink raw reply
* Re: Git Books
From: Thomas Adam @ 2008-12-06 12:09 UTC (permalink / raw)
To: Scott Chacon; +Cc: git list
In-Reply-To: <d411cc4a0812060358ub640ea3kd04072c5640eef68@mail.gmail.com>
2008/12/6 Scott Chacon <schacon@gmail.com>:
> So, since I'm near the beginning of this process, I was wondering if
> the group had any feedback as to what might be super helpful to
> include. I mean, I have a pretty good layout and all, but if you
> wanted to point me to some threads that tend to crop up in the mailing
> list and IRC channel from relative newcomers that I might be able to
> nip in the bud, I would like to. I'm addressing the stuff that _I_
> hear a lot, and I'm scanning the IRC logs and list for topics, but I
> figured many of you must answer the same questions all the time, too.
Perhaps you're able to share this layout? Whilst I am not about to go
trawling through the archives, one thing I do know that comes up a
lot, isn't so much how to use git to mimick coming from another SCM,
but more workflows -- that's the big stumbling point for people using
Git. I know I'd like to see that aspect in a Git book heavily
addressed.
-- Thomas Adam
^ permalink raw reply
* Git Books
From: Scott Chacon @ 2008-12-06 11:58 UTC (permalink / raw)
To: git list
Hey all,
I have been talked into helping write a real, paper-based book on Git
for a publisher big enough that you may even see it in your local
Borders or whatnot. (And, it appears that Junio has been as well:
http://gitster.livejournal.com/21616.html)
So, since I'm near the beginning of this process, I was wondering if
the group had any feedback as to what might be super helpful to
include. I mean, I have a pretty good layout and all, but if you
wanted to point me to some threads that tend to crop up in the mailing
list and IRC channel from relative newcomers that I might be able to
nip in the bud, I would like to. I'm addressing the stuff that _I_
hear a lot, and I'm scanning the IRC logs and list for topics, but I
figured many of you must answer the same questions all the time, too.
Thanks,
Scott
^ permalink raw reply
* Re: gitk: 'show origin of this line' barfs if gitk run in subdir
From: Paul Mackerras @ 2008-12-06 10:56 UTC (permalink / raw)
To: Mark Burton; +Cc: Git Mailing List, Shawn O. Pearce
In-Reply-To: <20081203222258.6db9b34e@crow>
Mark Burton writes:
> I think the same problem exists with the 'run git gui blame on this
> line' feature. It doesn't work when gitk is started in a subdir of the
> working tree but it does work when gitk is started in the top-level
> directory.
I have checked in a fix into my gitk.git repository at
git://git.kernel.org/pub/scm/gitk/gitk.git. However, there seems to
be a bug in git gui blame; it gives a "list index out of range" error
sometimes.
Shawn, to see an example of the error, get a current kernel tree and
do:
$ cd arch/powerpc
$ git gui blame --line=1183 2c5e76158fcea6e3b9536a74efa7b5e2e846d374 \
../../net/sunrpc/svcsock.c
Paul.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox