On 05/06/2014 12:57 AM, Ronnie Sahlberg wrote:
> Add two new functions, reflog_exists and delete_reflog to hide the internal
Need comma after "delete_reflog".
> reflog implementation (that they are files under .git/logs/...) from callers.
> Update checkout.c to use these functions in update_refs_for_switch instead of
> building pathnames and calling out to file access functions. Update reflog.c
> to use these too check if the reflog exists. Now there are still many places
s/too/to/
> in reflog.c where we are still leaking the reflog storage implementation but
> this at least reduces the number of such dependencies by one. Finally
> change two places in refs.c itself to use the new function to check if a ref
> exists or not isntead of build-path-and-stat(). Now, this is strictly not all
s/isntead/instead/
> that important since these are in parts of refs that are implementing the
> actual file storage backend but on the other hand it will not hurt either.
As an aside, I expect long term that reflog handling will be married
more tightly to reference handling and probably both will become
pluggable via a single mechanism.
> In config.c we also change to use the existing function ref_exists instead of
s/config.c/checkout.c/
> checking if the loose ref file exist. The previous code made the assumption
> that the branch we switched from must exist as a loose ref and thus checking
> the file would be sufficent. I think that assumption is always true in the
s/sufficent/sufficient/
> current code but it is still somewhat fragile since if git would change so that
> the checkedout branch could exist as a packed ref without a corresponding
s/checkedout/checked-out/
> loose ref then this subtle 'does the lose ref not exist' check would suddenly
> fail.
I don't understand. It can *already* be the case that the checked-out
branch only exists as a packed reference:
$ git checkout master
$ git pack-refs --all
$ find .git/refs -type f
$
So we already have a bug:
$ git config core.logallrefupdates true
$ git commit -m Initial --allow-empty
[master (root-commit) 3a03d51] Initial
$ git branch foo
$ git pack-refs --all
$ find .git/{refs,logs} -type f
.git/logs/HEAD
.git/logs/refs/heads/foo
.git/logs/refs/heads/master
$ git checkout foo
Switched to branch 'foo'
$ find .git/{refs,logs} -type f
.git/logs/HEAD
.git/logs/refs/heads/foo
$ history | tail -10
Note that the reflog for refs/heads/master is incorrectly deleted when
we run "git checkout foo".
By the way, in case it wasn't clear to you I think the code in question
is trying to avoid leaving a reflog file behind when leaving an orphan
branch that hasn't actually been created yet. So I think your change to
using ref_exists() will indeed fix the bug (but please test!)
Given that this is a real bug, I suggest breaking this change out into a
separate patch with a corresponding addition to the test suite.
> Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx>
> ---
> builtin/checkout.c | 8 ++------
> builtin/reflog.c | 2 +-
> refs.c | 20 ++++++++++++++------
> refs.h | 6 ++++++
> 4 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index ff44921..f1dc56e 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -651,12 +651,8 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
> }
> }
> if (old->path && old->name) {
> - char log_file[PATH_MAX], ref_file[PATH_MAX];
> -
> - git_snpath(log_file, sizeof(log_file), "logs/%s", old->path);
> - git_snpath(ref_file, sizeof(ref_file), "%s", old->path);
> - if (!file_exists(ref_file) && file_exists(log_file))
> - remove_path(log_file);
> + if (!ref_exists(old->path) && reflog_exists(old->path))
> + delete_reflog(old->path);
> }
> }
> remove_branch_state();
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index c12a9784..0e7ea74 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -369,7 +369,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,
> if (!lock)
> return error("cannot lock ref '%s'", ref);
> log_file = git_pathdup("logs/%s", ref);
> - if (!file_exists(log_file))
> + if (!ref_exists(ref))
Shouldn't this be reflog_exists()?
> goto finish;
> if (!cmd->dry_run) {
> newlog_path = git_pathdup("logs/%s.lock", ref);
> diff --git a/refs.c b/refs.c
> index e59bc18..7d12ac7 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2013,7 +2013,6 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
>
> *log = NULL;
> for (p = ref_rev_parse_rules; *p; p++) {
> - struct stat st;
> unsigned char hash[20];
> char path[PATH_MAX];
> const char *ref, *it;
> @@ -2022,12 +2021,9 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
> ref = resolve_ref_unsafe(path, hash, 1, NULL);
> if (!ref)
> continue;
> - if (!stat(git_path("logs/%s", path), &st) &&
> - S_ISREG(st.st_mode))
> + if (reflog_exists(path))
> it = path;
> - else if (strcmp(ref, path) &&
> - !stat(git_path("logs/%s", ref), &st) &&
> - S_ISREG(st.st_mode))
> + else if (strcmp(ref, path) && reflog_exists(ref))
> it = ref;
> else
> continue;
> @@ -3030,6 +3026,18 @@ int read_ref_at(const char *refname, unsigned long at_time, int cnt,
> return 1;
> }
>
> +int reflog_exists(const char *ref)
We try to use the variable name "refname" for variables that hold the
full names of references (the same comment applies to delete_reflog()).
> +{
> + struct stat st;
> +
> + return !lstat(git_path("logs/%s", ref), &st) && S_ISREG(st.st_mode);
> +}
> +
> +int delete_reflog(const char *ref)
> +{
> + return remove_path(git_path("logs/%s", ref));
> +}
> +
> static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *cb_data)
> {
> unsigned char osha1[20], nsha1[20];
> diff --git a/refs.h b/refs.h
> index 71e39b9..5a93f27 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -159,6 +159,12 @@ extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,
> unsigned char *sha1, char **msg,
> unsigned long *cutoff_time, int *cutoff_tz, int *cutoff_cnt);
>
> +/** Check if a particular ref log exists */
> +extern int reflog_exists(const char *);
We usually spell s/ref log/reflog/. The same thing below.
My preference is that you give a name "refname" to the parameter in this
function signature. That makes it clear at a glance what is expected.
(Though I admit that this practice is far from universally practiced in
the Git project so maybe other people disagree.)
> +
> +/** Delete a ref log */
> +extern int delete_reflog(const char *);
> +
> /* iterate over reflog entries */
> typedef int each_reflog_ent_fn(unsigned char *osha1, unsigned char *nsha1, const char *, unsigned long, int, const char *, void *);
> int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn, void *cb_data);
>
Thanks!
Michael
--
Michael Haggerty
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html