Thanks.
On Tue, May 6, 2014 at 8:55 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> 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".
Done. And the other typos too.
>
>> 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!)
I tested with the sequence above and it does indeed fix the issue.
I will put this change in a separate patch and create a test for it.
>
> 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.
Will do.
>
>> 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()?
Yes, fixed.
>
>> 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()).
Ok. Changed.
>
>> +{
>> + 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.
Ok. Changed.
>
> 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.)
Ok. Changed.
>
>> +
>> +/** 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