mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <>
To: Stefan Beller <>
Cc: "git\" <>,
	Jonathan Nieder <>,
	Jens Lehmann <>
Subject: Re: [PATCHv4 5/8] clone: factor out checking for an alternate path
Date: Mon, 15 Aug 2016 13:36:31 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <> (Stefan Beller's message of "Mon, 15 Aug 2016 12:03:22 -0700")

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.

  reply	other threads:[~2016-08-15 20:36 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-11 23:13 [PATCHv4 0/6] git clone: Marry --recursive and --reference Stefan Beller
2016-08-11 23:13 ` [PATCHv4 1/8] t7408: modernize style Stefan Beller
2016-08-11 23:13 ` [PATCHv4 2/8] t7408: merge short tests, factor out testing method Stefan Beller
2016-08-11 23:14 ` [PATCHv4 3/8] submodule--helper module-clone: allow multiple references Stefan Beller
2016-08-11 23:14 ` [PATCHv4 4/8] submodule--helper update-clone: " Stefan Beller
2016-08-11 23:14 ` [PATCHv4 5/8] clone: factor out checking for an alternate path Stefan Beller
2016-08-12 22:25   ` Junio C Hamano
2016-08-15 19:03     ` Stefan Beller
2016-08-15 20:36       ` Junio C Hamano [this message]
2016-08-15 21:44         ` Stefan Beller
2016-08-11 23:14 ` [PATCHv4 6/8] clone: clarify option_reference as required Stefan Beller
2016-08-11 23:14 ` [PATCHv4 7/8] clone: implement optional references Stefan Beller
2016-08-11 23:14 ` [PATCHv4 8/8] clone: recursive and reference option triggers submodule alternates Stefan Beller
2016-08-12 22:32   ` Junio C Hamano
2016-08-17 20:02   ` Junio C Hamano
2016-08-17 20:53     ` Stefan Beller
2016-08-17 21:31     ` Junio C Hamano
2016-08-17 21:41       ` Stefan Beller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).