[PATCH] add a reflog_exists and delete_reflog abstraction

[PATCH] add a reflog_exists and delete_reflog abstraction

[Ronnie Sahlberg]

This is a single patch that adds two new functions to try to hide the reflog
implementation details from the callers in checkout.c and reflog.c.
It adds new functions to test if a reflog exists and to delete it, thus
allowing checkout.c to perform this if-test-then-delete operation without
having to know the internal implementation of reflogs (i.e. that they are files
that live unde r.git/logs)

It also changes checkout.c to use ref_exists when checking for whether a ref
exists or not instead of checking if the loose ref file exists or not.
Using ref_exists instead both hides the reflog implementation details from
checkout.c as well as making the code more robust against future changes.
It currently has a hard assumption that the loose ref file must exist at this
stage or else it would end up deleting the reflog which is true today, as far
as I can tell, but would break if git would change such that we could have
a branch checked out without having a loose ref file for that branch.


Ronnie Sahlberg (1):
  refs.c: add new functions reflog_exists and delete_reflog

 builtin/checkout.c |  8 ++------
 builtin/reflog.c   |  2 +-
 refs.c             | 20 ++++++++++++++------
 refs.h             |  6 ++++++
 4 files changed, 23 insertions(+), 13 deletions(-)

-- 
2.0.0.rc1.351.gd2b7e18.dirty

[PATCH] refs.c: add new functions reflog_exists and delete_reflog

[Ronnie Sahlberg]

Add two new functions, reflog_exists and delete_reflog to hide the internal
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
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
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.

In config.c we also change to use the existing function ref_exists instead of
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
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
loose ref then this subtle 'does the lose ref not exist' check would suddenly
fail.

Signed-off-by: Ronnie Sahlberg <sahlberg@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))
 		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)
