Skip to contentSkip to conversation

dir.c: coding style fix

PATCH In review Karsten Blees 2 participants 6 messages Jun 15, 2016

lore
Share & tools
lore
#0 lore raw

PATCH 1/3 dir.c: coding style fix

From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?=
<[email protected]>

---
 dir.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
dir.c +6 -6
index e65888d..3068ca8 100644
@@ -557,8 +557,7 @@ int add_excludes_from_file_to_list(const char *fname,
 			buf = xrealloc(buf, size+1);
 			buf[size++] = '\n';
 		}
-	}
-	else {
+	} else {
 		size = xsize_t(st.st_size);
 		if (size == 0) {
 			close(fd);
@@ -793,9 +792,11 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 
 	group = &dir->exclude_list_group[EXC_DIRS];
 
-	/* Pop the exclude lists from the EXCL_DIRS exclude_list_group
+	/*
+	 * Pop the exclude lists from the EXCL_DIRS exclude_list_group
 	 * which originate from directories not in the prefix of the
-	 * path being checked. */
+	 * path being checked.
+	 */
 	while ((stk = dir->exclude_stack) != NULL) {
 		if (stk->baselen <= baselen &&
 		    !strncmp(dir->basebuf, base, stk->baselen))
@@ -822,8 +823,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 		if (current < 0) {
 			cp = base;
 			current = 0;
-		}
-		else {
+		} else {
 			cp = strchr(base + current + 1, '/');
 			if (!cp)
 				die("oops in prep_exclude");
Signature · 1 line
2.0.0.9646.g840d1f9.dirty
#1 lore raw

PATCH 2/3 dir.h: move struct exclude declaration to top level

From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?=
<[email protected]>

There is no actual nested struct here. Move it out for clarity.

---
 dir.h | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)
dir.h +22 -20
index 55e5345..02e3710 100644
@@ -15,6 +15,27 @@ struct dir_entry {
 #define EXC_FLAG_MUSTBEDIR 8
 #define EXC_FLAG_NEGATIVE 16
 
+struct exclude {
+	/*
+	 * This allows callers of last_exclude_matching() etc.
+	 * to determine the origin of the matching pattern.
+	 */
+	struct exclude_list *el;
+
+	const char *pattern;
+	int patternlen;
+	int nowildcardlen;
+	const char *base;
+	int baselen;
+	int flags;
+
+	/*
+	 * Counting starts from 1 for line numbers in ignore files,
+	 * and from -1 decrementing for patterns from CLI args.
+	 */
+	int srcpos;
+};
+
 /*
  * Each excludes file will be parsed into a fresh exclude_list which
  * is appended to the relevant exclude_list_group (either EXC_DIRS or
@@ -32,26 +53,7 @@ struct exclude_list {
 	/* origin of list, e.g. path to filename, or descriptive string */
 	const char *src;
 
-	struct exclude {
-		/*
-		 * This allows callers of last_exclude_matching() etc.
-		 * to determine the origin of the matching pattern.
-		 */
-		struct exclude_list *el;
-
-		const char *pattern;
-		int patternlen;
-		int nowildcardlen;
-		const char *base;
-		int baselen;
-		int flags;
-
-		/*
-		 * Counting starts from 1 for line numbers in ignore files,
-		 * and from -1 decrementing for patterns from CLI args.
-		 */
-		int srcpos;
-	} **excludes;
+	struct exclude **excludes;
 };
 
 /*
Signature · 1 line
2.0.0.9646.g840d1f9.dirty
#2 lore raw

PATCH 3/3 prep_exclude: remove the artificial PATH_MAX limit

In reply to Karsten Blees

From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?=
<[email protected]>

This fixes a segfault in git-status with long paths on Windows, where PATH_MAX is only 260.

This also fixes the problem of silently ignoring .gitignore if the full path exceeds PATH_MAX. Now add_excludes_from_file() will report if it gets ENAMETOOLONG.

---
 dir.c | 47 ++++++++++++++++++++++++++++-------------------
 dir.h |  2 +-
 2 files changed, 29 insertions(+), 20 deletions(-)
dir.c +28 -19
index 3068ca8..fcb6872 100644
@@ -799,12 +799,12 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 	 */
 	while ((stk = dir->exclude_stack) != NULL) {
 		if (stk->baselen <= baselen &&
-		    !strncmp(dir->basebuf, base, stk->baselen))
+		    !strncmp(dir->basebuf.buf, base, stk->baselen))
 			break;
 		el = &group->el[dir->exclude_stack->exclude_ix];
 		dir->exclude_stack = stk->prev;
 		dir->exclude = NULL;
-		free((char *)el->src); /* see strdup() below */
+		free((char *)el->src); /* see strbuf_detach() below */
 		clear_exclude_list(el);
 		free(stk);
 		group->nr--;
@@ -814,8 +814,17 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 	if (dir->exclude)
 		return;
 
+	/*
+	 * Lazy initialization. All call sites currently just
+	 * memset(dir, 0, sizeof(*dir)) before use. Changing all of
+	 * them seems lots of work for little benefit.
+	 */
+	if (!dir->basebuf.buf)
+		strbuf_init(&dir->basebuf, PATH_MAX);
+
 	/* Read from the parent directories and push them down. */
 	current = stk ? stk->baselen : -1;
+	strbuf_setlen(&dir->basebuf, current < 0 ? 0 : current);
 	while (current < baselen) {
 		struct exclude_stack *stk = xcalloc(1, sizeof(*stk));
 		const char *cp;
@@ -833,48 +842,47 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 		stk->baselen = cp - base;
 		stk->exclude_ix = group->nr;
 		el = add_exclude_list(dir, EXC_DIRS, NULL);
-		memcpy(dir->basebuf + current, base + current,
-		       stk->baselen - current);
+		strbuf_add(&dir->basebuf, base + current, stk->baselen - current);
+		assert(stk->baselen == dir->basebuf.len);
 
 		/* Abort if the directory is excluded */
 		if (stk->baselen) {
 			int dt = DT_DIR;
-			dir->basebuf[stk->baselen - 1] = 0;
+			dir->basebuf.buf[stk->baselen - 1] = 0;
 			dir->exclude = last_exclude_matching_from_lists(dir,
-				dir->basebuf, stk->baselen - 1,
-				dir->basebuf + current, &dt);
-			dir->basebuf[stk->baselen - 1] = '/';
+				dir->basebuf.buf, stk->baselen - 1,
+				dir->basebuf.buf + current, &dt);
+			dir->basebuf.buf[stk->baselen - 1] = '/';
 			if (dir->exclude &&
 			    dir->exclude->flags & EXC_FLAG_NEGATIVE)
 				dir->exclude = NULL;
 			if (dir->exclude) {
-				dir->basebuf[stk->baselen] = 0;
 				dir->exclude_stack = stk;
 				return;
 			}
 		}
 
-		/* Try to read per-directory file unless path is too long */
-		if (dir->exclude_per_dir &&
-		    stk->baselen + strlen(dir->exclude_per_dir) < PATH_MAX) {
-			strcpy(dir->basebuf + stk->baselen,
-					dir->exclude_per_dir);
+		/* Try to read per-directory file */
+		if (dir->exclude_per_dir) {
 			/*
 			 * dir->basebuf gets reused by the traversal, but we
 			 * need fname to remain unchanged to ensure the src
 			 * member of each struct exclude correctly
 			 * back-references its source file.  Other invocations
 			 * of add_exclude_list provide stable strings, so we
-			 * strdup() and free() here in the caller.
+			 * strbuf_detach() and free() here in the caller.
 			 */
-			el->src = strdup(dir->basebuf);
-			add_excludes_from_file_to_list(dir->basebuf,
-					dir->basebuf, stk->baselen, el, 1);
+			struct strbuf sb = STRBUF_INIT;
+			strbuf_addbuf(&sb, &dir->basebuf);
+			strbuf_addstr(&sb, dir->exclude_per_dir);
+			el->src = strbuf_detach(&sb, NULL);
+			add_excludes_from_file_to_list(el->src, el->src,
+						       stk->baselen, el, 1);
 		}
 		dir->exclude_stack = stk;
 		current = stk->baselen;
 	}
-	dir->basebuf[baselen] = '\0';
+	strbuf_setlen(&dir->basebuf, baselen);
 }
 
 /*
@@ -1671,4 +1679,5 @@ void clear_directory(struct dir_struct *dir)
 		free(stk);
 		stk = prev;
 	}
+	strbuf_release(&dir->basebuf);
 }
dir.h +1 -1
index 02e3710..6c45e9d 100644
@@ -119,7 +119,7 @@ struct dir_struct {
 	 */
 	struct exclude_stack *exclude_stack;
 	struct exclude *exclude;
-	char basebuf[PATH_MAX];
+	struct strbuf basebuf;
 };
 
 /*
Signature · 1 line
2.0.0.9646.g840d1f9.dirty
#3 lore raw
In reply to Karsten Blees
Quoted 6 lines · Karsten Blees

From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?=
<[email protected]>

---

Thanks for forwarding. I'll fix-up the Yikes (see how these two
lines show the same name in a very different way), but how did you produce the above? Is there some fix we need in the toolchain that produces patch e-mails?

Quoted 41 lines
 dir.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
dir.c +6 -6
index e65888d..3068ca8 100644
@@ -557,8 +557,7 @@ int add_excludes_from_file_to_list(const char *fname,
 			buf = xrealloc(buf, size+1);
 			buf[size++] = '\n';
 		}
-	}
-	else {
+	} else {
 		size = xsize_t(st.st_size);
 		if (size == 0) {
 			close(fd);
@@ -793,9 +792,11 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 
 	group = &dir->exclude_list_group[EXC_DIRS];
 
-	/* Pop the exclude lists from the EXCL_DIRS exclude_list_group
+	/*
+	 * Pop the exclude lists from the EXCL_DIRS exclude_list_group
 	 * which originate from directories not in the prefix of the
-	 * path being checked. */
+	 * path being checked.
+	 */
 	while ((stk = dir->exclude_stack) != NULL) {
 		if (stk->baselen <= baselen &&
 		    !strncmp(dir->basebuf, base, stk->baselen))
@@ -822,8 +823,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
 		if (current < 0) {
 			cp = base;
 			current = 0;
-		}
-		else {
+		} else {
 			cp = strchr(base + current + 1, '/');
 			if (!cp)
 				die("oops in prep_exclude");
#4 lore raw

Am 15.07.2014 00:30, schrieb Junio C Hamano:

Quoted 5 lines
Quoted 3 lines · Karsten Blees

From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?=
<[email protected]>

Quoted 9 lines
Quoted 3 lines

---

Thanks for forwarding. I'll fix-up the Yikes (see how these two
lines show the same name in a very different way), but how did you produce the above? Is there some fix we need in the toolchain that produces patch e-mails?

Hmmm...I simply thought that this is how its supposed to work. Mail headers can only contain US-ASCII, so the RFC 2047 Q-encoded-word generated by git-format-patch looked good to me.

It seems git-mailinfo doesn't handle line breaks in header lines in the mail body. I.e. if you remove the LF in the 'From:' line, everything is fine, despite the Q-encoding.

Now, 'git-format-patch --from' seems to work around the problem, but only for the 'From:' line. AFAICT there's no such option for 'Subject:', e.g. if you want to paste a patch after a scissors line, you're on your own (see the example in the git-format-patch(1) discussion section, with 'Subject:' both Q-encoded and line-wrapped).

Perhaps it should be clarified that git-format-patch output is not suitable for pasting into mail clients? Or it should print headers in plain text and let git-send-email handle the conversions?

#5 lore raw
Quoted 16 lines · Karsten Blees

Am 15.07.2014 00:30, schrieb Junio C Hamano:

Quoted 5 lines
Quoted 3 lines · Karsten Blees

From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?=
<[email protected]>

Quoted 9 lines
Quoted 3 lines

---

Thanks for forwarding. I'll fix-up the Yikes (see how these two
lines show the same name in a very different way), but how did you produce the above? Is there some fix we need in the toolchain that produces patch e-mails?

Quoted 3 lines

Hmmm...I simply thought that this is how its supposed to work. Mail headers can only contain US-ASCII, so the RFC 2047 Q-encoded-word generated by git-format-patch looked good to me.

But that quoted one is *NOT* a mail header. It is the first line of the payload of your message, and should be in plain text just like the remainder, e.g. S-o-b line that has the same name.

Quoted 3 lines

Perhaps it should be clarified that git-format-patch output is not suitable for pasting into mail clients? Or it should print headers in plain text and let git-send-email handle the conversions?

If the former is missing, then we should definitely add it to the documentation. We often see new people pasting the "From " line meant for /etc/magic and unwanted {From,Subject,Date}: in the body.

We may also want to add an option to tell it to produce an output that is suitable for pasting into mail clients. Hint, hint...