Re: [PATCH v3 6/9] rebase -i: check for missing commits in the rebase--helper

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> -check_todo_list
> +git rebase--helper --check-todo-list || {
> +	ret=$?
> +	checkout_onto
> +	exit $ret
> +}

I find this a better division of labor between "check_todo_list" and
its caller.  Compared to the original that did the "recover and exit
with failure" inside the helper, this is much easier to see what is
going on.

> +/*
> + * Check if the user dropped some commits by mistake
> + * Behaviour determined by rebase.missingCommitsCheck.
> + * Check if there is an unrecognized command or a
> + * bad SHA-1 in a command.
> + */
> +int check_todo_list(void)
> +{
> +	enum check_level check_level = get_missing_commit_check_level();
> +	struct strbuf todo_file = STRBUF_INIT;
> +	struct todo_list todo_list = TODO_LIST_INIT;
> +	struct commit_list *missing = NULL;
> +	int raise_error = 0, res = 0, fd, i;
> +
> +	strbuf_addstr(&todo_file, rebase_path_todo());
> +	fd = open(todo_file.buf, O_RDONLY);
> +	if (fd < 0) {
> +		res = error_errno(_("could not open '%s'"), todo_file.buf);
> +		goto leave_check;
> +	}
> +	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> +		close(fd);
> +		res = error(_("could not read '%s'."), todo_file.buf);
> +		goto leave_check;
> +	}
> +	close(fd);
> +	raise_error = res =
> +		parse_insn_buffer(todo_list.buf.buf, &todo_list);
> +
> +	if (check_level == CHECK_IGNORE)
> +		goto leave_check;

OK, so even it is set to ignore, unreadable todo list will be shown
with a loud error message that tells the user to use --edit-todo.

What should happen when it is not set to ignore and we found the
todo list unacceptable, I wonder?  Let's read on.

> +	/* Get the SHA-1 of the commits */
> +	for (i = 0; i < todo_list.nr; i++) {
> +		struct commit *commit = todo_list.items[i].commit;
> +		if (commit)
> +			commit->util = todo_list.items + i;
> +	}

It does not look like this loop is "Get(ting) the SHA-1 of the commits"
to me, though.  It is setting up ->util to be usable as a back-pointer
into the list.

> +
> +	todo_list_release(&todo_list);

But then the todo-list is released?  The util field we have set, if
any, in the previous loop are now dangling, no?

> +	strbuf_addstr(&todo_file, ".backup");
> +	fd = open(todo_file.buf, O_RDONLY);
> +	if (fd < 0) {
> +		res = error_errno(_("could not open '%s'"), todo_file.buf);
> +		goto leave_check;
> +	}
> +	if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> +		close(fd);
> +		res = error(_("could not read '%s'."), todo_file.buf);
> +		goto leave_check;
> +	}
> +	close(fd);
> +	strbuf_release(&todo_file);
> +	res = !!parse_insn_buffer(todo_list.buf.buf, &todo_list);

Then we read from .backup; failure to do so does not result in the
"you need to --edit-todo" warning.

> +	/* Find commits that are missing after editing */
> +	for (i = 0; i < todo_list.nr; i++) {
> +		struct commit *commit = todo_list.items[i].commit;
> +		if (commit && !commit->util) {
> +			commit_list_insert(commit, &missing);
> +			commit->util = todo_list.items + i;
> +		}
> +	}

And we check the commits mentioned in the backup; any commit whose
util is not marked in the previous loop is noticed and thrown into
the missing list.

The loop we later have does "while (missing)" and does not look at
commit->util for commits that are *not* missing, i.e. the ones that
are marked in the previous loop, so it does not matter that their
util field have dangling pointers.  In that sense, it may not be
buggy, but it is misleading.  The only thing these two loops care
about is that the commits found in the earlier loop get their util
field set to non-NULL, so instead of using "todo_list.items+i",
perhaps doing this

	if (commit)
		commit->util = (void *)1; /* mark as seen */

in the earlier loop instead would be much less confusing.

> +	/* Warn about missing commits */
> +	if (!missing)
> +		goto leave_check;

If there is no missing one, we may still return error about
unacceptable backup file.  But if we read backup fine and didn't
find anything missing, we'll return silently and with success.  OK.

> +	if (check_level == CHECK_ERROR)
> +		raise_error = res = 1;

Otherwise, we found missing ones and we want to report here.

The reason why I started reading this function aloud was because I
found two variables (raise_error and res) somewhat confusing.  I
think what the code does makes sense, but I still find the way how
the code expresses the logic with these two variables confusing.
Perhaps somebody else can hopefully offer possible improvements, as
I do not offhand think of a way better than what is currently in
this patch myself.

> +	fprintf(stderr,
> +		_("Warning: some commits may have been dropped accidentally.\n"
> +		"Dropped commits (newer to older):\n"));
> +
> +	/* Make the list user-friendly and display */
> +	while (missing) {
> +		struct commit *commit = pop_commit(&missing);
> +		struct todo_item *item = commit->util;
> +
> +		fprintf(stderr, " - %s %.*s\n", short_commit_name(commit),
> +			item->arg_len, item->arg);
> +	}
> +	free_commit_list(missing);
> +
> +	fprintf(stderr, _("To avoid this message, use \"drop\" to "
> +		"explicitly remove a commit.\n\n"
> +		"Use 'git config rebase.missingCommitsCheck' to change "
> +		"the level of warnings.\n"
> +		"The possible behaviours are: ignore, warn, error.\n\n"));
> +
> +leave_check:
> +	strbuf_release(&todo_file);
> +	todo_list_release(&todo_list);
> +
> +	if (raise_error)
> +		fprintf(stderr,
> +			_("You can fix this with 'git rebase --edit-todo' "
> +			  "and then run 'git rebase --continue'.\n"
> +			  "Or you can abort the rebase with 'git rebase"
> +			  " --abort'.\n"));
> +
> +	return res;
> +}
> diff --git a/sequencer.h b/sequencer.h
> index 47a81034e76..4978a61b83b 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -49,6 +49,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
>  		int argc, const char **argv);
>  
>  int transform_todo_ids(int shorten_sha1s);
> +int check_todo_list(void);
>  
>  extern const char sign_off_header[];