Git development
 help / color / mirror / Atom feed
* Re: how to determine oldest supported version of git
From: Jonathan Nieder @ 2012-02-15 18:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vaa4k38nj.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

> But think again, with the intimate knowledge of how these bugfix topics
> are merged down to older maintenance tracks.
[...]
> But nobody in the development community rebuilds 'maint' every time it is
> updated and runs the result as his or her primary production version. Even
> I do not do that (remember, I run 'next'). I only build and run full test
> suite. Older maintenance tracks are worse. I do not think anybody runs
> them before they are tagged and released.

I can offer one data point.  In the context of Debian sid, Gerrit and
I do test each version in daily work before uploading it.  I generally
build from and test whatever track is going to be used for the next
upload (usually plus a few extra features I am interested in for
private use) a little while before the release, to be prepared.
Anders sometimes uploads to the Ubuntu PPA which brings more testers.
After the upload, users running "sid" test for about a week before
even more users running "testing" get to take a look at it and test
for the sake of later users who will run "stable".

So little bugs get discovered, with time to fix them.

Even with this, the extra time to migrate from 1.7.6 to 1.7.7, for
example, was very helpful in the context of Debian sid.  Like it or
not, new features *do* come with minor regressions, and it helps the
sanity of a package maintainer to not have to suffer people who did
not request a bleeding-edge release complaining about regressions
until there has been time to fix them.

Of course, this has nothing to do with Debian stable, which is an
orthogonal story.  I'll discuss that below.

[...]
>  * At that point, old 'maint' and 1.7.9.X track cease to receive updates,
>    as there is no point maintaining them. It only encourages distros to
>    stay behind, adding unnecesary maintenance burden to us.

If you are thinking of distros like Debian stable, then that is just
wishful thinking.  Dropping support for old releases does not have any
effect except to cause patches to be missed there.  (See iceweasel and
chromium-browser for examples where using the version in Debian stable
is something I would usually not recommend.)

This may seem weird, but keep in mind that people like you and me are
not the target audience for the git package in Debian stable.  We use
git heavily.  If I am on a machine running stable or RHEL, I will
build a private copy of git in $HOME or ask the sysadmin to install a
more recent git as the first thing I do.

The reason that packages go into Debian stable and then just _don't
change_ is that the target users are not using those packages heavily.
If a new feature (e.g., "signatures from tags get incorporated into
the merge commit from pulling them") causes a regression (e.g., "the
script I used to run every week that pulls my favorite software
package and builds it just stopped working"), then these people get
zero benefit, for a sizable cost.

Though that's a digression.  The relevant detail to mention here is
that there is real demand on downstreams to continue to maintain
packages without adding new features.  They will help to maintain
old releases if you want.  If we want to influence that maintainance,
for example to ensure security bugs are fixed correctly and in the
same way everywhere, a good way is to keep a maintenance branch.

Hoping that clarifies a little.
Jonathan

^ permalink raw reply

* Re: [StGit PATCH] Parse commit object header correctly
From: "Andy Green (林安廸)" @ 2012-02-15 18:40 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Michael Haggerty, Junio C Hamano, Karl Hasselström, git
In-Reply-To: <CAHkRjk451=_XaQuUXmxAvB3sRRz6-J+c7A2ZrfLwfGz=z05Lag@mail.gmail.com>

On 02/15/2012 04:24 AM, Somebody in the thread at some point said:

Hi -

> Thank you all for comments and patches. I used a combination of
> Junio's patch with the comments from Michael and a fix from me. I'll
> publish it to the 'master' branch shortly and release a 0.16.1
> hopefully this week.

I cloned the master branch and installed it locally, it's working well.

Thanks a lot to the guys who spent time on this bug and stgit overall,
which I rely heavily on!

-Andy

^ permalink raw reply

* Re: how to determine oldest supported version of git
From: Jonathan Nieder @ 2012-02-15 18:44 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <20120215183454.GA23016@burratino>

Jonathan Nieder wrote:

> Even with this, the extra time to migrate from 1.7.6 to 1.7.7, for
> example, was very helpful in the context of Debian sid.

Whoops, off by one error.  The extra time to move from 1.7.4 to 1.7.5
and 1.7.5 to 1.7.6 was helpful.  1.7.7 was actually pretty painless,
so sid moved to it right away. ;-)

^ permalink raw reply

* Re: git status: small difference between stating whole repository and small subdirectory
From: Jeff King @ 2012-02-15 19:03 UTC (permalink / raw)
  To: Piotr Krukowiecki; +Cc: Thomas Rast, Git Mailing List, Nguyen Thai Ngoc Duy
In-Reply-To: <CAA01Cso_8=159UDMFUHiYz1X=gYtpbqRO4h3TMw7N=4YMV8YNg@mail.gmail.com>

On Wed, Feb 15, 2012 at 09:57:29AM +0100, Piotr Krukowiecki wrote:

> All is on local disk and system is idle.
> 
> Indeed, after gc the times went down:
> 10s -> 2.3s (subdirectory)
> 17s -> 9.5s (whole repo)
> 
> 2 seconds is much better and I'd say acceptable for me. But my questions are:

Obviously these answers didn't come from any deep analysis, but are
educated guesses from me based on previous performance patterns we've
seen on the list:

> - why is it so slow with not packed repo?

Your numbers show that you're I/O-bound:

>>> $ time git status    > /dev/null
>>> real    0m41.670s
>>> user    0m0.980s
>>> sys     0m2.908s
>>>
>>> $ time git status -- src/.../somedir   > /dev/null
>>> real    0m17.380s
>>> user    0m0.748s
>>> sys     0m0.328s

which is not surprising, since you said you dropped caches before-hand.
Repacking probably reduced your disk footprint by a lot, which meant
less I/O.

I notice that you're still I/O bound even after the repack:

> $ time git status  -- .
> real    0m2.503s
> user    0m0.160s
> sys     0m0.096s
> 
> $ time git status
> real    0m9.663s
> user    0m0.232s
> sys     0m0.556s

Did you drop caches here, too?  Usually that would not be the case on a
warm cache. If it is, then it sounds like you are short on memory to
actually hold the directory tree and object db in cache. If not, what do
the warm cache numbers look like?

> - can it be faster without repacking?

Not really. You're showing an I/O problem, and repacking is git's way of
reducing I/O.

> - even with packed repo, the time on small subdirectory is much higher
> than I'd expect given time on whole repo and subdirectory size - why?

Hard to say without profiling.  It may be that we reduced the object db
lookups, saving some time, but still end up stat()ing the whole tree.
The optimization to stat only the directories of interest was in 688cd6d
(status: only touch path we may need to check, 2010-01-14), which went
into v1.7.0. What version of git are you using?

-Peff

^ permalink raw reply

* Re: [PATCH/RFC] Document format of basic Git objects
From: Junio C Hamano @ 2012-02-15 19:48 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1329312140-24089-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This is just a draft text with a bunch of fixmes. But I'd like to hear
> from the community if this is a worthy effort. If so, then whether
> git-cat-file is a proper place for it. Or maybe we put relevant text
> in commit-tree, write-tree and mktag, then refer to them in cat-file
> because cat-file can show raw objects.
>
> So comments?

