date:20131126

Re: [PATCH] bash prompt: add option to disable for a repository

2013-11-26 Thread Thomas Rast

Heikki Hokkanen h...@users.sf.net writes:

 On Sat, Nov 23, 2013 at 4:42 PM, Johannes Sixt j...@kdbg.org wrote:
 Gah! This adds a fork+exec each time the prompt is shown. Not good,
 particularly on Windows.

 Since your intent is to disable the prompt in the home directory,
 wouldn't that mean that most of the time you *don't* want the prompt?
 Wouldn't you be better served with a method that *turns on* the prompt?
 For example, a shell function that sets PS1 and another one that unsets
 it? Or a wrapper that inspects a shell variable and calls __git_ps1 only
 when you want a prompt.

 Actually, I do want the prompt for all other git repositories. The
 problem with $HOME is that it's the default directory after logging in
 or opening a terminal, so if you have git prompt sourced and your
 $HOME under git, you get an unbearable delay every time you open a
 terminal, or type a command, anywhere, except for a separate git
 repository.

Umm... is __git_ps1 by itself so slow that you find it unbearable, or is
it the worktree status discovery?  Because the latter can already be
controlled per repository via bash.showUntrackedFiles and
bash.showUpstream.

-- 
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] gitweb: Add an option for adding more branch refs

2013-11-26 Thread Krzesimir Nowak

Overriding an @additional_branch_refs configuration variable with
value ('wip') will make gitweb to show branches that appear in
refs/heads and refs/wip (refs/heads is hardcoded). Might be useful for
gerrit setups where user branches are not stored under refs/heads/.

Signed-off-by: Krzesimir Nowak krzesi...@endocode.com
---
 gitweb/gitweb.perl | 99 --
 1 file changed, 74 insertions(+), 25 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 68c77f6..9bfd38b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -17,6 +17,7 @@ use Encode;
 use Fcntl ':mode';
 use File::Find qw();
 use File::Basename qw(basename);
+use List::Util qw(min);
 use Time::HiRes qw(gettimeofday tv_interval);
 binmode STDOUT, ':utf8';
 
@@ -122,6 +123,10 @@ our $logo_label = git homepage;
 # source of projects list
 our $projects_list = ++GITWEB_LIST++;
 
+# list of additional directories under refs/ we want to display as
+# branches
+our @additional_branch_refs = ();
+
 # the width (in characters) of the projects list Description column
 our $projects_list_description_width = 25;
 
@@ -626,14 +631,29 @@ sub feature_avatar {
return @val ? @val : @_;
 }
 
+sub get_branch_refs {
+return ('heads', @additional_branch_refs);
+}
+
 # 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.
 sub check_head_link {
my ($dir) = @_;
my $headfile = $dir/HEAD;
-   return ((-e $headfile) ||
-   (-l $headfile  readlink($headfile) =~ /^refs\/heads\//));
+
+   if (-e $headfile) {
+   return 1;
+   }
+   if (-l $headfile) {
+   my $rl = readlink($headfile);
+
+   for my $ref (get_branch_refs()) {
+   return 1 if $rl =~ /^refs\/$ref\//;
+   }
+   }
+
+   return 0;
 }
 
 sub check_export_ok {
@@ -2515,6 +2535,7 @@ sub format_snapshot_links {
 sub get_feed_info {
my $format = shift || 'Atom';
my %res = (action = lc($format));
+   my $matched_ref = 0;
 
# feed links are possible only for project views
return unless (defined $project);
@@ -2522,12 +2543,17 @@ sub get_feed_info {
# or don't have specific feed yet (so they should use generic)
return if (!$action || $action =~ /^(?:tags|heads|forks|tag|search)$/x);
 
-   my $branch;
-   # branches refs uses 'refs/heads/' prefix (fullname) to differentiate
-   # from tag links; this also makes possible to detect branch links
-   if ((defined $hash_base  $hash_base =~ m!^refs/heads/(.*)$!) ||
-   (defined $hash   $hash  =~ m!^refs/heads/(.*)$!)) {
-   $branch = $1;
+   my $branch = undef;
+   # branches refs uses 'refs/' + $get_branch_refs()[x] + '/' prefix
+   # (fullname) to differentiate from tag links; this also makes
+   # possible to detect branch links
+   for my $ref (get_branch_refs()) {
+   if ((defined $hash_base  $hash_base =~ m!^refs/$ref/(.*)$!) ||
+   (defined $hash   $hash  =~ m!^refs/$ref/(.*)$!)) {
+   $branch = $1;
+   $matched_ref = $ref;
+   last;
+   }
}
# find log type for feed description (title)
my $type = 'log';
@@ -2540,7 +2566,7 @@ sub get_feed_info {
}
 
$res{-title} = $type;
-   $res{'hash'} = (defined $branch ? refs/heads/$branch : undef);
+   $res{'hash'} = (defined $branch ? refs/$matched_ref/$branch : undef);
$res{'file_name'} = $file_name;
 
return %res;
@@ -3184,24 +3210,43 @@ sub git_get_project_owner {
return $owner;
 }
 
-sub git_get_last_activity {
-   my ($path) = @_;
-   my $fd;
+sub git_get_last_activity_age {
+   my ($refs) = @_;
+   my $fd = -1;
 
-   $git_dir = $projectroot/$path;
open($fd, -|, git_cmd(), 'for-each-ref',
 '--format=%(committer)',
 '--sort=-committerdate',
 '--count=1',
-'refs/heads') or return;
+$refs) or return undef;
+
my $most_recent = $fd;
-   close $fd or return;
+   close $fd or return undef;
if (defined $most_recent 
$most_recent =~ / (\d+) [-+][01]\d\d\d$/) {
my $timestamp = $1;
-   my $age = time - $timestamp;
-   return ($age, age_string($age));
+   return time - $timestamp;
}
+
+   return undef;
+}
+
+sub git_get_last_activity {
+   my ($path) = @_;
+   my @ages = ();
+
+   $git_dir = $projectroot/$path;
+   for my $ref (get_branch_refs()) {
+   my $age = git_get_last_activity_age('refs/' . $_);
+
+   push @ages, $age if defined $age;
+   }
+   if (@ages) {
+   my $min_age = min(@ages);
+
+

Re: [PATCH] gitweb: Make showing branches configurable

2013-11-26 Thread Krzesimir Nowak

On Mon, 2013-11-25 at 11:32 -0800, Junio C Hamano wrote:
 Krzesimir Nowak krzesi...@endocode.com writes:
 
  On Fri, 2013-11-22 at 09:34 -0800, Junio C Hamano wrote:
  Krzesimir Nowak krzesi...@endocode.com writes:
  
   Running 'make GITWEB_WANTED_REFS=heads wip gitweb.cgi' will create a
   gitweb CGI script showing branches that appear in refs/heads/ and in
   refs/wip/. Might be useful for gerrit setups where user branches are
   not stored under refs/heads/.
  
   Signed-off-by: Krzesimir Nowak krzesi...@endocode.com
   ---
  
   Notes:
   I'm actually not sure if all those changes are really necessary as I
   was mostly targeting it for Gerrit use. Especially I mean the changes
   in git_get_remotes_list, fill_remote_heads and print_page_nav. I 
   tried
   to make it as general as it gets, so there's nothing Gerrit specific.
  
  Thanks.
  
  Two knee-jerk reactions after a quick scan.
  
   - You include heads for normal builds by hardcoded
 GITWEB_WANTED_REFS = heads but include tags unconditionally
 by having @ref_views = (tags, @wanted_refs) in the code.  Why?
  
 
  Earlier both tags and heads were hardcoded there. So now instead of
  heads we have @wanted_refs.
 
  I suppose I should have given it a better name, like @branch_refs.
  Right?
 
 My original question was why the change was not done like this:
 
   In gitweb/Makefile, to give the default that is the same as
   before:
 
   GITWEB_WANTED_REFS = heads tags
 
 
   In format_refs_views
 
   my @ref_views = @wanted_refs;
 
 But looking at the existing code again, you are only interested in
 renaming 'heads' to some other possibly multiple hierarchies, so
 that would not fly well.  Indeed, as you said, wanted refs is a
 misnomer that led to the above confusion.  If it is only about what
 are the branches, then the configuration should be named after the
 branch ness of that thing.
 
 But that leads to another question.  Is there _any_ use case where
 showing 'heads/' hierarchy is undesirable?  It would be utterly
 confusing if something that claims to show branches does not include
 the 'heads/', even though it might be acceptable if it showed other
 things.

Agreed, although certainly such setup is possible, albeit inpractical
and, as you said, confusing. Also, remembering about adding 'heads' to
overriden config variable is bothersome. Gitweb rather does not need to
support such unusual setup then.

 
   - Does this have be a compile-time decision?  It looks like this is
 something that can and should be made controllable with the
 normal gitweb configuration mechanism.
  
 
  Maybe. I was just looking at Makefile and saw a bunch of configuration
  options there, so I just added another one. Haven't noticed the gitweb
  config thing. Sorry.
 
  So, we should just hardcode the @wanted_refs (or @branch_refs after the
  rename) to simply ('heads'), let it be overriden by perl gitweb config
  file and get rid of a new substitution from Makefile?
 
 Something along that line, but perhaps use @additional_branch_refs
 to allow users to specify additional hierarchies to be shown, and
 always include 'heads' to where we currently show 'heads' hierarchy,
 just like your version of format_refs_views always included 'tags'
 hierarchy regardless of the setting of @wanted_refs?
 

Right. New patch in topic [PATCH] gitweb: Add an option for adding more
branch refs will follow.

 Thanks.
 
  
gitweb/Makefile|  4 ++-
gitweb/gitweb.perl | 94 
   +++---
2 files changed, 72 insertions(+), 26 deletions(-)
  
   diff --git a/gitweb/Makefile b/gitweb/Makefile
   index cd194d0..361dce9 100644
   --- a/gitweb/Makefile
   +++ b/gitweb/Makefile
   @@ -38,6 +38,7 @@ GITWEB_SITE_HTML_HEAD_STRING =
GITWEB_SITE_HEADER =
GITWEB_SITE_FOOTER =
HIGHLIGHT_BIN = highlight
   +GITWEB_WANTED_REFS = heads

# include user config
-include ../config.mak.autogen
   @@ -148,7 +149,8 @@ GITWEB_REPLACE = \
-e 
   's|++GITWEB_SITE_HTML_HEAD_STRING++|$(GITWEB_SITE_HTML_HEAD_STRING)|g' \
-e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
-e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
   --e 's|++HIGHLIGHT_BIN++|$(HIGHLIGHT_BIN)|g'
   +-e 's|++HIGHLIGHT_BIN++|$(HIGHLIGHT_BIN)|g' \
   +-e 's|++GITWEB_WANTED_REFS++|$(GITWEB_WANTED_REFS)|g'

GITWEB-BUILD-OPTIONS: FORCE
@rm -f $@+
   diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
   index 68c77f6..8bc9e9a 100755
   --- a/gitweb/gitweb.perl
   +++ b/gitweb/gitweb.perl
   @@ -17,6 +17,7 @@ use Encode;
use Fcntl ':mode';
use File::Find qw();
use File::Basename qw(basename);
   +use List::Util qw(min);
use Time::HiRes qw(gettimeofday tv_interval);
binmode STDOUT, ':utf8';

   @@ -122,6 +123,9 @@ our $logo_label = git homepage;
# source of projects list
our $projects_list =

Re: [PATCHv3] transport: Catch non positive --depth option value

2013-11-26 Thread Andrés G. Aragoneses

On 26/11/13 04:06, Duy Nguyen wrote:
 On Tue, Nov 26, 2013 at 6:34 AM, Andrés G. Aragoneses
 kno...@gmail.com wrote:
 On 22/11/13 02:18, Duy Nguyen wrote:
 On Fri, Nov 22, 2013 at 3:18 AM, Junio C Hamano gits...@pobox.com wrote:
 Have you run the tests with this patch?  It seems that it breaks
 quite a lot of them, including t5500, t5503, t5510, among others.

 I guess it's caused by builtin/fetch.c:backfill_tags(). And the call
 could be replaced with

 transport_set_option(transport, TRANS_OPT_DEPTH, NULL);


 Not sure what you mean,
 https://github.com/git/git/blob/master/t/t5550-http-fetch.sh doesn't
 call backfill_tags. What do you mean?

That was a typo, I meant
https://github.com/git/git/blob/master/t/t5500-fetch-pack.sh


 I wrote I guess ;-) I did not check what t5550 does.

Any hint on where to start looking? It doesn't look like any test is
using a non-positive depth, so I'm really confused.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] transport: Catch non positive --depth option value

2013-11-26 Thread Duy Nguyen

On Tue, Nov 26, 2013 at 5:43 PM, Andrés G. Aragoneses
kno...@gmail.com wrote:
 On 26/11/13 04:06, Duy Nguyen wrote:
 On Tue, Nov 26, 2013 at 6:34 AM, Andrés G. Aragoneses
 kno...@gmail.com wrote:
 On 22/11/13 02:18, Duy Nguyen wrote:
 On Fri, Nov 22, 2013 at 3:18 AM, Junio C Hamano gits...@pobox.com wrote:
 Have you run the tests with this patch?  It seems that it breaks
 quite a lot of them, including t5500, t5503, t5510, among others.

 I guess it's caused by builtin/fetch.c:backfill_tags(). And the call
 could be replaced with

 transport_set_option(transport, TRANS_OPT_DEPTH, NULL);


 Not sure what you mean,
 https://github.com/git/git/blob/master/t/t5550-http-fetch.sh doesn't
 call backfill_tags. What do you mean?

 That was a typo, I meant
 https://github.com/git/git/blob/master/t/t5500-fetch-pack.sh


 I wrote I guess ;-) I did not check what t5550 does.

 Any hint on where to start looking? It doesn't look like any test is
 using a non-positive depth, so I'm really confused.

Replace die() with *(char*)0 = 0; and run t5500, I got a core dump.
Running gdb shows this

(gdb) bt
#0  0x0053d98b in set_git_option (opts=0xb63d00, name=0x575767
depth, value=0x575c91 0) at transport.c:487
#1  0x0053f163 in transport_set_option (transport=0xb63f00,
name=0x575767 depth, value=0x575c91 0) at transport.c:1000
#2  0x00437b68 in backfill_tags (transport=0xb63f00,
ref_map=0xb64d60) at builtin/fetch.c:773
#3  0x00437f91 in do_fetch (transport=0xb63f00, refs=0xb643c0,
ref_count=1) at builtin/fetch.c:869
#4  0x004386d4 in fetch_one (remote=0xb63c20, argc=1,
argv=0x7fff32f63588) at builtin/fetch.c:1037
#5  0x00438a1d in cmd_fetch (argc=2, argv=0x7fff32f63580,
prefix=0x0) at builtin/fetch.c:1115
#6  0x0040590f in run_builtin (p=0x7d59a8, argc=4,
argv=0x7fff32f63580) at git.c:314
#7  0x00405aa2 in handle_internal_command (argc=4,
argv=0x7fff32f63580) at git.c:478
#8  0x00405bbc in run_argv (argcp=0x7fff32f6346c,
argv=0x7fff32f63470) at git.c:524
#9  0x00405d61 in main (argc=4, av=0x7fff32f63578) at git.c:607

