diff mbox series

[2/2,GSOC] cat-file: merge two block into one

Message ID f02c1144d916bd36bd32069e439cfc08b2a7898a.1622558157.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series cat-file: fix --batch report changed-type bug | expand

Commit Message

ZheNing Hu June 1, 2021, 2:35 p.m. UTC 
From: ZheNing Hu <adlternative@gmail.com>

 Because the two "if (opt->all_objects)" block
 are redundant, merge them into one, to provide
 better readability.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
 builtin/cat-file.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

Jeff King June 1, 2021, 3:55 p.m. UTC | #1 
On Tue, Jun 01, 2021 at 02:35:57PM +0000, ZheNing Hu via GitGitGadget wrote:

> From: ZheNing Hu <adlternative@gmail.com>
> 
>  Because the two "if (opt->all_objects)" block
>  are redundant, merge them into one, to provide
>  better readability.

Funny indentation of the commit message. :)

I think this is worth doing, and I agree the end-result is easier to
read. Really minor nit, but I probably wouldn't have said "redundant"
here. The conditionals themselves are redundant, but not the blocks.
Maybe: There are two "if (opt->all_objects)" blocks next to each other;
merge them into one to provide better readability.

(not a huge deal, as I think seeing the patch helps explain what is
going on. But again, just trying to offer polishing advice for future
patches)
ZheNing Hu June 2, 2021, 1:27 p.m. UTC | #2 
Jeff King <peff@peff.net> 于2021年6月1日周二 下午11:55写道：
>
> On Tue, Jun 01, 2021 at 02:35:57PM +0000, ZheNing Hu via GitGitGadget wrote:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> >  Because the two "if (opt->all_objects)" block
> >  are redundant, merge them into one, to provide
> >  better readability.
>
> Funny indentation of the commit message. :)
>

Yes, there is a small space that I didn’t notice. :)

> I think this is worth doing, and I agree the end-result is easier to
> read. Really minor nit, but I probably wouldn't have said "redundant"
> here. The conditionals themselves are redundant, but not the blocks.
> Maybe: There are two "if (opt->all_objects)" blocks next to each other;
> merge them into one to provide better readability.
>

Yeah, your expression is correct.

> (not a huge deal, as I think seeing the patch helps explain what is
> going on. But again, just trying to offer polishing advice for future
> patches)

Thanks.
--
ZheNing Hu
diff mbox series

Patch

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 02461bb5ea6f..243fe6844bc6 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -520,14 +520,11 @@  static int batch_objects(struct batch_options *opt)
 		data.info.typep = &data.type;
 
 	if (opt->all_objects) {
+		struct object_cb_data cb;
 		struct object_info empty = OBJECT_INFO_INIT;
 
 		if (!memcmp(&data.info, &empty, sizeof(empty)))
 			data.skip_object_info = 1;
-	}
-
-	if (opt->all_objects) {
-		struct object_cb_data cb;
 
 		if (has_promisor_remote())
 			warning("This repository uses promisor remotes. Some objects may not be loaded.");