* [PATCH 1/7] Enable wt-status output to a given FILE pointer.
@ 2007-09-18 0:06 Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 2/7] Enable wt-status to run against non-standard index file Kristian Høgsberg
0 siblings, 1 reply; 18+ messages in thread
From: Kristian Høgsberg @ 2007-09-18 0:06 UTC (permalink / raw)
To: git; +Cc: Kristian Høgsberg
Still defaults to stdout, but you can now override wt_status.fp after
calling wt_status_prepare().
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
color.c | 18 ++++++------
color.h | 4 +-
wt-status.c | 85 ++++++++++++++++++++++++++++++----------------------------
wt-status.h | 3 ++
4 files changed, 58 insertions(+), 52 deletions(-)
diff --git a/color.c b/color.c
index 09d82ee..124ba33 100644
--- a/color.c
+++ b/color.c
@@ -135,39 +135,39 @@ int git_config_colorbool(const char *var, const char *value)
return git_config_bool(var, value);
}
-static int color_vprintf(const char *color, const char *fmt,
+static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
va_list args, const char *trail)
{
int r = 0;
if (*color)
- r += printf("%s", color);
- r += vprintf(fmt, args);
+ r += fprintf(fp, "%s", color);
+ r += vfprintf(fp, fmt, args);
if (*color)
- r += printf("%s", COLOR_RESET);
+ r += fprintf(fp, "%s", COLOR_RESET);
if (trail)
- r += printf("%s", trail);
+ r += fprintf(fp, "%s", trail);
return r;
}
-int color_printf(const char *color, const char *fmt, ...)
+int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
{
va_list args;
int r;
va_start(args, fmt);
- r = color_vprintf(color, fmt, args, NULL);
+ r = color_vfprintf(fp, color, fmt, args, NULL);
va_end(args);
return r;
}
-int color_printf_ln(const char *color, const char *fmt, ...)
+int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...)
{
va_list args;
int r;
va_start(args, fmt);
- r = color_vprintf(color, fmt, args, "\n");
+ r = color_vfprintf(fp, color, fmt, args, "\n");
va_end(args);
return r;
}
diff --git a/color.h b/color.h
index 88bb8ff..6809800 100644
--- a/color.h
+++ b/color.h
@@ -6,7 +6,7 @@
int git_config_colorbool(const char *var, const char *value);
void color_parse(const char *var, const char *value, char *dst);
-int color_printf(const char *color, const char *fmt, ...);
-int color_printf_ln(const char *color, const char *fmt, ...);
+int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
+int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
#endif /* COLOR_H */
diff --git a/wt-status.c b/wt-status.c
index 10ce6ee..eeb1691 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -52,31 +52,33 @@ void wt_status_prepare(struct wt_status *s)
head = resolve_ref("HEAD", sha1, 0, NULL);
s->branch = head ? xstrdup(head) : NULL;
s->reference = "HEAD";
+ s->fp = stdout;
}
-static void wt_status_print_cached_header(const char *reference)
+static void wt_status_print_cached_header(struct wt_status *s)
{
const char *c = color(WT_STATUS_HEADER);
- color_printf_ln(c, "# Changes to be committed:");
- if (reference) {
- color_printf_ln(c, "# (use \"git reset %s <file>...\" to unstage)", reference);
+ color_fprintf_ln(s->fp, c, "# Changes to be committed:");
+ if (s->reference) {
+ color_fprintf_ln(s->fp, c, "# (use \"git reset %s <file>...\" to unstage)", s->reference);
} else {
- color_printf_ln(c, "# (use \"git rm --cached <file>...\" to unstage)");
+ color_fprintf_ln(s->fp, c, "# (use \"git rm --cached <file>...\" to unstage)");
}
- color_printf_ln(c, "#");
+ color_fprintf_ln(s->fp, c, "#");
}
-static void wt_status_print_header(const char *main, const char *sub)
+static void wt_status_print_header(struct wt_status *s,
+ const char *main, const char *sub)
{
const char *c = color(WT_STATUS_HEADER);
- color_printf_ln(c, "# %s:", main);
- color_printf_ln(c, "# (%s)", sub);
- color_printf_ln(c, "#");
+ color_fprintf_ln(s->fp, c, "# %s:", main);
+ color_fprintf_ln(s->fp, c, "# (%s)", sub);
+ color_fprintf_ln(s->fp, c, "#");
}
-static void wt_status_print_trailer(void)
+static void wt_status_print_trailer(struct wt_status *s)
{
- color_printf_ln(color(WT_STATUS_HEADER), "#");
+ color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "#");
}
static const char *quote_crlf(const char *in, char *buf, size_t sz)
@@ -108,7 +110,8 @@ static const char *quote_crlf(const char *in, char *buf, size_t sz)
return ret;
}
-static void wt_status_print_filepair(int t, struct diff_filepair *p)
+static void wt_status_print_filepair(struct wt_status *s,
+ int t, struct diff_filepair *p)
{
const char *c = color(t);
const char *one, *two;
@@ -117,36 +120,36 @@ static void wt_status_print_filepair(int t, struct diff_filepair *p)
one = quote_crlf(p->one->path, onebuf, sizeof(onebuf));
two = quote_crlf(p->two->path, twobuf, sizeof(twobuf));
- color_printf(color(WT_STATUS_HEADER), "#\t");
+ color_fprintf(s->fp, color(WT_STATUS_HEADER), "#\t");
switch (p->status) {
case DIFF_STATUS_ADDED:
- color_printf(c, "new file: %s", one);
+ color_fprintf(s->fp, c, "new file: %s", one);
break;
case DIFF_STATUS_COPIED:
- color_printf(c, "copied: %s -> %s", one, two);
+ color_fprintf(s->fp, c, "copied: %s -> %s", one, two);
break;
case DIFF_STATUS_DELETED:
- color_printf(c, "deleted: %s", one);
+ color_fprintf(s->fp, c, "deleted: %s", one);
break;
case DIFF_STATUS_MODIFIED:
- color_printf(c, "modified: %s", one);
+ color_fprintf(s->fp, c, "modified: %s", one);
break;
case DIFF_STATUS_RENAMED:
- color_printf(c, "renamed: %s -> %s", one, two);
+ color_fprintf(s->fp, c, "renamed: %s -> %s", one, two);
break;
case DIFF_STATUS_TYPE_CHANGED:
- color_printf(c, "typechange: %s", one);
+ color_fprintf(s->fp, c, "typechange: %s", one);
break;
case DIFF_STATUS_UNKNOWN:
- color_printf(c, "unknown: %s", one);
+ color_fprintf(s->fp, c, "unknown: %s", one);
break;
case DIFF_STATUS_UNMERGED:
- color_printf(c, "unmerged: %s", one);
+ color_fprintf(s->fp, c, "unmerged: %s", one);
break;
default:
die("bug: unhandled diff status %c", p->status);
}
- printf("\n");
+ fprintf(s->fp, "\n");
}
static void wt_status_print_updated_cb(struct diff_queue_struct *q,
@@ -160,14 +163,14 @@ static void wt_status_print_updated_cb(struct diff_queue_struct *q,
if (q->queue[i]->status == 'U')
continue;
if (!shown_header) {
- wt_status_print_cached_header(s->reference);
+ wt_status_print_cached_header(s);
s->commitable = 1;
shown_header = 1;
}
- wt_status_print_filepair(WT_STATUS_UPDATED, q->queue[i]);
+ wt_status_print_filepair(s, WT_STATUS_UPDATED, q->queue[i]);
}
if (shown_header)
- wt_status_print_trailer();
+ wt_status_print_trailer(s);
}
static void wt_status_print_changed_cb(struct diff_queue_struct *q,
@@ -184,12 +187,12 @@ static void wt_status_print_changed_cb(struct diff_queue_struct *q,
msg = use_add_rm_msg;
break;
}
- wt_status_print_header("Changed but not updated", msg);
+ wt_status_print_header(s, "Changed but not updated", msg);
}
for (i = 0; i < q->nr; i++)
- wt_status_print_filepair(WT_STATUS_CHANGED, q->queue[i]);
+ wt_status_print_filepair(s, WT_STATUS_CHANGED, q->queue[i]);
if (q->nr)
- wt_status_print_trailer();
+ wt_status_print_trailer(s);
}
static void wt_read_cache(struct wt_status *s)
@@ -206,16 +209,16 @@ static void wt_status_print_initial(struct wt_status *s)
wt_read_cache(s);
if (active_nr) {
s->commitable = 1;
- wt_status_print_cached_header(NULL);
+ wt_status_print_cached_header(s);
}
for (i = 0; i < active_nr; i++) {
- color_printf(color(WT_STATUS_HEADER), "#\t");
- color_printf_ln(color(WT_STATUS_UPDATED), "new file: %s",
+ color_fprintf(s->fp, color(WT_STATUS_HEADER), "#\t");
+ color_fprintf_ln(s->fp, color(WT_STATUS_UPDATED), "new file: %s",
quote_crlf(active_cache[i]->name,
buf, sizeof(buf)));
}
if (active_nr)
- wt_status_print_trailer();
+ wt_status_print_trailer(s);
}
static void wt_status_print_updated(struct wt_status *s)
@@ -282,12 +285,12 @@ static void wt_status_print_untracked(struct wt_status *s)
}
if (!shown_header) {
s->workdir_untracked = 1;
- wt_status_print_header("Untracked files",
+ wt_status_print_header(s, "Untracked files",
use_add_to_include_msg);
shown_header = 1;
}
- color_printf(color(WT_STATUS_HEADER), "#\t");
- color_printf_ln(color(WT_STATUS_UNTRACKED), "%.*s",
+ color_fprintf(s->fp, color(WT_STATUS_HEADER), "#\t");
+ color_fprintf_ln(s->fp, color(WT_STATUS_UNTRACKED), "%.*s",
ent->len, ent->name);
}
}
@@ -317,14 +320,14 @@ void wt_status_print(struct wt_status *s)
branch_name = "";
on_what = "Not currently on any branch.";
}
- color_printf_ln(color(WT_STATUS_HEADER),
+ color_fprintf_ln(s->fp, color(WT_STATUS_HEADER),
"# %s%s", on_what, branch_name);
}
if (s->is_initial) {
- color_printf_ln(color(WT_STATUS_HEADER), "#");
- color_printf_ln(color(WT_STATUS_HEADER), "# Initial commit");
- color_printf_ln(color(WT_STATUS_HEADER), "#");
+ color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "#");
+ color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "# Initial commit");
+ color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "#");
wt_status_print_initial(s);
}
else {
@@ -338,7 +341,7 @@ void wt_status_print(struct wt_status *s)
wt_status_print_verbose(s);
if (!s->commitable) {
if (s->amend)
- printf("# No changes\n");
+ fprintf(s->fp, "# No changes\n");
else if (s->workdir_dirty)
printf("no changes added to commit (use \"git add\" and/or \"git commit -a\")\n");
else if (s->workdir_untracked)
diff --git a/wt-status.h b/wt-status.h
index cfea4ae..4f3a615 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -1,6 +1,8 @@
#ifndef STATUS_H
#define STATUS_H
+#include <stdio.h>
+
enum color_wt_status {
WT_STATUS_HEADER,
WT_STATUS_UPDATED,
@@ -19,6 +21,7 @@ struct wt_status {
int commitable;
int workdir_dirty;
int workdir_untracked;
+ FILE *fp;
};
int git_status_config(const char *var, const char *value);
--
1.5.3.1.993.gbf388-dirty
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/7] Enable wt-status to run against non-standard index file.
2007-09-18 0:06 [PATCH 1/7] Enable wt-status output to a given FILE pointer Kristian Høgsberg
@ 2007-09-18 0:06 ` Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 3/7] Introduce entry point for launching add--interactive Kristian Høgsberg
0 siblings, 1 reply; 18+ messages in thread
From: Kristian Høgsberg @ 2007-09-18 0:06 UTC (permalink / raw)
To: git; +Cc: Kristian Høgsberg
We still default to get_index_file(), but this can be overridden
by setting wt_status.index_file after calling wt_status_prepare().
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
wt-status.c | 3 ++-
wt-status.h | 1 +
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index eeb1691..03b5ec4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -53,6 +53,7 @@ void wt_status_prepare(struct wt_status *s)
s->branch = head ? xstrdup(head) : NULL;
s->reference = "HEAD";
s->fp = stdout;
+ s->index_file = get_index_file();
}
static void wt_status_print_cached_header(struct wt_status *s)
@@ -198,7 +199,7 @@ static void wt_status_print_changed_cb(struct diff_queue_struct *q,
static void wt_read_cache(struct wt_status *s)
{
discard_cache();
- read_cache();
+ read_cache_from(s->index_file);
}
static void wt_status_print_initial(struct wt_status *s)
diff --git a/wt-status.h b/wt-status.h
index 4f3a615..7744932 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -21,6 +21,7 @@ struct wt_status {
int commitable;
int workdir_dirty;
int workdir_untracked;
+ const char *index_file;
FILE *fp;
};
--
1.5.3.1.993.gbf388-dirty
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/7] Introduce entry point for launching add--interactive.
2007-09-18 0:06 ` [PATCH 2/7] Enable wt-status to run against non-standard index file Kristian Høgsberg
@ 2007-09-18 0:06 ` Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 4/7] Clean up stripspace a bit, use strbuf even more Kristian Høgsberg
0 siblings, 1 reply; 18+ messages in thread
From: Kristian Høgsberg @ 2007-09-18 0:06 UTC (permalink / raw)
To: git; +Cc: Kristian Høgsberg
This refactors builtin-add.c a little to provide a unique entry point
for launching git add --interactive, which will be used by
builtin-commit too. If we later want to make add --interactive a
builtin or change how it is launched, we just start from this function.
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
builtin-add.c | 15 ++++++++++-----
commit.h | 3 +++
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/builtin-add.c b/builtin-add.c
index 0d7d0ce..a5cb910 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -12,6 +12,7 @@
#include "diffcore.h"
#include "commit.h"
#include "revision.h"
+#include "run-command.h"
static const char builtin_add_usage[] =
"git-add [-n] [-v] [-f] [--interactive | -i] [-u] [--refresh] [--] <filepattern>...";
@@ -153,6 +154,13 @@ static int git_add_config(const char *var, const char *value)
return git_default_config(var, value);
}
+int interactive_add(void)
+{
+ const char *argv[2] = { "add--interactive", NULL };
+
+ return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
static struct lock_file lock_file;
static const char ignore_error[] =
@@ -172,12 +180,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
add_interactive++;
}
if (add_interactive) {
- const char *args[] = { "add--interactive", NULL };
-
- if (add_interactive != 1 || argc != 2)
+ if (argc != 2)
die("add --interactive does not take any parameters");
- execv_git_cmd(args);
- exit(1);
+ exit(interactive_add());
}
git_config(git_add_config);
diff --git a/commit.h b/commit.h
index b779de8..da34c23 100644
--- a/commit.h
+++ b/commit.h
@@ -128,4 +128,7 @@ extern struct commit_list *get_shallow_commits(struct object_array *heads,
int depth, int shallow_flag, int not_shallow_flag);
int in_merge_bases(struct commit *, struct commit **, int);
+
+extern int interactive_add(void);
+
#endif /* COMMIT_H */
--
1.5.3.1.993.gbf388-dirty
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/7] Clean up stripspace a bit, use strbuf even more.
2007-09-18 0:06 ` [PATCH 3/7] Introduce entry point for launching add--interactive Kristian Høgsberg
@ 2007-09-18 0:06 ` Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 5/7] Add strbuf_read_file() Kristian Høgsberg
2007-09-18 13:12 ` [PATCH 4/7] Clean up stripspace a bit, use strbuf even more Johannes Schindelin
0 siblings, 2 replies; 18+ messages in thread
From: Kristian Høgsberg @ 2007-09-18 0:06 UTC (permalink / raw)
To: git; +Cc: Kristian Høgsberg
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
builtin-stripspace.c | 56 ++++++++++++++++++++++---------------------------
builtin-tag.c | 5 +---
builtin.h | 1 -
strbuf.h | 2 +
4 files changed, 28 insertions(+), 36 deletions(-)
diff --git a/builtin-stripspace.c b/builtin-stripspace.c
index c4cf2f0..a5e7518 100644
--- a/builtin-stripspace.c
+++ b/builtin-stripspace.c
@@ -9,17 +9,13 @@
*/
static size_t cleanup(char *line, size_t len)
{
- if (len) {
- if (line[len - 1] == '\n')
- len--;
-
- while (len) {
- unsigned char c = line[len - 1];
- if (!isspace(c))
- break;
- len--;
- }
+ while (len) {
+ unsigned char c = line[len - 1];
+ if (!isspace(c))
+ break;
+ len--;
}
+
return len;
}
@@ -35,42 +31,42 @@ static size_t cleanup(char *line, size_t len)
* If the input has only empty lines and spaces,
* no output will be produced.
*
- * If last line has a newline at the end, it will be removed.
+ * If last line does not have a newline at the end, one is added.
*
* Enable skip_comments to skip every line starting with "#".
*/
-size_t stripspace(char *buffer, size_t length, int skip_comments)
+void stripspace(struct strbuf *sb, int skip_comments)
{
- int empties = -1;
+ int empties = 0;
size_t i, j, len, newlen;
char *eol;
- for (i = j = 0; i < length; i += len, j += newlen) {
- eol = memchr(buffer + i, '\n', length - i);
- len = eol ? eol - (buffer + i) + 1 : length - i;
+ /* We may have to add a newline. */
+ strbuf_grow(sb, 1);
- if (skip_comments && len && buffer[i] == '#') {
+ for (i = j = 0; i < sb->len; i += len, j += newlen) {
+ eol = memchr(sb->buf + i, '\n', sb->len - i);
+ len = eol ? eol - (sb->buf + i) + 1 : sb->len - i;
+
+ if (skip_comments && len && sb->buf[i] == '#') {
newlen = 0;
continue;
}
- newlen = cleanup(buffer + i, len);
+ newlen = cleanup(sb->buf + i, len);
/* Not just an empty line? */
if (newlen) {
- if (empties != -1)
- buffer[j++] = '\n';
- if (empties > 0)
- buffer[j++] = '\n';
+ if (empties > 0 && j > 0)
+ sb->buf[j++] = '\n';
empties = 0;
- memmove(buffer + j, buffer + i, newlen);
- continue;
+ memmove(sb->buf + j, sb->buf + i, newlen);
+ sb->buf[newlen + j++] = '\n';
+ } else {
+ empties++;
}
- if (empties < 0)
- continue;
- empties++;
}
- return j;
+ strbuf_setlen(sb, j);
}
int cmd_stripspace(int argc, const char **argv, const char *prefix)
@@ -86,9 +82,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
if (strbuf_read(&buf, 0, 1024) < 0)
die("could not read the input");
- strbuf_setlen(&buf, stripspace(buf.buf, buf.len, strip_comments));
- if (buf.len)
- strbuf_addch(&buf, '\n');
+ stripspace(&buf, strip_comments);
write_or_die(1, buf.buf, buf.len);
strbuf_release(&buf);
diff --git a/builtin-tag.c b/builtin-tag.c
index 9f29342..68c9a20 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -297,14 +297,11 @@ static void create_tag(const unsigned char *object, const char *tag,
free(path);
}
- strbuf_setlen(buf, stripspace(buf->buf, buf->len, 1));
+ stripspace(buf, 1);
if (!message && !buf->len)
die("no tag message?");
- /* insert the header and add the '\n' if needed: */
- if (buf->len)
- strbuf_addch(buf, '\n');
strbuf_insert(buf, 0, header_buf, header_len);
if (sign && do_sign(buf) < 0)
diff --git a/builtin.h b/builtin.h
index 03ee7bf..d6f2c76 100644
--- a/builtin.h
+++ b/builtin.h
@@ -7,7 +7,6 @@ extern const char git_version_string[];
extern const char git_usage_string[];
extern void help_unknown_cmd(const char *cmd);
-extern size_t stripspace(char *buffer, size_t length, int skip_comments);
extern int write_tree(unsigned char *sha1, int missing_ok, const char *prefix);
extern void prune_packed_objects(int);
diff --git a/strbuf.h b/strbuf.h
index 21fc111..5960637 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -98,4 +98,6 @@ extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
extern void read_line(struct strbuf *, FILE *, int);
+extern void stripspace(struct strbuf *buf, int skip_comments);
+
#endif /* STRBUF_H */
--
1.5.3.1.993.gbf388-dirty
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 5/7] Add strbuf_read_file().
2007-09-18 0:06 ` [PATCH 4/7] Clean up stripspace a bit, use strbuf even more Kristian Høgsberg
@ 2007-09-18 0:06 ` Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 6/7] Export rerere() and launch_editor() Kristian Høgsberg
2007-09-18 13:12 ` [PATCH 4/7] Clean up stripspace a bit, use strbuf even more Johannes Schindelin
1 sibling, 1 reply; 18+ messages in thread
From: Kristian Høgsberg @ 2007-09-18 0:06 UTC (permalink / raw)
To: git; +Cc: Kristian Høgsberg
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
builtin-tag.c | 10 +++-------
strbuf.c | 15 +++++++++++++++
strbuf.h | 1 +
3 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/builtin-tag.c b/builtin-tag.c
index 68c9a20..c97673f 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -53,13 +53,9 @@ static void launch_editor(const char *path, struct strbuf *buffer)
if (run_command(&child))
die("There was a problem with the editor %s.", editor);
- fd = open(path, O_RDONLY);
- if (fd < 0)
- die("could not open '%s': %s", path, strerror(errno));
- if (strbuf_read(buffer, fd, 0) < 0) {
- die("could not read message file '%s': %s", path, strerror(errno));
- }
- close(fd);
+ if (strbuf_read_file(buffer, path) < 0)
+ die("could not read message file '%s': %s",
+ path, strerror(errno));
}
struct tag_filter {
diff --git a/strbuf.c b/strbuf.c
index d919047..438606d 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -110,6 +110,21 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
return sb->len - oldlen;
}
+int strbuf_read_file(struct strbuf *sb, const char *path)
+{
+ int fd, len;
+
+ fd = open(path, O_RDONLY);
+ if (fd < 0)
+ return -1;
+ len = strbuf_read(sb, fd, 0);
+ close(fd);
+ if (len < 0)
+ return -1;
+
+ return len;
+}
+
void read_line(struct strbuf *sb, FILE *fp, int term) {
int ch;
if (feof(fp)) {
diff --git a/strbuf.h b/strbuf.h
index 5960637..3a523ab 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -95,6 +95,7 @@ extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
/* XXX: if read fails, any partial read is undone */
extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
+extern int strbuf_read_file(struct strbuf *sb, const char *path);
extern void read_line(struct strbuf *, FILE *, int);
--
1.5.3.1.993.gbf388-dirty
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 6/7] Export rerere() and launch_editor().
2007-09-18 0:06 ` [PATCH 5/7] Add strbuf_read_file() Kristian Høgsberg
@ 2007-09-18 0:06 ` Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 7/7] Implement git commit as a builtin command Kristian Høgsberg
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Kristian Høgsberg @ 2007-09-18 0:06 UTC (permalink / raw)
To: git; +Cc: Kristian Høgsberg
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
builtin-rerere.c | 16 ++++++++++++++++
commit.h | 1 +
2 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/builtin-rerere.c b/builtin-rerere.c
index 826d346..54f3575 100644
--- a/builtin-rerere.c
+++ b/builtin-rerere.c
@@ -395,6 +395,22 @@ static int is_rerere_enabled(void)
return 1;
}
+/* Export for builtin-commit. */
+int rerere(void)
+{
+ struct path_list merge_rr = { NULL, 0, 0, 1 };
+ int fd;
+
+ git_config(git_rerere_config);
+ if (!is_rerere_enabled())
+ return 0;
+
+ merge_rr_path = xstrdup(git_path("rr-cache/MERGE_RR"));
+ fd = hold_lock_file_for_update(&write_lock, merge_rr_path, 1);
+ read_rr(&merge_rr);
+ return do_plain_rerere(&merge_rr, fd);
+}
+
int cmd_rerere(int argc, const char **argv, const char *prefix)
{
struct path_list merge_rr = { NULL, 0, 0, 1 };
diff --git a/commit.h b/commit.h
index da34c23..cc8d890 100644
--- a/commit.h
+++ b/commit.h
@@ -130,5 +130,6 @@ extern struct commit_list *get_shallow_commits(struct object_array *heads,
int in_merge_bases(struct commit *, struct commit **, int);
extern int interactive_add(void);
+extern int rerere(void);
#endif /* COMMIT_H */
--
1.5.3.1.993.gbf388-dirty
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 7/7] Implement git commit as a builtin command.
2007-09-18 0:06 ` [PATCH 6/7] Export rerere() and launch_editor() Kristian Høgsberg
@ 2007-09-18 0:06 ` Kristian Høgsberg
2007-09-18 13:58 ` Johannes Schindelin
2007-09-20 1:27 ` Junio C Hamano
2007-09-18 13:14 ` [PATCH 6/7] Export rerere() and launch_editor() Johannes Schindelin
2007-09-19 23:52 ` Junio C Hamano
2 siblings, 2 replies; 18+ messages in thread
From: Kristian Høgsberg @ 2007-09-18 0:06 UTC (permalink / raw)
To: git; +Cc: Kristian Høgsberg
Move git-commit.sh to contrib/examples.
Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
Makefile | 9 +-
builtin-commit.c | 740 +++++++++++++++++++++++
builtin-tag.c | 2 +-
builtin.h | 3 +-
git-commit.sh => contrib/examples/git-commit.sh | 0
git.c | 3 +-
strbuf.h | 1 +
t/t3501-revert-cherry-pick.sh | 4 +-
t/t3901-i18n-patch.sh | 8 +-
t/test-lib.sh | 4 +-
10 files changed, 757 insertions(+), 17 deletions(-)
create mode 100644 builtin-commit.c
rename git-commit.sh => contrib/examples/git-commit.sh (100%)
diff --git a/Makefile b/Makefile
index 0055eef..df82d92 100644
--- a/Makefile
+++ b/Makefile
@@ -206,7 +206,7 @@ BASIC_LDFLAGS =
SCRIPT_SH = \
git-bisect.sh git-checkout.sh \
- git-clean.sh git-clone.sh git-commit.sh \
+ git-clean.sh git-clone.sh \
git-fetch.sh \
git-ls-remote.sh \
git-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \
@@ -255,7 +255,7 @@ EXTRA_PROGRAMS =
BUILT_INS = \
git-format-patch$X git-show$X git-whatchanged$X git-cherry$X \
git-get-tar-commit-id$X git-init$X git-repo-config$X \
- git-fsck-objects$X git-cherry-pick$X \
+ git-fsck-objects$X git-cherry-pick$X git-status$X\
$(patsubst builtin-%.o,git-%$X,$(BUILTIN_OBJS))
# what 'all' will build and 'install' will install, in gitexecdir
@@ -327,6 +327,7 @@ BUILTIN_OBJS = \
builtin-check-attr.o \
builtin-checkout-index.o \
builtin-check-ref-format.o \
+ builtin-commit.o \
builtin-commit-tree.o \
builtin-count-objects.o \
builtin-describe.o \
@@ -363,7 +364,6 @@ BUILTIN_OBJS = \
builtin-rev-parse.o \
builtin-revert.o \
builtin-rm.o \
- builtin-runstatus.o \
builtin-shortlog.o \
builtin-show-branch.o \
builtin-stripspace.o \
@@ -819,9 +819,6 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
chmod +x $@+ && \
mv $@+ $@
-git-status: git-commit
- $(QUIET_GEN)cp $< $@+ && mv $@+ $@
-
gitweb/gitweb.cgi: gitweb/gitweb.perl
$(QUIET_GEN)$(RM) $@ $@+ && \
sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
diff --git a/builtin-commit.c b/builtin-commit.c
new file mode 100644
index 0000000..ee98de9
--- /dev/null
+++ b/builtin-commit.c
@@ -0,0 +1,740 @@
+/*
+ * Builtin "git commit"
+ *
+ * Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
+ * Based on git-commit.sh by Junio C Hamano and Linus Torvalds
+ */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "cache.h"
+#include "cache-tree.h"
+#include "builtin.h"
+#include "diff.h"
+#include "diffcore.h"
+#include "commit.h"
+#include "revision.h"
+#include "wt-status.h"
+#include "run-command.h"
+#include "refs.h"
+#include "log-tree.h"
+#include "strbuf.h"
+#include "utf8.h"
+
+static const char builtin_commit_usage[] =
+ "[-a | --interactive] [-s] [-v] [--no-verify] [-m <message> | -F <logfile> | (-C|-c) <commit> | --amend] [-u] [-e] [--author <author>] [--template <file>] [[-i | -o] <path>...]";
+
+static unsigned char head_sha1[20], merge_head_sha1[20];
+static char *use_message_buffer;
+static const char commit_editmsg[] = "COMMIT_EDITMSG";
+static struct lock_file lock_file;
+
+enum option_type {
+ OPTION_NONE,
+ OPTION_STRING,
+ OPTION_INTEGER,
+ OPTION_LAST,
+};
+
+struct option {
+ enum option_type type;
+ const char *long_name;
+ char short_name;
+ void *value;
+};
+
+static int scan_options(const char ***argv, struct option *options)
+{
+ const char *value, *eq;
+ int i;
+
+ if (**argv == NULL)
+ return 0;
+ if ((**argv)[0] != '-')
+ return 0;
+ if (!strcmp(**argv, "--"))
+ return 0;
+
+ value = NULL;
+ for (i = 0; options[i].type != OPTION_LAST; i++) {
+ if ((**argv)[1] == '-') {
+ if (!prefixcmp(options[i].long_name, **argv + 2)) {
+ if (options[i].type != OPTION_NONE)
+ value = *++(*argv);
+ goto match;
+ }
+
+ eq = strchr(**argv + 2, '=');
+ if (eq && options[i].type != OPTION_NONE &&
+ !strncmp(**argv + 2,
+ options[i].long_name, eq - **argv - 2)) {
+ value = eq + 1;
+ goto match;
+ }
+ }
+
+ if ((**argv)[1] == options[i].short_name) {
+ if ((**argv)[2] == '\0') {
+ if (options[i].type != OPTION_NONE)
+ value = *++(*argv);
+ goto match;
+ }
+
+ if (options[i].type != OPTION_NONE) {
+ value = **argv + 2;
+ goto match;
+ }
+ }
+ }
+
+ usage(builtin_commit_usage);
+
+ match:
+ switch (options[i].type) {
+ case OPTION_NONE:
+ *(int *)options[i].value = 1;
+ break;
+ case OPTION_STRING:
+ if (value == NULL)
+ die("option %s requires a value.", (*argv)[-1]);
+ *(const char **)options[i].value = value;
+ break;
+ case OPTION_INTEGER:
+ if (value == NULL)
+ die("option %s requires a value.", (*argv)[-1]);
+ *(int *)options[i].value = atoi(value);
+ break;
+ default:
+ assert(0);
+ }
+
+ (*argv)++;
+
+ return 1;
+}
+
+static char *logfile, *force_author, *message, *template_file;
+static char *edit_message, *use_message;
+static int all, edit_flag, also, interactive, only, no_verify, amend, signoff;
+static int quiet, verbose, untracked_files;
+
+static int no_edit, initial_commit, in_merge;
+const char *only_include_assumed;
+
+static struct option commit_options[] = {
+ { OPTION_STRING, "file", 'F', (void *) &logfile },
+ { OPTION_NONE, "all", 'a', &all },
+ { OPTION_STRING, "author", 0, (void *) &force_author },
+ { OPTION_NONE, "edit", 'e', &edit_flag },
+ { OPTION_NONE, "include", 'i', &also },
+ { OPTION_NONE, "interactive", 0, &interactive },
+ { OPTION_NONE, "only", 'o', &only },
+ { OPTION_STRING, "message", 'm', &message },
+ { OPTION_NONE, "no-verify", 'n', &no_verify },
+ { OPTION_NONE, "amend", 0, &amend },
+ { OPTION_STRING, "reedit-message", 'c', &edit_message },
+ { OPTION_STRING, "reuse-message", 'C', &use_message },
+ { OPTION_NONE, "signoff", 's', &signoff },
+ { OPTION_NONE, "quiet", 'q', &quiet },
+ { OPTION_NONE, "verbose", 'v', &verbose },
+ { OPTION_NONE, "untracked-files", 0, &untracked_files },
+ { OPTION_STRING, "template", 't', &template_file },
+ { OPTION_LAST },
+};
+
+/* FIXME: Taken from builtin-add, should be shared. */
+
+static void update_callback(struct diff_queue_struct *q,
+ struct diff_options *opt, void *cbdata)
+{
+ int i, verbose;
+
+ verbose = *((int *)cbdata);
+ for (i = 0; i < q->nr; i++) {
+ struct diff_filepair *p = q->queue[i];
+ const char *path = p->one->path;
+ switch (p->status) {
+ default:
+ die("unexpacted diff status %c", p->status);
+ case DIFF_STATUS_UNMERGED:
+ case DIFF_STATUS_MODIFIED:
+ case DIFF_STATUS_TYPE_CHANGED:
+ add_file_to_cache(path, verbose);
+ break;
+ case DIFF_STATUS_DELETED:
+ remove_file_from_cache(path);
+ cache_tree_invalidate_path(active_cache_tree, path);
+ if (verbose)
+ printf("remove '%s'\n", path);
+ break;
+ }
+ }
+}
+
+static void
+add_files_to_cache(int fd, const char **files, const char *prefix)
+{
+ struct rev_info rev;
+
+ init_revisions(&rev, "");
+ setup_revisions(0, NULL, &rev, NULL);
+ rev.prune_data = get_pathspec(prefix, files);
+ rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+ rev.diffopt.format_callback = update_callback;
+ rev.diffopt.format_callback_data = &verbose;
+
+ run_diff_files(&rev, 0);
+ refresh_cache(REFRESH_QUIET);
+
+ if (write_cache(fd, active_cache, active_nr) || close(fd))
+ die("unable to write new index file");
+}
+
+static char *
+prepare_index(const char **files, const char *prefix)
+{
+ int fd;
+ struct tree *tree;
+ struct lock_file *next_index_lock;
+
+ fd = hold_locked_index(&lock_file, 1);
+ if (read_cache() < 0)
+ die("index file corrupt");
+
+ if (all) {
+ add_files_to_cache(fd, files, NULL);
+ return lock_file.filename;
+ } else if (also) {
+ add_files_to_cache(fd, files, prefix);
+ return lock_file.filename;
+ }
+
+ if (interactive)
+ interactive_add();
+
+ if (*files == NULL) {
+ /* Commit index as-is. */
+ rollback_lock_file(&lock_file);
+ return get_index_file();
+ }
+
+ /*
+ * FIXME: Warn on unknown files. Shell script does
+ *
+ * commit_only=`git-ls-files --error-unmatch -- "$@"`
+ */
+
+ /*
+ * FIXME: shell script does
+ *
+ * git-read-tree --index-output="$TMP_INDEX" -i -m HEAD
+ *
+ * which warns about unmerged files in the index.
+ */
+
+ /* update the user index file */
+ add_files_to_cache(fd, files, prefix);
+
+ if (!initial_commit) {
+ tree = parse_tree_indirect(head_sha1);
+ if (!tree)
+ die("failed to unpack HEAD tree object");
+ if (read_tree(tree, 0, NULL))
+ die("failed to read HEAD tree object");
+ }
+
+ /* Uh oh, abusing lock_file to create a garbage collected file */
+ next_index_lock = xmalloc(sizeof(*next_index_lock));
+ fd = hold_lock_file_for_update(next_index_lock,
+ git_path("next-index-%d", getpid()), 1);
+ add_files_to_cache(fd, files, prefix);
+
+ return next_index_lock->filename;
+}
+
+static int run_status(FILE *fp, const char *index_file)
+{
+ struct wt_status s;
+
+ wt_status_prepare(&s);
+
+ if (amend) {
+ s.amend = 1;
+ s.reference = "HEAD^1";
+ }
+ s.verbose = verbose;
+ s.untracked = untracked_files;
+ s.index_file = index_file;
+ s.fp = fp;
+
+ wt_status_print(&s);
+
+ return s.commitable;
+}
+
+static const char sign_off_header[] = "Signed-off-by: ";
+
+static int prepare_log_message(const char *index_file)
+{
+ struct stat statbuf;
+ int commitable;
+ struct strbuf sb;
+ char *buffer;
+ FILE *fp;
+
+ strbuf_init(&sb, 0);
+ if (message) {
+ strbuf_add(&sb, message, strlen(message));
+ } else if (logfile && !strcmp(logfile, "-")) {
+ if (isatty(0))
+ fprintf(stderr, "(reading log message from standard input)\n");
+ if (strbuf_read(&sb, 0, 0) < 0)
+ die("could not read log from standard input");
+ } else if (logfile) {
+ if (strbuf_read_file(&sb, logfile) < 0)
+ die("could not read log file '%s': %s",
+ logfile, strerror(errno));
+ } else if (use_message) {
+ buffer = strstr(use_message_buffer, "\n\n");
+ if (!buffer || buffer[2] == '\0')
+ die("commit has empty message");
+ strbuf_add(&sb, buffer + 2, strlen(buffer + 2));
+ } else if (!stat(git_path("MERGE_MSG"), &statbuf)) {
+ if (strbuf_read_file(&sb, git_path("MERGE_MSG")) < 0)
+ die("could not read MERGE_MSG: %s", strerror(errno));
+ } else if (!stat(git_path("SQUASH_MSG"), &statbuf)) {
+ if (strbuf_read_file(&sb, git_path("SQUASH_MSG")) < 0)
+ die("could not read SQUASH_MSG: %s", strerror(errno));
+ } else if (!stat(template_file, &statbuf)) {
+ if (strbuf_read_file(&sb, template_file) < 0)
+ die("could not read %s: %s",
+ template_file, strerror(errno));
+ }
+
+ fp = fopen(git_path(commit_editmsg), "w");
+ if (fp == NULL)
+ die("could not open %s\n", git_path(commit_editmsg));
+
+ stripspace(&sb, 0);
+ if (fwrite(sb.buf, 1, sb.len, fp) < sb.len)
+ die("could not write commit template: %s\n",
+ strerror(errno));
+
+ if (signoff) {
+ const char *info, *bol;
+
+ info = git_committer_info(1);
+ strbuf_addch(&sb, '\0');
+ bol = strrchr(sb.buf + sb.len - 1, '\n');
+ if (!bol || prefixcmp(bol, sign_off_header))
+ fprintf(fp, "\n");
+ fprintf(fp, "%s%s\n", sign_off_header, git_committer_info(1));
+ }
+
+ strbuf_release(&sb);
+
+ if (in_merge && !no_edit) {
+ fprintf(fp,
+ "#\n"
+ "# It looks like you may be committing a MERGE.\n"
+ "# If this is not correct, please remove the file\n"
+ "# %s\n"
+ "# and try again.\n"
+ "#\n",
+ git_path("MERGE_HEAD"));
+ }
+
+ fprintf(fp,
+ "\n"
+ "# Please enter the commit message for your changes.\n"
+ "# (Comment lines starting with '#' will not be included)\n");
+ if (only_include_assumed)
+ fprintf(fp, "# %s\n", only_include_assumed);
+
+ commitable = run_status(fp, index_file);
+
+ fclose(fp);
+
+ return commitable;
+}
+
+/* Find out if the message starting at position 'start' in the strbuf
+ * contains only whitespace and Signed-off-by lines. */
+static int message_is_empty(struct strbuf *sb, int start)
+{
+ struct strbuf tmpl;
+ const char *nl;
+ int eol, i;
+
+ /* See if the template is just a prefix of the message. */
+ strbuf_init(&tmpl, 0);
+ if (template_file && strbuf_read_file(&tmpl, template_file) > 0) {
+ stripspace(&tmpl, 1);
+ if (start + tmpl.len <= sb->len &&
+ memcmp(tmpl.buf, sb->buf + start, tmpl.len) == 0)
+ start += tmpl.len;
+ }
+ strbuf_release(&tmpl);
+
+ /* Check if the rest is just whitespace and Signed-of-by's. */
+ for (i = start; i < sb->len; i++) {
+ nl = memchr(sb->buf + i, '\n', sb->len - i);
+ if (nl)
+ eol = nl - sb->buf;
+ else
+ eol = sb->len;
+
+ if (strlen(sign_off_header) <= eol - i &&
+ !prefixcmp(sb->buf + i, sign_off_header)) {
+ i = eol;
+ continue;
+ }
+ while (i < eol)
+ if (!isspace(sb->buf[i++]))
+ return 0;
+ }
+
+ return 1;
+}
+
+static void determine_author_info(struct strbuf *sb)
+{
+ char *p, *eol;
+ char *name = NULL, *email = NULL;
+
+ if (use_message) {
+ p = strstr(use_message_buffer, "\nauthor");
+ if (!p)
+ die("invalid commit: %s\n", use_message);
+ p++;
+ eol = strchr(p, '\n');
+ if (!eol)
+ die("invalid commit: %s\n", use_message);
+
+ strbuf_add(sb, p, eol + 1 - p);
+ } else if (force_author) {
+ const char *eoname = strstr(force_author, " <");
+ const char *eomail = strchr(force_author, '>');
+
+ if (!eoname || !eomail)
+ die("malformed --author parameter\n");
+ name = xstrndup(force_author, eoname - force_author);
+ email = xstrndup(eoname + 2, eomail - eoname - 2);
+ strbuf_addf(sb, "author %s\n",
+ fmt_ident(name, email,
+ getenv("GIT_AUTHOR_DATE"), 1));
+ free(name);
+ free(email);
+ } else {
+ strbuf_addf(sb, "author %s\n", git_author_info(1));
+ }
+}
+
+static void parse_and_validate_options(const char ***argv)
+{
+ int f = 0;
+
+ (*argv)++;
+ while (scan_options(argv, commit_options))
+ ;
+
+ if (logfile || message || use_message)
+ no_edit = 1;
+ if (edit_flag)
+ no_edit = 0;
+
+ if (get_sha1("HEAD", head_sha1))
+ initial_commit = 1;
+
+ if (!get_sha1("MERGE_HEAD", merge_head_sha1))
+ in_merge = 1;
+
+ /* Sanity check options */
+ if (amend && initial_commit)
+ die("You have nothing to amend.");
+ if (amend && in_merge)
+ die("You are in the middle of a merger -- cannot amend.");
+
+ if (use_message)
+ f++;
+ if (edit_message)
+ f++;
+ if (logfile)
+ f++;
+ if (f > 1)
+ die("Only one of -c/-C/-F can be used.");
+ if (message && f > 0)
+ die("Option -m cannot be combined with -c/-C/-F.");
+ if (edit_message)
+ use_message = edit_message;
+ if (amend)
+ use_message = "HEAD";
+ if (use_message) {
+ unsigned char sha1[20];
+ static char utf8[] = "UTF-8";
+ const char *out_enc;
+ char *enc, *end;
+ struct commit *commit;
+
+ if (get_sha1(use_message, sha1))
+ die("could not lookup commit %s", use_message);
+ commit = lookup_commit(sha1);
+ if (!commit || parse_commit(commit))
+ die("could not parse commit %s", use_message);
+
+ enc = strstr(commit->buffer, "\nencoding");
+ if (enc) {
+ end = strchr(enc + 10, '\n');
+ enc = xstrndup(enc + 10, end - (enc + 10));
+ } else {
+ enc = utf8;
+ }
+ out_enc = git_commit_encoding ? git_commit_encoding : utf8;
+
+ use_message_buffer =
+ reencode_string(commit->buffer, out_enc, enc);
+ if (enc != utf8)
+ free(enc);
+ }
+
+ if (also && only)
+ die("Only one of --include/--only can be used.");
+ if (!*argv && (also || (only && !amend)))
+ die("No paths with --include/--only does not make sense.");
+ if (!*argv && only && amend)
+ only_include_assumed = "Clever... amending the last one with dirty index.";
+ if (*argv && !also && !only) {
+ only_include_assumed = "Explicit paths specified without -i nor -o; assuming --only paths...";
+ also = 0;
+ }
+
+ if (all && interactive)
+ die("Cannot use -a, --interactive or -i at the same time.");
+ else if (all && **argv)
+ die("Paths with -a does not make sense.");
+ else if (interactive && **argv)
+ die("Paths with --interactive does not make sense.");
+}
+
+int cmd_status(int argc, const char **argv, const char *prefix)
+{
+ const char *index_file;
+ int commitable;
+
+ git_config(git_status_config);
+
+ parse_and_validate_options(&argv);
+
+ index_file = prepare_index(argv, prefix);
+
+ commitable = run_status(stdout, index_file);
+
+ rollback_lock_file(&lock_file);
+
+ return commitable ? 0 : 1;
+}
+
+static int run_hook(const char *index_file, const char *name, const char *arg)
+{
+ struct child_process hook;
+ const char *argv[3], *env[2];
+ char index[PATH_MAX];
+
+ argv[0] = git_path("hooks/%s", name);
+ argv[1] = arg;
+ argv[2] = NULL;
+ snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+ env[0] = index;
+ env[1] = NULL;
+
+ if (access(argv[0], X_OK) < 0)
+ return 0;
+
+ memset(&hook, 0, sizeof(hook));
+ hook.argv = argv;
+ hook.no_stdin = 1;
+ hook.stdout_to_stderr = 1;
+ hook.env = env;
+
+ return run_command(&hook);
+}
+
+static void print_summary(const char *prefix, const unsigned char *sha1)
+{
+ struct rev_info rev;
+ struct commit *commit;
+
+ commit = lookup_commit(sha1);
+ if (!commit)
+ die("couldn't look up newly created commit\n");
+ if (!commit || parse_commit(commit))
+ die("could not parse newly created commit");
+
+ init_revisions(&rev, prefix);
+ setup_revisions(0, NULL, &rev, NULL);
+
+ rev.abbrev = 0;
+ rev.diff = 1;
+ rev.diffopt.output_format =
+ DIFF_FORMAT_SHORTSTAT | DIFF_FORMAT_SUMMARY;
+
+ rev.verbose_header = 1;
+ rev.show_root_diff = 1;
+ rev.commit_format = get_commit_format("format:%h: %s");
+ rev.always_show_header = 1;
+
+ printf("Created %scommit ", initial_commit ? "initial " : "");
+
+ log_tree_commit(&rev, commit);
+}
+
+int git_commit_config(const char *k, const char *v)
+{
+ if (!strcmp(k, "commit.template")) {
+ template_file = xstrdup(v);
+ return 0;
+ }
+
+ return git_status_config(k, v);
+}
+
+static const char commit_utf8_warn[] =
+"Warning: commit message does not conform to UTF-8.\n"
+"You may want to amend it after fixing the message, or set the config\n"
+"variable i18n.commitencoding to the encoding your project uses.\n";
+
+int cmd_commit(int argc, const char **argv, const char *prefix)
+{
+ int header_len, parent_count = 0;
+ struct strbuf sb;
+ const char *index_file, *reflog_msg;
+ char *nl, *header_line;
+ unsigned char commit_sha1[20];
+ struct ref_lock *ref_lock;
+
+ git_config(git_commit_config);
+
+ parse_and_validate_options(&argv);
+
+ index_file = prepare_index(argv, prefix);
+
+ if (run_hook(index_file, "pre-commit", NULL))
+ exit(1);
+
+ if (!prepare_log_message(index_file) && !in_merge) {
+ run_status(stdout, index_file);
+ unlink(commit_editmsg);
+ return 1;
+ }
+
+ strbuf_init(&sb, 0);
+
+ /* Start building up the commit header */
+ read_cache_from(index_file);
+ active_cache_tree = cache_tree();
+ if (cache_tree_update(active_cache_tree,
+ active_cache, active_nr, 0, 0) < 0)
+ die("Error building trees");
+ strbuf_addf(&sb, "tree %s\n",
+ sha1_to_hex(active_cache_tree->sha1));
+
+ /* Determine parents */
+ if (initial_commit) {
+ reflog_msg = "commit (initial)";
+ parent_count = 0;
+ } else if (amend) {
+ struct commit_list *c;
+ struct commit *commit;
+
+ reflog_msg = "commit (amend)";
+ commit = lookup_commit(head_sha1);
+ if (!commit || parse_commit(commit))
+ die("could not parse HEAD commit");
+
+ for (c = commit->parents; c; c = c->next)
+ strbuf_addf(&sb, "parent %s\n",
+ sha1_to_hex(c->item->object.sha1));
+ } else if (in_merge) {
+ struct strbuf m;
+ FILE *fp;
+
+ reflog_msg = "commit (merge)";
+ strbuf_addf(&sb, "parent %s\n", sha1_to_hex(head_sha1));
+ strbuf_init(&m, 0);
+ fp = fopen(git_path("MERGE_HEAD"), "r");
+ if (fp == NULL)
+ die("could not open %s for reading: %s",
+ git_path("MERGE_HEAD"), strerror(errno));
+ while (!m.eof) {
+ read_line(&m, fp, '\n');
+ if (!m.eof)
+ strbuf_addf(&sb, "parent %s\n", m.buf);
+ }
+ fclose(fp);
+ strbuf_release(&m);
+ } else {
+ reflog_msg = "commit";
+ strbuf_addf(&sb, "parent %s\n", sha1_to_hex(head_sha1));
+ }
+
+ determine_author_info(&sb);
+ strbuf_addf(&sb, "committer %s\n", git_committer_info(1));
+ if (!is_encoding_utf8(git_commit_encoding))
+ strbuf_addf(&sb, "encoding %s\n", git_commit_encoding);
+ strbuf_addch(&sb, '\n');
+
+ /* Get the commit message and validate it */
+ header_len = sb.len;
+ if (!no_edit) {
+ fprintf(stderr, "launching editor, log %s\n", logfile);
+ launch_editor(git_path(commit_editmsg), &sb);
+ }
+ else if (strbuf_read_file(&sb, git_path(commit_editmsg)) < 0)
+ die("could not read commit message\n");
+ if (run_hook(index_file, "commit-msg", commit_editmsg))
+ exit(1);
+ stripspace(&sb, 1);
+ if (sb.len < header_len ||
+ message_is_empty(&sb, header_len))
+ die("* no commit message? aborting commit.");
+ strbuf_addch(&sb, '\0');
+ if (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))
+ fprintf(stderr, commit_utf8_warn);
+
+ if (write_sha1_file(sb.buf, sb.len - 1, commit_type, commit_sha1))
+ die("failed to write commit object");
+
+ ref_lock = lock_any_ref_for_update("HEAD",
+ initial_commit ? NULL : head_sha1,
+ 0);
+
+ nl = strchr(sb.buf + header_len, '\n');
+ header_line = xstrndup(sb.buf + header_len,
+ nl - (sb.buf + header_len));
+ strbuf_release(&sb);
+ strbuf_addf(&sb, "%s: %s\n", reflog_msg, header_line);
+ strbuf_addch(&sb, '\0');
+ free(header_line);
+
+ if (!ref_lock)
+ die("cannot lock HEAD ref");
+ if (write_ref_sha1(ref_lock, commit_sha1, sb.buf) < 0)
+ die("cannot update HEAD ref");
+
+ unlink(git_path("MERGE_HEAD"));
+ unlink(git_path("MERGE_MSG"));
+
+ if (lock_file.filename[0] && commit_locked_index(&lock_file))
+ die("failed to write new index");
+
+ rerere();
+
+ run_hook(index_file, "post-commit", NULL);
+
+ if (!quiet)
+ print_summary(prefix, commit_sha1);
+
+ return 0;
+}
diff --git a/builtin-tag.c b/builtin-tag.c
index c97673f..5c4dc4f 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -18,7 +18,7 @@ static const char builtin_tag_usage[] =
static char signingkey[1000];
-static void launch_editor(const char *path, struct strbuf *buffer)
+void launch_editor(const char *path, struct strbuf *buffer)
{
const char *editor, *terminal;
struct child_process child;
diff --git a/builtin.h b/builtin.h
index d6f2c76..0aea4db 100644
--- a/builtin.h
+++ b/builtin.h
@@ -23,6 +23,7 @@ extern int cmd_check_attr(int argc, const char **argv, const char *prefix);
extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
extern int cmd_cherry(int argc, const char **argv, const char *prefix);
extern int cmd_cherry_pick(int argc, const char **argv, const char *prefix);
+extern int cmd_commit(int argc, const char **argv, const char *prefix);
extern int cmd_commit_tree(int argc, const char **argv, const char *prefix);
extern int cmd_count_objects(int argc, const char **argv, const char *prefix);
extern int cmd_describe(int argc, const char **argv, const char *prefix);
@@ -64,10 +65,10 @@ extern int cmd_rev_list(int argc, const char **argv, const char *prefix);
extern int cmd_rev_parse(int argc, const char **argv, const char *prefix);
extern int cmd_revert(int argc, const char **argv, const char *prefix);
extern int cmd_rm(int argc, const char **argv, const char *prefix);
-extern int cmd_runstatus(int argc, const char **argv, const char *prefix);
extern int cmd_shortlog(int argc, const char **argv, const char *prefix);
extern int cmd_show(int argc, const char **argv, const char *prefix);
extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
+extern int cmd_status(int argc, const char **argv, const char *prefix);
extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
extern int cmd_tag(int argc, const char **argv, const char *prefix);
diff --git a/git-commit.sh b/contrib/examples/git-commit.sh
similarity index 100%
rename from git-commit.sh
rename to contrib/examples/git-commit.sh
diff --git a/git.c b/git.c
index 56ae8cc..f915b47 100644
--- a/git.c
+++ b/git.c
@@ -326,6 +326,7 @@ static void handle_internal_command(int argc, const char **argv)
{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
{ "cherry", cmd_cherry, RUN_SETUP },
{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
+ { "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
{ "commit-tree", cmd_commit_tree, RUN_SETUP },
{ "config", cmd_config },
{ "count-objects", cmd_count_objects, RUN_SETUP },
@@ -369,10 +370,10 @@ static void handle_internal_command(int argc, const char **argv)
{ "rev-parse", cmd_rev_parse, RUN_SETUP },
{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
{ "rm", cmd_rm, RUN_SETUP | NEED_WORK_TREE },
- { "runstatus", cmd_runstatus, RUN_SETUP | NEED_WORK_TREE },
{ "shortlog", cmd_shortlog, RUN_SETUP | USE_PAGER },
{ "show-branch", cmd_show_branch, RUN_SETUP },
{ "show", cmd_show, RUN_SETUP | USE_PAGER },
+ { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
{ "stripspace", cmd_stripspace },
{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
{ "tag", cmd_tag, RUN_SETUP },
diff --git a/strbuf.h b/strbuf.h
index 3a523ab..eba7369 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -100,5 +100,6 @@ extern int strbuf_read_file(struct strbuf *sb, const char *path);
extern void read_line(struct strbuf *, FILE *, int);
extern void stripspace(struct strbuf *buf, int skip_comments);
+extern void launch_editor(const char *path, struct strbuf *buffer);
#endif /* STRBUF_H */
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 552af1c..2dbe04f 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -44,7 +44,7 @@ test_expect_success setup '
test_expect_success 'cherry-pick after renaming branch' '
git checkout rename2 &&
- EDITOR=: VISUAL=: git cherry-pick added &&
+ git cherry-pick added &&
test -f opos &&
grep "Add extra line at the end" opos
@@ -53,7 +53,7 @@ test_expect_success 'cherry-pick after renaming branch' '
test_expect_success 'revert after renaming branch' '
git checkout rename1 &&
- EDITOR=: VISUAL=: git revert added &&
+ git revert added &&
test -f spoo &&
! grep "Add extra line at the end" spoo
diff --git a/t/t3901-i18n-patch.sh b/t/t3901-i18n-patch.sh
index 28e9e37..235f372 100755
--- a/t/t3901-i18n-patch.sh
+++ b/t/t3901-i18n-patch.sh
@@ -154,7 +154,7 @@ test_expect_success 'cherry-pick(U/U)' '
git reset --hard master &&
git cherry-pick side^ &&
git cherry-pick side &&
- EDITOR=: VISUAL=: git revert HEAD &&
+ git revert HEAD &&
check_encoding 3
'
@@ -169,7 +169,7 @@ test_expect_success 'cherry-pick(L/L)' '
git reset --hard master &&
git cherry-pick side^ &&
git cherry-pick side &&
- EDITOR=: VISUAL=: git revert HEAD &&
+ git revert HEAD &&
check_encoding 3 8859
'
@@ -184,7 +184,7 @@ test_expect_success 'cherry-pick(U/L)' '
git reset --hard master &&
git cherry-pick side^ &&
git cherry-pick side &&
- EDITOR=: VISUAL=: git revert HEAD &&
+ git revert HEAD &&
check_encoding 3
'
@@ -200,7 +200,7 @@ test_expect_success 'cherry-pick(L/U)' '
git reset --hard master &&
git cherry-pick side^ &&
git cherry-pick side &&
- EDITOR=: VISUAL=: git revert HEAD &&
+ git revert HEAD &&
check_encoding 3 8859
'
diff --git a/t/test-lib.sh b/t/test-lib.sh
index cc1253c..a232bd6 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -9,8 +9,8 @@ LC_ALL=C
PAGER=cat
TZ=UTC
export LANG LC_ALL PAGER TZ
-EDITOR=:
-VISUAL=:
+EDITOR=/bin/true
+VISUAL=/bin/true
unset GIT_EDITOR
unset AUTHOR_DATE
unset AUTHOR_EMAIL
--
1.5.3.1.993.gbf388-dirty
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 4/7] Clean up stripspace a bit, use strbuf even more.
2007-09-18 0:06 ` [PATCH 4/7] Clean up stripspace a bit, use strbuf even more Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 5/7] Add strbuf_read_file() Kristian Høgsberg
@ 2007-09-18 13:12 ` Johannes Schindelin
2007-09-18 13:52 ` Kristian Høgsberg
1 sibling, 1 reply; 18+ messages in thread
From: Johannes Schindelin @ 2007-09-18 13:12 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1001 bytes --]
Hi,
I really like your patch, except for this:
On Mon, 17 Sep 2007, Kristian Høgsberg wrote:
> diff --git a/builtin.h b/builtin.h
> index 03ee7bf..d6f2c76 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -7,7 +7,6 @@ extern const char git_version_string[];
> extern const char git_usage_string[];
>
> extern void help_unknown_cmd(const char *cmd);
> -extern size_t stripspace(char *buffer, size_t length, int skip_comments);
> extern int write_tree(unsigned char *sha1, int missing_ok, const char *prefix);
> extern void prune_packed_objects(int);
>
> diff --git a/strbuf.h b/strbuf.h
> index 21fc111..5960637 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -98,4 +98,6 @@ extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
>
> extern void read_line(struct strbuf *, FILE *, int);
>
> +extern void stripspace(struct strbuf *buf, int skip_comments);
> +
> #endif /* STRBUF_H */
If you do that, you have to move the function "stripspace" to strbuf.c,
too...
Ciao,
Dscho
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/7] Export rerere() and launch_editor().
2007-09-18 0:06 ` [PATCH 6/7] Export rerere() and launch_editor() Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 7/7] Implement git commit as a builtin command Kristian Høgsberg
@ 2007-09-18 13:14 ` Johannes Schindelin
2007-09-19 23:52 ` Junio C Hamano
2 siblings, 0 replies; 18+ messages in thread
From: Johannes Schindelin @ 2007-09-18 13:14 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
Hi,
I see rerere() in the patch, but not launch_editor().
Ciao,
Dscho
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/7] Clean up stripspace a bit, use strbuf even more.
2007-09-18 13:12 ` [PATCH 4/7] Clean up stripspace a bit, use strbuf even more Johannes Schindelin
@ 2007-09-18 13:52 ` Kristian Høgsberg
0 siblings, 0 replies; 18+ messages in thread
From: Kristian Høgsberg @ 2007-09-18 13:52 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
On Tue, 2007-09-18 at 14:12 +0100, Johannes Schindelin wrote:
> Hi,
>
> I really like your patch, except for this:
>
> On Mon, 17 Sep 2007, Kristian Høgsberg wrote:
>
> > diff --git a/builtin.h b/builtin.h
> > index 03ee7bf..d6f2c76 100644
> > --- a/builtin.h
> > +++ b/builtin.h
> > @@ -7,7 +7,6 @@ extern const char git_version_string[];
> > extern const char git_usage_string[];
> >
> > extern void help_unknown_cmd(const char *cmd);
> > -extern size_t stripspace(char *buffer, size_t length, int skip_comments);
> > extern int write_tree(unsigned char *sha1, int missing_ok, const char *prefix);
> > extern void prune_packed_objects(int);
> >
> > diff --git a/strbuf.h b/strbuf.h
> > index 21fc111..5960637 100644
> > --- a/strbuf.h
> > +++ b/strbuf.h
> > @@ -98,4 +98,6 @@ extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
> >
> > extern void read_line(struct strbuf *, FILE *, int);
> >
> > +extern void stripspace(struct strbuf *buf, int skip_comments);
> > +
> > #endif /* STRBUF_H */
>
> If you do that, you have to move the function "stripspace" to strbuf.c,
> too...
Right, the alternative is to #include strbuf.h in builtin.h, or maybe
just add struct strbuf; at the top. And with Pierres latest patch
strbuf is included everywhere, so maybe this is already moot.
Kristian
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 7/7] Implement git commit as a builtin command.
2007-09-18 0:06 ` [PATCH 7/7] Implement git commit as a builtin command Kristian Høgsberg
@ 2007-09-18 13:58 ` Johannes Schindelin
2007-09-18 15:07 ` Kristian Høgsberg
2007-09-20 1:27 ` Junio C Hamano
1 sibling, 1 reply; 18+ messages in thread
From: Johannes Schindelin @ 2007-09-18 13:58 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
Hi,
very nice!
Four nits, though, and a half:
- it would be nicer to put the option parsing it option.[ch] (you would
also need to pass the usage line then, instead of hardwiring it to
"git_commit_usage"),
- it seems more logical to me to call it "parse_option()" than
"scan_options()", since that is what it does,
- you might want to rename OPTION_NONE to OPTION_BOOLEAN, and maybe even
allow "--no-<option>" in that case for free,
- wt_status_prepare() could take a parameter "index_file", which would
default to git_path("index") when passed as NULL, and
- launch_editor() is defined in builtin-tag.c, which is not part of the
library, and therefore it would be technically more correct to either
move the function to editor.c (my preferred solution), or declare it in
builtin.h instead of strbuf.h.
As you can see, my nits are really minor, which means that I am pretty
happy with your work!
Thanks,
Dscho
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 7/7] Implement git commit as a builtin command.
2007-09-18 13:58 ` Johannes Schindelin
@ 2007-09-18 15:07 ` Kristian Høgsberg
0 siblings, 0 replies; 18+ messages in thread
From: Kristian Høgsberg @ 2007-09-18 15:07 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
On Tue, 2007-09-18 at 14:58 +0100, Johannes Schindelin wrote:
> Hi,
>
> very nice!
>
> Four nits, though, and a half:
>
> - it would be nicer to put the option parsing it option.[ch] (you would
> also need to pass the usage line then, instead of hardwiring it to
> "git_commit_usage"),
Yes, good point.
> - it seems more logical to me to call it "parse_option()" than
> "scan_options()", since that is what it does,
Yup.
> - you might want to rename OPTION_NONE to OPTION_BOOLEAN, and maybe even
> allow "--no-<option>" in that case for free,
Agree.
> - wt_status_prepare() could take a parameter "index_file", which would
> default to git_path("index") when passed as NULL, and
Yeah, the way I did it, I preserved the API, but that's not really a
concern, I guess.
> - launch_editor() is defined in builtin-tag.c, which is not part of the
> library, and therefore it would be technically more correct to either
> move the function to editor.c (my preferred solution), or declare it in
> builtin.h instead of strbuf.h.
Yeah, and we should move the stripspace code there too. Or maybe we
should rename that strbuf_stripspace and put it in strbuf.c.
> As you can see, my nits are really minor, which means that I am pretty
> happy with your work!
Great, I hope we can get it in soon :)
Kristian
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/7] Export rerere() and launch_editor().
2007-09-18 0:06 ` [PATCH 6/7] Export rerere() and launch_editor() Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 7/7] Implement git commit as a builtin command Kristian Høgsberg
2007-09-18 13:14 ` [PATCH 6/7] Export rerere() and launch_editor() Johannes Schindelin
@ 2007-09-19 23:52 ` Junio C Hamano
2007-09-21 18:01 ` Kristian Høgsberg
2 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2007-09-19 23:52 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
Kristian Høgsberg <krh@redhat.com> writes:
> +/* Export for builtin-commit. */
> +int rerere(void)
> +{
> + struct path_list merge_rr = { NULL, 0, 0, 1 };
> + int fd;
> +
> + git_config(git_rerere_config);
> + if (!is_rerere_enabled())
> + return 0;
> +
> + merge_rr_path = xstrdup(git_path("rr-cache/MERGE_RR"));
> + fd = hold_lock_file_for_update(&write_lock, merge_rr_path, 1);
> + read_rr(&merge_rr);
> + return do_plain_rerere(&merge_rr, fd);
> +}
Is it just me who sees a suboptimal cut and paste here?
BTW, [1-5/7] look good so far.
diff --git a/builtin-rerere.c b/builtin-rerere.c
index 29d057c..2f51ae0 100644
--- a/builtin-rerere.c
+++ b/builtin-rerere.c
@@ -415,18 +415,39 @@ static int is_rerere_enabled(void)
return 1;
}
-int cmd_rerere(int argc, const char **argv, const char *prefix)
+static int setup_rerere(struct path_list *merge_rr)
{
- struct path_list merge_rr = { NULL, 0, 0, 1 };
- int i, fd = -1;
+ int fd;
git_config(git_rerere_config);
if (!is_rerere_enabled())
- return 0;
+ return -1;
merge_rr_path = xstrdup(git_path("rr-cache/MERGE_RR"));
fd = hold_lock_file_for_update(&write_lock, merge_rr_path, 1);
- read_rr(&merge_rr);
+ read_rr(merge_rr);
+ return fd;
+}
+
+int rerere(void)
+{
+ struct path_list merge_rr = { NULL, 0, 0, 1 };
+ int fd;
+
+ fd = setup_rerere(&merge_rr);
+ if (fd < 0)
+ return 0;
+ return do_plain_rerere(&merge_rr, fd);
+}
+
+int cmd_rerere(int argc, const char **argv, const char *prefix)
+{
+ struct path_list merge_rr = { NULL, 0, 0, 1 };
+ int i, fd;
+
+ fd = setup_rerere(&merge_rr);
+ if (fd < 0)
+ return 0;
if (argc < 2)
return do_plain_rerere(&merge_rr, fd);
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 7/7] Implement git commit as a builtin command.
2007-09-18 0:06 ` [PATCH 7/7] Implement git commit as a builtin command Kristian Høgsberg
2007-09-18 13:58 ` Johannes Schindelin
@ 2007-09-20 1:27 ` Junio C Hamano
2007-09-21 17:18 ` Kristian Høgsberg
1 sibling, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2007-09-20 1:27 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
Kristian Høgsberg <krh@redhat.com> writes:
> diff --git a/builtin-commit.c b/builtin-commit.c
> new file mode 100644
> index 0000000..ee98de9
> --- /dev/null
> +++ b/builtin-commit.c
> @@ -0,0 +1,740 @@
> +/*
> + * Builtin "git commit"
> + *
> + * Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
> + * Based on git-commit.sh by Junio C Hamano and Linus Torvalds
> + */
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +
With 85023577a8f4b540aa64aa37f6f44578c0c305a3 (simplify
inclusion of system header files.), we introduced a rule against
these lines. We probably would want a Coding Guideline (aka
"Hacking Git") document somewhere.
> +#include "cache.h"
> +#include "cache-tree.h"
> +#include "builtin.h"
> +#include "diff.h"
> +#include "diffcore.h"
> +#include "commit.h"
> +#include "revision.h"
> +#include "wt-status.h"
> +#include "run-command.h"
> +#include "refs.h"
> +#include "log-tree.h"
> +#include "strbuf.h"
> +#include "utf8.h"
> +
> +static const char builtin_commit_usage[] =
> + "[-a | --interactive] [-s] [-v] [--no-verify] [-m <message> | -F <logfile> | (-C|-c) <commit> | --amend] [-u] [-e] [--author <author>] [--template <file>] [[-i | -o] <path>...]";
> +
> +static unsigned char head_sha1[20], merge_head_sha1[20];
> +static char *use_message_buffer;
> +static const char commit_editmsg[] = "COMMIT_EDITMSG";
> +static struct lock_file lock_file;
> +
> +enum option_type {
> + OPTION_NONE,
> + OPTION_STRING,
> + OPTION_INTEGER,
> + OPTION_LAST,
> +};
> +
> +struct option {
> + enum option_type type;
> + const char *long_name;
> + char short_name;
> + void *value;
> +};
> +
> +static int scan_options(const char ***argv, struct option *options)
> +{
> + const char *value, *eq;
> + int i;
> +
> + if (**argv == NULL)
> + return 0;
> + if ((**argv)[0] != '-')
> + return 0;
> + if (!strcmp(**argv, "--"))
> + return 0;
> +
> + value = NULL;
> + for (i = 0; options[i].type != OPTION_LAST; i++) {
> + if ((**argv)[1] == '-') {
> + if (!prefixcmp(options[i].long_name, **argv + 2)) {
> + if (options[i].type != OPTION_NONE)
> + value = *++(*argv);
> + goto match;
> + }
> +
> + eq = strchr(**argv + 2, '=');
> + if (eq && options[i].type != OPTION_NONE &&
> + !strncmp(**argv + 2,
> + options[i].long_name, eq - **argv - 2)) {
> + value = eq + 1;
> + goto match;
> + }
> + }
> +
> + if ((**argv)[1] == options[i].short_name) {
> + if ((**argv)[2] == '\0') {
> + if (options[i].type != OPTION_NONE)
> + value = *++(*argv);
> + goto match;
> + }
> +
> + if (options[i].type != OPTION_NONE) {
> + value = **argv + 2;
> + goto match;
> + }
> + }
> + }
How do you disambiguate between "--author <me>" and "--amend"?
"The order in *options list matters" is an acceptable answer but
it needs to be documented.
> +
> + usage(builtin_commit_usage);
> +
> + match:
> + switch (options[i].type) {
> + case OPTION_NONE:
> + *(int *)options[i].value = 1;
> + break;
> + case OPTION_STRING:
> + if (value == NULL)
> + die("option %s requires a value.", (*argv)[-1]);
> + *(const char **)options[i].value = value;
> + break;
> + case OPTION_INTEGER:
> + if (value == NULL)
> + die("option %s requires a value.", (*argv)[-1]);
> + *(int *)options[i].value = atoi(value);
> + break;
> + default:
> + assert(0);
> + }
> +
> + (*argv)++;
> +
> + return 1;
> +}
I do not particularly like this OPTION_LAST convention, but that
is a minor detail. I also suspect in the longer term we might
be better off using getopt() or popt(), but that would be a
larger project.
In any case, if you want to use this option parser, you would
need to add a new file, perhaps parse_options.c, and move this
part there, so that other parts of the system can reuse it.
And the same comment goes for the launch_editor still in
builtin-tag.c. We should move it to editor.c or something.
> +
> +static char *logfile, *force_author, *message, *template_file;
> +static char *edit_message, *use_message;
> +static int all, edit_flag, also, interactive, only, no_verify, amend, signoff;
> +static int quiet, verbose, untracked_files;
> +
> +static int no_edit, initial_commit, in_merge;
> +const char *only_include_assumed;
> +
> +static struct option commit_options[] = {
> + { OPTION_STRING, "file", 'F', (void *) &logfile },
> + { OPTION_NONE, "all", 'a', &all },
> + { OPTION_STRING, "author", 0, (void *) &force_author },
> + { OPTION_NONE, "edit", 'e', &edit_flag },
Why do some get casted to (void*) and others don't? It doesn't
seem to have any pattern. I am puzzled...
> + { OPTION_NONE, "include", 'i', &also },
> + { OPTION_NONE, "interactive", 0, &interactive },
> + { OPTION_NONE, "only", 'o', &only },
> + { OPTION_STRING, "message", 'm', &message },
> + { OPTION_NONE, "no-verify", 'n', &no_verify },
> + { OPTION_NONE, "amend", 0, &amend },
> + { OPTION_STRING, "reedit-message", 'c', &edit_message },
> + { OPTION_STRING, "reuse-message", 'C', &use_message },
> + { OPTION_NONE, "signoff", 's', &signoff },
> + { OPTION_NONE, "quiet", 'q', &quiet },
> + { OPTION_NONE, "verbose", 'v', &verbose },
> + { OPTION_NONE, "untracked-files", 0, &untracked_files },
> + { OPTION_STRING, "template", 't', &template_file },
> + { OPTION_LAST },
> +};
> +
> +/* FIXME: Taken from builtin-add, should be shared. */
You're darn right. I thought you have some other patch that
touches builtin-add already...
> +
> +static void update_callback(struct diff_queue_struct *q,
> + struct diff_options *opt, void *cbdata)
> +{
> + int i, verbose;
> +
> + verbose = *((int *)cbdata);
> + for (i = 0; i < q->nr; i++) {
> + struct diff_filepair *p = q->queue[i];
> + const char *path = p->one->path;
> + switch (p->status) {
> + default:
> + die("unexpacted diff status %c", p->status);
> + case DIFF_STATUS_UNMERGED:
> + case DIFF_STATUS_MODIFIED:
> + case DIFF_STATUS_TYPE_CHANGED:
> + add_file_to_cache(path, verbose);
> + break;
> + case DIFF_STATUS_DELETED:
> + remove_file_from_cache(path);
> + cache_tree_invalidate_path(active_cache_tree, path);
I've updated remove_file_from_cache() to invalidate the path in
cache-tree so this does not hurt but is no longer necessary. I
removed this line in the version I queued in 'pu'.
> + if (verbose)
> + printf("remove '%s'\n", path);
> + break;
> + }
> + }
> +}
> +
> +static void
> +add_files_to_cache(int fd, const char **files, const char *prefix)
> +{
> + struct rev_info rev;
> +
> + init_revisions(&rev, "");
> + setup_revisions(0, NULL, &rev, NULL);
> + rev.prune_data = get_pathspec(prefix, files);
> + rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
> + rev.diffopt.format_callback = update_callback;
> + rev.diffopt.format_callback_data = &verbose;
> +
> + run_diff_files(&rev, 0);
> + refresh_cache(REFRESH_QUIET);
Your update_callback() does add_file_to_cache() which picks up
the stat information from the filesystem already, so I do not
see much point calling refresh_cache() afterwards. On the other
hand, refreshing before running diff_files() might make sense,
as you can avoid a lot of unnecessary work when you are told to
do "git commit ."
Have you tested this with an unmerged index?
> +
> + if (write_cache(fd, active_cache, active_nr) || close(fd))
> + die("unable to write new index file");
> +}
> +
> +static char *
> +prepare_index(const char **files, const char *prefix)
> +{
> + int fd;
> + struct tree *tree;
> + struct lock_file *next_index_lock;
> +
> + fd = hold_locked_index(&lock_file, 1);
> + if (read_cache() < 0)
> + die("index file corrupt");
> +
> + if (all) {
> + add_files_to_cache(fd, files, NULL);
> + return lock_file.filename;
Should you be passing files, which is used as pathspec to decide
if your update_callback() should be called, under --all option?
> + } else if (also) {
> + add_files_to_cache(fd, files, prefix);
> + return lock_file.filename;
> + }
> +
> + if (interactive)
> + interactive_add();
Don't you need to
(1) abort if the user aborts out of interactive add with ^C?
(2) re-read the index after interactive_add() returns?
> + if (*files == NULL) {
> + /* Commit index as-is. */
> + rollback_lock_file(&lock_file);
> + return get_index_file();
> + }
> +
> + /*
> + * FIXME: Warn on unknown files. Shell script does
> + *
> + * commit_only=`git-ls-files --error-unmatch -- "$@"`
> + */
This should be much easier to do than from the shell script, as
you have active_cache[] (aka "the_index.cache[]") in-core.
> + /*
> + * FIXME: shell script does
> + *
> + * git-read-tree --index-output="$TMP_INDEX" -i -m HEAD
> + *
> + * which warns about unmerged files in the index.
> + */
I think "unmerged warning" is an unintended side effect. The
point of the command is to have a copy of index, as there is no
way for shell script to work with more than one index at a time
without using a temporary file.
> +
> + /* update the user index file */
> + add_files_to_cache(fd, files, prefix);
> +
> + if (!initial_commit) {
> + tree = parse_tree_indirect(head_sha1);
> + if (!tree)
> + die("failed to unpack HEAD tree object");
> + if (read_tree(tree, 0, NULL))
> + die("failed to read HEAD tree object");
> + }
Huh? Doesn't this read_tree() defeat the add_files_to_cache()
you did earlier?
> +
> + /* Uh oh, abusing lock_file to create a garbage collected file */
> + next_index_lock = xmalloc(sizeof(*next_index_lock));
> + fd = hold_lock_file_for_update(next_index_lock,
> + git_path("next-index-%d", getpid()), 1);
That's not an abuse, but I wonder what happened to the fd you
got at the beginning of the function.
> + add_files_to_cache(fd, files, prefix);
and then this is puzzling to me.
I am starting to suspect that you might be better off if you do
not follow the use of temporary index file that was in the shell
script version to the letter. You can use more than one index
in the core at the same time, now you are built-in.
> +
> + return next_index_lock->filename;
> +}
... and if we were to go that route, wt_status structure would
have a pointer to the "struct index_state" instead of the
filename of the index... oops, wt_status_print() will discard
and re-read the cache from file, so that approach would not work
right now without fixing wt-status first. Hmm.
> +static int run_status(FILE *fp, const char *index_file)
> +{
> + struct wt_status s;
> +
> + wt_status_prepare(&s);
> +
> + if (amend) {
> + s.amend = 1;
> + s.reference = "HEAD^1";
> + }
> + s.verbose = verbose;
> + s.untracked = untracked_files;
> + s.index_file = index_file;
> + s.fp = fp;
> +
> + wt_status_print(&s);
> +
> + return s.commitable;
> +}
I did not look at the rest of the patch; it should be either
obviously correct or outright incorrect --- anybody would notice
the breakage immediately.
The prepare_index() part is the most (and only) "interesting"
part of the puzzle. I need to look at this again and think
about it a bit more. It needs to:
* save the current index and restore in case the whole
"git-commit" is aborted in any way (e.g. ^C, empty log
message from the editor); This is easy to do from C with
hold_locked_index().
* when doing a partial commit,
- read HEAD in a temporary index, if not initial commit;
- update the temporary index with the given paths;
- write out the temporary index as a tree to build a commit;
- update the real index the same way with the given paths;
- if all goes well, commit the updated real index; again,
this is easy in C with commit_locked_index();
* when not doing a partial commit,
- update the real index with given paths (if "--also") or all
what diff-files reports (if "--all");
- write out the real index as a tree to build a commit;
- if all goes well, commit the updated real index; again,
this is easy in C with commit_locked_index();
A thing to keep in mind is that you can write out the temporary
index as a tree from the core without writing it out first to a
temporary index file at all. Perhaps the code should be
structured like this.
- parse options and all the other necessary setup;
- hold_locked_index() to lock the "real" index;
- prepare_index() is responsibile for doing two things
1. build, write out and return the tree object to be
committed;
2. leave the_index[] in the state to become the "real"
index if this commit command is not aborted;
- take the tree object, put commit log message around it and
make a commit object;
- commit_locked_index() to write out the "real" index.
Now, the task #1 for prepare_index() is simpler for "also" and
"all" case. You do the equivalent of "git-add -u" or "git add
<path>" and run cache_tree_update() to write out the tree, and
when you are done, both the_index.cache and the_index.cache_tree
are up-to-date, ready to be written out by the caller at the
very end.
For a partial commit, the task is a bit more convoluted, as
writing out the tree needs to be done from outside the_index. I
would say something like this:
int prepare_index(unsigned char *sha1) {
if (partial commit) {
struct index_state tmp_index;
/* "temporary index" only in-core */
memset(&tmp_index, 0, sizeof(tmp_index));
read_index(&tmp_index);
add_files_to_index(files, prefix, &tmp_index);
write_index_as_tree(&tmp_index, sha1);
discard_index(&tmp_index);
/* update the index the same way */
add_files_to_index(files, prefix, &the_index);
return ok;
}
/* otherwise */
if (files && *files)
add_files_to_index(files, prefix, &the_index);
else if (all)
add_files_to_index(NULL, prefix, &the_index);
write_index_as_tree(&the_index, sha1);
return ok;
}
where
* add_files_to_index() is similar to your add_files_to_cache()
but takes struct index_state; the callback can still diff
with the_index.cache (aka active_cache) as that diff is what
we are interested in updating anyway;
* write_index_as_tree() would be a new function that takes
struct index_state and writes that as a tree. It would
essentially be the builtin-write-tree.c::write_tree()
function except the opening and reading the index (or
updating the cache-tree) part, something like:
write_index_as_tree(struct index_state *istate, unsigned char *sha1)
{
if (!cache_tree_fully_valid(istate->cache_tree))
cache_tree_update(istate->cache_tree,
istate->cache,
istate->cache_nr, 0, 0);
hashcpy(sha1, istate->cache_tree->sha1);
}
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 7/7] Implement git commit as a builtin command.
2007-09-20 1:27 ` Junio C Hamano
@ 2007-09-21 17:18 ` Kristian Høgsberg
2007-09-21 19:32 ` Junio C Hamano
0 siblings, 1 reply; 18+ messages in thread
From: Kristian Høgsberg @ 2007-09-21 17:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Wed, 2007-09-19 at 18:27 -0700, Junio C Hamano wrote:
> Kristian Høgsberg <krh@redhat.com> writes:
>
> > diff --git a/builtin-commit.c b/builtin-commit.c
> > new file mode 100644
> > index 0000000..ee98de9
> > --- /dev/null
> > +++ b/builtin-commit.c
> > @@ -0,0 +1,740 @@
> > +/*
> > + * Builtin "git commit"
> > + *
> > + * Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
> > + * Based on git-commit.sh by Junio C Hamano and Linus Torvalds
> > + */
> > +
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <unistd.h>
> > +
>
> With 85023577a8f4b540aa64aa37f6f44578c0c305a3 (simplify
> inclusion of system header files.), we introduced a rule against
> these lines. We probably would want a Coding Guideline (aka
> "Hacking Git") document somewhere.
Ok. I see you removed them in the pu commit, thanks.
> > +#include "cache.h"
> > +#include "cache-tree.h"
> > +#include "builtin.h"
> > +#include "diff.h"
> > +#include "diffcore.h"
> > +#include "commit.h"
> > +#include "revision.h"
> > +#include "wt-status.h"
> > +#include "run-command.h"
> > +#include "refs.h"
> > +#include "log-tree.h"
> > +#include "strbuf.h"
> > +#include "utf8.h"
> > +
> > +static const char builtin_commit_usage[] =
> > + "[-a | --interactive] [-s] [-v] [--no-verify] [-m <message> | -F <logfile> | (-C|-c) <commit> | --amend] [-u] [-e] [--author <author>] [--template <file>] [[-i | -o] <path>...]";
> > +
> > +static unsigned char head_sha1[20], merge_head_sha1[20];
> > +static char *use_message_buffer;
> > +static const char commit_editmsg[] = "COMMIT_EDITMSG";
> > +static struct lock_file lock_file;
> > +
> > +enum option_type {
> > + OPTION_NONE,
> > + OPTION_STRING,
> > + OPTION_INTEGER,
> > + OPTION_LAST,
> > +};
> > +
> > +struct option {
> > + enum option_type type;
> > + const char *long_name;
> > + char short_name;
> > + void *value;
> > +};
> > +
> > +static int scan_options(const char ***argv, struct option *options)
> > +{
> > + const char *value, *eq;
> > + int i;
> > +
> > + if (**argv == NULL)
> > + return 0;
> > + if ((**argv)[0] != '-')
> > + return 0;
> > + if (!strcmp(**argv, "--"))
> > + return 0;
> > +
> > + value = NULL;
> > + for (i = 0; options[i].type != OPTION_LAST; i++) {
> > + if ((**argv)[1] == '-') {
> > + if (!prefixcmp(options[i].long_name, **argv + 2)) {
> > + if (options[i].type != OPTION_NONE)
> > + value = *++(*argv);
> > + goto match;
> > + }
> > +
> > + eq = strchr(**argv + 2, '=');
> > + if (eq && options[i].type != OPTION_NONE &&
> > + !strncmp(**argv + 2,
> > + options[i].long_name, eq - **argv - 2)) {
> > + value = eq + 1;
> > + goto match;
> > + }
> > + }
> > +
> > + if ((**argv)[1] == options[i].short_name) {
> > + if ((**argv)[2] == '\0') {
> > + if (options[i].type != OPTION_NONE)
> > + value = *++(*argv);
> > + goto match;
> > + }
> > +
> > + if (options[i].type != OPTION_NONE) {
> > + value = **argv + 2;
> > + goto match;
> > + }
> > + }
> > + }
>
> How do you disambiguate between "--author <me>" and "--amend"?
> "The order in *options list matters" is an acceptable answer but
> it needs to be documented.
Yes, the order matters. I made sure the C version has the same
precedence as the shell script. I'll add a comment in parse-options.h.
> > +
> > + usage(builtin_commit_usage);
> > +
> > + match:
> > + switch (options[i].type) {
> > + case OPTION_NONE:
> > + *(int *)options[i].value = 1;
> > + break;
> > + case OPTION_STRING:
> > + if (value == NULL)
> > + die("option %s requires a value.", (*argv)[-1]);
> > + *(const char **)options[i].value = value;
> > + break;
> > + case OPTION_INTEGER:
> > + if (value == NULL)
> > + die("option %s requires a value.", (*argv)[-1]);
> > + *(int *)options[i].value = atoi(value);
> > + break;
> > + default:
> > + assert(0);
> > + }
> > +
> > + (*argv)++;
> > +
> > + return 1;
> > +}
>
> I do not particularly like this OPTION_LAST convention, but that
> is a minor detail. I also suspect in the longer term we might
> be better off using getopt() or popt(), but that would be a
> larger project.
>
> In any case, if you want to use this option parser, you would
> need to add a new file, perhaps parse_options.c, and move this
> part there, so that other parts of the system can reuse it.
I'll split it out in parse-options.[ch]. I still don't understand why
using getopt is better; parse-options.c is a 75 line file and it's
simple code. It does more work that getopt, since for most options it
automatically writes back the option argument into a global. All I have
to do then is validate that no illegal combination of options was
passed.
> And the same comment goes for the launch_editor still in
> builtin-tag.c. We should move it to editor.c or something.
Yeah.
> > +
> > +static char *logfile, *force_author, *message, *template_file;
> > +static char *edit_message, *use_message;
> > +static int all, edit_flag, also, interactive, only, no_verify, amend, signoff;
> > +static int quiet, verbose, untracked_files;
> > +
> > +static int no_edit, initial_commit, in_merge;
> > +const char *only_include_assumed;
> > +
> > +static struct option commit_options[] = {
> > + { OPTION_STRING, "file", 'F', (void *) &logfile },
> > + { OPTION_NONE, "all", 'a', &all },
> > + { OPTION_STRING, "author", 0, (void *) &force_author },
> > + { OPTION_NONE, "edit", 'e', &edit_flag },
>
> Why do some get casted to (void*) and others don't? It doesn't
> seem to have any pattern. I am puzzled...
Yes, oops, not sure what that was about.
> > + { OPTION_NONE, "include", 'i', &also },
> > + { OPTION_NONE, "interactive", 0, &interactive },
> > + { OPTION_NONE, "only", 'o', &only },
> > + { OPTION_STRING, "message", 'm', &message },
> > + { OPTION_NONE, "no-verify", 'n', &no_verify },
> > + { OPTION_NONE, "amend", 0, &amend },
> > + { OPTION_STRING, "reedit-message", 'c', &edit_message },
> > + { OPTION_STRING, "reuse-message", 'C', &use_message },
> > + { OPTION_NONE, "signoff", 's', &signoff },
> > + { OPTION_NONE, "quiet", 'q', &quiet },
> > + { OPTION_NONE, "verbose", 'v', &verbose },
> > + { OPTION_NONE, "untracked-files", 0, &untracked_files },
> > + { OPTION_STRING, "template", 't', &template_file },
> > + { OPTION_LAST },
> > +};
> > +
> > +/* FIXME: Taken from builtin-add, should be shared. */
>
> You're darn right. I thought you have some other patch that
> touches builtin-add already...
Yeah, I'll fix that.
> > +
> > +static void update_callback(struct diff_queue_struct *q,
> > + struct diff_options *opt, void *cbdata)
> > +{
> > + int i, verbose;
> > +
> > + verbose = *((int *)cbdata);
> > + for (i = 0; i < q->nr; i++) {
> > + struct diff_filepair *p = q->queue[i];
> > + const char *path = p->one->path;
> > + switch (p->status) {
> > + default:
> > + die("unexpacted diff status %c", p->status);
> > + case DIFF_STATUS_UNMERGED:
> > + case DIFF_STATUS_MODIFIED:
> > + case DIFF_STATUS_TYPE_CHANGED:
> > + add_file_to_cache(path, verbose);
> > + break;
> > + case DIFF_STATUS_DELETED:
> > + remove_file_from_cache(path);
> > + cache_tree_invalidate_path(active_cache_tree, path);
>
> I've updated remove_file_from_cache() to invalidate the path in
> cache-tree so this does not hurt but is no longer necessary. I
> removed this line in the version I queued in 'pu'.
>
> > + if (verbose)
> > + printf("remove '%s'\n", path);
> > + break;
> > + }
> > + }
> > +}
> > +
> > +static void
> > +add_files_to_cache(int fd, const char **files, const char *prefix)
> > +{
> > + struct rev_info rev;
> > +
> > + init_revisions(&rev, "");
> > + setup_revisions(0, NULL, &rev, NULL);
> > + rev.prune_data = get_pathspec(prefix, files);
> > + rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
> > + rev.diffopt.format_callback = update_callback;
> > + rev.diffopt.format_callback_data = &verbose;
> > +
> > + run_diff_files(&rev, 0);
> > + refresh_cache(REFRESH_QUIET);
>
> Your update_callback() does add_file_to_cache() which picks up
> the stat information from the filesystem already, so I do not
> see much point calling refresh_cache() afterwards. On the other
> hand, refreshing before running diff_files() might make sense,
> as you can avoid a lot of unnecessary work when you are told to
> do "git commit ."
I didn't have refresh_cache() in there originally, there was some test
case that didn't pass until I added it. The git-commit.sh shell script
calls git update-index --refresh after the prepare_index() part, which
is where I got it from. Will investigate.
> Have you tested this with an unmerged index?
Not yet.
> > +
> > + if (write_cache(fd, active_cache, active_nr) || close(fd))
> > + die("unable to write new index file");
> > +}
> > +
> > +static char *
> > +prepare_index(const char **files, const char *prefix)
> > +{
> > + int fd;
> > + struct tree *tree;
> > + struct lock_file *next_index_lock;
> > +
> > + fd = hold_locked_index(&lock_file, 1);
> > + if (read_cache() < 0)
> > + die("index file corrupt");
> > +
> > + if (all) {
> > + add_files_to_cache(fd, files, NULL);
> > + return lock_file.filename;
>
> Should you be passing files, which is used as pathspec to decide
> if your update_callback() should be called, under --all option?
Oh... hmm, if 'all' is set there are no paths, but yes, that is
confusing.
> > + } else if (also) {
> > + add_files_to_cache(fd, files, prefix);
> > + return lock_file.filename;
> > + }
> > +
> > + if (interactive)
> > + interactive_add();
>
> Don't you need to
>
> (1) abort if the user aborts out of interactive add with ^C?
> (2) re-read the index after interactive_add() returns?
Uhm, yes, good points.
> > + if (*files == NULL) {
> > + /* Commit index as-is. */
> > + rollback_lock_file(&lock_file);
> > + return get_index_file();
> > + }
> > +
> > + /*
> > + * FIXME: Warn on unknown files. Shell script does
> > + *
> > + * commit_only=`git-ls-files --error-unmatch -- "$@"`
> > + */
>
> This should be much easier to do than from the shell script, as
> you have active_cache[] (aka "the_index.cache[]") in-core.
>
> > + /*
> > + * FIXME: shell script does
> > + *
> > + * git-read-tree --index-output="$TMP_INDEX" -i -m HEAD
> > + *
> > + * which warns about unmerged files in the index.
> > + */
>
> I think "unmerged warning" is an unintended side effect. The
> point of the command is to have a copy of index, as there is no
> way for shell script to work with more than one index at a time
> without using a temporary file.
Yes, we talked about this in IRC a few weeks back, and agreed that just
read_tree() should be fine. I'll just remove that FIXME.
> > +
> > + /* update the user index file */
> > + add_files_to_cache(fd, files, prefix);
> > +
> > + if (!initial_commit) {
> > + tree = parse_tree_indirect(head_sha1);
> > + if (!tree)
> > + die("failed to unpack HEAD tree object");
> > + if (read_tree(tree, 0, NULL))
> > + die("failed to read HEAD tree object");
> > + }
>
> Huh? Doesn't this read_tree() defeat the add_files_to_cache()
> you did earlier?
This is the case where we add the files on the command line
to .git/index, but commit from a clean index file corresponding to HEAD
with the files from the command line added (partial commit?). The first
add_files_to_cache() updates .git/index, then we do read_tree() to build
a tmp index from HEAD and then we add the files again. The tmp index is
written to a tmp index file.
> > +
> > + /* Uh oh, abusing lock_file to create a garbage collected file */
> > + next_index_lock = xmalloc(sizeof(*next_index_lock));
> > + fd = hold_lock_file_for_update(next_index_lock,
> > + git_path("next-index-%d", getpid()), 1);
>
> That's not an abuse, but I wonder what happened to the fd you
> got at the beginning of the function.
Ugh, yeah, that's a bit ugly... it gets closed in add_files_to_cache()
once we've written the index. The patch I have here uses the
add_files_to_cache() from builtin-add.c which doesn't close the fd or
write the index. That now happens in prepare_index(), so it's a little
clearer what's going on.
> > + add_files_to_cache(fd, files, prefix);
>
> and then this is puzzling to me.
>
> I am starting to suspect that you might be better off if you do
> not follow the use of temporary index file that was in the shell
> script version to the letter. You can use more than one index
> in the core at the same time, now you are built-in.
As described above, we need to add the files to the user index and the
temporary index we're committing from. As for just using an in-memory
index, I wanted to do it that way originally, but you have to write it
to disk after all for the pre-commit hook. Maybe doing it in-memory is
better after all, and then only write it to disk if we actually have a
pre-commit hook.
> > +
> > + return next_index_lock->filename;
> > +}
>
> ... and if we were to go that route, wt_status structure would
> have a pointer to the "struct index_state" instead of the
> filename of the index... oops, wt_status_print() will discard
> and re-read the cache from file, so that approach would not work
> right now without fixing wt-status first. Hmm.
Yep, that was the other reason. I suppose a short term fix for this
would be to make run_status() take a struct index_state and write it to
a temp file and run against that. We can then clean that up to not use
a temp file in a second patch.
> > +static int run_status(FILE *fp, const char *index_file)
> > +{
> > + struct wt_status s;
> > +
> > + wt_status_prepare(&s);
> > +
> > + if (amend) {
> > + s.amend = 1;
> > + s.reference = "HEAD^1";
> > + }
> > + s.verbose = verbose;
> > + s.untracked = untracked_files;
> > + s.index_file = index_file;
> > + s.fp = fp;
> > +
> > + wt_status_print(&s);
> > +
> > + return s.commitable;
> > +}
>
> I did not look at the rest of the patch; it should be either
> obviously correct or outright incorrect --- anybody would notice
> the breakage immediately.
>
> The prepare_index() part is the most (and only) "interesting"
> part of the puzzle. I need to look at this again and think
> about it a bit more. It needs to:
>
> * save the current index and restore in case the whole
> "git-commit" is aborted in any way (e.g. ^C, empty log
> message from the editor); This is easy to do from C with
> hold_locked_index().
This is working in the current version. I use hold_locked_index() in
the beginning of prepare_index() and calls commit_locked_index() to
write the new index at the end of cmd_commit() once the commit is done.
> * when doing a partial commit,
> - read HEAD in a temporary index, if not initial commit;
> - update the temporary index with the given paths;
> - write out the temporary index as a tree to build a commit;
>
> - update the real index the same way with the given paths;
> - if all goes well, commit the updated real index; again,
> this is easy in C with commit_locked_index();
prepare_index() does this, that's the case you pointed out above, where
I'm calling add_files_to_cache() twice.
> * when not doing a partial commit,
> - update the real index with given paths (if "--also") or all
> what diff-files reports (if "--all");
> - write out the real index as a tree to build a commit;
> - if all goes well, commit the updated real index; again,
> this is easy in C with commit_locked_index();
This is also in prepare_index() - the 'if (all)' and 'if (also)' cases
in the beginning of the function does that, except commiting the the
updated real index, that's done at the end of cmd_commit(), share among
all cases.
> A thing to keep in mind is that you can write out the temporary
> index as a tree from the core without writing it out first to a
> temporary index file at all. Perhaps the code should be
> structured like this.
>
> - parse options and all the other necessary setup;
>
> - hold_locked_index() to lock the "real" index;
>
> - prepare_index() is responsibile for doing two things
>
> 1. build, write out and return the tree object to be
> committed;
>
> 2. leave the_index[] in the state to become the "real"
> index if this commit command is not aborted;
>
> - take the tree object, put commit log message around it and
> make a commit object;
>
> - commit_locked_index() to write out the "real" index.
>
> Now, the task #1 for prepare_index() is simpler for "also" and
> "all" case. You do the equivalent of "git-add -u" or "git add
> <path>" and run cache_tree_update() to write out the tree, and
> when you are done, both the_index.cache and the_index.cache_tree
> are up-to-date, ready to be written out by the caller at the
> very end.
Yes, but isn't that what the code is doing now? I write out the tree in
cmd_commit(), not in prepare_index(), but prepare_index() is also used
in cmd_status(). It seems better to only write the tree if we're going
to do a commit. Other than that, the flow of the code follows your
description pretty much step by step.
> For a partial commit, the task is a bit more convoluted, as
> writing out the tree needs to be done from outside the_index. I
> would say something like this:
>
> int prepare_index(unsigned char *sha1) {
> if (partial commit) {
> struct index_state tmp_index;
>
> /* "temporary index" only in-core */
> memset(&tmp_index, 0, sizeof(tmp_index));
> read_index(&tmp_index);
> add_files_to_index(files, prefix, &tmp_index);
> write_index_as_tree(&tmp_index, sha1);
> discard_index(&tmp_index);
>
> /* update the index the same way */
> add_files_to_index(files, prefix, &the_index);
> return ok;
> }
> /* otherwise */
> if (files && *files)
> add_files_to_index(files, prefix, &the_index);
> else if (all)
> add_files_to_index(NULL, prefix, &the_index);
> write_index_as_tree(&the_index, sha1);
> return ok;
> }
>
> where
>
> * add_files_to_index() is similar to your add_files_to_cache()
> but takes struct index_state; the callback can still diff
> with the_index.cache (aka active_cache) as that diff is what
> we are interested in updating anyway;
>
> * write_index_as_tree() would be a new function that takes
> struct index_state and writes that as a tree. It would
> essentially be the builtin-write-tree.c::write_tree()
> function except the opening and reading the index (or
> updating the cache-tree) part, something like:
>
> write_index_as_tree(struct index_state *istate, unsigned char *sha1)
> {
> if (!cache_tree_fully_valid(istate->cache_tree))
> cache_tree_update(istate->cache_tree,
> istate->cache,
> istate->cache_nr, 0, 0);
> hashcpy(sha1, istate->cache_tree->sha1);
> }
That looks nicer, the add_files_to_index() function is a pretty clean
solution. But again, we can't write the tree in prepare_index(); we
need the temporary index for the pre-commit hook, and prepare_index() is
used in cmd_status(). But we can just return an struct index_state
pointer from prepare_index().
Thanks for reviewing,
Kristian
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/7] Export rerere() and launch_editor().
2007-09-19 23:52 ` Junio C Hamano
@ 2007-09-21 18:01 ` Kristian Høgsberg
0 siblings, 0 replies; 18+ messages in thread
From: Kristian Høgsberg @ 2007-09-21 18:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Wed, 2007-09-19 at 16:52 -0700, Junio C Hamano wrote:
> Kristian Høgsberg <krh@redhat.com> writes:
>
> > +/* Export for builtin-commit. */
> > +int rerere(void)
> > +{
> > + struct path_list merge_rr = { NULL, 0, 0, 1 };
> > + int fd;
> > +
> > + git_config(git_rerere_config);
> > + if (!is_rerere_enabled())
> > + return 0;
> > +
> > + merge_rr_path = xstrdup(git_path("rr-cache/MERGE_RR"));
> > + fd = hold_lock_file_for_update(&write_lock, merge_rr_path, 1);
> > + read_rr(&merge_rr);
> > + return do_plain_rerere(&merge_rr, fd);
> > +}
>
> Is it just me who sees a suboptimal cut and paste here?
>
> BTW, [1-5/7] look good so far.
>
> diff --git a/builtin-rerere.c b/builtin-rerere.c
> index 29d057c..2f51ae0 100644
> --- a/builtin-rerere.c
> +++ b/builtin-rerere.c
> @@ -415,18 +415,39 @@ static int is_rerere_enabled(void)
> return 1;
> }
>
> -int cmd_rerere(int argc, const char **argv, const char *prefix)
> +static int setup_rerere(struct path_list *merge_rr)
> {
> - struct path_list merge_rr = { NULL, 0, 0, 1 };
> - int i, fd = -1;
> + int fd;
>
> git_config(git_rerere_config);
> if (!is_rerere_enabled())
> - return 0;
> + return -1;
>
> merge_rr_path = xstrdup(git_path("rr-cache/MERGE_RR"));
> fd = hold_lock_file_for_update(&write_lock, merge_rr_path, 1);
> - read_rr(&merge_rr);
> + read_rr(merge_rr);
> + return fd;
> +}
> +
> +int rerere(void)
> +{
> + struct path_list merge_rr = { NULL, 0, 0, 1 };
> + int fd;
> +
> + fd = setup_rerere(&merge_rr);
> + if (fd < 0)
> + return 0;
> + return do_plain_rerere(&merge_rr, fd);
> +}
> +
> +int cmd_rerere(int argc, const char **argv, const char *prefix)
> +{
> + struct path_list merge_rr = { NULL, 0, 0, 1 };
> + int i, fd;
> +
> + fd = setup_rerere(&merge_rr);
> + if (fd < 0)
> + return 0;
>
> if (argc < 2)
> return do_plain_rerere(&merge_rr, fd);
That looks better, yes.
Kristian
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 7/7] Implement git commit as a builtin command.
2007-09-21 17:18 ` Kristian Høgsberg
@ 2007-09-21 19:32 ` Junio C Hamano
2007-09-24 20:27 ` Kristian Høgsberg
0 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2007-09-21 19:32 UTC (permalink / raw)
To: Kristian Høgsberg; +Cc: git
Kristian Høgsberg <krh@redhat.com> writes:
>> > +
>> > + /* update the user index file */
>> > + add_files_to_cache(fd, files, prefix);
>> > +
>> > + if (!initial_commit) {
>> > + tree = parse_tree_indirect(head_sha1);
>> > + if (!tree)
>> > + die("failed to unpack HEAD tree object");
>> > + if (read_tree(tree, 0, NULL))
>> > + die("failed to read HEAD tree object");
>> > + }
>>
>> Huh? Doesn't this read_tree() defeat the add_files_to_cache()
>> you did earlier?
>
> This is the case where we add the files on the command line
> to .git/index, but commit from a clean index file corresponding to HEAD
> with the files from the command line added (partial commit?). The first
> add_files_to_cache() updates .git/index, then we do read_tree() to build
> a tmp index from HEAD and then we add the files again. The tmp index is
> written to a tmp index file.
Still, if you are doing read_tree() that reads into the same
in-core cache you have just prepared in the add_fiels_to_cache()
above, potentially overwriting whatever you did, doesn't it?
That was what I was puzzled about...
> ... As for just using an in-memory
> index, I wanted to do it that way originally, but you have to write it
> to disk after all for the pre-commit hook.
Ah, I completely forgot about the hook. Ok, scratch the idea of
not using a temporary index file. The is not much potential for
performance gain anyway.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 7/7] Implement git commit as a builtin command.
2007-09-21 19:32 ` Junio C Hamano
@ 2007-09-24 20:27 ` Kristian Høgsberg
0 siblings, 0 replies; 18+ messages in thread
From: Kristian Høgsberg @ 2007-09-24 20:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Fri, 2007-09-21 at 12:32 -0700, Junio C Hamano wrote:
> Kristian Høgsberg <krh@redhat.com> writes:
>
> >> > +
> >> > + /* update the user index file */
> >> > + add_files_to_cache(fd, files, prefix);
> >> > +
> >> > + if (!initial_commit) {
> >> > + tree = parse_tree_indirect(head_sha1);
> >> > + if (!tree)
> >> > + die("failed to unpack HEAD tree object");
> >> > + if (read_tree(tree, 0, NULL))
> >> > + die("failed to read HEAD tree object");
> >> > + }
> >>
> >> Huh? Doesn't this read_tree() defeat the add_files_to_cache()
> >> you did earlier?
> >
> > This is the case where we add the files on the command line
> > to .git/index, but commit from a clean index file corresponding to HEAD
> > with the files from the command line added (partial commit?). The first
> > add_files_to_cache() updates .git/index, then we do read_tree() to build
> > a tmp index from HEAD and then we add the files again. The tmp index is
> > written to a tmp index file.
>
> Still, if you are doing read_tree() that reads into the same
> in-core cache you have just prepared in the add_fiels_to_cache()
> above, potentially overwriting whatever you did, doesn't it?
> That was what I was puzzled about...
Ah, I understand the confusion - add_files_to_cache() will write out the
cache to the given fd and close it. That's not clear, and I've moved
the write+close part back into prepare_index() in the follow-on patches
I sent that shares out add_files_to_cache() with builtin-add.c.
> > ... As for just using an in-memory
> > index, I wanted to do it that way originally, but you have to write it
> > to disk after all for the pre-commit hook.
>
> Ah, I completely forgot about the hook. Ok, scratch the idea of
> not using a temporary index file. The is not much potential for
> performance gain anyway.
Ok, cool, I'll keep the current structure of the code then.
Kristian
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2007-09-24 20:27 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-09-18 0:06 [PATCH 1/7] Enable wt-status output to a given FILE pointer Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 2/7] Enable wt-status to run against non-standard index file Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 3/7] Introduce entry point for launching add--interactive Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 4/7] Clean up stripspace a bit, use strbuf even more Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 5/7] Add strbuf_read_file() Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 6/7] Export rerere() and launch_editor() Kristian Høgsberg
2007-09-18 0:06 ` [PATCH 7/7] Implement git commit as a builtin command Kristian Høgsberg
2007-09-18 13:58 ` Johannes Schindelin
2007-09-18 15:07 ` Kristian Høgsberg
2007-09-20 1:27 ` Junio C Hamano
2007-09-21 17:18 ` Kristian Høgsberg
2007-09-21 19:32 ` Junio C Hamano
2007-09-24 20:27 ` Kristian Høgsberg
2007-09-18 13:14 ` [PATCH 6/7] Export rerere() and launch_editor() Johannes Schindelin
2007-09-19 23:52 ` Junio C Hamano
2007-09-21 18:01 ` Kristian Høgsberg
2007-09-18 13:12 ` [PATCH 4/7] Clean up stripspace a bit, use strbuf even more Johannes Schindelin
2007-09-18 13:52 ` Kristian Høgsberg
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).