My guess seems right.
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv4] transport: Catch non positive --depth option value

2013-11-26 Thread Andrés G. Aragoneses

From 4f3b24379090b7b69046903fba494f3191577b20 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= kno...@gmail.com
Date: Tue, 26 Nov 2013 12:38:19 +0100
Subject: [PATCH] transport: Catch non positive --depth option value

Instead of simply ignoring the value passed to --depth
option when it is zero or negative, now it is caught
and reported.

This will let people know that they were using the
option incorrectly (as depth0 should be simply invalid,
and under the hood depth==0 didn't have any effect).

(The change in fetch.c is needed to avoid the tests
failing because of this new restriction.)

Signed-off-by: Andres G. Aragoneses kno...@gmail.com
Reviewed-by: Duy Nguyen pclo...@gmail.com
---
 builtin/fetch.c | 2 +-
 transport.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index bd7a101..88c04d7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -770,7 +770,7 @@ static void backfill_tags(struct transport *transport, 
struct ref *ref_map)
}
 
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
-   transport_set_option(transport, TRANS_OPT_DEPTH, 0);
+   transport_set_option(transport, TRANS_OPT_DEPTH, NULL);
fetch_refs(transport, ref_map);
 
if (gsecondary) {
diff --git a/transport.c b/transport.c
index 7202b77..5b42ccb 100644
--- a/transport.c
+++ b/transport.c
@@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options 
*opts,
opts-depth = strtol(value, end, 0);
if (*end)
die(transport: invalid depth option '%s', 
value);
+   if (opts-depth  1)
+   die(transport: invalid depth option '%s' (must 
be positive), value);
}
return 0;
}
-- 
1.8.1.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/9] contrib: remove git-p4import

2013-11-26 Thread Pete Wyckoff

jrnie...@gmail.com wrote on Mon, 25 Nov 2013 12:58 -0800:
 The git p4import documentation has suggested git p4 as a better
 alternative for more than 6 years.  (According to the mailing list
 discussion when it was moved to contrib/, git-p4import has serious
 bugs --- e.g., its incremental mode just doesn't work.) Since then,
 git p4 has been actively developed and was promoted to a standard git
 command alongside git svn.
 
 Searches on google.com/trends and stackoverflow suggest that no one is
 looking for git-p4import any more.  Remove it.
 
 Noticed while considering marking the contrib/p4import/git-p4import.py
 script executable as part of a wider sweep.

I haven't seen git-p4import mentioned for the last 6 years either.
Thanks,

Acked-by: Pete Wyckoff p...@padd.com 

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 04/28] update-server-info: do not publish shallow clones

2013-11-26 Thread Duy Nguyen

On Tue, Nov 26, 2013 at 3:08 AM, Junio C Hamano gits...@pobox.com wrote:
 Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 Dumb commit walker does not care about .git/shallow and until someone
 steps up to make it happen, let's not publish shallow clones using
 dumb protocols.

 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
 ---
  server-info.c | 9 +
  1 file changed, 9 insertions(+)

 I doubt that pros-and-cons is in this patch's favor.  Without this
 patch, if a fetch requests commits just on the surface in this
 shallow repository, the walker would happily get the necessary
 objects just fine.  If the request has to dig deeper and cross the
 shallow boundary, the walker will get a failure and error out.

 With this patch, both will error out.  So overall, the patch did not
 make anything safer (e.g. preventing from introducing new corruption
 on the recipient's end) while breaking a case where it worked just
 fine, no?

 Or am I missing something?  Not that dumb walkers would matter very
 much these days, ...

No you're not. If it may fail, in my opinion it should fail early than
walk all the way and fail. But yeah dropping the patch is fine too. I
don't care too much about dumb walkers.
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 07/28] shallow.c: add remove_reachable_shallow_points()

2013-11-26 Thread Duy Nguyen

On Tue, Nov 26, 2013 at 4:53 AM, Junio C Hamano gits...@pobox.com wrote:
 Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 When we receive a pack and the shallow points from another repository,
 we may want to add more shallow points to current repo to make sure no
 commits point to nowhere. However we do not want to add unnecessary
 shallow points and cut our history short because the other end is a
 shallow version of this repo. The output shallow points may or may not
 be added to .git/shallow, depending on whether they are actually
 reachable in the new pack.

 This function filters such shallow points out, leaving ones that might
 potentially be added. A simple has_sha1_file won't do because we may
 have incomplete object islands (i.e. not connected to any refs) and
 the shallow points are on these islands. In that case we want to keep
 the shallow points as candidates until we figure out if the new pack
 connects to such object islands.

 Typical cases that use remove_reachable_shallow_points() are:

  - fetch from a repo that has longer history: in this case all remote
shallow roots do not exist in the client repo, this function will
be cheap as it just does a few lookup_commit_graft and
has_sha1_file.

 It is unclear why.  If you fetched from a repository more complete
 than you, you may deepen your history which may allow you to unplug
 the shallow points you originally had, and remote shallow root (by
 the way, lets find a single good phrase to represent this thing and
 stick to it) may want to replace them, no?

Except that deepen/shorten history is a different mode that this
function is not used at all. I should have made that clear. This and
the next patch are about stick to our base and add something on top

Any suggestions about a good phase? I've been swinging between
shallow points (from 4 months ago) and shallow roots (recently).

  - fetch from a repo that has exactly the same shallow root set
(e.g. a clone from a shallow repo): this case may trigger
in_merge_bases_many all the way to roots. An exception is made to
avoid such costly path with a faith that .git/shallow does not
usually points to unreachable commit islands.

 ... and when the faith is broken, you will end up with a broken
 repository???

Not really broken because the new ref will be cut at the troublesome
shallow root before it goes out of bound, so the user may be surprised
that he got a history shorter than he wanted. It's when the root is
removed that we have a problem. But commits in .git/shallow are only
removed by

1) deepening history
2) the prune patch 28/28

