git-fast-import bug?
git-fast-import is not writing a commit even after a checkpoint/progress command. See my previous message "git p4 diff-tree ambiguous argument error". The error in git-p4 is caused by git not writing the commit even after git-fast-import has been given a checkpoint and progress command. On initial use of git p4 to sync a p4 repository, the commits are written properly. But on a subsequent run the commit is not flushed to the file system during the run. Specifically, I can stop the git-p4 command directly after the progress checkpoint command (see the checkpoint() function in git-p4). The file is not found. If I abort/exit the application at that point, the file appears. There is a pattern of behavior here that is consistent but I am unable to understand. A bare repository works fine. An already populated repository does not work until the app is quit. What would cause git-fast-import to _NOT_ flush the file? This certainly seems like a bug. But I don't know enough of the git internals to reproduce. Suggestions on how to test or isolate this problem? Thanks Reproduced consistently on two systems: $ git --version git version 1.8.5.2 (Apple Git-48) $ python --version Python 2.7.5 $ uname -a Darwin Kernel Version 13.3.0: Tue Jun 3 21:27:35 PDT 2014; root:xnu-2422.110.17~1/RELEASE_X86_64 x86_64 and $ git --version git version 1.7.12.4 $ python --version Python 2.6.6 OS: GNU/Linux 2.6.32-431.el6.x86_64 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: fast-import bug?
On Sun, Jun 23, 2013 at 07:19:25AM -0700, Dave Abrahams wrote: > on Sun Jun 23 2013, John Keeping wrote: > > In this case, I think I do now understand why the mode is 0: in > > parse_ls a new tree object is created and the SHA1 of the original is > > copied in but the mode is left blank; clearly this should be set to > > S_IFDIR when the SHA1 is non-null. > > > > I think the patch I now have is correct (and addresses the "copy from > > root" scenario), but I need to spend some time understanding t9300 so > > that I can add suitable test cases. > > t9300? t/t9300-fast-import.sh in Git's source tree - it's where the tests for fast-import live. > Thanks; I'll try this one too. Thanks. I now have a patch series incorporating this which also adds a few tests for handling of empty paths. I'm sending it out in the next few minutes. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: fast-import bug?
on Sun Jun 23 2013, John Keeping wrote:
> On Sat, Jun 22, 2013 at 07:16:48PM -0700, Dave Abrahams wrote:
>> I also note that the docs
>> don't make it clear that quoting the path is mandatory if it might turn
>> out to be empty.
>
> That's not quite the case. It looks to me like quoting the path is
> mandatory if no "" is given, and indeed the documentation says:
>
>Reading from the active commit
>This form can only be used in the middle of a commit. The path
>names a directory entry within fast-import’s active commit. The
>path must be quoted in this case.
>
>'ls' SP LF
Oops; good eye.
>> > It seems to be slightly more complicated than that though, because after
>> > allowing empty trees I get the "missing" message for the root tree.
>>
>> Yeah, I've tried to patch Git to solve this but ran into that problem
>> and gave up.
>>
>> > This seems to be because its mode is 0 and not S_IFDIR.
>>
>> Aha.
>>
>> > With the patch below, things are working as I expect
>>
>> Awesome; works for me, too!
>>
>> > but I don't understand why the mode of the root is not set correctly
>> > at this point. Perhaps someone more familiar with fast-import will
>> > have some insight...
>>
>> Yeah... there's no bug tracker for Git, right? So if nobody pays
>> attention to this thread, the problem will persist?
>
> Yes, but I don't see that happening particularly often. In the worst
> case issues are normally documented by a failing test case.
The reason I ask is because from scouring this list it looks like
there's a history of people having issues with this, and someone
intended to get to a fix in sometime around 1.17.10, but nothing ever
happened.
> In this case, I think I do now understand why the mode is 0: in
> parse_ls a new tree object is created and the SHA1 of the original is
> copied in but the mode is left blank; clearly this should be set to
> S_IFDIR when the SHA1 is non-null.
>
> I think the patch I now have is correct (and addresses the "copy from
> root" scenario), but I need to spend some time understanding t9300 so
> that I can add suitable test cases.
t9300?
Thanks; I'll try this one too.
> -- >8 --
> diff --git a/fast-import.c b/fast-import.c
> index 23f625f..e2c9d50 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1629,7 +1629,8 @@ del_entry:
> static int tree_content_get(
> struct tree_entry *root,
> const char *p,
> - struct tree_entry *leaf)
> + struct tree_entry *leaf,
> + int allow_root)
> {
> struct tree_content *t;
> const char *slash1;
> @@ -1641,31 +1642,39 @@ static int tree_content_get(
> n = slash1 - p;
> else
> n = strlen(p);
> - if (!n)
> + if (!n && !allow_root)
> die("Empty path component found in input");
>
> if (!root->tree)
> load_tree(root);
> +
> + if (!n) {
> + e = root;
> + goto found_entry;
> + }
> +
> t = root->tree;
> for (i = 0; i < t->entry_count; i++) {
> e = t->entries[i];
> if (e->name->str_len == n && !strncmp_icase(p,
> e->name->str_dat, n)) {
> - if (!slash1) {
> - memcpy(leaf, e, sizeof(*leaf));
> - if (e->tree &&
> is_null_sha1(e->versions[1].sha1))
> - leaf->tree = dup_tree_content(e->tree);
> - else
> - leaf->tree = NULL;
> - return 1;
> - }
> + if (!slash1)
> + goto found_entry;
> if (!S_ISDIR(e->versions[1].mode))
> return 0;
> if (!e->tree)
> load_tree(e);
> - return tree_content_get(e, slash1 + 1, leaf);
> + return tree_content_get(e, slash1 + 1, leaf, 0);
> }
> }
> return 0;
> +
> +found_entry:
> + memcpy(leaf, e, sizeof(*leaf));
> + if (e->tree && is_null_sha1(e->versions[1].sha1))
> + leaf->tree = dup_tree_content(e->tree);
> + else
> + leaf->tree = NULL;
> + return 1;
> }
>
> static int update_branch(struct branch *b)
> @@ -2415,7 +2424,7 @@ static void file_change_cr(struct branch *b, int rename)
> if (rename)
> tree_content_remove(&b->branch_tree, s, &leaf);
> else
> - tree_content_get(&b->branch_tree, s, &leaf);
> + tree_content_get(&b->branch_tree, s, &leaf, 1);
> if (!leaf.versions[1].mode)
> die("Path %s not in branch", s);
> if (!*d) { /* C "path/to/subdir" "" */
> @@ -3051,6 +3060,8 @@ static void parse_ls(struct branch *b)
> struct object_entry *e = parse_treeish_dataref(&p);
>
Re: fast-import bug?
On Sat, Jun 22, 2013 at 07:16:48PM -0700, Dave Abrahams wrote:
>
> on Sat Jun 22 2013, John Keeping wrote:
>
> > On Fri, Jun 21, 2013 at 02:21:47AM -0700, Dave Abrahams wrote:
> >> The docs for fast-import seem to imply that I can use "ls" to get the
> >> SHA1 of a commit for which I have a mark:
> >>
> >>Reading from a named tree
> >>The can be a mark reference (:) or the full
> >> 40-byte
> >
> >>SHA-1 of a Git tag, commit, or tree object, preexisting or
> >> waiting to
> >>be written. The path is relative to the top level of the tree
> >> named by
> >>.
> >>
> >>'ls' SP SP LF
> >>
> >>See filemodify above for a detailed description of .
> >>
> >>Output uses the same format as git ls-tree -- :
> >>
> >> SP ('blob' | 'tree' | 'commit') SP HT LF
> >>
> >>The represents the blob, tree, or commit object at
> >> and
> >>^^
> >>can be used in later cat-blob, filemodify, or ls commands.
> >>
> >> but I can't get it to work. It's not entirely clear it's supposed to
> >> work. What path would I pass? Passing an empty path simply causes git
> >> to report "missing ".
> >
> > Which version of Git are you using?
>
> ,[ git --version ]
> | git version 1.8.3.1
> `
>
> > I just tried this and get the error
> > "fatal: Empty path component found in input",
>
> I get that too.
>
> > which seems to be from commit 178e1de (fast-import: don't allow 'ls'
> > of path with empty components, 2012-03-09), which is included in Git
> > 1.7.9.5.
>
> Yes, that's at least part of the issue. I notice git-fast-import
> rejects the root path "" for other commands, e.g. when used as the
> source of a filecopy we get the same issue. I also note that the docs
> don't make it clear that quoting the path is mandatory if it might turn
> out to be empty.
Interesting. There are two places that can produce this error message,
tree_content_get and tree_content_set, but I wonder if this means that
tree_content_get should not be doing this check. The two places that
call it are:
1) "parse_ls" as discussed here
2) "file_change_cr" which deals with file copy and rename.
My patch in the previous message only changes the behaviour for the
parse_ls case, but it seems that you have a valid use case for removing
this check in the file_change_cr case as well.
> I also note that the docs
> don't make it clear that quoting the path is mandatory if it might turn
> out to be empty.
That's not quite the case. It looks to me like quoting the path is
mandatory if no "" is given, and indeed the documentation says:
Reading from the active commit
This form can only be used in the middle of a commit. The path
names a directory entry within fast-import’s active commit. The
path must be quoted in this case.
'ls' SP LF
> > It seems to be slightly more complicated than that though, because after
> > allowing empty trees I get the "missing" message for the root tree.
>
> Yeah, I've tried to patch Git to solve this but ran into that problem
> and gave up.
>
> > This seems to be because its mode is 0 and not S_IFDIR.
>
> Aha.
>
> > With the patch below, things are working as I expect
>
> Awesome; works for me, too!
>
> > but I don't understand why the mode of the root is not set correctly
> > at this point. Perhaps someone more familiar with fast-import will
> > have some insight...
>
> Yeah... there's no bug tracker for Git, right? So if nobody pays
> attention to this thread, the problem will persist?
Yes, but I don't see that happening particularly often. In the worst
case issues are normally documented by a failing test case.
In this case, I think I do now understand why the mode is 0: in parse_ls
a new tree object is created and the SHA1 of the original is copied in
but the mode is left blank; clearly this should be set to S_IFDIR when
the SHA1 is non-null.
I think the patch I now have is correct (and addresses the "copy from
root" scenario), but I need to spend some time understanding t9300 so
that I can add suitable test cases.
-- >8 --
diff --git a/fast-import.c b/fast-import.c
index 23f625f..e2c9d50 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1629,7 +1629,8 @@ del_entry:
static int tree_content_get(
struct tree_entry *root,
const char *p,
- struct tree_entry *leaf)
+ struct tree_entry *leaf,
+ int allow_root)
{
struct tree_content *t;
const char *slash1;
@@ -1641,31 +1642,39 @@ static int tree_content_get(
n = slash1 - p;
else
n = strlen(p);
- if (!n)
+ if (!n && !allow_root)
die("Empty path component found in input");
if (!root->tree)
load_tree(root);
+
+ if (!n) {
+
Re: fast-import bug?
on Sat Jun 22 2013, John Keeping wrote:
> On Fri, Jun 21, 2013 at 02:21:47AM -0700, Dave Abrahams wrote:
>> The docs for fast-import seem to imply that I can use "ls" to get the
>> SHA1 of a commit for which I have a mark:
>>
>>Reading from a named tree
>>The can be a mark reference (:) or the full
>> 40-byte
>
>>SHA-1 of a Git tag, commit, or tree object, preexisting or
>> waiting to
>>be written. The path is relative to the top level of the tree
>> named by
>>.
>>
>>'ls' SP SP LF
>>
>>See filemodify above for a detailed description of .
>>
>>Output uses the same format as git ls-tree -- :
>>
>> SP ('blob' | 'tree' | 'commit') SP HT LF
>>
>>The represents the blob, tree, or commit object at
>> and
>>^^
>>can be used in later cat-blob, filemodify, or ls commands.
>>
>> but I can't get it to work. It's not entirely clear it's supposed to
>> work. What path would I pass? Passing an empty path simply causes git
>> to report "missing ".
>
> Which version of Git are you using?
,[ git --version ]
| git version 1.8.3.1
`
> I just tried this and get the error
> "fatal: Empty path component found in input",
I get that too.
> which seems to be from commit 178e1de (fast-import: don't allow 'ls'
> of path with empty components, 2012-03-09), which is included in Git
> 1.7.9.5.
Yes, that's at least part of the issue. I notice git-fast-import
rejects the root path "" for other commands, e.g. when used as the
source of a filecopy we get the same issue. I also note that the docs
don't make it clear that quoting the path is mandatory if it might turn
out to be empty.
> It seems to be slightly more complicated than that though, because after
> allowing empty trees I get the "missing" message for the root tree.
Yeah, I've tried to patch Git to solve this but ran into that problem
and gave up.
> This seems to be because its mode is 0 and not S_IFDIR.
Aha.
> With the patch below, things are working as I expect
Awesome; works for me, too!
> but I don't understand why the mode of the root is not set correctly
> at this point. Perhaps someone more familiar with fast-import will
> have some insight...
Yeah... there's no bug tracker for Git, right? So if nobody pays
attention to this thread, the problem will persist?
> -- >8 --
> diff --git a/fast-import.c b/fast-import.c
> index 23f625f..bcce651 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1626,6 +1626,15 @@ del_entry:
> return 1;
> }
>
> +static void copy_tree_entry(struct tree_entry *dst, struct tree_entry *src)
> +{
> + memcpy(dst, src, sizeof(*dst));
> + if (src->tree && is_null_sha1(src->versions[1].sha1))
> + dst->tree = dup_tree_content(src->tree);
> + else
> + dst->tree = NULL;
> +}
> +
> static int tree_content_get(
> struct tree_entry *root,
> const char *p,
> @@ -1651,11 +1660,7 @@ static int tree_content_get(
> e = t->entries[i];
> if (e->name->str_len == n && !strncmp_icase(p,
> e->name->str_dat, n)) {
> if (!slash1) {
> - memcpy(leaf, e, sizeof(*leaf));
> - if (e->tree &&
> is_null_sha1(e->versions[1].sha1))
> - leaf->tree = dup_tree_content(e->tree);
> - else
> - leaf->tree = NULL;
> + copy_tree_entry(leaf, e);
> return 1;
> }
> if (!S_ISDIR(e->versions[1].mode))
> @@ -3065,7 +3070,11 @@ static void parse_ls(struct branch *b)
> die("Garbage after path in: %s", command_buf.buf);
> p = uq.buf;
> }
> - tree_content_get(root, p, &leaf);
> + if (!*p) {
> + copy_tree_entry(&leaf, root);
> + leaf.versions[1].mode = S_IFDIR;
> + } else
> + tree_content_get(root, p, &leaf);
> /*
>* A directory in preparation would have a sha1 of zero
>* until it is saved. Save, for simplicity.
--
Dave Abrahams
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: fast-import bug?
On Fri, Jun 21, 2013 at 02:21:47AM -0700, Dave Abrahams wrote:
> The docs for fast-import seem to imply that I can use "ls" to get the
> SHA1 of a commit for which I have a mark:
>
>Reading from a named tree
>The can be a mark reference (:) or the full
> 40-byte
>SHA-1 of a Git tag, commit, or tree object, preexisting or waiting
> to
>be written. The path is relative to the top level of the tree
> named by
>.
>
>'ls' SP SP LF
>
>See filemodify above for a detailed description of .
>
>Output uses the same format as git ls-tree -- :
>
> SP ('blob' | 'tree' | 'commit') SP HT LF
>
>The represents the blob, tree, or commit object at and
>^^
>can be used in later cat-blob, filemodify, or ls commands.
>
> but I can't get it to work. It's not entirely clear it's supposed to
> work. What path would I pass? Passing an empty path simply causes git
> to report "missing ".
Which version of Git are you using? I just tried this and get the error
"fatal: Empty path component found in input", which seems to be from
commit 178e1de (fast-import: don't allow 'ls' of path with empty
components, 2012-03-09), which is included in Git 1.7.9.5.
It seems to be slightly more complicated than that though, because after
allowing empty trees I get the "missing" message for the root tree.
This seems to be because its mode is 0 and not S_IFDIR.
With the patch below, things are working as I expect but I don't
understand why the mode of the root is not set correctly at this point.
Perhaps someone more familiar with fast-import will have some insight...
-- >8 --
diff --git a/fast-import.c b/fast-import.c
index 23f625f..bcce651 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1626,6 +1626,15 @@ del_entry:
return 1;
}
+static void copy_tree_entry(struct tree_entry *dst, struct tree_entry *src)
+{
+ memcpy(dst, src, sizeof(*dst));
+ if (src->tree && is_null_sha1(src->versions[1].sha1))
+ dst->tree = dup_tree_content(src->tree);
+ else
+ dst->tree = NULL;
+}
+
static int tree_content_get(
struct tree_entry *root,
const char *p,
@@ -1651,11 +1660,7 @@ static int tree_content_get(
e = t->entries[i];
if (e->name->str_len == n && !strncmp_icase(p,
e->name->str_dat, n)) {
if (!slash1) {
- memcpy(leaf, e, sizeof(*leaf));
- if (e->tree &&
is_null_sha1(e->versions[1].sha1))
- leaf->tree = dup_tree_content(e->tree);
- else
- leaf->tree = NULL;
+ copy_tree_entry(leaf, e);
return 1;
}
if (!S_ISDIR(e->versions[1].mode))
@@ -3065,7 +3070,11 @@ static void parse_ls(struct branch *b)
die("Garbage after path in: %s", command_buf.buf);
p = uq.buf;
}
- tree_content_get(root, p, &leaf);
+ if (!*p) {
+ copy_tree_entry(&leaf, root);
+ leaf.versions[1].mode = S_IFDIR;
+ } else
+ tree_content_get(root, p, &leaf);
/*
* A directory in preparation would have a sha1 of zero
* until it is saved. Save, for simplicity.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
fast-import bug?
The docs for fast-import seem to imply that I can use "ls" to get the
SHA1 of a commit for which I have a mark:
Reading from a named tree
The can be a mark reference (:) or the full 40-byte
SHA-1 of a Git tag, commit, or tree object, preexisting or waiting to
be written. The path is relative to the top level of the tree named
by
.
'ls' SP SP LF
See filemodify above for a detailed description of .
Output uses the same format as git ls-tree -- :
SP ('blob' | 'tree' | 'commit') SP HT LF
The represents the blob, tree, or commit object at and
^^
can be used in later cat-blob, filemodify, or ls commands.
but I can't get it to work. It's not entirely clear it's supposed to
work. What path would I pass? Passing an empty path simply causes git
to report "missing ".
TIA,
Dave
--
Dave Abrahams
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
