Re: [PATCH v5 05/24] files-backend: move "logs/" out of TMP_RENAMED_LOG

On Wed, Mar 1, 2017 at 12:19 AM, Michael Haggerty [off-list ref] wrote:

quoted

@@ -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.

quoted

 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