* [PATCH 1/7] git-mv: Remove dead code branch
From: Petr Baudis @ 2008-07-16 19:11 UTC (permalink / raw)
To: git
In-Reply-To: <20080716190753.19772.93357.stgit@localhost>
The path list builder had a branch for the case the source is not in index, but
this can happen only if the source was a directory. However, in that case we
have already expanded the list to the directory contents and set mode
to WORKING_DIRECTORY, which is tested earlier.
The patch removes the superfluous branch and adds an assert() instead. git-mv
testsuite still passes.
Signed-off-by: Petr Baudis <pasky@suse.cz>
---
builtin-mv.c | 16 +++++++---------
1 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/builtin-mv.c b/builtin-mv.c
index 5530e11..158a83d 100644
--- a/builtin-mv.c
+++ b/builtin-mv.c
@@ -227,15 +227,13 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
if (mode == WORKING_DIRECTORY)
continue;
- if (cache_name_pos(src, strlen(src)) >= 0) {
- path_list_insert(src, &deleted);
-
- /* destination can be a directory with 1 file inside */
- if (path_list_has_path(&overwritten, dst))
- path_list_insert(dst, &changed);
- else
- path_list_insert(dst, &added);
- } else
+ assert(cache_name_pos(src, strlen(src)) >= 0);
+
+ path_list_insert(src, &deleted);
+ /* destination can be a directory with 1 file inside */
+ if (path_list_has_path(&overwritten, dst))
+ path_list_insert(dst, &changed);
+ else
path_list_insert(dst, &added);
}
^ permalink raw reply related
* [PATCH 2/7] t7400: Add short "git submodule add" testsuite
From: Petr Baudis @ 2008-07-16 19:11 UTC (permalink / raw)
To: git
In-Reply-To: <20080716190753.19772.93357.stgit@localhost>
This patch introduces basic tests for
git submodule add
covering the basic functionality and the -b parameter.
A trivial update --init test fix freeloads on this commit as well.
Signed-off-by: Petr Baudis <pasky@suse.cz>
---
t/t7400-submodule-basic.sh | 28 +++++++++++++++++++++++++++-
1 files changed, 27 insertions(+), 1 deletions(-)
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 6c7b902..ab5eb1e 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -200,7 +200,7 @@ test_expect_success 'update --init' '
mv init init2 &&
git config -f .gitmodules submodule.example.url "$(pwd)/init2" &&
- git config --remove-section submodule.example
+ git config --remove-section submodule.example &&
git submodule update init > update.out &&
grep "not initialized" update.out &&
test ! -d init/.git &&
@@ -209,4 +209,30 @@ test_expect_success 'update --init' '
'
+test_expect_success 'submodule add' '
+
+ git submodule add "$(pwd)/init2" init-added &&
+ test -d init-added/.git &&
+ [ "$(git config -f .gitmodules submodule.init-added.url)" = "$(pwd)/init2" ] &&
+ [ "$(git config -f .gitmodules submodule.init-added.path)" = "init-added" ]
+
+'
+
+test_expect_success 'submodule add -b' '
+
+ (
+ cd init2 &&
+ git checkout -b branch &&
+ echo t >s &&
+ git add s &&
+ git commit -m "change branch" &&
+ git checkout master
+ ) &&
+ git submodule add -b branch -- "$(pwd)/init2" init-added-b &&
+ test -d init-added-b/.git &&
+ [ "$(git config -f .gitmodules submodule.init-added-b.url)" = "$(pwd)/init2" ] &&
+ [ "$(cd init2 && git rev-parse branch)" = "$(cd init-added-b && git rev-parse HEAD)" ]
+
+'
+
test_done
^ permalink raw reply related
* [PATCH 3/7] git submodule add: Fix naming clash handling
From: Petr Baudis @ 2008-07-16 19:11 UTC (permalink / raw)
To: git
In-Reply-To: <20080716190753.19772.93357.stgit@localhost>
This patch fixes git submodule add behaviour when we add submodule
living at a same path as logical name of existing submodule. This
can happen e.g. in case the user git mv's the previous submodule away
and then git submodule add's another under the same name.
A test-case is obviously included.
This is not completely satisfactory since .git/config cross-commit
conflicts can still occur. A question is whether this is worth
handling, maybe it would be worth adding some kind of randomization
of the autogenerated submodule name, e.g. appending $$ or a timestamp.
Signed-off-by: Petr Baudis <pasky@suse.cz>
---
git-submodule.sh | 15 ++++++++++++---
t/t7400-submodule-basic.sh | 11 +++++++++++
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 9228f56..a93547b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -192,10 +192,19 @@ cmd_add()
git add "$path" ||
die "Failed to add submodule '$path'"
- git config -f .gitmodules submodule."$path".path "$path" &&
- git config -f .gitmodules submodule."$path".url "$repo" &&
+ name="$path"
+ if git config -f .gitmodules submodule."$name".path; then
+ name="$path~"; i=1;
+ while git config -f .gitmodules submodule."$name".path; do
+ name="$path~$i"
+ i=$((i+1))
+ done
+ fi
+
+ git config -f .gitmodules submodule."$name".path "$path" &&
+ git config -f .gitmodules submodule."$name".url "$repo" &&
git add .gitmodules ||
- die "Failed to register submodule '$path'"
+ die "Failed to register submodule '$path' (name '$name')"
}
#
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index ab5eb1e..092dffc 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -235,4 +235,15 @@ test_expect_success 'submodule add -b' '
'
+test_expect_success 'submodule add auto-naming clash' '
+
+ git submodule add "$(pwd)/init2/.git" example &&
+ test -d example/.git &&
+ [ "$(git config -f .gitmodules submodule.example.url)" = "$(pwd)/init2" ] &&
+ [ "$(git config -f .gitmodules submodule.example.path)" = "init" ]
+ [ "$(git config -f .gitmodules submodule.example~.url)" = "$(pwd)/init2/.git" ] &&
+ [ "$(git config -f .gitmodules submodule.example~.path)" = "example" ]
+
+'
+
test_done
^ permalink raw reply related
* [PATCH 5/7] git mv: Support moving submodules
From: Petr Baudis @ 2008-07-16 19:11 UTC (permalink / raw)
To: git
In-Reply-To: <20080716190753.19772.93357.stgit@localhost>
This patch adds support for moving submodules to 'git mv', including
rewriting of the .gitmodules file to reflect the movement.
The usage of struct path_list here is a bit abusive, but keeps the
code simple and hopefully still reasonably readable. The horrid
index_path_src_sha1 hack is unfortunately much worse, however the author
is currently unaware of any more reasonable solution of the problem.
Signed-off-by: Petr Baudis <pasky@suse.cz>
---
builtin-mv.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++------
cache.h | 2 ++
sha1_file.c | 10 ++++++++++
3 files changed, 61 insertions(+), 6 deletions(-)
diff --git a/builtin-mv.c b/builtin-mv.c
index 158a83d..30c4e7d 100644
--- a/builtin-mv.c
+++ b/builtin-mv.c
@@ -9,6 +9,7 @@
#include "cache-tree.h"
#include "path-list.h"
#include "parse-options.h"
+#include "submodule.h"
static const char * const builtin_mv_usage[] = {
"git-mv [options] <source>... <destination>",
@@ -60,6 +61,24 @@ static const char *add_slash(const char *path)
return path;
}
+static int ce_is_gitlink(int i)
+{
+ return i < 0 ? 0 : S_ISGITLINK(active_cache[i]->ce_mode);
+}
+
+static void rename_submodule(struct path_list_item *i)
+{
+ char *key = submodule_by_path(i->path);
+
+ config_exclusive_filename = ".gitmodules";
+ if (git_config_set(key, (const char *) i->util))
+ die("cannot update .gitmodules");
+ config_exclusive_filename = NULL;
+
+ free(key);
+}
+
+
static struct lock_file lock_file;
int cmd_mv(int argc, const char **argv, const char *prefix)
@@ -77,9 +96,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
struct stat st;
struct path_list overwritten = {NULL, 0, 0, 0};
struct path_list src_for_dst = {NULL, 0, 0, 0};
+ /* .util contains sha1 for submodules */
struct path_list added = {NULL, 0, 0, 0};
struct path_list deleted = {NULL, 0, 0, 0};
struct path_list changed = {NULL, 0, 0, 0};
+ /* .path is source path, .util is destination path */
+ struct path_list submodules = {NULL, 0, 0, 0};
git_config(git_default_config, NULL);
@@ -99,7 +121,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
/* special case: "." was normalized to "" */
destination = copy_pathspec(dest_path[0], argv, argc, 1);
else if (!lstat(dest_path[0], &st) &&
- S_ISDIR(st.st_mode)) {
+ S_ISDIR(st.st_mode) &&
+ !ce_is_gitlink(cache_name_pos(dest_path[0], strlen(dest_path[0])))) {
dest_path[0] = add_slash(dest_path[0]);
destination = copy_pathspec(dest_path[0], argv, argc, 1);
} else {
@@ -111,7 +134,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
/* Checking */
for (i = 0; i < argc; i++) {
const char *src = source[i], *dst = destination[i];
- int length, src_is_dir;
+ int length, src_is_dir, src_cache_pos;
const char *bad = NULL;
if (show_only)
@@ -126,7 +149,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
} else if ((src_is_dir = S_ISDIR(st.st_mode))
&& lstat(dst, &st) == 0)
bad = "cannot move directory over file";
- else if (src_is_dir) {
+ else if ((src_cache_pos = cache_name_pos(src, length)) < 0 && src_is_dir) {
const char *src_w_slash = add_slash(src);
int len_w_slash = length + 1;
int first, last;
@@ -193,7 +216,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
} else
bad = "Cannot overwrite";
}
- } else if (cache_name_pos(src, length) < 0)
+ } else if (src_cache_pos < 0)
bad = "not under version control";
else if (path_list_has_path(&src_for_dst, dst))
bad = "multiple sources for the same target";
@@ -218,6 +241,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
for (i = 0; i < argc; i++) {
const char *src = source[i], *dst = destination[i];
enum update_mode mode = modes[i];
+ struct path_list_item *last = NULL;
+ int j;
if (show_only || verbose)
printf("Renaming %s to %s\n", src, dst);
if (!show_only && mode != INDEX &&
@@ -227,14 +252,24 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
if (mode == WORKING_DIRECTORY)
continue;
- assert(cache_name_pos(src, strlen(src)) >= 0);
+ j = cache_name_pos(src, strlen(src));
+ assert(j >= 0);
path_list_insert(src, &deleted);
/* destination can be a directory with 1 file inside */
if (path_list_has_path(&overwritten, dst))
path_list_insert(dst, &changed);
else
- path_list_insert(dst, &added);
+ last = path_list_insert(dst, &added);
+ if (ce_is_gitlink(j)) {
+ path_list_insert(src, &submodules)->util = (void *) dst;
+ /* We will note the original HEAD sha1; the submodule
+ * may not be checked out, in which case we would have
+ * no way to re-obtain it later when adding the new
+ * entry. */
+ assert(last);
+ last->util = active_cache[j]->sha1;
+ }
}
if (show_only) {
@@ -254,13 +289,21 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
for (i = 0; i < added.nr; i++) {
const char *path = added.items[i].path;
+ index_path_src_sha1 = added.items[i].util;
if (add_file_to_cache(path, verbose ? ADD_CACHE_VERBOSE : 0))
die("updating index entries failed");
+ index_path_src_sha1 = NULL;
}
for (i = 0; i < deleted.nr; i++)
remove_file_from_cache(deleted.items[i].path);
+ for (i = 0; i < submodules.nr; i++)
+ rename_submodule(&submodules.items[i]);
+
+ if (submodules.nr > 0 && add_file_to_cache(".gitmodules", 0))
+ die("cannot add new .gitmodules to the index");
+
if (active_cache_changed) {
if (write_cache(newfd, active_cache, active_nr) ||
commit_locked_index(&lock_file))
diff --git a/cache.h b/cache.h
index a779d92..998b5fb 100644
--- a/cache.h
+++ b/cache.h
@@ -387,6 +387,8 @@ extern int ce_same_name(struct cache_entry *a, struct cache_entry *b);
extern int ie_match_stat(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);
extern int ie_modified(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);
+extern unsigned char *index_path_src_sha1;
+
extern int ce_path_match(const struct cache_entry *ce, const char **pathspec);
extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, enum object_type type, const char *path);
extern int index_pipe(unsigned char *sha1, int fd, const char *type, int write_object);
diff --git a/sha1_file.c b/sha1_file.c
index e281c14..4da6048 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2416,12 +2416,22 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
return ret;
}
+unsigned char *index_path_src_sha1;
+
int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object)
{
int fd;
char *target;
size_t len;
+ if (index_path_src_sha1) {
+ /* SHA1 for this path was prepared by the caller specially;
+ * e.g. comes from original cache entry in some cases of
+ * a rename. */
+ memcpy(sha1, index_path_src_sha1, 20);
+ return 0;
+ }
+
switch (st->st_mode & S_IFMT) {
case S_IFREG:
fd = open(path, O_RDONLY);
^ permalink raw reply related
* [PATCH 4/7] submodule.*: Introduce simple C interface for submodule lookup by path
From: Petr Baudis @ 2008-07-16 19:11 UTC (permalink / raw)
To: git
In-Reply-To: <20080716190753.19772.93357.stgit@localhost>
The interface will be used for git-mv and git-rm submodule support.
So far, only the submodule_by_path() function is defined, however more
can be probably expected in the future if/when the git-submodule command
is ported from shell.
Signed-off-by: Petr Baudis <pasky@suse.cz>
---
Makefile | 2 ++
submodule.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
submodule.h | 8 ++++++++
3 files changed, 60 insertions(+), 0 deletions(-)
create mode 100644 submodule.c
create mode 100644 submodule.h
diff --git a/Makefile b/Makefile
index 9b52071..742b5cb 100644
--- a/Makefile
+++ b/Makefile
@@ -368,6 +368,7 @@ LIB_H += run-command.h
LIB_H += sha1-lookup.h
LIB_H += sideband.h
LIB_H += strbuf.h
+LIB_H += submodule.h
LIB_H += tag.h
LIB_H += transport.h
LIB_H += tree.h
@@ -458,6 +459,7 @@ LIB_OBJS += sha1_name.o
LIB_OBJS += shallow.o
LIB_OBJS += sideband.o
LIB_OBJS += strbuf.o
+LIB_OBJS += submodule.o
LIB_OBJS += symlinks.o
LIB_OBJS += tag.o
LIB_OBJS += trace.o
diff --git a/submodule.c b/submodule.c
new file mode 100644
index 0000000..2883ae6
--- /dev/null
+++ b/submodule.c
@@ -0,0 +1,50 @@
+#include "cache.h"
+#include "submodule.h"
+
+
+struct gitmodules_info {
+ const char *path;
+ char *key;
+};
+
+static int gitmodules_worker(const char *key, const char *value, void *info_)
+{
+ struct gitmodules_info *info = info_;
+ const char *subkey;
+
+ if (prefixcmp(key, "submodule."))
+ return 0;
+
+ subkey = strrchr(key, '.');
+ if (!subkey)
+ return 0;
+
+ if (strcmp(subkey, ".path"))
+ return 0;
+
+ if (strcmp(value, info->path))
+ return 0;
+
+ /* Found the key to change. */
+ if (info->key) {
+ error("multiple submodules live at path `%s'", info->path);
+ /* The last one is supposed to win. */
+ free(info->key);
+ }
+ info->key = xstrdup(key);
+ return 0;
+}
+
+char *submodule_by_path(const char *path)
+{
+ struct gitmodules_info info = { path, NULL };
+
+ config_exclusive_filename = ".gitmodules";
+ if (git_config(gitmodules_worker, &info))
+ die("cannot process .gitmodules");
+ if (!info.key)
+ die("the submodule of `%s' not found in .gitmodules", path);
+ config_exclusive_filename = NULL;
+
+ return info.key;
+}
diff --git a/submodule.h b/submodule.h
new file mode 100644
index 0000000..bc74fa0
--- /dev/null
+++ b/submodule.h
@@ -0,0 +1,8 @@
+#ifndef SUBMODULE_H
+#define SUBMODULE_H
+
+/* Find submodule living at given path in .gitmodules and return the key
+ * of its path config variable (dynamically allocated). */
+extern char *submodule_by_path(const char *path);
+
+#endif
^ permalink raw reply related
* [PATCH 6/7] git rm: Support for removing submodules
From: Petr Baudis @ 2008-07-16 19:11 UTC (permalink / raw)
To: git
In-Reply-To: <20080716190753.19772.93357.stgit@localhost>
This patch adds support for removing submodules to 'git rm', including
removing the appropriate sections from the .gitmodules file to reflect this
Signed-off-by: Petr Baudis <pasky@suse.cz>
---
Documentation/git-rm.txt | 6 +++-
builtin-rm.c | 65 ++++++++++++++++++++++++++++++++++++++--------
2 files changed, 58 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index 4d0c495..bfc3dfa 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -20,7 +20,8 @@ and no updates to their contents can be staged in the index,
though that default behavior can be overridden with the `-f` option.
When '--cached' is given, the staged content has to
match either the tip of the branch or the file on disk,
-allowing the file to be removed from just the index.
+allowing the file to be removed from just the index;
+this is always the case when removing submodules.
OPTIONS
@@ -56,7 +57,8 @@ OPTIONS
--cached::
Use this option to unstage and remove paths only from the index.
Working tree files, whether modified or not, will be
- left alone.
+ left alone. Note that this is always assumed when removing
+ a checked-out submodule.
--ignore-unmatch::
Exit with a zero status even if no files matched.
diff --git a/builtin-rm.c b/builtin-rm.c
index 22c9bd1..363d1fa 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -9,6 +9,7 @@
#include "cache-tree.h"
#include "tree-walk.h"
#include "parse-options.h"
+#include "submodule.h"
static const char * const builtin_rm_usage[] = {
"git-rm [options] [--] <file>...",
@@ -17,16 +18,21 @@ static const char * const builtin_rm_usage[] = {
static struct {
int nr, alloc;
- const char **name;
+ struct {
+ const char *name;
+ int is_gitlink;
+ } *info;
} list;
-static void add_list(const char *name)
+static void add_list(const char *name, int is_gitlink)
{
if (list.nr >= list.alloc) {
list.alloc = alloc_nr(list.alloc);
- list.name = xrealloc(list.name, list.alloc * sizeof(const char *));
+ list.info = xrealloc(list.info, list.alloc * sizeof(*list.info));
}
- list.name[list.nr++] = name;
+ list.info[list.nr].name = name;
+ list.info[list.nr].is_gitlink = is_gitlink;
+ list.nr++;
}
static int remove_file(const char *name)
@@ -38,6 +44,13 @@ static int remove_file(const char *name)
if (ret && errno == ENOENT)
/* The user has removed it from the filesystem by hand */
ret = errno = 0;
+ if (ret && errno == EISDIR) {
+ /* This is a gitlink entry; try to remove at least the
+ * directory if the submodule is not checked out; we always
+ * leave the checked out ones as they are */
+ if (!rmdir(name) || errno == ENOTEMPTY)
+ ret = errno = 0;
+ }
if (!ret && (slash = strrchr(name, '/'))) {
char *n = xstrdup(name);
@@ -65,7 +78,7 @@ static int check_local_mod(unsigned char *head, int index_only)
struct stat st;
int pos;
struct cache_entry *ce;
- const char *name = list.name[i];
+ const char *name = list.info[i].name;
unsigned char sha1[20];
unsigned mode;
int local_changes = 0;
@@ -83,7 +96,7 @@ static int check_local_mod(unsigned char *head, int index_only)
/* It already vanished from the working tree */
continue;
}
- else if (S_ISDIR(st.st_mode)) {
+ else if (S_ISDIR(st.st_mode) && !S_ISGITLINK(ce->ce_mode)) {
/* if a file was removed and it is now a
* directory, that is the same as ENOENT as
* far as git is concerned; we do not track
@@ -122,6 +135,22 @@ static int check_local_mod(unsigned char *head, int index_only)
return errs;
}
+static void remove_submodule(const char *name)
+{
+ char *key = submodule_by_path(name);
+ char *sectend = strrchr(key, '.');
+
+ assert(sectend);
+ *sectend = 0;
+
+ config_exclusive_filename = ".gitmodules";
+ if (git_config_rename_section(key, NULL) <= 0)
+ die("cannot remove section `%s' from .gitmodules", key);
+ config_exclusive_filename = NULL;
+
+ free(key);
+}
+
static struct lock_file lock_file;
static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
@@ -140,7 +169,7 @@ static struct option builtin_rm_options[] = {
int cmd_rm(int argc, const char **argv, const char *prefix)
{
- int i, newfd;
+ int i, newfd, subs;
const char **pathspec;
char *seen;
@@ -168,7 +197,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
struct cache_entry *ce = active_cache[i];
if (!match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, seen))
continue;
- add_list(ce->name);
+ add_list(ce->name, S_ISGITLINK(ce->ce_mode));
}
if (pathspec) {
@@ -216,9 +245,11 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
* the index unless all of them succeed.
*/
for (i = 0; i < list.nr; i++) {
- const char *path = list.name[i];
+ const char *path = list.info[i].name;
if (!quiet)
- printf("rm '%s'\n", path);
+ printf("rm%s '%s'\n",
+ list.info[i].is_gitlink ? "dir" : "",
+ path);
if (remove_file_from_cache(path))
die("git-rm: unable to remove %s", path);
@@ -238,7 +269,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
if (!index_only) {
int removed = 0;
for (i = 0; i < list.nr; i++) {
- const char *path = list.name[i];
+ const char *path = list.info[i].name;
if (!remove_file(path)) {
removed = 1;
continue;
@@ -248,6 +279,18 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
}
}
+ /*
+ * Get rid of stale submodule setup.
+ */
+ subs = 0;
+ for (i = 0; i < list.nr; i++)
+ if (list.info[i].is_gitlink) {
+ remove_submodule(list.info[i].name);
+ subs++;
+ }
+ if (subs && add_file_to_cache(".gitmodules", 0))
+ die("cannot add new .gitmodules to the index");
+
if (active_cache_changed) {
if (write_cache(newfd, active_cache, active_nr) ||
commit_locked_index(&lock_file))
^ permalink raw reply related
* [PATCH 7/7] t7403: Submodule git mv, git rm testsuite
From: Petr Baudis @ 2008-07-16 19:11 UTC (permalink / raw)
To: git
In-Reply-To: <20080716190753.19772.93357.stgit@localhost>
The testsuite for newly added submodule support in git mv, git rm.
Signed-off-by: Petr Baudis <pasky@suse.cz>
---
t/t7403-submodule-mvrm.sh | 242 +++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 242 insertions(+), 0 deletions(-)
create mode 100755 t/t7403-submodule-mvrm.sh
diff --git a/t/t7403-submodule-mvrm.sh b/t/t7403-submodule-mvrm.sh
new file mode 100755
index 0000000..9b50d6a
--- /dev/null
+++ b/t/t7403-submodule-mvrm.sh
@@ -0,0 +1,242 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Johannes Schindelin
+#
+
+test_description='Test submodules support in git mv and git rm'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+ (mkdir sub-repo &&
+ cd sub-repo &&
+ git init &&
+ echo file > file &&
+ git add file &&
+ git commit -m "sub initial") &&
+ (cp -r sub-repo sub2-repo &&
+ cd sub2-repo &&
+ echo file2 > file &&
+ git add file &&
+ git commit -m "sub commit2") &&
+ git submodule add "$(pwd)/sub-repo" sub &&
+ git submodule add "$(pwd)/sub2-repo" sub2 &&
+ git commit -m initial &&
+ test "$(git config -f .gitmodules submodule.sub.path)" = "sub" &&
+ test "$(git config -f .gitmodules submodule.sub2.path)" = "sub2"
+
+'
+
+test_expect_success 'git mv of a submodule' '
+
+ git mv sub sub.moved &&
+ ! test -d sub &&
+ test -d sub.moved/.git &&
+ ! git ls-files --error-unmatch sub &&
+ test "$(git ls-files --stage --error-unmatch sub.moved | cut -d " " -f 1)" = 160000 &&
+ test "$(git config -f .gitmodules submodule.sub.path)" = "sub.moved" &&
+ ! git config -f .gitmodules submodule.sub.moved.path
+
+'
+
+test_expect_success 'git submodule add vs. git mv' '
+
+ ! git submodule add "$(pwd)/sub2-repo" sub.moved &&
+ git submodule add "$(pwd)/sub2-repo" sub &&
+ test -d sub/.git &&
+ test "$(git config -f .gitmodules submodule.sub.url)" = "$(pwd)/sub-repo" &&
+ test "$(git config -f .gitmodules submodule.sub.path)" = "sub.moved" &&
+ test "$(git config -f .gitmodules submodule.sub~.path)" = "sub"
+
+'
+
+test_expect_success 'git mv onto existing file' '
+
+ echo file > file &&
+ git add file &&
+ ! git mv sub.moved file &&
+ test -d sub.moved &&
+ ! test -d file/.git &&
+ test "$(git ls-files --stage --error-unmatch file | cut -d " " -f 1)" = 100644 &&
+ test "$(git ls-files --stage --error-unmatch sub.moved | cut -d " " -f 1)" = 160000 &&
+ test "$(git config -f .gitmodules submodule.sub.path)" = "sub.moved"
+
+'
+
+test_expect_success 'git mv onto existing directory' '
+
+ mkdir -p dir &&
+ echo file > dir/file &&
+ git add dir/file &&
+ git mv sub.moved dir &&
+ ! test -d sub.moved &&
+ test -d dir/sub.moved/.git &&
+ ! git ls-files --error-unmatch sub.moved &&
+ test "$(git ls-files --stage --error-unmatch dir/sub.moved | cut -d " " -f 1)" = 160000 &&
+ test "$(git config -f .gitmodules submodule.sub.path)" = "dir/sub.moved" &&
+ git mv dir/sub.moved . &&
+ test "$(git config -f .gitmodules submodule.sub.path)" = "sub.moved"
+
+'
+
+test_expect_success 'git mv onto existing submodule' '
+
+ ! git mv sub.moved sub2 &&
+ test -d sub.moved/.git &&
+ ! test -d sub2/sub.moved &&
+ test "$(git ls-files --stage --error-unmatch sub2 | cut -d " " -f 1)" = 160000 &&
+ test "$(git ls-files --stage --error-unmatch sub.moved | cut -d " " -f 1)" = 160000 &&
+ test "$(git config -f .gitmodules submodule.sub.path)" = "sub.moved"
+
+'
+
+test_expect_success 'git mv of multiple submodules' '
+
+ mkdir -p dir &&
+ git mv sub.moved sub dir &&
+ ! test -d sub.moved &&
+ ! test -d sub &&
+ test -d dir/sub.moved/.git &&
+ test -d dir/sub/.git &&
+ ! git ls-files --error-unmatch sub.moved sub &&
+ test "$(git ls-files --stage --error-unmatch dir/sub.moved dir/sub | cut -d " " -f 1 | uniq)" = 160000 &&
+ ! git config -f .gitmodules submodule.dir.path &&
+ test "$(git config -f .gitmodules submodule.sub.path)" = "dir/sub.moved" &&
+ test "$(git config -f .gitmodules submodule.sub~.path)" = "dir/sub"
+
+'
+
+test_expect_success 'git mv of multiple submodules back from a subdir' '
+
+ (cd dir && git mv sub.moved sub .. && cd ..) &&
+ test -d sub.moved &&
+ test -d sub &&
+ ! test -d dir/sub.moved/.git &&
+ ! test -d dir/sub/.git &&
+ ! git ls-files --error-unmatch dir/sub.moved dir/sub &&
+ test "$(git ls-files --stage --error-unmatch sub.moved sub | cut -d " " -f 1 | uniq)" = 160000 &&
+ test "$(git config -f .gitmodules submodule.sub.path)" = "sub.moved" &&
+ test "$(git config -f .gitmodules submodule.sub~.path)" = "sub"
+
+'
+
+test_expect_success 'git mv of non-checked-out submodules' '
+
+ git clone . clone &&
+ (cd clone &&
+ test -d sub &&
+ test -d sub2 &&
+ ! test -d sub/.git &&
+ ! test -d sub2/.git &&
+ git ls-files --stage --error-unmatch sub sub2 > ls-files.out &&
+ mkdir -p dir &&
+ git mv sub sub2 dir &&
+ ! test -d sub &&
+ ! test -d sub2 &&
+ test -d dir/sub &&
+ test -d dir/sub2 &&
+ ! git ls-files --error-unmatch sub sub2 &&
+ test "$(git ls-files --stage --error-unmatch dir/sub dir/sub2 | cut -d " " -f 1 | uniq)" = 160000 &&
+ git ls-files --stage --error-unmatch dir/sub dir/sub2 | sed "s#dir/##g" | diff - ls-files.out &&
+ test "$(git config -f .gitmodules submodule.sub.path)" = "dir/sub" &&
+ test "$(git config -f .gitmodules submodule.sub2.path)" = "dir/sub2" &&
+ (cd dir && git mv sub2 .. && cd ..) &&
+ test -d sub2 &&
+ ! test -d dir/sub2 &&
+ ! git ls-files --error-unmatch dir/sub2 &&
+ test "$(git ls-files --stage --error-unmatch sub2 | cut -d " " -f 1)" = 160000 &&
+ test "$(git config -f .gitmodules submodule.sub2.path)" = "sub2")
+
+'
+
+test_expect_success 'checkpointing state with git commit' '
+
+ git commit -m"checkpoint" -a &&
+ (cd clone && git commit -m"clone checkpoint" -a)
+
+'
+
+test_expect_success 'git rm of a regular submodule' '
+
+ git rm sub2 &&
+ test -d sub2/.git &&
+ ! git ls-files --error-unmatch sub2 &&
+ ! git config -f .gitmodules submodule.sub2.path &&
+ ! git config -f .gitmodules submodule.sub2.url
+
+'
+
+test_expect_success 'git rm of a submodule with name different from path' '
+
+ git rm sub.moved &&
+ test -d sub.moved/.git &&
+ ! git ls-files --error-unmatch sub.moved &&
+ ! git config -f .gitmodules submodule.sub.path &&
+ ! git config -f .gitmodules submodule.sub.url
+
+'
+
+test_expect_success 'git rm of a modified submodule' '
+
+ git mv sub dir/sub && # more fun with richer path
+ (cd dir/sub &&
+ echo mod > file &&
+ git commit -m "sub mod" file) &&
+ git add dir/sub &&
+ ! git rm dir/sub &&
+ test -d dir/sub/.git &&
+ test "$(git ls-files --stage --error-unmatch dir/sub | cut -d " " -f 1)" = "160000" &&
+ git config -f .gitmodules submodule.sub~.path &&
+ git config -f .gitmodules submodule.sub~.url &&
+ git rm -f dir/sub &&
+ test -d dir/sub/.git &&
+ ! git ls-files --error-unmatch dir/sub &&
+ ! git config -f .gitmodules submodule.sub~.path &&
+ ! git config -f .gitmodules submodule.sub~.url
+
+'
+
+test_expect_success 'git rm of a submodule from within a subdirectory' '
+
+ git submodule add "$(pwd)/sub-repo" sub-torm &&
+ mkdir -p dir &&
+ # -f since we did not commit the submodule
+ (cd dir && git rm -f ../sub-torm && cd ..) &&
+ test -d sub-torm/.git &&
+ ! git ls-files --error-unmatch sub-torm &&
+ ! git config -f .gitmodules submodule.sub-torm.path &&
+ ! git config -f .gitmodules submodule.sub-torm.url
+
+'
+
+test_expect_success 'git rm of a non-checked-out submodule' '
+
+ (cd clone &&
+ test -d dir/sub &&
+ ! test -d dir/sub/.git &&
+ git rm dir/sub &&
+ ! test -d dir/sub &&
+ ! git ls-files --error-unmatch dir/sub &&
+ ! git config -f .gitmodules submodule.sub.path &&
+ ! git config -f .gitmodules submodule.sub.url)
+
+'
+
+test_expect_success 'git rm of a non-checked-out submodule w/ different working tree' '
+
+ (cd clone &&
+ rmdir sub2 &&
+ echo cunning > sub2 &&
+ ! git rm sub2 &&
+ test -f sub2 &&
+ test "$(git ls-files --stage --error-unmatch sub2 | cut -d " " -f 1)" = "160000" &&
+ git rm -f sub2 &&
+ ! test -e sub2 &&
+ ! git ls-files --error-unmatch sub2 &&
+ ! git config -f .gitmodules submodule.sub2.path &&
+ ! git config -f .gitmodules submodule.sub2.url)
+
+'
+
+test_done
^ permalink raw reply related
* Re: [PATCHv2] Documentation/git-submodule.txt: Add Description section
From: Kalle Olavi Niemitalo @ 2008-07-16 19:15 UTC (permalink / raw)
To: git
In-Reply-To: <20080716184248.6524.38463.stgit@localhost>
Petr Baudis <pasky@suse.cz> writes:
> +Submodules are a special kind of tree entries which refer to a particular tree
> +in another repository (living at a given URL). The tree entry describes
> +the existence of a submodule with the given name and the exact revision that
> +should be used, while the location of the repository is described in the
> +`/.gitmodules` file.
I was surprised to learn that a commit ID in a tree does not
prevent Git from pruning the corresponding commit object if
one happens to exist in the same repository. That might be
best documented under "Tree Object" in user-manual.txt though,
rather than in git-submodule.txt.
^ permalink raw reply
* Re: [PATCH] Rename ".dotest/" to ".git/rebase" and ".dotest-merge" to "rebase-merge"
From: Junio C Hamano @ 2008-07-16 19:18 UTC (permalink / raw)
To: Theodore Tso, Nanako Shiraishi
Cc: Johannes Schindelin, René Scharfe, gitster, Stephan Beyer,
Joe Fiorini, git, Jari Aalto
In-Reply-To: <20080716012619.GM8185@mit.edu>
Theodore Tso <tytso@mit.edu> writes:
> While you have "git am" open, how about adding an "git am --abort"
> which nukes the .dotest aka .git/rebase directory, and resets HEAD
> back to the original position?
This does not seem to have reached the list nor its archives. I cannot
say I have really looked at it deeply but it may be a good starting
point. It needs docs ;-)
-- >8 --
From: Nanako Shiraishi <nanako3@lavabit.com>
Date: Wed, 16 Jul 2008 19:39:10 +0900
Subject: [PATCH] git am --abort
After failing to apply patches in the middle of a series, "git am --abort"
lets you go back to the original commit.
Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
---
git-am.sh | 19 +++++++++++++++----
t/t4151-am-abort.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 63 insertions(+), 4 deletions(-)
create mode 100755 t/t4151-am-abort.sh
diff --git a/git-am.sh b/git-am.sh
index cc8787b..a44bd7a 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -22,6 +22,7 @@ p= pass it through git-apply
resolvemsg= override error message when patch failure occurs
r,resolved to be used after a patch failure
skip skip the current patch
+abort abandon patch application and clear .dotest directory
rebasing (internal use for git-rebase)"
. git-sh-setup
@@ -120,7 +121,7 @@ It does not apply to blobs recorded in its index."
prec=4
dotest="$GIT_DIR/rebase"
-sign= utf8=t keep= skip= interactive= resolved= binary= rebasing=
+sign= utf8=t keep= skip= interactive= resolved= binary= rebasing= abort=
resolvemsg= resume=
git_apply_opt=
@@ -145,6 +146,8 @@ do
resolved=t ;;
--skip)
skip=t ;;
+ --abort)
+ abort=t ;;
--rebasing)
rebasing=t threeway=t keep=t binary=t ;;
-d|--dotest)
@@ -177,7 +180,7 @@ fi
if test -d "$dotest"
then
- case "$#,$skip$resolved" in
+ case "$#,$skip$resolved$abort" in
0,*t*)
# Explicit resume command and we do not have file, so
# we are happy.
@@ -197,9 +200,17 @@ then
esac ||
die "previous rebase directory $dotest still exists but mbox given."
resume=yes
+
+ case "$abort" in
+ t)
+ rm -fr "$dotest" &&
+ git read-tree -m -u ORIG_HEAD &&
+ git reset ORIG_HEAD && :
+ exit ;;
+ esac
else
- # Make sure we are not given --skip nor --resolved
- test ",$skip,$resolved," = ,,, ||
+ # Make sure we are not given --skip, --resolved, nor --abort
+ test "$skip$resolved$abort" = "" ||
die "Resolve operation not in progress, we are not resuming."
# Start afresh.
diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh
new file mode 100755
index 0000000..96b2cd5
--- /dev/null
+++ b/t/t4151-am-abort.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+test_description='am --abort'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ for i in a b c d e f g
+ do
+ echo $i
+ done >file-1 &&
+ cp file-1 file-2 &&
+ test_tick &&
+ git add file-1 file-2 &&
+ git commit -m initial &&
+ git tag initial &&
+ for i in 2 3 4 5
+ do
+ echo $i >>file-1 &&
+ test_tick &&
+ git commit -a -m $i || break
+ done &&
+ git format-patch initial &&
+ git checkout -b side initial &&
+ echo local change >file-2 &&
+ cp file-2 file-2-expect
+'
+
+test_expect_success 'am stops at a patch that does not apply' '
+ test_must_fail git am 000[124]-*.patch &&
+ git log --pretty=tformat:%s >actual &&
+ for i in 3 2 initial
+ do
+ echo $i
+ done >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'am --abort goes back' '
+ git am --abort &&
+ git rev-parse HEAD >actual &&
+ git rev-parse initial >expect &&
+ test_cmp expect actual &&
+ test_cmp file-2-expect file-2 &&
+ git diff-index --exit-code --cached HEAD
+'
+
+test_done
--
1.5.6
^ permalink raw reply related
* Re: Considering teaching plumbing to users harmful
From: Avery Pennarun @ 2008-07-16 19:22 UTC (permalink / raw)
To: Petr Baudis; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <20080716185930.GP32184@machine.or.cz>
On 7/16/08, Petr Baudis <pasky@suse.cz> wrote:
> On Wed, Jul 16, 2008 at 02:51:30PM -0400, Avery Pennarun wrote:
> > On 7/16/08, Junio C Hamano <gitster@pobox.com> wrote:
> > > You can skip merges with "git log --no-merges", just in case you didn't
> > > know.
> > Perhaps this is mostly a user education or documentation issue. I
> > know about --no-merges, but it's unclear that this is really a safe
> > thing to use, particularly if some of your merges have conflicts.
> > Leaving them out leaves out an important part of history. Do you use
> > this option yourself?
>
> Whereas if you rebase, not only you don't show the conflicts resolution,
> you didn't even _store_ it in the first place. That isn't much of an
> improvement. :-) (This is the main reason why I prefer to avoid rebase
> unless absolutely necessary for the workflow.)
The key here is that I'd expect "git log -p" with a rebased merge at
least shows me the actual changes that are in my repository. "git log
--no-merges" will actually omit things.
Avery
^ permalink raw reply
* [PATCH] Reformat "your branch has diverged..." lines to reduce line length.
From: Avery Pennarun @ 2008-07-16 19:19 UTC (permalink / raw)
To: gitster, git; +Cc: Avery Pennarun
The message length depends on the length of the branch name. In my case,
the branch name "origin/add-chickens2" put the first line of the "your
branch has diverged" message over 80 characters, which triggered "less -FS"
to not exit automatically as expected.
This patch puts the newlines in slightly different places to reduce the
probability of this happening. Now you'd need a significantly longer
branch name to trigger the problem.
Signed-off-by: Avery Pennarun <apenwarr@gmail.com>
---
remote.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
I suppose a full-on automatic wordwrapper would be nicer :)
diff --git a/remote.c b/remote.c
index df8bd72..4f32032 100644
--- a/remote.c
+++ b/remote.c
@@ -1321,19 +1321,19 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
remote_msg = "";
}
if (!num_theirs)
- strbuf_addf(sb, "Your branch is ahead of the tracked%s branch '%s' "
+ strbuf_addf(sb, "Your branch is ahead of the tracked%s branch '%s'\n"
"by %d commit%s.\n",
remote_msg, base,
num_ours, (num_ours == 1) ? "" : "s");
else if (!num_ours)
- strbuf_addf(sb, "Your branch is behind the tracked%s branch '%s' "
- "by %d commit%s,\n"
+ strbuf_addf(sb, "Your branch is behind the tracked%s branch '%s'\n"
+ "by %d commit%s, "
"and can be fast-forwarded.\n",
remote_msg, base,
num_theirs, (num_theirs == 1) ? "" : "s");
else
- strbuf_addf(sb, "Your branch and the tracked%s branch '%s' "
- "have diverged,\nand respectively "
+ strbuf_addf(sb, "Your branch and the tracked%s branch '%s'\n"
+ "have diverged, and respectively "
"have %d and %d different commit(s) each.\n",
remote_msg, base,
num_ours, num_theirs);
--
1.5.6.3.385.g94745
^ permalink raw reply related
* Re: [PATCHv2] Documentation/git-submodule.txt: Add Description section
From: Junio C Hamano @ 2008-07-16 19:29 UTC (permalink / raw)
To: Petr Baudis; +Cc: git, Heikki Orsila
In-Reply-To: <20080716184248.6524.38463.stgit@localhost>
Petr Baudis <pasky@suse.cz> writes:
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 76702a0..87c4ece 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -16,6 +16,28 @@ SYNOPSIS
> 'git submodule' [--quiet] summary [--summary-limit <n>] [commit] [--] [<path>...]
>
>
> +DESCRIPTION
> +-----------
> +Submodules are a special kind of tree entries which refer to a particular tree
> +in another repository (living at a given URL). ...
In the documentation, "tree" has a specific meaning. Perhaps "a
particular tree state" is a better wording than another alternative "a
particular commit", because you mention "the exact revision" in the
following sentence.
I'd suggest dropping " (living at a given URL)" from here, though.
> ... The tree entry describes
> +the existence of a submodule with the given name and the exact revision that
> +should be used, while the location of the repository is described in the
> +`/.gitmodules` file.
Strictly speaking, ".gitmodules" merely gives a hint to be used by
"submodule init", the canonical location from which the repository is
expected to be cloned. I do not think this overview needs to go into such
a detail. The description of "init" subcommand might need clarification,
though.
> +When checked out, submodules will maintain their own independent repositories
> +within their directories; the only link between the submodule and the "parent
> +project" is the tree entry within the parent project mentioned above.
> +
> +This command will manage the tree entries and contents of the gitmodules file
> +for you, as well as inspecting the status of your submodules and updating them.
> +When adding a new submodule to the tree, the 'add' subcommand is to be used.
> +However, when pulling a tree containing submodules, these will not be checked
> +out by default; the 'init' and 'update' subcommands will maintain submodules
> +checked out and at appropriate revision in your working tree. You can inspect
> +the current status of your submodules using the 'submodule' subcommand and get
> +an overview of changes 'update' would perform using the 'summary' subcommand.
Otherwise this is a nice write-up. Will queue; further comments from
other submodule users are appreciated if there are any. Thanks.
^ permalink raw reply
* Re: Considering teaching plumbing to users harmful
From: Avery Pennarun @ 2008-07-16 19:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <7vmykhr6h1.fsf@gitster.siamese.dyndns.org>
On 7/16/08, Junio C Hamano <gitster@pobox.com> wrote:
> "Avery Pennarun" <apenwarr@gmail.com> writes:
> > svn avoids these excess merges by default, albeit because it puts your
> > working copy at risk every time you do "svn update".
>
> By default? As if it has other mode of operation.
>
> Of course if you do not allow any commits in between to make the history
> truly forked, you won't see merge commits. It is like saying that you
> like your broken keyboard whose SHIFT key does not work because you think
> capital letters look ugly and your keyboard protects you from typing them
> by accident.
>
> Is that an improvement?
>
> I won't waste my time further on the apples and rotten oranges comparison,
I find it interesting how git usability discussions tend to go. It
usually starts out by someone saying, "Look, git really isn't that
hard to learn, just do it like this..." and then someone says, "But
actually, that's still really complicated. Everyone thinks xxx other
VCS is easier to learn. Here's how they do it..." And then someone
says, "Yeah, but xxx VCS sucks!" and that somehow makes it okay that
git is empirically harder to learn than xxx VCS, as anyone can see by
browsing the web.
svn is fundamentally broken, but just because they did *some* things
wrong doesn't mean they did *everything* wrong. You can learn lessons
even from your inferiors.
Have fun,
Avery
^ permalink raw reply
* Re: Considering teaching plumbing to users harmful
From: Junio C Hamano @ 2008-07-16 19:34 UTC (permalink / raw)
To: Avery Pennarun; +Cc: Johannes Schindelin, git
In-Reply-To: <32541b130807161229ob4c21cbsc6c86ee3e42c4101@mail.gmail.com>
"Avery Pennarun" <apenwarr@gmail.com> writes:
> On 7/16/08, Junio C Hamano <gitster@pobox.com> wrote:
>> "Avery Pennarun" <apenwarr@gmail.com> writes:
>> > svn avoids these excess merges by default, albeit because it puts your
>> > working copy at risk every time you do "svn update".
>>
>> By default? As if it has other mode of operation.
>>
>> Of course if you do not allow any commits in between to make the history
>> truly forked, you won't see merge commits. It is like saying that you
>> like your broken keyboard whose SHIFT key does not work because you think
>> capital letters look ugly and your keyboard protects you from typing them
>> by accident.
>>
>> Is that an improvement?
>>
>> I won't waste my time further on the apples and rotten oranges comparison,
> ...
> svn is fundamentally broken, but just because they did *some* things
> wrong doesn't mean they did *everything* wrong. You can learn lessons
> even from your inferiors.
I agree in principle, but read what you wrote again and realize that your
criticism does not apply to this case *at all*.
You said svn makes it easier because it makes it very hard to do merges
and forces users to stay away from them. This results in user doing "svn
update" which is to resolve conflicts with large uncommitted changes but
keeps the history straight single-strand-of-pearls.
I am not saying the merge based workflow in git does not have any room to
improve. I am just saying that there is nothing we can learn from svn in
that area. "Solves it by not letting us to do merges" is not a solution.
^ permalink raw reply
* [StGit PATCH] Add some performance testing scripts
From: Karl Hasselström @ 2008-07-16 19:35 UTC (permalink / raw)
To: Catalin Marinas; +Cc: git
find_patchbomb.py: Given a git repo, finds the longest linear sequence
of commits. Useful for testing StGit on a real repository.
setup.sh: Creates two test repositories, one synthetic and one based
on the Linux kernel repo, with strategically placed tags.
create_synthetic_repo.py: Helper script for setup.sh; it produces
output that is to be fed to git fast-import.
perftest.py: Runs one of a (small) number of hard-coded performance
tests against a copy of one of the repos created by setup.sh. The
initial testcases all involve uncommitting a large number of patches
and then rebasing them.
Signed-off-by: Karl Hasselström <kha@treskal.com>
---
perf/.gitignore | 2 +
perf/create_synthetic_repo.py | 61 ++++++++++++++++++++++++++++
perf/find_patchbomb.py | 31 ++++++++++++++
perf/perftest.py | 89 +++++++++++++++++++++++++++++++++++++++++
perf/setup.sh | 52 ++++++++++++++++++++++++
5 files changed, 235 insertions(+), 0 deletions(-)
create mode 100644 perf/.gitignore
create mode 100644 perf/create_synthetic_repo.py
create mode 100644 perf/find_patchbomb.py
create mode 100644 perf/perftest.py
create mode 100644 perf/setup.sh
diff --git a/perf/.gitignore b/perf/.gitignore
new file mode 100644
index 0000000..dfae110
--- /dev/null
+++ b/perf/.gitignore
@@ -0,0 +1,2 @@
+/*.orig
+/*.trash
diff --git a/perf/create_synthetic_repo.py b/perf/create_synthetic_repo.py
new file mode 100644
index 0000000..4d6ef6b
--- /dev/null
+++ b/perf/create_synthetic_repo.py
@@ -0,0 +1,61 @@
+next_mark = 1
+def get_mark():
+ global next_mark
+ next_mark += 1
+ return (next_mark - 1)
+
+def write_data(s):
+ print 'data %d' % len(s)
+ print s
+
+def write_blob(s):
+ print 'blob'
+ m = get_mark()
+ print 'mark :%d' % m
+ write_data(s)
+ return m
+
+def write_commit(branch, files, msg, parent = None):
+ print 'commit %s' % branch
+ m = get_mark()
+ print 'mark :%d' % m
+ auth = 'X Ample <xa@example.com> %d +0000' % (1000000000 + m)
+ print 'author %s' % auth
+ print 'committer %s' % auth
+ write_data(msg)
+ if parent != None:
+ print 'from :%d' % parent
+ for fn, fm in sorted(files.iteritems()):
+ print 'M 100644 :%d %s' % (fm, fn)
+ return m
+
+def set_ref(ref, mark):
+ print 'reset %s' % ref
+ print 'from :%d' % mark
+
+def stdblob(fn):
+ return ''.join('%d %s\n' % (x, fn) for x in xrange(10))
+
+def iter_paths():
+ for i in xrange(32):
+ for j in xrange(32):
+ for k in xrange(32):
+ yield '%02d/%02d/%02d' % (i, j, k)
+
+def setup():
+ def t(name): return 'refs/tags/%s' % name
+ files = dict((fn, write_blob(stdblob(fn))) for fn in iter_paths())
+ initial = write_commit(t('bomb-base'), files, 'Initial commit')
+ set_ref(t('bomb-top'), initial)
+ for fn in iter_paths():
+ write_commit(t('bomb-top'),
+ { fn: write_blob(stdblob(fn) + 'Last line\n') },
+ 'Add last line to %s' % fn)
+ write_commit(t('add-file'), { 'woo-hoo.txt': write_blob('woo-hoo\n') },
+ 'Add a new file', parent = initial)
+ files = dict((fn, write_blob('First line\n' + stdblob(fn)))
+ for fn in iter_paths())
+ write_commit(t('modify-all'), files, 'Add first line to all files',
+ parent = initial)
+
+setup()
diff --git a/perf/find_patchbomb.py b/perf/find_patchbomb.py
new file mode 100644
index 0000000..69a78c7
--- /dev/null
+++ b/perf/find_patchbomb.py
@@ -0,0 +1,31 @@
+# Feed this with git rev-list HEAD --parents
+
+import sys
+
+parents = {}
+for line in sys.stdin.readlines():
+ commits = line.split()
+ parents[commits[0]] = commits[1:]
+
+sequence_num = {}
+stack = []
+for commit in parents.keys():
+ stack.append(commit)
+ while stack:
+ c = stack.pop()
+ if c in sequence_num:
+ continue
+ ps = parents[c]
+ if len(ps) == 1:
+ p = ps[0]
+ if p in sequence_num:
+ sequence_num[c] = 1 + sequence_num[p]
+ else:
+ stack.append(c)
+ stack.append(p)
+ else:
+ sequence_num[c] = 0
+
+(num, commit) = max((num, commit) for (commit, num)
+ in sequence_num.iteritems())
+print '%s is a sequence of %d patches' % (commit, num)
diff --git a/perf/perftest.py b/perf/perftest.py
new file mode 100644
index 0000000..2655869
--- /dev/null
+++ b/perf/perftest.py
@@ -0,0 +1,89 @@
+import datetime, subprocess, sys
+
+def duration(t1, t2):
+ d = t2 - t1
+ return 86400*d.days + d.seconds + 1e-6*d.microseconds
+
+class Run(object):
+ def __init__(self):
+ self.__cwd = None
+ self.__log = []
+ def __call__(self, *cmd, **args):
+ kwargs = { 'cwd': self.__cwd }
+ if args.get('capture_stdout', False):
+ kwargs['stdout'] = subprocess.PIPE
+ start = datetime.datetime.now()
+ p = subprocess.Popen(cmd, **kwargs)
+ (out, err) = p.communicate()
+ stop = datetime.datetime.now()
+ self.__log.append((cmd, duration(start, stop)))
+ return out
+ def cd(self, dir):
+ self.__cwd = dir
+ def summary(self):
+ def pcmd(c): return ' '.join(c)
+ def ptime(t): return '%.3f' % t
+ (cs, times) = zip(*self.__log)
+ ttime = sum(times)
+ cl = max(len(pcmd(c)) for c in cs)
+ tl = max(len(ptime(t)) for t in list(times) + [ttime])
+ for (c, t) in self.__log:
+ print '%*s %*s' % (tl, ptime(t), -cl, pcmd(c))
+ print '%*s' % (tl, ptime(ttime))
+
+perftests = {}
+perftestdesc = {}
+def perftest(desc, name = None):
+ def decorator(f):
+ def g():
+ r = Run()
+ f(r)
+ r.summary()
+ perftests[name or f.__name__] = g
+ perftestdesc[name or f.__name__] = desc
+ return g
+ return decorator
+
+def copy_testdir(dir):
+ dir = dir.rstrip('/')
+ tmp = dir + '.trash'
+ r = Run()
+ r('rsync', '-a', '--delete', dir + '/', tmp)
+ return tmp
+
+def new_rebase(r, ref):
+ top = r('stg', 'top', capture_stdout = True)
+ r('stg', 'pop', '-a')
+ r('git', 'reset', '--hard', ref)
+ r('stg', 'goto', top.strip())
+
+def old_rebase(r, ref):
+ r('stg', 'rebase', ref)
+
+def def_rebasetest(rebase, dir, tag):
+ @perftest('%s rebase onto %s in %s' % (rebase, tag, dir),
+ 'rebase-%srebase-%s-%s' % (rebase, tag, dir))
+ def rebasetest(r):
+ r.cd(copy_testdir(dir))
+ r('stg', 'init')
+ if dir == 'synt':
+ r('stg', 'uncommit', '-n', '500')
+ else:
+ r('stg', 'uncommit', '-x', '-t', 'bomb-base')
+ if rebase == 'new':
+ new_rebase(r, tag)
+ else:
+ old_rebase(r, tag)
+for rebase in ['old', 'new']:
+ for (dir, tag) in [('synt', 'add-file'),
+ ('synt', 'modify-all'),
+ ('linux', 'add-file')]:
+ def_rebasetest(rebase, dir, tag)
+
+args = sys.argv[1:]
+if len(args) == 0:
+ for (fun, desc) in sorted(perftestdesc.iteritems()):
+ print '%s: %s' % (fun, desc)
+else:
+ for test in args:
+ perftests[test]()
diff --git a/perf/setup.sh b/perf/setup.sh
new file mode 100644
index 0000000..b92ddfc
--- /dev/null
+++ b/perf/setup.sh
@@ -0,0 +1,52 @@
+krepo='git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git'
+
+get_linux() {
+ rm -rf linux.orig
+ git clone "$krepo" linux.orig
+}
+
+mod_linux() {
+ # Tag the top and base of a very long linear sequence of commits.
+ git tag bomb-top 85040bcb4643cba578839e953f25e2d1965d83d0
+ git tag bomb-base bomb-top~1470
+
+ # Add a file at the base of the linear sequence.
+ git checkout bomb-base
+ echo "woo-hoo" > woo-hoo.txt
+ git add woo-hoo.txt
+ git commit -m "Add a file"
+ git tag add-file
+
+ # Clean up and go to start position.
+ git gc
+ git update-ref refs/heads/master bomb-top
+ git checkout master
+}
+
+setup_linux () {
+ get_linux
+ ( cd linux.orig && mod_linux )
+}
+
+create_empty () {
+ dir="$1"
+ rm -rf $dir
+ mkdir $dir
+ ( cd $dir && git init )
+}
+
+fill_synthetic () {
+ python ../create_synthetic_repo.py | git fast-import
+ git gc --aggressive
+ git update-ref refs/heads/master bomb-top
+ git checkout master
+}
+
+setup_synthetic()
+{
+ create_empty synt.orig
+ ( cd synt.orig && fill_synthetic )
+}
+
+setup_linux
+setup_synthetic
^ permalink raw reply related
* Re: Considering teaching plumbing to users harmful
From: Avery Pennarun @ 2008-07-16 19:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <7vabghr5br.fsf@gitster.siamese.dyndns.org>
On 7/16/08, Junio C Hamano <gitster@pobox.com> wrote:
> You said svn makes it easier because it makes it very hard to do merges
> and forces users to stay away from them. This results in user doing "svn
> update" which is to resolve conflicts with large uncommitted changes but
> keeps the history straight single-strand-of-pearls.
>
> I am not saying the merge based workflow in git does not have any room to
> improve. I am just saying that there is nothing we can learn from svn in
> that area. "Solves it by not letting us to do merges" is not a solution.
What svn does is essentially an unsafe version of
git stash && git pull x y && git stash apply
And that's actually a good example of what I'm talking about; in svn,
that's just "svn up", which is a daily operation that's easy and
leaves a clean, linear history. In git, it takes three commands
instead of one (and 'git stash' wasn't anywhere in Dscho's list of
commands he teaches to newbies).
I think there's value in thinking about the relative convenience of
svn's workflow for novice users in their day-to-day lives. Now, in
the case of svn, that "convenience" also leads to novice users blowing
up the local changes in their working copy occasionally, but that's
just an svn *architectural* problem. Git doesn't have that
architectural problem.
To be more concrete: would anyone object to a patch that simply made
'git pull' include the above stash commands (or something like them)
by default, rather than giving up when dirty files would be changed?
Or can we do even better?
Have fun,
Avery
^ permalink raw reply
* Re: [PATCH] Reformat "your branch has diverged..." lines to reduce line length.
From: Junio C Hamano @ 2008-07-16 19:48 UTC (permalink / raw)
To: Avery Pennarun; +Cc: git
In-Reply-To: <1216235967-9510-1-git-send-email-apenwarr@gmail.com>
Avery Pennarun <apenwarr@gmail.com> writes:
> if (!num_theirs)
> - strbuf_addf(sb, "Your branch is ahead of the tracked%s branch '%s' "
> + strbuf_addf(sb, "Your branch is ahead of the tracked%s branch '%s'\n"
> "by %d commit%s.\n",
I wonder if a simple "s/of the tracked%s branch //" is better for this
kind of thing. If the message says 'origin/master', you know it is a
tracked remote branch anyway, don't you?
Too wide is bad, but too tall is worse. Some of us still work in 80x24
;-) and I prefer to make the message succinct when possible, rather than
keeping it long and spread over multiple lines.
> else
> - strbuf_addf(sb, "Your branch and the tracked%s branch '%s' "
> - "have diverged,\nand respectively "
> + strbuf_addf(sb, "Your branch and the tracked%s branch '%s'\n"
> + "have diverged, and respectively "
> "have %d and %d different commit(s) each.\n",
This does not make the message taller, but if we were to make the "only
one side advanced" cases shorter, we would need to reword this to be
consistent. Perhaps something like this would be just as easy to read and
more compact?
Your branch is ahead of 'origin/add-chickens2' by 21 commits.
Your branch is behind 'origin/add-chickens2' by 1 commit.
Your branch and 'origin/add-chickens2' have diverged, and have
21 and 1 different commit(s) each, respectively.
I moved "respectively" so that the variable parts will come close to the
beginning of physical line.
^ permalink raw reply
* [PATCH (GIT-GUI)] Fix pre-commit hooks under MinGW/MSYS
From: Alexander Gavrilov @ 2008-07-16 20:12 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
Apply the work-around for checking the executable
permission of hook files not only on Cygwin, but on
Windows in general.
Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
This is a fix for msysgit issue #118.
(http://code.google.com/p/msysgit/issues/detail?id=118)
I've already sent this patch, but resend it as I haven't received any reply,
and it is not in git-gui.git yet.
-- Alexander
git-gui.sh | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-gui.sh b/git-gui.sh
index e6e8890..2d14bf2 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -473,10 +473,10 @@ proc githook_read {hook_name args} {
set pchook [gitdir hooks $hook_name]
lappend args 2>@1
- # On Cygwin [file executable] might lie so we need to ask
+ # On Windows [file executable] might lie so we need to ask
# the shell if the hook is executable. Yes that's annoying.
#
- if {[is_Cygwin]} {
+ if {[is_Windows]} {
upvar #0 _sh interp
if {![info exists interp]} {
set interp [_which sh]
--
1.5.3.3
^ permalink raw reply related
* Re: Considering teaching plumbing to users harmful
From: Junio C Hamano @ 2008-07-16 20:12 UTC (permalink / raw)
To: Avery Pennarun; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <32541b130807161246l579d3a5em65496ee9119ef1ef@mail.gmail.com>
"Avery Pennarun" <apenwarr@gmail.com> writes:
> On 7/16/08, Junio C Hamano <gitster@pobox.com> wrote:
>> You said svn makes it easier because it makes it very hard to do merges
>> and forces users to stay away from them. This results in user doing "svn
>> update" which is to resolve conflicts with large uncommitted changes but
>> keeps the history straight single-strand-of-pearls.
>>
>> I am not saying the merge based workflow in git does not have any room to
>> improve. I am just saying that there is nothing we can learn from svn in
>> that area. "Solves it by not letting us to do merges" is not a solution.
>
> What svn does is essentially an unsafe version of
>
> git stash && git pull x y && git stash apply
If you are advocating that mode of operation to be easy in git, you should
think again. That pull (be it with or without --rebase) can conflict and
you would need to resolve it, and then your "stash pop" can conflict
again. You can have your own "git avery-up" alias which is your "svn up"
equivalent, but if you train users with that first, the users have to
learn how to cope with conflicts in individual steps anyway.
Making these three into a single alias is not an improvement either. If
you are not ready to incorporate other's changes to your history, why are
you pulling? Being distributed gives the power of working independently
and at your own pace. You should train your brain to think about the
workflow first. "You should stash before pull" is _not_ a good advice.
"Do not pull when you are not ready" is.
Suppose you are about to finish something you have been cooking (say a
series of five logical commits), you've made three of these commits
already, and what you have in your work tree and the index is to be split
into the last two commits. Somehow you learn that $x above has a updated
version.
Yes, running "git stash && git pull --rebase && git stash pop" would be
better than running "git pull --rebase" alone from that state. But that
would mean your history would have your first 3 commits (of 5 commit
series), somebody else's totally unrelated commits, and then you will work
on finishing the remaining 2 commits on top of it. Why? Why is such a
bogus linear history any better?
With git, instead, you have the choice:
* "git fetch" first to see if it is truly urgent; otherwise you
don't even have to pull. First finish whatever you were doing
and then make the pull
$ make commit 1
$ make commit 2
$ git pull
This results in a merge but it is a good merge. The resulting
history shows your 5 commits are isolated work independently
developed while that urgent thing was being done elsewhere.
* if it is, then, you save away your work and pull first:
$ git branch mywork
$ git stash save "remaining two commits' worth of changes"
$ git reset --hard HEAD~3 # wipe your 3 commits
$ git pull
and then continue working:
$ git checkout mywork
$ git stash pop
$ make commit 1
$ make commit 2
and finally integrate:
$ git checkout master
$ git merge mywork
or if you really want linear, pretend all 5 of your commits
were done on top of that urgent thing.
$ git rebase master
$ git checkout master
$ git merge mywork
> And that's actually a good example of what I'm talking about; in svn,
> that's just "svn up",...
What you forgot to add in the above is that in svn the equivalent of "pull
x y" step will always fast forward because you will not be making forked
development with the upstream. In svn it's just "svn up" and it results
in a linear history because that command does not work with merges. By
definition, not working with merges will result in linear history.
^ permalink raw reply
* Re: Considering teaching plumbing to users harmful
From: Theodore Tso @ 2008-07-16 20:13 UTC (permalink / raw)
To: Avery Pennarun; +Cc: Johannes Schindelin, git
In-Reply-To: <32541b130807161135h64024151xc60e23d222a3a508@mail.gmail.com>
On Wed, Jul 16, 2008 at 02:35:16PM -0400, Avery Pennarun wrote:
> In svn, a branch is a revision-controlled directory. In git, a branch
> is a "ref". What's a ref? Well, it's a name for a commit. What's a
> commit? Well, it's a blob. What's a blob? Err, that's complicated.
> What happens when I delete a branch? Well, it's still in the reflog.
> What's the reflog? Well, it's the local revision history of each
> branch. Local? Why not shared? In svn, the revision history of each
> branch is shared, but in git, you don't need to, because...
>
> Even git branches are surprisingly concept heavy, unless your users
> ask a lot fewer questions than mine. The really critical question is
> why it's so easy to delete a branch in git, and that leads rapidly
> into the commit-tree stuff, which is always a spiral into plumbing as
> you try to explain the tree of commits.
I don't think you need to go into the plumbing to explain the commit
tree. What I normally do is tell people that branches point at
commits, and that commits are identified by commit ID's, which can be
full SHA-1 hashes, or which can be abbreviated for convenience's sake.
It's not strictly necessary to tell them about the commit-tree
plumbing command; just that each commit creates a snapshot, and that
commits can have one or more parents, plus the commit mesage, plus the
snapshot.
I do absolutely agree with Johannes' assertion that you don't have to
explain commit-tree, git-rev-list, and all the rest. The only reason
why users will need to see git-rev-list is because git-log references
it so prominently, and some of the more powerful git-log options are
only documented in git-rev-list.
- Ted
^ permalink raw reply
* Re: [PATCH,RFC] Implement 'git rm --if-missing'
From: David Christensen @ 2008-07-16 19:43 UTC (permalink / raw)
To: Peter Baumann; +Cc: Junio C Hamano, Ciaran McCreesh, git
In-Reply-To: <20080716185811.GA3517@xp.machine.xx>
On Jul 16, 2008, at 1:58 PM, Peter Baumann wrote:
> On Wed, Jul 16, 2008 at 11:48:42AM -0700, Junio C Hamano wrote:
>> Ciaran McCreesh <ciaran.mccreesh@googlemail.com> writes:
>>
>>> git rm --if-missing will only remove files if they've already been
>>> removed from
>>> disk.
>>
>> This probably is a borderline with feaping creaturism. What's the
>> use of
>> it in a real workflow that you need this for?
>>
>> "git add -u" may be too broad in that it also adds anything
>> modified, but
>> so is --if-missing too broad in that it removes anything removed,
>> and if
>> you are going to limit by giving pathspecs _anyway_, then...
>>
>> Old timers might just do:
>>
>> git diff --name-only --diff-filter=D |
>> git update-index --remove --stdin
>>
>> ;-)
>>
>
> Ah. This comes in handy. I already searched for a command to delete
> all
> missing files. After reading through the fine manual of 'git rm', I
> went
> to git update-index but didn't come up with a solution to my problem.
>
> But I have to say, an argument to 'git rm' would be preferable than
> the
> above plumping.
Wouldn't:
git rm $(git ls-files --deleted)
do the trick, or am I missing something?
Regards,
David
--
David Christensen
End Point Corporation
david@endpoint.com
^ permalink raw reply
* Re: Considering teaching plumbing to users harmful
From: Stephen R. van den Berg @ 2008-07-16 20:23 UTC (permalink / raw)
To: Avery Pennarun; +Cc: Johannes Schindelin, git
In-Reply-To: <32541b130807161053w24a21d7bh1fa800a714ce75db@mail.gmail.com>
Avery Pennarun wrote:
>Also, inevitably, someone will ask "what happened to those simple svn
>revision numbers?" or "when I do a merge, why are the patches from
>branch #1 interspersed with the ones from branch #2 in git log?" The
>answers are "look at gitk to see the real merge history, that's way
>more powerful than svn, and check out git-bisect!" and "use git log
>--topo-order" respectively, but those are pretty nontrivial answers
>too.
Try --first-parent, it simplifies the history.
--
Sincerely,
Stephen R. van den Berg.
The eleventh commandment: Thou shalt not re-curse!
^ permalink raw reply
* Re: Considering teaching plumbing to users harmful
From: Nicolas Pitre @ 2008-07-16 20:27 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.1.00.0807161804400.8950@racer>
On Wed, 16 Jul 2008, Johannes Schindelin wrote:
> I had the pleasure of introducing Git to a few users in the last months
> and in my opinion, restricting myself to teaching them these commands
> first helped tremendously:
>
> - clone, pull, status, add, commit, push, log
>
> All of these were presented without options, to keep things simple.
I completely agree with you.
In the context of remote tracking branches, I usually talk about
'git init' + 'git remote' + 'git fetch' + 'git merge' and/or
'git rebase' which is somehow simpler to really understand than
'git clone' + 'git pull'. At that point the branch concept is usually
clear.
Nicolas
^ permalink raw reply
* Re: [PATCH] Reformat "your branch has diverged..." lines to reduce line length.
From: Avery Pennarun @ 2008-07-16 20:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v4p6pr4ob.fsf@gitster.siamese.dyndns.org>
On 7/16/08, Junio C Hamano <gitster@pobox.com> wrote:
> Avery Pennarun <apenwarr@gmail.com> writes:
> > if (!num_theirs)
> > - strbuf_addf(sb, "Your branch is ahead of the tracked%s branch '%s' "
> > + strbuf_addf(sb, "Your branch is ahead of the tracked%s branch '%s'\n"
> > "by %d commit%s.\n",
>
> I wonder if a simple "s/of the tracked%s branch //" is better for this
> kind of thing. If the message says 'origin/master', you know it is a
> tracked remote branch anyway, don't you?
Personally, I would be fine with shorter messages; this doesn't seem
the best time to
report the name of the tracked branch anyhow. I use 80x24 terminals
too most of the time.
However, I didn't write the original patch either, and I recall that
this feature was so popular that it actually resulted in a list thread
complimenting it, so I was hesitant to change it too much :)
> Your branch is ahead of 'origin/add-chickens2' by 21 commits.
>
> Your branch is behind 'origin/add-chickens2' by 1 commit.
>
> Your branch and 'origin/add-chickens2' have diverged, and have
> 21 and 1 different commit(s) each, respectively.
>
> I moved "respectively" so that the variable parts will come close to the
> beginning of physical line.
Well, the fact that the number of commits is "variable" isn't so
important, unless you start diverging by 1e9 commits or something :)
It might be nice to minimize the amount of static text on the line
containing the branch name, though. Your rephrasing would allow us to
go as far as:
Your branch and 'origin/add-chickens2'
have diverged, and have 21 and 1 different commit(s) each, respectively.
Which looks a little imbalanced, but works with long branch names.
Alternatively, your rephrasing above made me think of the idea of just
printing *both* of the first two messages in the "diverging" case.
Depending how you think of it, that would be either more clear or less
clear. It's actually easier to parse the two short sentences than the
long one.
Please let me know if you want me to resubmit the patch with your
suggestions or whether you'll handle it. I'm still a little vague on
the exact patch approval process.
Have fun,
Avery
^ permalink raw reply
* Re: [PATCH 2/3] add new Git::Repo API
From: Lea Wiemann @ 2008-07-16 20:32 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, John Hawley, Petr Baudis
In-Reply-To: <200807162021.56380.jnareb@gmail.com>
Jakub Narebski wrote:
> [Here is promised patch review]
Thanks. A new patch series will follow today (hopefully).
For brevity, I'm incorporating all your suggested changes unless noted
otherwise.
> On Fri, 11 July 2008, Lea Wiemann wrote:
>> create mode 100644 perl/Git/Commit.pm
>> create mode 100644 perl/Git/Object.pm
>> [...]
>
> Does splitting into many small files is really necessary?
I think it's better. People shouldn't get an error message if they
write "use Git::Commit". Also we cannot split later if we don't split
now since people would write "use Git::Repo" and then access
Git::Commit; subsequently splitting the API would (might?) cause
breakage, I believe.
> Good that you provided test suite.
;-)
> You can add new headers, and old git binary should simply ignore
> unknown headers.
Unknown headers are now ignored.
>> +# Keep documentation in one place to save space.
>
> Embedded PODs in Perl modules serve as sort of literate programming,
> serving to describe code (technical/usage documentation) in addition
> to comments in code.
Yeah, but this part is only a bunch of trivial accessor methods. If the
module grows and the documentation needs to be split, it can be done
later. No need to be purist here. ;-) Also ...
> [The fact that documentation is separated from code means that
> I cannot easily tell and write if code match documentation]
Several of the methods actually only exist in the Git::Object base
class. I still documented them in the Commit and Tag modules since
having to look up methods in base class documentation can be a tad
annoying, especially if the base class is never used by users of the API.
>> +=item $commit = Git::Commit->new($repo, $sha1)
>> +
>> +Calls to this method are free, [...]
>
> The technique you use has a name, and it is (IIUC) "lazy evaluation".
You understand correctly; I'd call it "lazy loading" though. I've added
that term for clarity.
> By the way, wouldn't it be better to make this method internal, and
> use instead the following code to generate Git::Commit object
>
> $commit = $repo->commit($sha1);
I think the constructor shouldn't be internal (= underscore-prefixed,
you mean?), since the Commit/Tag APIs are usable on their own.
>> +=item $obj->repo
>> +=item $obj->sha1
>
> Those do not access the repository, isn't it?
No, they don't. (Clarified that in the documentation.)
>> +=item $commit->parents
>> +Return a list of zero or more parent commit objects.
>
> Array or arrayref?
Array. I've replaced "list" with "array" in case it helps clarity.
> There is little inconsistency that tree object is (from the lack of
> Git::Tree object) returned as SHA1, and parents as objects.
If we add a Git::Tree API, the Git::Tree objects will stringify to their
SHA1s, so we shouldn't have compatibility issues. I've changed the
documentation of $commit->tree to this:
"Return an object that stringifies to the SHA1 of the tree that this
commit object refers to. (Currently this returns an actual string,
but don't rely on it.)"
> NOTE that element of list of revisions has in addition to that also
> _effective_ parents in the event of history simplification, for example
> for 'history' view, or when using '--first-parent' extra option.
Yes, but we don't actually care about those effective parents for the
purpose of the Git::Commit API. IOW, the effective parent should be
managed by the code that created a list of revisions, not by the
Git::Commit API.
>> +Return the author string of this commit object. [...]
>> +Return the committer string of this commit object.
>
> It returns whole value of 'author' and 'committer' headers, not
> something extracted from it (into name, email, epoch and timezone),
> isn't it?
Yup; that's why I wrote "{author,committer} *string*". ;)
>> +=item $commit->message
>> +
>> +Return the undecoded commit message of this commit object.
>
> Just raw data?
Yes, just raw data. Decoding is too tricky (i.e. not guaranteed to
work) to just add a simple method to the API; IOW, it needs error
handling and perhaps fallback encodings.
> NOTE that for element of list of revisions (as returned by git-rev-list
> or git-log) would probably have commit message decoded to UTF-8 by git.
Yes, but the API doesn't use any of those commands internally, if that's
what you're worried about.
>> +=item $commit->encoding
>> +
>> +Return the encoding header of the commit object.
>
> Normalized?
No. (Otherwise I'd have written that ;-).)
>> +sub author { [...]
>> + $self->{_AUTHOR()} or ''; }
>
> Nowhere in documentation is mentioned that you use empty value for no
> author or no committer (isn't commit object invalid then?).
Yes, I'd believe so. I basically wanted to make sure that those methods
always return a string; do you think that this is a bad idea?
>> + if (!defined $raw_text) {
>> + # Retrieve from the repository.
>> + (my $type, $raw_text) = $self->repo->cat_file($sha1);
>
> The above makes Git::Commit good solution for gitweb's 'commit' and
> 'commitdiff' views, but bad solution for 'log', 'shortlog', 'history'
> and 'rss'/'atom' views, where you would need to many command
> invocations, which is very bad on OS with slow fork.
$repo->cat_file (now renamed to get_object) actually doesn't fork but
uses a pipe (cat-file --batch); I don't think it should be a performance
issue.
>> + (my $header, $self->{_MESSAGE()}) = split "\n\n", $raw_text, 2;
>
> Why not simply parse headers, then slurp rest of object into 'message'
> field?
Because we happen to get the raw text in a single string from the Repo
API. (It shouldn't be a performance/memory issue for Commit or Tag
objects at all. ;-))
> IMHO you should not die at unrecognized header,
Yes, changed, but ...
> but simply store it under its name (and make available using
> ->header('<NAME>')).
... let's not complicate the API unnecessarily. If a new header pops up
we can immediately add it to the Commit/Tag API.
> Nevertheless I think you can check for header name validation (there
> are some restrictions on header names, isn't it?).
I don't really care, and it's too much work to come up with a test case
for this. ;-) If the repository is borked to the point of invalid
header names, it's fine for Git::Commit to behave undefinedly.
>> +You will usually want to call $repo->get_commit($sha1) instead of
>> +instantiating this class directly; see L<Git::Repo>.
>
> Why not $repo->commit($sha1)?
Intuition. ;-) I think I'd read $repo->commit as "the commit of the
repository", akin to $commit->tree, which doesn't make sense here.
>> +Git::Object - Object-oriented interface to Git objects (base class).
>
> Is it base class which represents types of objects in git repository:
> commits, tags, trees and blobs? Or just a class which represent
> headers+payload objects, i.e. commits and tags?
It could represent any object, though I don't see a need for Git::Blob
right now (though it's possible that it's needed later).
> I wonder if Git::Object should provide $obj->id alias to $obj->sha1...
Why? I don't think it's necessary.
>> +use 5.006002;
>
> Why is this "use 5.006002" for?
It signifies that this module won't run with Perl <5.6.2. I've had to
bump it to 5.008 (Perl 5.8); more about that in the message announcing
the next version of the patch series.
> Wouldn't it be better to allow the same discovery of '.git' directory
> as other git command do, and leave 'git_dir' to set directly path to
> repository itself?
I wouldn't use discovery magic here, at least for now, since it's
non-trivial to get it right (and it interacts with possible future
extensions of the API, like Git::WC). Such a feature can be implemented
if/when it's needed.
>> +=item 'git_binary'
>> +The name or full path of the git binary (default: 'git').
>
> Probably should be Git::Cmd or Git object, instead.
I don't think something Git::Cmd is a good idea (as I pointed out in my
reply to Petr, <487BD0F3.2060508@gmail.com>), or at least it shouldn't
be implemented as part of this patch series. This method is really just
supposed to return an argument for exec*p, nothing more.
>> +Calling this method is free, since it does not check whether the
>> +repository exists. Trying to access the repository through one of the
>> +instance methods will fail if it doesn't exist though.
>
> Not even rudimentary check: if directory exists, if it looks like
> git repository?
No. It's not helpful for error handling (which should happen in the
caller), and it's not helpful for bug detection (since it will die on
the first access to the repo anyway), but it causes performance penalty
that can be significant for programs like gitweb.
>> +=item $repo->repo_dir
>> +Return the directory of the repository (.../.git in case of a working
>> +copy).
>
> I think $repo->git_dir (perhaps in addition to above) would be better
> name, as it is already established among git commands.
I find repo_dir somewhat clearer (and I don't like having more than one
name per method). We're not trying to mimic or wrap standard git
commands here, anyway.
>> +sub version{
>
> We could rely instead on embedded (during build) version string...
Yup; it's been deleted anyway.
>> +=item $repo->cmd_output(%opts)
>
> Please do remember that there are git commands which do not need
> access to git repository,
As I wrote in my reply to Petr, Git::Repo is not trying to be a wrapper
around git binaries, so this method really shouldn't be part of the
official API -- it's just auxiliary; I'll underscore-prefix it.
> I think it would be easier on users if you provide two ways of calling
> [cmd_output]: simple and advanced
It's not part of the API anyway, so no need for complicated calling
conventions, IMO.
>> +# To do: According to Git.pm, this might not work with ActiveState
>> +# Perl on Win 32. Need to check or wait for reports.
>
> Why not copy code from Git.pm, then?
Apart from the fact that I don't do cargo-cult programming? ;-) Git.pm
forks, whereas Git::Repo uses open, '-|', so it's actually different
(and it's not possible to copy the code).
>> +=item $repo->get_bidi_pipe(%opts)
>> +
>> +Open a new bidirectional pipe and return its STDIN and STDOUT file
>> +handles. Valid options are:
>
> What about returning context, as it was done in Git.pm?
Why, what should it do? This just opens a pipe, nothing more. No need
for introducing complicated concepts.
>> +=item 'reuse'
>> +
>> +If true, reuse a previously opened pipe with the same command line and
>> +whose C<reuse> option was true (default: false).
>
> What is this for? Can you show example usage of this feature?
You found it below. :-) (If you had snipped this, I wouldn't have spent
time finding and pasting an example. ;-))
> I think I'd rather allow extended SHA1 syntax in Git::Commit
> and Git::Tag constructors; it is one call to git command less
> (I think).
I wouldn't -- see my blurb about error handling at the top of my reply
to Petr (<487BD0F3.2060508@gmail.com>). You're not supposed to pass
anything that you didn't get from get_sha1 into Git::Commit or Git::Tag
constructors, or your error handling is invariably broken.
>> + my ($in, $out) = $self->get_bidi_pipe(
>> + cmd => ['cat-file','--batch-check'], reuse => 1);
>
> Ahhh... here I can see what 'reuse => 1' means, and when it is useful.
> But doesn't it make sense _only_ for _bi-directional pipe_? Are you
> sure that you wouldn't get deadlock?
Yes to both questions. :-)
>> [get_object:]
>> +# Possible to-do items: Add optional $file_handle parameter.
>
> If I remember correctly you do implement something like that (streamed
> output) in gitweb patch.
Yes, but only for generic command calls, and with a somewhat unpleasant
(cache-specific) interface. It'd need a bit of work for the API.
> By the way, for gitweb you would need (for performance and for
> rewritten parents) also get_log / get_commits / get_commits_list
No. ;-) Doing fine without those.
>> +=item $repo->get_path($tree_sha1, $file_sha1)
This one has been removed as well since it would belong into Git::Tree.
>> + my @lines = split "\n", $self->cmd_output(cmd => ['ls-tree', '-r', '-t', $tree]);
>
> You would have troubles with filename quoting!
Thanks. Since get_path isn't in the Git::Repo API anymore and gitweb's
get_path subroutine didn't handle quoted filenames even before my
patches, I'll only mark it as TODO for now. ;-)
>> +sub get_refs {
This has been removed as well, since it's not used and the interface
would need work.
>> +=item $repo->name_rev($committish_sha1, $tags_only = 0)
>
> Why name_rev, and no describe?
Feel free to add it. ;-) (It might take some work to come up with a
decent interface for that method.)
> Does Git::RepoRoot provides way to scan for repositories under
> commin $projectroot?
No. If it's needed it could reasonably be extracted from gitweb though
(I think).
>> +Return the tagger string of this tag object.
>
> We would probably want some way to extract name, email, epoch/date
> (and a way to convert epoch+timezone to RFC or ISO format), timezone.
Yeah. At some point. ;-)
>> +=item $tag->encoding
>> +Return the encoding header of the tag object.
>
> Does tag provide 'encoding' header?
Sure, for the message.
> Should (for completeness) Git::Tag provide $tag->validate() method?
No, since 'validate' sounds like it would have to do error handling.
If you mean that this should check if the object exists (and has the
advertised type), the user of the API should test for "defined
$tag->repo->get_sha1($tag->object)" or somesuch and do error handling
themselves.
-- Lea
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox