| Message ID | 20181030232337.GC32038@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] t1450: check large blob in trailing-garbage test | expand |
On Tue, Oct 30, 2018 at 07:23:38PM -0400, Jeff King wrote: > There are three ways to convince cat-file to stream a blob: > > - cat-file -p $blob > > - cat-file blob $blob > > - echo $batch | cat-file --batch > > In the first two, we simply exit with the error code of > streaw_blob_to_fd(). That means that an error will cause us > to exit with "-1" (which we try to avoid) without printing > any kind of error message (which is confusing to the user). > > Instead, let's match the third case, which calls die() on an > error. Unfortunately we cannot be more specific, as > stream_blob_to_fd() does not tell us whether the problem was > on reading (e.g., a corrupt object) or on writing (e.g., > ENOSPC). That might be an opportunity for future work, but > for now we will at least exit with a sane message and exit > code. > > Signed-off-by: Jeff King <peff@peff.net> > --- > builtin/cat-file.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/builtin/cat-file.c b/builtin/cat-file.c > index 8d97c84725..0d403eb77d 100644 > --- a/builtin/cat-file.c > +++ b/builtin/cat-file.c > @@ -50,6 +50,13 @@ static int filter_object(const char *path, unsigned mode, > return 0; > } > > +static int stream_blob(const struct object_id *oid) Sorry for nit-picking: could this be renamed into stream_blob_to_stdout() ? > +{ > + if (stream_blob_to_fd(1, oid, NULL, 0)) And I wonder if we could make things clearer: s/1/STDOUT_FILENO/ (Stolen from fast-import.c) > + die("unable to stream %s to stdout", oid_to_hex(oid)); > + return 0; > +} > + []
Torsten Bögershausen <tboegi@web.de> writes: >> +static int stream_blob(const struct object_id *oid) > > Sorry for nit-picking: > could this be renamed into stream_blob_to_stdout() ? I think that name makes sense, even though stream_blob() is just fine for a fuction that takes a single parameter oid, as there is no other sane choice than streaming to the standard output stream the blob data. >> +{ >> + if (stream_blob_to_fd(1, oid, NULL, 0)) > > And I wonder if we could make things clearer: > s/1/STDOUT_FILENO/ What would benefit from symbolic constant more in that function call may be CAN_SEEK thing, but s/1/STDOUT_FILENO/ adds negative value to that line, I would think. The name of the function already makes it clear this is sending the output to a file descriptor, and an integer 1 that specifies a file descriptor cannot mean anything other than the standard output stream.
On Wed, Oct 31, 2018 at 11:23:48PM +0900, Junio C Hamano wrote: > Torsten Bögershausen <tboegi@web.de> writes: > > >> +static int stream_blob(const struct object_id *oid) > > > > Sorry for nit-picking: > > could this be renamed into stream_blob_to_stdout() ? > > I think that name makes sense, even though stream_blob() is just > fine for a fuction that takes a single parameter oid, as there is no > other sane choice than streaming to the standard output stream the > blob data. I was trying to keep the name small since it is a static-local convenience helper. I'd rather write it as: stream_blob(1, oid); than change the name. ;) > >> +{ > >> + if (stream_blob_to_fd(1, oid, NULL, 0)) > > > > And I wonder if we could make things clearer: > > s/1/STDOUT_FILENO/ > > What would benefit from symbolic constant more in that function call > may be CAN_SEEK thing, but s/1/STDOUT_FILENO/ adds negative value to > that line, I would think. The name of the function already makes it > clear this is sending the output to a file descriptor, and an > integer 1 that specifies a file descriptor cannot mean anything > other than the standard output stream. Yes, I'd agree (there are very few cases where I think STDOUT_FILENO actually increases the readability, since it is usually pretty clear from the context when something is a descriptor). -Peff
On Tue, Oct 30, 2018 at 07:23:38PM -0400, Jeff King wrote: > There are three ways to convince cat-file to stream a blob: > > - cat-file -p $blob > > - cat-file blob $blob > > - echo $batch | cat-file --batch > > In the first two, we simply exit with the error code of > streaw_blob_to_fd(). That means that an error will cause us Your "m" got confused and ended up upside-down. > to exit with "-1" (which we try to avoid) without printing > any kind of error message (which is confusing to the user). > > Instead, let's match the third case, which calls die() on an > error. Unfortunately we cannot be more specific, as > stream_blob_to_fd() does not tell us whether the problem was > on reading (e.g., a corrupt object) or on writing (e.g., > ENOSPC). That might be an opportunity for future work, but > for now we will at least exit with a sane message and exit > code. > > Signed-off-by: Jeff King <peff@peff.net>
On Wed, Oct 31, 2018 at 01:38:59PM -0400, Eric Sunshine wrote: > On Tue, Oct 30, 2018 at 07:23:38PM -0400, Jeff King wrote: > > There are three ways to convince cat-file to stream a blob: > > > > - cat-file -p $blob > > > > - cat-file blob $blob > > > > - echo $batch | cat-file --batch > > > > In the first two, we simply exit with the error code of > > streaw_blob_to_fd(). That means that an error will cause us > > Your "m" got confused and ended up upside-down. Heh. I'm not sure how I managed that. They're not exactly next to each other on a qwerty keyboard. -Peff
diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 8d97c84725..0d403eb77d 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -50,6 +50,13 @@ static int filter_object(const char *path, unsigned mode, return 0; } +static int stream_blob(const struct object_id *oid) +{ + if (stream_blob_to_fd(1, oid, NULL, 0)) + die("unable to stream %s to stdout", oid_to_hex(oid)); + return 0; +} + static int cat_one_file(int opt, const char *exp_type, const char *obj_name, int unknown_type) { @@ -132,7 +139,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, } if (type == OBJ_BLOB) - return stream_blob_to_fd(1, &oid, NULL, 0); + return stream_blob(&oid); buf = read_object_file(&oid, &type, &size); if (!buf) die("Cannot read object %s", obj_name); @@ -155,7 +162,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, oidcpy(&blob_oid, &oid); if (oid_object_info(the_repository, &blob_oid, NULL) == OBJ_BLOB) - return stream_blob_to_fd(1, &blob_oid, NULL, 0); + return stream_blob(&blob_oid); /* * we attempted to dereference a tag to a blob * and failed; there may be new dereference @@ -319,8 +326,9 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d BUG("invalid cmdmode: %c", opt->cmdmode); batch_write(opt, contents, size); free(contents); - } else if (stream_blob_to_fd(1, oid, NULL, 0) < 0) - die("unable to stream %s to stdout", oid_to_hex(oid)); + } else { + stream_blob(oid); + } } else { enum object_type type;
There are three ways to convince cat-file to stream a blob: - cat-file -p $blob - cat-file blob $blob - echo $batch | cat-file --batch In the first two, we simply exit with the error code of streaw_blob_to_fd(). That means that an error will cause us to exit with "-1" (which we try to avoid) without printing any kind of error message (which is confusing to the user). Instead, let's match the third case, which calls die() on an error. Unfortunately we cannot be more specific, as stream_blob_to_fd() does not tell us whether the problem was on reading (e.g., a corrupt object) or on writing (e.g., ENOSPC). That might be an opportunity for future work, but for now we will at least exit with a sane message and exit code. Signed-off-by: Jeff King <peff@peff.net> --- builtin/cat-file.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)