#1 should send the missing objects and insert a new commit to
.git/shallow to plug the hole, so we're good. #2 only removes commits
from .git/shallow if they are not reachable from any refs, which is no
longer true.

 +static int add_ref(const char *refname,
 +const unsigned char *sha1, int flags, void *cb_data)
 +{
 + struct commit_array *ca = cb_data;
 + ALLOC_GROW(ca-commits, ca-nr + 1, ca-alloc);
 + ca-commits[ca-nr++] = lookup_commit(sha1);
 + return 0;
 +}

 Can't a ref point at a non-commit-ish?  Is the code prepared to deal
 with such an entry (possibly a NULL pointer) in the commit_array
 struct?

Eck, yes a ref can. No the code is not :P Thanks for pointing this
out. We don't care about non-commit refs, so we just need to filter
them out.
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 08/28] shallow.c: add mark_new_shallow_refs()

2013-11-26 Thread Duy Nguyen

On Tue, Nov 26, 2013 at 5:20 AM, Junio C Hamano gits...@pobox.com wrote:
 Hmph.  the use of -util field in this patch feels that it was
 something commit-slab data structure was invented to solve.

Good stuff! Thanks.

 + if (c-util == NULL)
 + c-util = bitmap;
 + else {
 + /*
 +  * Deliberately leak a lot in commit-util
 +  * because there can be many pointers to the
 +  * same bitmap. Probably should allocate in a
 +  * pool and free the whole pool at the end.
 +  */

 ... or perhaps make the bitmap into

 struct {
 int refcnt;
 uint32_t bits[FLEX_ARRAY];
 }

 and refcnt them?

I still prefer memory pools so I just need to do a few free() than
walking through all the commits again and refcnt-- or free() them.

 + /*
 +  * Quick check to see if we may need to add new shallow
 +  * roots. Go through the list of root candidates and check if
 +  * they exist (either in current repo, or in the new pack, we
 +  * can't distinguish).
 +  *
 +  * 1) If none of the new roots exist, the pack must connect to
 +  *the main object graph, which is already guarded by
 +  *current repo's shallow roots and we will not need to
 +  *consider adding new shallow roots, so we can exit early.
 +  *
 +  * 2) The pack may connect to some existing object islands in
 +  *current repo then add shallow roots to plug loose ends
 +  *from those islands. In that case, new shallow roots must
 +  *also exist in the repo as this stage (old objects plus
 +  *the new pack).
 +  *
 +  * 3) The last, easiest case, is the pack contains some
 +  *shallow roots, which may be used to tie up loose ends of
 +  *some new refs, or redundanty (tying up loose ends of new
 +  *object islands)
 +  */
 + for (i = 0;i  shallow-nr; i++)
 + if (has_sha1_file(shallow-array[i]))
 + break;
 + if (i == shallow-nr)
 + /*
 +  * this is the first and also the common case, where
 +  * the new pack does not carry any new shallow
 +  * points. No need to to the expensive commit traverse
 +  * dance below.
 +  */
 + return 0;

 I am Confused.

 The loop only made sure that all the elements in the array[] is
 still missing (or, ... is this function supposed to be called before
 installing a new pack???  It is unclear.  But if new objects were
 unpacked while receiving, then there is no not install the new
 objects and check possible, so I'd assume this is called after
 receiving and registering a new pack to the object store).

Yes it's called after installing the new packs (or after unpacking).
I'll mention about this.

 But then, can it be that you had N-1 shallow points originally,
 the pack has a reference to a new missing commit, and the array has
 N shallow points in total?  Or is the caller expected to call this
 function with shallow pointing at a pre-transfer shallow points?

Another thing I did not mention is all shallow commits we have are
already filtered out by remove_reachable_shallow_points. By the time
you get here, the array only contains the shallow commits we don't
have that may or may not be referenced by a something (oh yeah I did
not handle tags!) in the new pack. So if all of them are missing (i.e.
the new pack does not reference to any of them), they're useless and
can be thrown away.

Sorry to break the patches this way and lose the overall call flow.
It's just too big to put all into one patch. 13/28 is the one that put
the pieces together but basically

 1. receive the remote's .git/shallow
 2. call remote_reachable_shallow_points() to exclude our shallow commits
 3. get the pack and install it (or unpack it)
 4. call this function to determine what new ref needs new shallow
commits from the result of #2

 + /*
 +  * Prepare the commit graph to track what refs can reach what
 +  * (new) shallow points.
 +  */
 + nr = get_max_object_index();

 Hmph. At this point (again, there is no description on where in the
 overall sequence of events this function is designed to be called,
 so it is impossible to review the logic), is it expected that we
 have seen all the objects under the sun and marked them in a
 specific way?

No. At this point we just make sure to have clean flags in commit
objects. paint_down() is the one that walks through and marks them as
seen.
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] submodule recursion in git-archive

2013-11-26 Thread René Scharfe

Am 26.11.2013 01:04, schrieb Nick Townsend:
 My first git patch - so shout out if I’ve got the etiquette wrong! Or
 of course if I’ve missed something.

Thanks for the patches!  Please send only one per message (the second
one as a reply to the first one, or both as replies to a cover letter),
though -- that makes commenting on them much easier.

Side note: Documentation/SubmittingPatches doesn't mention that (yet),
AFAICS.

 Subject: [PATCH 1/2] submodule: add_submodule_odb() usability
 
 Although add_submodule_odb() is documented as being
 externally usable, it is declared static and also
 has incorrect documentation.
 
 This commit fixes those and makes no changes to
 existing code using them. All tests still pass.

Sign-off missing (see Documentation/SubmittingPatches).

 ---
  Documentation/technical/api-ref-iteration.txt | 4 ++--
  submodule.c   | 2 +-
  submodule.h   | 1 +
  3 files changed, 4 insertions(+), 3 deletions(-)
 
 diff --git a/Documentation/technical/api-ref-iteration.txt 
 b/Documentation/technical/api-ref-iteration.txt
 index aa1c50f..cbee624 100644
 --- a/Documentation/technical/api-ref-iteration.txt
 +++ b/Documentation/technical/api-ref-iteration.txt
 @@ -50,10 +50,10 @@ submodules object database. You can do this by a 
 code-snippet like
  this:
  
   const char *path = path/to/submodule
 - if (!add_submodule_odb(path))
 + if (add_submodule_odb(path))
   die(Error submodule '%s' not populated., path);
  
 -`add_submodule_odb()` will return an non-zero value on success. If you
 +`add_submodule_odb()` will return a zero value on success. If you

return zero on success instead?

  do not do this you will get an error for each ref that it does not point
  to a valid object.
  
 diff --git a/submodule.c b/submodule.c
 index 1905d75..1ea46be 100644
 --- a/submodule.c
 +++ b/submodule.c
 @@ -143,7 +143,7 @@ void stage_updated_gitmodules(void)
   die(_(staging updated .gitmodules failed));
  }
  
 -static int add_submodule_odb(const char *path)
 +int add_submodule_odb(const char *path)
  {
   struct strbuf objects_directory = STRBUF_INIT;
   struct alternate_object_database *alt_odb;
 diff --git a/submodule.h b/submodule.h
 index 7beec48..3e3cdca 100644
 --- a/submodule.h
 +++ b/submodule.h
 @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], 
 const char *remotes_nam
   struct string_list *needs_pushing);
  int push_unpushed_submodules(unsigned char new_sha1[20], const char 
 *remotes_name);
  void connect_work_tree_and_git_dir(const char *work_tree, const char 
 *git_dir);
 +int add_submodule_odb(const char *path);
  
  #endif

 Subject: [PATCH 2/2] archive: allow submodule recursion on git-archive
 
 When using git-archive to produce a dump of a
 repository, the existing code does not recurse
 into a submodule when it encounters it in the tree
 traversal. These changes add a command line flag
 that permits this.
 
 Note that the submodules must be updated in the
 repository, otherwise this cannot take place.
 
 The feature is disabled for remote repositories as
 the git_work_tree fails. This is a possible future
 enhancement.

Hmm, curious.  Why does it fail?  I guess that happens with bare
repositories, only, right?  (Which are the most likely kind of remote
repos to encounter, of course.)

 Two additional fields are added to archiver_args:
   * recurse  - a boolean indicator
   * treepath - the path part of the tree-ish
eg. the 'www' in HEAD:www
 
 The latter is used within the archive writer to
 determin the correct path for the submodule .git
 file.
 
 Signed-off-by: Nick Townsend nick.towns...@mac.com
 ---
  Documentation/git-archive.txt |  9 +
  archive.c | 38 --
  archive.h |  2 ++
  3 files changed, 47 insertions(+), 2 deletions(-)
 
 diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
 index b97aaab..b4df735 100644
 --- a/Documentation/git-archive.txt
 +++ b/Documentation/git-archive.txt
 @@ -11,6 +11,7 @@ SYNOPSIS
  [verse]
  'git archive' [--format=fmt] [--list] [--prefix=prefix/] [extra]
 [-o file | --output=file] [--worktree-attributes]
 +   [--recursive|--recurse-submodules]

I'd expect git archive --recurse to add subdirectories and their
contents, which it does right now, and --no-recurse to only archive the
specified objects, which is not implemented.  IAW: I wouldn't normally
associate an option with that name with submodules.  Would
--recurse-submodules alone suffice?

Side note: With only one of the options defined you could shorten them
on the command line to e.g. --rec; with both you'd need to type at least
--recursi or --recurse to disambiguate -- even though they ultimately do
the same.

 [--remote=repo [--exec=git-upload-archive]] tree-ish
 [path...]
  
 @@

[PATCH] Prevent buffer overflows when path is too long

2013-11-26 Thread Antoine Pelisse

Some buffers created with PATH_MAX length are not checked when being
written, and can overflow if PATH_MAX is not big enough to hold the
path.

Some of the use-case are probably impossible to reach, and the program
dies if the path looks too long. When it would be possible for the user
to use a longer path, simply use strbuf to build it.

Reported-by: Wataru Noguchi wnoguchi.0...@gmail.com
Signed-off-by: Antoine Pelisse apeli...@gmail.com
---
 abspath.c| 10 --
 diffcore-order.c | 14 +-
 unpack-trees.c   |  2 ++
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/abspath.c b/abspath.c
index e390994..29a5f9d 100644
--- a/abspath.c
+++ b/abspath.c
@@ -216,11 +216,16 @@ const char *absolute_path(const char *path)
 const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
 {
static char path[PATH_MAX];
+
+   if (pfx_len = PATH_MAX)
+   die(Too long prefix path: %s, pfx);
+
 #ifndef GIT_WINDOWS_NATIVE
if (!pfx_len || is_absolute_path(arg))
return arg;
memcpy(path, pfx, pfx_len);
-   strcpy(path + pfx_len, arg);
+   if (strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len)  PATH_MAX)
+   die(Too long path: %s, path);
 #else
