From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: Christian Couder <chriscool@tuxfamily•org>,
git@vger•kernel.org, Joey Hess <joey@kitenet•net>
Subject: Re: [PATCH 0/5] teach replace objects to sha1_object_info_extended()
Date: Mon, 02 Dec 2013 15:41:47 -0800 [thread overview]
Message-ID: <xmqqtxeqq0qs.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20131202150634.GA20416@sigill.intra.peff.net> (Jeff King's message of "Mon, 2 Dec 2013 10:06:34 -0500")
Jeff King <peff@peff•net> writes:
> On Mon, Dec 02, 2013 at 09:52:25AM -0500, Jeff King wrote:
>
>> I find it a little funny that we reuse the READ_SHA1_FILE_REPLACE flag
>> directly in lookup_replace_object. That means that it is now a
>> meaningful flag for sha1_object_info_extended, even though the name does
>> not say so. It also means that the two may have to coordinate further
>> flags (since a portion of their flag namespace is shared by
>> lookup_replace_object). I don't foresee adding a lot of new flags,
>> though, so it probably isn't a huge deal.
>>
>> I also would have expected sha1_object_info_extended to simply receive
>> the new flag via the struct object_info. Again, probably not a big deal,
>> because there aren't many callsites that needed updating. But if we were
>> not sharing flags with read_sha1_file, I think doing it as a flag in the
>> struct would be nicer.
>
> Curious what this would look like, I wrote the patch. If you drop your
> patches 2 and 3, then your final patch (actually fixing the problem)
> would look like the one below:
>
> We may be getting into bikeshed territory, and I don't feel
> super-strongly about it, so I won't say anything more. Now we've seen
> both alternatives, and you or Junio can pick. :)
FWIW, I shared that "a little funny" feeling ;-)
>
> ---
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index b2ca775..a2e3e26 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -270,6 +270,7 @@ static int batch_objects(struct batch_options *opt)
> * object.
> */
> memset(&data, 0, sizeof(data));
> + data.info.respect_replace = 1;
> data.mark_query = 1;
> strbuf_expand(&buf, opt->format, expand_format, &data);
> data.mark_query = 0;
> diff --git a/cache.h b/cache.h
> index ce377e1..0ad262f 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1074,6 +1074,7 @@ struct object_info {
> enum object_type *typep;
> unsigned long *sizep;
> unsigned long *disk_sizep;
> + unsigned respect_replace:1;
>
> /* Response */
> enum {
> diff --git a/sha1_file.c b/sha1_file.c
> index 7dadd04..b6ddad0 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2519,8 +2519,11 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
> struct cached_object *co;
> struct pack_entry e;
> int rtype;
> + const unsigned char *real = oi->respect_replace ?
> + lookup_replace_object(sha1) :
> + sha1;
>
> - co = find_cached_object(sha1);
> + co = find_cached_object(real);
> if (co) {
> if (oi->typep)
> *(oi->typep) = co->type;
> @@ -2532,16 +2535,16 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi)
> return 0;
> }
>
> - if (!find_pack_entry(sha1, &e)) {
> + if (!find_pack_entry(real, &e)) {
> /* Most likely it's a loose object. */
> - if (!sha1_loose_object_info(sha1, oi)) {
> + if (!sha1_loose_object_info(real, oi)) {
> oi->whence = OI_LOOSE;
> return 0;
> }
>
> /* Not a loose object; someone else may have just packed it. */
> reprepare_packed_git();
> - if (!find_pack_entry(sha1, &e))
> + if (!find_pack_entry(real, &e))
> return -1;
> }
>
> @@ -2570,6 +2573,7 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
>
> oi.typep = &type;
> oi.sizep = sizep;
> + oi.respect_replace = 1;
> if (sha1_object_info_extended(sha1, &oi) < 0)
> return -1;
> return type;
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index b90dbdc..bb785ec 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -276,7 +276,7 @@ test_expect_success '-f option bypasses the type check' '
> git replace -f HEAD^ $BLOB
> '
>
> -test_expect_failure 'git cat-file --batch works on replace objects' '
> +test_expect_success 'git cat-file --batch works on replace objects' '
> git replace | grep $PARA3 &&
> echo $PARA3 | git cat-file --batch
> '
next prev parent reply other threads:[~2013-12-02 23:41 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-30 13:51 [PATCH 0/5] teach replace objects to sha1_object_info_extended() Christian Couder
2013-11-30 13:51 ` [PATCH 1/5] replace_object: don't check read_replace_refs twice Christian Couder
2013-11-30 13:51 ` [PATCH 2/5] introduce lookup_replace_object_extended() to pass flags Christian Couder
2013-11-30 13:51 ` [PATCH 3/5] Add an "unsigned flags" parameter to sha1_object_info_extended() Christian Couder
2013-11-30 13:51 ` [PATCH 4/5] t6050: show that git cat-file --batch fails with replace objects Christian Couder
2013-11-30 13:51 ` [PATCH 5/5] sha1_file: perform object replacement in sha1_object_info_extended() Christian Couder
2013-12-02 14:52 ` [PATCH 0/5] teach replace objects to sha1_object_info_extended() Jeff King
2013-12-02 15:06 ` Jeff King
2013-12-02 23:41 ` Junio C Hamano [this message]
2013-12-03 20:42 ` Christian Couder
2013-12-03 20:46 ` Christian Couder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqtxeqq0qs.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=chriscool@tuxfamily$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=joey@kitenet$(echo .)net \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox