Re: [PATCH v6 02/10] replace: add --graft option
On Fri, Jul 11, 2014 at 11:25:43AM -0700, Junio C Hamano wrote: > Christian Couder writes: > > > On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano wrote: > >> Christian Couder writes: > >> > >>> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano wrote: > >>> > > "Making sure A's parent is B" would be an > > idempotent operation, no? Why not just make sure A's parent is > > already B and report "Your wish has been granted" to the user? > >>> > >>> ... and here you say we should report "your wish has been granted"... > >> > >> Normal way for "git replace" to report that is to exit with status 0 > >> and without any noise, I would think. > > > > In a similar case "git replace --edit" we error out instead of just > > exiting (with status 0), see: > > > > f22166b5fee7dc (replace: make sure --edit results in a different object) > > I do not care *too* deeply, but if you ask me, that may be a mistake > we may want to fix before the next release. Yeah, I also do not care too deeply, but I mentioned in the earlier review that I would expect it to just remove the replacement if it ends up generating the same object. -Peff -- 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: [PATCH v6 02/10] replace: add --graft option
Christian Couder writes: > On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano wrote: >> Christian Couder writes: >> >>> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano wrote: >>> > "Making sure A's parent is B" would be an > idempotent operation, no? Why not just make sure A's parent is > already B and report "Your wish has been granted" to the user? >>> >>> ... and here you say we should report "your wish has been granted"... >> >> Normal way for "git replace" to report that is to exit with status 0 >> and without any noise, I would think. > > In a similar case "git replace --edit" we error out instead of just > exiting (with status 0), see: > > f22166b5fee7dc (replace: make sure --edit results in a different object) I do not care *too* deeply, but if you ask me, that may be a mistake we may want to fix before the next release. -- 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: [PATCH v6 02/10] replace: add --graft option
On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano wrote: > Christian Couder writes: > >> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano wrote: >> "Making sure A's parent is B" would be an idempotent operation, no? Why not just make sure A's parent is already B and report "Your wish has been granted" to the user? >> >> ... and here you say we should report "your wish has been granted"... > > Normal way for "git replace" to report that is to exit with status 0 > and without any noise, I would think. In a similar case "git replace --edit" we error out instead of just exiting (with status 0), see: f22166b5fee7dc (replace: make sure --edit results in a different object) -- 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: [PATCH v6 02/10] replace: add --graft option
Christian Couder writes: > On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano wrote: > >>> "Making sure A's parent is B" would be an >>> idempotent operation, no? Why not just make sure A's parent is >>> already B and report "Your wish has been granted" to the user? > > ... and here you say we should report "your wish has been granted"... Normal way for "git replace" to report that is to exit with status 0 and without any noise, I would think. >>> Why would it be simpler for the user to get an error, inspect the >>> situation and realize that his wish has been granted after all? > > ... but for me reporting to the user "your wish has been granted" and > warning (or errorring out) saying "the new commit would be the same as > the old one" are nearly the same thing. > > So I wonder what exactly you are not happy with. See above. -- 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: [PATCH v6 02/10] replace: add --graft option
On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano wrote: > Junio C Hamano writes: > >>> >>> As the user might expect that a new replace ref was created on success >>> (0 exit code), and as we should at least warn if we would create a >>> commit that is the same as an existing one,... >> >> Why is it an event that needs a warning? I do not buy that "as we >> should at least" at all. Here you ask why this event needs a warning... > Ehh, it came a bit differently from what I meant. Perhaps s/do not > buy/do not understand/ is closer to what I think---that is, it is > not like I with a strong conviction think you are wrong. It is more > like I do not understand why you think it needs a warning, meaing > you would need to explain it better. > >> If you say "make sure A's parent is B" and then you asked the same >> thing again when there already is a replacement in place, that >> should be a no-op. (When there is already a replacement in place we error out in replace_object_sha1() unless --force is used. What we are talking about here is what happens if the replacement commit is the same as the original commit.) >> "Making sure A's parent is B" would be an >> idempotent operation, no? Why not just make sure A's parent is >> already B and report "Your wish has been granted" to the user? ... and here you say we should report "your wish has been granted"... >> Why would it be simpler for the user to get an error, inspect the >> situation and realize that his wish has been granted after all? ... but for me reporting to the user "your wish has been granted" and warning (or errorring out) saying "the new commit would be the same as the old one" are nearly the same thing. So I wonder what exactly you are not happy with. Is it the fact that I use the error() function, because it would prefix the message with "fatal:" and that would be too scary? Is it because with error() the exit code would not be 0? Is it because the message "new commit is the same as the old one: '%s'" is too scary or unnecessarily technical by itself? Is it ok if I just print the message on stderr without "warning:" or "fatal:" in front of it? By the way, when I say "As ... and ..., I think it is just simpler to error out in this case.", I mean that it is simpler from the developer's point of view, not necessarily for the user. Of course I am ok with improving things for the user even if it requires more code/work, but it is difficult to properly do that if I don't see how I could do it. Thanks, Christian. -- 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: [PATCH v6 02/10] replace: add --graft option
Junio C Hamano writes: > Christian Couder writes: > >>> Is this really an error? It may be a warning-worthy situation for a >>> user or a script to end up doing a no-op graft, e.g. >>> >>> git replace --graft HEAD HEAD^ >>> >>> but I wonder if it is more convenient to signal an error (like this >>> patch does) or just ignore the request and return without adding the >>> replace ref. >> >> As the user might expect that a new replace ref was created on success >> (0 exit code), and as we should at least warn if we would create a >> commit that is the same as an existing one,... > > Why is it an event that needs a warning? I do not buy that "as we > should at least" at all. Ehh, it came a bit differently from what I meant. Perhaps s/do not buy/do not understand/ is closer to what I think---that is, it is not like I with a strong conviction think you are wrong. It is more like I do not understand why you think it needs a warning, meaing you would need to explain it better. > If you say "make sure A's parent is B" and then you asked the same > thing again when there already is a replacement in place, that > should be a no-op. "Making sure A's parent is B" would be an > idempotent operation, no? Why not just make sure A's parent is > already B and report "Your wish has been granted" to the user? > > Why would it be simpler for the user to get an error, inspect the > situation and realize that his wish has been granted after all? -- 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: [PATCH v6 02/10] replace: add --graft option
Christian Couder writes: >> Is this really an error? It may be a warning-worthy situation for a >> user or a script to end up doing a no-op graft, e.g. >> >> git replace --graft HEAD HEAD^ >> >> but I wonder if it is more convenient to signal an error (like this >> patch does) or just ignore the request and return without adding the >> replace ref. > > As the user might expect that a new replace ref was created on success > (0 exit code), and as we should at least warn if we would create a > commit that is the same as an existing one,... Why is it an event that needs a warning? I do not buy that "as we should at least" at all. If you say "make sure A's parent is B" and then you asked the same thing again when there already is a replacement in place, that should be a no-op. "Making sure A's parent is B" would be an idempotent operation, no? Why not just make sure A's parent is already B and report "Your wish has been granted" to the user? Why would it be simpler for the user to get an error, inspect the situation and realize that his wish has been granted after all? -- 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: [PATCH v6 02/10] replace: add --graft option
On Wed, Jul 9, 2014 at 4:59 PM, Junio C Hamano wrote:
> Christian Couder writes:
>
>> +static void replace_parents(struct strbuf *buf, int argc, const char **argv)
>> +{
>> + struct strbuf new_parents = STRBUF_INIT;
>> + const char *parent_start, *parent_end;
>> + int i;
>> +
>> + /* find existing parents */
>> + parent_start = buf->buf;
>> + parent_start += 46; /* "tree " + "hex sha1" + "\n" */
>> + parent_end = parent_start;
>> +
>> + while (starts_with(parent_end, "parent "))
>> + parent_end += 48; /* "parent " + "hex sha1" + "\n" */
>> +
>> + /* prepare new parents */
>> + for (i = 1; i < argc; i++) {
>
> It looks somewhat strange that both replace_parents() and
> create_graft() take familiar-looking pair, but one
> ignores argv[0] and uses the remainder and the other uses argv[0].
> Shouldn't this function consume argv[] starting from [0] for
> consistency? You'd obviously need to update the caller to adjust
> the arguments it gives to this function.
Ok, will do.
>> +static int create_graft(int argc, const char **argv, int force)
>> +{
>> + unsigned char old[20], new[20];
>> + const char *old_ref = argv[0];
>> +...
>> +
>> + replace_parents(&buf, argc, argv);
>> +
>> + if (write_sha1_file(buf.buf, buf.len, commit_type, new))
>> + die(_("could not write replacement commit for: '%s'"),
>> old_ref);
>> +
>> + strbuf_release(&buf);
>> +
>> + if (!hashcmp(old, new))
>> + return error("new commit is the same as the old one: '%s'",
>> sha1_to_hex(old));
>
> Is this really an error? It may be a warning-worthy situation for a
> user or a script to end up doing a no-op graft, e.g.
>
> git replace --graft HEAD HEAD^
>
> but I wonder if it is more convenient to signal an error (like this
> patch does) or just ignore the request and return without adding the
> replace ref.
As the user might expect that a new replace ref was created on success
(0 exit code), and as we should at least warn if we would create a
commit that is the same as an existing one, I think it is just simpler
to error out in this case.
Though maybe we could use a special exit code (for example 2) in this
case, so that the user might more easily ignore this error in a
script.
> Other than these two points, looks good to me.
Thanks,
Christian.
--
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: [PATCH v6 02/10] replace: add --graft option
Christian Couder writes:
> The usage string for this option is:
>
> git replace [-f] --graft [...]
>
> First we create a new commit that is the same as
> except that its parents are [...]
>
> Then we create a replace ref that replace with
> the commit we just created.
>
> With this new option, it should be straightforward to
> convert grafts to replace refs.
Sensible.
>
> Signed-off-by: Christian Couder
> Signed-off-by: Junio C Hamano
> ---
> builtin/replace.c | 74
> ++-
> 1 file changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 1bb491d..ad47237 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -17,6 +17,7 @@
> static const char * const git_replace_usage[] = {
> N_("git replace [-f] "),
> N_("git replace [-f] --edit "),
> + N_("git replace [-f] --graft [...]"),
> N_("git replace -d ..."),
> N_("git replace [--format=] [-l []]"),
> NULL
> @@ -294,6 +295,66 @@ static int edit_and_replace(const char *object_ref, int
> force)
> return replace_object_sha1(object_ref, old, "replacement", new, force);
> }
>
> +static void replace_parents(struct strbuf *buf, int argc, const char **argv)
> +{
> + struct strbuf new_parents = STRBUF_INIT;
> + const char *parent_start, *parent_end;
> + int i;
> +
> + /* find existing parents */
> + parent_start = buf->buf;
> + parent_start += 46; /* "tree " + "hex sha1" + "\n" */
> + parent_end = parent_start;
> +
> + while (starts_with(parent_end, "parent "))
> + parent_end += 48; /* "parent " + "hex sha1" + "\n" */
> +
> + /* prepare new parents */
> + for (i = 1; i < argc; i++) {
It looks somewhat strange that both replace_parents() and
create_graft() take familiar-looking pair, but one
ignores argv[0] and uses the remainder and the other uses argv[0].
Shouldn't this function consume argv[] starting from [0] for
consistency? You'd obviously need to update the caller to adjust
the arguments it gives to this function.
> +static int create_graft(int argc, const char **argv, int force)
> +{
> + unsigned char old[20], new[20];
> + const char *old_ref = argv[0];
> +...
> +
> + replace_parents(&buf, argc, argv);
> +
> + if (write_sha1_file(buf.buf, buf.len, commit_type, new))
> + die(_("could not write replacement commit for: '%s'"), old_ref);
> +
> + strbuf_release(&buf);
> +
> + if (!hashcmp(old, new))
> + return error("new commit is the same as the old one: '%s'",
> sha1_to_hex(old));
Is this really an error? It may be a warning-worthy situation for a
user or a script to end up doing a no-op graft, e.g.
git replace --graft HEAD HEAD^
but I wonder if it is more convenient to signal an error (like this
patch does) or just ignore the request and return without adding the
replace ref.
Other than these two points, looks good to me.
Thanks.
--
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
