* [PATCH] revision: remove stray whitespace when name empty @ 2019-06-07 22:59 Emily Shaffer 2019-06-08 0:42 ` Eric Sunshine ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Emily Shaffer @ 2019-06-07 22:59 UTC (permalink / raw) To: git; +Cc: jrnieder, Emily Shaffer Teach show_object_with_name() to avoid writing a space before a name which is empty. Also teach tests for rev-list --objects --filter to not require a space between the object ID and name. show_object_with_name() inserts a space between an object's OID and name regardless of whether the name is empty or not. This causes 'git cat-file (--batch | --batch-check)' to fail to discover the type of root directories: git rev-list --objects --filter=tree:1 --max-count=1 HEAD \ | git cat-file --batch-check git rev-parse HEAD: | xargs -I% git cat-file -t % git rev-list --objects --filter=tree:1 --max-count=1 HEAD \ | xargs -I% echo "AA%AA" git rev-list --objects --filter=tree:1 --max-count=1 HEAD \ | cut -f 1 -d ' ' | git cat-file --batch-check The extra space provided by 'show_object_with_name()' sticks to the output of 'git rev-list' even if it's piped into a file. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- I don't see any reason _not_ to remove this stray whitespace at the end, since it seems like it just gets in the way of easy scripting. I also think this case will only present itself for root trees. Not sure if making the whitespace optional is the right solution for the test, although I couldn't come up with a cleaner approach. Maybe something like this would be better, even though handling it in the regex is shorter? if [[ -z "$name" ]] then grep "^$hash" actual else grep "^$hash $name" actual fi - Emily revision.c | 3 ++- t/t6112-rev-list-filters-objects.sh | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/revision.c b/revision.c index d4aaf0ef25..260258b371 100644 --- a/revision.c +++ b/revision.c @@ -40,7 +40,8 @@ void show_object_with_name(FILE *out, struct object *obj, const char *name) { const char *p; - fprintf(out, "%s ", oid_to_hex(&obj->oid)); + fprintf(out, "%s%s", oid_to_hex(&obj->oid), + strcmp(name, "") == 0 ? "" : " "); for (p = name; *p && *p != '\n'; p++) fputc(*p, out); fputc('\n', out); diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh index acd7f5ab80..e05faa7a67 100755 --- a/t/t6112-rev-list-filters-objects.sh +++ b/t/t6112-rev-list-filters-objects.sh @@ -288,7 +288,7 @@ expect_has () { name=$2 && hash=$(git -C r3 rev-parse $commit:$name) && - grep "^$hash $name$" actual + grep "^$hash \?$name$" actual } test_expect_success 'verify tree:1 includes root trees' ' -- 2.22.0.rc2.383.gf4fbbf30c2-goog ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] revision: remove stray whitespace when name empty 2019-06-07 22:59 [PATCH] revision: remove stray whitespace when name empty Emily Shaffer @ 2019-06-08 0:42 ` Eric Sunshine 2019-06-12 19:23 ` Emily Shaffer 2019-06-09 13:00 ` Jeff King 2019-06-13 21:51 ` [PATCH v2] rev-list: teach --oid-only to enable piping Emily Shaffer 2 siblings, 1 reply; 22+ messages in thread From: Eric Sunshine @ 2019-06-08 0:42 UTC (permalink / raw) To: Emily Shaffer; +Cc: Git List, Jonathan Nieder On Fri, Jun 7, 2019 at 6:59 PM Emily Shaffer <emilyshaffer@google.com> wrote: > Teach show_object_with_name() to avoid writing a space before a name > which is empty. Also teach tests for rev-list --objects --filter to not > require a space between the object ID and name. > [...] > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > Not sure if making the whitespace optional is the right solution for the > test, although I couldn't come up with a cleaner approach. Maybe > something like this would be better, even though handling it in the > regex is shorter? > > if [[ -z "$name" ]] then In Git, we avoid Bash-isms. Just use 'test'. And, style is to place 'then' on its own line. if test -z "$name" then > diff --git a/revision.c b/revision.c > @@ -40,7 +40,8 @@ void show_object_with_name(FILE *out, struct object *obj, const char *name) > { > - fprintf(out, "%s ", oid_to_hex(&obj->oid)); > + fprintf(out, "%s%s", oid_to_hex(&obj->oid), > + strcmp(name, "") == 0 ? "" : " "); > for (p = name; *p && *p != '\n'; p++) > fputc(*p, out); > fputc('\n', out); It's subjective, but this starts to be a bit too noisy and unreadable. An alternative: fputs(oid_to_hex(...), out); if (*name) putc(' ', out); > diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh > @@ -288,7 +288,7 @@ expect_has () { > hash=$(git -C r3 rev-parse $commit:$name) && > - grep "^$hash $name$" actual > + grep "^$hash \?$name$" actual This would be the first use of \? with 'grep' in the test suite. I would worry about portability. Your suggestion earlier to tailor 'grep' invocation based upon whether $name is empty would be safer. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] revision: remove stray whitespace when name empty 2019-06-08 0:42 ` Eric Sunshine @ 2019-06-12 19:23 ` Emily Shaffer 0 siblings, 0 replies; 22+ messages in thread From: Emily Shaffer @ 2019-06-12 19:23 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Jonathan Nieder On Fri, Jun 07, 2019 at 08:42:45PM -0400, Eric Sunshine wrote: > On Fri, Jun 7, 2019 at 6:59 PM Emily Shaffer <emilyshaffer@google.com> wrote: > > Teach show_object_with_name() to avoid writing a space before a name > > which is empty. Also teach tests for rev-list --objects --filter to not > > require a space between the object ID and name. > > [...] > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > > --- > > Not sure if making the whitespace optional is the right solution for the > > test, although I couldn't come up with a cleaner approach. Maybe > > something like this would be better, even though handling it in the > > regex is shorter? > > > > if [[ -z "$name" ]] then > > In Git, we avoid Bash-isms. Just use 'test'. And, style is to place > 'then' on its own line. > > if test -z "$name" > then > > > diff --git a/revision.c b/revision.c > > @@ -40,7 +40,8 @@ void show_object_with_name(FILE *out, struct object *obj, const char *name) > > { > > - fprintf(out, "%s ", oid_to_hex(&obj->oid)); > > + fprintf(out, "%s%s", oid_to_hex(&obj->oid), > > + strcmp(name, "") == 0 ? "" : " "); > > for (p = name; *p && *p != '\n'; p++) > > fputc(*p, out); > > fputc('\n', out); > > It's subjective, but this starts to be a bit too noisy and unreadable. > An alternative: > > fputs(oid_to_hex(...), out); > if (*name) > putc(' ', out); > > > diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh > > @@ -288,7 +288,7 @@ expect_has () { > > hash=$(git -C r3 rev-parse $commit:$name) && > > - grep "^$hash $name$" actual > > + grep "^$hash \?$name$" actual > > This would be the first use of \? with 'grep' in the test suite. I > would worry about portability. Your suggestion earlier to tailor > 'grep' invocation based upon whether $name is empty would be safer. Thanks for the review, Eric. I'm going to try again from scratch with Peff and Junio's suggestion about adding an option to remove object names, and I'll try to keep your general style concerns in mind when I do so. - Emily ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] revision: remove stray whitespace when name empty 2019-06-07 22:59 [PATCH] revision: remove stray whitespace when name empty Emily Shaffer 2019-06-08 0:42 ` Eric Sunshine @ 2019-06-09 13:00 ` Jeff King 2019-06-10 16:29 ` Junio C Hamano 2019-06-13 21:51 ` [PATCH v2] rev-list: teach --oid-only to enable piping Emily Shaffer 2 siblings, 1 reply; 22+ messages in thread From: Jeff King @ 2019-06-09 13:00 UTC (permalink / raw) To: Emily Shaffer; +Cc: git, jrnieder On Fri, Jun 07, 2019 at 03:59:00PM -0700, Emily Shaffer wrote: > Teach show_object_with_name() to avoid writing a space before a name > which is empty. Also teach tests for rev-list --objects --filter to not > require a space between the object ID and name. > [...] > --- > I don't see any reason _not_ to remove this stray whitespace at the end, > since it seems like it just gets in the way of easy scripting. I also > think this case will only present itself for root trees. I'm a bit worried that this might break existing scripts. As ugly as trailing whitespace is, it does tell you something here: that the object is a root tree and not a commit. So in the past I have done things like: git rev-list --objects --all | grep ' ' to get only the non-commits. I'm undecided on whether we're straying into https://xkcd.com/1172/ territory here. I'd be more in favor if this were making things significantly easier, but... > show_object_with_name() inserts a space between an object's OID and name > regardless of whether the name is empty or not. This causes 'git > cat-file (--batch | --batch-check)' to fail to discover the type of root > directories: > > git rev-list --objects --filter=tree:1 --max-count=1 HEAD \ > | git cat-file --batch-check > git rev-parse HEAD: | xargs -I% git cat-file -t % > git rev-list --objects --filter=tree:1 --max-count=1 HEAD \ > | xargs -I% echo "AA%AA" > git rev-list --objects --filter=tree:1 --max-count=1 HEAD \ > | cut -f 1 -d ' ' | git cat-file --batch-check Your patch only helps with this at all because you're using the "tree:1" filter. It would not help: git rev-list --objects HEAD | git cat-file --batch-check because there you'll have actual names which cat-file will choke on. So it seems like this is helping only a very limited use case. cat-file actually does know how to split on whitespace. Unfortunately it does not do so by default, because that breaks some cases. See 97be04077f (cat-file: only split on whitespace when %(rest) is used, 2013-08-02). So you _can_ do: git rev-list --objects HEAD | git cat-file --batch-check='%(objectname) %(objecttype) %(rest)' But: 1. That puts the %(rest) bits in your output, which you may not want. 2. You have to actually specify the full format, so you might have to repeat batch-check's default format items. I think it would be reasonable for cat-file to have an option to split on whitespace (and if not given explicitly by the user, default to the presence of %(rest) as we do now). Alternatively, it would be reasonable to me to have an option for "rev-list --objects" to have an option to suppress the filename (and space) entirely. I think in the longer run along those lines that "--objects" should allow cat-file style pretty-print formats, which would eliminate the need to pipe to cat-file in the first place. That makes this parsing problem go away entirely, and it's way more efficient to boot (rev-list already knows the types!). -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] revision: remove stray whitespace when name empty 2019-06-09 13:00 ` Jeff King @ 2019-06-10 16:29 ` Junio C Hamano 2019-06-12 19:37 ` Emily Shaffer 0 siblings, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2019-06-10 16:29 UTC (permalink / raw) To: Jeff King; +Cc: Emily Shaffer, git, jrnieder Jeff King <peff@peff.net> writes: > Your patch only helps with this at all because you're using the "tree:1" > ... > because there you'll have actual names which cat-file will choke on. So > it seems like this is helping only a very limited use case. > ... > Alternatively, it would be reasonable to me to have an option for > "rev-list --objects" to have an option to suppress the filename (and > space) entirely. Yup, I think that is a more reasonable short-term change compared to the patch being discussed, and ... > I think in the longer run along those lines that "--objects" should > allow cat-file style pretty-print formats, which would eliminate the > need to pipe to cat-file in the first place. That makes this parsing > problem go away entirely, and it's way more efficient to boot (rev-list > already knows the types!). ... of course this makes tons of sense. Thanks. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] revision: remove stray whitespace when name empty 2019-06-10 16:29 ` Junio C Hamano @ 2019-06-12 19:37 ` Emily Shaffer 2019-06-13 15:20 ` Jeff King 0 siblings, 1 reply; 22+ messages in thread From: Emily Shaffer @ 2019-06-12 19:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, jrnieder On Mon, Jun 10, 2019 at 09:29:14AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Your patch only helps with this at all because you're using the "tree:1" > > ... > > because there you'll have actual names which cat-file will choke on. So > > it seems like this is helping only a very limited use case. > > ... > > Alternatively, it would be reasonable to me to have an option for > > "rev-list --objects" to have an option to suppress the filename (and > > space) entirely. > > Yup, I think that is a more reasonable short-term change compared to > the patch being discussed, and ... I like this too. Starting work on it today. > > > I think in the longer run along those lines that "--objects" should > > allow cat-file style pretty-print formats, which would eliminate the > > need to pipe to cat-file in the first place. That makes this parsing > > problem go away entirely, and it's way more efficient to boot (rev-list > > already knows the types!). > > ... of course this makes tons of sense. Probably not going to start work on this now, but I will make a note to myself to have a look if I have some spare time soon, and keep an eye out for reviews in that area. > > Thanks. Thanks all for the feedback, I'll have a reroll soon. - Emily ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] revision: remove stray whitespace when name empty 2019-06-12 19:37 ` Emily Shaffer @ 2019-06-13 15:20 ` Jeff King 0 siblings, 0 replies; 22+ messages in thread From: Jeff King @ 2019-06-13 15:20 UTC (permalink / raw) To: Emily Shaffer; +Cc: Olga Telezhnaya, Junio C Hamano, git, jrnieder On Wed, Jun 12, 2019 at 12:37:45PM -0700, Emily Shaffer wrote: > > > Alternatively, it would be reasonable to me to have an option for > > > "rev-list --objects" to have an option to suppress the filename (and > > > space) entirely. > > > > Yup, I think that is a more reasonable short-term change compared to > > the patch being discussed, and ... > > I like this too. Starting work on it today. Great! > > > I think in the longer run along those lines that "--objects" should > > > allow cat-file style pretty-print formats, which would eliminate the > > > need to pipe to cat-file in the first place. That makes this parsing > > > problem go away entirely, and it's way more efficient to boot (rev-list > > > already knows the types!). > > > > ... of course this makes tons of sense. > > Probably not going to start work on this now, but I will make a note to > myself to have a look if I have some spare time soon, and keep an eye > out for reviews in that area. That's fine. I suspect it will be quite involved, because the format placeholders you'd want for "objects" are going to be more like "cat-file" ones than like the existing pretty.c ones. So I think this really implies unifying those format systems, which is a long-term project that Olga has been working on. -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2] rev-list: teach --oid-only to enable piping 2019-06-07 22:59 [PATCH] revision: remove stray whitespace when name empty Emily Shaffer 2019-06-08 0:42 ` Eric Sunshine 2019-06-09 13:00 ` Jeff King @ 2019-06-13 21:51 ` Emily Shaffer 2019-06-14 16:07 ` Jeff King 2019-06-14 23:48 ` [PATCH v3] rev-list: teach --no-object-names " Emily Shaffer 2 siblings, 2 replies; 22+ messages in thread From: Emily Shaffer @ 2019-06-13 21:51 UTC (permalink / raw) To: git Cc: Emily Shaffer, Jeff King, Junio C Hamano, Eric Sunshine, Jonathan Nieder Allow easier parsing by cat-file by giving rev-list an option to print only the OID of an object without any additional information. This is a short-term shim; later on, rev-list should be taught how to print the types of objects it finds in a format similar to cat-file's. Before this commit, the output from rev-list needed to be massaged before being piped to cat-file, like so: git rev-list --objects HEAD | cut -f 1 -d ' ' \ | git cat-file --batch-check This was especially unexpected when dealing with root trees, as an invisible whitespace exists at the end of the OID: git rev-list --objects --filter=tree:1 --max-count=1 HEAD \ | xargs -I% echo "AA%AA" Now, it can be piped directly, as in the added test case: git rev-list --objects --oid-only HEAD | git cat-file --batch-check Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Change-Id: I489bdf0a8215532e540175188883ff7541d70e1b --- It didn't appear that using an existing --pretty string would do the trick, as the formatting options for --pretty apply specifically to commit objects (you can specify the commit hash and the tree hash, but I didn't see a way to more generally pretty-print all types of objects). rev-list doesn't appear to use parse-options-h, so I added a new option as simply as I could see without breaking existing style; it seemed wrong to add a flag to the `struct rev_list_info` as the definition of struct and flag values alike are contained in bisect.h. There are a couple other options to rev-list which are stored as globals. At the moment, this doesn't work with --abbrev, and although I think it'd be a good fit, right now --abbrev doesn't seem to work without --abbrev-commit, and both those options end up being eaten by setup_revisions() rather than by rev-list itself. - Emily builtin/rev-list.c | 21 ++++++++++++++++++++- t/t6000-rev-list-misc.sh | 18 ++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 9f31837d30..ff07ea9564 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -48,7 +48,7 @@ static const char rev_list_usage[] = " --children\n" " --objects | --objects-edge\n" " --unpacked\n" -" --header | --pretty\n" +" --header | --pretty | --oid-only\n" " --abbrev=<n> | --no-abbrev\n" " --abbrev-commit\n" " --left-right\n" @@ -75,6 +75,8 @@ enum missing_action { }; static enum missing_action arg_missing_action; +static int arg_oid_only; /* display only the oid of each object encountered */ + #define DEFAULT_OIDSET_SIZE (16*1024) static void finish_commit(struct commit *commit, void *data); @@ -90,6 +92,11 @@ static void show_commit(struct commit *commit, void *data) return; } + if (arg_oid_only) { + printf("%s\n", oid_to_hex(&commit->object.oid)); + return; + } + graph_show_commit(revs->graph); if (revs->count) { @@ -255,6 +262,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data) display_progress(progress, ++progress_counter); if (info->flags & REV_LIST_QUIET) return; + if (arg_oid_only) { + printf("%s\n", oid_to_hex(&obj->oid)); + return; + } show_object_with_name(stdout, obj, name); } @@ -484,6 +495,11 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) if (skip_prefix(arg, "--missing=", &arg)) continue; /* already handled above */ + if (!strcmp(arg, ("--oid-only"))) { + arg_oid_only = 1; + continue; + } + usage(rev_list_usage); } @@ -499,6 +515,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) /* Only --header was specified */ revs.commit_format = CMIT_FMT_RAW; + if (arg_oid_only && revs.commit_format != CMIT_FMT_UNSPECIFIED) + die(_("cannot combine --oid-only and --pretty or --header")); + if ((!revs.commits && reflog_walk_empty(revs.reflog_info) && (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) && !revs.pending.nr) && diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh index 0507999729..996ba1799b 100755 --- a/t/t6000-rev-list-misc.sh +++ b/t/t6000-rev-list-misc.sh @@ -48,6 +48,24 @@ test_expect_success 'rev-list --objects with pathspecs and copied files' ' ! grep one output ' +test_expect_success 'rev-list --objects --oid-only has no whitespace/names' ' + git rev-list --objects --oid-only HEAD >output && + ! grep wanted_file output && + ! grep unwanted_file output && + ! grep " " output +' + +test_expect_success 'rev-list --objects --oid-only is usable by cat-file' ' + git rev-list --objects --oid-only --all >list-output && + git cat-file --batch-check <list-output >cat-output && + ! grep missing cat-output +' + +test_expect_success 'rev-list --oid-only is incompatible with --pretty' ' + test_must_fail git rev-list --objects --oid-only --pretty HEAD && + test_must_fail git rev-list --objects --oid-only --header HEAD +' + test_expect_success 'rev-list A..B and rev-list ^A B are the same' ' git commit --allow-empty -m another && git tag -a -m "annotated" v1.0 && -- 2.22.0.rc2.383.gf4fbbf30c2-goog ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2] rev-list: teach --oid-only to enable piping 2019-06-13 21:51 ` [PATCH v2] rev-list: teach --oid-only to enable piping Emily Shaffer @ 2019-06-14 16:07 ` Jeff King 2019-06-14 20:25 ` Junio C Hamano 2019-06-14 23:29 ` Emily Shaffer 2019-06-14 23:48 ` [PATCH v3] rev-list: teach --no-object-names " Emily Shaffer 1 sibling, 2 replies; 22+ messages in thread From: Jeff King @ 2019-06-14 16:07 UTC (permalink / raw) To: Emily Shaffer; +Cc: git, Junio C Hamano, Eric Sunshine, Jonathan Nieder On Thu, Jun 13, 2019 at 02:51:03PM -0700, Emily Shaffer wrote: > It didn't appear that using an existing --pretty string would do the > trick, as the formatting options for --pretty apply specifically to > commit objects (you can specify the commit hash and the tree hash, but > I didn't see a way to more generally pretty-print all types of objects). Right, it would be a big change to start custom-formatting the non-commits. I think this is a good step in the right direction. > rev-list doesn't appear to use parse-options-h, so I added a new option > as simply as I could see without breaking existing style; it seemed > wrong to add a flag to the `struct rev_list_info` as the definition of > struct and flag values alike are contained in bisect.h. There are a > couple other options to rev-list which are stored as globals. I think that's reasonable. This is definitely a rev-list option (not a revision.c one). The interaction between bisect.h and rev-list is a bit funny, but it's probably simpler not to try solving it here. > + if (arg_oid_only && revs.commit_format != CMIT_FMT_UNSPECIFIED) > + die(_("cannot combine --oid-only and --pretty or --header")); As you've implemented it here, I definitely agree that this conflicts with --pretty. But I think it also interacts badly with other options, like "--graph". TBH, I am not sure that "rev-list --graph --objects" is all that useful, because the non-commit objects are not prefixed with the graph lines. And without "--objects", this new option is not useful because the default is already to show only the oid. But I wonder if things would be simpler if we did not touch the commit code path at all. I.e., if this were simply "--no-object-names", and it touched only show_object(). And then you do not have to worry about funny interactions with commit formatting at all. It would be valid to do: git rev-list --pretty=raw --objects --no-object-names HEAD if you really wanted to (though I admit I do not have an immediate use for it). > @@ -255,6 +262,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data) > display_progress(progress, ++progress_counter); > if (info->flags & REV_LIST_QUIET) > return; > + if (arg_oid_only) { > + printf("%s\n", oid_to_hex(&obj->oid)); > + return; > + } > show_object_with_name(stdout, obj, name); > } > A minor style point, but I think this might be easier to follow without the early return, since we are really choosing to do A or B. Writing: if (arg_oid_only) printf(...); else show_object_with_name(...); shows that more clearly, I think. > [...] Otherwise, this looks good, including the tests. One thing I did notice in the tests: > +test_expect_success 'rev-list --objects --oid-only is usable by cat-file' ' > + git rev-list --objects --oid-only --all >list-output && > + git cat-file --batch-check <list-output >cat-output && > + ! grep missing cat-output > +' Usually we prefer to look for the expected output, rather than making sure we did not find the unexpected. But I'm not sure if that might be burdensome in this case (i.e., if there's a bunch of cruft coming out of "rev-list" that would be annoying to match, and might even change as people add more tests). So I'm OK with it either way. -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] rev-list: teach --oid-only to enable piping 2019-06-14 16:07 ` Jeff King @ 2019-06-14 20:25 ` Junio C Hamano 2019-06-14 23:18 ` Emily Shaffer 2019-06-14 23:29 ` Emily Shaffer 1 sibling, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2019-06-14 20:25 UTC (permalink / raw) To: Jeff King; +Cc: Emily Shaffer, git, Eric Sunshine, Jonathan Nieder Jeff King <peff@peff.net> writes: > But I wonder if things would be simpler if we did not touch the commit > code path at all. I.e., if this were simply "--no-object-names", and it > touched only show_object(). Yeah, that sounds more tempting. And the refined code structure you suggested ... >> @@ -255,6 +262,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data) >> display_progress(progress, ++progress_counter); >> if (info->flags & REV_LIST_QUIET) >> return; >> + if (arg_oid_only) { >> + printf("%s\n", oid_to_hex(&obj->oid)); >> + return; >> + } >> show_object_with_name(stdout, obj, name); >> } >> > > A minor style point, but I think this might be easier to follow without > the early return, since we are really choosing to do A or B. Writing: > > if (arg_oid_only) > printf(...); > else > show_object_with_name(...); > > shows that more clearly, I think. ... is a good way to clearly show that intention, I would think. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] rev-list: teach --oid-only to enable piping 2019-06-14 20:25 ` Junio C Hamano @ 2019-06-14 23:18 ` Emily Shaffer 0 siblings, 0 replies; 22+ messages in thread From: Emily Shaffer @ 2019-06-14 23:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, Eric Sunshine, Jonathan Nieder On Fri, Jun 14, 2019 at 01:25:59PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > But I wonder if things would be simpler if we did not touch the commit > > code path at all. I.e., if this were simply "--no-object-names", and it > > touched only show_object(). > > Yeah, that sounds more tempting. And the refined code structure you > suggested ... > > >> @@ -255,6 +262,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data) > >> display_progress(progress, ++progress_counter); > >> if (info->flags & REV_LIST_QUIET) > >> return; > >> + if (arg_oid_only) { > >> + printf("%s\n", oid_to_hex(&obj->oid)); > >> + return; > >> + } > >> show_object_with_name(stdout, obj, name); > >> } > >> > > > > A minor style point, but I think this might be easier to follow without > > the early return, since we are really choosing to do A or B. Writing: > > > > if (arg_oid_only) > > printf(...); > > else > > show_object_with_name(...); > > > > shows that more clearly, I think. > > ... is a good way to clearly show that intention, I would think. Sounds good. Thanks, both; I'll reroll that quickly today. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] rev-list: teach --oid-only to enable piping 2019-06-14 16:07 ` Jeff King 2019-06-14 20:25 ` Junio C Hamano @ 2019-06-14 23:29 ` Emily Shaffer 2019-06-19 21:24 ` Jeff King 1 sibling, 1 reply; 22+ messages in thread From: Emily Shaffer @ 2019-06-14 23:29 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano, Eric Sunshine, Jonathan Nieder On Fri, Jun 14, 2019 at 12:07:28PM -0400, Jeff King wrote: > On Thu, Jun 13, 2019 at 02:51:03PM -0700, Emily Shaffer wrote: > > +test_expect_success 'rev-list --objects --oid-only is usable by cat-file' ' > > + git rev-list --objects --oid-only --all >list-output && > > + git cat-file --batch-check <list-output >cat-output && > > + ! grep missing cat-output > > +' > > Usually we prefer to look for the expected output, rather than making > sure we did not find the unexpected. But I'm not sure if that might be > burdensome in this case (i.e., if there's a bunch of cruft coming out of > "rev-list" that would be annoying to match, and might even change as > people add more tests). So I'm OK with it either way. My (newbie) opinion is that in this case, we specifically want to know that cat-file didn't choke on objects which we know exist (since they came from rev-list). I have the feeling that checking for the exact objects returned instead (or a sample of them) would be more brittle and would also make the wording of the test less direct. So if there's no complaint either way, I'd prefer to leave it the way it is. By the way, rev-list-misc.sh has a number of other existing "! grep ..." lines. - Emily ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] rev-list: teach --oid-only to enable piping 2019-06-14 23:29 ` Emily Shaffer @ 2019-06-19 21:24 ` Jeff King 0 siblings, 0 replies; 22+ messages in thread From: Jeff King @ 2019-06-19 21:24 UTC (permalink / raw) To: Emily Shaffer; +Cc: git, Junio C Hamano, Eric Sunshine, Jonathan Nieder On Fri, Jun 14, 2019 at 04:29:46PM -0700, Emily Shaffer wrote: > On Fri, Jun 14, 2019 at 12:07:28PM -0400, Jeff King wrote: > > On Thu, Jun 13, 2019 at 02:51:03PM -0700, Emily Shaffer wrote: > > > > +test_expect_success 'rev-list --objects --oid-only is usable by cat-file' ' > > > + git rev-list --objects --oid-only --all >list-output && > > > + git cat-file --batch-check <list-output >cat-output && > > > + ! grep missing cat-output > > > +' > > > > Usually we prefer to look for the expected output, rather than making > > sure we did not find the unexpected. But I'm not sure if that might be > > burdensome in this case (i.e., if there's a bunch of cruft coming out of > > "rev-list" that would be annoying to match, and might even change as > > people add more tests). So I'm OK with it either way. > > My (newbie) opinion is that in this case, we specifically want to know > that cat-file didn't choke on objects which we know exist (since they > came from rev-list). I have the feeling that checking for the exact > objects returned instead (or a sample of them) would be more brittle and > would also make the wording of the test less direct. > > So if there's no complaint either way, I'd prefer to leave it the way it > is. Yeah, that's fine with me if it seems more clear to use grep here (and I was on the fence). > By the way, rev-list-misc.sh has a number of other existing "! grep ..." > lines. It never fails that when I complain about a style issue, the surrounding code is full of the same thing. ;) I'd have to look at each one to determine if they're sensible or not, and it's probably not worth anybody's time to do that cleanup at this point in time. -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3] rev-list: teach --no-object-names to enable piping 2019-06-13 21:51 ` [PATCH v2] rev-list: teach --oid-only to enable piping Emily Shaffer 2019-06-14 16:07 ` Jeff King @ 2019-06-14 23:48 ` Emily Shaffer 2019-06-17 22:32 ` Junio C Hamano 2019-06-18 22:29 ` [PATCH v4] " Emily Shaffer 1 sibling, 2 replies; 22+ messages in thread From: Emily Shaffer @ 2019-06-14 23:48 UTC (permalink / raw) To: git Cc: Emily Shaffer, Junio C Hamano, Jeff King, Eric Sunshine, Jonathan Nieder Allow easier parsing by cat-file by giving rev-list an option to print only the OID of a non-commit object without any additional information. This is a short-term shim; later on, rev-list should be taught how to print the types of objects it finds in a format similar to cat-file's. Before this commit, the output from rev-list needed to be massaged before being piped to cat-file, like so: git rev-list --objects HEAD | cut -f 1 -d ' ' \ | git cat-file --batch-check This was especially unexpected when dealing with root trees, as an invisible whitespace exists at the end of the OID: git rev-list --objects --filter=tree:1 --max-count=1 HEAD \ | xargs -I% echo "AA%AA" Now, it can be piped directly, as in the added test case: git rev-list --objects --no-object-names HEAD | git cat-file --batch-check Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Change-Id: I489bdf0a8215532e540175188883ff7541d70e1b --- Based on Peff and Junio's comments, made following changes since v2: - Removed interaction with commit objects - Renamed option to "no-object-names" - Removed warnings when new option is combined with commit-formatting options (and reflected in usage) - Simplified logic in show_object() Thanks for the thoughts, all. - Emily builtin/rev-list.c | 14 +++++++++++++- t/t6000-rev-list-misc.sh | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 660172b014..7e2598fd22 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -49,6 +49,7 @@ static const char rev_list_usage[] = " --objects | --objects-edge\n" " --unpacked\n" " --header | --pretty\n" +" --no-object-names\n" " --abbrev=<n> | --no-abbrev\n" " --abbrev-commit\n" " --left-right\n" @@ -75,6 +76,9 @@ enum missing_action { }; static enum missing_action arg_missing_action; +/* display only the oid of each object encountered */ +static int arg_no_object_names; + #define DEFAULT_OIDSET_SIZE (16*1024) static void finish_commit(struct commit *commit); @@ -255,7 +259,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data) display_progress(progress, ++progress_counter); if (info->flags & REV_LIST_QUIET) return; - show_object_with_name(stdout, obj, name); + if (arg_no_object_names) + printf("%s\n", oid_to_hex(&obj->oid)); + else + show_object_with_name(stdout, obj, name); } static void show_edge(struct commit *commit) @@ -484,6 +491,11 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) if (skip_prefix(arg, "--missing=", &arg)) continue; /* already handled above */ + if (!strcmp(arg, ("--no-object-names"))) { + arg_no_object_names = 1; + continue; + } + usage(rev_list_usage); } diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh index 0507999729..5d87171b99 100755 --- a/t/t6000-rev-list-misc.sh +++ b/t/t6000-rev-list-misc.sh @@ -48,6 +48,24 @@ test_expect_success 'rev-list --objects with pathspecs and copied files' ' ! grep one output ' +test_expect_success 'rev-list --objects --no-object-names has no space/names' ' + git rev-list --objects --no-object-names HEAD >output && + ! grep wanted_file output && + ! grep unwanted_file output && + ! grep " " output +' + +test_expect_success 'rev-list --objects --no-object-names works with cat-file' ' + git rev-list --objects --no-object-names --all >list-output && + git cat-file --batch-check <list-output >cat-output && + ! grep missing cat-output +' + +test_expect_success 'rev-list --oid-only is incompatible with --pretty' ' + test_must_fail git rev-list --objects --oid-only --pretty HEAD && + test_must_fail git rev-list --objects --oid-only --header HEAD +' + test_expect_success 'rev-list A..B and rev-list ^A B are the same' ' git commit --allow-empty -m another && git tag -a -m "annotated" v1.0 && -- 2.22.0.410.gd8fdbe21b5-goog ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3] rev-list: teach --no-object-names to enable piping 2019-06-14 23:48 ` [PATCH v3] rev-list: teach --no-object-names " Emily Shaffer @ 2019-06-17 22:32 ` Junio C Hamano 2019-06-18 22:08 ` Emily Shaffer 2019-06-18 22:29 ` [PATCH v4] " Emily Shaffer 1 sibling, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2019-06-17 22:32 UTC (permalink / raw) To: Emily Shaffer; +Cc: git, Jeff King, Eric Sunshine, Jonathan Nieder Emily Shaffer <emilyshaffer@google.com> writes: > Allow easier parsing by cat-file by giving rev-list an option to print > only the OID of a non-commit object without any additional information. > This is a short-term shim; later on, rev-list should be taught how to > print the types of objects it finds in a format similar to cat-file's. > > Before this commit, the output from rev-list needed to be massaged > before being piped to cat-file, like so: > > git rev-list --objects HEAD | cut -f 1 -d ' ' \ > | git cat-file --batch-check Write this with '|' at the end of the first line; that way the shell knows you haven't finished your sentence without any backslash. > diff --git a/builtin/rev-list.c b/builtin/rev-list.c > index 660172b014..7e2598fd22 100644 > --- a/builtin/rev-list.c > +++ b/builtin/rev-list.c > @@ -49,6 +49,7 @@ static const char rev_list_usage[] = > " --objects | --objects-edge\n" > " --unpacked\n" > " --header | --pretty\n" > +" --no-object-names\n" Ideally, this should be "--[no-]object-names", i.e. --object-names which may be the default when using --objects could be tweaked with --no-object-names and then again turned on with a --object-names later on the command line, following the usual "last one wins". > @@ -75,6 +76,9 @@ enum missing_action { > }; > static enum missing_action arg_missing_action; > > +/* display only the oid of each object encountered */ > +static int arg_no_object_names; And this would become static int show_object_names = 1; that can be turned off via --no-object-names (and --object-names would flip it on). It would reduce the double negation brain twister, hopefully. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3] rev-list: teach --no-object-names to enable piping 2019-06-17 22:32 ` Junio C Hamano @ 2019-06-18 22:08 ` Emily Shaffer 0 siblings, 0 replies; 22+ messages in thread From: Emily Shaffer @ 2019-06-18 22:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Eric Sunshine, Jonathan Nieder On Mon, Jun 17, 2019 at 03:32:34PM -0700, Junio C Hamano wrote: > Emily Shaffer <emilyshaffer@google.com> writes: > > > Allow easier parsing by cat-file by giving rev-list an option to print > > only the OID of a non-commit object without any additional information. > > This is a short-term shim; later on, rev-list should be taught how to > > print the types of objects it finds in a format similar to cat-file's. > > > > Before this commit, the output from rev-list needed to be massaged > > before being piped to cat-file, like so: > > > > git rev-list --objects HEAD | cut -f 1 -d ' ' \ > > | git cat-file --batch-check > > Write this with '|' at the end of the first line; that way the shell > knows you haven't finished your sentence without any backslash. Oh cool. Will do. > > > diff --git a/builtin/rev-list.c b/builtin/rev-list.c > > index 660172b014..7e2598fd22 100644 > > --- a/builtin/rev-list.c > > +++ b/builtin/rev-list.c > > @@ -49,6 +49,7 @@ static const char rev_list_usage[] = > > " --objects | --objects-edge\n" > > " --unpacked\n" > > " --header | --pretty\n" > > +" --no-object-names\n" > > Ideally, this should be "--[no-]object-names", i.e. --object-names > which may be the default when using --objects could be tweaked with > --no-object-names and then again turned on with a --object-names > later on the command line, following the usual "last one wins". Sure, good point. Will fix. > > > @@ -75,6 +76,9 @@ enum missing_action { > > }; > > static enum missing_action arg_missing_action; > > > > +/* display only the oid of each object encountered */ > > +static int arg_no_object_names; > > And this would become > > static int show_object_names = 1; > > that can be turned off via --no-object-names (and --object-names > would flip it on). It would reduce the double negation brain > twister, hopefully. > I'll send a new patch today. Thanks. - Emily ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v4] rev-list: teach --no-object-names to enable piping 2019-06-14 23:48 ` [PATCH v3] rev-list: teach --no-object-names " Emily Shaffer 2019-06-17 22:32 ` Junio C Hamano @ 2019-06-18 22:29 ` Emily Shaffer 2019-06-19 14:08 ` Junio C Hamano 2019-06-19 20:56 ` [PATCH v5] " Emily Shaffer 1 sibling, 2 replies; 22+ messages in thread From: Emily Shaffer @ 2019-06-18 22:29 UTC (permalink / raw) To: git Cc: Emily Shaffer, Junio C Hamano, Jeff King, Eric Sunshine, Jonathan Nieder Allow easier parsing by cat-file by giving rev-list an option to print only the OID of a non-commit object without any additional information. This is a short-term shim; later on, rev-list should be taught how to print the types of objects it finds in a format similar to cat-file's. Before this commit, the output from rev-list needed to be massaged before being piped to cat-file, like so: git rev-list --objects HEAD | cut -f 1 -d ' ' | git cat-file --batch-check This was especially unexpected when dealing with root trees, as an invisible whitespace exists at the end of the OID: git rev-list --objects --filter=tree:1 --max-count=1 HEAD | xargs -I% echo "AA%AA" Now, it can be piped directly, as in the added test case: git rev-list --objects --no-object-names HEAD | git cat-file --batch-check Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Change-Id: I489bdf0a8215532e540175188883ff7541d70e1b --- Since v3, added a corresponding "--object-names" arg to pair with "--no-object-names", and "last-one-wins" logic. Also added a test to validate this new arg and the logic. I did not take Junio's suggestion of naming the arg "show_object_names" as "arg_show_object_names" better matches the existing style of builtin/revlist.c. In adding the test, I noticed that I had left in a test about --oid-only that doesn't apply after the changes from v2->v3; that test is removed. - Emily builtin/rev-list.c | 19 ++++++++++++++++++- t/t6000-rev-list-misc.sh | 20 ++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 660172b014..301ccb970b 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -49,6 +49,7 @@ static const char rev_list_usage[] = " --objects | --objects-edge\n" " --unpacked\n" " --header | --pretty\n" +" --[no-]object-names\n" " --abbrev=<n> | --no-abbrev\n" " --abbrev-commit\n" " --left-right\n" @@ -75,6 +76,9 @@ enum missing_action { }; static enum missing_action arg_missing_action; +/* display only the oid of each object encountered */ +static int arg_show_object_names = 1; + #define DEFAULT_OIDSET_SIZE (16*1024) static void finish_commit(struct commit *commit); @@ -255,7 +259,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data) display_progress(progress, ++progress_counter); if (info->flags & REV_LIST_QUIET) return; - show_object_with_name(stdout, obj, name); + if (arg_show_object_names) + show_object_with_name(stdout, obj, name); + else + printf("%s\n", oid_to_hex(&obj->oid)); } static void show_edge(struct commit *commit) @@ -484,6 +491,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) if (skip_prefix(arg, "--missing=", &arg)) continue; /* already handled above */ + if (!strcmp(arg, ("--no-object-names"))) { + arg_show_object_names = 0; + continue; + } + + if (!strcmp(arg, ("--object-names"))) { + arg_show_object_names = 1; + continue; + } + usage(rev_list_usage); } diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh index 0507999729..52a9e38d66 100755 --- a/t/t6000-rev-list-misc.sh +++ b/t/t6000-rev-list-misc.sh @@ -48,6 +48,26 @@ test_expect_success 'rev-list --objects with pathspecs and copied files' ' ! grep one output ' +test_expect_success 'rev-list --objects --no-object-names has no space/names' ' + git rev-list --objects --no-object-names HEAD >output && + ! grep wanted_file output && + ! grep unwanted_file output && + ! grep " " output +' + +test_expect_success 'rev-list --objects --no-object-names works with cat-file' ' + git rev-list --objects --no-object-names --all >list-output && + git cat-file --batch-check <list-output >cat-output && + ! grep missing cat-output +' + +test_expect_success '--no-object-names and --object-names are last-one-wins' ' + git rev-list --objects --no-object-names --object-names --all >output && + grep wanted_file output && + git rev-list --objects --object-names --no-object-names --all >output && + ! grep wanted_file output +' + test_expect_success 'rev-list A..B and rev-list ^A B are the same' ' git commit --allow-empty -m another && git tag -a -m "annotated" v1.0 && -- 2.22.0.410.gd8fdbe21b5-goog ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v4] rev-list: teach --no-object-names to enable piping 2019-06-18 22:29 ` [PATCH v4] " Emily Shaffer @ 2019-06-19 14:08 ` Junio C Hamano 2019-06-19 19:31 ` Emily Shaffer 2019-06-19 20:56 ` [PATCH v5] " Emily Shaffer 1 sibling, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2019-06-19 14:08 UTC (permalink / raw) To: Emily Shaffer; +Cc: git, Jeff King, Eric Sunshine, Jonathan Nieder Emily Shaffer <emilyshaffer@google.com> writes: > Since v3, added a corresponding "--object-names" arg to pair with > "--no-object-names", and "last-one-wins" logic. Also added a test to > validate this new arg and the logic. Thanks for a quick turnaround (unfortunately, I was OOO yesterday and I am half-sick today, so please expect slow responses---sorry about that). > In adding the test, I noticed that I had left in a test about --oid-only > that doesn't apply after the changes from v2->v3; that test is removed. I noticed in range-diff, too. So now --object-names can be used with --pretty (not that "rev-list --pretty --objects" makes much sense in the first place, so no point in testing that it works). ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4] rev-list: teach --no-object-names to enable piping 2019-06-19 14:08 ` Junio C Hamano @ 2019-06-19 19:31 ` Emily Shaffer 2019-06-19 21:30 ` Jeff King 0 siblings, 1 reply; 22+ messages in thread From: Emily Shaffer @ 2019-06-19 19:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Eric Sunshine, Jonathan Nieder On Wed, Jun 19, 2019 at 07:08:14AM -0700, Junio C Hamano wrote: > Emily Shaffer <emilyshaffer@google.com> writes: > > > Since v3, added a corresponding "--object-names" arg to pair with > > "--no-object-names", and "last-one-wins" logic. Also added a test to > > validate this new arg and the logic. > > Thanks for a quick turnaround (unfortunately, I was OOO yesterday > and I am half-sick today, so please expect slow responses---sorry > about that). > > > In adding the test, I noticed that I had left in a test about --oid-only > > that doesn't apply after the changes from v2->v3; that test is removed. > > I noticed in range-diff, too. So now --object-names can be used > with --pretty (not that "rev-list --pretty --objects" makes much > sense in the first place, so no point in testing that it works). Yeah, it works. It looks weird, but it works pretty much as you'd expect: emilyshaffer@podkayne:~/git-second [stray-whitespace]$ tg2 rev-list --object-names --objects --pretty=short --max-count=1 HEAD | head -n 20 commit 701c66d5f2fafe163892fa0968ce8bca041dbc92 Author: Emily Shaffer <emilyshaffer@google.com> rev-list: teach --no-object-names to enable piping d4b1d372d16aaff35b221afce017f90542fd9293 41d4cd23fd97f599053a19555a173894da71e560 .clang-format 42cdc4bbfb05934bb9c3ed2fe0e0d45212c32d7a .editorconfig emilyshaffer@podkayne:~/git-second [stray-whitespace]$ tg2 rev-list --no-object-names --objects --pretty=short --max-count=1 HEAD | head -n 20 commit 701c66d5f2fafe163892fa0968ce8bca041dbc92 Author: Emily Shaffer <emilyshaffer@google.com> rev-list: teach --no-object-names to enable piping d4b1d372d16aaff35b221afce017f90542fd9293 41d4cd23fd97f599053a19555a173894da71e560 42cdc4bbfb05934bb9c3ed2fe0e0d45212c32d7a Ah, but when I was grabbing these samples, I noticed that I didn't update the manpage for rev-list. So please wait while I reroll again... sorry! - Emily ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v4] rev-list: teach --no-object-names to enable piping 2019-06-19 19:31 ` Emily Shaffer @ 2019-06-19 21:30 ` Jeff King 0 siblings, 0 replies; 22+ messages in thread From: Jeff King @ 2019-06-19 21:30 UTC (permalink / raw) To: Emily Shaffer; +Cc: Junio C Hamano, git, Eric Sunshine, Jonathan Nieder On Wed, Jun 19, 2019 at 12:31:34PM -0700, Emily Shaffer wrote: > > I noticed in range-diff, too. So now --object-names can be used > > with --pretty (not that "rev-list --pretty --objects" makes much > > sense in the first place, so no point in testing that it works). > > Yeah, it works. It looks weird, but it works pretty much as you'd > expect: I think that something like: git rev-list --in-commit-order --oneline --objects HEAD could make sense to get a rough idea of which commits are mentioning which objects (though in most cases I'd probably use "log --find-object" these days, since it's more precise; rev-list is looking in a funny reverse order). -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v5] rev-list: teach --no-object-names to enable piping 2019-06-18 22:29 ` [PATCH v4] " Emily Shaffer 2019-06-19 14:08 ` Junio C Hamano @ 2019-06-19 20:56 ` Emily Shaffer 2019-06-19 21:38 ` Jeff King 1 sibling, 1 reply; 22+ messages in thread From: Emily Shaffer @ 2019-06-19 20:56 UTC (permalink / raw) To: git Cc: Emily Shaffer, Junio C Hamano, Jeff King, Eric Sunshine, Jonathan Nieder Allow easier parsing by cat-file by giving rev-list an option to print only the OID of a non-commit object without any additional information. This is a short-term shim; later on, rev-list should be taught how to print the types of objects it finds in a format similar to cat-file's. Before this commit, the output from rev-list needed to be massaged before being piped to cat-file, like so: git rev-list --objects HEAD | cut -f 1 -d ' ' | git cat-file --batch-check This was especially unexpected when dealing with root trees, as an invisible whitespace exists at the end of the OID: git rev-list --objects --filter=tree:1 --max-count=1 HEAD | xargs -I% echo "AA%AA" Now, it can be piped directly, as in the added test case: git rev-list --objects --no-object-names HEAD | git cat-file --batch-check Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Change-Id: I489bdf0a8215532e540175188883ff7541d70e1b --- Since v4, added the new options to `git help rev-list`. Documentation/git-rev-list.txt | 1 + Documentation/rev-list-options.txt | 10 ++++++++++ builtin/rev-list.c | 19 ++++++++++++++++++- t/t6000-rev-list-misc.sh | 20 ++++++++++++++++++++ 4 files changed, 49 insertions(+), 1 deletion(-) diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt index 88609ff435..9392760b25 100644 --- a/Documentation/git-rev-list.txt +++ b/Documentation/git-rev-list.txt @@ -48,6 +48,7 @@ SYNOPSIS [ --date=<format>] [ [ --objects | --objects-edge | --objects-edge-aggressive ] [ --unpacked ] + [ --object-names | --no-object-names ] [ --filter=<filter-spec> [ --filter-print-omitted ] ] ] [ --missing=<missing-action> ] [ --pretty | --header ] diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 71a1fcc093..286fc163f1 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -708,6 +708,16 @@ ifdef::git-rev-list[] Only useful with `--objects`; print the object IDs that are not in packs. +--object-names:: + Only useful with `--objects`; print the names of the object IDs + that are found. This is the default behavior. + +--no-object-names:: + Only useful with `--objects`; does not print the names of the object + IDs that are found. This inverts `--object-names`. This flag allows + the output to be more easily parsed by commands such as + linkgit:git-cat-file[1]. + --filter=<filter-spec>:: Only useful with one of the `--objects*`; omits objects (usually blobs) from the list of printed objects. The '<filter-spec>' diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 660172b014..301ccb970b 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -49,6 +49,7 @@ static const char rev_list_usage[] = " --objects | --objects-edge\n" " --unpacked\n" " --header | --pretty\n" +" --[no-]object-names\n" " --abbrev=<n> | --no-abbrev\n" " --abbrev-commit\n" " --left-right\n" @@ -75,6 +76,9 @@ enum missing_action { }; static enum missing_action arg_missing_action; +/* display only the oid of each object encountered */ +static int arg_show_object_names = 1; + #define DEFAULT_OIDSET_SIZE (16*1024) static void finish_commit(struct commit *commit); @@ -255,7 +259,10 @@ static void show_object(struct object *obj, const char *name, void *cb_data) display_progress(progress, ++progress_counter); if (info->flags & REV_LIST_QUIET) return; - show_object_with_name(stdout, obj, name); + if (arg_show_object_names) + show_object_with_name(stdout, obj, name); + else + printf("%s\n", oid_to_hex(&obj->oid)); } static void show_edge(struct commit *commit) @@ -484,6 +491,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) if (skip_prefix(arg, "--missing=", &arg)) continue; /* already handled above */ + if (!strcmp(arg, ("--no-object-names"))) { + arg_show_object_names = 0; + continue; + } + + if (!strcmp(arg, ("--object-names"))) { + arg_show_object_names = 1; + continue; + } + usage(rev_list_usage); } diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh index 0507999729..52a9e38d66 100755 --- a/t/t6000-rev-list-misc.sh +++ b/t/t6000-rev-list-misc.sh @@ -48,6 +48,26 @@ test_expect_success 'rev-list --objects with pathspecs and copied files' ' ! grep one output ' +test_expect_success 'rev-list --objects --no-object-names has no space/names' ' + git rev-list --objects --no-object-names HEAD >output && + ! grep wanted_file output && + ! grep unwanted_file output && + ! grep " " output +' + +test_expect_success 'rev-list --objects --no-object-names works with cat-file' ' + git rev-list --objects --no-object-names --all >list-output && + git cat-file --batch-check <list-output >cat-output && + ! grep missing cat-output +' + +test_expect_success '--no-object-names and --object-names are last-one-wins' ' + git rev-list --objects --no-object-names --object-names --all >output && + grep wanted_file output && + git rev-list --objects --object-names --no-object-names --all >output && + ! grep wanted_file output +' + test_expect_success 'rev-list A..B and rev-list ^A B are the same' ' git commit --allow-empty -m another && git tag -a -m "annotated" v1.0 && -- 2.22.0.410.gd8fdbe21b5-goog ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v5] rev-list: teach --no-object-names to enable piping 2019-06-19 20:56 ` [PATCH v5] " Emily Shaffer @ 2019-06-19 21:38 ` Jeff King 0 siblings, 0 replies; 22+ messages in thread From: Jeff King @ 2019-06-19 21:38 UTC (permalink / raw) To: Emily Shaffer; +Cc: git, Junio C Hamano, Eric Sunshine, Jonathan Nieder On Wed, Jun 19, 2019 at 01:56:56PM -0700, Emily Shaffer wrote: > Allow easier parsing by cat-file by giving rev-list an option to print > only the OID of a non-commit object without any additional information. > This is a short-term shim; later on, rev-list should be taught how to > print the types of objects it finds in a format similar to cat-file's. > [...] I missed some of the intermediate rounds, but fortunately Junio already said everything I was going to. :) This version looks good to me, though with one minor nit: > diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt > index 88609ff435..9392760b25 100644 > --- a/Documentation/git-rev-list.txt > +++ b/Documentation/git-rev-list.txt > @@ -48,6 +48,7 @@ SYNOPSIS > [ --date=<format>] > [ [ --objects | --objects-edge | --objects-edge-aggressive ] > [ --unpacked ] > + [ --object-names | --no-object-names ] > [ --filter=<filter-spec> [ --filter-print-omitted ] ] ] > [ --missing=<missing-action> ] > [ --pretty | --header ] Here you put --object-names along with the --objects. Which kind of makes sense, but everything else in that block is about choosing _which_ commits to show. In the short help, you put it near --pretty: > @@ -49,6 +49,7 @@ static const char rev_list_usage[] = > " --objects | --objects-edge\n" > " --unpacked\n" > " --header | --pretty\n" > +" --[no-]object-names\n" > " --abbrev=<n> | --no-abbrev\n" > " --abbrev-commit\n" > " --left-right\n" which I think makes more sense. I think maybe you were trying to imply that "--object-names" is not useful unless you're also using "--objects". Which is true, but I'm not sure it's obvious from that mass of brackets (and I think is sufficiently covered in the actual option descriptions you give later). > +test_expect_success '--no-object-names and --object-names are last-one-wins' ' > + git rev-list --objects --no-object-names --object-names --all >output && > + grep wanted_file output && > + git rev-list --objects --object-names --no-object-names --all >output && > + ! grep wanted_file output > +' We don't generally test this behavior for each option, since it would lead to a ton of uninteresting tests (and parse-options generally just handles it). But after our discussion about --no-abbrev, I can see how you might be more interested in the topic. :) So I'm OK with it either way. -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-06-19 21:38 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-07 22:59 [PATCH] revision: remove stray whitespace when name empty Emily Shaffer 2019-06-08 0:42 ` Eric Sunshine 2019-06-12 19:23 ` Emily Shaffer 2019-06-09 13:00 ` Jeff King 2019-06-10 16:29 ` Junio C Hamano 2019-06-12 19:37 ` Emily Shaffer 2019-06-13 15:20 ` Jeff King 2019-06-13 21:51 ` [PATCH v2] rev-list: teach --oid-only to enable piping Emily Shaffer 2019-06-14 16:07 ` Jeff King 2019-06-14 20:25 ` Junio C Hamano 2019-06-14 23:18 ` Emily Shaffer 2019-06-14 23:29 ` Emily Shaffer 2019-06-19 21:24 ` Jeff King 2019-06-14 23:48 ` [PATCH v3] rev-list: teach --no-object-names " Emily Shaffer 2019-06-17 22:32 ` Junio C Hamano 2019-06-18 22:08 ` Emily Shaffer 2019-06-18 22:29 ` [PATCH v4] " Emily Shaffer 2019-06-19 14:08 ` Junio C Hamano 2019-06-19 19:31 ` Emily Shaffer 2019-06-19 21:30 ` Jeff King 2019-06-19 20:56 ` [PATCH v5] " Emily Shaffer 2019-06-19 21:38 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.