[MERGE][0.15] Handle empty merge directive texts

John Arbash Meinel john at arbash-meinel.com
Mon Mar 26 16:55:49 BST 2007


Aaron Bentley wrote:
>> The patch is fine.
> 
> Please don't use +1 conditional when you think the patch is fine.
> Conditional means that there is something I must change in order to have
> your +1.

To me it means "+1, but I have a comment, though if you agree and make
the change, it doesn't need another review".

So (conditional) is there is something to discuss. +1 is "merge this as
soon as you see the +1.".

I can use +1 for "I agree, but there is something I want to discuss".
But I always felt like that was "merge it".

> 
>> Though I wonder if we want to pass the whole set of lines, rather than just the first one.
>> Considering you now will search through the entire file for a merge directive header, it seems certainly possible that this would not help a user find the problem.
> 
> I certainly don't want to see a mess-o-lines on the commandline when a
> merge directive fails.  I think in the vast majority of cases, spewing
> entire patches at users will be unhelpfully overwhelming.  It's like
> drinking from a firehose.
> 
> The first line gives a pretty clear idea of what they just tried to
> merge.  If they want to look at the whole thing, they would probably
> prefer to do so in a pager or editor.
> 
> Aaron

True enough. Though we don't give them a way to review it in a pager or
editor if they are using it from remote....

Anyway, I'm fine with a single line, I was just pointing out that the
line could just as easily have no information (the first line is blank).
But I agree it is probably better than seeing a screenful each time.

(What I would really rather prefer is a check for something that looks
like a merge directive, but maybe is newer/older than what we expect)

John
=:->



More information about the bazaar mailing list