From: Junio C Hamano <hidden> Date: 2016-08-29 23:16:21
Uma Srinivasan [off-list ref] writes:
On Mon, Aug 29, 2016 at 2:13 PM, Uma Srinivasan [off-list ref] wrote:
quoted
Ok that makes sense. Thanks much.
Uma
With respect to my original problem with a corrupted .git directory
under the submodule directory, I am thinking of adding the following 4
lines marked with ### to is_submodule_modified() to detect the
corrupted dir and die quickly instead of forking several child
processes:
strbuf_addf(&buf, "%s/.git", path);
git_dir = read_gitfile(buf.buf);
if (!git_dir) {
### strbuf_addf(&head_ref, "%s/HEAD",buf.buf);
### if (strbuf_read_file(&temp_ref, head_ref.buf,0) < 0) {
### die("Corrupted .git dir in submodule %s", path);
###}
git_dir = buf.buf;
}
This fixes my issue but what do you think? Is this the right way to
fix it? Is there a better way?
I think we already have a helper function that does a lot better
than "does it have a file called HEAD" to ask "is this a git
directory?" and its name I think is "is_git_directory" (I know, we
are not imaginative when naming our functions).
As to the check makes sense in the context of this function, I am
not an expert to judge. I'd expect Jens, Heiko and/or Stefan to
know better than I do.
From: Uma Srinivasan <hidden> Date: 2016-08-29 23:34:59
Yes, is_git_directory() is much better. Thanks for the pointer.
I will submit a patch unless I hear more suggestions from others.
Uma
On Mon, Aug 29, 2016 at 4:15 PM, Junio C Hamano [off-list ref] wrote:
Uma Srinivasan [off-list ref] writes:
quoted
On Mon, Aug 29, 2016 at 2:13 PM, Uma Srinivasan [off-list ref] wrote:
quoted
Ok that makes sense. Thanks much.
Uma
With respect to my original problem with a corrupted .git directory
under the submodule directory, I am thinking of adding the following 4
lines marked with ### to is_submodule_modified() to detect the
corrupted dir and die quickly instead of forking several child
processes:
strbuf_addf(&buf, "%s/.git", path);
git_dir = read_gitfile(buf.buf);
if (!git_dir) {
### strbuf_addf(&head_ref, "%s/HEAD",buf.buf);
### if (strbuf_read_file(&temp_ref, head_ref.buf,0) < 0) {
### die("Corrupted .git dir in submodule %s", path);
###}
git_dir = buf.buf;
}
This fixes my issue but what do you think? Is this the right way to
fix it? Is there a better way?
I think we already have a helper function that does a lot better
than "does it have a file called HEAD" to ask "is this a git
directory?" and its name I think is "is_git_directory" (I know, we
are not imaginative when naming our functions).
As to the check makes sense in the context of this function, I am
not an expert to judge. I'd expect Jens, Heiko and/or Stefan to
know better than I do.
From: Jacob Keller <hidden> Date: 2016-08-30 00:02:33
On Mon, Aug 29, 2016 at 4:15 PM, Junio C Hamano [off-list ref] wrote:
Uma Srinivasan [off-list ref] writes:
quoted
This fixes my issue but what do you think? Is this the right way to
fix it? Is there a better way?
I think we already have a helper function that does a lot better
than "does it have a file called HEAD" to ask "is this a git
directory?" and its name I think is "is_git_directory" (I know, we
are not imaginative when naming our functions).
As to the check makes sense in the context of this function, I am
not an expert to judge. I'd expect Jens, Heiko and/or Stefan to
know better than I do.
One of my patches adds a "is_git_directory()" call to this, and if we
fail falls back to checking the .gitmodules and git-config for
information regarding the submodule should it no longer be checked
out. I suspect this patch will address your concern.
Thanks,
Jake
From: Uma Srinivasan <hidden> Date: 2016-08-30 00:13:19
This is great! Thanks Jake. If you happen to have the patch ID it
would be helpful.
Uma
On Mon, Aug 29, 2016 at 5:02 PM, Jacob Keller [off-list ref] wrote:
On Mon, Aug 29, 2016 at 4:15 PM, Junio C Hamano [off-list ref] wrote:
quoted
Uma Srinivasan [off-list ref] writes:
quoted
This fixes my issue but what do you think? Is this the right way to
fix it? Is there a better way?
I think we already have a helper function that does a lot better
than "does it have a file called HEAD" to ask "is this a git
directory?" and its name I think is "is_git_directory" (I know, we
are not imaginative when naming our functions).
As to the check makes sense in the context of this function, I am
not an expert to judge. I'd expect Jens, Heiko and/or Stefan to
know better than I do.
One of my patches adds a "is_git_directory()" call to this, and if we
fail falls back to checking the .gitmodules and git-config for
information regarding the submodule should it no longer be checked
out. I suspect this patch will address your concern.
Thanks,
Jake
From: Uma Srinivasan <hidden> Date: 2016-08-30 17:40:42
Thanks for the patch. Unfortunately, it doesn't help in my case as it
invokes the is_submodule_modified() routine which you didn't modify.
Here's my call trace....
#0 is_submodule_modified (path=path@entry=0x17c2f08 "groc", ignore_untracked=0)
at submodule.c:939
#1 0x00000000004aa4dc in match_stat_with_submodule (
diffopt=diffopt@entry=0x7fffffffde18, ce=ce@entry=0x17c2eb0,
st=st@entry=0x7fffffffd840, ce_option=ce_option@entry=0,
dirty_submodule=dirty_submodule@entry=0x7fffffffd83c) at diff-lib.c:81
#2 0x00000000004ab4f5 in run_diff_files (revs=revs@entry=0x7fffffffd920,
option=option@entry=0) at diff-lib.c:217
#3 0x000000000054c0d4 in wt_status_collect_changes_worktree
(s=s@entry=0x7de280 <s>)
at wt-status.c:559
#4 0x000000000054ecf6 in wt_status_collect (s=s@entry=0x7de280 <s>)
at wt-status.c:678
#5 0x0000000000422171 in cmd_status (argc=<optimized out>,
argv=<optimized out>,
prefix=0x0) at builtin/commit.c:1390
#6 0x0000000000405abe in run_builtin (argv=<optimized out>,
argc=<optimized out>,
p=<optimized out>) at git.c:352
#7 handle_builtin (argc=1, argv=0x7fffffffe570) at git.c:551
#8 0x0000000000405dd8 in run_argv (argv=0x7fffffffe320, argcp=0x7fffffffe32c)
at git.c:606
#9 cmd_main (argc=1, argc@entry=2, argv=0x7fffffffe570,
argv@entry=0x7fffffffe568)
at git.c:678
#10 0x0000000000405060 in main (argc=2, argv=0x7fffffffe568) at common-main.c:40
I think the following fix is still needed to is_submodule_modified():
strbuf_addf(&buf, "%s/.git", path);
git_dir = read_gitfile(buf.buf);
if (!git_dir) {
git_dir = buf.buf;
==> if (!is_git_directory(git_dir)) {
==> die("Corrupted .git dir in submodule %s", path);
==> }
}
Thanks,
Uma
On Mon, Aug 29, 2016 at 11:23 PM, Jacob Keller [off-list ref] wrote:
On Mon, Aug 29, 2016 at 11:09 PM, Jacob Keller [off-list ref] wrote:
quoted
On Mon, Aug 29, 2016 at 5:12 PM, Uma Srinivasan [off-list ref] wrote:
quoted
This is great! Thanks Jake. If you happen to have the patch ID it
would be helpful.
Uma