Re: [PATCHv4 5/8] clone: factor out checking for an alternate path
On Mon, Aug 15, 2016 at 1:36 PM, Junio C Hamano wrote:
>
>>> Why do you need "err_buf", instead of directly writing the error to
>>> "err", especially if "err" is not optional?
>>>
+ ...
+out:
+ if (err_buf.len) {
>>
>> If we were directly writing to err, we would have checked
>> err.len here. Then you open up a subtle way of saying "dry run"
>> by giving a non empty error buffer.
>>
>> I contemplated doing that actually instead of splitting up into 2 functions,
>> but I considered that bad taste as it would require documentation.
>
> Sorry, but I do not understand this response at all.
Me neither. I think I elided the interesting part.
> I am still
> talking about keeping one function "compute_alternate_path()". The
> suggestion was just to drop "struct strbuf err_buf" local variable,
> and instead add the error messages the patch adds to err_buf with
> code like:
>
>> + if (!ref_git_s) {
>> + strbuf_addf(&err_buf, _("path '%s' does not exist"), path);
>> + goto out;
>
> to directly do
>
> strbuf_addf(err, _("path '%s' does not exist"), path);
done.
> err->len at "out:" label, or (2) using a new bool "seen_error = 0"
> and setting it to true when you diagnose an error. The latter would
> make the code a bit more verbose but I suspect the result would be
> easier to read than the former.
>
(2) is very readable, will reroll with that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 5/8] clone: factor out checking for an alternate path
Stefan Beller writes:
> On Fri, Aug 12, 2016 at 3:25 PM, Junio C Hamano wrote:
>> Stefan Beller writes:
>>
>>> + struct strbuf sb = STRBUF_INIT;
>>> + char *ref_git = compute_alternate_path(item->string, &sb);
>>
>> Who owns the memory for ref_git?
>
> The caller of compute_alternate_path(..), which makes
> add_one_reference faulty as of this patch.
>
>>
>>> - if (!access(mkpath("%s/shallow", ref_git), F_OK))
>>> - die(_("reference repository '%s' is shallow"), item->string);
>>> + if (!ref_git)
>>> + die("%s", sb.buf);
>>
>> Presumably the second argument to compute_alternate_path() is a
>> strbuf to receive the error message? It is unfortunate that the
>> variable used for this purpose is a bland "sb", but perhaps that
>> cannot be helped as you would reuse that strbuf for a different
>> purpose (i.e. not to store the error message, but to formulate a
>> pathname).
>
> Ok. I had an intermediate version with 2 strbufs but for some reason I
> decided one is better. We'll have 2 again. (err and sb; sb will have a
> smaller scope only in the else part.)
My "unfortunate" was meant to say "it is unfortunate that this is
the best, adding one extra variable is not worth the cost".
>> Why do you need "err_buf", instead of directly writing the error to
>> "err", especially if "err" is not optional?
>>
>>> + ...
>>> +out:
>>> + if (err_buf.len) {
>
> If we were directly writing to err, we would have checked
> err.len here. Then you open up a subtle way of saying "dry run"
> by giving a non empty error buffer.
>
> I contemplated doing that actually instead of splitting up into 2 functions,
> but I considered that bad taste as it would require documentation.
Sorry, but I do not understand this response at all. I am still
talking about keeping one function "compute_alternate_path()". The
suggestion was just to drop "struct strbuf err_buf" local variable,
and instead add the error messages the patch adds to err_buf with
code like:
> + if (!ref_git_s) {
> + strbuf_addf(&err_buf, _("path '%s' does not exist"), path);
> + goto out;
to directly do
strbuf_addf(err, _("path '%s' does not exist"), path);
instead. That way you do not have to move the bytes around from one
buffer to the other, like this:
>>> + strbuf_addbuf(err, &err_buf);
>>> + free(ref_git);
>>> + ref_git = NULL;
>>> + }
If you allow err->len to be non-zero upon entry, you'd need a way to
remember if you noticed an error, so that you can free and clear
ref_git after "out:" label, and doing so with a separate variable
would make the code more readable, I would think, either by (1)
recording the original err->len upon entry, and comparing it with
err->len at "out:" label, or (2) using a new bool "seen_error = 0"
and setting it to true when you diagnose an error. The latter would
make the code a bit more verbose but I suspect the result would be
easier to read than the former.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 5/8] clone: factor out checking for an alternate path
On Fri, Aug 12, 2016 at 3:25 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> + struct strbuf sb = STRBUF_INIT;
>> + char *ref_git = compute_alternate_path(item->string, &sb);
>
> Who owns the memory for ref_git?
The caller of compute_alternate_path(..), which makes
add_one_reference faulty as of this patch.
>
>> - if (!access(mkpath("%s/shallow", ref_git), F_OK))
>> - die(_("reference repository '%s' is shallow"), item->string);
>> + if (!ref_git)
>> + die("%s", sb.buf);
>
> Presumably the second argument to compute_alternate_path() is a
> strbuf to receive the error message? It is unfortunate that the
> variable used for this purpose is a bland "sb", but perhaps that
> cannot be helped as you would reuse that strbuf for a different
> purpose (i.e. not to store the error message, but to formulate a
> pathname).
Ok. I had an intermediate version with 2 strbufs but for some reason I
decided one is better. We'll have 2 again. (err and sb; sb will have a
smaller scope only in the else part.)
>
>> - if (!access(mkpath("%s/info/grafts", ref_git), F_OK))
>> - die(_("reference repository '%s' is grafted"), item->string);
>> + strbuf_addf(&sb, "%s/objects", ref_git);
>> + add_to_alternates_file(sb.buf);
>>
>> - strbuf_addf(&alternate, "%s/objects", ref_git);
>> - add_to_alternates_file(alternate.buf);
>> - strbuf_release(&alternate);
>> - free(ref_git);
>> + strbuf_release(&sb);
>
> I am wondering about the loss of free() here in the first comment.
fixed in a reroll.
>
>> +/*
>> + * Compute the exact path an alternate is at and returns it. In case of
>> + * error NULL is returned and the human readable error is added to `err`
>> + * `path` may be relative and should point to $GITDIR.
>> + * `err` must not be null.
>> + */
>> +char *compute_alternate_path(const char *path, struct strbuf *err)
>> +{
>> + char *ref_git = NULL;
>> + const char *repo, *ref_git_s;
>> + struct strbuf err_buf = STRBUF_INIT;
>
> Why do you need "err_buf", instead of directly writing the error to
> "err", especially if "err" is not optional?
>
>> + ...
>> +out:
>> + if (err_buf.len) {
If we were directly writing to err, we would have checked
err.len here. Then you open up a subtle way of saying "dry run"
by giving a non empty error buffer.
I contemplated doing that actually instead of splitting up into 2 functions,
but I considered that bad taste as it would require documentation.
>> + strbuf_addbuf(err, &err_buf);
>> + free(ref_git);
>> + ref_git = NULL;
>> + }
>> +
>> + strbuf_release(&err_buf);
>> + return ref_git;
>> +}
>
> So ref_git is a piece of memory on heap, and the caller is
> responsible for not leaking it.
Correct.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4 5/8] clone: factor out checking for an alternate path
Stefan Beller writes:
> + struct strbuf sb = STRBUF_INIT;
> + char *ref_git = compute_alternate_path(item->string, &sb);
Who owns the memory for ref_git?
> - if (!access(mkpath("%s/shallow", ref_git), F_OK))
> - die(_("reference repository '%s' is shallow"), item->string);
> + if (!ref_git)
> + die("%s", sb.buf);
Presumably the second argument to compute_alternate_path() is a
strbuf to receive the error message? It is unfortunate that the
variable used for this purpose is a bland "sb", but perhaps that
cannot be helped as you would reuse that strbuf for a different
purpose (i.e. not to store the error message, but to formulate a
pathname).
> - if (!access(mkpath("%s/info/grafts", ref_git), F_OK))
> - die(_("reference repository '%s' is grafted"), item->string);
> + strbuf_addf(&sb, "%s/objects", ref_git);
> + add_to_alternates_file(sb.buf);
>
> - strbuf_addf(&alternate, "%s/objects", ref_git);
> - add_to_alternates_file(alternate.buf);
> - strbuf_release(&alternate);
> - free(ref_git);
> + strbuf_release(&sb);
I am wondering about the loss of free() here in the first comment.
> +/*
> + * Compute the exact path an alternate is at and returns it. In case of
> + * error NULL is returned and the human readable error is added to `err`
> + * `path` may be relative and should point to $GITDIR.
> + * `err` must not be null.
> + */
> +char *compute_alternate_path(const char *path, struct strbuf *err)
> +{
> + char *ref_git = NULL;
> + const char *repo, *ref_git_s;
> + struct strbuf err_buf = STRBUF_INIT;
Why do you need "err_buf", instead of directly writing the error to
"err", especially if "err" is not optional?
> + ...
> +out:
> + if (err_buf.len) {
> + strbuf_addbuf(err, &err_buf);
> + free(ref_git);
> + ref_git = NULL;
> + }
> +
> + strbuf_release(&err_buf);
> + return ref_git;
> +}
So ref_git is a piece of memory on heap, and the caller is
responsible for not leaking it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv4 5/8] clone: factor out checking for an alternate path
In a later patch we want to determine if a path is suitable as an
alternate from other commands than builtin/clone. Move the checking
functionality of `add_one_reference` to `compute_alternate_path` that is
defined in cache.h.
Signed-off-by: Stefan Beller
---
builtin/clone.c | 42 ++--
cache.h | 1 +
sha1_file.c | 74 +
3 files changed, 82 insertions(+), 35 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index f044a8c..24b17539 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -282,44 +282,16 @@ static void strip_trailing_slashes(char *dir)
static int add_one_reference(struct string_list_item *item, void *cb_data)
{
- char *ref_git;
- const char *repo;
- struct strbuf alternate = STRBUF_INIT;
-
- /* Beware: read_gitfile(), real_path() and mkpath() return static
buffer */
- ref_git = xstrdup(real_path(item->string));
-
- repo = read_gitfile(ref_git);
- if (!repo)
- repo = read_gitfile(mkpath("%s/.git", ref_git));
- if (repo) {
- free(ref_git);
- ref_git = xstrdup(repo);
- }
-
- if (!repo && is_directory(mkpath("%s/.git/objects", ref_git))) {
- char *ref_git_git = mkpathdup("%s/.git", ref_git);
- free(ref_git);
- ref_git = ref_git_git;
- } else if (!is_directory(mkpath("%s/objects", ref_git))) {
- struct strbuf sb = STRBUF_INIT;
- if (get_common_dir(&sb, ref_git))
- die(_("reference repository '%s' as a linked checkout
is not supported yet."),
- item->string);
- die(_("reference repository '%s' is not a local repository."),
- item->string);
- }
+ struct strbuf sb = STRBUF_INIT;
+ char *ref_git = compute_alternate_path(item->string, &sb);
- if (!access(mkpath("%s/shallow", ref_git), F_OK))
- die(_("reference repository '%s' is shallow"), item->string);
+ if (!ref_git)
+ die("%s", sb.buf);
- if (!access(mkpath("%s/info/grafts", ref_git), F_OK))
- die(_("reference repository '%s' is grafted"), item->string);
+ strbuf_addf(&sb, "%s/objects", ref_git);
+ add_to_alternates_file(sb.buf);
- strbuf_addf(&alternate, "%s/objects", ref_git);
- add_to_alternates_file(alternate.buf);
- strbuf_release(&alternate);
- free(ref_git);
+ strbuf_release(&sb);
return 0;
}
diff --git a/cache.h b/cache.h
index 95a0bd3..35f41f7 100644
--- a/cache.h
+++ b/cache.h
@@ -1344,6 +1344,7 @@ extern struct alternate_object_database {
} *alt_odb_list;
extern void prepare_alt_odb(void);
extern void read_info_alternates(const char * relative_base, int depth);
+extern char *compute_alternate_path(const char *path, struct strbuf *err);
extern void add_to_alternates_file(const char *reference);
typedef int alt_odb_fn(struct alternate_object_database *, void *);
extern int foreach_alt_odb(alt_odb_fn, void*);
diff --git a/sha1_file.c b/sha1_file.c
index 02940f1..7351d8c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -418,6 +418,80 @@ void add_to_alternates_file(const char *reference)
free(alts);
}
+/*
+ * Compute the exact path an alternate is at and returns it. In case of
+ * error NULL is returned and the human readable error is added to `err`
+ * `path` may be relative and should point to $GITDIR.
+ * `err` must not be null.
+ */
+char *compute_alternate_path(const char *path, struct strbuf *err)
+{
+ char *ref_git = NULL;
+ const char *repo, *ref_git_s;
+ struct strbuf err_buf = STRBUF_INIT;
+
+ ref_git_s = real_path_if_valid(path);
+ if (!ref_git_s) {
+ strbuf_addf(&err_buf, _("path '%s' does not exist"), path);
+ goto out;
+ } else
+ /*
+* Beware: read_gitfile(), real_path() and mkpath()
+* return static buffer
+*/
+ ref_git = xstrdup(ref_git_s);
+
+ repo = read_gitfile(ref_git);
+ if (!repo)
+ repo = read_gitfile(mkpath("%s/.git", ref_git));
+ if (repo) {
+ free(ref_git);
+ ref_git = xstrdup(repo);
+ }
+
+ if (!repo && is_directory(mkpath("%s/.git/objects", ref_git))) {
+ char *ref_git_git = mkpathdup("%s/.git", ref_git);
+ free(ref_git);
+ ref_git = ref_git_git;
+ } else if (!is_directory(mkpath("%s/objects", ref_git))) {
+ struct strbuf sb = STRBUF_INIT;
+ if (get_common_dir(&sb, ref_git)) {
+ strbuf_addf(&err_buf,
+ _("reference repository '%s' as a linked "
+ "checkout is not supported yet."),
+ path);
+
