[PATCH 0/3] Implement 'git archive --submodules'

[PATCH 0/3] Implement 'git archive --submodules'

[Lars Hjemli]

This series teaches read_tree_recursive() how to traverse into gitlinked
repositories by automatically adding submodule object databases as
alternates during traversal. It is still perfectly legal for a submodule
not to be checked out, in which case the submodule will be ignored.

On top of this, the implementation of 'git archive --submodules' simply
activates the new feature in read_tree_recursive().

Lars Hjemli (3):
  sha1_file: add function to insert alternate object db
  Teach read_tree_recursive() how to traverse into submodules
  git-archive: add support for --submodules

 Documentation/git-archive.txt |    7 +++-
 archive.c                     |    4 ++
 cache.h                       |    3 ++
 environment.c                 |   12 ++++++
 sha1_file.c                   |    5 +++
 t/t5001-archive-submodules.sh |   78 +++++++++++++++++++++++++++++++++++++++
 tree.c                        |   80 +++++++++++++++++++++++++++++++++++++++++
 7 files changed, 187 insertions(+), 2 deletions(-)
 create mode 100755 t/t5001-archive-submodules.sh

[PATCH 1/3] sha1_file: add function to insert alternate object db

[Lars Hjemli]

This function will be used when implementing traversal into submodules.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 cache.h     |    1 +
 sha1_file.c |    5 +++++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/cache.h b/cache.h
index 8e1af26..daa2d4e 100644
--- a/cache.h
+++ b/cache.h
@@ -724,6 +724,7 @@ extern struct alternate_object_database {
 	char base[FLEX_ARRAY]; /* more */
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
+extern int add_alt_odb(const char *path);
 extern void add_to_alternates_file(const char *reference);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
 extern void foreach_alt_odb(alt_odb_fn, void*);
diff --git a/sha1_file.c b/sha1_file.c
index f08493f..19f9725 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -356,6 +356,11 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
 	}
 }
 
+int add_alt_odb(const char *path)
+{
+	return link_alt_odb_entry(path, strlen(path), NULL, 0);
+}
+
 static void read_info_alternates(const char * relative_base, int depth)
 {
 	char *map;
-- 
1.6.1.150.g5e733b

[PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Lars Hjemli]

The traversal of submodules is only triggered if the current submodule
HEAD commit object is accessible. To this end, read_tree_recursive()
will try to insert the submodule odb as an alternate odb but the lack
of such an odb is not treated as an error since it is then assumed that
the user is not interested in the submodule content. However, if the
submodule odb is found it is treated as an error if the HEAD commit
object is missing.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 cache.h       |    2 +
 environment.c |   12 ++++++++
 tree.c        |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+), 0 deletions(-)

diff --git a/cache.h b/cache.h
index daa2d4e..6728467 100644
--- a/cache.h
+++ b/cache.h
@@ -382,6 +382,8 @@ extern int set_git_dir(const char *path);
 extern const char *get_git_work_tree(void);
 extern const char *read_gitfile_gently(const char *path);
 extern void set_git_work_tree(const char *tree);
+extern int get_traverse_gitlinks();
+extern void set_traverse_gitlinks(int traverse);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
diff --git a/environment.c b/environment.c
index e278bce..35cc557 100644
--- a/environment.c
+++ b/environment.c
@@ -53,6 +53,8 @@ static char *work_tree;
 static const char *git_dir;
 static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;
 
+static int traverse_gitlinks = 0;
+
 static void setup_git_env(void)
 {
 	git_dir = getenv(GIT_DIR_ENVIRONMENT);
@@ -159,3 +161,13 @@ int set_git_dir(const char *path)
 	setup_git_env();
 	return 0;
 }
+
+int get_traverse_gitlinks()
+{
+	return traverse_gitlinks;
+}
+
+void set_traverse_gitlinks(int traverse)
+{
+	traverse_gitlinks = traverse;
+}
diff --git a/tree.c b/tree.c
index 03e782a..87cf309 100644
--- a/tree.c
+++ b/tree.c
@@ -5,6 +5,7 @@
 #include "commit.h"
 #include "tag.h"
 #include "tree-walk.h"
+#include "refs.h"
 
 const char *tree_type = "tree";
 
@@ -89,6 +90,61 @@ static int match_tree_entry(const char *base, int baselen, const char *path, uns
 	return 0;
 }
 
+/* Try to add the objectdb of a submodule */
+int add_gitlink_odb(char *relpath)
+{
+	const char *odbpath;
+	struct stat st;
+
+	odbpath = read_gitfile_gently(mkpath("%s/.git", relpath));
+	if (!odbpath)
+		odbpath = mkpath("%s/.git/objects", relpath);
+
+	if (stat(odbpath, &st))
+		return 1;
+
+	return add_alt_odb(odbpath);
+}
+
+/* Check if we should recurse into the specified submodule */
+int traverse_gitlink(char *path, const unsigned char *commit_sha1,
+		     struct tree **subtree)
+{
+	unsigned char sha1[20];
+	int linked_odb = 0;
+	struct commit *commit;
+	void *buffer;
+	enum object_type type;
+	unsigned long size;
+
+	hashcpy(sha1, commit_sha1);
+	if (!add_gitlink_odb(path)) {
+		linked_odb = 1;
+		if (resolve_gitlink_ref(path, "HEAD", sha1))
+			die("Unable to lookup HEAD in %s", path);
+	}
+
+	buffer = read_sha1_file(sha1, &type, &size);
+	if (!buffer) {
+		if (linked_odb)
+			die("Unable to read object %s in submodule %s",
+			    sha1_to_hex(sha1), path);
+		else
+			return 0;
+	}
+
+	commit = lookup_commit(sha1);
+	if (!commit)
+		die("traverse_gitlink(): internal error");
+
+	if (parse_commit_buffer(commit, buffer, size))
+		die("Failed to parse commit %s in submodule %s",
+		    sha1_to_hex(sha1), path);
+
+	*subtree = commit->tree;
+	return 1;
+}
+
 int read_tree_recursive(struct tree *tree,
 			const char *base, int baselen,
 			int stage, const char **match,
@@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,
 				return -1;
 			continue;
 		}
+		if (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {
+			int retval;
+			char *newbase;
+			struct tree *subtree;
+			unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);
+
+			newbase = xmalloc(baselen + 1 + pathlen);
+			memcpy(newbase, base, baselen);
+			memcpy(newbase + baselen, entry.path, pathlen);
+			newbase[baselen + pathlen] = 0;
+			if (!traverse_gitlink(newbase, entry.sha1, &subtree)) {
+				free(newbase);
+				continue;
+			}
+			newbase[baselen + pathlen] = '/';
+			retval = read_tree_recursive(subtree,
+						     newbase,
+						     baselen + pathlen + 1,
+						     stage, match, fn, context);
+			free(newbase);
+			if (retval)
+				return -1;
+			continue;
+		}
 	}
 	return 0;
 }
