Hi, On Thu, 5 Jun 2008, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes: > > > Sometimes, the easiest way to fix up a patch is to edit it directly, > > even adding or deleting lines. Now, many people are not as divine as > > certain benevolent dictators as to update the hunk headers correctly > > at the first try. > > > > So teach the tool to do it for us. > > Two comments and a half. > > * Latest POSIX draft talks about unified context and allows an empty line > to represent an empty common context line. GNU diff already emits such > a diff. fixup_counts() should take this into account. As you pointed out, I wanted to support only add -e. But that should not be an issue at all. I think a "case ' ': case '\n':" should be enough, right? > * I'd sleep better at night if 'Probably "diff ..."' part were written > in a bit more robust way. How about stopping on "@@" and end of file only, and complaining otherwise? > * (minor) There is an established term for this operation: recountdiff, > so --recount might be a better name. fixup_counts() also is better > called recount_diff() if we go this route. Fine! > If you are too narrowly focused to only support "git add -e", the first > issue does not matter, because we always emit "SP LF" for such a common > context. The reason why I care about the first two points is because we > may want to teach git-am about this new option as well in 1.6.0. Point taken. > And the robustness issue I worry about the second point also applies to > a line that is "^-- $", especially if we were to make this available to > git-am. Perhaps when the line begins with a '-', the logic could be > extra careful to detect the case where the line looks like the e-mail > signature separator and check one line beyond it to see if it does not > look anything like part of a diff (in which case you stop, without > considering the line you are currently looking at, "^-- $", a deletion > of "^- $", as part of the preimage context). Is this really an issue? fixup_counts() is only called after a hunk header was read, and that should be well after any "^-- $". > As to code structure, we might want to make the later parameters to > apply_patch() an integer, of OR'ed flag values, or even a pointer to a > structure that holds options. Right. Will fix up and resubmit. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
