On Wed, Mar 1, 2017 at 12:19 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
>> @@ -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.
>> 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.
> 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