This _only_ describes the payload (i.e. without the 'blob <size>\n' header
used in loose object, in other words, what read_object() may return).
There should be a sentence to stress this.  As many Git intros (including
my book) begin with the "a short header 'blob <size>\n' concatenated with
the contents is hashed to compute the object name" picture, it would be
confusing unless you explicitly say that you are only describing the
"contents" part.

It makes sense to mention that the cat-file subcommand is used to obtain
this raw data somewhere in the documentation, but I would say the content
of this patch belongs to Documentation/technical/ somewhere.

>  PS. This also makes me wonder if tag object supports "encoding".

I do not think so.

> +OBJECT FORMAT
> +-------------
> +
> +Tree object consists of a series of tree entries sorted in memcmp()
> +order by entry name. Each entry consists of:
> +
> +- POSIX file mode encoded in octal ascii

Add ", no 0 padding to the right" at the end, as I heard that every
imitation of Git gets this wrong in its first version.

> +- One space character
> +- Entry name terminated by one character NUL
> +- 20 byte SHA-1 of the entry

> +Tag object is ascii plain text in a format similar to email format
> +(RFC 822). ...

Do not mention "email format (RFC 822)" at all.  The differences are
significant enough that it only confuses the readers.

We do not have colon at the end of the header, we do not promise to parse
field names case insensitively, and the way continuation lines are parsed
is totally different (a "similar" construct in RFC 2822 is "folded header
lines", but it is signalled by "folding white space", it discards the
end-of-line from the previous line and makes the result a logical single
line. Our continuation lines are introduced by a single SP and the result
of concatenation keeps the end-of-line from the previous lines, making the
result multiple lines).

Also we do not promise that the lines in the header part are always
<field,value> pairs.  So rephrase this while carefully distinguishing
between "a line in header" and "field".

    A commit or a tag object begins with the "header" that consists of one
    or more lines delimited by LF. The end of the header is signalled by
    an empty line.

    A "continuation line" in the header begins with a SP.  The remainder
    of the line, after removing that SP, is concatenated to the previous
    line, while retaining the LF at the end of the previous line.

    When a line in the header begins with a letter other than SP, and has
    at least one SP in it, it is called a "field".  A field consists of
    the "field name", which is the string before the first SP on the line,
    and its "value", which is everything after that SP.  When the value
    consists of multiple lines, continuation lines are used.

    More than one field with the same name can appear in the header of an
    object, and the order in which they appear is significant.

    In a commit object, the header begins with the following fields that
    have such and such meaning.

    In a tag object, the header begins with the following fields...

    After these defined fields, newer versions of git may add more lines
    in the header. Some of them may be fields, others might not be. The
    implementations to parse commit and tag objects must ignore lines in
    the header that it does not understand without triggering an error.

>  ... It consists of a header and a body, separated by a blank
> +line. The header includes exactly four fields in the following order:
> +

If you hand-craft a tag-like object that has unknown field after these
four, how badly the current implementations behave?

> +1. "object" field, followed by SHA-1 in ascii of the tagged object
> +2. "type" field, followed by the type in ascii of the tagged object
> +   (either "commit", "tag", "blob" or "tree" without quotes,
> +   case-sensitive)
> +3. "tag" field, followed by the tag name
> +4. "tagger" field, followed by the <XXX, to be named>

> +The tag body contains the tag's message and possibly GPG signature.
> +
> +Commit object is in similar format to tag object. The commit body is

It is strange that you introduce tag and then commit.  I would think that
readers expect to see them presented in the usual blob/tree/commit/tag
order.

> +in plain text of the chosen encoding (by default UTF-8). The commit
> +header has the following fields in listed order
> +
> +1. One "tree" field, followed by the commit's tree's SHA-1 in ascii
> +2. Zero, one or more "parent" field
> +3. One "author" field, in <XXX to be named> format
> +3. One "committer" field, in <XXX to be named> format
> +4. Optionally one "encoding" field, followed by the encoding used for
> +   commit body
> +5. GPG signature (fixme)
> +
> +More headers after these fields are allowed. Unrecognized header
> +fields must be kept untouched if the commit is rewritten.

Replace the first sentence with "New kinds of fields may be added in later
versions of git." and drop the second one entirely.  Depending on the
reason and nature of the "rewrite", we may or may not want to keep these
unknown header lines, so it is best to leave the behaviour unspecified.
For example, it makes sense to retain "mergetag" because it is about the
parent, not the resulting commit.  It does not make sense to keep "gpgsig"
because it is about the commit you are rewriting to invalidate that old
signature.

^ permalink raw reply

* [PATCH] completion: --list option for git-branch
From: Ralf Thielow @ 2012-02-15 20:36 UTC (permalink / raw)
  To: spearce; +Cc: git, gitster, Ralf Thielow

Signed-off-by: Ralf Thielow <ralf.thielow@googlemail.com>
---
 contrib/completion/git-completion.bash |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index d7367e9..1505cff 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1137,7 +1137,7 @@ _git_branch ()
 		__gitcomp "
 			--color --no-color --verbose --abbrev= --no-abbrev
 			--track --no-track --contains --merged --no-merged
-			--set-upstream --edit-description
+			--set-upstream --edit-description --list
 			"
 		;;
 	*)
-- 
1.7.9.1

^ permalink raw reply related

* [PATCHv2 0/8] gitweb: Faster and improved project search
From: Jakub Narebski @ 2012-02-15 20:38 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski

[Cc-ing Junio because of his involvement in discussion about first
 patch in previous version of this series.]

First three patches in this series are mainly about speeding up
project search (and perhaps in the future also project pagination).
Well, first one is unification, refactoring and future-proofing.
The second and third patch could be squashed together; second adds
@fill_only, but third actually uses it.

Next set of patches is about highlighting matched part, making it
easier to recognize why project was selected, what we were searching
for (though better page title would also help second issue).

Well, fourth patch (first in set mentioned above) is here for the
commit message, otherwise it could have been squashed with next one.

Last patch in this series is beginning of using esc_html_match_hl()
for other searches in gitweb -- the easiest part.

Jakub Narebski (8):
  gitweb: Refactor checking if part of project info need filling
  gitweb: Option for filling only specified info in
    fill_project_list_info
  gitweb: Faster project search
  gitweb: Introduce esc_html_hl_regions
  gitweb: Highlight matched part of project name when searching
    projects
  gitweb: Highlight matched part of project description when searching
    projects
  gitweb: Highlight matched part of shortened project description
  gitweb: Use esc_html_match_hl() in 'grep' search

 gitweb/gitweb.perl |  158 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 135 insertions(+), 23 deletions(-)

-- 
1.7.9

^ permalink raw reply

* [PATCHv2 1/8] gitweb: Refactor checking if part of project info need filling
From: Jakub Narebski @ 2012-02-15 20:38 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski
In-Reply-To: <1329338332-30358-1-git-send-email-jnareb@gmail.com>

Extract the check if given keys (given parts) of project info needs to
be filled into project_info_needs_filling() subroutine.  It is for now
a thin wrapper around "!exists $project_info->{$key}".

