Re: [PATCH v2 03/10] scalar-diagnose: add directory to archiver more gently
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Aug 4, 2022, 06:19 UTC
- Message-ID
- <220804.868ro4fryg.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <23349bfaf8fc5f5001f1ed1fa19e9b3909466ae3.1659577543.git.gitgitgadget@gmail.com>
On Thu, Aug 04 2022, Victoria Dye via GitGitGadget wrote:
Show 37 quoted lines
> From: Victoria Dye <vdye@github.com>
>
> If a directory added to the 'scalar diagnose' archiver does not exist, warn
> and return 0 from 'add_directory_to_archiver()' rather than failing with a
> fatal error. This handles a failure edge case where the '.git/logs' has not
> yet been created when running 'scalar diagnose', but extends to any
> situation where a directory may be missing in the '.git' dir.
>
> Now, when a directory is missing a warning is captured in the diagnostic
> logs. This provides a user with more complete information than if 'scalar
> diagnose' simply failed with an error.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> contrib/scalar/scalar.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
> index 04046452284..b9092f0b612 100644
> --- a/contrib/scalar/scalar.c
> +++ b/contrib/scalar/scalar.c
> @@ -266,14 +266,20 @@ 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 (!dir)
> + dir = opendir(at_root ? "." : path);
> + if (!dir) {
> + if (errno == ENOENT) {Per [1] I think this is incorrect or overly strict. Let's not spew warnings if the user "rm -rf .git/hooks" or whatever.
It might be valuable to note in some file in the archive such oddities we find, but warning() won't give us that.
> + warning(_("could not archive missing directory '%s'"), path);In any case, this should be e.g.:
warning_errno(_("could not archive directory '%s'"), path);You already have an errno, so using *_errno() will add the standard information about what the issue is.
Show 7 quoted lines
> + return 0;
> + }
> return error_errno(_("could not open directory '%s'"), path);
> + }
>
> if (!at_root)
> strbuf_addf(&buf, "%s/", path);1. https://lore.kernel.org/git/220610.86ilp9s1x7.gmgdl@evledraar.gmail.com/