Re: [RFC/PATCH 1/4] Add git-sequencer shell prototype

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 3, 2008, 22:11 UTC
Message-ID: <7vlk0iy5we.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080703210950.GC6677@leksak.fem-net>

Stephan Beyer <s-beyer@gmx.net> writes:

Show 7 quoted lines

> +		sed -e 's/[$"\\]/\\&/g' -n -e '
>  			s/^Author: \(.*\)$/GIT_AUTHOR_NAME="\1"/p;
>  			s/^Email: \(.*\)$/GIT_AUTHOR_EMAIL="\1"/p;
>  			s/^Date: \(.*\)$/GIT_AUTHOR_DATE="\1"/p
> ###
>
> Is escaping $, " and \ enough?

Look at how it is done in git-sh-setup get_author_ident_from_commit.

Show 23 quoted lines

>> > +	if test -n "$failed"
>> > +	then
>> > +		# XXX: This is just a stupid hack:
>> > +		with_author git apply $apply_opts --reject --index "$PATCH"
>> 
>> Please don't do this without being asked, if you are planning to use this
>> in "am" when 3-way fallback was not asked.  It _may_ make sense to give an
>> option to the users to ask for .rej if they prefer to work that way better
>> than working with 3-way merge fallback, but doing this without being asked
>> is not acceptable.
>
> The --reject was just a mind marker for that what I actually think is
> useful and less annoying than the current behavior:
>
>> > +		die_to_continue 'Patch failed. See the .rej files.'
>> > +		# XXX: We actually needed a git-apply flag that creates
>> > +		# conflict markers and sets the DIFF_STATUS_UNMERGED flag.
>> 
>> That is what -3way is all about, and this codepath is when the user did
>> not ask for it, isn't it?
>
> Now imagine you apply a patch that cannot be applied 100% cleanly and
> you don't have the 3-way base in the repo. You know what happens?

Do you think I don't? You can check who invented 3way by running "git log" or "git blame" on git-am.sh ;-)

I think you misread my "That is what -3way is all about". That remark is about the comment you have about "creates conflict markers". The conflict markers is only possible because we do 3-way merge when you ran "am -3". If you do not have the base object but only a blob and an unapplicable patch, you cannot do "here is our change since common ancestor, and here is their change the patch wants to make" conflict markers, because you do not have the common ancestor.

> Yes, the patch is completly rejected, because apply is atomic.
> And I think a git-apply option that results in a non-atomic behavior,
> that creates conflict markers (and no .rej files), would be a great
> usability feature for the "patch" insn in sequencer.

Yes, I think I already said in the message you are responding to that it may make sense to have such an option (but at the same time we should remember that nobody asked to add --reject to "git am").