Message ID | 4bc290fbf43e0193aae288b79249014d899ea34a.1659388498.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Generalize 'scalar diagnose' into 'git bugreport --diagnose' | expand |
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes: > int at_root = !*path; > - DIR *dir = opendir(at_root ? "." : path); > + DIR *dir; > struct dirent *e; > struct strbuf buf = STRBUF_INIT; > size_t len; > int res = 0; > > + if (!file_exists(at_root ? "." : path)) { > + warning(_("directory '%s' does not exist, will not be archived"), path); > + return 0; > + } > + > + dir = opendir(at_root ? "." : path); > if (!dir) > return error_errno(_("could not open directory '%s'"), path); I am not sure if TOCTTOU is how we want to be more gentle. Do we rather want to do something like this dir = opendir(...); if (!dir) { if (errno == ENOENT) { warning(_("not archiving missing directory '%s'", path); return 0; } return error_errno(_("cannot open directory '%s'"), path); } or am I missing something subtle? Thanks. > diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh > index 3cf983aa67f..e9db89ef2c8 100755 > --- a/t/t0091-bugreport.sh > +++ b/t/t0091-bugreport.sh > @@ -78,7 +78,7 @@ test_expect_success 'indicates populated hooks' ' > test_cmp expect actual > ' > > -test_expect_failure UNZIP '--diagnose creates diagnostics zip archive' ' > +test_expect_success UNZIP '--diagnose creates diagnostics zip archive' ' > test_when_finished rm -rf report && > > git bugreport --diagnose -o report -s test >out && > @@ -98,4 +98,13 @@ test_expect_failure UNZIP '--diagnose creates diagnostics zip archive' ' > grep "^Total: [0-9][0-9]*" out > ' > > +test_expect_success '--diagnose warns when archived dir does not exist' ' > + test_when_finished rm -rf report && > + > + # Remove logs - not guaranteed to exist > + rm -rf .git/logs && > + git bugreport --diagnose -o report -s test 2>err && > + grep "directory .\.git/logs. does not exist, will not be archived" err > +' > + > test_done
Junio C Hamano wrote: > "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> int at_root = !*path; >> - DIR *dir = opendir(at_root ? "." : path); >> + DIR *dir; >> struct dirent *e; >> struct strbuf buf = STRBUF_INIT; >> size_t len; >> int res = 0; >> >> + if (!file_exists(at_root ? "." : path)) { >> + warning(_("directory '%s' does not exist, will not be archived"), path); >> + return 0; >> + } >> + >> + dir = opendir(at_root ? "." : path); >> if (!dir) >> return error_errno(_("could not open directory '%s'"), path); > > I am not sure if TOCTTOU is how we want to be more gentle. Do we > rather want to do something like this > > dir = opendir(...); > if (!dir) { > if (errno == ENOENT) { > warning(_("not archiving missing directory '%s'", path); > return 0; > } > return error_errno(_("cannot open directory '%s'"), path); > } > > or am I missing something subtle? > The "gentleness" was meant to be a reference only to the error -> warning change, the TOCTTOU change was just a miss by me. I'll fix it in the next version, thanks! > Thanks. > >> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh >> index 3cf983aa67f..e9db89ef2c8 100755 >> --- a/t/t0091-bugreport.sh >> +++ b/t/t0091-bugreport.sh >> @@ -78,7 +78,7 @@ test_expect_success 'indicates populated hooks' ' >> test_cmp expect actual >> ' >> >> -test_expect_failure UNZIP '--diagnose creates diagnostics zip archive' ' >> +test_expect_success UNZIP '--diagnose creates diagnostics zip archive' ' >> test_when_finished rm -rf report && >> >> git bugreport --diagnose -o report -s test >out && >> @@ -98,4 +98,13 @@ test_expect_failure UNZIP '--diagnose creates diagnostics zip archive' ' >> grep "^Total: [0-9][0-9]*" out >> ' >> >> +test_expect_success '--diagnose warns when archived dir does not exist' ' >> + test_when_finished rm -rf report && >> + >> + # Remove logs - not guaranteed to exist >> + rm -rf .git/logs && >> + git bugreport --diagnose -o report -s test 2>err && >> + grep "directory .\.git/logs. does not exist, will not be archived" err >> +' >> + >> test_done
diff --git a/builtin/bugreport.c b/builtin/bugreport.c index 720889a37ad..dea11f91386 100644 --- a/builtin/bugreport.c +++ b/builtin/bugreport.c @@ -176,12 +176,18 @@ static int add_directory_to_archiver(struct strvec *archiver_args, const char *path, int recurse) { int at_root = !*path; - DIR *dir = opendir(at_root ? "." : path); + DIR *dir; struct dirent *e; struct strbuf buf = STRBUF_INIT; size_t len; int res = 0; + if (!file_exists(at_root ? "." : path)) { + warning(_("directory '%s' does not exist, will not be archived"), path); + return 0; + } + + dir = opendir(at_root ? "." : path); if (!dir) return error_errno(_("could not open directory '%s'"), path); diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh index 3cf983aa67f..e9db89ef2c8 100755 --- a/t/t0091-bugreport.sh +++ b/t/t0091-bugreport.sh @@ -78,7 +78,7 @@ test_expect_success 'indicates populated hooks' ' test_cmp expect actual ' -test_expect_failure UNZIP '--diagnose creates diagnostics zip archive' ' +test_expect_success UNZIP '--diagnose creates diagnostics zip archive' ' test_when_finished rm -rf report && git bugreport --diagnose -o report -s test >out && @@ -98,4 +98,13 @@ test_expect_failure UNZIP '--diagnose creates diagnostics zip archive' ' grep "^Total: [0-9][0-9]*" out ' +test_expect_success '--diagnose warns when archived dir does not exist' ' + test_when_finished rm -rf report && + + # Remove logs - not guaranteed to exist + rm -rf .git/logs && + git bugreport --diagnose -o report -s test 2>err && + grep "directory .\.git/logs. does not exist, will not be archived" err +' + test_done