Re* [RFC/PATCH] extend meaning of "--root" option to index comparisons
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 21, 2008, 18:48 UTC
- Message-ID
- <7v63opz66t.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <20080921135616.GA25238@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> On Fri, Sep 19, 2008 at 01:27:46PM -0700, Junio C Hamano wrote: > ... >> (2) "--root" is about "do we show a creation event as a huge diff from >> emptyness?". Yes, we turn it on for "git log" but it does not have >> anything to do with the issue of yet to be born branch, where there >> isn't even a big creation event yet. > > What about index comparisons? What should an index comparison to a > branch yet-to-be-born look like? Right now it is an error.
It should be an error, because that is _not_ even an comparison. At least at diff-index level.
The diff wrapper UI could do something different, though. And an obvious thing to do is to give a fake creation event.
The current output feels perfectly sensible to me.
$ mkdir d; cd d; tar xf .../t.tar; git init; git add .
$ git diff --cached
fatal: No HEAD commit to compare with (yet)The alternative is no different from "find . -type f | xargs cat" from the point of view of reviewability. To make sure you have what you want in your initial revision, so that you can get things right from the start, you would want to check things like:
$ tar df .../t.tar ;# did I change anything?
$ git ls-files ;# do I have what I want?
$ git clean -n ;# have I missed anything?By allowing an auto-fallback to the comparison with an empty tree object, you are giving these possibilities:
$ git diff --cached --stat $ git diff --cached --name-only
but the latter is already available from ls-files anyway, and the former does not feel so interesting.
In exchange, we lose the reminder to the user that this is a creation event. An interactive user (remember, I am not talking about diff-index here, but diff front-end) may want to treat it specially perhaps by being extra careful. If there were no downsides like this in "fall back to comparing with an empty tree" approach, I wouldn't hesitate to agree it is a good idea, though.
Show 6 quoted lines
>> I am reluctant to agree with the opinion that "git log" should be _silent_ >> in a world without any history. > > It feels a bit more Unix-y to me. That is, if I am asking for some set > of commits, and there are _no_ commits in the set, then I expect no > output.
To this, I am inclined to agree. We could do something like the attached patch, but there is a caveat.
There may be commands that
(1) cannot sanely operate without any positive ref;
(2) give default "HEAD";
(3) do not have its own input verification to make sure there is at least
one positive ref, because they have been relying on revision
machinery to die() with the existing check.and this patch is actively breaking them. If people like this approach (and I probably will join them), commands that match the above criteria need to be identified and fixed by setting revs->require_valid_def.
revision.c | 22 ++++++++++++++++++---- revision.h | 5 ++++- 2 files changed, 22 insertions(+), 5 deletions(-)
diff --git i/revision.c w/revision.c index 2f646de..5ce7795 100644 --- i/revision.c +++ w/revision.c @@ -1295,12 +1295,26 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch prepare_show_merge(revs); if (revs->def && !revs->pending.nr) { unsigned char sha1[20]; - struct object *object; unsigned mode; - if (get_sha1_with_mode(revs->def, sha1, &mode)) + int flag; + if (!get_sha1_with_mode(revs->def, sha1, &mode)) { + struct object *object; + object = get_reference(revs, revs->def, sha1, 0); + add_pending_object_with_mode(revs, object, revs->def, mode); + } else if (!revs->require_valid_def && + !strcmp(revs->def, "HEAD") && + resolve_ref(revs->def, sha1, 0, &flag) && + (flag & REF_ISSYMREF)) { + /* + * Most commands can operate on an unborn branch + * without an explicit ref parameter as if no + * positive ref was specified (as opposed to the + * traditional behaviour of barfing on invalid HEAD). + */ + ; + } else { die("bad default revision '%s'", revs->def); - object = get_reference(revs, revs->def, sha1, 0); - add_pending_object_with_mode(revs, object, revs->def, mode); + } } /* Did the user ask for any diff output? Run the diff! */ diff --git i/revision.h w/revision.h index 2fdb2dd..7890fb7 100644 --- i/revision.h +++ w/revision.h @@ -30,7 +30,10 @@ struct rev_info { const char *prefix; const char *def; void *prune_data; - unsigned int early_output; + + /* Miscellaneous */ + unsigned int early_output:1, + require_valid_def:1; /* Traversal flags */ unsigned int dense:1,