Re: [PATCHv8 4/4] status: better advices when splitting a commit (during rebase -i)

Lucien Kong [off-list ref] writes:

+static int split_commit_in_progress()

static int split_commit_in_progress(void)

+{
+	FILE *head;
+	struct strbuf buf_head;
+	FILE *orig_head;
+	struct strbuf buf_orig_head;
+	FILE *rebase_amend;
+	struct strbuf buf_rebase_amend;
+	FILE *rebase_orig_head;
+	struct strbuf buf_rebase_orig_head;
+	int split_in_progress = 0;
+
+	head = fopen(git_path("HEAD"), "r");
+	if (!head)
+		return 0;
+
+	orig_head = fopen(git_path("ORIG_HEAD"), "r");
+	if (!orig_head) {
+		fclose(head);
+		return 0;
+	}
+
+	rebase_amend = fopen(git_path("rebase-merge/amend"), "r");
+	if (!rebase_amend) {
+		fclose(head);
+		fclose(orig_head);
+		return 0;
+	}
+
+	rebase_orig_head = fopen(git_path("rebase-merge/orig-head"), "r");
+	if (!rebase_orig_head) {
+		fclose(head);
+		fclose(orig_head);
+		fclose(rebase_amend);
+		return 0;
+	}
+
+	strbuf_init(&buf_head, 0);
+	strbuf_init(&buf_orig_head, 0);
+	strbuf_init(&buf_rebase_amend, 0);
+	strbuf_init(&buf_rebase_orig_head, 0);
+
+	strbuf_getline(&buf_head, head, '\n');
+	strbuf_getline(&buf_orig_head, orig_head, '\n');
+	strbuf_getline(&buf_rebase_amend, rebase_amend, '\n');
+	strbuf_getline(&buf_rebase_orig_head, rebase_orig_head, '\n');
+
+	fclose(head);
+	fclose(orig_head);
+	fclose(rebase_amend);
+	fclose(rebase_orig_head);

That is somewhat an ugly sequence; wouldn't it make sense to have a
small helper that takes a path in git_path() and returns a pointer
to an allocated string, i.e. something like...

	static char *read_line_from_git_path(const char *filename)
        {
        	struct struct buf = STRBUF_INIT;
                FILE *fp = fopen(git_path(filename), "r");
                if (!fp)
                	return NULL;
		strbuf_getline(&buf, fp, '\n');
		fclose(fp);
                return strbuf_detach(&buf, NULL);
	}

nb. return value from fclose() may need to be be checked as well,
but this is just an illustration.

Reading from git_path("HEAD") looked funny, as you may end up
reading the "ref: refs/heads/master".  Of course that would not
compare equal to what you would read from "rebase-merge/amend", and
that may be fine for the purpose of your test, but it still looks
somewhat funny.  As modern rebase is done on a detached HEAD,
perhaps it is a good idea to check if the HEAD is detached and
return false from this function if that is not the case.  I dunno.

Other than that, all four patches in the series looked good.  Will
replace what I queued on 'pu' last night.

Thanks.

Junio C Hamano [off-list ref] a écrit :

Reading from git_path("HEAD") looked funny, as you may end up
reading the "ref: refs/heads/master".  Of course that would not
compare equal to what you would read from "rebase-merge/amend", and
that may be fine for the purpose of your test, but it still looks
somewhat funny.  As modern rebase is done on a detached HEAD,
perhaps it is a good idea to check if the HEAD is detached and
return false from this function if that is not the case.  I dunno.

On second thoughts, I do not think that checking if HEAD is detached
or not is needed, as the part of the code that includes reading can
only be called during a rebase interactive, in which case the HEAD
can only be detached.