Re: [PATCH v6 02/10] replace: add --graft option

From: Christian Couder <christian.couder@gmail.com>
Date: Jul 11, 2014, 08:59 UTC
Message-ID: <CAP8UFD0_m5aFVcBQr3d9pXR=9rLjAVPGrj=UsBYFcnTQFwNKGA@mail.gmail.com>
In-Reply-To: <xmqqegxt86ba.fsf@gitster.dls.corp.google.com>

On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 9 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>>>
>>> As the user might expect that a new replace ref was created on success
>>> (0 exit code), and as we should at least warn if we would create a
>>> commit that is the same as an existing one,...
>>
>> Why is it an event that needs a warning?  I do not buy that "as we
>> should at least" at all.

Here you ask why this event needs a warning...

Show 9 quoted lines

> Ehh, it came a bit differently from what I meant.  Perhaps s/do not
> buy/do not understand/ is closer to what I think---that is, it is
> not like I with a strong conviction think you are wrong.  It is more
> like I do not understand why you think it needs a warning, meaing
> you would need to explain it better.
>
>> If you say "make sure A's parent is B" and then you asked the same
>> thing again when there already is a replacement in place, that
>> should be a no-op.

(When there is already a replacement in place we error out in replace_object_sha1() unless --force is used. What we are talking about here is what happens if the replacement commit is the same as the original commit.)

>> "Making sure A's parent is B" would be an
>> idempotent operation, no?  Why not just make sure A's parent is
>> already B and report "Your wish has been granted" to the user?

... and here you say we should report "your wish has been granted"...

>> Why would it be simpler for the user to get an error, inspect the
>> situation and realize that his wish has been granted after all?

... but for me reporting to the user "your wish has been granted" and warning (or errorring out) saying "the new commit would be the same as the old one" are nearly the same thing.

So I wonder what exactly you are not happy with.

Is it the fact that I use the error() function, because it would prefix the message with "fatal:" and that would be too scary?

Is it because with error() the exit code would not be 0?

Is it because the message "new commit is the same as the old one: '%s'" is too scary or unnecessarily technical by itself?

Is it ok if I just print the message on stderr without "warning:" or "fatal:" in front of it?

By the way, when I say "As ... and ..., I think it is just simpler to error out in this case.", I mean that it is simpler from the developer's point of view, not necessarily for the user.

Of course I am ok with improving things for the user even if it requires more code/work, but it is difficult to properly do that if I don't see how I could do it.

Thanks, Christian.