On Fri, Jan 19, 2018 at 6:23 PM, Jeff King <peff@xxxxxxxx> wrote: > On Fri, Jan 19, 2018 at 06:14:56PM +0100, Christian Couder wrote: > >> > Let's discuss, what behavior we are waiting for >> > when atom seems useless for the command. Die or ignore? >> >> We could alternatively just emit a warning, but it looks like there >> are a lot of die() calls already in ref-filter.c, so I would suggest >> die(). > > I actually think it makes sense to just expand nonsense into the empty > string, for two reasons: > > 1. That's what ref-filter already does for the existing cases. For > example, try: > > git for-each-ref --format='%(objecttype) %(authordate)' > > and you will see that the annotated tags just get a blank author > field. > > 2. I think we may end up with a case where we feed multiple objects > via --batch-check, and the format is only nonsense for _some_ of > them. E.g., I envision a world where you can do: > > git cat-file --batch-check='%(objecttype) %(refname)' <<-\EOF > master > 12345abcde12345abcde12345abcde12345abcde > EOF > > and get output like: > > commit refs/heads/master > commit > > (i.e., if we would remember the refname discovered during the name > resolution, we could still report it). It would be annoying if the > second line caused us to die(). Yeah, ok, that makes sense. >> > And, which >> > atoms are useless (as I understand, "rest" and "deltabase" from >> > cat-file are useless for all ref-filter users, so the question is >> > about - am I right in it, and about ref-filter atoms for cat-file). >> >> For now and I think until the migration process is finished, you could >> just die() in case of any atom not already supported by the command. > > I'm OK with dying in the interim if it's easier, though I suspect it is > not much extra work to just expand to the empty string in such cases. If > that's where we want to end up eventually, it may be worth going > straight there. > > I also think %(deltabase) does make sense for anything that points to an > object. I suspect it's not all that _useful_ for for-each-ref, but that > doesn't mean we can't return the sensible thing if somebody asks for it. > > I agree that %(rest) probably doesn't make any sense for a caller which > isn't parsing input. Yeah, ok.
