Re: [PATCH 4/7] builtin/bugreport.c: add directory to archiver more gently
- From
Victoria Dye <vdye@github.com>
- Date
- Aug 2, 2022, 15:43 UTC
- Message-ID
- <760ebe95-552f-c0d7-18c3-f19671fdd192@github.com>
- In-Reply-To
- <xmqqles7mwnc.fsf@gitster.g>
Junio C Hamano wrote:
Show 33 quoted lines
> "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!
Show 29 quoted lines
> 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