git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 11/17] refs.c: simplify strbufs in reflog setup and writing

From
Jeff King <peff@peff.net>
Date
Aug 10, 2015, 09:36 UTC
Message-ID
<20150810093638.GK30981@sigill.intra.peff.net>
In-Reply-To
<20150810092731.GA9027@sigill.intra.peff.net>

Commit 1a83c24 (git_snpath(): retire and replace with strbuf_git_path(), 2014-11-30) taught log_ref_setup and log_ref_write_1 to take a strbuf parameter, rather than a bare string. It then makes an alias to the strbuf's "buf" field under the original name.

This made the original diff much shorter, but the resulting code is more complicated that it needs to be. Since we've aliased the pointer, we drop our reference to the strbuf to ensure we don't accidentally change it. But if we simply drop our alias and use "logfile.buf" directly, we do not have to worry about this aliasing. It's a larger diff, but the resulting code is simpler.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)
diff --git a/refs.c b/refs.c
index 06f95c4..3666132 100644
--- a/refs.c
+++ b/refs.c
@@ -3150,46 +3150,42 @@ static int should_autocreate_reflog(const char *refname)
  * should_autocreate_reflog returns non-zero.  Otherwise, create it
  * regardless of the ref name.  Fill in *err and return -1 on failure.
  */
-static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create)
+static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
 {
 	int logfd, oflags = O_APPEND | O_WRONLY;
-	char *logfile;
 
-	strbuf_git_path(sb_logfile, "logs/%s", refname);
-	logfile = sb_logfile->buf;
-	/* make sure the rest of the function can't change "logfile" */
-	sb_logfile = NULL;
+	strbuf_git_path(logfile, "logs/%s", refname);
 	if (force_create || should_autocreate_reflog(refname)) {
-		if (safe_create_leading_directories(logfile) < 0) {
+		if (safe_create_leading_directories(logfile->buf) < 0) {
 			strbuf_addf(err, "unable to create directory for %s: "
-				    "%s", logfile, strerror(errno));
+				    "%s", logfile->buf, strerror(errno));
 			return -1;
 		}
 		oflags |= O_CREAT;
 	}
 
-	logfd = open(logfile, oflags, 0666);
+	logfd = open(logfile->buf, oflags, 0666);
 	if (logfd < 0) {
 		if (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR))
 			return 0;
 
 		if (errno == EISDIR) {
-			if (remove_empty_directories(logfile)) {
+			if (remove_empty_directories(logfile->buf)) {
 				strbuf_addf(err, "There are still logs under "
-					    "'%s'", logfile);
+					    "'%s'", logfile->buf);
 				return -1;
 			}
-			logfd = open(logfile, oflags, 0666);
+			logfd = open(logfile->buf, oflags, 0666);
 		}
 
 		if (logfd < 0) {
 			strbuf_addf(err, "unable to append to %s: %s",
-				    logfile, strerror(errno));
+				    logfile->buf, strerror(errno));
 			return -1;
 		}
 	}
 
-	adjust_shared_perm(logfile);
+	adjust_shared_perm(logfile->buf);
 	close(logfd);
 	return 0;
 }
@@ -3233,36 +3229,32 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
 
 static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 			   const unsigned char *new_sha1, const char *msg,
-			   struct strbuf *sb_log_file, int flags,
+			   struct strbuf *log_file, int flags,
 			   struct strbuf *err)
 {
 	int logfd, result, oflags = O_APPEND | O_WRONLY;
-	char *log_file;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
 
-	result = log_ref_setup(refname, sb_log_file, err, flags & REF_FORCE_CREATE_REFLOG);
+	result = log_ref_setup(refname, log_file, err, flags & REF_FORCE_CREATE_REFLOG);
 
 	if (result)
 		return result;
-	log_file = sb_log_file->buf;
-	/* make sure the rest of the function can't change "log_file" */
-	sb_log_file = NULL;
 
-	logfd = open(log_file, oflags);
+	logfd = open(log_file->buf, oflags);
 	if (logfd < 0)
 		return 0;
 	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
 				  git_committer_info(0), msg);
 	if (result) {
-		strbuf_addf(err, "unable to append to %s: %s", log_file,
+		strbuf_addf(err, "unable to append to %s: %s", log_file->buf,
 			    strerror(errno));
 		close(logfd);
 		return -1;
 	}
 	if (close(logfd)) {
-		strbuf_addf(err, "unable to append to %s: %s", log_file,
+		strbuf_addf(err, "unable to append to %s: %s", log_file->buf,
 			    strerror(errno));
 		return -1;
 	}

-- 
2.5.0.414.g670f2a4
Previous: Jeff KingNext: Michael Haggerty
Message 20 of 34 in “removing questionable uses of git_path”
  1. 0/17 removing questionable uses of git_pathJeff King, Aug 10, 2015
  2. 01/17 cache.h: clarify documentation for git_path, et alJeff King, Aug 10, 2015
  3. 02/17 cache.h: complete set of git_path_submodule helpersJeff King, Aug 10, 2015
  4. 03/17 t5700: modernize styleJeff King, Aug 10, 2015
  5. 04/17 add_to_alternates_file: don't add duplicate entriesJeff King, Aug 10, 2015
  6. Michael HaggertyAug 11, 2015
  7. Jeff KingAug 11, 2015
  8. 05/17 remove hold_lock_file_for_appendJeff King, Aug 10, 2015
  9. Junio C HamanoAug 10, 2015
  10. Jeff KingAug 11, 2015
  11. 06/17 prefer git_pathdup to git_path in some possibly-dangerous casesJeff King, Aug 10, 2015
  12. 07/17 prefer mkpathdup to mkpath in assignmentsJeff King, Aug 10, 2015
  13. 08/17 remote.c: drop extraneous local variable from migrate_fileJeff King, Aug 10, 2015
  14. 09/17 refs.c: remove extra git_path calls from read_loose_refsJeff King, Aug 10, 2015
  15. 10/17 path.c: drop git_path_submoduleJeff King, Aug 10, 2015
  16. Junio C HamanoAug 10, 2015
  17. Junio C HamanoAug 10, 2015
  18. Junio C HamanoAug 10, 2015
  19. Jeff KingAug 11, 2015
  20. 11/17 refs.c: simplify strbufs in reflog setup and writingJeff King, Aug 10, 2015
  21. Michael HaggertyAug 10, 2015
  22. Jeff KingAug 10, 2015
  23. 12/17 refs.c: avoid repeated git_path calls in rename_tmp_logJeff King, Aug 10, 2015
  24. 13/17 refs.c: avoid git_path assignment in lock_ref_sha1_basicJeff King, Aug 10, 2015
  25. 14/17 refs.c: remove_empty_directories can take a strbufJeff King, Aug 10, 2015
  26. 15/17 find_hook: keep our own static bufferJeff King, Aug 10, 2015
  27. 16/17 get_repo_path: refactor path-allocationJeff King, Aug 10, 2015
  28. 17/17 memoize common git-path "constant" filesJeff King, Aug 10, 2015
  29. Michael HaggertyAug 10, 2015
  30. Jeff KingAug 10, 2015
  31. Michael HaggertyAug 10, 2015
  32. Junio C HamanoAug 10, 2015
  33. Jeff KingAug 10, 2015
  34. Duy NguyenAug 15, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.