git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 1/2] describe: do not use cmd_*() as a subroutine
  2017-10-10  4:06 ` [PATCH 0/2] Do not call cmd_*() as a subroutine Junio C Hamano
@ 2017-10-10  4:06   ` " Junio C Hamano
  2017-10-10 13:43     ` SZEDER Gábor
  0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2017-10-10  4:06 UTC (permalink / raw)
  To: git

The cmd_foo() function is a moral equivalent of 'main' for a Git
subcommand 'git foo', and as such, it is allowed to do many things
that make it unsuitable to be called as a subroutine, including

 - call exit(3) to terminate the process;

 - allocate resource held and used throughout its lifetime, without
   releasing it upon return/exit;

 - rely on global variables being initialized at program startup,
   and update them as needed, making another clean invocation of the
   function impossible.

The call to cmd_diff_index() "git describe" makes has been working
by accident that the function did not call exit(3); it sets a bad
precedent for people to cut and paste.

We could invoke it via the run_command() interface, but the diff
family of commands have helper functions in diff-lib.c that are
meant to be usable as subroutines, and using the latter does not
make the resulting code all that longer.  Use it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/describe.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 89ea1cdd60..50263759cb 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -7,12 +7,12 @@
 #include "builtin.h"
 #include "exec_cmd.h"
 #include "parse-options.h"
+#include "revision.h"
 #include "diff.h"
 #include "hashmap.h"
 #include "argv-array.h"
 #include "run-command.h"
 
-#define SEEN		(1u << 0)
 #define MAX_TAGS	(FLAG_BITS - 1)
 
 static const char * const describe_usage[] = {
@@ -532,7 +532,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			}
 		} else if (dirty) {
 			static struct lock_file index_lock;
-			int fd;
+			struct rev_info revs;
+			struct argv_array args = ARGV_ARRAY_INIT;
+			int fd, result;
 
 			read_cache_preload(NULL);
 			refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,
@@ -541,8 +543,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			if (0 <= fd)
 				update_index_if_able(&the_index, &index_lock);
 
-			if (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,
-					    diff_index_args, prefix))
+			init_revisions(&revs, prefix);
+			argv_array_pushv(&args, diff_index_args);
+			if (setup_revisions(args.argc, args.argv, &revs, NULL) != 1)
+				BUG("malformed internal diff-index command line");
+			result = run_diff_index(&revs, 0);
+
+			if (!diff_result_code(&revs.diffopt, result))
 				suffix = NULL;
 			else
 				suffix = dirty;
-- 
2.15.0-rc0-203-g4c8d0e28b1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH 1/2] describe: do not use cmd_*() as a subroutine
  2017-10-10  4:06   ` [PATCH 1/2] describe: do not use " Junio C Hamano
@ 2017-10-10 13:43     ` SZEDER Gábor
  2017-10-11  6:00       ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: SZEDER Gábor @ 2017-10-10 13:43 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: SZEDER Gábor, git


> The cmd_foo() function is a moral equivalent of 'main' for a Git
> subcommand 'git foo', and as such, it is allowed to do many things
> that make it unsuitable to be called as a subroutine, including
> 
>  - call exit(3) to terminate the process;
> 
>  - allocate resource held and used throughout its lifetime, without
>    releasing it upon return/exit;
> 
>  - rely on global variables being initialized at program startup,
>    and update them as needed, making another clean invocation of the
>    function impossible.
> 
> The call to cmd_diff_index() "git describe" makes has been working
> by accident that the function did not call exit(3); it sets a bad
> precedent for people to cut and paste.

The subject implies to me that this patch eliminates all cmd_*() calls
in builtin/describe.c, but a call to cmd_name_rev() still remains.

However, that call is special in the sense that cmd_describe() returns
immediately thereafter, so none of the above three points are an issue
there.  Someone might argue that it still sets a bad precedent, but I
won't :)  To avoid the direct cmd_name_rev() call we would have to use
run_command(), because there are no libified helper functions
available to do the job, adding the overhead of a fork()+exec(),
though only once or nonce, depending on cmdline options.

Maybe you already considered all this WRT that cmd_name_rev() call, I
don't know.  In any case, I think at least the subject line should
spell out cmd_diff_index().


Gábor


> We could invoke it via the run_command() interface, but the diff
> family of commands have helper functions in diff-lib.c that are
> meant to be usable as subroutines, and using the latter does not
> make the resulting code all that longer.  Use it.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/describe.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 89ea1cdd60..50263759cb 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -7,12 +7,12 @@
>  #include "builtin.h"
>  #include "exec_cmd.h"
>  #include "parse-options.h"
> +#include "revision.h"
>  #include "diff.h"
>  #include "hashmap.h"
>  #include "argv-array.h"
>  #include "run-command.h"
>  
> -#define SEEN		(1u << 0)
>  #define MAX_TAGS	(FLAG_BITS - 1)
>  
>  static const char * const describe_usage[] = {
> @@ -532,7 +532,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>  			}
>  		} else if (dirty) {
>  			static struct lock_file index_lock;
> -			int fd;
> +			struct rev_info revs;
> +			struct argv_array args = ARGV_ARRAY_INIT;
> +			int fd, result;
>  
>  			read_cache_preload(NULL);
>  			refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,
> @@ -541,8 +543,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>  			if (0 <= fd)
>  				update_index_if_able(&the_index, &index_lock);
>  
> -			if (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,
> -					    diff_index_args, prefix))
> +			init_revisions(&revs, prefix);
> +			argv_array_pushv(&args, diff_index_args);
> +			if (setup_revisions(args.argc, args.argv, &revs, NULL) != 1)
> +				BUG("malformed internal diff-index command line");
> +			result = run_diff_index(&revs, 0);
> +
> +			if (!diff_result_code(&revs.diffopt, result))
>  				suffix = NULL;
>  			else
>  				suffix = dirty;
> -- 
> 2.15.0-rc0-203-g4c8d0e28b1

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH 1/2] describe: do not use cmd_*() as a subroutine
  2017-10-10 13:43     ` SZEDER Gábor
@ 2017-10-11  6:00       ` Junio C Hamano
  0 siblings, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2017-10-11  6:00 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: git

SZEDER Gábor <szeder.dev@gmail.com> writes:

> Maybe you already considered all this WRT that cmd_name_rev() call, I
> don't know.  In any case, I think at least the subject line should
> spell out cmd_diff_index().

Perhaps.  The intent was to eradicate cmd_*() used as a subroutine,
so I'd rather opt to explain why name_rev() that does not return is
OK in this case in the log message without changing anything else.

Thanks for looking it over.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-05 10:03 [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1() Junio C Hamano
2017-10-10  4:06 ` [PATCH 0/2] Do not call cmd_*() as a subroutine Junio C Hamano
2017-10-10  4:06   ` [PATCH 1/2] describe: do not use " Junio C Hamano
2017-10-10 13:43     ` SZEDER Gábor
2017-10-11  6:00       ` Junio C Hamano

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox