Re: [PATCH v2] contrib/subtree bugfix: Can't `add` annotated tag

From: Junio C Hamano <gitster@pobox.com>
Date: May 14, 2014, 21:40 UTC
Message-ID: <xmqqtx8shwel.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140514213206.GA12228@debian>

James Denholm <nod.helm@gmail.com> writes:

Show 17 quoted lines

> On Tue, May 13, 2014 at 04:12:56PM -0700, Junio C Hamano wrote:
>> James Denholm <nod.helm@gmail.com> writes:
>> 
>> > I'm not sure that can actually happen - peel_committish is essentially
>> > implemented as `rev-parse $arg^0` (though with a bit of bling, of
>> > course), and to my understanding FETCH_HEAD will always parse to a
>> > committish - I could have missed something, of course.
>> 
>>  $ git fetch git://repo.or.cz/alt-git junio-gpg-pub
>>  $ git rev-parse FETCH_HEAD^0
>
> That would be a problem... Sadly I doubt I'll have time to develop a
> solution into subtree's overall design before the end of June. As that
> eventual change would probably involve altering the inclusions of this
> fix, and that users have a workaround in adding either squashed commits
> or referencing lightweight tags, would you rather drop the patch and
> wait for that?

Sorry, I am lost.  What would be a problem exactly?

A FETCH_HEAD can be pointing at an object that is not committish, and users involved, both at the originating end who controls the repository you fetched from and at the receiving end who wanted to fetch the object, are *not* expeting to be able to make a merge of such an object anyway. My suggestion was not to ask you to come up with a sane behaviour when the user told us to add a single blob with "subtree add"; it was merely to detect such unintended use as an error.

To me, it looks like all that is necessary is to accept your patch but with a three-byte tightening to detect such a pathological case and signal an error, which is what " &&", which I added to your new line that sets revp=$(peel_committish ...), is about.

This patch, with or without these extra " &&" three bytes, will not be part of the upcoming 2.0 release anyway, so we have enough time to iron it out.