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

[Junio C Hamano <gitster@pobox.com>]

Lucien Kong <Lucien.Kong@ensimag.imag.fr> 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.