Note that "!defined" was replaced by more correct "!exists".

While at it uniquify treating of all project info, adding checks for
'age' field before running git_get_last_activity(), and also checking
for all keys filled in code protected by conditional, and not only
one.

The code now looks like this

  foreach my $project (@$project_list) {
  	if (given keys need to be filled) {
  		fill given keys
  	}
  	...
  }

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch is new in this series, and splits off introduction of
project_info_needs_filling() function from partial filling via
@fill_only.

Hopefully this makes change more clear.

Note that this change stands alone, regardless of speeding up project
search.

 gitweb/gitweb.perl |   35 +++++++++++++++++++++++++++--------
 1 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 057ba5b..e62c2ef 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5195,6 +5195,21 @@ sub git_project_search_form {
 	print "</div>\n";
 }
 
+# entry for given @keys needs filling if at least one of keys in list
+# is not present in %$project_info
+sub project_info_needs_filling {
+	my ($project_info, @keys) = @_;
+
+	# return List::MoreUtils::any { !exists $project_info->{$_} } @keys;
+	foreach my $key (@keys) {
+		if (!exists $project_info->{$key}) {
+			return 1;
+		}
+	}
+	return;
+}
+
+
 # fills project list info (age, description, owner, category, forks)
 # for each project in the list, removing invalid projects from
 # returned list
