Re: [PATCH v2 3/3] scalar reconfigure: help users remove buggy repos
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 22, 2023, 19:45 UTC
- Message-ID
- <xmqqmsyilv3e.fsf@gitster.g>
- In-Reply-To
- <7ac7311863d2e05c3dc8e26cb821fe8a7c4b6804.1692725056.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 15 quoted lines
> @@ -664,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)
> git_config(get_scalar_repos, &scalar_repos);
>
> for (i = 0; i < scalar_repos.nr; i++) {
> + int succeeded = 0;
> const char *dir = scalar_repos.items[i].string;
>
> strbuf_reset(&commondir);
> @@ -674,27 +675,51 @@ static int cmd_reconfigure(int argc, const char **argv)
>
> if (errno != ENOENT) {
> warning_errno(_("could not switch to '%s'"), dir);
> - res = -1;
> - continue;
> + goto loop_end;This is after seeing chdir(dir) failed. If the user manually removed the enlisted directory, ENOENT would be one of the most likely errors. If the user dropped a file to the place after it was vacated, we may get ENOTDIR, which is also not so bad.
In any case, is it desirable to keep the enlistment still configured by jumping to loop_end in these "other" error conditions? If the reason why we cannot chdir() into it is because of some tentative glitch that may resolve by itself, retaining the enlistment data may have value, because it can be reused without the user having to recreate the enlistment when the "tentatively unavailable" directory comes back online, I guess, but how realistic would such an error be?
Show 20 quoted lines
> }
>
> strbuf_addstr(&buf, dir);
> if (remove_deleted_enlistment(&buf))
> - res = error(_("could not remove stale "
> - "scalar.repo '%s'"), dir);
> - else
> - warning(_("removing stale scalar.repo '%s'"),
> + error(_("could not remove stale "
> + "scalar.repo '%s'"), dir);
> + else {
> + warning(_("removed stale scalar.repo '%s'"),
> dir);
> + succeeded = 1;
> + }
> strbuf_release(&buf);
> - } else {
> - git_config_clear();
> + goto loop_end;
> + }So the above is "what if we fail to chdir()", which looked sensible. Then comes the "what if we don't have a usable repository there?", which was lost in [PATCH 2/3].
Show 12 quoted lines
> + switch (discover_git_directory_reason(&commondir, &gitdir)) {
> + case GIT_DIR_INVALID_OWNERSHIP:
> + warning(_("repository at '%s' has different owner"), dir);
> + goto loop_end;
> +
> + case GIT_DIR_DISCOVERED:
> + succeeded = 1;
> + break;
> +
> + default:
> + warning(_("repository not found in '%s'"), dir);
> + break;Among the error cases, INVALID_OWNERSHIP is one of the possibilities that merits specialized message to the end-user. I wonder if others also deserve to be explained, though.
- HIT_CEILING and HIT_MOUNT_POINT will happen when there is no usable repository between "dir" and the specified ceiling.
- INVALID_GITFILE and INVALID_FORMAT are signs of some repository corruption.
- DISALLOWED_BARE is unlikely to happen in the scalar context.
Show 22 quoted lines
> + }
> +
> + git_config_clear();
> +
> + the_repository = &r;
> + r.commondir = commondir.buf;
> + r.gitdir = gitdir.buf;
>
> - the_repository = &r;
> - r.commondir = commondir.buf;
> - r.gitdir = gitdir.buf;
> + if (set_recommended_config(1) >= 0)
> + succeeded = 1;
>
> - if (set_recommended_config(1) < 0)
> - res = -1;
> +loop_end:
> + if (!succeeded) {
> + res = -1;
> + warning(_("to unregister this repository from Scalar, run\n"
> + "\tgit config --global --unset --fixed-value scalar.repo \"%s\""),
> + dir);Ah, OK. So the strategy is to punt on accepting the responsibility for removing an inaccessible directory; rather, we just report that we had trouble chdir() and let the user decide. Which makes sense.
Thanks.
> } > }