* [PATCH 1/5] git-compat-util: add xstrdup_or_null helper
2015-01-13 1:54 ` Jeff King
@ 2015-01-13 1:57 ` Jeff King
2015-01-13 2:21 ` Jonathan Nieder
2015-01-13 1:58 ` [PATCH 2/5] builtin/apply.c: use xstrdup_or_null instead of null_strdup Jeff King
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Jeff King @ 2015-01-13 1:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lukas Fleischer, git
It's a common idiom to duplicate a string if it is non-NULL,
or pass a literal NULL through. This is already a one-liner
in C, but you do have to repeat the name of the string
twice. So if there's a function call, you must write:
const char *x = some_fun(...);
return x ? xstrdup(x) : NULL;
instead of (with this patch) just:
return xstrdup_or_null(some_fun(...));
Signed-off-by: Jeff King <peff@peff.net>
---
This example is the heart of the readability question to me. It is nice
to avoid the temporary, which makes the code more direct. But it also
sticks some_fun(...) inside another function, which is a little harder
to read.
git-compat-util.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index dcecd85..8157eb2 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -675,6 +675,11 @@ extern char *xgetcwd(void);
#define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
+static inline char *xstrdup_or_null(const char *str)
+{
+ return str ? xstrdup(str) : NULL;
+}
+
static inline size_t xsize_t(off_t len)
{
if (len > (size_t) len)
--
2.2.1.425.g441bb3c
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/5] git-compat-util: add xstrdup_or_null helper
2015-01-13 1:57 ` [PATCH 1/5] git-compat-util: add xstrdup_or_null helper Jeff King
@ 2015-01-13 2:21 ` Jonathan Nieder
2015-01-13 2:23 ` Jeff King
0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Nieder @ 2015-01-13 2:21 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Lukas Fleischer, git
Jeff King wrote:
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -675,6 +675,11 @@ extern char *xgetcwd(void);
>
> #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
>
> +static inline char *xstrdup_or_null(const char *str)
> +{
> + return str ? xstrdup(str) : NULL;
> +}
Would it make sense for xstrdup to always include the NULL check,
avoiding the need for the more verbose xstrdup_or_null?
Jonathan
diff --git i/wrapper.c w/wrapper.c
index 007ec0d..5a835e8 100644
--- i/wrapper.c
+++ w/wrapper.c
@@ -40,7 +40,11 @@ try_to_free_t set_try_to_free_routine(try_to_free_t routine)
char *xstrdup(const char *str)
{
- char *ret = strdup(str);
+ char *ret;
+
+ if (!str)
+ return NULL;
+ ret = strdup(str);
if (!ret) {
try_to_free_routine(strlen(str) + 1);
ret = strdup(str);
@@ -125,7 +129,11 @@ void *xmemdupz(const void *data, size_t len)
char *xstrndup(const char *str, size_t len)
{
- char *p = memchr(str, '\0', len);
+ char *p;
+
+ if (!str)
+ return NULL;
+ p = memchr(str, '\0', len);
return xmemdupz(str, p ? p - str : len);
}
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/5] git-compat-util: add xstrdup_or_null helper
2015-01-13 2:21 ` Jonathan Nieder
@ 2015-01-13 2:23 ` Jeff King
0 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2015-01-13 2:23 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Lukas Fleischer, git
On Mon, Jan 12, 2015 at 06:21:19PM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -675,6 +675,11 @@ extern char *xgetcwd(void);
> >
> > #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
> >
> > +static inline char *xstrdup_or_null(const char *str)
> > +{
> > + return str ? xstrdup(str) : NULL;
> > +}
>
> Would it make sense for xstrdup to always include the NULL check,
> avoiding the need for the more verbose xstrdup_or_null?
Read the rest of the thread. :)
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/5] builtin/apply.c: use xstrdup_or_null instead of null_strdup
2015-01-13 1:54 ` Jeff King
2015-01-13 1:57 ` [PATCH 1/5] git-compat-util: add xstrdup_or_null helper Jeff King
@ 2015-01-13 1:58 ` Jeff King
2015-01-13 1:58 ` [PATCH 3/5] builtin/commit.c: use xstrdup_or_null instead of envdup Jeff King
` (3 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2015-01-13 1:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lukas Fleischer, git
This file had its own identical helper that predates
xstrdup_or_null. Let's use the global one to avoid
repetition.
Signed-off-by: Jeff King <peff@peff.net>
---
You'll note that every one of these could just be a ?:-conditional
(there are no function calls).
builtin/apply.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 0aad912..dfd7a34 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -657,11 +657,6 @@ static size_t diff_timestamp_len(const char *line, size_t len)
return line + len - end;
}
-static char *null_strdup(const char *s)
-{
- return s ? xstrdup(s) : NULL;
-}
-
static char *find_name_common(const char *line, const char *def,
int p_value, const char *end, int terminate)
{
@@ -684,10 +679,10 @@ static char *find_name_common(const char *line, const char *def,
start = line;
}
if (!start)
- return squash_slash(null_strdup(def));
+ return squash_slash(xstrdup_or_null(def));
len = line - start;
if (!len)
- return squash_slash(null_strdup(def));
+ return squash_slash(xstrdup_or_null(def));
/*
* Generally we prefer the shorter name, especially
@@ -909,7 +904,7 @@ static void parse_traditional_patch(const char *first, const char *second, struc
patch->old_name = name;
} else {
patch->old_name = name;
- patch->new_name = null_strdup(name);
+ patch->new_name = xstrdup_or_null(name);
}
}
if (!name)
@@ -998,7 +993,7 @@ static int gitdiff_delete(const char *line, struct patch *patch)
{
patch->is_delete = 1;
free(patch->old_name);
- patch->old_name = null_strdup(patch->def_name);
+ patch->old_name = xstrdup_or_null(patch->def_name);
return gitdiff_oldmode(line, patch);
}
@@ -1006,7 +1001,7 @@ static int gitdiff_newfile(const char *line, struct patch *patch)
{
patch->is_new = 1;
free(patch->new_name);
- patch->new_name = null_strdup(patch->def_name);
+ patch->new_name = xstrdup_or_null(patch->def_name);
return gitdiff_newmode(line, patch);
}
--
2.2.1.425.g441bb3c
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/5] builtin/commit.c: use xstrdup_or_null instead of envdup
2015-01-13 1:54 ` Jeff King
2015-01-13 1:57 ` [PATCH 1/5] git-compat-util: add xstrdup_or_null helper Jeff King
2015-01-13 1:58 ` [PATCH 2/5] builtin/apply.c: use xstrdup_or_null instead of null_strdup Jeff King
@ 2015-01-13 1:58 ` Jeff King
2015-01-13 1:59 ` [PATCH 4/5] use xstrdup_or_null to replace ternary conditionals Jeff King
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2015-01-13 1:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lukas Fleischer, git
The only reason for envdup to be its own function is that we
have to save the result in a temporary string. With
xstrdup_or_null, we can feed the result of getenv()
directly.
Signed-off-by: Jeff King <peff@peff.net>
---
These ones need the temporary. Is the result more readable, or less?
builtin/commit.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 7d90c35..5cd1478 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -559,20 +559,14 @@ static void set_ident_var(char **buf, char *val)
*buf = val;
}
-static char *envdup(const char *var)
-{
- const char *val = getenv(var);
- return val ? xstrdup(val) : NULL;
-}
-
static void determine_author_info(struct strbuf *author_ident)
{
char *name, *email, *date;
struct ident_split author;
- name = envdup("GIT_AUTHOR_NAME");
- email = envdup("GIT_AUTHOR_EMAIL");
- date = envdup("GIT_AUTHOR_DATE");
+ name = xstrdup_or_null(getenv("GIT_AUTHOR_NAME"));
+ email = xstrdup_or_null(getenv("GIT_AUTHOR_EMAIL"));
+ date = xstrdup_or_null(getenv("GIT_AUTHOR_DATE"));
if (author_message) {
struct ident_split ident;
--
2.2.1.425.g441bb3c
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/5] use xstrdup_or_null to replace ternary conditionals
2015-01-13 1:54 ` Jeff King
` (2 preceding siblings ...)
2015-01-13 1:58 ` [PATCH 3/5] builtin/commit.c: use xstrdup_or_null instead of envdup Jeff King
@ 2015-01-13 1:59 ` Jeff King
2015-01-13 1:59 ` [PATCH 5/5] blame.c: fix garbled error message Jeff King
2015-01-14 14:21 ` [PATCH] " Lukas Fleischer
5 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2015-01-13 1:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lukas Fleischer, git
This replaces "x ? xstrdup(x) : NULL" with xstrdup_or_null(x).
The change is fairly mechanical, with the exception of
resolve_refdup, which can eliminate a temporary variable.
There are still a few hits grepping for "?.*xstrdup", but
these are of slightly different forms and cannot be
converted (e.g., "x ? xstrdup(x->foo) : NULL").
Signed-off-by: Jeff King <peff@peff.net>
---
resolve_refdup is the interesting one to look at for readability here.
config.c | 2 +-
grep.c | 4 ++--
notes.c | 2 +-
refs.c | 3 +--
remote.c | 4 ++--
shallow.c | 2 +-
walker.c | 2 +-
7 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/config.c b/config.c
index 752e2e2..e5e64dc 100644
--- a/config.c
+++ b/config.c
@@ -1340,7 +1340,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
string_list_init(&e->value_list, 1);
hashmap_add(&cs->config_hash, e);
}
- si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+ si = string_list_append_nodup(&e->value_list, xstrdup_or_null(value));
ALLOC_GROW(cs->list.items, cs->list.nr + 1, cs->list.alloc);
l_item = &cs->list.items[cs->list.nr++];
diff --git a/grep.c b/grep.c
index 6e085f8..b58c7c6 100644
--- a/grep.c
+++ b/grep.c
@@ -1661,8 +1661,8 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
const void *identifier)
{
gs->type = type;
- gs->name = name ? xstrdup(name) : NULL;
- gs->path = path ? xstrdup(path) : NULL;
+ gs->name = xstrdup_or_null(name);
+ gs->path = xstrdup_or_null(path);
gs->buf = NULL;
gs->size = 0;
gs->driver = NULL;
diff --git a/notes.c b/notes.c
index c763a21..2be4d7f 100644
--- a/notes.c
+++ b/notes.c
@@ -1006,7 +1006,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
t->root = (struct int_node *) xcalloc(1, sizeof(struct int_node));
t->first_non_note = NULL;
t->prev_non_note = NULL;
- t->ref = notes_ref ? xstrdup(notes_ref) : NULL;
+ t->ref = xstrdup_or_null(notes_ref);
t->combine_notes = combine_notes;
t->initialized = 1;
t->dirty = 0;
diff --git a/refs.c b/refs.c
index 5fcacc6..78fec5c 100644
--- a/refs.c
+++ b/refs.c
@@ -1618,8 +1618,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, unsigned
char *resolve_refdup(const char *ref, int resolve_flags, unsigned char *sha1, int *flags)
{
- const char *ret = resolve_ref_unsafe(ref, resolve_flags, sha1, flags);
- return ret ? xstrdup(ret) : NULL;
+ return xstrdup_or_null(resolve_ref_unsafe(ref, resolve_flags, sha1, flags));
}
/* The argument to filter_refs */
diff --git a/remote.c b/remote.c
index 5b9c693..7b71ebf 100644
--- a/remote.c
+++ b/remote.c
@@ -975,8 +975,8 @@ struct ref *copy_ref(const struct ref *ref)
cpy = xmalloc(sizeof(struct ref) + len + 1);
memcpy(cpy, ref, sizeof(struct ref) + len + 1);
cpy->next = NULL;
- cpy->symref = ref->symref ? xstrdup(ref->symref) : NULL;
- cpy->remote_status = ref->remote_status ? xstrdup(ref->remote_status) : NULL;
+ cpy->symref = xstrdup_or_null(ref->symref);
+ cpy->remote_status = xstrdup_or_null(ref->remote_status);
cpy->peer_ref = copy_ref(ref->peer_ref);
return cpy;
}
diff --git a/shallow.c b/shallow.c
index cdd0775..f5e6720 100644
--- a/shallow.c
+++ b/shallow.c
@@ -22,7 +22,7 @@ void set_alternate_shallow_file(const char *path, int override)
if (alternate_shallow_file && !override)
return;
free(alternate_shallow_file);
- alternate_shallow_file = path ? xstrdup(path) : NULL;
+ alternate_shallow_file = xstrdup_or_null(path);
}
int register_shallow(const unsigned char *sha1)
diff --git a/walker.c b/walker.c
index f149371..483da4e 100644
--- a/walker.c
+++ b/walker.c
@@ -232,7 +232,7 @@ int walker_targets_stdin(char ***target, const char ***write_ref)
REALLOC_ARRAY(*write_ref, targets_alloc);
}
(*target)[targets] = xstrdup(tg_one);
- (*write_ref)[targets] = rf_one ? xstrdup(rf_one) : NULL;
+ (*write_ref)[targets] = xstrdup_or_null(rf_one);
targets++;
}
strbuf_release(&buf);
--
2.2.1.425.g441bb3c
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 5/5] blame.c: fix garbled error message
2015-01-13 1:54 ` Jeff King
` (3 preceding siblings ...)
2015-01-13 1:59 ` [PATCH 4/5] use xstrdup_or_null to replace ternary conditionals Jeff King
@ 2015-01-13 1:59 ` Jeff King
2015-01-14 14:21 ` [PATCH] " Lukas Fleischer
5 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2015-01-13 1:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lukas Fleischer, git
From: Lukas Fleischer <git@cryptocrack.de>
The helper functions prepare_final() and prepare_initial() return a
pointer to a string that is a member of an object in the revs->pending
array. This array is later rebuilt when running prepare_revision_walk()
which potentially transforms the pointer target into a bogus string. Fix
this by maintaining a copy of the original string.
Signed-off-by: Lukas Fleischer <git@cryptocrack.de>
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/blame.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 303e217..0374fe8 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2390,7 +2390,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
return commit;
}
-static const char *prepare_final(struct scoreboard *sb)
+static char *prepare_final(struct scoreboard *sb)
{
int i;
const char *final_commit_name = NULL;
@@ -2415,10 +2415,10 @@ static const char *prepare_final(struct scoreboard *sb)
sb->final = (struct commit *) obj;
final_commit_name = revs->pending.objects[i].name;
}
- return final_commit_name;
+ return xstrdup_or_null(final_commit_name);
}
-static const char *prepare_initial(struct scoreboard *sb)
+static char *prepare_initial(struct scoreboard *sb)
{
int i;
const char *final_commit_name = NULL;
@@ -2445,7 +2445,7 @@ static const char *prepare_initial(struct scoreboard *sb)
}
if (!final_commit_name)
die("No commit to dig down to?");
- return final_commit_name;
+ return xstrdup(final_commit_name);
}
static int blame_copy_callback(const struct option *option, const char *arg, int unset)
@@ -2489,7 +2489,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
struct origin *o;
struct blame_entry *ent = NULL;
long dashdash_pos, lno;
- const char *final_commit_name = NULL;
+ char *final_commit_name = NULL;
enum object_type type;
static struct string_list range_list;
@@ -2786,6 +2786,8 @@ parse_done:
assign_blame(&sb, opt);
+ free(final_commit_name);
+
if (incremental)
return 0;
--
2.2.1.425.g441bb3c
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] blame.c: fix garbled error message
2015-01-13 1:54 ` Jeff King
` (4 preceding siblings ...)
2015-01-13 1:59 ` [PATCH 5/5] blame.c: fix garbled error message Jeff King
@ 2015-01-14 14:21 ` Lukas Fleischer
2015-01-14 17:22 ` Junio C Hamano
5 siblings, 1 reply; 19+ messages in thread
From: Lukas Fleischer @ 2015-01-14 14:21 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
On Tue, 13 Jan 2015 at 02:54:27, Jeff King wrote:
> On Mon, Jan 12, 2015 at 04:11:06PM -0800, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > As an aside, I have often been tempted to have xstrdup silently
> > > propagate a NULL. It would have been the right thing to do here, but
> > > maybe there are cases where the segfault is preferable for catching a
> > > mistake early (otherwise you might store the NULL and then segfault much
> > > later).
> >
> > Great minds think alike. The sentence after "but maybe ..." was
> > what I had in mind as a response in anticipation that somebody might
> > suggest that; a separate xstrdup_or_null() might be fine, but I'd
> > rather not to have xstrdup() that is _too_ magical.
>
> Yeah. Of course, it is not _that_ many more characters to do a ternary
> conditional. I guess the main benefit is that you do not have to repeat
> the name of the variable (which lets you reuse a function result
> directly, avoiding an explicit temporary).
>
> Here's my attempt. Some cases are a little nicer, but overall, it does
> not feel significantly more readable to me. I dunno. I could go either
> way. I stuck Lukas's patch on top (modified to use xstrdup_or_null), if
> we do want to go that route. Otherwise it needs the ?: treatment.
>
> [1/5]: git-compat-util: add xstrdup_or_null helper
> [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup
> [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup
> [4/5]: use xstrdup_or_null to replace ternary conditionals
> [5/5]: blame.c: fix garbled error message
>
Looks good to me! I am not sure whether those patches should be built on
top of (a fixed version of) my patch, though, which would make
backporting the fix to the maintenance branch straightforward. Junio?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] blame.c: fix garbled error message
2015-01-14 14:21 ` [PATCH] " Lukas Fleischer
@ 2015-01-14 17:22 ` Junio C Hamano
2015-01-14 20:49 ` Jeff King
0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2015-01-14 17:22 UTC (permalink / raw)
To: Lukas Fleischer; +Cc: Jeff King, git
Lukas Fleischer <git@cryptocrack.de> writes:
>> [1/5]: git-compat-util: add xstrdup_or_null helper
>> [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup
>> [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup
>> [4/5]: use xstrdup_or_null to replace ternary conditionals
>> [5/5]: blame.c: fix garbled error message
>>
>
> Looks good to me! I am not sure whether those patches should be built on
> top of (a fixed version of) my patch, though, which would make
> backporting the fix to the maintenance branch straightforward. Junio?
We can queue these five on top of 1da1e07c (clean up name allocation
in prepare_revision_walk, 2014-10-15), which changed the rule of the
game to break this code, that only is in v2.2 and later.
And the result should merge just fine to 'maint'.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] blame.c: fix garbled error message
2015-01-14 17:22 ` Junio C Hamano
@ 2015-01-14 20:49 ` Jeff King
2015-01-14 21:54 ` Junio C Hamano
0 siblings, 1 reply; 19+ messages in thread
From: Jeff King @ 2015-01-14 20:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lukas Fleischer, git
On Wed, Jan 14, 2015 at 09:22:52AM -0800, Junio C Hamano wrote:
> Lukas Fleischer <git@cryptocrack.de> writes:
>
> >> [1/5]: git-compat-util: add xstrdup_or_null helper
> >> [2/5]: builtin/apply.c: use xstrdup_or_null instead of null_strdup
> >> [3/5]: builtin/commit.c: use xstrdup_or_null instead of envdup
> >> [4/5]: use xstrdup_or_null to replace ternary conditionals
> >> [5/5]: blame.c: fix garbled error message
> >>
> >
> > Looks good to me! I am not sure whether those patches should be built on
> > top of (a fixed version of) my patch, though, which would make
> > backporting the fix to the maintenance branch straightforward. Junio?
>
> We can queue these five on top of 1da1e07c (clean up name allocation
> in prepare_revision_walk, 2014-10-15), which changed the rule of the
> game to break this code, that only is in v2.2 and later.
>
> And the result should merge just fine to 'maint'.
Are we in agreement then that the resulting code with the helper is
actually easier to read? I think replacing the straight ?: lines is, but
I am on the fence on whether:
const char *x = some_fun(...);
return xstrdup_or_null(x);
is better or worse than:
return xstrdup_or_null(some_fun(....));
-Peff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] blame.c: fix garbled error message
2015-01-14 20:49 ` Jeff King
@ 2015-01-14 21:54 ` Junio C Hamano
0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2015-01-14 21:54 UTC (permalink / raw)
To: Jeff King; +Cc: Lukas Fleischer, git
Jeff King <peff@peff.net> writes:
> On Wed, Jan 14, 2015 at 09:22:52AM -0800, Junio C Hamano wrote:
> ...
>> And the result should merge just fine to 'maint'.
>
> Are we in agreement then that the resulting code with the helper is
> actually easier to read? I think replacing the straight ?: lines is, but
> I am on the fence on whether:
>
> const char *x = some_fun(...);
> return xstrdup_or_null(x);
>
> is better or worse than:
>
> return xstrdup_or_null(some_fun(....));
I think the latter is fine as long as some_fun(...) invocation does
not get overly long, and even the longest I saw in refs.c, i.e.
return xstrdup_or_null(resolve_ref_unsafe(ref, resolve_flags, sha1, flags));
did not bother me too much.
^ permalink raw reply [flat|nested] 19+ messages in thread