"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
commit -F
To:
gameoftrees@openbsd.org
Date:
Thu, 28 Jan 2021 01:16:39 +0100

Download raw body.

Thread

  • Stefan Sperling:

    commit -F

I've always had concerns about adding 'got commit -F' as it exists in
other version control tools. Because this feature can easily lead to
accidents where a bogus log message ends up in history:

	got commit -F pf.c pf_if.c

	got commit -F *

I have seen this happen in real life, it is not a theoretical concern.
It happened in OpenBSD's CVS tree a couple of times where it was usually
fixed with cvsadmin. In distributed repositories it is impossible to
repair such mistakes once a bad commit has been pushed out.

The patch below adds commit -F, but with a twist: The prepared log message
is still shown in the editor. This gives the user a chance to double-check
the message and it can simply be saved if everything looks good.
If the message looks bad it can be fixed in the editor before a commit
is created.

Because commit -F can be useful for scripting there is a second option
which disables the editor: -N

So 'got commit -F' behaves reasonably safe, and 'got commit -N -F' behaves
like 'commit -F' behaves elsewhere.

If this is acceptable then I will apply the same approach to the 'got import'
and 'got tag' commands.

ok?


 add 'got commit -F' option to commit with a log message stored in a file

diff 1ffbfbd1e900b662cf076b63edb8ddbd2fe934a1 1c62d10a85f695ca53a78fb19943455eda275b30
blob - 5a69c331382003ee5c2d946293c4d1647cf56088
blob + 1991d4245ec041b192b623d07a0b5659e4757156
--- got/got.1
+++ got/got.1
@@ -1216,7 +1216,7 @@ is a directory.
 .It Cm rv
 Short alias for
 .Cm revert .
-.It Cm commit Oo Fl m Ar message Oc Oo Fl S Oc Op Ar path ...
+.It Cm commit Oo Fl F Ar path Oc Oo Fl m Ar message Oc Oo Fl N Oc Oo Fl S Oc Op Ar path ...
 Create a new commit in the repository from changes in a work tree
 and use this commit as the new base commit for the work tree.
 If no
@@ -1229,6 +1229,17 @@ If changes have been explicitly staged for commit with
 only commit staged changes and reject any specified paths which
 have not been staged.
 .Pp
+.Cm got commit
+opens a temporary file in an editor where a log message can be written
+unless the
+.Fl m
+option is used
+or the
+.Fl F
+and
+.Fl N
+options are used together.
+.Pp
 Show the status of each affected file, using the following status codes:
 .Bl -column YXZ description
 .It M Ta modified file
@@ -1269,13 +1280,28 @@ The options for
 .Cm got commit
 are as follows:
 .Bl -tag -width Ds
+.It Fl F Ar path
+Use the prepared log message stored in the file found at
+.Ar path
+when creating the new commit.
+.Cm got commit
+opens a temporary file in an editor where the prepared log message can be
+reviewed and edited further if needed.
+Cannot be used together with the
+.Fl m
+option.
 .It Fl m Ar message
 Use the specified log message when creating the new commit.
-Without the
-.Fl m
-option,
+Cannot be used together with the
+.Fl F
+option.
+.It Fl N
+This option prevents
 .Cm got commit
-opens a temporary file in an editor where a log message can be written.
+from opening the commit message in an editor.
+It has no effect unless it is used together with the
+.Fl F
+option and is intended for non-interactive use such as scripting.
 .It Fl S
 Allow the addition of symbolic links which point outside of the path space
 that is under version control.
@@ -2126,16 +2152,9 @@ with a pre-defined log message.
 .Pp
 Alternatively, create a new commit from local changes in a work tree
 directory with a log message that has been prepared in the file
-.Pa /tmp/msg .
-If
-.Xr vi 1
-is set as the
-.Ev EDITOR ,
-.Pa /tmp/msg
-can be read into the buffer for review:
+.Pa /tmp/msg:
 .Pp
