Re: [PATCH 3/6] revert: Parse instruction sheet more cautiously

Ramkumar Ramachandra wrote:

Fix a buffer overflow bug by checking that parsed SHA-1 hex will fit
in the buffer we've created for it.

Nit: it seems best to either describe the behavior or the code change.
So, for example:

	Do not overflow a buffer when the second word in a "pick"
	or "revert" line in .git/sequencer/todo is very long.

Or:

	Check that the commit name argument to a "pick" or "revert"
	action is not too long, to avoid overflowing an on-stack
	buffer.

It would also be comforting to readers to mention when the bug was
introduced, so they don't start worrying about protecting their
installations against privilege escalation:

	This fixes a regression introduced by <...>, which has not
	escaped into the wild yet, luckily.

Could we have a test for this?

Also change the instruction sheet format

How is this "Also" part related to the earlier bit?  Does one make the
other easier, or is it more of a "While we're changing this code" kind
of thing?

subtly so that a description of the commit after the object
name is optional.  So now, an instruction sheet like this is perfectly
valid:

  pick 35b0426
  pick fbd5bbcbc2e
  pick 7362160f

Sounds convenient. :)  Thanks.

By the way, why are these non-const?  (Not about this patch.)

I don't know why, but I would be more comfortable reading something
like this:

	const char *p, *q;

	p = start;
	if (!prefixcmp(p, "pick ")) {
		action = CHERRY_PICK;
		p += strlen("pick ");
	} else if (...) {
		...
	} ...

	q = p + strcspn(p, " \n");
	if (q - p + 1 > sizeof(...))
		...
	...

Maybe because I can imagine how the pointers move, always forward and
never past the end of the line.

What assertion does this test check?  "commit descriptions in insn
sheet are optional"?

+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&

A failed cherry-pick.

+	echo "c" >foo &&
+	git add foo &&
+	git commit &&

Resolving the conflict.

+	cut -d" " -f1,2 .git/sequencer/todo >new_sheet &&
+	cp new_sheet .git/sequencer/todo &&

Mucking about with the insn sheet.

+	git cherry-pick --continue &&

Continuing.

+	test_path_is_missing .git/sequencer &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	cat >expect <<-\EOF &&
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	OBJID
+	:100644 100644 OBJID OBJID M	unrelated
+	OBJID
+	:000000 100644 OBJID OBJID A	foo
+	:000000 100644 OBJID OBJID A	unrelated
+	EOF
+	test_cmp expect actual

Checking that the cherry-pick succeeded and the resulting list of
commits.  How is this expected to potentially fail?  Maybe something
like

	git rev-list HEAD >commits &&
	test_line_count = 4 commits

or

	git diff --exit-code <something>

would make what this is intended to check clearer.  As hinted above,
some blank lines or comments might make the earlier part easier to
read.

Thanks, this patch does two good things.  For what it's worth, with
the changes hinted at above and a split into two patches if that seems
sensible,
Acked-by: Jonathan Nieder <redacted>