Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
Thanks.
The patch looks correct but I have a slight maintainability concern and a
suggestion for possible improvement.
> ...
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index b7c6302..a66d3eb 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -696,17 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
> {
> int ret = 0;
> struct branch_info old;
> + char *path;
> unsigned char rev[20];
> int flag;
> memset(&old, 0, sizeof(old));
> - old.path = resolve_ref("HEAD", rev, 0, &flag);
> - if (old.path)
> - old.path = xstrdup(old.path);
> + old.path = path = resolve_refdup("HEAD", rev, 0, &flag);
This uses "one 'const char *' pointer that is used for reading data from
and an extra 'char *' pointer that is used only for freeing" approach,
which has two advantages and one disadvantage:
+ Obviously, we do not have to cast away constness when freeing.
+ A caller that follows the pattern could move the "for-data" pointer
without having to worry about affecting "for-freeing" pointer. It could
even do something like this:
char *for_freeing;
const char *for_data;
for_data = for_freeing = resolve_refdup(...);
if (prefixcmp(for_data, "refs/heads/"))
for_data = skip_prefix(for_data, "refs/heads/");
... do other things using for_data pointer ...
free(for_freeing);
- If the for-freeing and for-data pointers are named too similarly, the
code becomes harder to read. It is easy for a person who is new to the
codebase to start using the for-freeing pointer to manipulate the data
(mostly harmless) or even advance the pointer by mistake (bad).
A handful of places in the existing codebase use this "two pointers"
approach primarily for the second advantage. The way they avoid the
disadvantage is by naming the other pointer "$something_to_free" (and the
"$something_" part is optional if there is only one such variable in the
context) to make it clear that the variable is not meant to be looked at
in the code and its primary purpose is for cleaning up at the end.
Perhaps renaming this "path" to "to_free" (or "path_to_free" if you want
to hint the "path-ness" to the readers, but see below) would make it less
likely that future tweakers mistakenly look at, modify the string thru, or
worse yet move the pointer itself.
> diff --git a/builtin/merge.c b/builtin/merge.c
> index a1c8534..6437db2 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1096,6 +1096,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> struct commit_list *common = NULL;
> const char *best_strategy = NULL, *wt_strategy = NULL;
> struct commit_list **remotes = &remoteheads;
> + char *branch_ref;
>
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage_with_options(builtin_merge_usage, builtin_merge_options);
> @@ -1104,12 +1105,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> * Check if we are _not_ on a detached HEAD, i.e. if there is a
> * current branch.
> */
> - branch = resolve_ref("HEAD", head_sha1, 0, &flag);
> - if (branch) {
> - if (!prefixcmp(branch, "refs/heads/"))
> - branch += 11;
> - branch = xstrdup(branch);
> - }
> + branch = branch_ref = resolve_refdup("HEAD", head_sha1, 0, &flag);
Without s/branch_ref/to_free/ this part is unreadable and unmaintainable,
as it invites the "which variable should I use?" question.
It is more important to clarify that the "branch" variable is what the
code should look at and manipulate by *not* calling this for-freeing
pointer after what it contains (i.e. "branch").
When naming a "for-freeing" pointer variable, the kind of data the area of
memory happens to contain is of secondary importance. Being deliberately
vague about what the area of memory may contain is a good thing, because
it actively discourages the program from looking at the area via the
pointer if the variable is named "to_free" or something that does not
specify what it contains.
Other places in this patch that call the for-freeing variable "ref" share
the same issue but they are less specific than their for-data variable
counterpart, which makes it slightly less risky than this instance.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html