+{
+	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 *);
+
+/** 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);
-- 
2.0.0.rc1.351.gd2b7e18.dirty

Re: [PATCH] refs.c: add new functions reflog_exists and delete_reflog

[Michael Haggerty]

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@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()?

>  		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@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH] refs.c: add new functions reflog_exists and delete_reflog

[Ronnie Sahlberg]

Thanks.

On Tue, May 6, 2014 at 8:55 AM, Michael Haggerty <mhagger@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 <sahlberg@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
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

Re: [PATCH] add a reflog_exists and delete_reflog abstraction

[Michael Haggerty]

On 05/06/2014 12:57 AM, Ronnie Sahlberg wrote:
> This is a single patch that adds two new functions to try to hide the reflog
> implementation details from the callers in checkout.c and reflog.c.
> It adds new functions to test if a reflog exists and to delete it, thus
> allowing checkout.c to perform this if-test-then-delete operation without
> having to know the internal implementation of reflogs (i.e. that they are files
> that live unde r.git/logs)
> 
> It also changes checkout.c to use ref_exists when checking for whether a ref
> exists or not instead of checking if the loose ref file exists or not.
> Using ref_exists instead both hides the reflog implementation details from
> checkout.c as well as making the code more robust against future changes.
> It currently has a hard assumption that the loose ref file must exist at this
> stage or else it would end up deleting the reflog which is true today, as far
> as I can tell, but would break if git would change such that we could have
> a branch checked out without having a loose ref file for that branch.

For single patches, people usually don't send a separate cover letter.
Any comments that you want to make that are not suitable for the commit
message can go between the "---" line and the diffstat of the patch email.

Regarding this change, I think before too long we will also need an API
to turn reflogs on for a reference.  We might want to add a flag to ref
transaction entries that cause the reflog to be created if it doesn't
already exist.  Reflogs can currently be created for a reference via the
config setting "core.logAllRefUpdates" or (for branches) by "git branch
--create-reflog" (maybe there are others).

Several tests in the test suite currently create reflog files by hand.
Thus we might also need a way to create reflogs at the command line by
the time we implement pluggable reference backends.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH] add a reflog_exists and delete_reflog abstraction

[Ronnie Sahlberg]

On Tue, May 6, 2014 at 8:23 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/06/2014 12:57 AM, Ronnie Sahlberg wrote:
>> This is a single patch that adds two new functions to try to hide the reflog
>> implementation details from the callers in checkout.c and reflog.c.
>> It adds new functions to test if a reflog exists and to delete it, thus
>> allowing checkout.c to perform this if-test-then-delete operation without
>> having to know the internal implementation of reflogs (i.e. that they are files
>> that live unde r.git/logs)
>>
>> It also changes checkout.c to use ref_exists when checking for whether a ref
>> exists or not instead of checking if the loose ref file exists or not.
>> Using ref_exists instead both hides the reflog implementation details from
>> checkout.c as well as making the code more robust against future changes.
>> It currently has a hard assumption that the loose ref file must exist at this
>> stage or else it would end up deleting the reflog which is true today, as far
>> as I can tell, but would break if git would change such that we could have
>> a branch checked out without having a loose ref file for that branch.
>
> For single patches, people usually don't send a separate cover letter.
> Any comments that you want to make that are not suitable for the commit
> message can go between the "---" line and the diffstat of the patch email.
>
> Regarding this change, I think before too long we will also need an API
> to turn reflogs on for a reference.  We might want to add a flag to ref
> transaction entries that cause the reflog to be created if it doesn't
> already exist.  Reflogs can currently be created for a reference via the
> config setting "core.logAllRefUpdates" or (for branches) by "git branch
> --create-reflog" (maybe there are others).

Yes. I agree.

I think we need a flag to ref_transaction_create and ref_transaction_update to
create a new reflog.

Then I also think we need to add a new transaction command,
ref_transaction_reflog() that can be used to either append or overwrite the
reflog.  Builtin/reflog.c will need an API to create/overwrite the reflog
with new text.

And finally we also need an api to create symrefs.
ref_transaction_symref() or something.


But this will wait until my current patch series is merged.

>
> Several tests in the test suite currently create reflog files by hand.
> Thus we might also need a way to create reflogs at the command line by
> the time we implement pluggable reference backends.

I can add this once we have an api.

>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

Re: [PATCH] add a reflog_exists and delete_reflog abstraction

[Junio C Hamano]

Ronnie Sahlberg <sahlberg@google.com> writes:

> It currently has a hard assumption that the loose ref file must exist at this
> stage or else it would end up deleting the reflog which is true today, as far
> as I can tell, but would break if git would change such that we could have
> a branch checked out without having a loose ref file for that branch.

Hmmmm.  Do you mean this sequence will break?

        : gitster x; git init lo
        Initialized empty Git repository in /var/tmp/x/lo/.git/
        : gitster x; cd lo
        : gitster lo/master; git commit --allow-empty -m initial
        [master (root-commit) db2b430] initial
        : gitster lo/master; git branch next

Now we have two branches, master and next, and we are on master.

        : gitster lo/master; git pack-refs --all
        : gitster lo/master; ls .git/refs/heads
        ./  ../
        : gitster lo/master; cat .git/packed-refs
        # pack-refs with: peeled fully-peeled 
        db2b43072749258d1e3c119c9570349d77c26b3a refs/heads/master
        db2b43072749258d1e3c119c9570349d77c26b3a refs/heads/next

And loose refs are fully packed.

        : gitster lo/master; git checkout next
        Switched to branch 'next'
        : gitster lo/next; ls .git/refs/heads
        ./  ../

Re: [PATCH] add a reflog_exists and delete_reflog abstraction

[Ronnie Sahlberg]

On Tue, May 6, 2014 at 12:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> It currently has a hard assumption that the loose ref file must exist at this
>> stage or else it would end up deleting the reflog which is true today, as far
>> as I can tell, but would break if git would change such that we could have
>> a branch checked out without having a loose ref file for that branch.
>
> Hmmmm.  Do you mean this sequence will break?

As Michael suggested, this is already broken :-(
This sequence will delete the reflog from master :

    $ git init-db
    $ git config core.logallrefupdates true
    $ git commit -m Initial --allow-empty
        [master (root-commit) bb11abe] Initial
    $ git reflog master
        [8561dcb master@{0}: commit (initial): Initial]
    $ find .git/{refs,logs} -type f | grep master
        [.git/refs/heads/master]
        [.git/logs/refs/heads/master]
    $ git branch foo
    $ git pack-refs --all
    $ find .git/{refs,logs} -type f | grep master
        [.git/logs/refs/heads/master]
    $ git checkout foo
    $ find .git/{refs,logs} -type f | grep master
        ... reflog file is missing ...
    $ git reflog master
        ... nothing ...

I am adding a test for this in the next set of patches :

diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 236b13a..8cab06f 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -245,4 +245,12 @@ test_expect_success 'gc.reflogexpire=false' '

 '

+test_expect_success 'checkout should not delete log for packed ref' '
+       test $(git reflog master | wc -l) = 4 &&
+       git branch foo &&
+       git pack-refs --all &&
+       git checkout foo &&
+       test $(git reflog master | wc -l) = 4
+'
+
 test_done





>
>         : gitster x; git init lo
>         Initialized empty Git repository in /var/tmp/x/lo/.git/
>         : gitster x; cd lo
>         : gitster lo/master; git commit --allow-empty -m initial
>         [master (root-commit) db2b430] initial
>         : gitster lo/master; git branch next
>
> Now we have two branches, master and next, and we are on master.
>
>         : gitster lo/master; git pack-refs --all
>         : gitster lo/master; ls .git/refs/heads
>         ./  ../
>         : gitster lo/master; cat .git/packed-refs
>         # pack-refs with: peeled fully-peeled
>         db2b43072749258d1e3c119c9570349d77c26b3a refs/heads/master
>         db2b43072749258d1e3c119c9570349d77c26b3a refs/heads/next
>
> And loose refs are fully packed.
>
>         : gitster lo/master; git checkout next
>         Switched to branch 'next'
>         : gitster lo/next; ls .git/refs/heads
>         ./  ../
>