-- 
1.6.1.150.g5e733b

[PATCH 3/3] git-archive: add support for --submodules

[Lars Hjemli]

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
 Documentation/git-archive.txt |    7 +++-
 archive.c                     |    4 ++
 t/t5001-archive-submodules.sh |   78 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 2 deletions(-)
 create mode 100755 t/t5001-archive-submodules.sh

diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
index 41cbf9c..84e0b43 100644
--- a/Documentation/git-archive.txt
+++ b/Documentation/git-archive.txt
@@ -10,8 +10,8 @@ SYNOPSIS
 --------
 [verse]
 'git archive' --format=<fmt> [--list] [--prefix=<prefix>/] [<extra>]
-	      [--remote=<repo> [--exec=<git-upload-archive>]] <tree-ish>
-	      [path...]
+	      [--remote=<repo> [--exec=<git-upload-archive>]] [--submodules]
+	      <tree-ish> [path...]
 
 DESCRIPTION
 -----------
@@ -59,6 +59,9 @@ OPTIONS
 	Used with --remote to specify the path to the
 	'git-upload-archive' on the remote side.
 
+--submodules::
+	Include files from checked out submodules.
+
 <tree-ish>::
 	The tree or commit to produce an archive for.
 
diff --git a/archive.c b/archive.c
index 9ac455d..0c024b8 100644
--- a/archive.c
+++ b/archive.c
@@ -255,6 +255,7 @@ static int parse_archive_args(int argc, const char **argv,
 	const char *exec = NULL;
 	int compression_level = -1;
 	int verbose = 0;
+	int submodules = 0;
 	int i;
 	int list = 0;
 	struct option opts[] = {
@@ -262,6 +263,8 @@ static int parse_archive_args(int argc, const char **argv,
 		OPT_STRING(0, "format", &format, "fmt", "archive format"),
 		OPT_STRING(0, "prefix", &base, "prefix",
 			"prepend prefix to each pathname in the archive"),
+		OPT_BOOLEAN(0, "submodules", &submodules,
+			"recurse into submodules"),
 		OPT__VERBOSE(&verbose),
 		OPT__COMPR('0', &compression_level, "store only", 0),
 		OPT__COMPR('1', &compression_level, "compress faster", 1),
@@ -320,6 +323,7 @@ static int parse_archive_args(int argc, const char **argv,
 	args->base = base;
 	args->baselen = strlen(base);
 
+	set_traverse_gitlinks(submodules);
 	return argc;
 }
 
diff --git a/t/t5001-archive-submodules.sh b/t/t5001-archive-submodules.sh
new file mode 100755
index 0000000..5a499a4
--- /dev/null
+++ b/t/t5001-archive-submodules.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+
+test_description='git archive can include submodule content'
+
+. ./test-lib.sh
+
+add_file()
+{
+	git add $1 &&
+	git commit -m "added $1"
+}
+
+add_submodule()
+{
+	mkdir $1 && (
+		cd $1 &&
+		git init &&
+		echo "File $2" >$2 &&
+		add_file $2
+	) &&
+	add_file $1
+}
+
+test_expect_success 'setup submodules' '
+	echo "File 1" >1 &&
+	add_file 1 &&
+	add_submodule 2 3 &&
+	add_submodule 4 5 &&
+	(cd 4 && add_submodule 6 7)
+'
+
+test_expect_success 'git archive usually ignores submodules' '
+	cat <<EOF >expected &&
+1
+2/
+4/
+EOF
+	git archive HEAD >normal.tar &&
+	tar -tf normal.tar >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git archive includes submodules when requested' '
+	cat <<EOF >expected &&
+1
+2/
+2/3
+4/
+4/5
+4/6/
+4/6/7
+EOF
+	git archive --submodules HEAD >full.tar &&
+	tar -tf full.tar >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git archive ignores uninteresting submodules' '
+	cat <<EOF >expected &&
+1
+2/
+4/
+4/5
+4/6/
+4/6/7
+EOF
+	rm -rf 2/.git &&
+	git archive --submodules HEAD >partial.tar &&
+	tar -tf partial.tar >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git archive fails on missing object in interesting submodule' '
+	find 4/.git/objects -type f | xargs rm &&
+	test_must_fail git archive --submodules HEAD
+'
+
+test_done
-- 
1.6.1.150.g5e733b

Re: [PATCH 3/3] git-archive: add support for --submodules

[Johannes Schindelin]

Hi,

On Sun, 18 Jan 2009, Lars Hjemli wrote:

> @@ -320,6 +323,7 @@ static int parse_archive_args(int argc, const char **argv,
>  	args->base = base;
>  	args->baselen = strlen(base);
>  
> +	set_traverse_gitlinks(submodules);

As I said, this is a per-call thing.  So you need to add that option to 
the archiver_args struct and use it in write_archive_entries().

Ciao,
Dscho

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Johannes Schindelin]

Hi,

On Sun, 18 Jan 2009, Lars Hjemli wrote:

> diff --git a/environment.c b/environment.c
> index e278bce..35cc557 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -53,6 +53,8 @@ static char *work_tree;
>  static const char *git_dir;
>  static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;
>  
> +static int traverse_gitlinks = 0;
> +
>  static void setup_git_env(void)
>  {
>  	git_dir = getenv(GIT_DIR_ENVIRONMENT);
> @@ -159,3 +161,13 @@ int set_git_dir(const char *path)
>  	setup_git_env();
>  	return 0;
>  }
> +
> +int get_traverse_gitlinks()
> +{
> +	return traverse_gitlinks;
> +}
> +
> +void set_traverse_gitlinks(int traverse)
> +{
> +	traverse_gitlinks = traverse;
> +}

If you have full accessors anyway, it is much easier and cleaner to make 
this a global variable to begin with.

However, environment.c is reserved for things that come from the config 
and can be overridden by the user.  That is certainly not the case for 
traverse_gitlinks.

But let's think about it again: should traverse_gitlinks be a global 
varible at all?  I think not.  It should be a per-call decision.

 > diff --git a/tree.c b/tree.c
> index 03e782a..87cf309 100644
> --- a/tree.c
> +++ b/tree.c
> @@ -5,6 +5,7 @@
>  #include "commit.h"
>  #include "tag.h"
>  #include "tree-walk.h"
> +#include "refs.h"
>  
>  const char *tree_type = "tree";
>  
> @@ -89,6 +90,61 @@ static int match_tree_entry(const char *base, int baselen, const char *path, uns
>  	return 0;
>  }
>  
> +/* Try to add the objectdb of a submodule */
> +int add_gitlink_odb(char *relpath)

This wants to be static.

> +{
> +	const char *odbpath;
> +	struct stat st;
> +
> +	odbpath = read_gitfile_gently(mkpath("%s/.git", relpath));
> +	if (!odbpath)
> +		odbpath = mkpath("%s/.git/objects", relpath);
> +
> +	if (stat(odbpath, &st))
> +		return 1;
> +
> +	return add_alt_odb(odbpath);
> +}
> +
> +/* Check if we should recurse into the specified submodule */
> +int traverse_gitlink(char *path, const unsigned char *commit_sha1,

This, too.

> +		     struct tree **subtree)
> +{
> +	unsigned char sha1[20];
> +	int linked_odb = 0;
> +	struct commit *commit;
> +	void *buffer;
> +	enum object_type type;
> +	unsigned long size;
> +
> +	hashcpy(sha1, commit_sha1);
> +	if (!add_gitlink_odb(path)) {
> +		linked_odb = 1;
> +		if (resolve_gitlink_ref(path, "HEAD", sha1))
> +			die("Unable to lookup HEAD in %s", path);
> +	}

Why would you want to continue if add_gitlink_odb() did not find a checked 
out submodule?

Seems you want to fall back to look in the superproject's object database.  
But I think that is wrong, as I have a superproject with many platform 
dependent submodules, only one of which is checked out, and for 
convenience, the submodules all live in the superproject's repository.

But I might misunderstand your code.

> +	commit = lookup_commit(sha1);
> +	if (!commit)
> +		die("traverse_gitlink(): internal error");

s/internal error/could not access commit '%s' of submodule '%s'",
			sha1_to_hex(sha1), path);/

> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,
>  				return -1;
>  			continue;
>  		}
> +		if (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {

Like I said, traverse_gitlinks should be a flag to read_tree_recursive.  
So preferably, you should add a parameter 'flags' and make that option an 
enum.

> +			int retval;
> +			char *newbase;
> +			struct tree *subtree;
> +			unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);

Nit: Long line.

> +
> +			newbase = xmalloc(baselen + 1 + pathlen);
> +			memcpy(newbase, base, baselen);
> +			memcpy(newbase + baselen, entry.path, pathlen);
> +			newbase[baselen + pathlen] = 0;

We have strbufs for that.

> +			if (!traverse_gitlink(newbase, entry.sha1, &subtree)) {
> +				free(newbase);
> +				continue;
> +			}
> +			newbase[baselen + pathlen] = '/';

... to avoid this off-by-one.

> +			retval = read_tree_recursive(subtree,
> +						     newbase,
> +						     baselen + pathlen + 1,
> +						     stage, match, fn, context);
> +			free(newbase);
> +			if (retval)
> +				return -1;
> +			continue;
> +		}
>  	}
>  	return 0;
>  }

