Re: [PATCH/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()
Hi Dscho, (Sorry for the very late reply, I got caught up with some unexpected work and am still clearing my inbox ><) On Thu, Mar 17, 2016 at 1:11 AM, Johannes Schindelin wrote: > On Wed, 16 Mar 2016, Paul Tan wrote: >> On Wed, Mar 16, 2016 at 4:04 PM, Johannes Schindelin >> wrote: >> > In addition I want to point out that sequencer's replay_opts seem to be at >> > least related, but the patch shares none of its code with the sequencer. >> > Let's avoid that. >> > >> > In other words, let's try to add as little code as possible when we can >> > enhance existing code. >> >> Well, both git-rebase--am.sh and git-rebase--merge.sh do not use the >> sequencer functionality at all, and we don't see git-am for example >> needing to be aware of onto, orig-head, head-name etc. > > That is arguing that the implementation of --am and --merge is too far > away from the sequencer and therefore should not be made closer. > > By that token, has_unstaged_changes() should never be allowed to call > init_revisions(): it *never* looks at any revisions at all! > > And the idea of the sequencer is so much more related to --am and --merge > than unstaged changes are to revisions: the entire purpose of the > sequencer (no matter the *current* implementation with all its > limitations) is to apply a bunch of patches, in sequence. That is pretty > much precisely what *all* members of the rebase family are about. > > In other words: please do not let current limitations dictate that we > should introduce diverging code for essentially the same workflow. Ah, so you are thinking of replacing the --am and --merge scripts with sequencer? That sounds great :-) I'll wait for your sequencer patch series then. Thanks! Paul -- 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/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()
On Wed, Mar 16, 2016 at 5:04 AM, Paul Tan wrote:
>>
>> How is this specific to the state file? All it does is create the
>> leading directory
>> if it doesn't exist? (So I'd expect file_exists(concat(dir, file)) to
>> have the same
>> result without actually creating the directory if it doesn't exist as
>> a side effect?
>
> I don't quite understand, AFAIK mkpath() does not create any
> directories as a side-effect. And yes, I just wanted a short way to
> say file_exists(concat(dir, file)) or file_exists(mkpath("%s/%s", dir,
> file)) without cluttering up the code.
My bad. I should not assume functions doing stuff as their name might suggest.
(It "makes the path" but only in terms of creating the right string, not on the
file system, where you'd use functions like safe_create_leading_directories.
I thought all that was implied in mkpath).
>
>> If the dir doesn't exist it can be created in rebase_options_load explicitly?
>
> I don't intend to create any directories if they do not exist.
>
>>> +
>>> +static int read_state_file(struct strbuf *sb, const char *dir, const char
>>> *file)
>>> +{
>>> + const char *path = mkpath("%s/%s", dir, file);
>>> + strbuf_reset(sb);
>>> + if (strbuf_read_file(sb, path, 0) >= 0)
>>> + return sb->len;
>>> + else
>>> + return error(_("could not read '%s'"), path);
>>> +}
>>> +
>>> +int rebase_options_load(struct rebase_options *opts, const char *dir)
>>> +{
>>> + struct strbuf sb = STRBUF_INIT;
>>> + const char *filename;
>>> +
>>> + /* opts->orig_refname */
>>> + if (read_state_file(&sb, dir, "head-name") < 0)
>>> + return -1;
>>> + strbuf_trim(&sb);
>>> + if (starts_with(sb.buf, "refs/heads/"))
>>> + opts->orig_refname = strbuf_detach(&sb, NULL);
>>> + else if (!strcmp(sb.buf, "detached HEAD"))
>>> + opts->orig_refname = NULL;
>>> + else
>>> + return error(_("could not parse %s"), mkpath("%s/%s", dir,
>>> "head-name"));
>>> +
>>> + /* opts->onto */
>>> + if (read_state_file(&sb, dir, "onto") < 0)
>>> + return -1;
>>> + strbuf_trim(&sb);
>>> + if (get_oid_hex(sb.buf, &opts->onto) < 0)
>>> + return error(_("could not parse %s"), mkpath("%s/%s", dir,
>>> "onto"));
>>> +
>>> + /*
>>> +* We always write to orig-head, but interactive rebase used to
>>> write
>>> +* to head. Fall back to reading from head to cover for the case
>>> that
>>> +* the user upgraded git with an ongoing interactive rebase.
>>> +*/
>>> + filename = state_file_exists(dir, "orig-head") ? "orig-head" :
>>> "head";
>>> + if (read_state_file(&sb, dir, filename) < 0)
>>> + return -1;
>>
>> So from here on we always use "orig-head" instead of "head" for
>> interactive rebase.
>> Would people ever rely on the (internal) file name and have e.g.
>> scripts which operate
>> on the "head" file ?
>
> This backwards-compatibility code is just a straight port from the
> code in git-rebase.sh.
>
> The usage of orig-head has been around since 2011 with 84df456
> (rebase: extract code for writing basic state, 2011-02-06), so I guess
> if people had issues with it, it would have been reported.
I did not read the rebase shell code, but commented on the C code only.
If this is already in there, let's keep it.
Sorry for the confusion.
Thanks,
Stefan
--
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/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()
Hi Stefan,
On Tue, Mar 15, 2016 at 4:30 AM, Stefan Beller wrote:
> On Sat, Mar 12, 2016 at 2:46 AM, Paul Tan wrote:
>> These functions can be used for loading and saving common rebase options
>> into a state directory.
>>
>> Signed-off-by: Paul Tan
>> ---
>> rebase-common.c | 69
>> +
>> rebase-common.h | 4
>> 2 files changed, 73 insertions(+)
>>
>> diff --git a/rebase-common.c b/rebase-common.c
>> index 5a49ac4..1835f08 100644
>> --- a/rebase-common.c
>> +++ b/rebase-common.c
>> @@ -26,3 +26,72 @@ void rebase_options_swap(struct rebase_options *dst,
>> struct rebase_options *src)
>> *dst = *src;
>> *src = tmp;
>> }
>> +
>> +static int state_file_exists(const char *dir, const char *file)
>> +{
>> + return file_exists(mkpath("%s/%s", dir, file));
>> +}
>
> How is this specific to the state file? All it does is create the
> leading directory
> if it doesn't exist? (So I'd expect file_exists(concat(dir, file)) to
> have the same
> result without actually creating the directory if it doesn't exist as
> a side effect?
I don't quite understand, AFAIK mkpath() does not create any
directories as a side-effect. And yes, I just wanted a short way to
say file_exists(concat(dir, file)) or file_exists(mkpath("%s/%s", dir,
file)) without cluttering up the code.
> If the dir doesn't exist it can be created in rebase_options_load explicitly?
I don't intend to create any directories if they do not exist.
>> +
>> +static int read_state_file(struct strbuf *sb, const char *dir, const char
>> *file)
>> +{
>> + const char *path = mkpath("%s/%s", dir, file);
>> + strbuf_reset(sb);
>> + if (strbuf_read_file(sb, path, 0) >= 0)
>> + return sb->len;
>> + else
>> + return error(_("could not read '%s'"), path);
>> +}
>> +
>> +int rebase_options_load(struct rebase_options *opts, const char *dir)
>> +{
>> + struct strbuf sb = STRBUF_INIT;
>> + const char *filename;
>> +
>> + /* opts->orig_refname */
>> + if (read_state_file(&sb, dir, "head-name") < 0)
>> + return -1;
>> + strbuf_trim(&sb);
>> + if (starts_with(sb.buf, "refs/heads/"))
>> + opts->orig_refname = strbuf_detach(&sb, NULL);
>> + else if (!strcmp(sb.buf, "detached HEAD"))
>> + opts->orig_refname = NULL;
>> + else
>> + return error(_("could not parse %s"), mkpath("%s/%s", dir,
>> "head-name"));
>> +
>> + /* opts->onto */
>> + if (read_state_file(&sb, dir, "onto") < 0)
>> + return -1;
>> + strbuf_trim(&sb);
>> + if (get_oid_hex(sb.buf, &opts->onto) < 0)
>> + return error(_("could not parse %s"), mkpath("%s/%s", dir,
>> "onto"));
>> +
>> + /*
>> +* We always write to orig-head, but interactive rebase used to write
>> +* to head. Fall back to reading from head to cover for the case that
>> +* the user upgraded git with an ongoing interactive rebase.
>> +*/
>> + filename = state_file_exists(dir, "orig-head") ? "orig-head" :
>> "head";
>> + if (read_state_file(&sb, dir, filename) < 0)
>> + return -1;
>
> So from here on we always use "orig-head" instead of "head" for
> interactive rebase.
> Would people ever rely on the (internal) file name and have e.g.
> scripts which operate
> on the "head" file ?
This backwards-compatibility code is just a straight port from the
code in git-rebase.sh.
The usage of orig-head has been around since 2011 with 84df456
(rebase: extract code for writing basic state, 2011-02-06), so I guess
if people had issues with it, it would have been reported.
>
>
>> + strbuf_trim(&sb);
>> + if (get_oid_hex(sb.buf, &opts->orig_head) < 0)
>> + return error(_("could not parse %s"), mkpath("%s/%s", dir,
>> filename));
>> +
>> + strbuf_release(&sb);
>> + return 0;
>> +}
>> +
>> +static int write_state_text(const char *dir, const char *file, const char
>> *string)
>> +{
>> + return write_file(mkpath("%s/%s", dir, file), "%s", string);
>> +}
>
> Same comment as on checking the state files existence. I'm not sure if the
> side
> effect of creating the dir is better done explicitly where it is used.
> The concat of dir and
> file name can still be done in the helper though? (If the helper is
> needed at all then)
Same as above -- AFAIK I don't think mkpath() creates any directories
as a side-effect.
>
>> +
>> +void rebase_options_save(const struct rebase_options *opts, const char *dir)
>> +{
>> + const char *head_name = opts->orig_refname;
>> + if (!head_name)
>> + head_name = "detached HEAD";
>> + write_state_text(dir, "head-name", head_name);
>> + write_state_text(dir, "onto", oid_to_hex(&opts->onto));
>> + write_state_text(dir, "orig-head", oid_to_hex(&opts->orig_head));
>> +}
>> diff --git a/rebase-common.h b
Re: [PATCH/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()
Hi Paul, On Wed, 16 Mar 2016, Paul Tan wrote: > On Wed, Mar 16, 2016 at 4:04 PM, Johannes Schindelin > wrote: > > In addition I want to point out that sequencer's replay_opts seem to be at > > least related, but the patch shares none of its code with the sequencer. > > Let's avoid that. > > > > In other words, let's try to add as little code as possible when we can > > enhance existing code. > > Well, both git-rebase--am.sh and git-rebase--merge.sh do not use the > sequencer functionality at all, and we don't see git-am for example > needing to be aware of onto, orig-head, head-name etc. That is arguing that the implementation of --am and --merge is too far away from the sequencer and therefore should not be made closer. By that token, has_unstaged_changes() should never be allowed to call init_revisions(): it *never* looks at any revisions at all! And the idea of the sequencer is so much more related to --am and --merge than unstaged changes are to revisions: the entire purpose of the sequencer (no matter the *current* implementation with all its limitations) is to apply a bunch of patches, in sequence. That is pretty much precisely what *all* members of the rebase family are about. In other words: please do not let current limitations dictate that we should introduce diverging code for essentially the same workflow. Ciao, Dscho -- 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/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()
Hi Dscho, On Wed, Mar 16, 2016 at 4:04 PM, Johannes Schindelin wrote: > In addition I want to point out that sequencer's replay_opts seem to be at > least related, but the patch shares none of its code with the sequencer. > Let's avoid that. > > In other words, let's try to add as little code as possible when we can > enhance existing code. Well, both git-rebase--am.sh and git-rebase--merge.sh do not use the sequencer functionality at all, and we don't see git-am for example needing to be aware of onto, orig-head, head-name etc. Besides, I don't see why the sequencer needs to be aware of these rebase-specific options. For simplicity[1], I would think the sequencer would only need to be aware of what the todo list is, since that is common to cherry-pick/revert and rebase-i, and all the other non-sequencer related stuff like checking out the --onto , updating refs can be done at the rebase-interactive.c or git-rebase--interactive.sh layer. [1] Of course, it's kind of unfortunate that sequencer.c has to be aware of whether it is being called as cherry-pick or revert, but I don't see why implementing interactive rebase functionality needs to make the same mistake. Thanks, Paul -- 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/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()
Hi Paul,
On Mon, 14 Mar 2016, Stefan Beller wrote:
> On Sat, Mar 12, 2016 at 2:46 AM, Paul Tan wrote:
> > These functions can be used for loading and saving common rebase options
> > into a state directory.
> >
> > Signed-off-by: Paul Tan
> > ---
> > rebase-common.c | 69
> > +
> > rebase-common.h | 4
> > 2 files changed, 73 insertions(+)
> >
> > diff --git a/rebase-common.c b/rebase-common.c
> > index 5a49ac4..1835f08 100644
> > --- a/rebase-common.c
> > +++ b/rebase-common.c
> > @@ -26,3 +26,72 @@ void rebase_options_swap(struct rebase_options *dst,
> > struct rebase_options *src)
> > *dst = *src;
> > *src = tmp;
> > }
> > +
> > +static int state_file_exists(const char *dir, const char *file)
> > +{
> > + return file_exists(mkpath("%s/%s", dir, file));
> > +}
>
> How is this specific to the state file? All it does is create the
> leading directory
> if it doesn't exist? (So I'd expect file_exists(concat(dir, file)) to
> have the same
> result without actually creating the directory if it doesn't exist as
> a side effect?
>
> If the dir doesn't exist it can be created in rebase_options_load explicitly?
In addition I want to point out that sequencer's replay_opts seem to be at
least related, but the patch shares none of its code with the sequencer.
Let's avoid that.
In other words, let's try to add as little code as possible when we can
enhance existing code.
Ciao,
Dscho
--
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/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()
On Sat, Mar 12, 2016 at 2:46 AM, Paul Tan wrote:
> These functions can be used for loading and saving common rebase options
> into a state directory.
>
> Signed-off-by: Paul Tan
> ---
> rebase-common.c | 69
> +
> rebase-common.h | 4
> 2 files changed, 73 insertions(+)
>
> diff --git a/rebase-common.c b/rebase-common.c
> index 5a49ac4..1835f08 100644
> --- a/rebase-common.c
> +++ b/rebase-common.c
> @@ -26,3 +26,72 @@ void rebase_options_swap(struct rebase_options *dst,
> struct rebase_options *src)
> *dst = *src;
> *src = tmp;
> }
> +
> +static int state_file_exists(const char *dir, const char *file)
> +{
> + return file_exists(mkpath("%s/%s", dir, file));
> +}
How is this specific to the state file? All it does is create the
leading directory
if it doesn't exist? (So I'd expect file_exists(concat(dir, file)) to
have the same
result without actually creating the directory if it doesn't exist as
a side effect?
If the dir doesn't exist it can be created in rebase_options_load explicitly?
> +
> +static int read_state_file(struct strbuf *sb, const char *dir, const char
> *file)
> +{
> + const char *path = mkpath("%s/%s", dir, file);
> + strbuf_reset(sb);
> + if (strbuf_read_file(sb, path, 0) >= 0)
> + return sb->len;
> + else
> + return error(_("could not read '%s'"), path);
> +}
> +
> +int rebase_options_load(struct rebase_options *opts, const char *dir)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + const char *filename;
> +
> + /* opts->orig_refname */
> + if (read_state_file(&sb, dir, "head-name") < 0)
> + return -1;
> + strbuf_trim(&sb);
> + if (starts_with(sb.buf, "refs/heads/"))
> + opts->orig_refname = strbuf_detach(&sb, NULL);
> + else if (!strcmp(sb.buf, "detached HEAD"))
> + opts->orig_refname = NULL;
> + else
> + return error(_("could not parse %s"), mkpath("%s/%s", dir,
> "head-name"));
> +
> + /* opts->onto */
> + if (read_state_file(&sb, dir, "onto") < 0)
> + return -1;
> + strbuf_trim(&sb);
> + if (get_oid_hex(sb.buf, &opts->onto) < 0)
> + return error(_("could not parse %s"), mkpath("%s/%s", dir,
> "onto"));
> +
> + /*
> +* We always write to orig-head, but interactive rebase used to write
> +* to head. Fall back to reading from head to cover for the case that
> +* the user upgraded git with an ongoing interactive rebase.
> +*/
> + filename = state_file_exists(dir, "orig-head") ? "orig-head" : "head";
> + if (read_state_file(&sb, dir, filename) < 0)
> + return -1;
So from here on we always use "orig-head" instead of "head" for
interactive rebase.
Would people ever rely on the (internal) file name and have e.g.
scripts which operate
on the "head" file ?
> + strbuf_trim(&sb);
> + if (get_oid_hex(sb.buf, &opts->orig_head) < 0)
> + return error(_("could not parse %s"), mkpath("%s/%s", dir,
> filename));
> +
> + strbuf_release(&sb);
> + return 0;
> +}
> +
> +static int write_state_text(const char *dir, const char *file, const char
> *string)
> +{
> + return write_file(mkpath("%s/%s", dir, file), "%s", string);
> +}
Same comment as on checking the state files existence. I'm not sure if the side
effect of creating the dir is better done explicitly where it is used.
The concat of dir and
file name can still be done in the helper though? (If the helper is
needed at all then)
> +
> +void rebase_options_save(const struct rebase_options *opts, const char *dir)
> +{
> + const char *head_name = opts->orig_refname;
> + if (!head_name)
> + head_name = "detached HEAD";
> + write_state_text(dir, "head-name", head_name);
> + write_state_text(dir, "onto", oid_to_hex(&opts->onto));
> + write_state_text(dir, "orig-head", oid_to_hex(&opts->orig_head));
> +}
> diff --git a/rebase-common.h b/rebase-common.h
> index db5146a..051c056 100644
> --- a/rebase-common.h
> +++ b/rebase-common.h
> @@ -20,4 +20,8 @@ void rebase_options_release(struct rebase_options *);
>
> void rebase_options_swap(struct rebase_options *dst, struct rebase_options
> *src);
>
> +int rebase_options_load(struct rebase_options *, const char *dir);
> +
> +void rebase_options_save(const struct rebase_options *, const char *dir);
> +
> #endif /* REBASE_COMMON_H */
> --
> 2.7.0
>
--
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
