On Sun, Jun 26, 2022 at 11:16:51AM +0200, Omar Polo wrote:
> to complement yesterday's diff about -Wmissing-prototypes, here's an
> attempt to build got with -Wwrite-strings.  The purpose is to enforce
> some `const's here and there, in the hope to make some parts clearer.
> 
> It correctly picked up a few places where we were using "char *" (or
> "void *") but passing "const" pointers, and I'd also argue that in the
> case of got_diff_objects_as_trees it's actually better to have the
> pointers correctly marked as const.
> 
> There are also two downsides however:
> 
>  - i need to add a cast in got_dial_ssh.  I don't know how to trackle
>    argv without extra casts with -Wwrite-strings.  The compiler
>    complained that assigning to argv[x] a const char * was loosing
>    const, but execv wants a "char *const *", and I can't have argv
>    declared as `char *const argv[11]' either.  (why execv has such
>    signature?  meh)
> 
>  - got_path_find_prog becomes more complex.  it mixes const and
>    non-const char pointers, but it's carefully designed to not modify
>    the const strings.  The compiler isn't smart enough to notice it (nor
>    that it should.)  A possible solution in the long-term could be to
>    maybe drop this function: it's just used to find the editor to exec;
>    we could just try to exec $VISUAL, $EDITOR and /bin/ed in cascade and
>    use the first that works.  not for today tho :)
> 
> so i'm not sure if we want this in or to just backport some changes and
> call it a day.
> 
> P.S.: tracey: I'm probably making your life in the gotwebd branch harder
> by adding these warnings (that turns into errors thanks to -Werror in
> non release builds.)  I'll try to see if gotwebd breaks with these
> warnings enabled and cook a diff soon(tm) ;)
> 

I'm fine with breakage. Let's see what we get.