@@ -5206,24 +5221,28 @@ sub fill_project_list_info {
 	my $show_ctags = gitweb_check_feature('ctags');
  PROJECT:
 	foreach my $pr (@$projlist) {
-		my (@activity) = git_get_last_activity($pr->{'path'});
-		unless (@activity) {
-			next PROJECT;
+		if (project_info_needs_filling($pr, 'age', 'age_string')) {
+			my (@activity) = git_get_last_activity($pr->{'path'});
+			unless (@activity) {
+				next PROJECT;
+			}
+			($pr->{'age'}, $pr->{'age_string'}) = @activity;
 		}
-		($pr->{'age'}, $pr->{'age_string'}) = @activity;
-		if (!defined $pr->{'descr'}) {
+		if (project_info_needs_filling($pr, 'descr', 'descr_long')) {
 			my $descr = git_get_project_description($pr->{'path'}) || "";
 			$descr = to_utf8($descr);
 			$pr->{'descr_long'} = $descr;
 			$pr->{'descr'} = chop_str($descr, $projects_list_description_width, 5);
 		}
-		if (!defined $pr->{'owner'}) {
+		if (project_info_needs_filling($pr, 'owner')) {
 			$pr->{'owner'} = git_get_project_owner("$pr->{'path'}") || "";
 		}
-		if ($show_ctags) {
+		if ($show_ctags &&
+		    project_info_needs_filling($pr, 'ctags')) {
 			$pr->{'ctags'} = git_get_project_ctags($pr->{'path'});
 		}
-		if ($projects_list_group_categories && !defined $pr->{'category'}) {
+		if ($projects_list_group_categories &&
+		    project_info_needs_filling($pr, 'category')) {
 			my $cat = git_get_project_category($pr->{'path'}) ||
 			                                   $project_list_default_category;
 			$pr->{'category'} = to_utf8($cat);
-- 
1.7.9

^ permalink raw reply related

* [PATCHv2 2/8] gitweb: Option for filling only specified info in fill_project_list_info
From: Jakub Narebski @ 2012-02-15 20:38 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski
In-Reply-To: <1329338332-30358-1-git-send-email-jnareb@gmail.com>

Introduce project_info_needs_filling($pr, $key[, \%fill_only]), which
is now used in place of simple 'defined $pr->{$key}' to check if
specific slot in project needs to be filled.

This is in preparation of future lazy filling of project info in
project search and pagination of sorted list of projects.  The only
functional change is that fill_project_list_info() now checks if 'age'
is already filled before running git_get_last_activity().

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This could have been squashed with the next commit, but this way it is
pure refactoring that shouldn't change gitweb behavior.

Changes from v1:
* Introduction of project_info_needs_filling() was moved to separate
  commit; this one just adds @fill_only / %fill_only, without actually 
  using it.

* It uses \%fill_only (reference to hash) rather than @fill_only,
  because project_info_needs_filling() now supports multiple keys,
  and because checking key in hash is faster O(1) than checking every
  element in array O(n).

  Though this shouldn't matter much, as @fill_only has at most two
  or three elements, as we would see in the next commit.

 gitweb/gitweb.perl |   34 +++++++++++++++++++++++-----------
 1 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e62c2ef..cae71f5 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5195,54 +5195,66 @@ sub git_project_search_form {
 	print "</div>\n";
 }
 
-# entry for given @keys needs filling if at least one of keys in list
-# is not present in %$project_info
+
+# entry for given @keys needs filling if at least one of interesting keys
+# in list is not present in %$project_info; key is interesting if $fill_only
+# is not passed, or is empty (all keys are interesting in both of those cases),
+# or if key is in $fill_only hash
+#
+# USAGE:
+# * project_info_needs_filling($project_info, 'key', ...)
+# * project_info_needs_filling($project_info, 'key', ..., \%fill_only)
+#   where %fill_only = map { $_ => 1 } @fill_only;
 sub project_info_needs_filling {
+	my $fill_only = ref($_[-1]) ? pop : undef;
 	my ($project_info, @keys) = @_;
 
 	# return List::MoreUtils::any { !exists $project_info->{$_} } @keys;
 	foreach my $key (@keys) {
-		if (!exists $project_info->{$key}) {
+		if ((!$fill_only || !%$fill_only || $fill_only->{$key}) &&
+		    !exists $project_info->{$key}) {
 			return 1;
 		}
 	}
 	return;
 }
 
-
 # fills project list info (age, description, owner, category, forks)
 # for each project in the list, removing invalid projects from
-# returned list
+# returned list, or fill only specified info (removing invalid projects
+# only when filling 'age').
+#
 # NOTE: modifies $projlist, but does not remove entries from it
 sub fill_project_list_info {
-	my $projlist = shift;
+	my ($projlist, @fill_only) = @_;
+	my %fill_only = map { $_ => 1 } @fill_only;
 	my @projects;
 
 	my $show_ctags = gitweb_check_feature('ctags');
  PROJECT:
 	foreach my $pr (@$projlist) {
-		if (project_info_needs_filling($pr, 'age', 'age_string')) {
+		if (project_info_needs_filling($pr, 'age', 'age_string', \%fill_only)) {
 			my (@activity) = git_get_last_activity($pr->{'path'});
 			unless (@activity) {
 				next PROJECT;
 			}
 			($pr->{'age'}, $pr->{'age_string'}) = @activity;
 		}
-		if (project_info_needs_filling($pr, 'descr', 'descr_long')) {
+		if (project_info_needs_filling($pr, 'descr', 'descr_long', \%fill_only)) {
 			my $descr = git_get_project_description($pr->{'path'}) || "";
 			$descr = to_utf8($descr);
 			$pr->{'descr_long'} = $descr;
 			$pr->{'descr'} = chop_str($descr, $projects_list_description_width, 5);
 		}
-		if (project_info_needs_filling($pr, 'owner')) {
+		if (project_info_needs_filling($pr, 'owner', \%fill_only)) {
 			$pr->{'owner'} = git_get_project_owner("$pr->{'path'}") || "";
 		}
 		if ($show_ctags &&
-		    project_info_needs_filling($pr, 'ctags')) {
+		    project_info_needs_filling($pr, 'ctags', \%fill_only)) {
 			$pr->{'ctags'} = git_get_project_ctags($pr->{'path'});
 		}
 		if ($projects_list_group_categories &&
-		    project_info_needs_filling($pr, 'category')) {
+		    project_info_needs_filling($pr, 'category', \%fill_only)) {
 			my $cat = git_get_project_category($pr->{'path'}) ||
 			                                   $project_list_default_category;
 			$pr->{'category'} = to_utf8($cat);
-- 
1.7.9

^ permalink raw reply related

* [PATCHv2 3/8] gitweb: Faster project search
From: Jakub Narebski @ 2012-02-15 20:38 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski
In-Reply-To: <1329338332-30358-1-git-send-email-jnareb@gmail.com>

Before searching by some field the information we search for must be
filled in.  For this fill_project_list_info() was enhanced in previous
commit to take additional parameters which part of projects info to
fill.  This way we can limit doing expensive calculations (like
running git-for-each-ref to get 'age' / "Last changed" info) only to
projects which we will show as search results.

With this commit the number of git commands used to generate search
results is 2*<matched projects> + 1, and depends on number of matched
projects rather than number of all projects (all repositories).

Note: this is 'git for-each-ref' to find last activity, and 'git config'
for each project, and 'git --version' once.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
search_projects_list() now pre-fills required parts of project info by
itself, so running fill_project_list_info() before calling it is no
longer necessary and actually you should not do it.

No changes from v1

 gitweb/gitweb.perl |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index cae71f5..cc2bd6d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2987,6 +2987,10 @@ sub search_projects_list {
 	return @$projlist
 		unless ($tagfilter || $searchtext);
 
+	# searching projects require filling to be run before it;
+	fill_project_list_info($projlist,
+	                       $tagfilter  ? 'ctags' : (),
+	                       $searchtext ? ('path', 'descr') : ());
 	my @projects;
  PROJECT:
 	foreach my $pr (@$projlist) {
@@ -5390,12 +5394,13 @@ sub git_project_list_body {
 	# filtering out forks before filling info allows to do less work
 	@projects = filter_forks_from_projects_list(\@projects)
 		if ($check_forks);
-	@projects = fill_project_list_info(\@projects);
-	# searching projects require filling to be run before it
+	# search_projects_list pre-fills required info
 	@projects = search_projects_list(\@projects,
 	                                 'searchtext' => $searchtext,
 	                                 'tagfilter'  => $tagfilter)
 		if ($tagfilter || $searchtext);
+	# fill the rest
+	@projects = fill_project_list_info(\@projects);
 
 	$order ||= $default_projects_order;
 	$from = 0 unless defined $from;
-- 
1.7.9

^ permalink raw reply related

* [PATCHv2 5/8] gitweb: Highlight matched part of project name when searching projects
From: Jakub Narebski @ 2012-02-15 20:38 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski
In-Reply-To: <1329338332-30358-1-git-send-email-jnareb@gmail.com>

Use newly introduced esc_html_match_hl() to escape HTML and mark match
with span element with 'match' class.  Currently only 'path' part
(i.e. project name) is highlighted; match might be on the project
description.

The code makes use of the fact that defined $search_regexp means that
there was search going on.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Introducing esc_html_match_hl() could have been put together with
introduction of esc_html_hl_regions() in previos commit.

Changes from v1:
* Main part of esc_html_match_hl() got split into esc_html_hl_regions(),
  which was introduced in previous commit.

 gitweb/gitweb.perl |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8dcd54b..5596701 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1742,6 +1742,20 @@ sub esc_html_hl_regions {
 	return $out;
 }
 
+# highlight match (if any), and escape HTML
+sub esc_html_match_hl {
+	my ($str, $regexp) = @_;
+	return esc_html($str) unless defined $regexp;
+
+	my @matches;
+	while ($str =~ /$regexp/g) {
+		push @matches, [$-[0], $+[0]];
+	}
+	return esc_html($str) unless @matches;
+
+	return esc_html_hl_regions($str, 'match', @matches);
+}
+
 ## ----------------------------------------------------------------------
 ## functions returning short strings
 
@@ -5389,7 +5403,9 @@ sub git_project_list_rows {
 			print "</td>\n";
 		}
 		print "<td>" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary"),
-		                        -class => "list"}, esc_html($pr->{'path'})) . "</td>\n" .
+		                        -class => "list"},
+		                       esc_html_match_hl($pr->{'path'}, $search_regexp)) .
+		      "</td>\n" .
 		      "<td>" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary"),
 		                        -class => "list", -title => $pr->{'descr_long'}},
 		                        esc_html($pr->{'descr'})) . "</td>\n" .
-- 
1.7.9

^ permalink raw reply related

* [PATCHv2 4/8] gitweb: Introduce esc_html_hl_regions
From: Jakub Narebski @ 2012-02-15 20:38 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski
In-Reply-To: <1329338332-30358-1-git-send-email-jnareb@gmail.com>

The esc_html_hl_regions() subroutine added in this commit is meant to
highlight in a given string a list of regions (given as a list of
[ beg, end ] pairs of positions in string), using HTML <span> element
with given class.

It is to be used in next commit for highlighting matched part in
project search.

Implementation and enhancement notes:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
* Currently esc_html_hl_regions() subroutine doesn't accept any
  parameters, like esc_html() does.  We might want for example to
  pass  nbsp=>1  to it.

  It can easily be done with the following code:

    my %opts = grep { ref($_) ne "ARRAY" } @sel;
    @sel     = grep { ref($_) eq "ARRAY" } @sel;

  This allow adding parameters after or before regions, e.g.:

    esc_html_hl_regions("foo bar", "mark", [ 0, 3 ], -nbsp => 1);

* esc_html_hl_regions() escapes like esc_html(); if we wanted to
  highlight with esc_path(), we could pass subroutine reference
  to now named esc_gen_hl_regions().

    esc_html_hl_regions("foo bar", "mark", \&esc_path, [ 0, 3 ]);

  Note that this way we can handle -nbsp=>1 case automatically,
  e.g.

    esc_html_hl_regions("foo bar", "mark",
                        sub { esc_html(@_, -nbsp=>1) },
                        [ 0, 3 ]);

* Alternate solution for highlighting region of a string would be to
  use the idea that strings are to be HTML-escaped, and references to
  scalars are HTML (like in the idea for generic committags).

  This would require modifying gitweb code or esc_html to get list of
  fragments, e.g.:

    esc_html(\'<span class="mark">', 'foo', \'</span>', ' bar',
             { -nbsp => 1 });

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This commit is here only for the notes in the commit message,
otherwise it could have been squashed with the next commit.

Note that we actually end up using first enhancement in notes section
of the above commit message.

This patch is new; it was not present in v1

 gitweb/gitweb.perl |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index cc2bd6d..8dcd54b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1715,6 +1715,33 @@ sub chop_and_escape_str {
 	}
 }
 
+# Highlight selected fragments of string, using given CSS class,
+# and escape HTML.  It is assumed that fragments do not overlap.
+# Regions are passed as list of pairs (array references).
+#
+# Example: esc_html_hl_regions("foobar", "mark", [ 0, 3 ]) returns
+# '<span class="mark">foo</span>bar'
+sub esc_html_hl_regions {
+	my ($str, $css_class, @sel) = @_;
+	return esc_html($str) unless @sel;
+
+	my $out = '';
+	my $pos = 0;
+
+	for my $s (@sel) {
+		$out .= esc_html(substr($str, $pos, $s->[0] - $pos))
+			if ($s->[0] - $pos > 0);
+		$out .= $cgi->span({-class => $css_class},
+		                   esc_html(substr($str, $s->[0], $s->[1] - $s->[0])));
+
+		$pos = $s->[1];
+	}
+	$out .= esc_html(substr($str, $pos))
+		if ($pos < length($str));
+
+	return $out;
+}
+
 ## ----------------------------------------------------------------------
 ## functions returning short strings
 
-- 
1.7.9

^ permalink raw reply related

* [PATCHv3/RFC 7/8] gitweb: Highlight matched part of shortened project description
From: Jakub Narebski @ 2012-02-15 20:38 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski
In-Reply-To: <1329338332-30358-1-git-send-email-jnareb@gmail.com>

Previous commit make gitweb use esc_html_match_hl() to mark match in
the _whole_ description of a project when searching projects.

This commit makes gitweb highlight match in _shortened_ description,
based on match in whole description, using esc_html_match_hl_chopped()
subroutine.

If match is in removed (chopped) part, even partially, then trailing
"... " is highlighted.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
It is marked as RFC, because I am not sure if it is right way to
highlight match in shortened string, or if we better use full string,
or full string if match is in chopped part.

Changes from v2:
* Harden esc_html_match_hl_chopped() against calling with both
  $chopped and $regexp undefined (even though it wouldn't happen
  with current code).

* esc_html_match_hl_chopped() uses now esc_html_hl_regions(),
  like esc_html_match_hl() used to do.

Changes from v1:
* Instead of esc_html_match_hl_chopped() duplicating much od code in
  esc_html_match_hl(), make esc_html_match_hl() call the *_chopped()
  one with $chopped set to undef.

  Now managing highlighting of $chopped part is just a matter of
  adjusting and filtering @matches to apply to $chopped rather than
  original $str where match was performed.

  As a side issue when match span past chop point current code uses
  one selection <span class+match">foo... </span> and not two
  <span class="match">foo</span><span class="match">... </span>

 gitweb/gitweb.perl |   40 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a109ebb..a2e2023 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1747,11 +1747,46 @@ sub esc_html_match_hl {
 	my ($str, $regexp) = @_;
 	return esc_html($str) unless defined $regexp;
 
+	return esc_html_match_hl_chopped($str, undef, $regexp);
+}
+
+
+# highlight match (if any) of shortened string, and escape HTML
+sub esc_html_match_hl_chopped {
+	my ($str, $chopped, $regexp) = @_;
+	return esc_html(defined $chopped ? $chopped : $str) unless defined $regexp;
+
 	my @matches;
 	while ($str =~ /$regexp/g) {
 		push @matches, [$-[0], $+[0]];
 	}
-	return esc_html($str) unless @matches;
+	return esc_html(defined $chopped ? $chopped : $str) unless @matches;
+
+	# filter matches so that we mark chopped string, if it is present
+	if (defined $chopped) {
+		my $tail = "... "; # see chop_str
+		unless ($chopped =~ s/\Q$tail\E$//) {
+			$tail = '';
+		}
+		my $chop_len = length($chopped);
+		my $tail_len = length($tail);
+		my @filtered;
+
+		for my $m (@matches) {
+			if ($m->[0] > $chop_len) {
+				push @filtered, [ $chop_len, $chop_len + $tail_len ] if ($tail_len > 0);
+				last;
+			} elsif ($m->[1] > $chop_len) {
+				push @filtered, [ $m->[0], $chop_len + $tail_len ];
+				last;
+			}
+			push @filtered, $m;
+		}
+
+		# further operations are on chopped string
+		$str = $chopped . $tail;
+		@matches = @filtered;
+	}
 
 	return esc_html_hl_regions($str, 'match', @matches);
 }
@@ -5409,7 +5444,8 @@ sub git_project_list_rows {
 		      "<td>" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary"),
 		                        -class => "list", -title => $pr->{'descr_long'}},
 		                        $search_regexp
-		                        ? esc_html_match_hl($pr->{'descr_long'}, $search_regexp)
+		                        ? esc_html_match_hl_chopped($pr->{'descr_long'},
+		                                                    $pr->{'descr'}, $search_regexp)
 		                        : esc_html($pr->{'descr'})) .
 		      "</td>\n" .
 		      "<td><i>" . chop_and_escape_str($pr->{'owner'}, 15) . "</i></td>\n";
-- 
1.7.9

^ permalink raw reply related

* [PATCHv2 8/8] gitweb: Use esc_html_match_hl() in 'grep' search
From: Jakub Narebski @ 2012-02-15 20:38 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski
In-Reply-To: <1329338332-30358-1-git-send-email-jnareb@gmail.com>

Use esc_html_match_hl() in git_search_files() i.e. subroutine that
implements "grep" search, instead of custom code which highlighted
only one, last match.

This required enhancing esc_html_match_hl() to accept -nbsp=>1 and
pass it down to esc_html().

Note that line is untabified (tabs turned into spaces) before
highlighting match, which means that highlighting won't work e.g. for
matching tab character "\t" explicitly; but this issue was present
before this commit, and is not that easy to fix.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch was not present in previous (v1) version of this patch
series.

The 'grep' search was chosen from other searches because of the
following reasons:
* 'pickaxe' search does not show matches in diff, only filenames.
* 'commit' search shortens leading text at beginning, one last match
  in the middle, and trailing text at the end; anyway I think this
  search should be rewritten to show just "log"-like view with match
  highlighting.

 gitweb/gitweb.perl |   34 ++++++++++++++--------------------
 1 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a2e2023..36a8cca 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1723,20 +1723,22 @@ sub chop_and_escape_str {
 # '<span class="mark">foo</span>bar'
 sub esc_html_hl_regions {
 	my ($str, $css_class, @sel) = @_;
-	return esc_html($str) unless @sel;
+	my %opts = grep { ref($_) ne 'ARRAY' } @sel;
+	@sel     = grep { ref($_) eq 'ARRAY' } @sel;
+	return esc_html($str, %opts) unless @sel;
 
 	my $out = '';
 	my $pos = 0;
 
 	for my $s (@sel) {
-		$out .= esc_html(substr($str, $pos, $s->[0] - $pos))
+		$out .= esc_html(substr($str, $pos, $s->[0] - $pos), %opts)
 			if ($s->[0] - $pos > 0);
 		$out .= $cgi->span({-class => $css_class},
-		                   esc_html(substr($str, $s->[0], $s->[1] - $s->[0])));
+		                   esc_html(substr($str, $s->[0], $s->[1] - $s->[0]), %opts));
 
 		$pos = $s->[1];
 	}
-	$out .= esc_html(substr($str, $pos))
+	$out .= esc_html(substr($str, $pos), %opts)
 		if ($pos < length($str));
 
 	return $out;
@@ -1744,23 +1746,23 @@ sub esc_html_hl_regions {
 
 # highlight match (if any), and escape HTML
 sub esc_html_match_hl {
-	my ($str, $regexp) = @_;
-	return esc_html($str) unless defined $regexp;
+	my ($str, $regexp, %opts) = @_;
+	return esc_html($str, %opts) unless defined $regexp;
 
-	return esc_html_match_hl_chopped($str, undef, $regexp);
+	return esc_html_match_hl_chopped($str, undef, $regexp, %opts);
 }
 
 
 # highlight match (if any) of shortened string, and escape HTML
 sub esc_html_match_hl_chopped {
-	my ($str, $chopped, $regexp) = @_;
-	return esc_html(defined $chopped ? $chopped : $str) unless defined $regexp;
+	my ($str, $chopped, $regexp, %opts) = @_;
+	return esc_html(defined $chopped ? $chopped : $str, %opts) unless defined $regexp;
 
 	my @matches;
 	while ($str =~ /$regexp/g) {
 		push @matches, [$-[0], $+[0]];
 	}
-	return esc_html(defined $chopped ? $chopped : $str) unless @matches;
+	return esc_html(defined $chopped ? $chopped : $str, %opts) unless @matches;
 
 	# filter matches so that we mark chopped string, if it is present
 	if (defined $chopped) {
@@ -1788,7 +1790,7 @@ sub esc_html_match_hl_chopped {
 		@matches = @filtered;
 	}
 
-	return esc_html_hl_regions($str, 'match', @matches);
+	return esc_html_hl_regions($str, 'match', @matches, %opts);
 }
 
 ## ----------------------------------------------------------------------
@@ -6070,15 +6072,7 @@ sub git_search_files {
 			print "<div class=\"binary\">Binary file</div>\n";
 		} else {
 			$ltext = untabify($ltext);
-			if ($ltext =~ m/^(.*)($search_regexp)(.*)$/i) {
-				$ltext = esc_html($1, -nbsp=>1);
-				$ltext .= '<span class="match">';
-				$ltext .= esc_html($2, -nbsp=>1);
-				$ltext .= '</span>';
-				$ltext .= esc_html($3, -nbsp=>1);
-			} else {
-				$ltext = esc_html($ltext, -nbsp=>1);
-			}
+			$ltext = esc_html_match_hl($ltext, qr/$search_regexp/i, -nbsp=>1);
 			print "<div class=\"pre\">" .
 				$cgi->a({-href => $file_href.'#l'.$lno,
 				        -class => "linenr"}, sprintf('%4i', $lno)) .
-- 
1.7.9

^ permalink raw reply related

* [PATCHv2 6/8] gitweb: Highlight matched part of project description when searching projects
From: Jakub Narebski @ 2012-02-15 20:38 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jakub Narebski
In-Reply-To: <1329338332-30358-1-git-send-email-jnareb@gmail.com>

Use esc_html_match_hl() from previous commit to mark match in the
_whole_ description when searching projects.

Currently, with this commit, when searching projects there is always
shown full description of a project, and not a shortened one (like for
ordinary projects list view), even if the match is on project name and
not project description.

Showing full description when there is match on it is useful to avoid
situation where match is in shortened, invisible part... well, perhaps
that could be solved (showing shortened description), but it would
require much more complicated code.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
The part about showing match using shortened description no longer
applies after the following patch... though it is an RFC for now,
that is why it is not mentioned in the commit message.

NOTE that we are actually always showing full description, so having
full description in "title" attribute to show on mouseover over
shortened title is no longer necessary, and should probably be
fixed... that is unless the next patch that highlights matches in
shortened description is accepted.

No changes from previous version (but see paragraph above about
possible changes).

 gitweb/gitweb.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 5596701..a109ebb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5408,7 +5408,10 @@ sub git_project_list_rows {
 		      "</td>\n" .
 		      "<td>" . $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary"),
 		                        -class => "list", -title => $pr->{'descr_long'}},
-		                        esc_html($pr->{'descr'})) . "</td>\n" .
+		                        $search_regexp
+		                        ? esc_html_match_hl($pr->{'descr_long'}, $search_regexp)
+		                        : esc_html($pr->{'descr'})) .
+		      "</td>\n" .
 		      "<td><i>" . chop_and_escape_str($pr->{'owner'}, 15) . "</i></td>\n";
 		print "<td class=\"". age_class($pr->{'age'}) . "\">" .
 		      (defined $pr->{'age_string'} ? $pr->{'age_string'} : "No commits") . "</td>\n" .
-- 
1.7.9

^ permalink raw reply related

* [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'
From: Brian Norris @ 2012-02-15 21:42 UTC (permalink / raw)
  To: git; +Cc: Brian Norris

According to the manpage:

 --smtp-encryption=<encryption>
        Specify the encryption to use, either ssl or tls. Any other value
	reverts to plain SMTP.  Default is the value of
        sendemail.smtpencryption.

However, if I have already set sendemail.smtpencryption but try to override
it with something like 'no', the authentication code block still tries to ask
for a password (and fails).

This patch forces $smtp_encryption to 'none' when a proper encryption type is
not provided, then checks $smtp_encryption before proceeding to authentication.

Example execution:

 $ git send-email --smtp-encryption=no --smtp-domain=<xxx> \
        --smtp-server=<xxx> --smtp-server-port=25 myfile.patch
 Password:
 Command unknown: 'AUTH' at /usr/local/libexec/git-core/git-send-email
 line 1115, <STDIN> line 1.

Tested on Git 1.7.5.4 and 1.7.9.1.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 git-send-email.perl |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index ef30c55..fa0a384 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -385,8 +385,9 @@ foreach my $setting (values %config_bool_settings) {
 	${$setting->[0]} = $setting->[1] unless (defined (${$setting->[0]}));
 }
 
-# 'default' encryption is none -- this only prevents a warning
-$smtp_encryption = '' unless (defined $smtp_encryption);
+# 'default' encryption is none
+$smtp_encryption = 'none' unless (defined $smtp_encryption);
+$smtp_encryption = 'none' unless ($smtp_encryption eq 'tls' || $smtp_encryption eq 'ssl');
 
 # Set CC suppressions
 my(%suppress_cc);
@@ -1113,7 +1114,7 @@ X-Mailer: git-send-email $gitversion
 			    defined $smtp_server_port ? " port=$smtp_server_port" : "";
 		}
 
-		if (defined $smtp_authuser) {
+		if (defined $smtp_authuser && $smtp_encryption ne 'none') {
 			# Workaround AUTH PLAIN/LOGIN interaction defect
 			# with Authen::SASL::Cyrus
 			eval {
-- 
1.7.5.4

^ permalink raw reply related

* Re: [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'
From: Jeff King @ 2012-02-15 22:06 UTC (permalink / raw)
  To: Brian Norris; +Cc: git
In-Reply-To: <1329342178-14540-1-git-send-email-computersforpeace@gmail.com>

On Wed, Feb 15, 2012 at 01:42:58PM -0800, Brian Norris wrote:

> According to the manpage:
> 
>  --smtp-encryption=<encryption>
>         Specify the encryption to use, either ssl or tls. Any other value
> 	reverts to plain SMTP.  Default is the value of
>         sendemail.smtpencryption.
> 
> However, if I have already set sendemail.smtpencryption but try to override
> it with something like 'no', the authentication code block still tries to ask
> for a password (and fails).

Sounds reasonable.

> This patch forces $smtp_encryption to 'none' when a proper encryption type is
> not provided, then checks $smtp_encryption before proceeding to authentication.

Defaulting everything except "ssl" or "tls" to "none" seems risky to me.
If I am understanding your patch correctly, then doing this:

  git send-email --smtp-encryption=SSL

will silently treat that as "don't do encryption", which could have
surprising security implications for the user. I chose all-caps as it is
an obvious mistake to make. We probably should treat it the same as
lowercase "ssl", but the same argument applies to other typos like
"tsl".

It seems like a much safer default would be to die() on an invalid
encryption specifier.

-Peff

^ permalink raw reply

* Re: [PATCH v2 2/2] submodules: always use a relative path from gitdir to work tree
From: Jens Lehmann @ 2012-02-15 22:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, Antony Male, Phil Hord
In-Reply-To: <7vzkcl5f37.fsf@alter.siamese.dyndns.org>

Am 14.02.2012 21:34, schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>
>>> After adding a comment, using test instead of [], testing both $a and
>>> $b and assigning each variable on it's own line I get the following
>>> interdiff. Does that make more sense?
>>
>> My earlier request for comment was to say
>>
>> 	# $a is always longer than $b for such and such reasons
>>
>> to explain why testing $b without testing $a was sufficient.
> 
> Heh, after I follow the entire module_clone, $gitdir is defined in earlier
> parts of the function to be "$(rev-parse --git-dir)/modules/$path", so it
> is clear that it is longer than $path.

Unfortunately only by accident. The usage of $path is not correct here,
$name should be used instead (I have a patch in the making to correct that,
but as that hits the same code area as these fixes I'll post that later
together with some tests moving submodules around inside a superproject).
Then the result of "$(rev-parse --git-dir)/modules/$name" can be shorter
than "$path" when a submodule is renamed into a higher directory level.

> Unless "cd $there && pwd" does not
> result in a funny situation (such as $something/modules is a symbolic link
> to another place that is much closer to the root of the filesystem), that
> is.
> 
> And in such a case, the prefix part of $a and $b would be different from
> the very beginning hopefully.

Yes, they should differ somewhere in any sane setup I can imagine.

>> It is obvious (at least to me) that the loop continues as long as $a and
>> $b begin with the same string before their first '/' and removes that
>> common segment from both of them, so I do not think the new comment is
>> absolutely necessary, but it would not hurt to have it, especially it is
>> short enough and to the point.
>>
>> Thanks.
>>
>>> diff --git a/git-submodule.sh b/git-submodule.sh
>>> index 3463d6d..ed76ce2 100755
>>> --- a/git-submodule.sh
>>> +++ b/git-submodule.sh
>>> @@ -172,9 +172,11 @@ module_clone()
>>>
>>>         a=$(cd "$gitdir" && pwd)
>>>         b=$(cd "$path" && pwd)
>>> -       while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
>>> +       # Remove all common leading directories
>>> +       while test -n "$a" && test -n "$b" && test "${a%%/*}" = "${b%%/*}"
>>>         do
>>> -               a=${a#*/} b=${b#*/};
>>> +               a=${a#*/}
>>> +               b=${b#*/}
>>>         done
>>>         rel=$(echo $a | sed -e 's|[^/]*|..|g')
> 
> Perhaps aseert that $a never becomes empty before this line (or set it
> explicitly to "." when $a is empty), as otherwise
> 
>>>         (clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")
> 
> this will refer to "/$b" from the root?

I think neither $a nor $b should be empty after that. But thinking deeper
about that while loop I suspect the real problem here is doing "a=${a#*/}"
or "b=${b#*/}" on a string that doesn't contain a slash anymore. We'll
happily remove a leading directory on the other path while the one without
slash will stay untouched, leading to a bogus result which is off by one
directory level.

AFAICS that will only happen when one path is a prefix of the other, which
is a pretty pathological case. So I'll whip up a new version asserting
that beforehand and dropping the -n test in the while loop, ok?

^ permalink raw reply

* Re: [PATCH v5 3/3] push: teach --recurse-submodules the on-demand option
From: Jens Lehmann @ 2012-02-15 22:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Heiko Voigt, git, Fredrik Gustafsson
In-Reply-To: <7v7gzq9jg2.fsf@alter.siamese.dyndns.org>

Am 14.02.2012 04:34, schrieb Junio C Hamano:
> Heiko Voigt <hvoigt@hvoigt.net> writes:
> 
>> diff --git a/submodule.c b/submodule.c
>> index 3c714c2..ff0cfd8 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -411,6 +411,54 @@ int check_submodule_needs_pushing(unsigned char new_sha1[20],
>>  	return needs_pushing->nr;
>>  }
>>  
>> +static int push_submodule(const char *path)
>> +{
>> +	if (add_submodule_odb(path))
>> +		return 1;
>> +
>> +	if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
>> +		struct child_process cp;
>> +		const char *argv[] = {"push", NULL};
>> +
>> +		memset(&cp, 0, sizeof(cp));
>> +		cp.argv = argv;
>> +		cp.env = local_repo_env;
>> +		cp.git_cmd = 1;
>> +		cp.no_stdin = 1;
>> +		cp.dir = path;
>> +		if (run_command(&cp))
>> +			return 0;
>> +		close(cp.out);
>> +	}
>> +
>> +	return 1;
>> +}
> 
> Hmm, this makes me wonder if we fire subprocesses and have them run in
> parallel (to a reasonably limited parallelism), it might make the overall
> user experience more pleasant, and if we did the same on the fetching
> side, it would be even nicer.

Yeah, I had the same idea and did some experiments when working on
fetch some time ago.

> We would need to keep track of children and after firing a handful of them
> we would need to start waiting for some to finish and collect their exit
> status before firing more, and at the end we would need to wait for the
> remaining ones and find how each one of them did before returning from
> push_unpushed_submodules().  If we were to do so, what are the missing
> support we would need from the run_command() subsystem?

We would not only have to collect the exit status but also the output
lines. You don't want to see the output of multiple fetches or pushes
mixed together, so it makes sense to just defer that until the command
exited and then print everything at once. The interesting part I couldn't
come up with an easy solution for is to preserve the output order between
the stdout and stdin lines, as they contain different parts of the
progress which would look strange when shuffled around.

And I saw that sometimes parallel fetches took way longer than doing them
sequentially (in my case because of strange DNS behavior of my DSL router),
so we would definitely want a config option for that (maybe setting the
maximum number of simultaneous threads to be used).

But don't get me wrong, I'm all for having that feature! :-)

^ permalink raw reply

* Re: [PATCH] git-latexdiff: new command in contrib, to use latexdiff and Git
From: Tim Haga @ 2012-02-15 23:33 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, gitster
In-Reply-To: <1329320987-15203-1-git-send-email-Matthieu.Moy@imag.fr>

While testing your script on my office machine i discovered that the
following might be a problem:

> +if [ "$view" = 1 ] || [ "$view" = maybe ] && [ "$output" = "" ]; then
> +    xpdf "$pdffile"
> +fi

Xpdf is not installed on all machines (e.g. it's not installed on my
office machine), so maybe it would be a good idea to use a environment
variable instead?


Tim

^ permalink raw reply

* Re: [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'
From: Brian Norris @ 2012-02-15 23:49 UTC (permalink / raw)
  To: Jeff King; +Cc: git
In-Reply-To: <20120215220629.GA17672@sigill.intra.peff.net>

On Wed, Feb 15, 2012 at 2:06 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Feb 15, 2012 at 01:42:58PM -0800, Brian Norris wrote:
>
>> According to the manpage:
>>
>>  --smtp-encryption=<encryption>
>>         Specify the encryption to use, either ssl or tls. Any other value
>>       reverts to plain SMTP.  Default is the value of
>>         sendemail.smtpencryption.
>>
>> However, if I have already set sendemail.smtpencryption but try to override
>> it with something like 'no', the authentication code block still tries to ask
>> for a password (and fails).
>
> Sounds reasonable.

An error like
  Command unknown: 'AUTH' at /usr/local/libexec/git-core/git-send-email
is reasonable?

>> This patch forces $smtp_encryption to 'none' when a proper encryption type is
>> not provided, then checks $smtp_encryption before proceeding to authentication.
>
> Defaulting everything except "ssl" or "tls" to "none" seems risky to me.
> If I am understanding your patch correctly, then doing this:
>
>  git send-email --smtp-encryption=SSL
>
> will silently treat that as "don't do encryption", which could have
> surprising security implications for the user. I chose all-caps as it is
> an obvious mistake to make. We probably should treat it the same as
> lowercase "ssl", but the same argument applies to other typos like
> "tsl".

Well, git-send-email already doesn't handle typos or capitalization
correctly, AFAICT. So nothing new here.

> It seems like a much safer default would be to die() on an invalid
> encryption specifier.

Fine. But then we need to define a behavior that means 'no
encryption.' Like 'none' instead of just saying 'anything but tls or
ssl.'

Now that I look at this again, I think part of the issue I have is
that there is no way to override *smtp-user* via command-line, in
order to do unencrypted, unauthenticated email. So the
*authentication* not the encryption is really my main problem...I'll
take another look and try a new patch.

Thanks,
Brian

^ permalink raw reply

* diff -G with case insensitivity
From: Chris Leong @ 2012-02-16  0:18 UTC (permalink / raw)
  To: git

Is there any way to run diff -G with a case insensitivity flag?

^ permalink raw reply

* Re: diff -G with case insensitivity
From: Chris Leong @ 2012-02-16  0:33 UTC (permalink / raw)
  To: git
In-Reply-To: <CAJ6vYjejtZkupy750rvz6HW_0SNPyBVTa78DO4nY8Bi368neQw@mail.gmail.com>

Also, is there a method to restrict it to only showing the chunks that
match - rather than files?

On Thu, Feb 16, 2012 at 11:18 AM, Chris Leong <walkraft@gmail.com> wrote:
> Is there any way to run diff -G with a case insensitivity flag?

^ permalink raw reply

* Re: [PATCH] git-send-email: allow overriding smtp-encryption config to 'none'
From: Jeff King @ 2012-02-16  0:49 UTC (permalink / raw)
  To: Brian Norris; +Cc: git
In-Reply-To: <CAN8TOE_BnkOcMQRTY-GWrHozYD0+0giWn2LtjB8AVnP_DzA+Sg@mail.gmail.com>

On Wed, Feb 15, 2012 at 03:49:59PM -0800, Brian Norris wrote:

> > Sounds reasonable.
> 
> An error like
>   Command unknown: 'AUTH' at /usr/local/libexec/git-core/git-send-email
> is reasonable?

Sorry, no, I meant your goal of allowing overriding config sounds like a
reasonable thing to want. But from reading your message below, it seems
that is not actually the problem you are trying to solve.

> > Defaulting everything except "ssl" or "tls" to "none" seems risky to me.
> > If I am understanding your patch correctly, then doing this:
> >
> >  git send-email --smtp-encryption=SSL
> >
> > will silently treat that as "don't do encryption", which could have
> > surprising security implications for the user. I chose all-caps as it is
> > an obvious mistake to make. We probably should treat it the same as
> > lowercase "ssl", but the same argument applies to other typos like
> > "tsl".
> 
> Well, git-send-email already doesn't handle typos or capitalization
> correctly, AFAICT. So nothing new here.

Hmm. From your description and the patch, I thought that was something
introduced by your patch. But looking at the existing code, it seems
like that is already the case. IOW, I don't understand why
"--smtp-encryption=none" does not already work looking at the current
code.

So being more careful about typos is an improvement we could make, but
it is not a feature that would need to be part of a bugfix patch.

> > It seems like a much safer default would be to die() on an invalid
> > encryption specifier.
> 
> Fine. But then we need to define a behavior that means 'no
> encryption.' Like 'none' instead of just saying 'anything but tls or
> ssl.'

Right. I meant that you should introduce "none" as an explicit "no, I
don't want this" and die when the flag is not one of {ssl, tls, none}.

> Now that I look at this again, I think part of the issue I have is
> that there is no way to override *smtp-user* via command-line, in
> order to do unencrypted, unauthenticated email. So the
> *authentication* not the encryption is really my main problem...I'll
> take another look and try a new patch.

Ah, I see. I misunderstood the original problem you were trying to solve
(I thought your example was "see? Encryption is off, so the server won't
do AUTH, demonstrating that the patch works.").

Overriding the smtp user from the config is a separate issue, and I
don't think that is currently possible. The usual way to spell an option
like that in git is "--no-smtp-user", but it seems that we use perl's
GetOptions, which does not understand that syntax. So you'd have to add
a "--no-smtp-user" by hand.

-Peff

^ permalink raw reply

* importance of .git extension for bare repos
From: Neal Kreitzinger @ 2012-02-16  1:04 UTC (permalink / raw)
  To: git

I have a user who has created bare repos without naming them with the .git 
extension.  As a result, the file paths are MYBARE/config instead of 
MYBARE.git/config, etc.  Is this a supported naming convention for bare 
repos in git.git, or is it going to have some problems?

v/r,
neal 

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox