* [PATCH 1/3] tree-walk: be more specific about corrupt tree errors
From: David Turner @ 2016-09-27 0:11 UTC (permalink / raw)
To: git; +Cc: Jeff King, David Turner
From: Jeff King <peff@peff.net>
When the tree-walker runs into an error, it just calls
die(), and the message is always "corrupt tree file".
However, we are actually covering several cases here; let's
give the user a hint about what happened.
Let's also avoid using the word "corrupt", which makes it
seem like the data bit-rotted on disk. Our sha1 check would
already have found that. These errors are ones of data that
is malformed in the first place.
Signed-off-by: David Turner <dturner@twosigma.com>
Signed-off-by: Jeff King <peff@peff.net>
---
t/t1007-hash-object.sh | 15 +++++++++++++--
t/t1007/.gitattributes | 1 +
t/t1007/tree-with-empty-filename | Bin 0 -> 28 bytes
t/t1007/tree-with-malformed-mode | Bin 0 -> 39 bytes
tree-walk.c | 12 +++++++-----
5 files changed, 21 insertions(+), 7 deletions(-)
create mode 100644 t/t1007/.gitattributes
create mode 100644 t/t1007/tree-with-empty-filename
create mode 100644 t/t1007/tree-with-malformed-mode
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index acca9ac..f21848b 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -183,9 +183,20 @@ for args in "-w --stdin-paths" "--stdin-paths -w"; do
pop_repo
done
-test_expect_success 'corrupt tree' '
+test_expect_success 'too-short tree' '
echo abc >malformed-tree &&
- test_must_fail git hash-object -t tree malformed-tree
+ test_must_fail git hash-object -t tree malformed-tree 2>err &&
+ grep "too-short tree object" err
+'
+
+test_expect_success 'malformed mode in tree' '
+ test_must_fail git hash-object -t tree ../t1007/tree-with-malformed-mode 2>err &&
+ grep "malformed mode in tree entry" err
+'
+
+test_expect_success 'empty filename in tree' '
+ test_must_fail git hash-object -t tree ../t1007/tree-with-empty-filename 2>err &&
+ grep "empty filename in tree entry" err
'
test_expect_success 'corrupt commit' '
diff --git a/t/t1007/.gitattributes b/t/t1007/.gitattributes
new file mode 100644
index 0000000..7352ef5
--- /dev/null
+++ b/t/t1007/.gitattributes
@@ -0,0 +1 @@
+tree-with-* -diff
diff --git a/t/t1007/tree-with-empty-filename b/t/t1007/tree-with-empty-filename
new file mode 100644
index 0000000000000000000000000000000000000000..aeb1ceb20e485eebd0acbb81c974d1c6fedcc1fe
GIT binary patch
literal 28
kcmXpsFfcPQQDAsB_tET47q2;ccWbUIkGgT_Nl)-Z0Hx{;SO5S3
literal 0
HcmV?d00001
diff --git a/t/t1007/tree-with-malformed-mode b/t/t1007/tree-with-malformed-mode
new file mode 100644
index 0000000000000000000000000000000000000000..24aa84d60ef8e269fb0b29c67b5208639b9da3ae
GIT binary patch
literal 39
vcmYewPcJRb%}+^HNXyJg%}dNpWq3CC(d<nZuQ_{nYpyGgx^d`9Pw+$lU*Quk
literal 0
HcmV?d00001
diff --git a/tree-walk.c b/tree-walk.c
index ce27842..ba544cf 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -27,12 +27,14 @@ static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned
const char *path;
unsigned int mode, len;
- if (size < 24 || buf[size - 21])
- die("corrupt tree file");
+ if (size < 23 || buf[size - 21])
+ die("too-short tree object");
path = get_mode(buf, &mode);
- if (!path || !*path)
- die("corrupt tree file");
+ if (!path)
+ die("malformed mode in tree entry for tree");
+ if (!*path)
+ die("empty filename in tree entry for tree");
len = strlen(path) + 1;
/* Initialize the descriptor entry */
@@ -81,7 +83,7 @@ void update_tree_entry(struct tree_desc *desc)
unsigned long len = end - (const unsigned char *)buf;
if (size < len)
- die("corrupt tree file");
+ die("too-short tree file");
buf = end;
size -= len;
desc->buffer = buf;
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* [PATCH 3/3] add David Turner's Two Sigma address
From: David Turner @ 2016-09-27 0:11 UTC (permalink / raw)
To: git; +Cc: David Turner
In-Reply-To: <1474935093-26757-1-git-send-email-dturner@twosigma.com>
From: David Turner <novalis@novalis.org>
Signed-off-by: David Turner <novalis@novalis.org>
---
.mailmap | 1 +
1 file changed, 1 insertion(+)
diff --git a/.mailmap b/.mailmap
index 9441a54..9cc33e9 100644
--- a/.mailmap
+++ b/.mailmap
@@ -48,6 +48,7 @@ David KÃ¥gedal <davidk@lysator.liu.se>
David Reiss <dreiss@facebook.com> <dreiss@dreiss-vmware.(none)>
David S. Miller <davem@davemloft.net>
David Turner <novalis@novalis.org> <dturner@twopensource.com>
+David Turner <novalis@novalis.org> <dturner@twosigma.com>
Deskin Miller <deskinm@umich.edu>
Dirk Süsserott <newsletter@dirk.my1.cc>
Eric Blake <eblake@redhat.com> <ebb9@byu.net>
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* [PATCH 2/3] fsck: handle bad trees like other errors
From: David Turner @ 2016-09-27 0:11 UTC (permalink / raw)
To: git; +Cc: David Turner
In-Reply-To: <1474935093-26757-1-git-send-email-dturner@twosigma.com>
Instead of dying when fsck hits a malformed tree object, log the error
like any other and continue. Now fsck can tell the user which tree is
bad, too.
Signed-off-by: David Turner <dturner@twosigma.com>
---
fsck.c | 18 +++--
t/t1450-fsck.sh | 17 ++++-
t/t1450/bad-objects/.gitattributes | 1 +
.../307e300745b82417cc1a903f875c7d22e45ef907 | Bin 0 -> 137 bytes
.../f506a346749bb96f52d8605ffba9fb93d46b5ffd | Bin 0 -> 45 bytes
tree-walk.c | 83 ++++++++++++++++++---
tree-walk.h | 8 ++
7 files changed, 108 insertions(+), 19 deletions(-)
create mode 100644 t/t1450/bad-objects/.gitattributes
create mode 100644 t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907
create mode 100644 t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd
diff --git a/fsck.c b/fsck.c
index c9cf3de..4a3069e 100644
--- a/fsck.c
+++ b/fsck.c
@@ -347,8 +347,9 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op
return -1;
name = get_object_name(options, &tree->object);
- init_tree_desc(&desc, tree->buffer, tree->size);
- while (tree_entry(&desc, &entry)) {
+ if (init_tree_desc_gently(&desc, tree->buffer, tree->size))
+ return -1;
+ while (tree_entry_gently(&desc, &entry)) {
struct object *obj;
int result;
@@ -520,7 +521,7 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con
static int fsck_tree(struct tree *item, struct fsck_options *options)
{
- int retval;
+ int retval = 0;
int has_null_sha1 = 0;
int has_full_path = 0;
int has_empty_name = 0;
@@ -535,7 +536,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
unsigned o_mode;
const char *o_name;
- init_tree_desc(&desc, item->buffer, item->size);
+ if (init_tree_desc_gently(&desc, item->buffer, item->size)) {
+ retval += report(options, &item->object, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
+ return retval;
+ }
o_mode = 0;
o_name = NULL;
@@ -556,7 +560,10 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
is_hfs_dotgit(name) ||
is_ntfs_dotgit(name));
has_zero_pad |= *(char *)desc.buffer == '0';
- update_tree_entry(&desc);
+ if (update_tree_entry_gently(&desc)) {
+ retval += report(options, &item->object, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
+ break;
+ }
switch (mode) {
/*
@@ -597,7 +604,6 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)
o_name = name;
}
- retval = 0;
if (has_null_sha1)
retval += report(options, &item->object, FSCK_MSG_NULL_SHA1, "contains entries pointing to null sha1");
if (has_full_path)
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 8f52da2..f456963 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -188,8 +188,7 @@ test_expect_success 'commit with NUL in header' '
grep "error in commit $new.*unterminated header: NUL at offset" out
'
-test_expect_success 'malformatted tree object' '
- test_when_finished "git update-ref -d refs/tags/wrong" &&
+test_expect_success 'tree object with duplicate entries' '
test_when_finished "remove_object \$T" &&
T=$(
GIT_INDEX_FILE=test-index &&
@@ -208,6 +207,20 @@ test_expect_success 'malformatted tree object' '
grep "error in tree .*contains duplicate file entries" out
'
+test_expect_success 'unparseable tree object' '
+ test_when_finished "git update-ref -d refs/heads/wrong" &&
+ test_when_finished "remove_object 307e300745b82417cc1a903f875c7d22e45ef907" &&
+ test_when_finished "remove_object f506a346749bb96f52d8605ffba9fb93d46b5ffd" &&
+ mkdir -p .git/objects/30 mkdir -p .git/objects/f5 &&
+ cp ../t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 .git/objects/30/7e300745b82417cc1a903f875c7d22e45ef907 &&
+ cp ../t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd .git/objects/f5/06a346749bb96f52d8605ffba9fb93d46b5ffd &&
+ git update-ref refs/heads/wrong 307e300745b82417cc1a903f875c7d22e45ef907 &&
+ test_must_fail git fsck 2>out &&
+ grep "warning: empty filename in tree entry" out &&
+ grep "f506a346749bb96f52d8605ffba9fb93d46b5ffd" out &&
+ ! grep "fatal: empty filename in tree entry" out
+'
+
test_expect_success 'tag pointing to nonexistent' '
cat >invalid-tag <<-\EOF &&
object ffffffffffffffffffffffffffffffffffffffff
diff --git a/t/t1450/bad-objects/.gitattributes b/t/t1450/bad-objects/.gitattributes
new file mode 100644
index 0000000..a173f27
--- /dev/null
+++ b/t/t1450/bad-objects/.gitattributes
@@ -0,0 +1 @@
+[0-9a-f]*[0-9a-f] -diff
diff --git a/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907 b/t/t1450/bad-objects/307e300745b82417cc1a903f875c7d22e45ef907
new file mode 100644
index 0000000000000000000000000000000000000000..6e23d625531856540364837ad76f8ce620b16102
GIT binary patch
literal 137
zcmV;40CxX)0iBLP4#FT1MO|}>xqxP{K%K-G7aqY2Fa=r?TM`QO`l9IxT>bpTd;bq<
zo?`(?@=&t(5HuRwDbp)rCKL48T@30F*ivBXoHE>+6SkHqWq8;vI(XK+_zc%2ZT1z{
r`<|zi#~Vo1WD<KV;fM-R48P6NfPd&6hj%O!a2o3h-{;~3ChI@mcIrR_
literal 0
HcmV?d00001
diff --git a/t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd b/t/t1450/bad-objects/f506a346749bb96f52d8605ffba9fb93d46b5ffd
new file mode 100644
index 0000000000000000000000000000000000000000..9111a7fc3c8578906e13c930a0fbd3cae047762e
GIT binary patch
literal 45
zcmb=Jqpj)X8)~pA!NA18z}PS_p~CF@#W%j<>n*Fxv)5_&?<#!Z>Hoon;loq@NdS%f
B6F2|>
literal 0
HcmV?d00001
diff --git a/tree-walk.c b/tree-walk.c
index ba544cf..0fb830b 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -22,33 +22,60 @@ static const char *get_mode(const char *str, unsigned int *modep)
return str;
}
-static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)
+static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size, struct strbuf *err)
{
const char *path;
unsigned int mode, len;
- if (size < 23 || buf[size - 21])
- die("too-short tree object");
+ if (size < 23 || buf[size - 21]) {
+ strbuf_addstr(err, "too-short tree object");
+ return -1;
+ }
path = get_mode(buf, &mode);
- if (!path)
- die("malformed mode in tree entry for tree");
- if (!*path)
- die("empty filename in tree entry for tree");
+ if (!path) {
+ strbuf_addstr(err, "malformed mode in tree entry");
+ return -1;
+ }
+ if (!*path) {
+ strbuf_addstr(err, "empty filename in tree entry");
+ return -1;
+ }
len = strlen(path) + 1;
/* Initialize the descriptor entry */
desc->entry.path = path;
desc->entry.mode = canon_mode(mode);
desc->entry.oid = (const struct object_id *)(path + len);
+
+ return 0;
}
-void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)
+static int init_tree_desc_internal(struct tree_desc *desc, const void *buffer, unsigned long size, struct strbuf *err)
{
desc->buffer = buffer;
desc->size = size;
if (size)
- decode_tree_entry(desc, buffer, size);
+ return decode_tree_entry(desc, buffer, size, err);
+ return 0;
+}
+
+void init_tree_desc(struct tree_desc *desc, const void *buffer, unsigned long size)
+{
+ struct strbuf err = STRBUF_INIT;
+ if (init_tree_desc_internal(desc, buffer, size, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
+}
+
+int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned long size)
+{
+ struct strbuf err = STRBUF_INIT;
+ int result = init_tree_desc_internal(desc, buffer, size, &err);
+ if (result)
+ warning("%s", err.buf);
+ strbuf_release(&err);
+ return result;
}
void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1)
@@ -75,7 +102,7 @@ static void entry_extract(struct tree_desc *t, struct name_entry *a)
*a = t->entry;
}
-void update_tree_entry(struct tree_desc *desc)
+static int update_tree_entry_internal(struct tree_desc *desc, struct strbuf *err)
{
const void *buf = desc->buffer;
const unsigned char *end = desc->entry.oid->hash + 20;
@@ -89,7 +116,30 @@ void update_tree_entry(struct tree_desc *desc)
desc->buffer = buf;
desc->size = size;
if (size)
- decode_tree_entry(desc, buf, size);
+ return decode_tree_entry(desc, buf, size, err);
+ return 0;
+}
+
+void update_tree_entry(struct tree_desc *desc)
+{
+ struct strbuf err = STRBUF_INIT;
+ if (update_tree_entry_internal(desc, &err))
+ die("%s", err.buf);
+ strbuf_release(&err);
+}
+
+int update_tree_entry_gently(struct tree_desc *desc)
+{
+ struct strbuf err = STRBUF_INIT;
+ if (update_tree_entry_internal(desc, &err)) {
+ warning("%s", err.buf);
+ strbuf_release(&err);
+ /* Stop processing this tree after error */
+ desc->size = 0;
+ return -1;
+ }
+ strbuf_release(&err);
+ return 0;
}
int tree_entry(struct tree_desc *desc, struct name_entry *entry)
@@ -102,6 +152,17 @@ int tree_entry(struct tree_desc *desc, struct name_entry *entry)
return 1;
}
+int tree_entry_gently(struct tree_desc *desc, struct name_entry *entry)
+{
+ if (!desc->size)
+ return 0;
+
+ *entry = desc->entry;
+ if (update_tree_entry_gently(desc))
+ return 0;
+ return 1;
+}
+
void setup_traverse_info(struct traverse_info *info, const char *base)
{
int pathlen = strlen(base);
diff --git a/tree-walk.h b/tree-walk.h
index 97a7d69..68bb78b 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -25,14 +25,22 @@ static inline int tree_entry_len(const struct name_entry *ne)
return (const char *)ne->oid - ne->path - 1;
}
+/*
+ * The _gently versions of these functions warn and return false on a
+ * corrupt tree entry rather than dying,
+ */
+
void update_tree_entry(struct tree_desc *);
+int update_tree_entry_gently(struct tree_desc *);
void init_tree_desc(struct tree_desc *desc, const void *buf, unsigned long size);
+int init_tree_desc_gently(struct tree_desc *desc, const void *buf, unsigned long size);
/*
* Helper function that does both tree_entry_extract() and update_tree_entry()
* and returns true for success
*/
int tree_entry(struct tree_desc *, struct name_entry *);
+int tree_entry_gently(struct tree_desc *, struct name_entry *);
void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1);
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
* [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules
From: Brandon Williams @ 2016-09-26 22:46 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <1474930003-83750-1-git-send-email-bmwill@google.com>
Pass through some known-safe options when recursing into submodules.
(--cached, --stage, -v, -t, -z, --debug, --eol)
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/ls-files.c | 34 ++++++++++++++++++++++++++++++----
t/t3007-ls-files-recurse-submodules.sh | 17 ++++++++++++-----
2 files changed, 42 insertions(+), 9 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index d4bfc60..a39367f 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -31,6 +31,7 @@ static int debug_mode;
static int show_eol;
static int recurse_submodules;
static const char *submodule_prefix;
+static struct argv_array recurse_submodules_opts = ARGV_ARRAY_INIT;
static const char *prefix;
static int max_prefix_len;
@@ -170,6 +171,27 @@ static void show_killed_files(struct dir_struct *dir)
}
}
+/*
+ * Compile an argv_array with all of the options supported by --recurse_submodules
+ */
+static void compile_submodule_options(int show_tag)
+{
+ if (show_cached)
+ argv_array_push(&recurse_submodules_opts, "--cached");
+ if (show_stage)
+ argv_array_push(&recurse_submodules_opts, "--stage");
+ if (show_valid_bit)
+ argv_array_push(&recurse_submodules_opts, "-v");
+ if (show_tag)
+ argv_array_push(&recurse_submodules_opts, "-t");
+ if (line_terminator == '\0')
+ argv_array_push(&recurse_submodules_opts, "-z");
+ if (debug_mode)
+ argv_array_push(&recurse_submodules_opts, "--debug");
+ if (show_eol)
+ argv_array_push(&recurse_submodules_opts, "--eol");
+}
+
/**
* Recursively call ls-files on a submodule
*/
@@ -184,6 +206,9 @@ static void show_gitlink(const struct cache_entry *ce)
argv_array_push(&cp.args, "ls-files");
argv_array_push(&cp.args, "--recurse-submodules");
+ /* add supported options */
+ argv_array_pushv(&cp.args, recurse_submodules_opts.argv);
+
cp.git_cmd = 1;
cp.dir = ce->name;
status = run_command(&cp);
@@ -568,14 +593,15 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
if (require_work_tree && !is_inside_work_tree())
setup_work_tree();
- if (recurse_submodules)
+ if (recurse_submodules) {
submodule_prefix = getenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT);
+ compile_submodule_options(show_tag);
+ }
if (recurse_submodules &&
- (show_stage || show_deleted || show_others || show_unmerged ||
+ (show_deleted || show_others || show_unmerged ||
show_killed || show_modified || show_resolve_undo ||
- show_valid_bit || show_tag || show_eol || with_tree ||
- (line_terminator == '\0')))
+ with_tree))
die("ls-files --recurse-submodules unsupported mode");
if (recurse_submodules && error_unmatch)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index 7d225ac..40767da 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -34,6 +34,18 @@ test_expect_success 'ls-files correctly outputs files in submodule' '
test_cmp expect actual
'
+test_expect_success 'ls-files correctly outputs files in submodule with -z' '
+ cat | tr "\n" "\0" >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ git ls-files --recurse-submodules -z >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'ls-files does not output files not added to a repo' '
cat >expect <<-\EOF &&
.gitmodules
@@ -86,15 +98,10 @@ test_incompatible_with_recurse_submodules () {
"
}
-test_incompatible_with_recurse_submodules -z
-test_incompatible_with_recurse_submodules -v
-test_incompatible_with_recurse_submodules -t
test_incompatible_with_recurse_submodules --deleted
test_incompatible_with_recurse_submodules --modified
test_incompatible_with_recurse_submodules --others
-test_incompatible_with_recurse_submodules --stage
test_incompatible_with_recurse_submodules --killed
test_incompatible_with_recurse_submodules --unmerged
-test_incompatible_with_recurse_submodules --eol
test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 4/4 v4] ls-files: add pathspec matching for submodules
From: Brandon Williams @ 2016-09-26 22:46 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <1474930003-83750-1-git-send-email-bmwill@google.com>
Pathspecs can be a bit tricky when trying to apply them to submodules.
The main challenge is that the pathspecs will be with respect to the
superproject and not with respect to paths in the submodule. The
approach this patch takes is to pass in the identical pathspec from the
superproject to the submodule in addition to the submodule-prefix, which
is the path from the root of the superproject to the submodule, and then
we can compare an entry in the submodule prepended with the
submodule-prefix to the pathspec in order to determine if there is a
match.
This patch also permits the pathspec logic to perform a prefix match against
submodules since a pathspec could refer to a file inside of a submodule.
Due to limitations in the wildmatch logic, a prefix match is only done
literally. If any wildcard character is encountered we'll simply punt
and produce a false positive match. More accurate matching will be done
once inside the submodule. This is due to the superproject not knowing
what files could exist in the submodule.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/ls-files.c | 28 ++++++---
dir.c | 46 +++++++++++++-
dir.h | 4 ++
t/t3007-ls-files-recurse-submodules.sh | 108 ++++++++++++++++++++++++++++++++-
4 files changed, 174 insertions(+), 12 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a39367f..2488a02 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -199,6 +199,7 @@ static void show_gitlink(const struct cache_entry *ce)
{
struct child_process cp = CHILD_PROCESS_INIT;
int status;
+ int i;
argv_array_pushf(&cp.args, "--submodule-prefix=%s%s/",
submodule_prefix ? submodule_prefix : "",
@@ -209,6 +210,15 @@ static void show_gitlink(const struct cache_entry *ce)
/* add supported options */
argv_array_pushv(&cp.args, recurse_submodules_opts.argv);
+ /*
+ * Pass in the original pathspec args. The submodule will be
+ * responsible for prepending the 'submodule_prefix' prior to comparing
+ * against the pathspec for matches.
+ */
+ argv_array_push(&cp.args, "--");
+ for (i = 0; i < pathspec.nr; i++)
+ argv_array_push(&cp.args, pathspec.items[i].original);
+
cp.git_cmd = 1;
cp.dir = ce->name;
status = run_command(&cp);
@@ -227,7 +237,8 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
if (len >= ce_namelen(ce))
die("git ls-files: internal error - cache entry not superset of prefix");
- if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+ if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+ submodule_path_match(&pathspec, name.buf, ps_matched)) {
show_gitlink(ce);
} else if (match_pathspec(&pathspec, name.buf, name.len,
len, ps_matched,
@@ -608,17 +619,20 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
die("ls-files --recurse-submodules does not support "
"--error-unmatch");
- if (recurse_submodules && argc)
- die("ls-files --recurse-submodules does not support path "
- "arguments");
-
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
prefix, argv);
- /* Find common prefix for all pathspec's */
- max_prefix = common_prefix(&pathspec);
+ /*
+ * Find common prefix for all pathspec's
+ * This is used as a performance optimization which unfortunately cannot
+ * be done when recursing into submodules
+ */
+ if (recurse_submodules)
+ max_prefix = NULL;
+ else
+ max_prefix = common_prefix(&pathspec);
max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
/* Treat unmatching pathspec elements as errors */
diff --git a/dir.c b/dir.c
index 0ea235f..28e9736 100644
--- a/dir.c
+++ b/dir.c
@@ -207,8 +207,9 @@ int within_depth(const char *name, int namelen,
return 1;
}
-#define DO_MATCH_EXCLUDE 1
-#define DO_MATCH_DIRECTORY 2
+#define DO_MATCH_EXCLUDE (1<<0)
+#define DO_MATCH_DIRECTORY (1<<1)
+#define DO_MATCH_SUBMODULE (1<<2)
/*
* Does 'match' match the given name?
@@ -283,6 +284,32 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
item->nowildcard_len - prefix))
return MATCHED_FNMATCH;
+ /* Perform checks to see if "name" is a super set of the pathspec */
+ if (flags & DO_MATCH_SUBMODULE) {
+ /* name is a literal prefix of the pathspec */
+ if ((namelen < matchlen) &&
+ (match[namelen] == '/') &&
+ !ps_strncmp(item, match, name, namelen))
+ return MATCHED_RECURSIVELY;
+
+ /* name" doesn't match up to the first wild character */
+ if (item->nowildcard_len < item->len &&
+ ps_strncmp(item, match, name,
+ item->nowildcard_len - prefix))
+ return 0;
+
+ /*
+ * Here is where we would perform a wildmatch to check if
+ * "name" can be matched as a directory (or a prefix) against
+ * the pathspec. Since wildmatch doesn't have this capability
+ * at the present we have to punt and say that it is a match,
+ * potentially returning a false positive
+ * The submodules themselves will be able to perform more
+ * accurate matching to determine if the pathspec matches.
+ */
+ return MATCHED_RECURSIVELY;
+ }
+
return 0;
}
@@ -386,6 +413,21 @@ int match_pathspec(const struct pathspec *ps,
return negative ? 0 : positive;
}
+/**
+ * Check if a submodule is a superset of the pathspec
+ */
+int submodule_path_match(const struct pathspec *ps,
+ const char *submodule_name,
+ char *seen)
+{
+ int matched = do_match_pathspec(ps, submodule_name,
+ strlen(submodule_name),
+ 0, seen,
+ DO_MATCH_DIRECTORY |
+ DO_MATCH_SUBMODULE);
+ return matched;
+}
+
int report_path_error(const char *ps_matched,
const struct pathspec *pathspec,
const char *prefix)
diff --git a/dir.h b/dir.h
index da1a858..97c83bb 100644
--- a/dir.h
+++ b/dir.h
@@ -304,6 +304,10 @@ extern int git_fnmatch(const struct pathspec_item *item,
const char *pattern, const char *string,
int prefix);
+extern int submodule_path_match(const struct pathspec *ps,
+ const char *submodule_name,
+ char *seen);
+
static inline int ce_path_match(const struct cache_entry *ce,
const struct pathspec *pathspec,
char *seen)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index 40767da..4a51d38 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -81,9 +81,111 @@ test_expect_success 'ls-files recurses more than 1 level' '
test_cmp expect actual
'
-test_expect_success '--recurse-submodules does not support using path arguments' '
- test_must_fail git ls-files --recurse-submodules b 2>actual &&
- test_i18ngrep "does not support path arguments" actual
+test_expect_success '--recurse-submodules and pathspecs setup' '
+ echo e >submodule/subsub/e.txt &&
+ git -C submodule/subsub add e.txt &&
+ git -C submodule/subsub commit -m "adding e.txt" &&
+ echo f >submodule/f.TXT &&
+ echo g >submodule/g.txt &&
+ git -C submodule add f.TXT g.txt &&
+ git -C submodule commit -m "add f and g" &&
+ echo h >h.txt &&
+ mkdir sib &&
+ echo sib >sib/file &&
+ git add h.txt sib/file &&
+ git commit -m "add h and sib/file" &&
+ git init sub &&
+ echo sub >sub/file &&
+ git -C sub add file &&
+ git -C sub commit -m "add file" &&
+ git submodule add ./sub &&
+ git commit -m "added sub" &&
+
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ h.txt
+ sib/file
+ sub/file
+ submodule/.gitmodules
+ submodule/c
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/d
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual &&
+ cat actual &&
+ git ls-files --recurse-submodules "*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/g.txt
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules "*.txt" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/e.txt
+ EOF
+
+ git ls-files --recurse-submodules ":(icase)*.txt" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ h.txt
+ submodule/f.TXT
+ submodule/g.txt
+ EOF
+
+ git ls-files --recurse-submodules ":(icase)*.txt" ":(exclude)submodule/subsub/*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ sub/file
+ EOF
+
+ git ls-files --recurse-submodules "sub" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "sub/" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "sub/file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "su*/file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "su?/file" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+ cat >expect <<-\EOF &&
+ sib/file
+ sub/file
+ EOF
+
+ git ls-files --recurse-submodules "s??/file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "s???file" >actual &&
+ test_cmp expect actual &&
+ git ls-files --recurse-submodules "s*file" >actual &&
+ test_cmp expect actual
'
test_expect_success '--recurse-submodules does not support --error-unmatch' '
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 2/4 v4] ls-files: optionally recurse into submodules
From: Brandon Williams @ 2016-09-26 22:46 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <1474930003-83750-1-git-send-email-bmwill@google.com>
Allow ls-files to recognize submodules in order to retrieve a list of
files from a repository's submodules. This is done by forking off a
process to recursively call ls-files on all submodules. Use top-level
--submodule_prefix option to pass a path to the submodule which it can
use to prepend to output or pathspec matching logic.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/git-ls-files.txt | 7 +-
builtin/ls-files.c | 143 ++++++++++++++++++++++++---------
git.c | 2 +-
t/t3007-ls-files-recurse-submodules.sh | 100 +++++++++++++++++++++++
4 files changed, 212 insertions(+), 40 deletions(-)
create mode 100755 t/t3007-ls-files-recurse-submodules.sh
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 0d933ac..446209e 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -18,7 +18,8 @@ SYNOPSIS
[--exclude-per-directory=<file>]
[--exclude-standard]
[--error-unmatch] [--with-tree=<tree-ish>]
- [--full-name] [--abbrev] [--] [<file>...]
+ [--full-name] [--recurse-submodules]
+ [--abbrev] [--] [<file>...]
DESCRIPTION
-----------
@@ -137,6 +138,10 @@ a space) at the start of each line:
option forces paths to be output relative to the project
top directory.
+--recurse-submodules::
+ Recursively calls ls-files on each submodule in the repository.
+ Currently there is only support for the --cached mode.
+
--abbrev[=<n>]::
Instead of showing the full 40-byte hexadecimal object
lines, show only a partial prefix.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 00ea91a..d4bfc60 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
#include "resolve-undo.h"
#include "string-list.h"
#include "pathspec.h"
+#include "run-command.h"
static int abbrev;
static int show_deleted;
@@ -28,6 +29,8 @@ static int show_valid_bit;
static int line_terminator = '\n';
static int debug_mode;
static int show_eol;
+static int recurse_submodules;
+static const char *submodule_prefix;
static const char *prefix;
static int max_prefix_len;
@@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
static void write_name(const char *name)
{
/*
+ * NEEDSWORK: To make this thread-safe, full_name would have to be owned
+ * by the caller.
+ *
+ * full_name get reused across output lines to minimize the allocation
+ * churn.
+ */
+ static struct strbuf full_name = STRBUF_INIT;
+ if (submodule_prefix && *submodule_prefix) {
+ strbuf_reset(&full_name);
+ strbuf_addstr(&full_name, submodule_prefix);
+ strbuf_addstr(&full_name, name);
+ name = full_name.buf;
+ }
+
+ /*
* With "--full-name", prefix_len=0; this caller needs to pass
* an empty string in that case (a NULL is good for "").
*/
@@ -152,55 +170,84 @@ static void show_killed_files(struct dir_struct *dir)
}
}
+/**
+ * Recursively call ls-files on a submodule
+ */
+static void show_gitlink(const struct cache_entry *ce)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ int status;
+
+ argv_array_pushf(&cp.args, "--submodule-prefix=%s%s/",
+ submodule_prefix ? submodule_prefix : "",
+ ce->name);
+ argv_array_push(&cp.args, "ls-files");
+ argv_array_push(&cp.args, "--recurse-submodules");
+
+ cp.git_cmd = 1;
+ cp.dir = ce->name;
+ status = run_command(&cp);
+ if (status)
+ exit(status);
+}
+
static void show_ce_entry(const char *tag, const struct cache_entry *ce)
{
+ struct strbuf name = STRBUF_INIT;
int len = max_prefix_len;
+ if (submodule_prefix)
+ strbuf_addstr(&name, submodule_prefix);
+ strbuf_addstr(&name, ce->name);
if (len >= ce_namelen(ce))
die("git ls-files: internal error - cache entry not superset of prefix");
- if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
- len, ps_matched,
- S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
- return;
+ if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+ show_gitlink(ce);
+ } else if (match_pathspec(&pathspec, name.buf, name.len,
+ len, ps_matched,
+ S_ISDIR(ce->ce_mode) ||
+ S_ISGITLINK(ce->ce_mode))) {
+ if (tag && *tag && show_valid_bit &&
+ (ce->ce_flags & CE_VALID)) {
+ static char alttag[4];
+ memcpy(alttag, tag, 3);
+ if (isalpha(tag[0]))
+ alttag[0] = tolower(tag[0]);
+ else if (tag[0] == '?')
+ alttag[0] = '!';
+ else {
+ alttag[0] = 'v';
+ alttag[1] = tag[0];
+ alttag[2] = ' ';
+ alttag[3] = 0;
+ }
+ tag = alttag;
+ }
- if (tag && *tag && show_valid_bit &&
- (ce->ce_flags & CE_VALID)) {
- static char alttag[4];
- memcpy(alttag, tag, 3);
- if (isalpha(tag[0]))
- alttag[0] = tolower(tag[0]);
- else if (tag[0] == '?')
- alttag[0] = '!';
- else {
- alttag[0] = 'v';
- alttag[1] = tag[0];
- alttag[2] = ' ';
- alttag[3] = 0;
+ if (!show_stage) {
+ fputs(tag, stdout);
+ } else {
+ printf("%s%06o %s %d\t",
+ tag,
+ ce->ce_mode,
+ find_unique_abbrev(ce->sha1,abbrev),
+ ce_stage(ce));
+ }
+ write_eolinfo(ce, ce->name);
+ write_name(ce->name);
+ if (debug_mode) {
+ const struct stat_data *sd = &ce->ce_stat_data;
+
+ printf(" ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
+ printf(" mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
+ printf(" dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
+ printf(" uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
+ printf(" size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
}
- tag = alttag;
}
- if (!show_stage) {
- fputs(tag, stdout);
- } else {
- printf("%s%06o %s %d\t",
- tag,
- ce->ce_mode,
- find_unique_abbrev(ce->sha1,abbrev),
- ce_stage(ce));
- }
- write_eolinfo(ce, ce->name);
- write_name(ce->name);
- if (debug_mode) {
- const struct stat_data *sd = &ce->ce_stat_data;
-
- printf(" ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
- printf(" mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
- printf(" dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
- printf(" uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
- printf(" size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
- }
+ strbuf_release(&name);
}
static void show_ru_info(void)
@@ -468,6 +515,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
N_("make the output relative to the project top directory"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
+ OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+ N_("recurse through submodules")),
OPT_BOOL(0, "error-unmatch", &error_unmatch,
N_("if any <file> is not in the index, treat this as an error")),
OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
@@ -519,6 +568,24 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
if (require_work_tree && !is_inside_work_tree())
setup_work_tree();
+ if (recurse_submodules)
+ submodule_prefix = getenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT);
+
+ if (recurse_submodules &&
+ (show_stage || show_deleted || show_others || show_unmerged ||
+ show_killed || show_modified || show_resolve_undo ||
+ show_valid_bit || show_tag || show_eol || with_tree ||
+ (line_terminator == '\0')))
+ die("ls-files --recurse-submodules unsupported mode");
+
+ if (recurse_submodules && error_unmatch)
+ die("ls-files --recurse-submodules does not support "
+ "--error-unmatch");
+
+ if (recurse_submodules && argc)
+ die("ls-files --recurse-submodules does not support path "
+ "arguments");
+
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
diff --git a/git.c b/git.c
index b2b096a..510b6a2 100644
--- a/git.c
+++ b/git.c
@@ -440,7 +440,7 @@ static struct cmd_struct commands[] = {
{ "init-db", cmd_init_db },
{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
{ "log", cmd_log, RUN_SETUP },
- { "ls-files", cmd_ls_files, RUN_SETUP },
+ { "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUBMODULES },
{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
{ "ls-tree", cmd_ls_tree, RUN_SETUP },
{ "mailinfo", cmd_mailinfo },
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
new file mode 100755
index 0000000..7d225ac
--- /dev/null
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -0,0 +1,100 @@
+#!/bin/sh
+
+test_description='Test ls-files recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly lists files from
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodules' '
+ echo a >a &&
+ mkdir b &&
+ echo b >b/b &&
+ git add a b &&
+ git commit -m "add a and b" &&
+ git init submodule &&
+ echo c >submodule/c &&
+ git -C submodule add c &&
+ git -C submodule commit -m "add c" &&
+ git submodule add ./submodule &&
+ git commit -m "added submodule"
+'
+
+test_expect_success 'ls-files correctly outputs files in submodule' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ls-files does not output files not added to a repo' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ echo a >not_added &&
+ echo b >b/not_added &&
+ echo c >submodule/not_added &&
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'ls-files recurses more than 1 level' '
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/.gitmodules
+ submodule/c
+ submodule/subsub/d
+ EOF
+
+ git init submodule/subsub &&
+ echo d >submodule/subsub/d &&
+ git -C submodule/subsub add d &&
+ git -C submodule/subsub commit -m "add d" &&
+ git -C submodule submodule add ./subsub &&
+ git -C submodule commit -m "added subsub" &&
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules does not support using path arguments' '
+ test_must_fail git ls-files --recurse-submodules b 2>actual &&
+ test_i18ngrep "does not support path arguments" actual
+'
+
+test_expect_success '--recurse-submodules does not support --error-unmatch' '
+ test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
+ test_i18ngrep "does not support --error-unmatch" actual
+'
+
+test_incompatible_with_recurse_submodules () {
+ test_expect_success "--recurse-submodules and $1 are incompatible" "
+ test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+ test_i18ngrep 'unsupported mode' actual
+ "
+}
+
+test_incompatible_with_recurse_submodules -z
+test_incompatible_with_recurse_submodules -v
+test_incompatible_with_recurse_submodules -t
+test_incompatible_with_recurse_submodules --deleted
+test_incompatible_with_recurse_submodules --modified
+test_incompatible_with_recurse_submodules --others
+test_incompatible_with_recurse_submodules --stage
+test_incompatible_with_recurse_submodules --killed
+test_incompatible_with_recurse_submodules --unmerged
+test_incompatible_with_recurse_submodules --eol
+
+test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 1/4 v4] submodules: make submodule-prefix option
From: Brandon Williams @ 2016-09-26 22:46 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <1474930003-83750-1-git-send-email-bmwill@google.com>
Add a submodule-prefix environment variable
'GIT_INTERNAL_SUBMODULE_PREFIX' which can be used by commands which have
--recurse-submodule options to give context to submodules about how they
were invoked. This option is only allowed for builtins which have
submodule support.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/git.txt | 5 +++++
cache.h | 1 +
environment.c | 1 +
git.c | 19 +++++++++++++++++++
4 files changed, 26 insertions(+)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7913fc2..d29967a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -13,6 +13,7 @@ SYNOPSIS
[--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
[-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
[--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
+ [--submodule-prefix=<path>]
<command> [<args>]
DESCRIPTION
@@ -601,6 +602,10 @@ foo.bar= ...`) sets `foo.bar` to the empty string.
details. Equivalent to setting the `GIT_NAMESPACE` environment
variable.
+--submodule-prefix=<path>::
+ Set a prefix which gives submodules context about the superproject that
+ invoked it. Only allowed for commands which support submodules.
+
--bare::
Treat the repository as a bare repository. If GIT_DIR
environment is not set, it is set to the current working
diff --git a/cache.h b/cache.h
index 3556326..ae88a35 100644
--- a/cache.h
+++ b/cache.h
@@ -408,6 +408,7 @@ static inline enum object_type object_type(unsigned int mode)
#define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
#define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
#define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
+#define GIT_SUBMODULE_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUBMODULE_PREFIX"
#define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
#define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
#define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
diff --git a/environment.c b/environment.c
index ca72464..7380815 100644
--- a/environment.c
+++ b/environment.c
@@ -120,6 +120,7 @@ const char * const local_repo_env[] = {
NO_REPLACE_OBJECTS_ENVIRONMENT,
GIT_REPLACE_REF_BASE_ENVIRONMENT,
GIT_PREFIX_ENVIRONMENT,
+ GIT_SUBMODULE_PREFIX_ENVIRONMENT,
GIT_SHALLOW_FILE_ENVIRONMENT,
GIT_COMMON_DIR_ENVIRONMENT,
NULL
diff --git a/git.c b/git.c
index 1c61151..b2b096a 100644
--- a/git.c
+++ b/git.c
@@ -164,6 +164,20 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
setenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1);
if (envchanged)
*envchanged = 1;
+ } else if (!strcmp(cmd, "--submodule-prefix")) {
+ if (*argc < 2) {
+ fprintf(stderr, "No prefix given for --submodule-prefix.\n" );
+ usage(git_usage_string);
+ }
+ setenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT, (*argv)[1], 1);
+ if (envchanged)
+ *envchanged = 1;
+ (*argv)++;
+ (*argc)--;
+ } else if (skip_prefix(cmd, "--submodule-prefix=", &cmd)) {
+ setenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT, cmd, 1);
+ if (envchanged)
+ *envchanged = 1;
} else if (!strcmp(cmd, "--bare")) {
char *cwd = xgetcwd();
is_bare_repository_cfg = 1;
@@ -310,6 +324,7 @@ static int handle_alias(int *argcp, const char ***argv)
* RUN_SETUP for reading from the configuration file.
*/
#define NEED_WORK_TREE (1<<3)
+#define SUPPORT_SUBMODULES (1<<4)
struct cmd_struct {
const char *cmd;
@@ -344,6 +359,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
}
commit_pager_choice();
+ if (!help && (getenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT) &&
+ !(p->option & SUPPORT_SUBMODULES)))
+ die("%s doesn't support submodules", p->cmd);
+
if (!help && p->option & NEED_WORK_TREE)
setup_work_tree();
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 0/4 v4] recursive support for ls-files
From: Brandon Williams @ 2016-09-26 22:46 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <1474676014-134568-1-git-send-email-bmwill@google.com>
A couple things have changed in v4:
- Restructured the patch series to prevent a breakage mid-way.
- Added an additional patch in the middle to pass through safe options. This
way the series is structured in a more coherent manor.
- Added --submodule-prefix to top-level git.c
Hopefully this series addresses some of issues brought up in v3
Brandon Williams (4):
submodules: make submodule-prefix option
ls-files: optionally recurse into submodules
ls-files: pass through safe options for --recurse-submodules
ls-files: add pathspec matching for submodules
Documentation/git-ls-files.txt | 7 +-
Documentation/git.txt | 5 +
builtin/ls-files.c | 187 ++++++++++++++++++++++-------
cache.h | 1 +
dir.c | 46 +++++++-
dir.h | 4 +
environment.c | 1 +
git.c | 21 +++-
t/t3007-ls-files-recurse-submodules.sh | 209 +++++++++++++++++++++++++++++++++
9 files changed, 437 insertions(+), 44 deletions(-)
create mode 100755 t/t3007-ls-files-recurse-submodules.sh
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* [PATCH v2 4/5] builtin/verify-tag: add --format to verify-tag
From: santiago @ 2016-09-26 22:42 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres
In-Reply-To: <20160926224233.32702-1-santiago@nyu.edu>
From: Santiago Torres <santiago@nyu.edu>
Callers of verify-tag may want to cross-check the tagname from refs/tags
with the tagname from the tag object header upon GPG verification. This
is to avoid tag refs that point to an incorrect object.
Add a --format parameter to git verify-tag to print the formatted tag
object header in addition to or instead of the --verbose or --raw GPG
verification output.
Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
builtin/verify-tag.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index de10198..a941053 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -12,12 +12,15 @@
#include <signal.h>
#include "parse-options.h"
#include "gpg-interface.h"
+#include "ref-filter.h"
static const char * const verify_tag_usage[] = {
- N_("git verify-tag [-v | --verbose] <tag>..."),
+ N_("git verify-tag [-v | --verbose] [--format=<format>] <tag>..."),
NULL
};
+static char *fmt_pretty;
+
static int git_verify_tag_config(const char *var, const char *value, void *cb)
{
int status = git_gpg_config(var, value, cb);
@@ -33,6 +36,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
const struct option verify_tag_options[] = {
OPT__VERBOSE(&verbose, N_("print tag contents")),
OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
+ OPT_STRING( 0 , "format", &fmt_pretty, N_("format"), N_("format to use for the output")),
OPT_END()
};
@@ -46,12 +50,17 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
if (verbose)
flags |= GPG_VERIFY_VERBOSE;
+ if (fmt_pretty) {
+ verify_ref_format(fmt_pretty);
+ flags |= GPG_VERIFY_QUIET;
+ }
+
while (i < argc) {
unsigned char sha1[20];
const char *name = argv[i++];
if (get_sha1(name, sha1))
had_error = !!error("tag '%s' not found.", name);
- else if (verify_and_format_tag(sha1, name, NULL, flags))
+ else if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
had_error = 1;
}
return had_error;
--
2.10.0
^ permalink raw reply related
* [PATCH v2 5/5] builtin/tag: add --format argument for tag -v
From: santiago @ 2016-09-26 22:42 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas P
In-Reply-To: <20160926224233.32702-1-santiago@nyu.edu>
From: Lukas P <luk.puehringer@gmail.com>
Adding --format to git tag -v mutes the default output of the GPG
verification and instead prints the formatted tag object.
This allows callers to cross-check the tagname from refs/tags with
the tagname from the tag object header upon GPG verification.
Caveat: The change adds a format specifier argument to the
(*each_tag_name_fn) function pointer, i.e. delete_tag now receives this
too, although it does not need it.
Signed-off-by: Lukas P <luk.puehringer@gmail.com>
---
builtin/tag.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 14f3b48..f53227e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -24,7 +24,7 @@ static const char * const git_tag_usage[] = {
N_("git tag -d <tagname>..."),
N_("git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>]"
"\n\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
- N_("git tag -v <tagname>..."),
+ N_("git tag -v [--format=<format>] <tagname>..."),
NULL
};
@@ -66,9 +66,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
}
typedef int (*each_tag_name_fn)(const char *name, const char *ref,
- const unsigned char *sha1);
+ const unsigned char *sha1, const char *fmt_pretty);
-static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
+static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
+ const char *fmt_pretty)
{
const char **p;
char ref[PATH_MAX];
@@ -87,14 +88,14 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
had_error = 1;
continue;
}
- if (fn(*p, ref, sha1))
+ if (fn(*p, ref, sha1, fmt_pretty))
had_error = 1;
}
return had_error;
}
static int delete_tag(const char *name, const char *ref,
- const unsigned char *sha1)
+ const unsigned char *sha1, const char *fmt_pretty)
{
if (delete_ref(ref, sha1, 0))
return 1;
@@ -103,9 +104,15 @@ static int delete_tag(const char *name, const char *ref,
}
static int verify_tag(const char *name, const char *ref,
- const unsigned char *sha1)
+ const unsigned char *sha1, const char *fmt_pretty)
{
- return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
+ int flags;
+ flags = GPG_VERIFY_VERBOSE;
+
+ if (fmt_pretty)
+ flags = GPG_VERIFY_QUIET;
+
+ return verify_and_format_tag(sha1, name, fmt_pretty, flags);
}
static int do_sign(struct strbuf *buffer)
@@ -424,9 +431,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (filter.merge_commit)
die(_("--merged and --no-merged option are only allowed with -l"));
if (cmdmode == 'd')
- return for_each_tag_name(argv, delete_tag);
- if (cmdmode == 'v')
- return for_each_tag_name(argv, verify_tag);
+ return for_each_tag_name(argv, delete_tag, NULL);
+ if (cmdmode == 'v') {
+ if (format)
+ verify_ref_format(format);
+ return for_each_tag_name(argv, verify_tag, format);
+ }
if (msg.given || msgfile) {
if (msg.given && msgfile)
--
2.10.0
^ permalink raw reply related
* [PATCH v2 3/5] tag: add format specifier to gpg_verify_tag
From: santiago @ 2016-09-26 22:42 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas P
In-Reply-To: <20160926224233.32702-1-santiago@nyu.edu>
From: Lukas P <luk.puehringer@gmail.com>
Calling functions for gpg_verify_tag() may desire to print relevant
information about the header for further verification. Add an optional
format argument to print any desired information after GPG verification.
Signed-off-by: Lukas P <luk.puehringer@gmail.com>
---
builtin/tag.c | 2 +-
builtin/verify-tag.c | 2 +-
tag.c | 17 +++++++++++------
tag.h | 4 ++--
4 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 50e4ae5..14f3b48 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -105,7 +105,7 @@ static int delete_tag(const char *name, const char *ref,
static int verify_tag(const char *name, const char *ref,
const unsigned char *sha1)
{
- return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);
+ return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
}
static int do_sign(struct strbuf *buffer)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 99f8148..de10198 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -51,7 +51,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
const char *name = argv[i++];
if (get_sha1(name, sha1))
had_error = !!error("tag '%s' not found.", name);
- else if (gpg_verify_tag(sha1, name, flags))
+ else if (verify_and_format_tag(sha1, name, NULL, flags))
had_error = 1;
}
return had_error;
diff --git a/tag.c b/tag.c
index 291073f..65e1a96 100644
--- a/tag.c
+++ b/tag.c
@@ -4,6 +4,7 @@
#include "tree.h"
#include "blob.h"
#include "gpg-interface.h"
+#include "ref-filter.h"
const char *tag_type = "tag";
@@ -33,8 +34,8 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
return ret;
}
-int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
- unsigned flags)
+int verify_and_format_tag(const unsigned char *sha1, const char *name,
+ const char *fmt_pretty, unsigned flags)
{
enum object_type type;
char *buf;
@@ -44,21 +45,25 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
type = sha1_object_info(sha1, NULL);
if (type != OBJ_TAG)
return error("%s: cannot verify a non-tag object of type %s.",
- name_to_report ?
- name_to_report :
+ name ?
+ name :
find_unique_abbrev(sha1, DEFAULT_ABBREV),
typename(type));
buf = read_sha1_file(sha1, &type, &size);
if (!buf)
return error("%s: unable to read file.",
- name_to_report ?
- name_to_report :
+ name ?
+ name :
find_unique_abbrev(sha1, DEFAULT_ABBREV));
ret = run_gpg_verify(buf, size, flags);
free(buf);
+
+ if (fmt_pretty)
+ format_ref(name, sha1, fmt_pretty, FILTER_REFS_TAGS);
+
return ret;
}
diff --git a/tag.h b/tag.h
index a5721b6..0b6e458 100644
--- a/tag.h
+++ b/tag.h
@@ -17,7 +17,7 @@ extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long si
extern int parse_tag(struct tag *item);
extern struct object *deref_tag(struct object *, const char *, int);
extern struct object *deref_tag_noverify(struct object *);
-extern int gpg_verify_tag(const unsigned char *sha1,
- const char *name_to_report, unsigned flags);
+extern int verify_and_format_tag(const unsigned char *sha1, const char *name,
+ const char *fmt_pretty, unsigned flags);
#endif /* TAG_H */
--
2.10.0
^ permalink raw reply related
* [PATCH v2 2/5] ref-filter: add function to print single ref_array_item
From: santiago @ 2016-09-26 22:42 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas P
In-Reply-To: <20160926224233.32702-1-santiago@nyu.edu>
From: Lukas P <luk.puehringer@gmail.com>
ref-filter functions are useful for printing git object information
using a format specifier. However, some other modules may not want to use
this functionality on a ref-array but only print a single item.
Expose a format_ref function to create, pretty print and free individual
ref-items.
Signed-off-by: Lukas P <luk.puehringer@gmail.com>
---
ref-filter.c | 10 ++++++++++
ref-filter.h | 4 ++++
2 files changed, 14 insertions(+)
diff --git a/ref-filter.c b/ref-filter.c
index bc551a7..e0aaf5f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1655,6 +1655,16 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
putchar('\n');
}
+void format_ref(const char *name, const unsigned char *sha1, const char *format,
+ unsigned kind)
+{
+ struct ref_array_item *ref_item;
+ ref_item = new_ref_array_item(name, sha1, 0);
+ ref_item->kind = kind;
+ show_ref_array_item(ref_item, format, 0);
+ free_array_item(ref_item);
+}
+
/* If no sorting option is given, use refname to sort as default */
struct ref_sorting *ref_default_sorting(void)
{
diff --git a/ref-filter.h b/ref-filter.h
index 14d435e..1ef7999 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -107,4 +107,8 @@ struct ref_sorting *ref_default_sorting(void);
/* Function to parse --merged and --no-merged options */
int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
+/* Pretty-print a single ref */
+void format_ref(const char *name, const unsigned char *sha1, const char *format,
+ unsigned kind);
+
#endif /* REF_FILTER_H */
--
2.10.0
^ permalink raw reply related
* [PATCH v2 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: santiago @ 2016-09-26 22:42 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas P
In-Reply-To: <20160926224233.32702-1-santiago@nyu.edu>
From: Lukas P <luk.puehringer@gmail.com>
Functions that print git object information may require that the
gpg-interface functions be silent. Add GPG_VERIFY_QUIET flag and prevent
print_signature_buffer from being called if flag is set.
Signed-off-by: Lukas P <luk.puehringer@gmail.com>
---
gpg-interface.h | 1 +
tag.c | 5 ++++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/gpg-interface.h b/gpg-interface.h
index ea68885..85dc982 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -3,6 +3,7 @@
#define GPG_VERIFY_VERBOSE 1
#define GPG_VERIFY_RAW 2
+#define GPG_VERIFY_QUIET 4
struct signature_check {
char *payload;
diff --git a/tag.c b/tag.c
index d1dcd18..291073f 100644
--- a/tag.c
+++ b/tag.c
@@ -3,6 +3,7 @@
#include "commit.h"
#include "tree.h"
#include "blob.h"
+#include "gpg-interface.h"
const char *tag_type = "tag";
@@ -24,7 +25,9 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
ret = check_signature(buf, payload_size, buf + payload_size,
size - payload_size, &sigc);
- print_signature_buffer(&sigc, flags);
+
+ if (!(flags & GPG_VERIFY_QUIET))
+ print_signature_buffer(&sigc, flags);
signature_check_clear(&sigc);
return ret;
--
2.10.0
^ permalink raw reply related
* [PATCH v2 0/5] Add --format to tag verification
From: santiago @ 2016-09-26 22:42 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres
From: Santiago Torres <santiago@nyu.edu>
This is the second iteration of [1], and as a result of the discussion
in [2].
In this re-roll we:
* Dropped the commit to move the format string parameter to a global
variable on builtin/tag. We had to change the signature of
for_each_name_fn to do this.
* Fixed the signed-off-by lines to match the author/committer
[0001]
* Moved the GPG_VERIFY_QUIET flag check into tag.c instead of
gpg_interface
[0002]
* Changed the exported function to "format_ref". This way, we can print
a single formatted ref element. This also made the patch
cleaner/easier to read.
[0003]
* We fixed style/formatting errors that we had introduced
This patch applies to 2.10.0 and master.
[1] http://public-inbox.org/git/20160922185317.349-1-santiago@nyu.edu/
[2] http://public-inbox.org/git/20160607195608.16643-1-santiago@nyu.edu/
Lukas P (4):
gpg-interface, tag: add GPG_VERIFY_QUIET flag
ref-filter: add function to print single ref_array_item
tag: add format specifier to gpg_verify_tag
builtin/tag: add --format argument for tag -v
Santiago Torres (1):
builtin/verify-tag: add --format to verify-tag
builtin/tag.c | 30 ++++++++++++++++++++----------
builtin/verify-tag.c | 16 ++++++++++++++--
gpg-interface.h | 1 +
ref-filter.c | 10 ++++++++++
ref-filter.h | 4 ++++
tag.c | 22 +++++++++++++++-------
tag.h | 4 ++--
7 files changed, 66 insertions(+), 21 deletions(-)
--
2.10.0
^ permalink raw reply
* Re: [PATCH v8 11/11] convert: add filter.<driver>.process option
From: Jakub Narębski @ 2016-09-26 22:41 UTC (permalink / raw)
To: Lars Schneider, git
Cc: Jeff King, Junio C Hamano, Stefan Beller, Martin-Louis Bright,
Torsten Bögershausen, Ramsay Jones
In-Reply-To: <20160920190247.82189-12-larsxschneider@gmail.com>
Part first of the review of 11/11.
W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
> From: Lars Schneider <larsxschneider@gmail.com>
>
> Git's clean/smudge mechanism invokes an external filter process for
> every single blob that is affected by a filter. If Git filters a lot of
> blobs then the startup time of the external filter processes can become
> a significant part of the overall Git execution time.
>
> In a preliminary performance test this developer used a clean/smudge
> filter written in golang to filter 12,000 files. This process took 364s
> with the existing filter mechanism and 5s with the new mechanism. See
> details here: https://github.com/github/git-lfs/pull/1382
>
> This patch adds the `filter.<driver>.process` string option which, if
> used, keeps the external filter process running and processes all blobs
> with the packet format (pkt-line) based protocol over standard input and
> standard output. The full protocol is explained in detail in
> `Documentation/gitattributes.txt`.
That is a good description. Enough detail to explain the new feature,
all without duplicating information with (added) docs.
>
> A few key decisions:
>
> * The long running filter process is referred to as filter protocol
> version 2 because the existing single shot filter invocation is
> considered version 1.
All right.
> * Git sends a welcome message and expects a response right after the
> external filter process has started. This ensures that Git will not
> hang if a version 1 filter is incorrectly used with the
> filter.<driver>.process option for version 2 filters. In addition,
> Git can detect this kind of error and warn the user.
On one hand side, this involved handshake means that implementing
a filter process script is harder; you need to write quite a lot of
boilerplate (though the example or examples would help).
On the other hand, this handshake is what allows good error detection,
easy extendability of the protocol, and forward-compatibility. Which,
as we agreed (AFAIU), is more important.
> * The status of a filter operation (e.g. "success" or "error) is set
> before the actual response and (if necessary!) re-set after the
> response. The advantage of this two step status response is that if
> the filter detects an error early, then the filter can communicate
> this and Git does not even need to create structures to read the
> response.
That's nice (well, among others I have argued for this :-))
> * All status responses are pkt-line lists terminated with a flush
> packet. This allows us to send other status fields with the same
> protocol in the future.
Good.
This also makes protocol simple, easier to implement (on Git side),
and easier to parse (on filter side).
>
> Helped-by: Martin-Louis Bright <mlbright@gmail.com>
> Reviewed-by: Jakub Narebski <jnareb@gmail.com>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> Documentation/gitattributes.txt | 156 +++++++++++++-
> contrib/long-running-filter/example.pl | 123 +++++++++++
> convert.c | 348 ++++++++++++++++++++++++++++---
> pkt-line.h | 1 +
> t/t0021-conversion.sh | 365 ++++++++++++++++++++++++++++++++-
> t/t0021/rot13-filter.pl | 191 +++++++++++++++++
> 6 files changed, 1153 insertions(+), 31 deletions(-)
That's quite a large change. Large changes are harder to review.
I was thinking about how one could split this change. I guess
that it is better to keep the new feature, its documentation, and
its tests together. But perhaps the example in `contrib/`
(which is newer, and thus less reviewed) would be better in
separate commit.
There is also another change that could be split off this patch
into purely preparatory commit, that is one that stands alone
but doesn't make much sense alone. I would write about this
proposal (important only if there would be yet another iteration
of this patch series) a bit later.
> create mode 100755 contrib/long-running-filter/example.pl
> create mode 100755 t/t0021/rot13-filter.pl
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 7aff940..946dcad 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -293,7 +293,13 @@ checkout, when the `smudge` command is specified, the command is
> fed the blob object from its standard input, and its standard
> output is used to update the worktree file. Similarly, the
> `clean` command is used to convert the contents of worktree file
> -upon checkin.
> +upon checkin. By default these commands process only a single
> +blob and terminate. If a long running `process` filter is used
^^^^
Should we use this terminology here? I have not read the preceding
part of documentation, so I don't know if it talks about "blobs" or
if it uses "files" and/or "file contents".
Though this is very minor nitpick.
> +in place of `clean` and/or `smudge` filters, then Git can process
> +all blobs with a single filter command invocation for the entire
> +life of a single Git command, for example `git add --all`. See
> +section below for the description of the protocol used to
> +communicate with a `process` filter.
Good introduction of long lived filter feature (`process` filter).
>
> One use of the content filtering is to massage the content into a shape
> that is more convenient for the platform, filesystem, and the user to use.
> @@ -373,6 +379,154 @@ not exist, or may have different contents. So, smudge and clean commands
> should not try to access the file on disk, but only act as filters on the
> content provided to them on standard input.
>
> +Long Running Filter Process
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +If the filter command (a string value) is defined via
> +`filter.<driver>.process` then Git can process all blobs with a
> +single filter invocation for the entire life of a single Git
> +command. This is achieved by using a packet format (pkt-line,
> +see technical/protocol-common.txt) based protocol over standard
> +input and standard output as follows. All packets are considered
> +text and therefore are terminated by an LF. Exceptions are the
> +"*CONTENT" packets and the flush packet.
I guess that reasoning here is that all but CONTENT packets are
metadata, and thus to aid debuggability of the protocol are "text",
as considered by pkt-line.
Perhaps a bit more readable would be the following (but current is
just fine; I am nitpicking):
All packets, except for the "{star}CONTENT" packets and the "0000"
flush packer, are considered text and therefore are terminated by
a LF.
Or maybe:
All metadata is considered text, and thus send as a text packet,
that is terminated with the newline (LF) character. The file
contents is send as binary data, as is, without appending LF.
The flush packet is a packet composed of 4 bytes, represented
in ASCII as "0000".
Anyway, this is all right as it is now; we can always polish it
later.
> +
> +Git starts the filter when it encounters the first file
> +that needs to be cleaned or smudged. After the filter started
> +Git sends a welcome message ("git-filter-client"), a list of
> +supported protocol version numbers, and a flush packet. Git expects
I see that example below explains what is the format of sending
this list of process filter protocol version numbers supported by
Git (I suppose it is to future proof adding new versions of protocol,
and removing old ones if they are somehow buggy). It would be nice,
I think, to explain it in more detail:
a list of supported protocol version numbers, each version in
a separate text packet using the "version=<n>" format,
But with an example few paragraphs below it might be not necessary.
I think it might be a good idea to describe what flush packet is
somewhere in this document; on the other hand referring (especially
if hyperlinked) to pkt-line technical documentation might be good
enough / better. I'm unsure, but I tend on the side that referring
to technical documentation is better.
> +to read a welcome response message ("git-filter-server") and exactly
> +one protocol version number from the previously sent list. All further
I guess that is to provide forward-compatibility, isn't it? Also,
"Git expects..." probably means filter process MUST send, in the
RFC2119 (https://tools.ietf.org/html/rfc2119) meaning.
> +communication will be based on the selected version. The remaining
> +protocol description below documents "version=2". Please note that
> +"version=42" in the example below does not exist and is only there
> +to illustrate how the protocol would look like with more than one
> +version.
All right.
> +
> +After the version negotiation Git sends a list of supported capabilities
> +and a flush packet.
Is it that Git SHOULD send list of ALL supported capabilities, or is
it that Git SHOULD NOT send capabilities it does not support, and that
it MAY send only those capabilities it needs (so for example if command
uses only `smudge`, it may not send `clean`, so that filter driver doesn't
need to initialize data it would not need).
I guess with the example few lines below there is no need to explain
the format of capabilities (or use BNF / EBNF notation to define it).
I wonder why it is "<capability>=true", and not "capability=<capability>".
Is there a case where we would want to send "<capability>=false". Or
is it to allow configurable / value based capabilities? Isn't it going
a bit too far: is there even a hind of an idea for parametrize-able
capability? YAGNI is a thing...
A few new capabilities that we might want to support in the near future
is "size", "stream", which are options describing how to communicate,
and "cleanFromFile", "smudgeToFile", which are new types of operations...
but neither needs any parameter.
I guess that adding new capabilities doesn't require having to come up
with the new version of the protocol, isn't it.
> Git expects to read a list of desired capabilities,
> +which must be a subset of the supported capabilities list, and a flush
> +packet as response:
All right, with Git speaking first, having Git provide list of supported
capabilities first is quite natural.
> +------------------------
> +packet: git> git-filter-client
I guess we assume that from the above description it is obvious that
this is
+packet: git> git-filter-client\n
All right.
> +packet: git> version=2
> +packet: git> version=42
"List" means "each in separate packet", right. Here also
+packet: git> version=2\n
> +packet: git> 0000
As I wrote, I hope everybody would understand that is a flush packet,
that is packet composed literally of 4 characters / bytes "0000",
and not binary or text packet with "0000" as contents, that is
"00040000" packet or "00050000\n" packet.
But as it is consistent with other examples, and with GIT_TRACE_PACKET
output, I think both skipping trailing \n for text packets, and
writing "0000" for flush packet is all right. Sorry for the noise.
Sidenote (you don't have to answer to): do we use "0004" packet as
a keep-alive packet anywhere?
> +packet: git< git-filter-server
> +packet: git< version=2
> +packet: git> clean=true
> +packet: git> smudge=true
> +packet: git> not-yet-invented=true
Hmmm... should we hint at the use of kebab-case versus snake_case
or camelCase for new capabilities?
> +packet: git> 0000
> +packet: git< clean=true
> +packet: git< smudge=true
> +packet: git< 0000
> +------------------------
> +Supported filter capabilities in version 2 are "clean" and
> +"smudge".
I think we can add new capabilities without increasing version number
of the protocol. But then we can and should update this part of the
documentation.
All right.
> +
> +Afterwards Git sends a list of "key=value" pairs terminated with
> +a flush packet. The list will contain at least the filter command
> +(based on the supported capabilities) and the pathname of the file
> +to filter relative to the repository root. Right after these packets
> +Git sends the content split in zero or more pkt-line packets and a
> +flush packet to terminate content.
All right, the example below shows what are the names of 'variables;
in those packets (that is "command=" ( "clean" | "smudge" ), and
"pathname=" <pathname>).
> +------------------------
> +packet: git> command=smudge
> +packet: git> pathname=path/testfile.dat
> +packet: git> 0000
> +packet: git> CONTENT
> +packet: git> 0000
> +------------------------
I think it is important to mention that (at least with current
`filter.<driver>.process` implementation, that is absent future
"stream" capability / option) the filter process needs to read
*whole contents* at once, *before* writing anything. Otherwise
it can lead to deadlock.
This is especially important in that it is different (!) from the
current behavior of `clean` and `smudge` filters, which can
stream their response because Git invokes them async.
> +
> +The filter is expected to respond with a list of "key=value" pairs
> +terminated with a flush packet. If the filter does not experience
> +problems then the list must contain a "success" status.
Perhaps "status" packet with value "success", or
If the filter does not experience
+problems then the list must contain a "status=success" line.
Possibly s/line./packet./
But as I see with how it is explained further, 'a "success" status'
works too. No need to change, then.
> Right after
> +these packets the filter is expected to send the content in zero
> +or more pkt-line packets and a flush packet at the end. Finally, a
> +second list of "key=value" pairs terminated with a flush packet
> +is expected. The filter can change the status in the second list.
> +------------------------
> +packet: git< status=success
> +packet: git< 0000
> +packet: git< SMUDGED_CONTENT
> +packet: git< 0000
> +packet: git< 0000 # empty list!
> +------------------------
All right. Empty list with no change in status looks good.
An alternative would be to assume different meaning to the "status"
before and after sending contents, e.g.
packet: git< received=ok
packet: git< 0000
packet: git< SMUDGED_CONTENT
packet: git< 0000
packet: git< sent=ok
packet: git< 0000
But I think current solution is good enough.
> +
> +If the result content is empty then the filter is expected to respond
> +with a success status and an empty list.
> +------------------------
> +packet: git< status=success
> +packet: git< 0000
> +packet: git< 0000 # empty content!
> +packet: git< 0000 # empty list!
> +------------------------
All right. This follows from the definition, but it is nice to have
it spelled in full.
> +
> +In case the filter cannot or does not want to process the content,
> +it is expected to respond with an "error" status. Depending on the
> +`filter.<driver>.required` flag Git will interpret that as error
> +but it will not stop or restart the filter process.
Right, and Git would not try to read contents from the filter then.
> +------------------------
> +packet: git< status=error
> +packet: git< 0000
> +------------------------
> +
> +If the filter experiences an error during processing, then it can
> +send the status "error" after the content was (partially or
> +completely) sent. Depending on the `filter.<driver>.required` flag
> +Git will interpret that as error but it will not stop or restart the
> +filter process.
> +------------------------
> +packet: git< status=success
> +packet: git< 0000
> +packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT
> +packet: git< 0000
> +packet: git< status=error
> +packet: git< 0000
> +------------------------
Good. A question is if the filter process can send "status=abort"
after partial contents, or does it need to wait for the next command?
> +
> +If the filter dies during the communication or does not adhere to
> +the protocol then Git will stop the filter process and restart it
> +with the next file that needs to be processed. Depending on the
> +`filter.<driver>.required` flag Git will interpret that as error.
> +
> +The error handling for all cases above mimic the behavior of
> +the `filter.<driver>.clean` / `filter.<driver>.smudge` error
> +handling.
Good.
> +
> +In case the filter cannot or does not want to process the content
> +as well as any future content for the lifetime of the Git process,
> +it is expected to respond with an "abort" status. Depending on
> +the `filter.<driver>.required` flag Git will interpret that as error
> +for the content as well as any future content for the lifetime of the
> +Git process but it will not stop or restart the filter process.
> +------------------------
> +packet: git< status=abort
> +packet: git< 0000
> +------------------------
I assume this is obvious that if filter process tells "abort", Git
would not try to send further files (regardless of the value of
`filter.<driver>.required`).
> +
> +After the filter has processed a blob it is expected to wait for
> +the next "key=value" list containing a command. Git will close
> +the command pipe on exit. The filter is expected to detect EOF
> +and exit gracefully on its own.
Good to have it documented.
Anyway, as it is Git command that spawns the filter driver process,
assuming that the filter process doesn't daemonize itself, wouldn't
the operating system reap it after its parent process, that is the
git command it invoked, dies? So detecting EOF is good, but not
strictly necessary for simple filter that do not need to free
its resources, or can leave freeing resources to the operating
system? But I may be wrong here.
> +
> +A long running filter demo implementation can be found in
> +`contrib/long-running-filter/example.pl` located in the Git
> +core repository. If you develop your own long running filter
> +process then the `GIT_TRACE_PACKET` environment variables can be
> +very helpful for debugging (see linkgit:git[1]).
Very good... though I wonder if adding demo implementation should
not be left for a separate commit.
> +
> +If a `filter.<driver>.process` command is configured then it
> +always takes precedence over a configured `filter.<driver>.clean`
> +or `filter.<driver>.smudge` command.
This is a change from what I remember of previous iterations of this
patch series, but I see that it might be the best solution of the
three possible (that I can think of):
* `filter.<driver>.clean` or `filter.<driver>.smudge` command
always takes precedence over `filter.<driver>.process`
ADVANTAGES:
- can convert only half of filter to process filter
DISADVANTAGES:
- uncommon "older type wins"
- cannot provide fallback of one-shot filters for older Git
* `filter.<driver>.process` command always takes precedence
over `filter.<driver>.clean` or `filter.<driver>.smudge` command
(this is the one chosen)
ADVANTAGES:
- can provide `clean` and `smudge` filters as fallback for
older Git (e.g. installed locally, and not always in PATH)
DISADVANTAGES:
- need to convert both `clean` and `smudge` part into `process`
at once
* `filter.<driver>.process` command takes precedence over
`filter.<driver>.clean` if it supports "clean" capability, and
similarly for `filter.<driver>.smudge`
ADVANTAGES:
- can convert only half of filter to process filter
- can provide `clean` and `smudge` filters as fallback for
older Git (e.g. installed locally, and not always in PATH)
DISADVANTAGES:
- you need to see the filter implementation to know which
one would be invoked
- complicated to understand, reason about, and implement
> +
> +Please note that you cannot use an existing `filter.<driver>.clean`
> +or `filter.<driver>.smudge` command with `filter.<driver>.process`
> +because the former two use a different inter process communication
> +protocol than the latter one.
Good.
> +
> +
> Interaction between checkin/checkout attributes
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
> new file mode 100755
> index 0000000..c13a631
> --- /dev/null
> +++ b/contrib/long-running-filter/example.pl
To repeat myself, I think it would serve better as a separate patch.
Perhaps even you could add the example filter in the Go language
from https://github.com/github/git-lfs/pull/1382 (there is already
at least one Go program in contrib/). In a separate patch, of course.
The commands/command_filter.go.
> @@ -0,0 +1,123 @@
> +#!/usr/bin/perl
> +#
> +# Example implementation for the Git filter protocol version 2
> +# See Documentation/gitattributes.txt, section "Filter Protocol"
We might add that it is a pass-thru filter, a skeleton to be
modified.
> +#
> +
> +use strict;
> +use warnings;
> +
> +my $MAX_PACKET_CONTENT_SIZE = 65516;
All right, constants (the built in / in core ones) are strange...
Variables are easier to use.
> +
> +sub packet_bin_read {
> + my $buffer;
> + my $bytes_read = read STDIN, $buffer, 4;
> + if ( $bytes_read == 0 ) {
> +
> + # EOF - Git stopped talking to us!
> + exit();
> + }
> + elsif ( $bytes_read != 4 ) {
That is a different Perl coding convention that the one I am used to,
but it is no less valid. Consistent style is more important. This is
the contrib/ area anyway.
> + die "invalid packet size '$bytes_read' field";
This would read "invalid packet size '000' field", for example.
Perhaps the following would be (slightly) better:
+ die "invalid packet size field: '$bytes_read'";
> + }
> + my $pkt_size = hex($buffer);
> + if ( $pkt_size == 0 ) {
> + return ( 1, "" );
It feels a bit strange to me to return a list / a pair combining
status / error condition with the actual result. I'm more used
to packing such in hash reference. But I think it is all right.
> + }
> + elsif ( $pkt_size > 4 ) {
Isn't a packet of $pkt_size == 4 a valid packet, a keep-alive
one? Or is it forbidden?
We can declare that Git should not use it for filter process anyway.
> + my $content_size = $pkt_size - 4;
> + $bytes_read = read STDIN, $buffer, $content_size;
> + if ( $bytes_read != $content_size ) {
> + die "invalid packet ($content_size expected; $bytes_read read)";
This error message would read "invalid packet (12 expected; 10 read)";
I think it would be better to rephrase it as
+ die "invalid packet ($content_size bytes expected; $bytes_read bytes read)";
> + }
> + return ( 0, $buffer );
> + }
> + else {
> + die "invalid packet size";
I'm not sure if it is worth it (especially for the demo script),
but perhaps we could show what this invalid size was?
+ die "invalid packet size value '$pkt_size'";
> + }
> +}
> +
> +sub packet_txt_read {
> + my ( $res, $buf ) = packet_bin_read();
> + unless ( $buf =~ /\n$/ ) {
Wouldn't
+ unless ( $buf =~ s/\n$// ) {
or (less so)
+ unless ( $buf =~ s/\n$\z// ) {
be more idiomatic (and not require use of 'substr')? Remember,
the s/// substitution quote-like operator returns number of
substitutions in the scalar context.
> + die "A non-binary line SHOULD BE terminated by an LF.";
This is SHOULD be, not MUST be, so perhaps 'warn' would be enough.
Not that Git should send us such line.
> + }
> + return ( $res, substr( $buf, 0, -1 ) );
This would be not necessary if s/// instead of m// was used.
> +}
> +
> +sub packet_bin_write {
> + my ($packet) = @_;
This is equivalent to
+ my $packet = shift;
which, I think, is more common for single-parameter subroutines.
Also, this is $data (or $buf), not $packet.
> + print STDOUT sprintf( "%04x", length($packet) + 4 );
> + print STDOUT $packet;
> + STDOUT->flush();
> +}
> +
> +sub packet_txt_write {
> + packet_bin_write( $_[0] . "\n" );
> +}
Nice.
> +
> +sub packet_flush {
> + print STDOUT sprintf( "%04x", 0 );
We could use simply
+ print STDOUT "0000";
but this is more explicit. Good.
> + STDOUT->flush();
> +}
> +
Perhaps some comment that main begins here?
> +( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
> +( packet_txt_read() eq ( 0, "version=2" ) ) || die "bad version";
> +( packet_bin_read() eq ( 1, "" ) ) || die "bad version end";
Actually, it is overly strict. It should not fail if there
are other "version=3", "version=4" etc. lines.
> +
> +packet_txt_write("git-filter-server");
> +packet_txt_write("version=2");
It needs to do
+packet_flush();
here.
> +
> +( packet_txt_read() eq ( 0, "clean=true" ) ) || die "bad capability";
> +( packet_txt_read() eq ( 0, "smudge=true" ) ) || die "bad capability";
> +( packet_bin_read() eq ( 1, "" ) ) || die "bad capability end";
It is also overly strict. The capabilities can be ordered in any
way, and there can be additional capabilities which this script
do not understand. It is all right to have such capabilities.
All this makes it better to extract the handshake / metadata part
into a subroutine.
> +
> +packet_txt_write("clean=true");
> +packet_txt_write("smudge=true");
> +packet_flush();
All right.
> +
> +while (1) {
> + my ($command) = packet_txt_read() =~ /^command=([^=]+)$/;
> + my ($pathname) = packet_txt_read() =~ /^pathname=([^=]+)$/;
Do we require this order? If it is, is that explained in the
documentation?
> +
> + packet_bin_read();
I think there can be other auxiliary data (like "size=<n>") that
filter do not need to understand.
Anyway, using packet_bin_read() is quite unreadable. What you
mean is to wait until flush packet, or expect flush packet.
my $done = 0;
while ( !$done ) {
( $done, undef ) = packet_bin_read();
}
Or
wait_for_flush();
Or
skip_until_flush();
> +
> + my $input = "";
> + {
> + binmode(STDIN);
> + my $buffer;
> + my $done = 0;
> + while ( !$done ) {
> + ( $done, $buffer ) = packet_bin_read();
> + $input .= $buffer;
> + }
> + }
All right.
> +
> + my $output;
> + if ( $command eq "clean" ) {
> + ### Perform clean here ###
> + $output = $input;
Perhaps we should also mention here how to handle errors
(the "status=error" and "status=abort" both for upfront
and partial contents case).
> + }
> + elsif ( $command eq "smudge" ) {
> + ### Perform smudge here ###
> + $output = $input;
Same as above.
> + }
> + else {
> + die "bad command '$command'";
> + }
All right.
> +
> + packet_txt_write("status=success");
> + packet_flush();
> + while ( length($output) > 0 ) {
> + my $packet = substr( $output, 0, $MAX_PACKET_CONTENT_SIZE );
> + packet_bin_write($packet);
> + if ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {
> + $output = substr( $output, $MAX_PACKET_CONTENT_SIZE );
> + }
> + else {
> + $output = "";
> + }
> + }
> + packet_flush(); # flush content!
All right.
> + packet_flush(); # empty list!
This is less "empty list!", and more keeping "status=success" unchanged.
> +}
> diff --git a/convert.c b/convert.c
> index 597f561..bd66257 100644
> --- a/convert.c
> +++ b/convert.c
I'll stop here, and I'll finish the review later.
To be continued,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v3 2/2] mailinfo: unescape quoted-pair in header fields
From: Junio C Hamano @ 2016-09-26 22:23 UTC (permalink / raw)
To: Kevin Daudt; +Cc: git, Swift Geek, Jeff King
In-Reply-To: <20160926194455.GB19089@ikke.info>
Kevin Daudt <me@ikke.info> writes:
> On Mon, Sep 26, 2016 at 12:26:13PM -0700, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > Don't these also need to be downcased if you prefer $data over
>> > $DATA, though?
>>
>> For now, I'll queue a SQUASH??? that reverts s/DATA/data/ you did to
>> 1/2 between your 1/2 and 2/2.
>
> Ugh, thanks. I'd replaced it in the first patch, but forgot it in the
> second.
Heh, I already guessed that much that these were sent without even
be in the tree, after editing only the patch files. Don't do that
;-)
I am not sure I agree that $data is better over $DATA, though.
Unlike the lowercase $mail and others used in the script that are
clearly "variables", this thing is used as a constant during the
lifetime of the test script.
^ permalink raw reply
* Re: [PATCH] Documentation/fetch-options: emit recurse-submodules, jobs unconditionally
From: Brandon Williams @ 2016-09-26 22:18 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org, Jens Lehmann
In-Reply-To: <CAGZ79kbeq+Wznm=ChWO0tU5A_haPQ6DvKNHbK+8Y3es5OVcRag@mail.gmail.com>
> > By the way, 7dce19d3 is interesting in another way and worth
> > studying in that it adds --submodule-prefix ;-) It may be something
> > we want to consider consolidating with what Brandon has been working
> > on.
>
> That's why Brandon is cc'd now. :)
Interesting. Once we get something we agree on for adding the
--submodule-prefix option to the top level we'll definitely have to
update this section of code with the change.
--
Brandon Williams
^ permalink raw reply
* A couple of bugs / things TODO I encountered
From: Ævar Arnfjörð Bjarmason @ 2016-09-26 22:13 UTC (permalink / raw)
To: Git
I have these on my TODO list to look at at some point, but who knows
when, meanwhile I thought I'd send them to the list if anyone's
interested or wants to comment on them.
= Inconsistent regexp usage:
If you supply --perl-regexp to git-log it only applies to --grep. The
documentation says "Consider the limiting patterns to be
Perl-compatible regular expressions". Which might lead you to think
that e.g. -G uses it too. It doesn't, only grep.c does PCRE, but -G is
handled by diffcore-pickaxe.c.
Looking at "git grep -l regexec -- '*.c'" this whole thing is a mess.
Ideally you should be able to say you want to use PCRE for everything,
except maybe things that end up in your .gitconfig or e.g. the diff
driver. But we could really use a more generic regexp interface.
In general Git's regexp use is a huge mess, e.g. there's
--regexp-ignore-case but no way to supply various other regexp options
like REG_NEWLINE or PCRE options consistently.
= "git describe" really needs some overhaul
$ git describe 6ebdac1
v2.10.0
$ git describe --contains 6ebdac1
v2.10.0^0
As far as I can tell for the former there's no options that'll give me
v2.10.0-g6ebdac1, or v2.10.0-g6ebdac1 for the latter.
The reason I want that is that I like using "git describe" to give me
approximately when a commit happened in a repo, but the tags may get
deleted. In that case the *g<commit> syntax makes sure we can still
look up the commit, but omitting the commit if it exactly corresponds
to a tag, or the entire otherwise useful --contains option breaks
that.
^ permalink raw reply
* Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev
From: Junio C Hamano @ 2016-09-26 22:11 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Santi Béjar, Kevin Bracey, Philip Oakley, Matthieu Moy,
Ramsay Jones, Jakub Narębski
In-Reply-To: <20160926204959.26007-1-vegard.nossum@oracle.com>
Vegard Nossum <vegard.nossum@oracle.com> writes:
> +test_expect_success 'rev-parse merge^-0' '
> + test_must_fail git rev-parse merge^-0
> +'
> +
> +test_expect_success 'rev-parse merge^-3' '
> + test_must_fail git rev-parse merge^-3
> +'
> +
> +test_expect_success 'rev-parse merge^-^' '
> + test_must_fail git rev-parse merge^-^
> +'
> +
> +test_expect_success 'rev-list merge^-0' '
> + test_must_fail git rev-list merge^-0
> +'
> +
> +test_expect_success 'rev-list merge^-3' '
> + test_must_fail git rev-list merge^-3
> +'
> +
> +test_expect_success 'rev-list merge^-^' '
> + test_must_fail git rev-list merge^-^
> +'
This seems to be testing failure cases fairly thoroughly, which is a
good sign. One thing not tested is "merge^-1x" to ensure that no
change mistakenly will break the strtoul() check you have to parse
the parent number in the future, but other than that (and possibly
reusing the set-up of an already existing test), I am fairly happy
with the tests in this patch.
Thanks.
^ permalink raw reply
* Re: git 2.9.2: is RUNTIME_PREFIX supposed to work?
From: Junio C Hamano @ 2016-09-26 21:57 UTC (permalink / raw)
To: Paul Smith; +Cc: Git Mailing List
In-Reply-To: <1474925524.4270.35.camel@mad-scientist.net>
On Mon, Sep 26, 2016 at 2:32 PM, Paul Smith <paul@mad-scientist.net> wrote:
> Hi all. I'm trying to create a relocatable installation of Git 2.9.2,
> so I can copy it anywhere and it continues to run without any problem.
> This is on GNU/Linux systems, FWIW.
I had an impression that the setting was only to support MS Windows.
^ permalink raw reply
* Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev
From: Junio C Hamano @ 2016-09-26 21:55 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Santi Béjar, Kevin Bracey, Philip Oakley, Matthieu Moy,
Ramsay Jones, Jakub Narębski
In-Reply-To: <xmqqr386pd7d.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Micronit. When splitting "for (init; fini; cont)" into multiple
> lines, it is often easier to read to make that into three lines:
>
> for (parent_number = 1, parents = commit->parents;
> parents;
> parents = parents->next, parent_number++) {
>
>> + if (exclude_parent && parent_number != exclude_parent)
>> + continue;
>> +
>> + show_rev(include_parents ? NORMAL : REVERSED,
>> + parents->item->object.oid.hash, arg);
>> + }
>
> It is very clear to see what is going on. Good job.
>
>> *dotdot = '^';
>> + if (exclude_parent >= parent_number)
>> + return 0;
>
> This is not quite nice. You've already called show_rev() number of
> times, and it is too late to signal an error here. I think you
> would need to count the number of parents much earlier when
> exclude_parent option is in effect and error out before making any
> call to show_rev().
> ...
> Likewise. It is way too late to say "Nah, this wasn't a valid rev^-
> notation after all" to the caller after calling add_rev_cmdline()
> and add_pending_object() in the above loop.
Taking these two together, perhaps squashing this in may be
sufficient.
Please do not use --no-prefix when sending a patch to this list, by
the way.
builtin/rev-parse.c | 18 +++++++++++++++---
revision.c | 17 ++++++++++++++++-
2 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 2c3da19..9474c37 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -333,8 +333,22 @@ static int try_parent_shorthands(const char *arg)
if (include_rev)
show_rev(NORMAL, sha1, arg);
commit = lookup_commit_reference(sha1);
+
+ if (exclude_parent) {
+ /* do we have enough parents? */
+ for (parent_number = 0, parents = commit->parents;
+ parents;
+ parents = parents->next)
+ parent_number++;
+ if (parent_number < exclude_parent) {
+ *dotdot = '^';
+ return 0;
+ }
+ }
+
for (parent_number = 1, parents = commit->parents;
- parents; parents = parents->next, parent_number++) {
+ parents;
+ parents = parents->next, parent_number++) {
if (exclude_parent && parent_number != exclude_parent)
continue;
@@ -343,8 +357,6 @@ static int try_parent_shorthands(const char *arg)
}
*dotdot = '^';
- if (exclude_parent >= parent_number)
- return 0;
return 1;
}
diff --git a/revision.c b/revision.c
index 511e1ed..09da7f4 100644
--- a/revision.c
+++ b/revision.c
@@ -1318,8 +1318,23 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
if (it->type != OBJ_COMMIT)
return 0;
commit = (struct commit *)it;
+
+ if (exclude_parent) {
+ struct commit_list *parents;
+ int parent_number;
+
+ /* do we have enough parents? */
+ for (parent_number = 0, parents = commit->parents;
+ parents;
+ parents = parents->next)
+ parent_number++;
+ if (parent_number < exclude_parent)
+ return 0;
+ }
+
for (parent_number = 1, parents = commit->parents;
- parents; parents = parents->next, parent_number++) {
+ parents;
+ parents = parents->next, parent_number++) {
if (exclude_parent && parent_number != exclude_parent)
continue;
^ permalink raw reply related
* Re: [PATCH] git-gui: Do not reset author details on amend
From: Junio C Hamano @ 2016-09-26 21:34 UTC (permalink / raw)
To: Orgad Shaneh; +Cc: Pat Thoyts, git
In-Reply-To: <CAGHpTB+Fnu4x1bV9TSNo8pYdOzJzRsXA9r3CwxVz64mjW_qsGw@mail.gmail.com>
Orgad Shaneh <orgads@gmail.com> writes:
> On Sun, Jul 10, 2016 at 7:36 AM, Orgad Shaneh <orgads@gmail.com> wrote:
>
>> On Wed, May 18, 2016 at 9:12 AM, Orgad Shaneh <orgads@gmail.com> wrote:
>>> ping?
>>>
>> It's been over 2 months. Can anyone please review and merge it?
>>
> 4.5 months and counting... :(
>>
>>> On Thu, May 5, 2016 at 8:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Pat, we haven't heard from you for a long time. Are you still
>>>> around and interested in helping us by maintaining git-gui?
>>>>
>>>> Otherwise we may have to start recruiting a volunteer or two to take
>>>> this over.
Sorry about that. No volunteers materialized yet X-<, and I really
really do not want to apply anything other than trivial patches to
it myself, as I am not a git-gui user.
^ permalink raw reply
* git 2.9.2: is RUNTIME_PREFIX supposed to work?
From: Paul Smith @ 2016-09-26 21:32 UTC (permalink / raw)
To: git
Hi all. I'm trying to create a relocatable installation of Git 2.9.2,
so I can copy it anywhere and it continues to run without any problem.
This is on GNU/Linux systems, FWIW.
Looking through the code (for some other reason) I discovered the
RUNTIME_PREFIX setting which appears to attempt to set up the system
paths based on a prefix determined from the directory containing the
git command. That looks like exactly what I want.
If I set RUNTIME_PREFIX=YesPlease and gitexecdir=libexec/git-core on
the make invocation, it appears to do the right thing when I invoke
.../libexec/git-core/git, even if I move it around. Cool!
Except, when it doesn't. And when it doesn't is all the situations
where Git runs subcommands: for example, "git pull" which wants to
invoke fetch and merge-base commands.
When RUNTIME_PREFIX is defined, it's a requirement that the argv[0] for
the process be a fully-qualified pathname: if it's not, then git will
assert at exec_cmd.c:23 in system_path():
assert(argv0_path);
The argv0_path variable is set based on argv[0] passed in to main().
When I invoke top-level Git commands, that value is a fully-qualified
path just as you'd expect. However, when Git itself invokes
subcommands it does so in a weird way where argv[0] is the command it
wants to invoke, using the magical execv() facility that lets you
invoke a command while providing a different value as argv[0].
For example my core dump from the assert of the merge-base shows:
(gdb) p argv[0]
$2 = 0x7fffd70c338e "merge-base"
(gdb) p argv[1]
$3 = 0x7fffd70c3399 "--fork-point"
(gdb) p argv[2]
$4 = 0x7fffd70c33a6 "refs/remotes/origin/master"
(gdb) p argv[3]
$5 = 0x7fffd70c33c1 "master"
Looking at builtin/pull.c I see get_rebase_fork_point() calls
capture_command() with this Git command, which calls start_command(),
which calls execv_git_command(), which calls sane_execvp(), which
invokes "git" as the filename but with argv[0] as "merge-base":
sane_execvp("git", (char **)nargv.argv);
...calls sane_execvp():
if (!execvp(file, argv))
return 0; /* cannot happen ;-) */
This causes the assert.
So, my question is: is this a bug in RUNTIME_PREFIX support? Or is
RUNTIME_PREFIX no longer supported, or maybe not supported at all on
UNIX-type operating systems?
Cheers!
^ permalink raw reply
* Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev
From: Junio C Hamano @ 2016-09-26 21:23 UTC (permalink / raw)
To: Vegard Nossum
Cc: git, Santi Béjar, Kevin Bracey, Philip Oakley, Matthieu Moy,
Ramsay Jones, Jakub Narębski
In-Reply-To: <20160926204959.26007-1-vegard.nossum@oracle.com>
Vegard Nossum <vegard.nossum@oracle.com> writes:
> I often use rev^..rev to get all the commits in the branch that was merged
> in by the merge commit 'rev' (including the merge itself). To save typing
> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
> we can make rev^- a shorthand for that.
>
> The existing syntax rev^! seems like it should do the same thing, but it
> doesn't really do the right thing for merge commits (it doesn't include
> the commits from side branches).
>
> As a natural generalisation, we also accept rev^-n where n excludes the
> nth parent of rev. For example, for a two-parent merge, you can use rev^-2
> to get the set of commits which were made to the main branch while the
> topic branch was prepared.
I am tempted to suggest that this four-line paragraph may be
sufficient:
"git log rev^..rev" is commonly used to show all work done on
and merged from a side branch. Introduce a short-hand "rev^-"
for this, and also allow it to take "rev^-$n" to mean "reachable
from rev, excluding what is reachable from n-th parent of rev".
This alone is not a strong enough reason to ask you to reroll the
patch.
> diff --git builtin/rev-parse.c builtin/rev-parse.c
> index 76cf05e..2c3da19 100644
> --- builtin/rev-parse.c
> +++ builtin/rev-parse.c
> @@ -298,14 +298,30 @@ static int try_parent_shorthands(const char *arg)
> unsigned char sha1[20];
> struct commit *commit;
> struct commit_list *parents;
> - int parents_only;
> -
> - if ((dotdot = strstr(arg, "^!")))
> - parents_only = 0;
> - else if ((dotdot = strstr(arg, "^@")))
> - parents_only = 1;
> -
> - if (!dotdot || dotdot[2])
> + int parent_number;
> + int include_rev = 0;
> + int include_parents = 0;
> + int exclude_parent = 0;
> +
> + if ((dotdot = strstr(arg, "^!"))) {
> + include_rev = 1;
> + if (dotdot[2])
> + return 0;
> + } else if ((dotdot = strstr(arg, "^@"))) {
> + include_parents = 1;
> + if (dotdot[2])
> + return 0;
> + } else if ((dotdot = strstr(arg, "^-"))) {
> + include_rev = 1;
> + exclude_parent = 1;
> +
> + if (dotdot[2]) {
> + char *end;
> + exclude_parent = strtoul(dotdot + 2, &end, 10);
> + if (*end != '\0' || !exclude_parent)
> + return 0;
> + }
> + } else
> return 0;
Nice; we can tell where this is going without looking at the rest,
which is a very good sign that the new variables are doing their
work of telling the readers what is going on clearly.
> @@ -314,14 +330,21 @@ static int try_parent_shorthands(const char *arg)
> return 0;
> }
>
> - if (!parents_only)
> + if (include_rev)
> show_rev(NORMAL, sha1, arg);
> commit = lookup_commit_reference(sha1);
> - for (parents = commit->parents; parents; parents = parents->next)
> - show_rev(parents_only ? NORMAL : REVERSED,
> - parents->item->object.oid.hash, arg);
> + for (parent_number = 1, parents = commit->parents;
> + parents; parents = parents->next, parent_number++) {
Micronit. When splitting "for (init; fini; cont)" into multiple
lines, it is often easier to read to make that into three lines:
for (parent_number = 1, parents = commit->parents;
parents;
parents = parents->next, parent_number++) {
> + if (exclude_parent && parent_number != exclude_parent)
> + continue;
> +
> + show_rev(include_parents ? NORMAL : REVERSED,
> + parents->item->object.oid.hash, arg);
> + }
It is very clear to see what is going on. Good job.
> *dotdot = '^';
> + if (exclude_parent >= parent_number)
> + return 0;
This is not quite nice. You've already called show_rev() number of
times, and it is too late to signal an error here. I think you
would need to count the number of parents much earlier when
exclude_parent option is in effect and error out before making any
call to show_rev().
> diff --git revision.c revision.c
> index 969b3d1..9ae95bf 100644
> --- revision.c
> +++ revision.c
> @@ -1289,12 +1289,14 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)
> }
> }
>
> -static int add_parents_only(struct rev_info *revs, const char *arg_, int flags)
> +static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
> + int exclude_parent)
> {
> unsigned char sha1[20];
> struct object *it;
> struct commit *commit;
> struct commit_list *parents;
> + int parent_number;
> const char *arg = arg_;
>
> if (*arg == '^') {
> @@ -1316,12 +1318,18 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags)
> if (it->type != OBJ_COMMIT)
> return 0;
> commit = (struct commit *)it;
> - for (parents = commit->parents; parents; parents = parents->next) {
> + for (parent_number = 1, parents = commit->parents;
> + parents; parents = parents->next, parent_number++) {
> + if (exclude_parent && parent_number != exclude_parent)
> + continue;
> +
> it = &parents->item->object;
> it->flags |= flags;
> add_rev_cmdline(revs, it, arg_, REV_CMD_PARENTS_ONLY, flags);
> add_pending_object(revs, it, arg);
> }
> + if (exclude_parent >= parent_number)
> + return 0;
Likewise. It is way too late to say "Nah, this wasn't a valid rev^-
notation after all" to the caller after calling add_rev_cmdline()
and add_pending_object() in the above loop. Just like "blob^-"
silently returns 0 in the pre-context in this hunk, count the number
of parents before entering this loop when exclude_parent is in
effect, and if the number after '-' exceeds the actual number of
parents, silently return 0, perhaps?
> diff --git t/t6070-rev-parent-exclusion.sh t/t6070-rev-parent-exclusion.sh
We already seem to have t6101 as the best place to add test for this
new feature. Near the end of that script, ^@ and ^! are tested.
Thanks.
^ permalink raw reply
* Re: [PATCH] Documentation/fetch-options: emit recurse-submodules, jobs unconditionally
From: Stefan Beller @ 2016-09-26 21:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org, Jens Lehmann
In-Reply-To: <xmqq37kmqt4a.fsf@gitster.mtv.corp.google.com>
On Mon, Sep 26, 2016 at 1:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> After a bit more research, I think 8f0700dd33f (fetch/pull: Add the
>> 'on-demand' value to the --recurse-submodules option) is the culprit,
>> where this patch should have been squashed into, as that made the
>> both locations word for word equal.
>
> Hmph, my digging points to elsewhere. 7811d960 ("pull: Document the
> "--[no-]recurse-submodules" options", 2011-02-07)
That commit seems like it want to intentionally keep it different for
fetch and pull
(otherwise the fetch-options.txt would have been reworded there).
Rereading the actual option descriptions, I realize they are different.
(Initially I used a diff tool to see if there is aminor difference, and I was
surprised they were word for word identical; It must have been a mistake
on copying one of the option texts)
The git-pull part actually conveys pull specific information, so let's drop
this patch entirely.
> which is older
> than 8f0700dd ("fetch/pull: Add the 'on-demand' value to the
> --recurse-submodules option", 2011-03-06) seems to be the real
> change that pulled the description of recurse-submodules made in
> fetch-options into "show this only when we are not describing pull".
>
> Unfortunately it is not clear why we actively wanted to be sketchier
> when showing "git help fetch"; otherwise the change would have been
> made to the existing description there without adding a new entry to
> "git-pull.txt".
>
^ 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