Ciao,
Dscho

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Lars Hjemli]

On Sun, Jan 18, 2009 at 16:48, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 18 Jan 2009, Lars Hjemli wrote:
>
>> diff --git a/environment.c b/environment.c
>> @@ -159,3 +161,13 @@ int set_git_dir(const char *path)
>>       setup_git_env();
>>       return 0;
>>  }
>> +
>> +int get_traverse_gitlinks()
>> +{
>> +     return traverse_gitlinks;
>> +}
>> +
>> +void set_traverse_gitlinks(int traverse)
>> +{
>> +     traverse_gitlinks = traverse;
>> +}
>
> If you have full accessors anyway, it is much easier and cleaner to make
> this a global variable to begin with.
>
> However, environment.c is reserved for things that come from the config
> and can be overridden by the user.  That is certainly not the case for
> traverse_gitlinks.
>
> But let's think about it again: should traverse_gitlinks be a global
> varible at all?  I think not.  It should be a per-call decision.

Yes, it might be a cleaner solution to add an extra parameter to
read_tree_recursive(), and since it currently has only 7 callsites the
patch wouldn't be too big either. Junio, what do you think?


>> +/* Try to add the objectdb of a submodule */
>> +int add_gitlink_odb(char *relpath)
>
> This wants to be static.

Thanks

>
>> +{
>> +     const char *odbpath;
>> +     struct stat st;
>> +
>> +     odbpath = read_gitfile_gently(mkpath("%s/.git", relpath));
>> +     if (!odbpath)
>> +             odbpath = mkpath("%s/.git/objects", relpath);
>> +
>> +     if (stat(odbpath, &st))
>> +             return 1;
>> +
>> +     return add_alt_odb(odbpath);
>> +}
>> +
>> +/* Check if we should recurse into the specified submodule */
>> +int traverse_gitlink(char *path, const unsigned char *commit_sha1,
>
> This, too.

Thanks again ;-)

>
>> +                  struct tree **subtree)
>> +{
>> +     unsigned char sha1[20];
>> +     int linked_odb = 0;
>> +     struct commit *commit;
>> +     void *buffer;
>> +     enum object_type type;
>> +     unsigned long size;
>> +
>> +     hashcpy(sha1, commit_sha1);
>> +     if (!add_gitlink_odb(path)) {
>> +             linked_odb = 1;
>> +             if (resolve_gitlink_ref(path, "HEAD", sha1))
>> +                     die("Unable to lookup HEAD in %s", path);
>> +     }
>
> Why would you want to continue if add_gitlink_odb() did not find a checked
> out submodule?
>
> Seems you want to fall back to look in the superproject's object database.
> But I think that is wrong, as I have a superproject with many platform
> dependent submodules, only one of which is checked out, and for
> convenience, the submodules all live in the superproject's repository.

Actually, I want this to work for bare repositories by specifying the
submodule odbs in the alternates file. So if the current submodule odb
wasn't found my plan was to check if the commit object was accessible
anyways but don't die() if it wasn't.


>> +     commit = lookup_commit(sha1);
>> +     if (!commit)
>> +             die("traverse_gitlink(): internal error");
>
> s/internal error/could not access commit '%s' of submodule '%s'",
>                        sha1_to_hex(sha1), path);/

Ok (I belive this codepath is virtually impossible to hit, hence the
"internal error", but I could of course be mistaken).


>> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,
>>                               return -1;
>>                       continue;
>>               }
>> +             if (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {
>
> Like I said, traverse_gitlinks should be a flag to read_tree_recursive.
> So preferably, you should add a parameter 'flags' and make that option an
> enum.
>
>> +                     int retval;
>> +                     char *newbase;
>> +                     struct tree *subtree;
>> +                     unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);
>
> Nit: Long line.

Will fix

>
>> +
>> +                     newbase = xmalloc(baselen + 1 + pathlen);
>> +                     memcpy(newbase, base, baselen);
>> +                     memcpy(newbase + baselen, entry.path, pathlen);
>> +                     newbase[baselen + pathlen] = 0;
>
> We have strbufs for that.
>
>> +                     if (!traverse_gitlink(newbase, entry.sha1, &subtree)) {
>> +                             free(newbase);
>> +                             continue;
>> +                     }
>> +                     newbase[baselen + pathlen] = '/';
>
> ... to avoid this off-by-one.

Actually, I don't think this is off-by-one, since (baselen + pathlen +
1) is passed along to read_tree_recursive() as the new baselen. But
using strbufs might be cleaner anyways.

Thanks for the review.

--
larsh

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Johannes Schindelin]

Hi,

On Sun, 18 Jan 2009, Lars Hjemli wrote:

> On Sun, Jan 18, 2009 at 16:48, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Sun, 18 Jan 2009, Lars Hjemli wrote:
> >
> >> +                  struct tree **subtree)
> >> +{
> >> +     unsigned char sha1[20];
> >> +     int linked_odb = 0;
> >> +     struct commit *commit;
> >> +     void *buffer;
> >> +     enum object_type type;
> >> +     unsigned long size;
> >> +
> >> +     hashcpy(sha1, commit_sha1);
> >> +     if (!add_gitlink_odb(path)) {
> >> +             linked_odb = 1;
> >> +             if (resolve_gitlink_ref(path, "HEAD", sha1))
> >> +                     die("Unable to lookup HEAD in %s", path);
> >> +     }
> >
> > Why would you want to continue if add_gitlink_odb() did not find a checked
> > out submodule?
> >
> > Seems you want to fall back to look in the superproject's object database.
> > But I think that is wrong, as I have a superproject with many platform
> > dependent submodules, only one of which is checked out, and for
> > convenience, the submodules all live in the superproject's repository.
> 
> Actually, I want this to work for bare repositories by specifying the
> submodule odbs in the alternates file. So if the current submodule odb
> wasn't found my plan was to check if the commit object was accessible
> anyways but don't die() if it wasn't.

Please make that an explicit option (cannot think of a good name, though), 
otherwise I will not be able to use your feature.  Making it the default 
would be inconsistent with the rest of our submodules framework.

> >> +     commit = lookup_commit(sha1);
> >> +     if (!commit)
> >> +             die("traverse_gitlink(): internal error");
> >
> > s/internal error/could not access commit '%s' of submodule '%s'",
> >                        sha1_to_hex(sha1), path);/
> 
> Ok (I belive this codepath is virtually impossible to hit, hence the
> "internal error", but I could of course be mistaken).

You make it a function that is exported to other parts of Git in cache.h.  
So you might just as well expect it to be used by other parts at some 
stage.

Ciao,
Dscho

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Lars Hjemli]

On Sun, Jan 18, 2009 at 19:33, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Sun, 18 Jan 2009, Lars Hjemli wrote:
>> Actually, I want this to work for bare repositories by specifying the
>> submodule odbs in the alternates file. So if the current submodule odb
>> wasn't found my plan was to check if the commit object was accessible
>> anyways but don't die() if it wasn't.
>
> Please make that an explicit option (cannot think of a good name, though),
> otherwise I will not be able to use your feature.  Making it the default
> would be inconsistent with the rest of our submodules framework.

Would a test on is_bare_repository() suffice for your use-case? That
is, something like this:

	if (!add_gitlink_odb(path->buf)) {
		linked_odb = 1;
		if (resolve_gitlink_ref(path->buf, "HEAD", sha1))
			die("Unable to lookup HEAD in %s", path->buf);
	} else if (!is_bare_repository())
		return 0;

If this isn't good enough, how do you propose it be solved?


>
>> >> +     commit = lookup_commit(sha1);
>> >> +     if (!commit)
>> >> +             die("traverse_gitlink(): internal error");
>> >
>> > s/internal error/could not access commit '%s' of submodule '%s'",
>> >                        sha1_to_hex(sha1), path);/
>>
>> Ok (I belive this codepath is virtually impossible to hit, hence the
>> "internal error", but I could of course be mistaken).
>
> You make it a function that is exported to other parts of Git in cache.h.
> So you might just as well expect it to be used by other parts at some
> stage.

This function is local to tree.c, but your point is still valid.

--
larsh

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Johannes Schindelin]

Hi,

On Sun, 18 Jan 2009, Lars Hjemli wrote:

> On Sun, Jan 18, 2009 at 19:33, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Sun, 18 Jan 2009, Lars Hjemli wrote:
> >> Actually, I want this to work for bare repositories by specifying the
> >> submodule odbs in the alternates file. So if the current submodule odb
> >> wasn't found my plan was to check if the commit object was accessible
> >> anyways but don't die() if it wasn't.
> >
> > Please make that an explicit option (cannot think of a good name, though),
> > otherwise I will not be able to use your feature.  Making it the default
> > would be inconsistent with the rest of our submodules framework.
> 
> Would a test on is_bare_repository() suffice for your use-case?

No.  Inconsistent is inconsistent.

> If this isn't good enough, how do you propose it be solved?

As I said, with an extra option that you _have_ to pass when you want 
that behavior.

> >> >> +     commit = lookup_commit(sha1);
> >> >> +     if (!commit)
> >> >> +             die("traverse_gitlink(): internal error");
> >> >
> >> > s/internal error/could not access commit '%s' of submodule '%s'",
> >> >                        sha1_to_hex(sha1), path);/
> >>
> >> Ok (I belive this codepath is virtually impossible to hit, hence the
> >> "internal error", but I could of course be mistaken).
> >
> > You make it a function that is exported to other parts of Git in cache.h.
> > So you might just as well expect it to be used by other parts at some
> > stage.
> 
> This function is local to tree.c, but your point is still valid.

My point is still valid because I never talked about the static function, 
but the non-static one which calls the static one.

Ciao,
Dscho

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Lars Hjemli]

On Sun, Jan 18, 2009 at 22:02, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 18 Jan 2009, Lars Hjemli wrote:
>
>> On Sun, Jan 18, 2009 at 19:33, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > On Sun, 18 Jan 2009, Lars Hjemli wrote:
>> >> Actually, I want this to work for bare repositories by specifying the
>> >> submodule odbs in the alternates file. So if the current submodule odb
>> >> wasn't found my plan was to check if the commit object was accessible
>> >> anyways but don't die() if it wasn't.
>> >
>> > Please make that an explicit option (cannot think of a good name, though),
>> > otherwise I will not be able to use your feature.  Making it the default
>> > would be inconsistent with the rest of our submodules framework.
>>
>> Would a test on is_bare_repository() suffice for your use-case?
>
> No.  Inconsistent is inconsistent.
>
>> If this isn't good enough, how do you propose it be solved?
>
> As I said, with an extra option that you _have_ to pass when you want
> that behavior.

My concern is how to discern between wanted and unwanted submodules in
a bare repository.

With my proposed solution `git archive --submodules HEAD` in a bare
repository would only include the content of the submodule repos
listed in objects/info/alternates (since the commit referenced by the
gitlink would then be reachable).

But you mentioned that you had a repository where all the objects of
all the submodules where stored in the odb of the superproject. With
my solution, `git archive --submodules HEAD` in your (bare) repo would
then always include the content of all the submodules (since all the
objects would always be reachable), and I believe this is the behavior
you don't like.

So, would you rather have something like `git archive --submodules=foo
--submodules=bar HEAD` to explicitly tell which submodule paths to
include in the archive when executed in a bare repo?

--
larsh

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Johannes Schindelin]

Hi,

On Sun, 18 Jan 2009, Lars Hjemli wrote:

> So, would you rather have something like `git archive --submodules=foo 
> --submodules=bar HEAD` to explicitly tell which submodule paths to 
> include in the archive when executed in a bare repo?

That does not quite say what you tried to do, does it?  You tried to 
traverse submodules whose commit can be found in the object database.

Setting aside the fact that we usually try to avoid accessing unreachable 
objects, which your handling does not do, our "git submodule" does not do 
that either; it only handles submodules that are checked out.

Now, this behavior might be wanted, in bare as well as in non-bare 
repositories, but I think it should be triggered by an option, such as 
"--submodules=look-in-superprojects-odb".

I know, I know, the naming is horrible, but I find it just wrong to 
introduce a behavior that would only confuse users because it introduces 
inconsistent behavior.  As it is, we see too many confused users in #git 
already with consistent behavior [*1*].

Ciao,
Dscho

[*1*] Maybe we should allow cloning empty repositories (with no default 
branch at all), disable pushing into checked out branches by default, and 
make "git add empty-dir/" add a .gitignore and add that -- to squash at 
least half of the questions inside #git so that we can go back to fooling 
around there.

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Lars Hjemli]

On Sun, Jan 18, 2009 at 22:55, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 18 Jan 2009, Lars Hjemli wrote:
>
>> So, would you rather have something like `git archive --submodules=foo
>> --submodules=bar HEAD` to explicitly tell which submodule paths to
>> include in the archive when executed in a bare repo?
>
> That does not quite say what you tried to do, does it?  You tried to
> traverse submodules whose commit can be found in the object database.
>
> Setting aside the fact that we usually try to avoid accessing unreachable
> objects, which your handling does not do, our "git submodule" does not do
> that either; it only handles submodules that are checked out.
>
> Now, this behavior might be wanted, in bare as well as in non-bare
> repositories, but I think it should be triggered by an option, such as
> "--submodules=look-in-superprojects-odb".

Sorry, but if your concern is whether to traverse a submodule in a
bare repo when the submodule isn't checked out (yeah, contradiction in
terms), I just don't see the point.

For non-bare repositories the policy has always been to ignore
submodules which isn't checked out, but for bare repositories there is
no obvious way (for me, at least) to apply the same policy. Therefore
I proposed to traverse all submodules where the linked commit is
reachable, but as you pointed out this would be wrong for non-bare
repositories.

I then modified my proposal to include a check on
is_bare_repository(): If we're not in a bare repository,
read_tree_recursive() is only allowed to recurse into checked out
submodules. But if we're in a bare repository, read_tree_recursive()
is allowed to recurse into any submodule with a reachable commit.

Now then, if `--submodules=look-in-superprojects-odb` should be
required to trigger the latter behavior, running `git archive
--submodules HEAD` in a bare repository would always produce identical
output as `git archive HEAD` and this is why I don't understand the
gain of 'look-in-superprojects-odb' (I thought you wanted to limit
which of the reachable submodules to recurse into).

--
larsh

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Johannes Schindelin]

Hi,

On Sun, 18 Jan 2009, Lars Hjemli wrote:

> Sorry, but if your concern is whether to traverse a submodule in a bare 
> repo when the submodule isn't checked out (yeah, contradiction in 
> terms), I just don't see the point.

Obviously.

> For non-bare repositories the policy has always been to ignore
> submodules which isn't checked out, but for bare repositories there is
> no obvious way (for me, at least) to apply the same policy.

There is one:  we never traverse them in bare repositories.

Never.

You are introducing that contradicts that on purpose.  Which I do not 
like at all.

Sure, what you want is a nifty feature, but you'll have to do it right.

For example, your handling for bare repositories precludes everybody from 
specifying -- just for this particular call to git archive -- what 
submodules they want to include.  And you preclude anybody from excluding 
-- just for this particular call to git archive -- certain submodules 
whose commits just so happen to be present in the superproject.

For me, that is a sign of a bad user interface design.

Ciao,
Dscho

P.S.: if you still don't get the point, I will just shut up, until the 
question crops up, and redirect every person confused by that behavior to 
you.  Be prepared.

[PATCH/RFC v1 1/1] bug fix, diff whitespace ignore options

[Keith Cascio]

  Fixed bug in diff whitespace ignore options.
  It is now OK to specify more than one whitespace ignore option
  on the command line. In unit test 4015, expect success rather
  than failure for 4 cases.
  Note: I do not fully understand why this fix works, but it passes
  all 68 t4???-* diff test scripts.

The semantics of the three whitespace ignore flags
{ -w, -b, --ignore-space-at-eol }
obey a relation of transitive implication, i.e. the stronger
options imply the weaker options:
-w                    implies the other two
-b                    implies --ignore-space-at-eol
--ignore-space-at-eol implies only itself

Therefore it is never necessary to specify more than one of these
on the command line.  Yet we imagine scenarios where software
wrappers (e.g. GUIs, etc) generate command lines that switch on
more than one of these flags simultaneously.  It is unreasonable
to prohibit specifying more than one, since a new user might not
immediately discern the implication relation.  Now we call such
a command line valid and legal.

Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
---
  t/t4015-diff-whitespace.sh |    8 ++++----
  xdiff/xutils.c             |   22 ++++++++++++----------
  2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index dbb608c..6d13da3 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -99,11 +99,11 @@ EOF
  git diff -w > out
  test_expect_success 'another test, with -w' 'test_cmp expect out'
  git diff -w -b > out
-test_expect_failure 'another test, with -w -b' 'test_cmp expect out'
+test_expect_success 'another test, with -w -b' 'test_cmp expect out'
  git diff -w --ignore-space-at-eol > out
-test_expect_failure 'another test, with -w --ignore-space-at-eol' 'test_cmp expect out'
+test_expect_success 'another test, with -w --ignore-space-at-eol' 'test_cmp expect out'
  git diff -w -b --ignore-space-at-eol > out
-test_expect_failure 'another test, with -w -b --ignore-space-at-eol' 'test_cmp expect out'
+test_expect_success 'another test, with -w -b --ignore-space-at-eol' 'test_cmp expect out'

  tr 'Q' '\015' << EOF > expect
  diff --git a/x b/x
@@ -123,7 +123,7 @@ EOF
  git diff -b > out
  test_expect_success 'another test, with -b' 'test_cmp expect out'
  git diff -b --ignore-space-at-eol > out
-test_expect_failure 'another test, with -b --ignore-space-at-eol' 'test_cmp expect out'
+test_expect_success 'another test, with -b --ignore-space-at-eol' 'test_cmp expect out'

  tr 'Q' '\015' << EOF > expect
  diff --git a/x b/x
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index d7974d1..b9bda86 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -245,17 +245,19 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
  			while (ptr + 1 < top && isspace(ptr[1])
  					&& ptr[1] != '\n')
  				ptr++;
-			if (flags & XDF_IGNORE_WHITESPACE_CHANGE
-					&& ptr[1] != '\n') {
-				ha += (ha << 5);
-				ha ^= (unsigned long) ' ';
-			}
-			if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
-					&& ptr[1] != '\n') {
-				while (ptr2 != ptr + 1) {
+			if( ! (          flags & XDF_IGNORE_WHITESPACE       )){
+				if(      flags & XDF_IGNORE_WHITESPACE_CHANGE
+						&& ptr[1] != '\n') {
  					ha += (ha << 5);
-					ha ^= (unsigned long) *ptr2;
-					ptr2++;
+					ha ^= (unsigned long) ' ';
+				}
+				else if( flags & XDF_IGNORE_WHITESPACE_AT_EOL
+						&& ptr[1] != '\n') {
+					while (ptr2 != ptr + 1) {
+						ha += (ha << 5);
+						ha ^= (unsigned long) *ptr2;
+						ptr2++;
+					}
  				}
  			}
  			continue;
-- 
1.6.1.203.ga83c8.dirty

Re: [PATCH/RFC v1 1/1] bug fix, diff whitespace ignore options

[Johannes Schindelin]

Hi,

On Sun, 18 Jan 2009, Keith Cascio wrote:

>  Fixed bug in diff whitespace ignore options.
>  It is now OK to specify more than one whitespace ignore option
>  on the command line. In unit test 4015, expect success rather
>  than failure for 4 cases.
>  Note: I do not fully understand why this fix works, but it passes
>  all 68 t4???-* diff test scripts.
> 
> The semantics of the three whitespace ignore flags
> { -w, -b, --ignore-space-at-eol }
> obey a relation of transitive implication, i.e. the stronger
> options imply the weaker options:
> -w                    implies the other two
> -b                    implies --ignore-space-at-eol
> --ignore-space-at-eol implies only itself
> 
> Therefore it is never necessary to specify more than one of these
> on the command line.  Yet we imagine scenarios where software
> wrappers (e.g. GUIs, etc) generate command lines that switch on
> more than one of these flags simultaneously.  It is unreasonable
> to prohibit specifying more than one, since a new user might not
> immediately discern the implication relation.  Now we call such
> a command line valid and legal.
> 
> Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
> ---

This does not really look all that similar to other commit messages.

For example, "Note: I do not fully understand why this fix works, but it 
passes all 68 t4???-* diff test scripts." is rather discouraging.  If you 
are not convinced, how should we be?

However, I almost can excuse that, but...

>  t/t4015-diff-whitespace.sh |    8 ++++----
>  xdiff/xutils.c             |   22 ++++++++++++----------
>  2 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index d7974d1..b9bda86 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -245,17 +245,19 @@ static unsigned long
> xdl_hash_record_with_whitespace(char const **data,
>  			while (ptr + 1 < top && isspace(ptr[1])
>  					&& ptr[1] != '\n')
>  				ptr++;
> -			if (flags & XDF_IGNORE_WHITESPACE_CHANGE
> -					&& ptr[1] != '\n') {
> -				ha += (ha << 5);
> -				ha ^= (unsigned long) ' ';
> -			}
> -			if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
> -					&& ptr[1] != '\n') {
> -				while (ptr2 != ptr + 1) {
> +			if( ! (          flags & XDF_IGNORE_WHITESPACE

... this is just plain ugly, not to mention breaking the coding style of 
the surrounding code in a rather blatant way.

> )){
> +				if(      flags & XDF_IGNORE_WHITESPACE_CHANGE
> +						&& ptr[1] != '\n') {
>  					ha += (ha << 5);
> -					ha ^= (unsigned long) *ptr2;
> -					ptr2++;
> +					ha ^= (unsigned long) ' ';
> +				}
> +				else if( flags & XDF_IGNORE_WHITESPACE_AT_EOL
> +						&& ptr[1] != '\n') {
> +					while (ptr2 != ptr + 1) {
> +						ha += (ha << 5);
> +						ha ^= (unsigned long) *ptr2;
> +						ptr2++;
> +					}

Besides, I think what you actually wanted is

		if (flags & XDF_IGNORE_WHITESPACE)
			; /* already handled */
		else if (flags & XDF_IGNORE_WHITESPACE_CHANGE)
			...
		else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL)
			...

for improved readability both of the code and the patch.

Ciao,
Dscho

[PATCH/RFC v2 1/1] bug fix, diff whitespace ignore options

[Keith Cascio]

  Fixed bug in diff whitespace ignore options.  It is now
  OK to specify more than one whitespace ignore option
  on the command line.

Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
---
Dscho,
You are right.  The code and the patch are more readable this way.
                                         -- Keith

  t/t4015-diff-whitespace.sh |    8 ++++----
  xdiff/xutils.c             |    6 ++++--
  2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index dbb608c..6d13da3 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -99,11 +99,11 @@ EOF
  git diff -w > out
  test_expect_success 'another test, with -w' 'test_cmp expect out'
  git diff -w -b > out
-test_expect_failure 'another test, with -w -b' 'test_cmp expect out'
+test_expect_success 'another test, with -w -b' 'test_cmp expect out'
  git diff -w --ignore-space-at-eol > out
-test_expect_failure 'another test, with -w --ignore-space-at-eol' 'test_cmp expect out'
+test_expect_success 'another test, with -w --ignore-space-at-eol' 'test_cmp expect out'
  git diff -w -b --ignore-space-at-eol > out
-test_expect_failure 'another test, with -w -b --ignore-space-at-eol' 'test_cmp expect out'
+test_expect_success 'another test, with -w -b --ignore-space-at-eol' 'test_cmp expect out'

  tr 'Q' '\015' << EOF > expect
  diff --git a/x b/x
@@ -123,7 +123,7 @@ EOF
  git diff -b > out
  test_expect_success 'another test, with -b' 'test_cmp expect out'
  git diff -b --ignore-space-at-eol > out
-test_expect_failure 'another test, with -b --ignore-space-at-eol' 'test_cmp expect out'
+test_expect_success 'another test, with -b --ignore-space-at-eol' 'test_cmp expect out'

  tr 'Q' '\015' << EOF > expect
  diff --git a/x b/x
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index d7974d1..04ad468 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -245,12 +245,14 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
  			while (ptr + 1 < top && isspace(ptr[1])
  					&& ptr[1] != '\n')
  				ptr++;
-			if (flags & XDF_IGNORE_WHITESPACE_CHANGE
+			if (flags & XDF_IGNORE_WHITESPACE)
+				; /* already handled */
+			else if (flags & XDF_IGNORE_WHITESPACE_CHANGE
  					&& ptr[1] != '\n') {
  				ha += (ha << 5);
  				ha ^= (unsigned long) ' ';
  			}
-			if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
+			else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
  					&& ptr[1] != '\n') {
  				while (ptr2 != ptr + 1) {
  					ha += (ha << 5);
-- 
1.6.1.213.g28da8.dirty

Re: [PATCH/RFC v2 1/1] bug fix, diff whitespace ignore options

[Johannes Schindelin]

Hi,

On Mon, 19 Jan 2009, Keith Cascio wrote:

>  Fixed bug in diff whitespace ignore options.  It is now
>  OK to specify more than one whitespace ignore option
>  on the command line.
> 
> Signed-off-by: Keith Cascio <keith@cs.ucla.edu>

ACK,
Dscho

Re: [PATCH/RFC v2 1/1] bug fix, diff whitespace ignore options

[Junio C Hamano]

Keith Cascio <keith@CS.UCLA.EDU> writes:

>  Fixed bug in diff whitespace ignore options.  It is now
>  OK to specify more than one whitespace ignore option
>  on the command line.
>
> Signed-off-by: Keith Cascio <keith@cs.ucla.edu>
> ---
> Dscho,
> You are right.  The code and the patch are more readable this way.

Thanks; I've fixed it up so there is no need to resend but your patch was
whitespace mangled (format=flowed), by the way.

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Junio C Hamano]

Lars Hjemli <hjemli@gmail.com> writes:

> On Sun, Jan 18, 2009 at 16:48, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> ...
>> Seems you want to fall back to look in the superproject's object database.
>> But I think that is wrong, as I have a superproject with many platform
>> dependent submodules, only one of which is checked out, and for
>> convenience, the submodules all live in the superproject's repository.
>
> Actually, I want this to work for bare repositories by specifying the
> submodule odbs in the alternates file. So if the current submodule odb
> wasn't found my plan was to check if the commit object was accessible
> anyways but don't die() if it wasn't.

The current submodule design is "do not recurse into them by default
without being told" throughout the Porcelain.  We can think of various
ways for the users to tell which submodules are of interest and which are
uninteresting.

The most general solution would be to give a list of submodules you are
interested in recursing into from the command line, something like:

    $ git command --with-submodule=path1 --with-submodule=path2...

That approach would work equally "well" with a bare repository or with a
non-bare repository, but if you have N submodules, you need to give up to
N extra options, which may be cumbersome (meaning, "works equally well"
above actually may mean "works equally awkwardly").  One way to solve
awkwardness may be to support a mechanism that allows you to use
configuration variables to name a group of submodules.

In addition to such configuration variables, you already have one default
group of submodules, defined by the way you set up your work tree, when
your superproject does have a work tree.  Some submodules have
repositories cloned in the work tree, while some don't, and the ones
without clones can be defined as "uninteresting ones" (to the work tree
owner) that are outside the default group.  For many work tree oriented
operations, it may even make sense to allow that group to be used with a
single "git command --with-submodule" (i.e. when you do not say which
submodule you mean, that can default to "cloned" group).

I do not know "has an entry in the superproject's alternate list that
points to its object store" is a good basis to define another default
group useful especially in a bare repository setting; you seem to be
suggesting that, and you might be correct.

For "git archive", however, I suspect the "default group based on work
tree checkout state" may make the least sense.  "git archive HEAD" is
expected to give a reproducible dump of the state recorded by the HEAD
commit no matter who runs it in what repository, and I think there should
be a conscious and explicit instruction from the end user that says "Here
is a dump from this commit in the superproject, *BUT* it was made together
with contents from this and that submodule".  Command line options that
list "this and that submodule" is explicit enough, and a configured
nickname given to a known group of submodules from the command line may be
so as well, but the group based on the checkout state feels a bit too
implicit and magical to my taste.  The group based on the "has an entry in
superproject's alternates" criterion is not much better in this regard,
methinks.

Another worrysome thing about "git archive" is that it marks the resulting
archive with the commit object name the tarball was taken from.  If you
allow recursing into an arbitrary subset of submodule, a project with N
submodules can produce 2^N different varieties of archive, all marked with
the same commit object name from the superproject.  That might be a bit
too confusing.

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[René Scharfe]

Lars Hjemli schrieb:
> The traversal of submodules is only triggered if the current submodule
> HEAD commit object is accessible. To this end, read_tree_recursive()
> will try to insert the submodule odb as an alternate odb but the lack
> of such an odb is not treated as an error since it is then assumed that
> the user is not interested in the submodule content. However, if the
> submodule odb is found it is treated as an error if the HEAD commit
> object is missing.

Callers of read_tree_recursive() specify a tree to traverse.
Unconditionally using the HEAD of submodules feels a bit restrictive,
but I don't use submodules, so I have no idea what I'm actually talking
about here. :)

>  int read_tree_recursive(struct tree *tree,
>  			const char *base, int baselen,
>  			int stage, const char **match,
> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,
>  				return -1;
>  			continue;
>  		}
> +		if (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {
> +			int retval;
> +			char *newbase;
> +			struct tree *subtree;
> +			unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);
> +
> +			newbase = xmalloc(baselen + 1 + pathlen);
> +			memcpy(newbase, base, baselen);
> +			memcpy(newbase + baselen, entry.path, pathlen);
> +			newbase[baselen + pathlen] = 0;
> +			if (!traverse_gitlink(newbase, entry.sha1, &subtree)) {
> +				free(newbase);
> +				continue;
> +			}
> +			newbase[baselen + pathlen] = '/';
> +			retval = read_tree_recursive(subtree,
> +						     newbase,
> +						     baselen + pathlen + 1,
> +						     stage, match, fn, context);
> +			free(newbase);
> +			if (retval)
> +				return -1;
> +			continue;
> +		}
>  	}
>  	return 0;
>  }

You don't need to call get_traverse_gitlinks() in the if statement above
if you make all read_tree_recursive() callback functions return 0 for
gitlinks that they don't want to follow and READ_TREE_RECURSIVE for
those they do.  It's cleaner without the static variable and its
accessors and more flexible, too: the callbacks might decide to traverse
only certain submodules.

René

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Lars Hjemli]

On Sun, Jan 18, 2009 at 17:13, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> Lars Hjemli schrieb:
>> The traversal of submodules is only triggered if the current submodule
>> HEAD commit object is accessible. To this end, read_tree_recursive()
>> will try to insert the submodule odb as an alternate odb but the lack
>> of such an odb is not treated as an error since it is then assumed that
>> the user is not interested in the submodule content. However, if the
>> submodule odb is found it is treated as an error if the HEAD commit
>> object is missing.
>
> Callers of read_tree_recursive() specify a tree to traverse.
> Unconditionally using the HEAD of submodules feels a bit restrictive,
> but I don't use submodules, so I have no idea what I'm actually talking
> about here. :)

For bare repositories (where the submodule repo is added to
objects/info/alternates), following the tree of the linked commit is
the only option. And for non-bare repositories with the submodule
checked out, I think we should honor the users choice of checked out
HEAD in the submodule (especially since we don't have any other way to
specify which submodule commit to follow).


>
>>  int read_tree_recursive(struct tree *tree,
>>                       const char *base, int baselen,
>>                       int stage, const char **match,
>> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,
>>                               return -1;
>>                       continue;
>>               }
>> +             if (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {
>> +                     int retval;
>> +                     char *newbase;
>> +                     struct tree *subtree;
>> +                     unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);
>> +
>> +                     newbase = xmalloc(baselen + 1 + pathlen);
>> +                     memcpy(newbase, base, baselen);
>> +                     memcpy(newbase + baselen, entry.path, pathlen);
>> +                     newbase[baselen + pathlen] = 0;
>> +                     if (!traverse_gitlink(newbase, entry.sha1, &subtree)) {
>> +                             free(newbase);
>> +                             continue;
>> +                     }
>> +                     newbase[baselen + pathlen] = '/';
>> +                     retval = read_tree_recursive(subtree,
>> +                                                  newbase,
>> +                                                  baselen + pathlen + 1,
>> +                                                  stage, match, fn, context);
>> +                     free(newbase);
>> +                     if (retval)
>> +                             return -1;
>> +                     continue;
>> +             }
>>       }
>>       return 0;
>>  }
>
> You don't need to call get_traverse_gitlinks() in the if statement above
> if you make all read_tree_recursive() callback functions return 0 for
> gitlinks that they don't want to follow and READ_TREE_RECURSIVE for
> those they do.  It's cleaner without the static variable and its
> accessors and more flexible, too: the callbacks might decide to traverse
> only certain submodules.