-.Dl $ got commit
-.Dl :r /tmp/msg
+.Dl $ got commit -F /tmp/msg
 .Pp
 Update any work tree checked out from the
 .Dq unified-buffer-cache
blob - f1d690299d64d615136d348d8a119f0828400162
blob + 7a85d64a195dba5931737769c04f02796efa3258
--- got/got.c
+++ got/got.c
@@ -431,7 +431,8 @@ doneediting:
 
 static const struct got_error *
 edit_logmsg(char **logmsg, const char *editor, const char *logmsg_path,
-    const char *initial_content, size_t initial_content_len, int check_comments)
+    const char *initial_content, size_t initial_content_len,
+    int require_modification)
 {
 	const struct got_error *err = NULL;
 	char *line = NULL;
@@ -453,7 +454,8 @@ edit_logmsg(char **logmsg, const char *editor, const c
 	if (stat(logmsg_path, &st2) == -1)
 		return got_error_from_errno("stat");
 
-	if (st.st_mtime == st2.st_mtime && st.st_size == st2.st_size)
+	if (require_modification &&
+	    st.st_mtime == st2.st_mtime && st.st_size == st2.st_size)
 		return got_error_msg(GOT_ERR_COMMIT_MSG_EMPTY,
 		    "no changes made to commit message, aborting");
 
@@ -526,7 +528,8 @@ edit_logmsg(char **logmsg, const char *editor, const c
 		    "commit message cannot be empty, aborting");
 		goto done;
 	}
-	if (check_comments && strcmp(*logmsg, initial_content_stripped) == 0)
+	if (require_modification &&
+	    strcmp(*logmsg, initial_content_stripped) == 0)
 		err = got_error_msg(GOT_ERR_COMMIT_MSG_EMPTY,
 		    "no changes made to commit message, aborting");
 done:
@@ -6927,13 +6930,15 @@ done:
 __dead static void
 usage_commit(void)
 {
-	fprintf(stderr, "usage: %s commit [-m msg] [-S] [path ...]\n",
-	    getprogname());
+	fprintf(stderr, "usage: %s commit [-F path] [-m msg] [-N] [-S] "
+	    "[path ...]\n", getprogname());
 	exit(1);
 }
 
 struct collect_commit_logmsg_arg {
 	const char *cmdline_log;
+	const char *prepared_log;
+	int non_interactive;
 	const char *editor;
 	const char *worktree_path;
 	const char *branch_name;
@@ -6943,6 +6948,56 @@ struct collect_commit_logmsg_arg {
 };
 
 static const struct got_error *
+read_prepared_logmsg(char **logmsg, const char *path)
+{
+	const struct got_error *err = NULL;
+	FILE *f = NULL;
+	struct stat sb;
+	size_t r;
+
+	*logmsg = NULL;
+	memset(&sb, 0, sizeof(sb));
+
+	f = fopen(path, "r");
+	if (f == NULL)
+		return got_error_from_errno2("fopen", path);
+
+	if (fstat(fileno(f), &sb) == -1) {
+		err = got_error_from_errno2("fstat", path);
+		goto done;
+	}
+	if (sb.st_size == 0) {
+		err = got_error(GOT_ERR_COMMIT_MSG_EMPTY);
+		goto done;
+	}
+
+	*logmsg = malloc(sb.st_size + 1);
+	if (*logmsg == NULL) {
+		err = got_error_from_errno("malloc");
+		goto done;
+	}
+
+	r = fread(*logmsg, 1, sb.st_size, f);
+	if (r != sb.st_size) {
+		if (ferror(f))
+			err = got_error_from_errno2("fread", path);
+		else
+			err = got_error(GOT_ERR_IO);
+		goto done;
+	}
+	(*logmsg)[sb.st_size] = '\0';
+done:
+	if (fclose(f) == EOF && err == NULL)
+		err = got_error_from_errno2("fclose", path);
+	if (err) {
+		free(*logmsg);
+		*logmsg = NULL;
+	}
+	return err;
+	
+}
+
+static const struct got_error *
 collect_commit_logmsg(struct got_pathlist_head *commitable_paths, char **logmsg,
     void *arg)
 {
@@ -6963,20 +7018,36 @@ collect_commit_logmsg(struct got_pathlist_head *commit
 			return got_error_from_errno("malloc");
 		strlcpy(*logmsg, a->cmdline_log, len);
 		return NULL;
-	}
+	} else if (a->prepared_log != NULL && a->non_interactive)
+		return read_prepared_logmsg(logmsg, a->prepared_log);
 
 	if (asprintf(&template, "%s/logmsg", a->worktree_path) == -1)
 		return got_error_from_errno("asprintf");
 
+	err = got_opentemp_named_fd(&a->logmsg_path, &fd, template);
+	if (err)
+		goto done;
+
+	if (a->prepared_log) {
+		char *msg;
+		err = read_prepared_logmsg(&msg, a->prepared_log);
+		if (err)
+			goto done;
+		if (write(fd, msg, strlen(msg)) == -1) {
+			err = got_error_from_errno2("write", a->logmsg_path);
+			free(msg);
+			goto done;
+		}
+		free(msg);
+	}
+
 	initial_content_len = asprintf(&initial_content,
 	    "\n# changes to be committed on branch %s:\n",
 	    a->branch_name);
-	if (initial_content_len == -1)
-		return got_error_from_errno("asprintf");
-
-	err = got_opentemp_named_fd(&a->logmsg_path, &fd, template);
-	if (err)
+	if (initial_content_len == -1) {
+		err = got_error_from_errno("asprintf");
 		goto done;
+	}
 
 	if (write(fd, initial_content, initial_content_len) == -1) {
 		err = got_error_from_errno2("write", a->logmsg_path);
@@ -6991,7 +7062,7 @@ collect_commit_logmsg(struct got_pathlist_head *commit
 	}
 
 	err = edit_logmsg(logmsg, a->editor, a->logmsg_path, initial_content,
-	    initial_content_len, 1);
+	    initial_content_len, a->prepared_log ? 0 : 1);
 done:
 	free(initial_content);
 	free(template);
@@ -7018,20 +7089,34 @@ cmd_commit(int argc, char *argv[])
 	char *cwd = NULL, *id_str = NULL;
 	struct got_object_id *id = NULL;
 	const char *logmsg = NULL;
+	char *prepared_logmsg = NULL;
 	struct collect_commit_logmsg_arg cl_arg;
 	char *gitconfig_path = NULL, *editor = NULL, *author = NULL;
 	int ch, rebase_in_progress, histedit_in_progress, preserve_logmsg = 0;
-	int allow_bad_symlinks = 0;
+	int allow_bad_symlinks = 0, non_interactive = 0;
 	struct got_pathlist_head paths;
 
 	TAILQ_INIT(&paths);
 	cl_arg.logmsg_path = NULL;
 
-	while ((ch = getopt(argc, argv, "m:S")) != -1) {
+	while ((ch = getopt(argc, argv, "F:m:NS")) != -1) {
 		switch (ch) {
+		case 'F':
+			if (logmsg != NULL)
+				option_conflict('F', 'm');
+			prepared_logmsg = realpath(optarg, NULL);
+			if (prepared_logmsg == NULL)
+				return got_error_from_errno2("realpath",
+				    optarg);
+			break;
 		case 'm':
+			if (prepared_logmsg)
+				option_conflict('m', 'F');
 			logmsg = optarg;
 			break;
+		case 'N':
+			non_interactive = 1;
+			break;
 		case 'S':
 			allow_bad_symlinks = 1;
 			break;
@@ -7104,6 +7189,8 @@ cmd_commit(int argc, char *argv[])
 
 	cl_arg.editor = editor;
 	cl_arg.cmdline_log = logmsg;
+	cl_arg.prepared_log = prepared_logmsg;
+	cl_arg.non_interactive = non_interactive;
 	cl_arg.worktree_path = got_worktree_get_root_path(worktree);
 	cl_arg.branch_name = got_worktree_get_head_ref_name(worktree);
 	if (!histedit_in_progress) {
@@ -7145,6 +7232,7 @@ done:
 	free(gitconfig_path);
 	free(editor);
 	free(author);
+	free(prepared_logmsg);
 	return error;
 }
 
blob - 1e055a9b1f81d1721ec308592989b045d4f027c9
blob + 3956dc0289b56e9ce806eab6702d0522b243de79
--- regress/cmdline/commit.sh
+++ regress/cmdline/commit.sh
@@ -1357,6 +1357,106 @@ test_commit_fix_bad_symlink() {
 	test_done "$testroot" "0"
 }
 
+test_commit_prepared_logmsg() {
+	local testroot=`test_init commit_prepared_logmsg`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "modified alpha" > $testroot/wt/alpha
+	(cd $testroot/wt && got rm beta >/dev/null)
+	echo "new file" > $testroot/wt/new
+	(cd $testroot/wt && got add new >/dev/null)
+
+	echo 'test commit_prepared_logmsg' > $testroot/logmsg
+
+	cat > $testroot/editor.sh <<EOF
+#!/bin/sh
+sed -i 's/foo/bar/' "\$1"
+EOF
+	chmod +x $testroot/editor.sh
+
+	(cd $testroot/wt && env EDITOR="$testroot/editor.sh" \
+		got commit -F "$testroot/logmsg" > $testroot/stdout)
+
+	local head_rev=`git_show_head $testroot/repo`
+	echo "A  new" > $testroot/stdout.expected
+	echo "M  alpha" >> $testroot/stdout.expected
+	echo "D  beta" >> $testroot/stdout.expected
+	echo "Created commit $head_rev" >> $testroot/stdout.expected
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	local author_time=`git_show_author_time $testroot/repo`
+	d=`env TZ=UTC date -r $author_time +"%a %b %e %X %Y UTC"`
+	echo "-----------------------------------------------" > $testroot/stdout.expected
+	echo "commit $head_rev (master)" >> $testroot/stdout.expected
+	echo "from: $GOT_AUTHOR" >> $testroot/stdout.expected
+	echo "date: $d" >> $testroot/stdout.expected
+	echo " " >> $testroot/stdout.expected
+	echo " test commit_prepared_logmsg" >> $testroot/stdout.expected
+	echo " " >> $testroot/stdout.expected
+
+	(cd $testroot/wt && got log -l 1 > $testroot/stdout)
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "modified alpha again" > $testroot/wt/alpha
+
+	echo 'test commit_prepared_logmsg non-interactive' \
+		> $testroot/logmsg
+
+	(cd $testroot/wt && got commit -N -F "$testroot/logmsg" \
+		> $testroot/stdout)
+
+	local head_rev=`git_show_head $testroot/repo`
+	echo "M  alpha" > $testroot/stdout.expected
+	echo "Created commit $head_rev" >> $testroot/stdout.expected
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	local author_time=`git_show_author_time $testroot/repo`
+	d=`env TZ=UTC date -r $author_time +"%a %b %e %X %Y UTC"`
+	echo "-----------------------------------------------" \
+		> $testroot/stdout.expected
+	echo "commit $head_rev (master)" >> $testroot/stdout.expected
+	echo "from: $GOT_AUTHOR" >> $testroot/stdout.expected
+	echo "date: $d" >> $testroot/stdout.expected
+	echo " " >> $testroot/stdout.expected
+	echo " test commit_prepared_logmsg non-interactive" \
+		>> $testroot/stdout.expected
+	echo " " >> $testroot/stdout.expected
+
+	(cd $testroot/wt && got log -l 1 > $testroot/stdout)
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+	test_done "$testroot" "$ret"
+}
+
 test_parseargs "$@"
 run_test test_commit_basic
 run_test test_commit_new_subdir
@@ -1382,3 +1482,4 @@ run_test test_commit_normalizes_filemodes
 run_test test_commit_with_unrelated_submodule
 run_test test_commit_symlink
 run_test test_commit_fix_bad_symlink
+run_test test_commit_prepared_logmsg