Thanks.

On Tue, May 6, 2014 at 8:55 AM, Michael Haggerty <mhag...@alum.mit.edu> 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 <sahlb...@google.com>
>> ---
>>  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
> mhag...@alum.mit.edu
> http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to