> -----------------------------------------------
> commit c02440c56013c8818bf510752cfed899a4c8abd6 (wwritestr)
> from: Omar Polo <op@omarpolo.com>
> date: Sun Jun 26 09:01:05 2022 UTC
>  
>  build with -Wwrite-strings
>  
> diff 3d589bee0bbbe812bb91f3b0284fbf2596304132 c02440c56013c8818bf510752cfed899a4c8abd6
> commit - 3d589bee0bbbe812bb91f3b0284fbf2596304132
> commit + c02440c56013c8818bf510752cfed899a4c8abd6
> blob - ef99224d337170364b2e5458bf6e2a3206c3e05a
> blob + 2b72df1a3c9e149cff31460ae7090698a3416a61
> --- Makefile.inc
> +++ Makefile.inc
> @@ -11,7 +11,7 @@ LIBEXECDIR ?= ${PREFIX}/libexec
>  MANDIR ?= ${PREFIX}/man/man
>  .else
>  CFLAGS += -Werror -Wall -Wstrict-prototypes -Wmissing-prototypes
> -CFLAGS += -Wunused-variable
> +CFLAGS += -Wwrite-strings -Wunused-variable
>  PREFIX ?= ${HOME}
>  BINDIR ?= ${PREFIX}/bin
>  LIBEXECDIR ?= ${BINDIR}
> blob - e1fc7e795e917a7be22aff253610e8c66eef455c
> blob + a6bd0096cd2d6e29e21d56f19ed44cd2365eb1ba
> --- gotweb/gotweb.c
> +++ gotweb/gotweb.c
> @@ -237,7 +237,7 @@ struct gw_query_action {
>  	unsigned int		 func_id;
>  	const char		*func_name;
>  	const struct got_error	*(*func_main)(struct gw_trans *);
> -	char			*template;
> +	const char		*template;
>  };
>  
>  enum gw_query_actions {
> @@ -2716,11 +2716,11 @@ gw_get_time_str(char **repo_age, time_t committer_time
>  {
>  	struct tm tm;
>  	time_t diff_time;
> -	char *years = "years ago", *months = "months ago";
> -	char *weeks = "weeks ago", *days = "days ago", *hours = "hours ago";
> -	char *minutes = "minutes ago", *seconds = "seconds ago";
> -	char *now = "right now";
> -	char *s;
> +	const char *years = "years ago", *months = "months ago";
> +	const char *weeks = "weeks ago", *days = "days ago", *hours = "hours ago";
> +	const char *minutes = "minutes ago", *seconds = "seconds ago";
> +	const char *now = "right now";
> +	const char *s;
>  	char datebuf[29];
>  
>  	*repo_age = NULL;
> @@ -4763,7 +4763,7 @@ static const struct got_error *
>  gw_colordiff_line(struct gw_trans *gw_trans, char *buf)
>  {
>  	const struct got_error *error = NULL;
> -	char *color = NULL;
> +	const char *color = NULL;
>  	enum kcgi_err kerr = KCGI_OK;
>  
>  	if (strncmp(buf, "-", 1) == 0)
> blob - 5dbe18b856a53a58bee722d773249a93e91993a8
> blob + 4f537113e196997bfae7b568a9d90ab8fe9e77b0
> --- include/got_diff.h
> +++ include/got_diff.h
> @@ -166,7 +166,7 @@ const struct got_error *got_diff_objects_as_blobs(off_
>   */
>  const struct got_error *got_diff_objects_as_trees(off_t **, size_t *,
>      FILE *, FILE *, struct got_object_id *, struct got_object_id *,
> -    struct got_pathlist_head *, char *, char *, int, int, int,
> +    struct got_pathlist_head *, const char *, const char *, int, int, int,
>      struct got_repository *, FILE *);
>  
>  /*
> blob - 4f688a96c44c72c60f3157e3d1293fcca25111c6
> blob + b7ce464748cabe3f0823af08f8961d87d13e0c7a
> --- lib/dial.c
> +++ lib/dial.c
> @@ -67,7 +67,7 @@ got_dial_apply_unveil(const char *proto)
>  }
>  
>  static int
> -hassuffix(char *base, char *suf)
> +hassuffix(const char *base, const char *suf)
>  {
>  	int nb, ns;
>  
> @@ -207,7 +207,7 @@ got_dial_ssh(pid_t *newpid, int *newfd, const char *ho
>  	const struct got_error *error = NULL;
>  	int pid, pfd[2];
>  	char cmd[64];
> -	char *argv[11];
> +	const char *argv[11];
>  	int i = 0, j;
>  
>  	*newpid = -1;
> @@ -252,7 +252,7 @@ got_dial_ssh(pid_t *newpid, int *newfd, const char *ho
>  		n = snprintf(cmd, sizeof(cmd), "git-%s-pack", direction);
>  		if (n < 0 || n >= ssizeof(cmd))
>  			err(1, "snprintf");
> -		if (execv(GOT_DIAL_PATH_SSH, argv) == -1)
> +		if (execv(GOT_DIAL_PATH_SSH, (char *const *)argv) == -1)
>  			err(1, "execv");
>  		abort(); /* not reached */
>  	} else {
> blob - 8337d25f3e5608a25af5b7e4a9940d9bcb2ceba5
> blob + 773169c32d3cb119ac330201afe43d53b115085a
> --- lib/diff.c
> +++ lib/diff.c
> @@ -62,7 +62,7 @@ diff_blobs(off_t **line_offsets, size_t *nlines,
>  	const struct got_error *err = NULL, *free_err;
>  	char hex1[SHA1_DIGEST_STRING_LENGTH];
>  	char hex2[SHA1_DIGEST_STRING_LENGTH];
> -	char *idstr1 = NULL, *idstr2 = NULL;
> +	const char *idstr1 = NULL, *idstr2 = NULL;
>  	off_t size1, size2;
>  	struct got_diffreg_result *result;
>  	off_t outoff = 0;
> @@ -220,7 +220,7 @@ diff_blob_file(struct got_diffreg_result **resultp,
>  {
>  	const struct got_error *err = NULL, *free_err;
>  	char hex1[SHA1_DIGEST_STRING_LENGTH];
> -	char *idstr1 = NULL;
> +	const char *idstr1 = NULL;
>  	struct got_diffreg_result *result = NULL;
>  
>  	if (resultp)
> @@ -904,9 +904,9 @@ show_object_id(off_t **line_offsets, size_t *nlines, c
>  static const struct got_error *
>  diff_objects_as_trees(off_t **line_offsets, size_t *nlines,
>      FILE *f1, FILE *f2, struct got_object_id *id1, struct got_object_id *id2,
> -    struct got_pathlist_head *paths,
> -    char *label1, char *label2, int diff_context, int ignore_whitespace,
> -    int force_text_diff, struct got_repository *repo, FILE *outfile)
> +    struct got_pathlist_head *paths, const char *label1, const char *label2,
> +    int diff_context, int ignore_whitespace, int force_text_diff,
> +    struct got_repository *repo, FILE *outfile)
>  {
>  	const struct got_error *err;
>  	struct got_tree_object *tree1 = NULL, *tree2 = NULL;
> @@ -960,9 +960,9 @@ done:
>  const struct got_error *
>  got_diff_objects_as_trees(off_t **line_offsets, size_t *nlines,
>      FILE *f1, FILE *f2, struct got_object_id *id1, struct got_object_id *id2,
> -    struct got_pathlist_head *paths,
> -    char *label1, char *label2, int diff_context, int ignore_whitespace,
> -    int force_text_diff, struct got_repository *repo, FILE *outfile)
> +    struct got_pathlist_head *paths, const char *label1, const char *label2,
> +    int diff_context, int ignore_whitespace, int force_text_diff,
> +    struct got_repository *repo, FILE *outfile)
>  {
>  	const struct got_error *err;
>  	char *idstr = NULL;
> blob - 70383527274d9e8c501d2480b0479657ced378b7
> blob + e44ac0a4e3f7fe9caf5b350a89c7e5c5beba00be
> --- lib/diff_output.c
> +++ lib/diff_output.c
> @@ -262,7 +262,7 @@ diff_output_match_function_prototype(char *prototype, 
>  	struct diff_atom *start_atom, *atom;
>  	const struct diff_data *data;
>  	unsigned char buf[DIFF_FUNCTION_CONTEXT_SIZE];
> -	char *state = NULL;
> +	const char *state = NULL;
>  	int rc, i, ch;
>  
>  	if (result->left->atoms.len > 0 && cc->left.start > 0) {
> blob - 16d1a758fc03fec245de789888abc9cb072f408d
> blob + 384b6d4beb4ecb887c5ba512e0dd1f882bb02d4b
> --- lib/pack_create.c
> +++ lib/pack_create.c
> @@ -1829,7 +1829,7 @@ done:
>  }
>  
>  static const struct got_error *
> -hwrite(FILE *f, void *buf, off_t len, SHA1_CTX *ctx)
> +hwrite(FILE *f, const void *buf, off_t len, SHA1_CTX *ctx)
>  {
>  	size_t n;
>  
> blob - 19dc9c983445c68dbea181cf6bfabcfd1ee9d949
> blob + e3d7799acfdd15ffc55d19126b7c7e27a696bee6
> --- lib/path.c
> +++ lib/path.c
> @@ -458,10 +458,11 @@ const struct got_error *
>  got_path_find_prog(char **filename, const char *prog)
>  {
>  	const struct got_error *err = NULL;
> +	const char *path;
>  	char *p;
>  	int len;
>  	struct stat sbuf;
> -	char *path, *pathcpy;
> +	char *pathcpy, *dup = NULL;
>  
>  	*filename = NULL;
>  
> @@ -480,19 +481,22 @@ got_path_find_prog(char **filename, const char *prog)
>  		return NULL;
>  	}
>  
> -	if ((path = strdup(path)) == NULL)
> +	if ((dup = strdup(path)) == NULL)
>  		return got_error_from_errno("strdup");
> -	pathcpy = path;
> +	pathcpy = dup;
>  
>  	while ((p = strsep(&pathcpy, ":")) != NULL) {
> -		if (*p == '\0')
> -			p = ".";
> +		const char *d;
>  
>  		len = strlen(p);
>  		while (len > 0 && p[len-1] == '/')
>  			p[--len] = '\0';	/* strip trailing '/' */
>  
> -		if (asprintf(filename, "%s/%s", p, prog) == -1) {
> +		d = p;
> +		if (*d == '\0')
> +			d = ".";
> +
> +		if (asprintf(filename, "%s/%s", d, prog) == -1) {
>  			err = got_error_from_errno("asprintf");
>  			break;
>  		}
> @@ -503,7 +507,7 @@ got_path_find_prog(char **filename, const char *prog)
>  		*filename = NULL;
>  		continue;
>  	}
> -	free(path);
> +	free(dup);
>  	return err;
>  }
>  
> blob - 8616b4e5f3930bdff53402d97880b3ae74a6fa5f
> blob + de71b5845e334bff8e56366f910514f149cf3519
> --- lib/reference.c
> +++ lib/reference.c
> @@ -998,8 +998,8 @@ got_ref_list(struct got_reflist_head *refs, struct got
>  {
>  	const struct got_error *err;
>  	char *packed_refs_path, *path_refs = NULL;
> -	char *abs_namespace = NULL;
> -	char *buf = NULL, *ondisk_ref_namespace = NULL;
> +	char *abs_namespace = NULL, *buf = NULL;
> +	const char *ondisk_ref_namespace = NULL;
>  	char *line = NULL;
>  	FILE *f = NULL;
>  	struct got_reference *ref;
> blob - 9d84105f23ebcedee81341088dc0fd38cb0c4261
> blob + c0e71877fd2d42e62b12140d90a427fecf8d0cee
> --- regress/fetch/fetch_test.c
> +++ regress/fetch/fetch_test.c
> @@ -46,7 +46,7 @@ static int verbose;
>  static int quiet;
>  
>  static void
> -test_printf(char *fmt, ...)
> +test_printf(const char *fmt, ...)
>  {
>  	va_list ap;
>  
> blob - e8001a520fd4f5763bc38dbfb1c94be3fdd0e531
> blob + bbf71a79eb4fa68027df6f06b7a1d9926000a518
> --- regress/path/path_test.c
> +++ regress/path/path_test.c
> @@ -34,7 +34,7 @@ static int verbose;
>  static int quiet;
>  
>  static void
> -test_printf(char *fmt, ...)
> +test_printf(const char *fmt, ...)
>  {
>  	va_list ap;
>  
> 
> 

-- 

Tracey Emery