Merge pull request #2402 from dscho/add-i-in-c-status-and-help-gfw

built-in add -i/-p: update to latest iteration

Author: dscho

add-interactive.c

@@ -279,7 +279,7 @@ static ssize_t list_and_choose(struct add_i_state *s,
 	find_unique_prefixes(items);
 
 	for (;;) {
-		char *p, *endp;
+		char *p;
 
 		strbuf_reset(&input);
 
@@ -330,7 +330,16 @@ static ssize_t list_and_choose(struct add_i_state *s,
 				from = 0;
 				to = items->items.nr;
 			} else if (isdigit(*p)) {
-				/* A range can be specified like 5-7 or 5-. */
+				char *endp;
+				/*
+				 * A range can be specified like 5-7 or 5-.
+				 *
+				 * Note: `from` is 0-based while the user input
+				 * is 1-based, hence we have to decrement by
+				 * one. We do not have to decrement `to` even
+				 * if it is 0-based because it is an exclusive
+				 * boundary.
+				 */
 				from = strtoul(p, &endp, 10) - 1;
 				if (endp == p + sep)
 					to = from + 1;
@@ -342,7 +351,8 @@ static ssize_t list_and_choose(struct add_i_state *s,
 				}
 			}
 
-			p[sep] = '\0';
+			if (p[sep])
+				p[sep++] = '\0';
 			if (from < 0) {
 				from = find_unique(p, items);
 				if (from >= 0)
@@ -368,7 +378,7 @@ static ssize_t list_and_choose(struct add_i_state *s,
 					res += choose ? +1 : -1;
 				}
 
-			p += sep + 1;
+			p += sep;
 		}
 
 		if ((immediate && res != LIST_AND_CHOOSE_ERROR) ||
@@ -980,7 +990,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
 
 static int run_help(struct add_i_state *s, const struct pathspec *unused_ps,
 		    struct prefix_item_list *unused_files,
-		    struct list_and_choose_options *opts)
+		    struct list_and_choose_options *unused_opts)
 {
 	color_fprintf_ln(stdout, s->help_color, "status        - %s",
 			 _("show paths with changes"));

add-patch.c

@@ -658,50 +658,62 @@ static void render_diff_header(struct add_p_state *s,
 
 /* Coalesce hunks again that were split */
 static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
-		       size_t *hunk_index, int use_all, struct hunk *temp)
+		       size_t *hunk_index, int use_all, struct hunk *merged)
 {
 	size_t i = *hunk_index, delta;
 	struct hunk *hunk = file_diff->hunk + i;
-	struct hunk_header *header = &temp->header, *next;
+	/* `header` corresponds to the merged hunk */
+	struct hunk_header *header = &merged->header, *next;
 
 	if (!use_all && hunk->use != USE_HUNK)
 		return 0;
 
-	memcpy(temp, hunk, sizeof(*temp));
+	*merged = *hunk;
 	/* We simply skip the colored part (if any) when merging hunks */
-	temp->colored_start = temp->colored_end = 0;
+	merged->colored_start = merged->colored_end = 0;
 
 	for (; i + 1 < file_diff->hunk_nr; i++) {
 		hunk++;
 		next = &hunk->header;
 
+		/*
+		 * Stop merging hunks when:
+		 *
+		 * - the hunk is not selected for use, or
+		 * - the hunk does not overlap with the already-merged hunk(s)
+		 */
 		if ((!use_all && hunk->use != USE_HUNK) ||
-		    header->new_offset >= next->new_offset + temp->delta ||
+		    header->new_offset >= next->new_offset + merged->delta ||
 		    header->new_offset + header->new_count
-		    < next->new_offset + temp->delta)
+		    < next->new_offset + merged->delta)
 			break;
 
-		if (temp->start < hunk->start && temp->end > hunk->start) {
-			temp->end = hunk->end;
-			temp->colored_end = hunk->colored_end;
+		/*
+		 * If the hunks were not edited, and overlap, we can simply
+		 * extend the line range.
+		 */
+		if (merged->start < hunk->start && merged->end > hunk->start) {
+			merged->end = hunk->end;
+			merged->colored_end = hunk->colored_end;
 			delta = 0;
 		} else {
 			const char *plain = s->plain.buf;
 			size_t  overlapping_line_count = header->new_offset
-				+ header->new_count - temp->delta
+				+ header->new_count - merged->delta
 				- next->new_offset;
 			size_t overlap_end = hunk->start;
 			size_t overlap_start = overlap_end;
-			size_t overlap_next, len, i;
+			size_t overlap_next, len, j;
 
 			/*
-			 * One of the hunks was edited; let's ensure that at
-			 * least the last context line of the first hunk
-			 * overlaps with the corresponding line of the second
-			 * hunk, and then merge.
+			 * One of the hunks was edited: the modified hunk was
+			 * appended to the strbuf `s->plain`.
+			 *
+			 * Let's ensure that at least the last context line of
+			 * the first hunk overlaps with the corresponding line
+			 * of the second hunk, and then merge.
 			 */
-
-			for (i = 0; i < overlapping_line_count; i++) {
+			for (j = 0; j < overlapping_line_count; j++) {
 				overlap_next = find_next_line(&s->plain,
 							      overlap_end);
 
@@ -715,7 +727,7 @@ static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
 				if (plain[overlap_end] != ' ')
 					return error(_("expected context line "
 						       "#%d in\n%.*s"),
-						     (int)(i + 1),
+						     (int)(j + 1),
 						     (int)(hunk->end
 							   - hunk->start),
 						     plain + hunk->start);
@@ -725,13 +737,13 @@ static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
 			}
 			len = overlap_end - overlap_start;
 
-			if (len > temp->end - temp->start ||
-			    memcmp(plain + temp->end - len,
+			if (len > merged->end - merged->start ||
+			    memcmp(plain + merged->end - len,
 				   plain + overlap_start, len))
 				return error(_("hunks do not overlap:\n%.*s\n"
 					       "\tdoes not end with:\n%.*s"),
-					     (int)(temp->end - temp->start),
-					     plain + temp->start,
+					     (int)(merged->end - merged->start),
+					     plain + merged->start,
 					     (int)len, plain + overlap_start);
 
 			/*
@@ -740,23 +752,23 @@ static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
 			 * address that, we temporarily append the union of the
 			 * lines to the `plain` strbuf.
 			 */
-			if (temp->end != s->plain.len) {
+			if (merged->end != s->plain.len) {
 				size_t start = s->plain.len;
 
-				strbuf_add(&s->plain, plain + temp->start,
-					   temp->end - temp->start);
+				strbuf_add(&s->plain, plain + merged->start,
+					   merged->end - merged->start);
 				plain = s->plain.buf;
-				temp->start = start;
-				temp->end = s->plain.len;
+				merged->start = start;
+				merged->end = s->plain.len;
 			}
 
 			strbuf_add(&s->plain,
 				   plain + overlap_end,
 				   hunk->end - overlap_end);
-			temp->end = s->plain.len;
-			temp->splittable_into += hunk->splittable_into;
-			delta = temp->delta;
-			temp->delta += hunk->delta;
+			merged->end = s->plain.len;
+			merged->splittable_into += hunk->splittable_into;
+			delta = merged->delta;
+			merged->delta += hunk->delta;
 		}
 
 		header->old_count = next->old_offset + next->old_count
@@ -783,16 +795,16 @@ static void reassemble_patch(struct add_p_state *s,
 	render_diff_header(s, file_diff, 0, out);
 
 	for (i = file_diff->mode_change; i < file_diff->hunk_nr; i++) {
-		struct hunk temp = { 0 };
+		struct hunk merged = { 0 };
 
 		hunk = file_diff->hunk + i;
 		if (!use_all && hunk->use != USE_HUNK)
 			delta += hunk->header.old_count
 				- hunk->header.new_count;
 		else {
 			/* merge overlapping hunks into a temporary hunk */
-			if (merge_hunks(s, file_diff, &i, use_all, &temp))
-				hunk = &temp;
+			if (merge_hunks(s, file_diff, &i, use_all, &merged))
+				hunk = &merged;
 
 			render_hunk(s, hunk, delta, 0, out);
 
@@ -829,7 +841,7 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
 	end = hunk->end;
 	colored_end = hunk->colored_end;
 
-	memcpy(&remaining, &hunk->header, sizeof(remaining));
+	remaining = hunk->header;
 
 	file_diff->hunk_nr += splittable_into - 1;
 	ALLOC_GROW(file_diff->hunk, file_diff->hunk_nr, file_diff->hunk_alloc);
@@ -997,21 +1009,13 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)
 
 static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
 {
-	char *path = xstrdup(git_path("addp-hunk-edit.diff"));
-	int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
-	struct strbuf buf = STRBUF_INIT;
-	size_t i, j;
-	int res, copy;
-
-	if (fd < 0) {
-		res = error_errno(_("could not open '%s' for writing"), path);
-		goto edit_hunk_manually_finish;
-	}
+	size_t i;
 
-	strbuf_commented_addf(&buf, _("Manual hunk edit mode -- see bottom for "
+	strbuf_reset(&s->buf);
+	strbuf_commented_addf(&s->buf, _("Manual hunk edit mode -- see bottom for "
 				      "a quick guide.\n"));
-	render_hunk(s, hunk, 0, 0, &buf);
-	strbuf_commented_addf(&buf,
+	render_hunk(s, hunk, 0, 0, &s->buf);
+	strbuf_commented_addf(&s->buf,
 			      _("---\n"
 				"To remove '%c' lines, make them ' ' lines "
 				"(context).\n"
@@ -1020,63 +1024,51 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
 			      s->mode->is_reverse ? '+' : '-',
 			      s->mode->is_reverse ? '-' : '+',
 			      comment_line_char);
-	strbuf_commented_addf(&buf, "%s", _(s->mode->edit_hunk_hint));
+	strbuf_commented_addf(&s->buf, "%s", _(s->mode->edit_hunk_hint));
 	/*
 	 * TRANSLATORS: 'it' refers to the patch mentioned in the previous
 	 * messages.
 	 */
-	strbuf_commented_addf(&buf,
+	strbuf_commented_addf(&s->buf,
 			      _("If it does not apply cleanly, you will be "
 				"given an opportunity to\n"
 				"edit again.  If all lines of the hunk are "
 				"removed, then the edit is\n"
 				"aborted and the hunk is left unchanged.\n"));
-	if (write_in_full(fd, buf.buf, buf.len) < 0) {
-		res = error_errno(_("could not write to '%s'"), path);
-		goto edit_hunk_manually_finish;
-	}
-
-	res = close(fd);
-	fd = -1;
-	if (res < 0)
-		goto edit_hunk_manually_finish;
 
-	hunk->start = s->plain.len;
-	if (launch_editor(path, &s->plain, NULL) < 0) {
-		res = error_errno(_("could not edit '%s'"), path);
-		goto edit_hunk_manually_finish;
-	}
-	unlink(path);
+	if (strbuf_edit_interactively(&s->buf, "addp-hunk-edit.diff", NULL) < 0)
+		return -1;
 
 	/* strip out commented lines */
-	copy = s->plain.buf[hunk->start] != comment_line_char;
-	for (i = j = hunk->start; i < s->plain.len; ) {
-		if (copy)
-			s->plain.buf[j++] = s->plain.buf[i];
-		if (s->plain.buf[i++] == '\n')
-			copy = s->plain.buf[i] != comment_line_char;
+	hunk->start = s->plain.len;
+	for (i = 0; i < s->buf.len; ) {
+		const char *bol = s->buf.buf + i;
+		size_t rest = s->buf.len - i;
+		const char *eol = memchr(bol, '\n', rest);
+		size_t len = eol ? eol + 1 - bol : rest;
+
+		if (*bol != comment_line_char)
+			strbuf_add(&s->plain, bol, len);
+		i += len;
 	}
 
-	if (j == hunk->start)
-		/* User aborted by deleting everything */
-		goto edit_hunk_manually_finish;
+	hunk->end = s->plain.len;
+	if (hunk->end == hunk->start)
+		/* The user aborted editing by deleting everything */
+		return 0;
 
-	res = 1;
-	strbuf_setlen(&s->plain, j);
-	hunk->end = j;
 	recolor_hunk(s, hunk);
+
+	/*
+	 * If the hunk header is intact, parse it, otherwise simply use the
+	 * hunk header prior to editing (which will adjust `hunk->start` to
+	 * skip the hunk header).
+	 */
 	if (s->plain.buf[hunk->start] == '@' &&
-	    /* If the hunk header was deleted, simply use the original one. */
 	    parse_hunk_header(s, hunk) < 0)
-		res = -1;
+		return error(_("could not parse hunk header"));
 
-edit_hunk_manually_finish:
-	if (fd >= 0)
-		close(fd);
-	free(path);
-	strbuf_release(&buf);
-
-	return res;
+	return 1;
 }
 
 static ssize_t recount_edited_hunk(struct add_p_state *s, struct hunk *hunk,
@@ -1158,13 +1150,13 @@ static int edit_hunk_loop(struct add_p_state *s,
 	size_t plain_len = s->plain.len, colored_len = s->colored.len;
 	struct hunk backup;
 
-	memcpy(&backup, hunk, sizeof(backup));
+	backup = *hunk;
 
 	for (;;) {
 		int res = edit_hunk_manually(s, hunk);
 		if (res == 0) {
 			/* abandonded */
-			memcpy(hunk, &backup, sizeof(backup));
+			*hunk = backup;
 			return -1;
 		}
 
@@ -1180,7 +1172,7 @@ static int edit_hunk_loop(struct add_p_state *s,
 		/* Drop edits (they were appended to s->plain) */
 		strbuf_setlen(&s->plain, plain_len);
 		strbuf_setlen(&s->colored, colored_len);
-		memcpy(hunk, &backup, sizeof(backup));
+		*hunk = backup;
 
 		/*
 		 * TRANSLATORS: do not translate [y/n]

builtin/add.c

@@ -191,9 +191,6 @@ int run_add_interactive(const char *revision, const char *patch_mode,
 	struct argv_array argv = ARGV_ARRAY_INIT;
 	int use_builtin_add_i =
 		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
-	if (use_builtin_add_i < 0)
-		git_config_get_bool("add.interactive.usebuiltin",
-				    &use_builtin_add_i);
 
 	if (use_builtin_add_i == 1) {
 		enum add_p_mode mode;