I like the idea, but it will require thorough review of all
read_tree_recursive() consumers. So now we've got three different
approaches:
* me: global setting
* dscho: parameter to read_tree_recursive()
* you: accept the return value from the callback function

Junio, what would you prefer?

--
larsh

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Junio C Hamano]

Lars Hjemli <hjemli@gmail.com> writes:

> I like the idea, but it will require thorough review of all
> read_tree_recursive() consumers. So now we've got three different
> approaches:
> * me: global setting
> * dscho: parameter to read_tree_recursive()
> * you: accept the return value from the callback function
>
> Junio, what would you prefer?

As usual René has the best taste in designing things ;-)

Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules

[Lars Hjemli]

On Sun, Jan 18, 2009 at 20:00, Junio C Hamano <gitster@pobox.com> wrote:
> Lars Hjemli <hjemli@gmail.com> writes:
>
>> I like the idea, but it will require thorough review of all
>> read_tree_recursive() consumers. So now we've got three different
>> approaches:
>> * me: global setting
>> * dscho: parameter to read_tree_recursive()
>> * you: accept the return value from the callback function
>>
>> Junio, what would you prefer?
>
> As usual René has the best taste in designing things ;-)

Agreed. I've pushed an updated series using his approach to the
lh/traverse-gitlinks branch in git://hjemli.net/pub/git/git
(http://hjemli.net/git/git/log/?h=lh/traverse-gitlinks), but without a
fix for Dscho's concern about the behaviour in
tree.c::traverse_gitlink(). Hopefully I'll get to send a reworked
series later tonight.

--
larsh

Re: [PATCH 1/3] sha1_file: add function to insert alternate object db

[Johannes Schindelin]

Hi,

On Sun, 18 Jan 2009, Lars Hjemli wrote:

> diff --git a/cache.h b/cache.h
> index 8e1af26..daa2d4e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -724,6 +724,7 @@ extern struct alternate_object_database {
>  	char base[FLEX_ARRAY]; /* more */
>  } *alt_odb_list;
>  extern void prepare_alt_odb(void);
> +extern int add_alt_odb(const char *path);
>  extern void add_to_alternates_file(const char *reference);
>  typedef int alt_odb_fn(struct alternate_object_database *, void *);
>  extern void foreach_alt_odb(alt_odb_fn, void*);
> diff --git a/sha1_file.c b/sha1_file.c
> index f08493f..19f9725 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -356,6 +356,11 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
>  	}
>  }
>  
> +int add_alt_odb(const char *path)
> +{
> +	return link_alt_odb_entry(path, strlen(path), NULL, 0);

This function can return the error message "object directory %s does not 
exist; check .git/objects/info/alternates."  Maybe you want to change 
that, even if the user you are introducing might not hit that code path.

Ciao,
Dscho

[PATCH] sha1_file: add function to insert alternate object db

[Lars Hjemli]

This function will be used when implementing traversal into submodules.

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---

On Sun, Jan 18, 2009 at 16:32, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> On Sun, 18 Jan 2009, Lars Hjemli wrote:
>> +int add_alt_odb(const char *path)
>> +{
>> +     return link_alt_odb_entry(path, strlen(path), NULL, 0);
>
> This function can return the error message "object directory %s does not
> exist; check .git/objects/info/alternates."  Maybe you want to change
> that, even if the user you are introducing might not hit that code path.

Something like this, maybe?


 cache.h     |    1 +
 sha1_file.c |   17 ++++++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index 8e1af26..0dbe2a6 100644
--- a/cache.h
+++ b/cache.h
@@ -724,6 +724,7 @@ extern struct alternate_object_database {
 	char base[FLEX_ARRAY]; /* more */
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
+extern int add_alt_odb(const char *path, int quiet);
 extern void add_to_alternates_file(const char *reference);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
 extern void foreach_alt_odb(alt_odb_fn, void*);
diff --git a/sha1_file.c b/sha1_file.c
index f08493f..4b7e691 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -252,7 +252,8 @@ static void read_info_alternates(const char * alternates, int depth);
  * SHA1, an extra slash for the first level indirection, and the
  * terminating NUL.
  */
-static int link_alt_odb_entry(const char * entry, int len, const char * relative_base, int depth)
+static int link_alt_odb_entry(const char * entry, int len,
+			      const char * relative_base, int depth, int quiet)
 {
 	const char *objdir = get_object_directory();
 	struct alternate_object_database *ent;
@@ -285,9 +286,10 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
 
 	/* Detect cases where alternate disappeared */
 	if (!is_directory(ent->base)) {
-		error("object directory %s does not exist; "
-		      "check .git/objects/info/alternates.",
-		      ent->base);
+		if (!quiet)
+			error("object directory %s does not exist; "
+			      "check .git/objects/info/alternates.",
+			      ent->base);
 		free(ent);
 		return -1;
 	}
@@ -347,7 +349,7 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
 						relative_base, last);
 			} else {
 				link_alt_odb_entry(last, cp - last,
-						relative_base, depth);
+						relative_base, depth, 0);
 			}
 		}
 		while (cp < ep && *cp == sep)
@@ -356,6 +358,11 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
 	}
 }
 
+int add_alt_odb(const char *path, int quiet)
+{
+	return link_alt_odb_entry(path, strlen(path), NULL, 0, quiet);
+}
+
 static void read_info_alternates(const char * relative_base, int depth)
 {
 	char *map;
-- 
1.6.1.150.g5e733b