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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 5, 2012, 21:16 UTC
Message-ID: <7vfwa9o3xo.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1338927687-29822-4-git-send-email-Lucien.Kong@ensimag.imag.fr>

Lucien Kong <Lucien.Kong@ensimag.imag.fr> writes:

> +static int split_commit_in_progress()

static int split_commit_in_progress(void)

Show 50 quoted lines

> +{
> +	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.