On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> This makes reflog path building consistent, always in the form of
>
> strbuf_git_path(sb, "logs/%s", refname);
>
> It reduces the mental workload a bit in the next patch when that
> function call is converted.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
> ---
> refs/files-backend.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 435db1293..69946b0de 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -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.
> 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 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.
For that matter, what if a user tries to rename a worktree ref into a
common ref or vice versa?
> if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
> [...]
Michael
