Re: [PATCH v5 05/24] files-backend: move "logs/" out of TMP_RENAMED_LOG
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Mar 2, 2017, 13:07 UTC
- Message-ID
- <CACsJy8AoZt-po5R7+1qPBqTzJ6aiVkKif9Lyp9y0ZwRM09Tj-Q@mail.gmail.com>
- In-Reply-To
- <cb86192d-8644-bed3-1e73-821931589e79@alum.mit.edu>
On Wed, Mar 1, 2017 at 12:19 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 10 quoted lines
>> @@ -2513,7 +2513,7 @@ static int files_delete_refs(struct ref_store *ref_store, >> * IOW, to avoid cross device rename errors, the temporary renamed log must >> * live into logs/refs. >> */ >> -#define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log" >> +#define TMP_RENAMED_LOG "refs/.tmp-renamed-log" > > The constant name feels a little bit misleading now that it is not the > name of a logfile but rather a reference name. OTOH "tmp-renamed-log" is > *in* the reference name so I guess it's not really wrong.
Heh.. I had a similar internal debate and almost renamed it to tmp_renamed_refname. But then it's technically not a valid ref name either (starting with a leading dot). My lazy side came in and declared that doing nothing was always the right way.
Show 31 quoted lines
>> struct rename_cb {
>> const char *tmp_renamed_log;
>> @@ -2549,7 +2549,7 @@ static int rename_tmp_log(const char *newrefname)
>> int ret;
>>
>> strbuf_git_path(&path, "logs/%s", newrefname);
>> - strbuf_git_path(&tmp, TMP_RENAMED_LOG);
>> + strbuf_git_path(&tmp, "logs/%s", TMP_RENAMED_LOG);
>> cb.tmp_renamed_log = tmp.buf;
>> ret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb);
>> if (ret) {
>> @@ -2626,12 +2626,12 @@ static int files_rename_ref(struct ref_store *ref_store,
>> return 1;
>>
>> strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
>> - strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
>> + strbuf_git_path(&tmp_renamed_log, "logs/%s", TMP_RENAMED_LOG);
>> ret = log && rename(sb_oldref.buf, tmp_renamed_log.buf);
>> strbuf_release(&sb_oldref);
>> strbuf_release(&tmp_renamed_log);
>> if (ret)
>> - return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
>> + return error("unable to move logfile logs/%s to logs/"TMP_RENAMED_LOG": %s",
>> oldrefname, strerror(errno));
>
> It seems like it would be preferable to use `sb_oldref.buf` and
> `tmp.buf` when building the error message. But I guess that `tmp.buf`
> might include some path preceding "logs/" that is unwanted in the error
> message? But it's a shame to hardcode the file naming scheme here again.
>
> Maybe we *do* want the path in the error message?It's an error, every piece of details matters. So yeah I'm inclined we should print full path.
Show 6 quoted lines
> It just occurred to me: this temporary logfile lives in the main > repository, right? What if a worktree reference is being renamed? Part > of the advertised use of worktrees is that the worktree might live far > from the main directory, or even on removable media. But it's not > possible to rename files across partitions. Maybe this will come out in > the wash once worktrees are ref_stores themselves.
The actual working directory may be separated, but all the things that belong to .git (even of a linked worktree) stay in the main worktree's .git directory. And I don't think we ever support having a .git directory on multiple partitions. You can rename refs freely even when the worktree is on a detached removable drive.
> For that matter, what if a user tries to rename a worktree ref into a > common ref or vice versa?
Interesting. It should work, it's just a rename(".git/worktrees/blah/refs/bisect/good", ".git/refs/heads/saved") after the path translation done by git_path().
-- Duy