[PATCH v2 0/2] add a reflog_exists and delete_reflog abstraction

[PATCH v2 0/2] add a reflog_exists and delete_reflog abstraction

[Ronnie Sahlberg]

This is a series 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 under .git/logs)

Additionally we change checkout.c to use ref_exists instead of file_exists
when checking for ref existence. This fixes a bug when checkout could delete
a valid reflog file if the branch was a packed ref. The tests have been updated
to test for this bug.


Version 2:
 - Typos and fixes suggested by mhagger.
 - Break the checkout-deletes reflog bugfix out into a separate patch.


Ronnie Sahlberg (2):
  refs.c: add new functions reflog_exists and delete_reflog
  checkout.c: use ref_exists instead of file_exist

 builtin/checkout.c |  8 ++------
 builtin/reflog.c   |  2 +-
 refs.c             | 21 +++++++++++++++------
 refs.h             |  6 ++++++
 t/t1410-reflog.sh  |  8 ++++++++
 5 files changed, 32 insertions(+), 13 deletions(-)

-- 
2.0.0.rc1.354.g7561c2b.dirty

[PATCH v2 1/2] 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 to 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.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/checkout.c |  7 +++----
 builtin/reflog.c   |  2 +-
 refs.c             | 21 +++++++++++++++------
 refs.h             |  6 ++++++
 4 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index ff44921..929f5bd 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -651,12 +651,11 @@ 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];
+			char 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 (!file_exists(ref_file) && reflog_exists(old->path))
+				delete_reflog(old->path);
 		}
 	}
 	remove_branch_state();
diff --git a/builtin/reflog.c b/builtin/reflog.c
index c12a9784..e8a8fb1 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 (!reflog_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..302a2b3 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,19 @@ int read_ref_at(const char *refname, unsigned long at_time, int cnt,
 	return 1;
 }
 
+int reflog_exists(const char *refname)
+{
+	struct stat st;
+
+	return !lstat(git_path("logs/%s", refname), &st) &&
+		S_ISREG(st.st_mode);
+}
+
+int delete_reflog(const char *refname)
+{
+	return remove_path(git_path("logs/%s", refname));
+}
+
 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 c1cb4b4..8bd815d 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 reflog exists */
+extern int reflog_exists(const char *refname);
+
+/** Delete a reflog */
+extern int delete_reflog(const char *refname);
+
 /* 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.354.g7561c2b.dirty

[PATCH v2 2/2] checkout.c: use ref_exists instead of file_exist

[Ronnie Sahlberg]

Change checkout.c to check if a ref exists instead of checking if a loose ref
file exists when deciding if to delete an orphaned log file. Otherwise, if a
ref only exists as a packed ref without a corresponding loose ref for the
currently checked out branch, we risk that the reflog will be deleted when we
switch to a different branch.

Update the reflog tests to check for this bug.

The following reproduces the bug:
$ 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 ...

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/checkout.c | 5 +----
 t/t1410-reflog.sh  | 8 ++++++++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 929f5bd..f1dc56e 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -651,10 +651,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 			}
 		}
 		if (old->path && old->name) {
-			char ref_file[PATH_MAX];
-
-			git_snpath(ref_file, sizeof(ref_file), "%s", old->path);
-			if (!file_exists(ref_file) && reflog_exists(old->path))
+			if (!ref_exists(old->path) && reflog_exists(old->path))
 				delete_reflog(old->path);
 		}
 	}
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
-- 
2.0.0.rc1.354.g7561c2b.dirty

Re: [PATCH v2 0/2] add a reflog_exists and delete_reflog abstraction

[Michael Haggerty]

On 05/07/2014 12:45 AM, Ronnie Sahlberg wrote:
> This is a series 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 under .git/logs)
> 
> Additionally we change checkout.c to use ref_exists instead of file_exists
> when checking for ref existence. This fixes a bug when checkout could delete
> a valid reflog file if the branch was a packed ref. The tests have been updated
> to test for this bug.
> 
> 
> Version 2:
>  - Typos and fixes suggested by mhagger.
>  - Break the checkout-deletes reflog bugfix out into a separate patch.
> 
> 
> Ronnie Sahlberg (2):
>   refs.c: add new functions reflog_exists and delete_reflog
>   checkout.c: use ref_exists instead of file_exist
> 
>  builtin/checkout.c |  8 ++------
>  builtin/reflog.c   |  2 +-
>  refs.c             | 21 +++++++++++++++------
>  refs.h             |  6 ++++++
>  t/t1410-reflog.sh  |  8 ++++++++
>  5 files changed, 32 insertions(+), 13 deletions(-)

+1 Looks good to me.  Thanks!

Michael

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

Re: [PATCH v2 0/2] add a reflog_exists and delete_reflog abstraction

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> +1 Looks good to me.  Thanks!

Will queue with your Ack; thanks.