char *p;
/* don't add prefix to absolute paths, but still replace '\' by '/' */
@@ -228,7 +233,8 @@ const char *prefix_filename(const char *pfx, int pfx_len, 
const char *arg)
pfx_len = 0;
else if (pfx_len)
memcpy(path, pfx, pfx_len);
-   strcpy(path + pfx_len, arg);
+   if (strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len)  PATH_MAX)
+   die(Too long path: %s, path);
for (p = path + pfx_len; *p; p++)
if (*p == '\\')
*p = '/';
diff --git a/diffcore-order.c b/diffcore-order.c
index 23e9385..87193f8 100644
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -73,20 +73,24 @@ struct pair_order {
 static int match_order(const char *path)
 {
int i;
-   char p[PATH_MAX];
+   struct strbuf p = STRBUF_INIT;
 
for (i = 0; i  order_cnt; i++) {
-   strcpy(p, path);
-   while (p[0]) {
+   strbuf_reset(p);
+   strbuf_addstr(p, path);
+   while (p.buf[0]) {
char *cp;
-   if (!fnmatch(order[i], p, 0))
+   if (!fnmatch(order[i], p.buf, 0)) {
+   strbuf_release(p);
return i;
-   cp = strrchr(p, '/');
+   }
+   cp = strrchr(p.buf, '/');
if (!cp)
break;
*cp = 0;
}
}
+   strbuf_release(p);
return order_cnt;
 }
 
diff --git a/unpack-trees.c b/unpack-trees.c
index 35cb05e..f93565b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -918,6 +918,8 @@ static int clear_ce_flags_1(struct cache_entry **cache, int 
nr,
int processed;
 
len = slash - name;
+   if (len + prefix_len = PATH_MAX)
+   die(Too long path: %s, prefix);
memcpy(prefix + prefix_len, name, len);
 
/*
-- 
1.8.5.rc3.1.ga0b6b91

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git issues with submodules

2013-11-26 Thread Jens Lehmann

Am 25.11.2013 22:01, schrieb Junio C Hamano:
 Jens Lehmann jens.lehm...@web.de writes:
 
 Looking good to me. Please add tests for diff.ignoreSubmodules
 and submodule.name.ignore, the latter both in .gitmodules and
 .git/config. While doing some testing for this thread I found an
 inconsistency in git show which currently honors the submodule
 specific option only from .git/config and ignores it in the
 .gitmodules file ...
 
 Sorry, but isn't that what should happen?  .git/config is the
 ultimate source of the truth, and .gitmodules is a hint to prime
 that when the user does git submodule init, no?

git submodule init only copies the update and url settings
to .git/config, all others default to the value they have in the
.gitmodules file if they aren't found in .git/config. This allows
upstream to change these settings unless the user copies them to
.git/config himself.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] submodule recursion in git-archive

2013-11-26 Thread Jens Lehmann

Am 26.11.2013 16:17, schrieb René Scharfe:
 Am 26.11.2013 01:04, schrieb Nick Townsend:
 diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
 index b97aaab..b4df735 100644
 --- a/Documentation/git-archive.txt
 +++ b/Documentation/git-archive.txt
 @@ -11,6 +11,7 @@ SYNOPSIS
  [verse]
  'git archive' [--format=fmt] [--list] [--prefix=prefix/] [extra]
[-o file | --output=file] [--worktree-attributes]
 +  [--recursive|--recurse-submodules]
 
 I'd expect git archive --recurse to add subdirectories and their
 contents, which it does right now, and --no-recurse to only archive the
 specified objects, which is not implemented.  IAW: I wouldn't normally
 associate an option with that name with submodules.  Would
 --recurse-submodules alone suffice?

It should. All new code recursing into submodules should not use
--recursive but always --recurse-submodules, as --recursive means
different things for different commands (the only exception being
git submodule, as --recursive is obvious here, and git clone
for backward compatibility reasons).

But I really like what these patches are aiming at.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4] transport: Catch non positive --depth option value

2013-11-26 Thread Jonathan Nieder

Hi,

Thanks for tackling this.  This review will be kind of nitpicky, as a
way to save time when reviewing future patches.

Andrés G. Aragoneses wrote:

 From 4f3b24379090b7b69046903fba494f3191577b20 Mon Sep 17 00:00:00 2001
 From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= kno...@gmail.com
 Date: Tue, 26 Nov 2013 12:38:19 +0100
 Subject: [PATCH] transport: Catch non positive --depth option value

These lines are redundant next to the mail header, so they can and
should be omitted to avoid some noise.

 Instead of simply ignoring the value passed to --depth
 option when it is zero or negative, now it is caught
 and reported.

Nit: commit messages usually give a command to the codebase, like
this:

When the value passed to --depth is zero or negative, instead of
treating it as infinite depth, catch and report the mistake.

 This will let people know that they were using the
 option incorrectly (as depth0 should be simply invalid,
 and under the hood depth==0 didn't have any effect).

Ok.  Do we know that no one was using --depth=0 this way deliberately?

 (The change in fetch.c is needed to avoid the tests
 failing because of this new restriction.)

Based on the surrounding thread I see that you're talking about the
test script t5500 here.  Which test failed?  How does it use git
fetch?  Does the change just fix the test but keep in broken in
production, or does it fix git fetch in production, too?

 Signed-off-by: Andres G. Aragoneses kno...@gmail.com
 Reviewed-by: Duy Nguyen pclo...@gmail.com
 ---
  builtin/fetch.c | 2 +-
  transport.c | 2 ++
  2 files changed, 3 insertions(+), 1 deletion(-)

It would be nice to have a brief test to demonstrate the fix and make
sure we don't break it in the future.  grep fetch.*--depth t/*.sh
tells me t5500 would be a good place to put it.  For example,
something like

test_expect_success 'fetch catches invalid --depth values' '
(
cd shallow 
test_must_fail git fetch --depth=0 
test_must_fail git fetch --depth=-2 
test_must_fail git fetch --depth= 
test_must_fail git fetch --depth=nonsense
)
'

What do you think?
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git issues with submodules

2013-11-26 Thread Junio C Hamano

Jens Lehmann jens.lehm...@web.de writes:

 Am 25.11.2013 22:01, schrieb Junio C Hamano:
 Jens Lehmann jens.lehm...@web.de writes:
 
 Looking good to me. Please add tests for diff.ignoreSubmodules
 and submodule.name.ignore, the latter both in .gitmodules and
 .git/config. While doing some testing for this thread I found an
 inconsistency in git show which currently honors the submodule
 specific option only from .git/config and ignores it in the
 .gitmodules file ...
 
 Sorry, but isn't that what should happen?  .git/config is the
 ultimate source of the truth, and .gitmodules is a hint to prime
 that when the user does git submodule init, no?

 git submodule init only copies the update and url settings
 to .git/config, all others default to the value they have in the
 .gitmodules file if they aren't found in .git/config. This allows
 upstream to change these settings unless the user copies them to
 .git/config himself.

I know what the code does. I was questioning if only copies X and
Y is a sensible thing.

Copying at init time will fix the values when copied and give the
user a stable and dependable behaviour.  I have a feeling that the
current not copy to fix it to a stable value, but look into
.gitmodules as a fallback was not a designed behaviour for the
other properties, but was done by accident and/or laziness.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Prevent buffer overflows when path is too long

2013-11-26 Thread Junio C Hamano

Antoine Pelisse apeli...@gmail.com writes:

 Some buffers created with PATH_MAX length are not checked when being
 written, and can overflow if PATH_MAX is not big enough to hold the
 path.

Perhaps it is time to update all of them to use strbuf?  The callers
of prefix_filename() aren't that many, and all of them are prepared
to stash the returned value away when they keep it longer term, so
they would not notice if we used static struct strbuf path and
gave back path.buf (without strbuf_detach() on it).  The buffer
used in clear_ce_flags() and passed to clear_ce_flags_{1,dir} are
not seen outside the callchain, and can safely become strbuf, I
think.

  abspath.c| 10 --
  diffcore-order.c | 14 +-
  unpack-trees.c   |  2 ++
  3 files changed, 19 insertions(+), 7 deletions(-)

 diff --git a/abspath.c b/abspath.c
 index e390994..29a5f9d 100644
 --- a/abspath.c
 +++ b/abspath.c
 @@ -216,11 +216,16 @@ const char *absolute_path(const char *path)
  const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
  {
   static char path[PATH_MAX];
 +
 + if (pfx_len = PATH_MAX)
 + die(Too long prefix path: %s, pfx);

I do not think this is needed, and will reject a valid input that
used to be accepted (e.g. arg is absolute so pfx does not matter).

  #ifndef GIT_WINDOWS_NATIVE
   if (!pfx_len || is_absolute_path(arg))
   return arg;
   memcpy(path, pfx, pfx_len);
 - strcpy(path + pfx_len, arg);
 + if (strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len)  PATH_MAX)
 + die(Too long path: %s, path);

Rather, have that too long a prefix? check before that memcpy().

  #else
   char *p;
   /* don't add prefix to absolute paths, but still replace '\' by '/' */
 @@ -228,7 +233,8 @@ const char *prefix_filename(const char *pfx, int pfx_len, 
 const char *arg)
   pfx_len = 0;
   else if (pfx_len)
   memcpy(path, pfx, pfx_len);

... and around here.

 - strcpy(path + pfx_len, arg);
 + if (strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len)  PATH_MAX)
 + die(Too long path: %s, path);
   for (p = path + pfx_len; *p; p++)
   if (*p == '\\')
   *p = '/';

The above is curious. Unless we are doing the short-cut for no
prefix so we can just return arg codepath, we know that the
resulting length is always pfx_len + strlen(arg), no?

 diff --git a/diffcore-order.c b/diffcore-order.c
 index 23e9385..87193f8 100644
 --- a/diffcore-order.c
 +++ b/diffcore-order.c
 @@ -73,20 +73,24 @@ struct pair_order {
  static int match_order(const char *path)
  {
   int i;
 - char p[PATH_MAX];
 + struct strbuf p = STRBUF_INIT;
  
   for (i = 0; i  order_cnt; i++) {
 - strcpy(p, path);
 - while (p[0]) {
 + strbuf_reset(p);
 + strbuf_addstr(p, path);
 + while (p.buf[0]) {
   char *cp;
 - if (!fnmatch(order[i], p, 0))
 + if (!fnmatch(order[i], p.buf, 0)) {
 + strbuf_release(p);
   return i;
 - cp = strrchr(p, '/');
 + }
 + cp = strrchr(p.buf, '/');
   if (!cp)
   break;
   *cp = 0;
   }
   }
 + strbuf_release(p);
   return order_cnt;
  }
  
 diff --git a/unpack-trees.c b/unpack-trees.c
 index 35cb05e..f93565b 100644
 --- a/unpack-trees.c
 +++ b/unpack-trees.c
 @@ -918,6 +918,8 @@ static int clear_ce_flags_1(struct cache_entry **cache, 
 int nr,
   int processed;
  
   len = slash - name;
 + if (len + prefix_len = PATH_MAX)
 + die(Too long path: %s, prefix);
   memcpy(prefix + prefix_len, name, len);
  
   /*
 -- 
 1.8.5.rc3.1.ga0b6b91

 -- 
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git issues with submodules

2013-11-26 Thread Jonathan Nieder

Junio C Hamano wrote:

  I have a feeling that the
 current not copy to fix it to a stable value, but look into
 .gitmodules as a fallback was not a designed behaviour for the
 other properties, but was done by accident and/or laziness.

It was designed.  See for example the thread surrounding [1]:

| And when you are on a superproject branch actively developing inside a
| submodule, you may want to increase fetch-activity to fetch all new
| commits in the submodule even if they aren't referenced in the
| superproject (yet), as that might be just what your fellow developers
| are about to do. And the person setting up that branch could do that
| once for all users so they don't have to repeat it in every clone. And
| when switching away from that branch all those developers cannot forget
| to reconfigure to fetch-on-demand, so not having that in .git/config is
| a plus here too.

Thanks,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/161193/focus=161357
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 17/28] Support pushing from a shallow clone

2013-11-26 Thread Eric Sunshine

On Sun, Nov 24, 2013 at 10:55 PM, Nguyễn Thái Ngọc Duy
pclo...@gmail.com wrote:
 Pushing from a shallow clone using today's send-pack and receive-pack
 may work, if the transferred pack does not end up at any graft
 points. If it does, recent receive-pack that does connectivity check
 will reject the push. If receive-pack is old and does not have the
 connectivity check, the upstream repo becomes corrupt.

 The pack protocol is updated and send-pack now sends all shallow
 grafts before it sends the commands, if the repo is shallow. This
 protocol extension will break current receive-pack, which is intended,
 mostly to stop corrupting the upstream repo.

 Changes on the receiver are similar to what has been done in
 fetch-pack, i.e. filter out refs that require new shallow roots then
 go along as usual.

 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
 ---
 diff --git a/Documentation/technical/pack-protocol.txt 
 b/Documentation/technical/pack-protocol.txt
 index eb8edd1..c73b62f 100644
 --- a/Documentation/technical/pack-protocol.txt
 +++ b/Documentation/technical/pack-protocol.txt
 @@ -43,6 +43,9 @@ static int fix_thin = 1;
  static const char *head_name;
  static void *head_name_to_free;
  static int sent_capabilities;
 +static int shallow_push;
 +static const char* alternate_shallow_file;

s/char\* /char */

 +static struct extra_have_objects shallow;

  static enum deny_action parse_deny_action(const char *var, const char *value)
  {
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 8/9 v2] test: replace shebangs with descriptions in shell libraries

2013-11-26 Thread Jonathan Nieder

A #! line in these files is misleading, since these scriptlets are
meant to be sourced with '.' (using whatever shell sources them)
instead of run directly using the interpreter named on the #! line.

Removing the #! line shouldn't hurt syntax highlighting since
these files have filenames ending with '.sh'.  For documentation,
add a brief description of how the files are meant to be used in
place of the shebang line.

Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Eric Sunshine wrote:
 On Mon, Nov 25, 2013 at 4:03 PM, Jonathan Nieder jrnie...@gmail.com wrote:

 +++ b/t/lib-bash.sh
 @@ -1,7 +1,6 @@
 -#!/bin/sh
 -#
 -# Ensures that tests are run under Bash; primarily intended for running 
 tests
 -# of the completion script.
 +# Shell library sourced instead of ./test-lib.sh by tests that need
 +# to run under Bash; primary intended for tests of the completion

 s/primary/primarily/

Good eyes.  Here's an updated version of the patch with that change.

Thanks,
Jonathan

 t/gitweb-lib.sh   |  3 ++-
 t/lib-bash.sh |  7 +++
 t/lib-cvs.sh  |  2 +-
 t/lib-diff-alternative.sh |  3 ++-
 t/lib-gettext.sh  |  3 ++-
 t/lib-git-daemon.sh   | 18 +-
 t/lib-httpd.sh| 29 -
 t/lib-pack.sh |  2 --
 t/lib-pager.sh|  2 +-
 t/lib-read-tree.sh|  2 --
 t/lib-rebase.sh   |  2 +-
 t/lib-terminal.sh |  2 +-
 t/perf/perf-lib.sh|  4 +++-
 t/test-lib-functions.sh   |  3 ++-
 t/test-lib.sh |  2 +-
 15 files changed, 64 insertions(+), 20 deletions(-)

diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh
index 8cf909a..d5dab5a 100644
--- a/t/gitweb-lib.sh
+++ b/t/gitweb-lib.sh
@@ -1,4 +1,5 @@
-#!/bin/sh
+# Initialization and helpers for Gitweb tests, which source this
+# shell library instead of test-lib.sh.
 #
 # Copyright (c) 2007 Jakub Narebski
 #
diff --git a/t/lib-bash.sh b/t/lib-bash.sh
index 11397f7..2be955f 100644
--- a/t/lib-bash.sh
+++ b/t/lib-bash.sh
@@ -1,7 +1,6 @@
-#!/bin/sh
-#
-# Ensures that tests are run under Bash; primarily intended for running tests
-# of the completion script.
+# Shell library sourced instead of ./test-lib.sh by tests that need
+# to run under Bash; primarily intended for tests of the completion
+# script.
 
 if test -n $BASH  test -z $POSIXLY_CORRECT; then
# we are in full-on bash mode
diff --git a/t/lib-cvs.sh b/t/lib-cvs.sh
index 44263ad..5076718 100644
--- a/t/lib-cvs.sh
+++ b/t/lib-cvs.sh
@@ -1,4 +1,4 @@
-#!/bin/sh
+# Shell library sourced instead of ./test-lib.sh by cvsimport tests.
 
 . ./test-lib.sh
 
diff --git a/t/lib-diff-alternative.sh b/t/lib-diff-alternative.sh
index 75ffd91..8b4dbf2 100644
--- a/t/lib-diff-alternative.sh
+++ b/t/lib-diff-alternative.sh
@@ -1,4 +1,5 @@
-#!/bin/sh
+# Helpers shared by the test scripts for diff algorithms (patience,
+# histogram, etc).
 
 test_diff_frobnitz() {
cat file1 \EOF
diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh
index ae8883a..eec757f 100644
--- a/t/lib-gettext.sh
+++ b/t/lib-gettext.sh
@@ -1,4 +1,5 @@
-#!/bin/sh
+# Initialization and Icelandic locale for basic git i18n tests,
+# which source this scriptlet instead of ./test-lib.sh.
 #
 # Copyright (c) 2010 Ævar Arnfjörð Bjarmason
 #
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 87f0ad8..394b06b 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -1,4 +1,20 @@
-#!/bin/sh
+# Shell library to run git-daemon in tests.  Ends the test early if
+# GIT_TEST_GIT_DAEMON is not set.
+#
+# Usage:
+#
+#  . ./test-lib.sh
+#  . $TEST_DIRECTORY/lib-git-daemon.sh
+#  start_git_daemon
+#
+#  test_expect_success '...' '
+#  ...
+#  '
+#
+#  test_expect_success ...
+#
+#  stop_git_daemon
+#  test_done
 
 if test -z $GIT_TEST_GIT_DAEMON
 then
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index ad8f1ef..c470784 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -1,4 +1,31 @@
-#!/bin/sh
+# Shell library to run an HTTP server for use in tests.
+# Ends the test early if httpd tests should not be run,
+# for example because the user has not enabled them.
+#
+# Usage:
+#
+#  . ./test-lib.sh
+#  . $TEST_DIRECTORY/lib-httpd.sh
+#  start_httpd
+#
+#  test_expect_success '...' '
+#  ...
+#  '
+#
+#  test_expect_success ...
+#
+#  stop_httpd
+#  test_done
+#
+# Can be configured using the following variables.
+#
+#GIT_TEST_HTTPD  enable HTTPD tests
+#LIB_HTTPD_PATH  web server path
+#LIB_HTTPD_MODULE_PATH   web server modules path
+#LIB_HTTPD_PORT  listening port
+#LIB_HTTPD_DAV   enable DAV
+#LIB_HTTPD_SVN   enable SVN
+#LIB_HTTPD_SSL   enable SSL
 #
 # Copyright (c) 2008 Clemens Buchacher dri...@aon.at
 #
diff --git a/t/lib-pack.sh b/t/lib-pack.sh
index b96e125..7509846 100644
--- a/t/lib-pack.sh
+++ b/t/lib-pack.sh
@@ -1,5

Re: [PATCH] gitweb: Add an option for adding more branch refs

2013-11-26 Thread Junio C Hamano

Krzesimir Nowak krzesi...@endocode.com writes:

 Overriding an @additional_branch_refs configuration variable with
 value ('wip') will make gitweb to show branches that appear in
 refs/heads and refs/wip (refs/heads is hardcoded). Might be useful for
 gerrit setups where user branches are not stored under refs/heads/.

 Signed-off-by: Krzesimir Nowak krzesi...@endocode.com
 ---
  gitweb/gitweb.perl | 99 
 --
  1 file changed, 74 insertions(+), 25 deletions(-)

 diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
 index 68c77f6..9bfd38b 100755
 --- a/gitweb/gitweb.perl
 +++ b/gitweb/gitweb.perl
 @@ -17,6 +17,7 @@ use Encode;
  use Fcntl ':mode';
  use File::Find qw();
  use File::Basename qw(basename);
 +use List::Util qw(min);
  use Time::HiRes qw(gettimeofday tv_interval);
  binmode STDOUT, ':utf8';
  
 @@ -122,6 +123,10 @@ our $logo_label = git homepage;
  # source of projects list
  our $projects_list = ++GITWEB_LIST++;
  
 +# list of additional directories under refs/ we want to display as
 +# branches
 +our @additional_branch_refs = ();
 +
  # the width (in characters) of the projects list Description column
  our $projects_list_description_width = 25;
  
 @@ -626,14 +631,29 @@ sub feature_avatar {
   return @val ? @val : @_;
  }
  
 +sub get_branch_refs {
 +return ('heads', @additional_branch_refs);
 +}
 +
  # 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.
  sub check_head_link {
   my ($dir) = @_;
   my $headfile = $dir/HEAD;
 - return ((-e $headfile) ||
 - (-l $headfile  readlink($headfile) =~ /^refs\/heads\//));
 +
 + if (-e $headfile) {
 + return 1;
 + }
 + if (-l $headfile) {
 + my $rl = readlink($headfile);
 +
 + for my $ref (get_branch_refs()) {
 + return 1 if $rl =~ /^refs\/$ref\//;
 + }
 + }
 + return 0;

The change to this function is wrong. A non-detached HEAD that
points at anything other than refs/heads/ should be considered a
repository corruption and should not be encouraged.

 @@ -2515,6 +2535,7 @@ sub format_snapshot_links {
  sub get_feed_info {
   my $format = shift || 'Atom';
   my %res = (action = lc($format));
 + my $matched_ref = 0;
  
   # feed links are possible only for project views
   return unless (defined $project);
 @@ -2522,12 +2543,17 @@ sub get_feed_info {
   # or don't have specific feed yet (so they should use generic)
   return if (!$action || $action =~ /^(?:tags|heads|forks|tag|search)$/x);
  
 - my $branch;
 - # branches refs uses 'refs/heads/' prefix (fullname) to differentiate
 - # from tag links; this also makes possible to detect branch links
 - if ((defined $hash_base  $hash_base =~ m!^refs/heads/(.*)$!) ||
 - (defined $hash   $hash  =~ m!^refs/heads/(.*)$!)) {
 - $branch = $1;
 + my $branch = undef;
 + # branches refs uses 'refs/' + $get_branch_refs()[x] + '/' prefix
 + # (fullname) to differentiate from tag links; this also makes
 + # possible to detect branch links
 + for my $ref (get_branch_refs()) {
 + if ((defined $hash_base  $hash_base =~ m!^refs/$ref/(.*)$!) ||
 + (defined $hash   $hash  =~ m!^refs/$ref/(.*)$!)) {
 + $branch = $1;
 + $matched_ref = $ref;
 + last;
 + }
   }
   # find log type for feed description (title)
   my $type = 'log';
 @@ -2540,7 +2566,7 @@ sub get_feed_info {
   }
  
   $res{-title} = $type;
 - $res{'hash'} = (defined $branch ? refs/heads/$branch : undef);
 + $res{'hash'} = (defined $branch ? refs/$matched_ref/$branch : undef);
   $res{'file_name'} = $file_name;

OK.

 @@ -3184,24 +3210,43 @@ sub git_get_project_owner {
   return $owner;
  }
  
 -sub git_get_last_activity {
 - my ($path) = @_;
 - my $fd;
 +sub git_get_last_activity_age {
 + my ($refs) = @_;
 + my $fd = -1;
  
 - $git_dir = $projectroot/$path;
   open($fd, -|, git_cmd(), 'for-each-ref',
'--format=%(committer)',
'--sort=-committerdate',
'--count=1',
 -  'refs/heads') or return;
 +  $refs) or return undef;
 +
   my $most_recent = $fd;
 - close $fd or return;
 + close $fd or return undef;
   if (defined $most_recent 
   $most_recent =~ / (\d+) [-+][01]\d\d\d$/) {
   my $timestamp = $1;
 - my $age = time - $timestamp;
 - return ($age, age_string($age));
 + return time - $timestamp;
   }
 +
 + return undef;
 +}
 +
 +sub git_get_last_activity {
 + my ($path) = @_;
 + my @ages = ();
 +
 + $git_dir = $projectroot/$path;
 + for my $ref (get_branch_refs()) {
 + my $age =

Re: [PATCH] submodule recursion in git-archive

2013-11-26 Thread Junio C Hamano

René Scharfe l@web.de writes:

 Thanks for the patches!  Please send only one per message (the second
 one as a reply to the first one, or both as replies to a cover letter),
 though -- that makes commenting on them much easier.

 Side note: Documentation/SubmittingPatches doesn't mention that (yet),
 AFAICS.

OK, how about doing this then?

 Documentation/SubmittingPatches | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 7055576..304b3c0 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -140,7 +140,12 @@ comment on the changes you are submitting.  It is 
important for
 a developer to be able to quote your changes, using standard
 e-mail tools, so that they may comment on specific portions of
 your code.  For this reason, all patches should be submitted
-inline.  If your log message (including your name on the
+inline.  A patch series that consists of N commits is sent as N
+separate e-mail messages, or a cover letter message (see below) with
+N separate e-mail messages, each being a response to the cover
+letter.
+
+If your log message (including your name on the
 Signed-off-by line) is not writable in ASCII, make sure that
 you send off a message in the correct encoding.
 

 The feature is disabled for remote repositories as
 the git_work_tree fails. This is a possible future
 enhancement.

 Hmm, curious.  Why does it fail?  I guess that happens with bare
 repositories, only, right?  (Which are the most likely kind of remote
 repos to encounter, of course.)

Yeah, I do not think of a reason why it should fail in a bare
repository, either. git archive is about writing out the contents
of an already recorded tree, so there shouldn't be a reason to even
call get_git_work_tree() in the first place.

Even if the code is run inside a repository with a working tree,
when producing a tarball out of an ancient commit that had a
submodule not at its current location, --recurse-submodules option
should do the right thing, so asking for working tree location of
that submodule to find its repository is wrong, I think.  It may
happen to find one if the archived revision is close enough to what
is currently checked out, but that may not necessarily be the case.

At that point when the code discovers an S_ISGITLINK entry, it
should have both a pathname to the submodule relative to the
toplevel and the commit object name bound to that submodule
location.  What it should do, when it does not find the repository
at the given path (maybe because there is no working tree, or the
sudmodule directory has moved over time) is roughly:

 - Read from .gitmodules at the top-level from the tree it is
   creating the tarball out of;

 - Find submodule.$name.path entry that records that path to the
   submodule; and then

 - Using that $name, find the stashed-away location of the submodule
   repository in $GIT_DIR/modules/$name.

or something like that.

This is a related tangent, but when used in a repository that people
often use as their remote, the repository discovery may have to
interact with the relative URL.  People often ship .gitmodules with

[submodule bar]
URL = ../bar.git
path = barDir

for a top-level project foo that can be cloned thusly:

git clone git://site.xz/foo.git

and host bar.git to be clonable with

git clone git://site.xz/bar.git barDir/

inside the working tree of the foo project.  In such a case, when
archive --recurse-submodules is running, it would find the
repository for the bar submodule at ../bar.git, I would think.

So this part needs a bit more thought, I am afraid.

  'git archive' [--format=fmt] [--list] [--prefix=prefix/] [extra]
[-o file | --output=file] [--worktree-attributes]
 +  [--recursive|--recurse-submodules]

 I'd expect git archive --recurse to add subdirectories and their
 contents, which it does right now, and --no-recurse to only archive the
 specified objects, which is not implemented.  IAW: I wouldn't normally
 associate an option with that name with submodules.  Would
 --recurse-submodules alone suffice?

Jens already commented on this, and I agree that --recursive should
be dropped from this patch.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git issues with submodules

2013-11-26 Thread Junio C Hamano

Jonathan Nieder jrnie...@gmail.com writes:

 Junio C Hamano wrote:

  I have a feeling that the
 current not copy to fix it to a stable value, but look into
 .gitmodules as a fallback was not a designed behaviour for the
 other properties, but was done by accident and/or laziness.

 It was designed.  See for example the thread surrounding [1]:

OK, thanks.


 | And when you are on a superproject branch actively developing inside a
 | submodule, you may want to increase fetch-activity to fetch all new
 | commits in the submodule even if they aren't referenced in the
 | superproject (yet), as that might be just what your fellow developers
 | are about to do. And the person setting up that branch could do that
 | once for all users so they don't have to repeat it in every clone. And
 | when switching away from that branch all those developers cannot forget
 | to reconfigure to fetch-on-demand, so not having that in .git/config is
 | a plus here too.

 Thanks,
 Jonathan

 [1] http://thread.gmane.org/gmane.comp.version-control.git/161193/focus=161357
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4] transport: Catch non positive --depth option value

2013-11-26 Thread Junio C Hamano

Andrés G. Aragoneses kno...@gmail.com writes:

 From 4f3b24379090b7b69046903fba494f3191577b20 Mon Sep 17 00:00:00 2001
 From: =?UTF-8?q?Andr=C3=A9s=20G=2E=20Aragoneses?= kno...@gmail.com
 Date: Tue, 26 Nov 2013 12:38:19 +0100
 Subject: [PATCH] transport: Catch non positive --depth option value

Please do not leave these four lines in your e-mail message, unless
there is a good reason to do so (e.g. when you are forwarding a
patch authored by somebody else, you may want a From: that names
the real author at the beginning, but that does not apply in this
case where you are sending your own).

The first line is merely a marker to say the file is a format-patch
output, and the header lines are there for those who use git
send-email to mail the messages out, and/or for those who want to
cut  paste some of them (not copy  paste) to their MUA header
input widgets.

 Instead of simply ignoring the value passed to --depth option when
 it is zero or negative, now it is caught and reported.

 This will let people know that they were using the option
 incorrectly (as depth0 should be simply invalid, and under the
 hood depth==0 didn't have any effect).

 (The change in fetch.c is needed to avoid the tests failing
 because of this new restriction.)

Good, but it is not just tests but without that change real
operations break.  In other words, it is an integral part of the
patch, not a workaround for a broken test.

Thanks.  Will queue with a bit of tweak.

 Signed-off-by: Andres G. Aragoneses kno...@gmail.com
 Reviewed-by: Duy Nguyen pclo...@gmail.com
 ---
  builtin/fetch.c | 2 +-
  transport.c | 2 ++
  2 files changed, 3 insertions(+), 1 deletion(-)

 diff --git a/builtin/fetch.c b/builtin/fetch.c
 index bd7a101..88c04d7 100644
 --- a/builtin/fetch.c
 +++ b/builtin/fetch.c
 @@ -770,7 +770,7 @@ static void backfill_tags(struct transport *transport, 
 struct ref *ref_map)
   }

   transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 - transport_set_option(transport, TRANS_OPT_DEPTH, 0);
 + transport_set_option(transport, TRANS_OPT_DEPTH, NULL);
   fetch_refs(transport, ref_map);

   if (gsecondary) {
 diff --git a/transport.c b/transport.c
 index 7202b77..5b42ccb 100644
 --- a/transport.c
 +++ b/transport.c
 @@ -483,6 +483,8 @@ static int set_git_option(struct git_transport_options 
 *opts,
   opts-depth = strtol(value, end, 0);
   if (*end)
   die(transport: invalid depth option '%s', 
 value);
 + if (opts-depth  1)
 + die(transport: invalid depth option '%s' (must 
 be positive), value);
   }
   return 0;
   }
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: [PATCH] submodule recursion in git-archive

2013-11-26 Thread Heiko Voigt

Hi,

I like where this is going.

On Tue, Nov 26, 2013 at 04:17:43PM +0100, René Scharfe wrote:
 Am 26.11.2013 01:04, schrieb Nick Townsend:
  +   strbuf_addstr(dotgit, work_tree);
  +   strbuf_addch(dotgit, '/');
  +   if (args-treepath) {
  + strbuf_addstr(dotgit, args-treepath);
  + strbuf_addch(dotgit, '/');
  +   }
  +   strbuf_add(dotgit, 
  path_without_prefix,strlen(path_without_prefix)-1);
  +   if (add_submodule_odb(dotgit.buf))
  + die(Can't add submodule: %s, dotgit.buf);
 
 Hmm, I wonder if we can traverse the tree and load all submodule object
 databases before traversing it again to actually write file contents.
 That would spare the user from getting half of an archive together with
 that error message.

I am not sure whether we should die here. What about submodules that
have not been initialized and or cloned? I think that is a quite regular
use case for example for libraries that not everyone needs or big media
submodules which only the design team uses. How about skipping them (maybe
issuing a warning) by returning 0 here and proceeding?

Cheers Heiko
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] submodule recursion in git-archive

2013-11-26 Thread René Scharfe

Am 26.11.2013 23:18, schrieb Junio C Hamano:
 René Scharfe l@web.de writes:
 
 Thanks for the patches!  Please send only one per message (the second
 one as a reply to the first one, or both as replies to a cover letter),
 though -- that makes commenting on them much easier.

 Side note: Documentation/SubmittingPatches doesn't mention that (yet),
 AFAICS.
 
 OK, how about doing this then?
 
  Documentation/SubmittingPatches | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)
 
 diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
 index 7055576..304b3c0 100644
 --- a/Documentation/SubmittingPatches
 +++ b/Documentation/SubmittingPatches
 @@ -140,7 +140,12 @@ comment on the changes you are submitting.  It is 
 important for
  a developer to be able to quote your changes, using standard
  e-mail tools, so that they may comment on specific portions of
  your code.  For this reason, all patches should be submitted
 -inline.  If your log message (including your name on the
 +inline.  A patch series that consists of N commits is sent as N
 +separate e-mail messages, or a cover letter message (see below) with
 +N separate e-mail messages, each being a response to the cover
 +letter.
 +
 +If your log message (including your name on the
  Signed-off-by line) is not writable in ASCII, make sure that
  you send off a message in the correct encoding.

OK, but the repetition of cover letter and e-mail messages
irritates me slightly for some reason.  What about the following?

-- 8 --
Subject: [PATCH] SubmittingPatches: document how to handle multiple patches

Signed-off-by: Rene Scharfe l@web.de
---
 Documentation/SubmittingPatches |   11 +--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 7055576..e6d46ed 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -139,8 +139,15 @@ People on the Git mailing list need to be able to read and
 comment on the changes you are submitting.  It is important for
 a developer to be able to quote your changes, using standard
 e-mail tools, so that they may comment on specific portions of
-your code.  For this reason, all patches should be submitted
-inline.  If your log message (including your name on the
+your code.  For this reason, each patch should be submitted
+inline in a separate message.
+
+Multiple related patches should be grouped into their own e-mail
+thread to help readers find all parts of the series.  To that end,
+send them as replies to either an additional cover letter message
+(see below), the first patch, or the respective preceding patch.
+
+If your log message (including your name on the
 Signed-off-by line) is not writable in ASCII, make sure that
 you send off a message in the correct encoding.
 
-- 
1.7.8


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 27/28] clone: use git protocol for cloning shallow repo locally

2013-11-26 Thread Eric Sunshine

On Sun, Nov 24, 2013 at 10:55 PM, Nguyễn Thái Ngọc Duy
pclo...@gmail.com wrote:
 clone_local() does not handle $SRC/shallow. It could be made so, but
 it's simpler to use fetch-pack/upload-pack instead.

 This used by be caught by the check in upload-pack, which is triggered

s/used by/used to/

 by transport_get_remote_refs(), even in local clone case. The check is
 now gone and check_everything_connected() should catch the result
 incomplete repo. But check_everything_connected() will soon be skipped
 in local clone case, opening a door to corrupt repo. This patch should
 close that door.

 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 15/28] fetch: add --update-shallow to get refs that require updating .git/shallow

2013-11-26 Thread Eric Sunshine

On Sun, Nov 24, 2013 at 10:55 PM, Nguyễn Thái Ngọc Duy
pclo...@gmail.com wrote:
 diff --git a/t/t5536-fetch-shallow.sh b/t/t5536-fetch-shallow.sh
 index e011ead..95b6313 100755
 --- a/t/t5536-fetch-shallow.sh
 +++ b/t/t5536-fetch-shallow.sh
 @@ -141,4 +141,26 @@ EOF
 )
  '

 +test_expect_success 'fetch --update-shallow' '
 +   (
 +   cd notshallow 
 +   git fetch --update-shallow ../shallow/.git 
 refs/heads/*:refs/remotes/shallow/* 
 +   git fsck 
 +   git for-each-ref --format=%(refname) |sort actual.refs 

Exit status of git-for-each-ref could be lost down the pipe.

 +   cat EOF expect.refs 
 +refs/remotes/shallow/master
 +refs/remotes/shallow/no-shallow
 +EOF
 +   test_cmp expect.refs actual.refs 
 +   git log --format=%s shallow/master actual 
 +   cat EOF expect 
 +6
 +5
 +4
 +3
 +EOF
 +   test_cmp expect actual
 +   )
 +'
 +
  test_done
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] submodule recursion in git-archive

2013-11-26 Thread Nick Townsend


On 26 Nov 2013, at 16:28, René Scharfe l@web.de wrote:

 Am 26.11.2013 23:18, schrieb Junio C Hamano:
 René Scharfe l@web.de writes:
 
 Thanks for the patches!  Please send only one per message (the second
 one as a reply to the first one, or both as replies to a cover letter),
 though -- that makes commenting on them much easier.
 
 Side note: Documentation/SubmittingPatches doesn't mention that (yet),
 AFAICS.
 
 OK, how about doing this then?
 
 Documentation/SubmittingPatches | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)
 
 diff --git a/Documentation/SubmittingPatches 
 b/Documentation/SubmittingPatches
 index 7055576..304b3c0 100644
 --- a/Documentation/SubmittingPatches
 +++ b/Documentation/SubmittingPatches
 @@ -140,7 +140,12 @@ comment on the changes you are submitting.  It is 
 important for
 a developer to be able to quote your changes, using standard
 e-mail tools, so that they may comment on specific portions of
 your code.  For this reason, all patches should be submitted
 -inline.  If your log message (including your name on the
 +inline.  A patch series that consists of N commits is sent as N
 +separate e-mail messages, or a cover letter message (see below) with
 +N separate e-mail messages, each being a response to the cover
 +letter.
 +
 +If your log message (including your name on the
 Signed-off-by line) is not writable in ASCII, make sure that
 you send off a message in the correct encoding.
 
 OK, but the repetition of cover letter and e-mail messages
 irritates me slightly for some reason.  What about the following?
 
 -- 8 --
 Subject: [PATCH] SubmittingPatches: document how to handle multiple patches
 
 Signed-off-by: Rene Scharfe l@web.de
 ---
 Documentation/SubmittingPatches |   11 +--
 1 files changed, 9 insertions(+), 2 deletions(-)
 
 diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
 index 7055576..e6d46ed 100644
 --- a/Documentation/SubmittingPatches
 +++ b/Documentation/SubmittingPatches
 @@ -139,8 +139,15 @@ People on the Git mailing list need to be able to read 
 and
 comment on the changes you are submitting.  It is important for
 a developer to be able to quote your changes, using standard
 e-mail tools, so that they may comment on specific portions of
 -your code.  For this reason, all patches should be submitted
 -inline.  If your log message (including your name on the
 +your code.  For this reason, each patch should be submitted
 +inline in a separate message.
 +
 +Multiple related patches should be grouped into their own e-mail
 +thread to help readers find all parts of the series.  To that end,
 +send them as replies to either an additional cover letter message
 +(see below), the first patch, or the respective preceding patch.
 +
 +If your log message (including your name on the
 Signed-off-by line) is not writable in ASCII, make sure that
 you send off a message in the correct encoding.
 
 -- 
 1.7.8
 
 
That seems clear to me.
At any rate I’m going to rework this based on the collective input and will 
submit them again.
Please check my other replies as there are some discussion points!

Nick--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] submodule recursion in git-archive

2013-11-26 Thread Nick Townsend


On 26 Nov 2013, at 14:38, Heiko Voigt hvo...@hvoigt.net wrote:

 Hi,
 
 I like where this is going.
 
 On Tue, Nov 26, 2013 at 04:17:43PM +0100, René Scharfe wrote:
 Am 26.11.2013 01:04, schrieb Nick Townsend:
 +   strbuf_addstr(dotgit, work_tree);
 +   strbuf_addch(dotgit, '/');
 +   if (args-treepath) {
 + strbuf_addstr(dotgit, args-treepath);
 + strbuf_addch(dotgit, '/');
 +   }
 +   strbuf_add(dotgit, 
 path_without_prefix,strlen(path_without_prefix)-1);
 +   if (add_submodule_odb(dotgit.buf))
 + die(Can't add submodule: %s, dotgit.buf);
 
 Hmm, I wonder if we can traverse the tree and load all submodule object
 databases before traversing it again to actually write file contents.
 That would spare the user from getting half of an archive together with
 that error message.
 
 I am not sure whether we should die here. What about submodules that
 have not been initialized and or cloned? I think that is a quite regular
 use case for example for libraries that not everyone needs or big media
 submodules which only the design team uses. How about skipping them (maybe
 issuing a warning) by returning 0 here and proceeding?
 
 Cheers Heiko

I agree that issuing a warning and continuing is best. If the submodule hasn’t 
been setup
then we should respect that and keep the current behaviour (just archive the 
directory entry).
There is some further debate to be had about the extent to which this should 
work with
un-initialized submodules which I’ll discuss in other replies.

Thanks
Nick--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] submodule recursion in git-archive

2013-11-26 Thread Nick Townsend


On 26 Nov 2013, at 14:18, Junio C Hamano gits...@pobox.com wrote:

 René Scharfe l@web.de writes:
 
 Thanks for the patches!  Please send only one per message (the second
 one as a reply to the first one, or both as replies to a cover letter),
 though -- that makes commenting on them much easier.
 
 Side note: Documentation/SubmittingPatches doesn't mention that (yet),
 AFAICS.
 
 OK, how about doing this then?
 
 Documentation/SubmittingPatches | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)
 
 diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
 index 7055576..304b3c0 100644
 --- a/Documentation/SubmittingPatches
 +++ b/Documentation/SubmittingPatches
 @@ -140,7 +140,12 @@ comment on the changes you are submitting.  It is 
 important for
 a developer to be able to quote your changes, using standard
 e-mail tools, so that they may comment on specific portions of
 your code.  For this reason, all patches should be submitted
 -inline.  If your log message (including your name on the
 +inline.  A patch series that consists of N commits is sent as N
 +separate e-mail messages, or a cover letter message (see below) with
 +N separate e-mail messages, each being a response to the cover
 +letter.
 +
 +If your log message (including your name on the
 Signed-off-by line) is not writable in ASCII, make sure that
 you send off a message in the correct encoding.
 
 
 The feature is disabled for remote repositories as
 the git_work_tree fails. This is a possible future
 enhancement.
 
 Hmm, curious.  Why does it fail?  I guess that happens with bare
 repositories, only, right?  (Which are the most likely kind of remote
 repos to encounter, of course.)
 
 Yeah, I do not think of a reason why it should fail in a bare
 repository, either. git archive is about writing out the contents
 of an already recorded tree, so there shouldn't be a reason to even
 call get_git_work_tree() in the first place.
 
See below for a discussion of why I use the .git file in the work tree to 
load the objects for the submodule. I also thought it should work in a
remote repository - but I ran it on a properly initialized remote repository and
it failed. Since I didn’t need it for my immediate use-case I just decided to 
disable 
it with an error. I can look into this further, but we must decide about the 
question 
below first…

 Even if the code is run inside a repository with a working tree,
 when producing a tarball out of an ancient commit that had a
 submodule not at its current location, --recurse-submodules option
 should do the right thing, so asking for working tree location of
 that submodule to find its repository is wrong, I think.  It may
 happen to find one if the archived revision is close enough to what
 is currently checked out, but that may not necessarily be the case.
 
 At that point when the code discovers an S_ISGITLINK entry, it
 should have both a pathname to the submodule relative to the
 toplevel and the commit object name bound to that submodule
 location.  What it should do, when it does not find the repository
 at the given path (maybe because there is no working tree, or the
 sudmodule directory has moved over time) is roughly:
 
 - Read from .gitmodules at the top-level from the tree it is
   creating the tarball out of;
 
 - Find submodule.$name.path entry that records that path to the
   submodule; and then
 
 - Using that $name, find the stashed-away location of the submodule
   repository in $GIT_DIR/modules/$name.
 
 or something like that.
 
 This is a related tangent, but when used in a repository that people
 often use as their remote, the repository discovery may have to
 interact with the relative URL.  People often ship .gitmodules with
 
   [submodule bar]
   URL = ../bar.git
   path = barDir
 
 for a top-level project foo that can be cloned thusly:
 
   git clone git://site.xz/foo.git
 
 and host bar.git to be clonable with
 
   git clone git://site.xz/bar.git barDir/
 
 inside the working tree of the foo project.  In such a case, when
 archive --recurse-submodules is running, it would find the
 repository for the bar submodule at ../bar.git, I would think.
 
 So this part needs a bit more thought, I am afraid.

I see that there is a lot of potential complexity around setting up a submodule:
* The .gitmodules file can be dirty (easy to flag, but should we allow archive 
to proceed?)
* Users can mess with settings both prior to git submodule init and before git 
submodule update.
* What if it’s a raw clone and the user manually changes things between init 
and update?
* I’m not a git-internals expert but looking through the code I see that you 
can add additional object
directories and change paths as you show above.

For those reasons I deliberately decided not to reproduce the above logic all 
by myself.
On the other hand, what it *did* seem to me is that once you have the .git file
then you know you’ve got all that

Re: [PATCH] submodule recursion in git-archive

2013-11-26 Thread Nick Townsend

On 26 Nov 2013, at 07:17, René Scharfe l@web.de wrote:

 Am 26.11.2013 01:04, schrieb Nick Townsend:
 My first git patch - so shout out if I’ve got the etiquette wrong! Or
 of course if I’ve missed something.
 
 Thanks for the patches!  Please send only one per message (the second
 one as a reply to the first one, or both as replies to a cover letter),
 though -- that makes commenting on them much easier.
 
 Side note: Documentation/SubmittingPatches doesn't mention that (yet),
 AFAICS.
 
 Subject: [PATCH 1/2] submodule: add_submodule_odb() usability
 
 Although add_submodule_odb() is documented as being
 externally usable, it is declared static and also
 has incorrect documentation.
 
 This commit fixes those and makes no changes to
 existing code using them. All tests still pass.
 
 Sign-off missing (see Documentation/SubmittingPatches).
 
 ---
 Documentation/technical/api-ref-iteration.txt | 4 ++--
 submodule.c   | 2 +-
 submodule.h   | 1 +
 3 files changed, 4 insertions(+), 3 deletions(-)
 
 diff --git a/Documentation/technical/api-ref-iteration.txt 
 b/Documentation/technical/api-ref-iteration.txt
 index aa1c50f..cbee624 100644
 --- a/Documentation/technical/api-ref-iteration.txt
 +++ b/Documentation/technical/api-ref-iteration.txt
 @@ -50,10 +50,10 @@ submodules object database. You can do this by a 
 code-snippet like
 this:
 
  const char *path = path/to/submodule
 -if (!add_submodule_odb(path))
 +if (add_submodule_odb(path))
  die(Error submodule '%s' not populated., path);
 
 -`add_submodule_odb()` will return an non-zero value on success. If you
 +`add_submodule_odb()` will return a zero value on success. If you
 
 return zero on success instead?

I like the brevity of your suggestion. Again, I just used what was there…

 
 do not do this you will get an error for each ref that it does not point
 to a valid object.
 
 diff --git a/submodule.c b/submodule.c
 index 1905d75..1ea46be 100644
 --- a/submodule.c
 +++ b/submodule.c
 @@ -143,7 +143,7 @@ void stage_updated_gitmodules(void)
  die(_(staging updated .gitmodules failed));
 }
 
 -static int add_submodule_odb(const char *path)
 +int add_submodule_odb(const char *path)
 {
  struct strbuf objects_directory = STRBUF_INIT;
  struct alternate_object_database *alt_odb;
 diff --git a/submodule.h b/submodule.h
 index 7beec48..3e3cdca 100644
 --- a/submodule.h
 +++ b/submodule.h
 @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], 
 const char *remotes_nam
  struct string_list *needs_pushing);
 int push_unpushed_submodules(unsigned char new_sha1[20], const char 
 *remotes_name);
 void connect_work_tree_and_git_dir(const char *work_tree, const char 
 *git_dir);
 +int add_submodule_odb(const char *path);
 
 #endif
 
 Subject: [PATCH 2/2] archive: allow submodule recursion on git-archive
 
 When using git-archive to produce a dump of a
 repository, the existing code does not recurse
 into a submodule when it encounters it in the tree
 traversal. These changes add a command line flag
 that permits this.
 
 Note that the submodules must be updated in the
 repository, otherwise this cannot take place.
 
 The feature is disabled for remote repositories as
 the git_work_tree fails. This is a possible future
 enhancement.
 
 Hmm, curious.  Why does it fail?  I guess that happens with bare
 repositories, only, right?  (Which are the most likely kind of remote
 repos to encounter, of course.)

I’m not sure why it failed - I didn’t think it should - but it did.
See discussion in other email.

 
 Two additional fields are added to archiver_args:
  * recurse  - a boolean indicator
  * treepath - the path part of the tree-ish
   eg. the 'www' in HEAD:www
 
 The latter is used within the archive writer to
 determin the correct path for the submodule .git
 file.
 
 Signed-off-by: Nick Townsend nick.towns...@mac.com
 ---
 Documentation/git-archive.txt |  9 +
 archive.c | 38 --
 archive.h |  2 ++
 3 files changed, 47 insertions(+), 2 deletions(-)
 
 diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
 index b97aaab..b4df735 100644
 --- a/Documentation/git-archive.txt
 +++ b/Documentation/git-archive.txt
 @@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git archive' [--format=fmt] [--list] [--prefix=prefix/] [extra]
[-o file | --output=file] [--worktree-attributes]
 +  [--recursive|--recurse-submodules]
 
 I'd expect git archive --recurse to add subdirectories and their
 contents, which it does right now, and --no-recurse to only archive the
 specified objects, which is not implemented.  IAW: I wouldn't normally
 associate an option with that name with submodules.  Would
 --recurse-submodules alone suffice?
 
 Side note: With only one of the options defined you could shorten them
 on the command line

Re: [PATCH] bash prompt: add option to disable for a repository

[PATCH] gitweb: Add an option for adding more branch refs

Re: [PATCH] gitweb: Make showing branches configurable

Re: [PATCHv3] transport: Catch non positive --depth option value

Re: [PATCHv3] transport: Catch non positive --depth option value

[PATCHv4] transport: Catch non positive --depth option value

Re: [PATCH 4/9] contrib: remove git-p4import

Re: [PATCH v3 04/28] update-server-info: do not publish shallow clones

Re: [PATCH v3 07/28] shallow.c: add remove_reachable_shallow_points()

Re: [PATCH v3 08/28] shallow.c: add mark_new_shallow_refs()

Re: [PATCH] submodule recursion in git-archive

[PATCH] Prevent buffer overflows when path is too long

Re: Git issues with submodules

Re: [PATCH] submodule recursion in git-archive

Re: [PATCHv4] transport: Catch non positive --depth option value

Re: Git issues with submodules

Re: [PATCH] Prevent buffer overflows when path is too long

Re: Git issues with submodules

Re: [PATCH v3 17/28] Support pushing from a shallow clone

[PATCH 8/9 v2] test: replace shebangs with descriptions in shell libraries

Re: [PATCH] gitweb: Add an option for adding more branch refs

Re: [PATCH] submodule recursion in git-archive

Re: Git issues with submodules

Re: [PATCHv4] transport: Catch non positive --depth option value

Re: Re: [PATCH] submodule recursion in git-archive

Re: [PATCH] submodule recursion in git-archive

Re: [PATCH v3 27/28] clone: use git protocol for cloning shallow repo locally

Re: [PATCH v3 15/28] fetch: add --update-shallow to get refs that require updating .git/shallow

Re: [PATCH] submodule recursion in git-archive

Re: [PATCH] submodule recursion in git-archive

Re: [PATCH] submodule recursion in git-archive

Re: [PATCH] submodule recursion in git-archive

32 matches

Site Navigation

Mail list logo

Footer information