Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper
On 19/06/17 05:45 AM, Johannes Schindelin wrote:
> Hi Liam,
>
> On Sat, 17 Jun 2017, Liam Beguin wrote:
>
>> On 16/06/17 09:56 AM, Johannes Schindelin wrote:
>>
>>> On Thu, 15 Jun 2017, Liam Beguin wrote:
>>>
On 14/06/17 09:08 AM, Johannes Schindelin wrote:
> diff --git a/sequencer.c b/sequencer.c
> index a697906d463..a0e020dab09 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2640,3 +2640,110 @@ int check_todo_list(void)
>
> return res;
> }
> +
> +/* skip picking commits whose parents are unchanged */
> +int skip_unnecessary_picks(void)
> +{
> + const char *todo_file = rebase_path_todo();
> + struct strbuf buf = STRBUF_INIT;
> + struct todo_list todo_list = TODO_LIST_INIT;
> + struct object_id onto_oid, *oid = &onto_oid, *parent_oid;
> + int fd, i;
> +
> + if (!read_oneliner(&buf, rebase_path_onto(), 0))
> + return error(_("could not read 'onto'"));
> + if (get_sha1(buf.buf, onto_oid.hash)) {
I missed this last time but we could also replace `get_sha1` with
`get_oid`
>>>
>>> Good point!
>>>
>>> I replaced this locally and force-pushed, but there is actually little
>>> chance of this patch series being integrated in a form with which I
>>> would be comfortable.
>>
>> Is there any chance of this moving forward? I was hoping to resend my
>> path to abbreviate command names on top of this.
>
> We are at an impasse right now.
>
> Junio wants me to change this code:
>
> revs.pretty_given = 1;
> git_config_get_string("rebase.instructionFormat", &format);
> if (!format || !*format) {
> free(format);
> format = xstrdup("%s");
> }
> get_commit_format(format, &revs);
> free(format);
> pp.fmt = revs.commit_format;
> pp.output_encoding = get_log_output_encoding();
>
> if (setup_revisions(argc, argv, &revs, NULL) > 1)
> ...
>
> which is reasonably compile-time safe, to something like this:
>
> struct strbuf userformat = STRBUF_INIT;
> struct argv_array args = ARGV_ARRAY_INIT;
> ...
> for (i = 0; i < argc; i++)
> argv_array_push(&args, argv[i]);
> git_config_get_string("rebase.instructionFormat", &format);
> if (!format || !*format)
> argv_array_push(&args, "--format=%s");
> else {
> strbuf_addf(&userformat, "--format=%s", format);
> argv_array_push(&args, userformat.buf);
> }
>
> if (setup_revisions(args.argc, args.argv, &revs, NULL) > 1)
> ...
> argv_array_clear(&args);
> strbuf_release(&userformat);
>
> which is not compile-time safe, harder to read, sloppy coding in my book.
>
> The reason for this suggestion is that one of the revision machinery's
> implementation details is an ugly little semi-secret: the pretty-printing
> machinery uses a global state, and that is why we need the "pretty_given"
> flag in the first place.
>
> Junio thinks that it would be better to not use the pretty_given flag in
> this patch series. I disagree: It would be better to use the pretty_given
> flag, also as a good motivator to work on removing the global state in the
> pretty-printing machinery.
>
> Junio wants to strong-arm me into accepting authorship for this sloppy
> coding, and I simply won't do it.
>
> That's why we are at an impasse right now.
>
> I am really, really sorry that this affects your patch series, as I had
> not foreseen Junio's insistence on the sloppy coding when I suggested that
> you rebase your work on top of my patch series. I just was really
> unprepared for this contention, and I am still surprised/shocked that this
> is even an issue up for discussion.
>
> Be that as it may, I see that this is a very bad experience for a
> motivated contributor such as yourself. I am very sorry about that!
It's not such a bad experience, I've found the people on the list to
be quite welcoming and supportive.
>
> So it may actually be better for you to go forward with your original
> patch series targeting git-rebase--interactive.sh.
I'll wait a bit longer to see how this goes and if it doesn't move, I'll
try re-sending an update to that series.
>
> My apologies,
> Dscho
>
Thanks,
Liam
Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper
Jeff King writes: > On Mon, Jun 19, 2017 at 11:45:50AM +0200, Johannes Schindelin wrote: > >> The reason for this suggestion is that one of the revision machinery's >> implementation details is an ugly little semi-secret: the pretty-printing >> machinery uses a global state, and that is why we need the "pretty_given" >> flag in the first place. > > I think that's mis-stating Junio's complaint. The point is not the > pretty_given flag itself, which we know about and can work around. The > point is that we don't know what other similar problems we have or will > have due to future changes in the revision code. > > In other words, there are two APIs: the one where C code manipulates > rev_info directly, and the one where revision.c responds to string > arguments. From a maintenance perspective, it is easy for somebody make > a change that works for the latter but not the former. There are (1) the API for users of revision traversal machinery and (2) the implementation detail of the API. Of course, the code that actually parses the plumbing and end-user supplied set of options needs to manipulate rev_info directly, but we should avoid direct manipulation when we can to future-proof the codebase. My main complaint is that Johannes's "compiler can catch mistakes" is true only for the most trivial kind of mistakes. The current implementation detail for reacting to --format="" given by the revision traversal machinery happens to be to set three things in rev_info structure: set verbose_header and pretty_given to 1 and then call get_commit_format() on the format string. Dscho is correct to say that the compiler can catch a mistake that misspells, say verbose_header as verbos_header. The compiler would not catch an equivalent mistake to misspell the option as --formta="", so from that point of view, his argument may seem to make sense. But the compiler would not help catching a copy-and-paste mistake to do only two out of the three things needed (e.g. forgetting to set pretty_given). If the code relies on setup_revisions() to react to options just the way "rev-list" plumbing does, such a mistake cannot happen in the first place---there is no need to worry about "catching". As you clarified correctly, the writer of the code that _uses_ the machinery, like the one in sequencer_make_script(), cannot anticipate how the internal implementation of reacting to the features will change, and more importantly, it should not have to know how it will change. By using the setup_revisions() API that uses the exactly the same parser as plumbing command "git rev-list" does, the forward compatibility is guaranteed. Copying and pasting the current internal implementation detail will break that. > I do agree that the lack of compile-time safety for obvious mistakes > like "--pertty" is a downside, though. On the other hand, there are > strong run-time checks there, so the tests would catch it. True. After setup_revisions() returns, if there are unrecognized options (e.g. misspelt "--pertty"), that can be used as the indication of a programming error, as the new code is not even parsing arbitrary set of options given by the end-user.
Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper
On Mon, Jun 19, 2017 at 11:45:50AM +0200, Johannes Schindelin wrote: > The reason for this suggestion is that one of the revision machinery's > implementation details is an ugly little semi-secret: the pretty-printing > machinery uses a global state, and that is why we need the "pretty_given" > flag in the first place. I think that's mis-stating Junio's complaint. The point is not the pretty_given flag itself, which we know about and can work around. The point is that we don't know what other similar problems we have or will have due to future changes in the revision code. In other words, there are two APIs: the one where C code manipulates rev_info directly, and the one where revision.c responds to string arguments. From a maintenance perspective, it is easy for somebody make a change that works for the latter but not the former. I do agree that the lack of compile-time safety for obvious mistakes like "--pertty" is a downside, though. On the other hand, there are strong run-time checks there, so the tests would catch it. I do not have a strong opinion myself in either direction. -Peff
Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper
Hi Liam,
On Sat, 17 Jun 2017, Liam Beguin wrote:
> On 16/06/17 09:56 AM, Johannes Schindelin wrote:
>
> > On Thu, 15 Jun 2017, Liam Beguin wrote:
> >
> >> On 14/06/17 09:08 AM, Johannes Schindelin wrote:
> >>> diff --git a/sequencer.c b/sequencer.c
> >>> index a697906d463..a0e020dab09 100644
> >>> --- a/sequencer.c
> >>> +++ b/sequencer.c
> >>> @@ -2640,3 +2640,110 @@ int check_todo_list(void)
> >>>
> >>> return res;
> >>> }
> >>> +
> >>> +/* skip picking commits whose parents are unchanged */
> >>> +int skip_unnecessary_picks(void)
> >>> +{
> >>> + const char *todo_file = rebase_path_todo();
> >>> + struct strbuf buf = STRBUF_INIT;
> >>> + struct todo_list todo_list = TODO_LIST_INIT;
> >>> + struct object_id onto_oid, *oid = &onto_oid, *parent_oid;
> >>> + int fd, i;
> >>> +
> >>> + if (!read_oneliner(&buf, rebase_path_onto(), 0))
> >>> + return error(_("could not read 'onto'"));
> >>> + if (get_sha1(buf.buf, onto_oid.hash)) {
> >>
> >> I missed this last time but we could also replace `get_sha1` with
> >> `get_oid`
> >
> > Good point!
> >
> > I replaced this locally and force-pushed, but there is actually little
> > chance of this patch series being integrated in a form with which I
> > would be comfortable.
>
> Is there any chance of this moving forward? I was hoping to resend my
> path to abbreviate command names on top of this.
We are at an impasse right now.
Junio wants me to change this code:
revs.pretty_given = 1;
git_config_get_string("rebase.instructionFormat", &format);
if (!format || !*format) {
free(format);
format = xstrdup("%s");
}
get_commit_format(format, &revs);
free(format);
pp.fmt = revs.commit_format;
pp.output_encoding = get_log_output_encoding();
if (setup_revisions(argc, argv, &revs, NULL) > 1)
...
which is reasonably compile-time safe, to something like this:
struct strbuf userformat = STRBUF_INIT;
struct argv_array args = ARGV_ARRAY_INIT;
...
for (i = 0; i < argc; i++)
argv_array_push(&args, argv[i]);
git_config_get_string("rebase.instructionFormat", &format);
if (!format || !*format)
argv_array_push(&args, "--format=%s");
else {
strbuf_addf(&userformat, "--format=%s", format);
argv_array_push(&args, userformat.buf);
}
if (setup_revisions(args.argc, args.argv, &revs, NULL) > 1)
...
argv_array_clear(&args);
strbuf_release(&userformat);
which is not compile-time safe, harder to read, sloppy coding in my book.
The reason for this suggestion is that one of the revision machinery's
implementation details is an ugly little semi-secret: the pretty-printing
machinery uses a global state, and that is why we need the "pretty_given"
flag in the first place.
Junio thinks that it would be better to not use the pretty_given flag in
this patch series. I disagree: It would be better to use the pretty_given
flag, also as a good motivator to work on removing the global state in the
pretty-printing machinery.
Junio wants to strong-arm me into accepting authorship for this sloppy
coding, and I simply won't do it.
That's why we are at an impasse right now.
I am really, really sorry that this affects your patch series, as I had
not foreseen Junio's insistence on the sloppy coding when I suggested that
you rebase your work on top of my patch series. I just was really
unprepared for this contention, and I am still surprised/shocked that this
is even an issue up for discussion.
Be that as it may, I see that this is a very bad experience for a
motivated contributor such as yourself. I am very sorry about that!
So it may actually be better for you to go forward with your original
patch series targeting git-rebase--interactive.sh.
My apologies,
Dscho
Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper
Hi Johannes,
On 16/06/17 09:56 AM, Johannes Schindelin wrote:
> Hi Liam,
>
> On Thu, 15 Jun 2017, Liam Beguin wrote:
>
>> On 14/06/17 09:08 AM, Johannes Schindelin wrote:
>>> diff --git a/sequencer.c b/sequencer.c
>>> index a697906d463..a0e020dab09 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -2640,3 +2640,110 @@ int check_todo_list(void)
>>>
>>> return res;
>>> }
>>> +
>>> +/* skip picking commits whose parents are unchanged */
>>> +int skip_unnecessary_picks(void)
>>> +{
>>> + const char *todo_file = rebase_path_todo();
>>> + struct strbuf buf = STRBUF_INIT;
>>> + struct todo_list todo_list = TODO_LIST_INIT;
>>> + struct object_id onto_oid, *oid = &onto_oid, *parent_oid;
>>> + int fd, i;
>>> +
>>> + if (!read_oneliner(&buf, rebase_path_onto(), 0))
>>> + return error(_("could not read 'onto'"));
>>> + if (get_sha1(buf.buf, onto_oid.hash)) {
>>
>> I missed this last time but we could also replace `get_sha1` with `get_oid`
>
> Good point!
>
> I replaced this locally and force-pushed, but there is actually little
> chance of this patch series being integrated in a form with which I would
> be comfortable.
Is there any chance of this moving forward? I was hoping to resend my path to
abbreviate command names on top of this.
>
> Ciao,
> Dscho
>
Thanks,
- Liam
Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper
Hi Liam,
On Thu, 15 Jun 2017, Liam Beguin wrote:
> On 14/06/17 09:08 AM, Johannes Schindelin wrote:
> > diff --git a/sequencer.c b/sequencer.c
> > index a697906d463..a0e020dab09 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2640,3 +2640,110 @@ int check_todo_list(void)
> >
> > return res;
> > }
> > +
> > +/* skip picking commits whose parents are unchanged */
> > +int skip_unnecessary_picks(void)
> > +{
> > + const char *todo_file = rebase_path_todo();
> > + struct strbuf buf = STRBUF_INIT;
> > + struct todo_list todo_list = TODO_LIST_INIT;
> > + struct object_id onto_oid, *oid = &onto_oid, *parent_oid;
> > + int fd, i;
> > +
> > + if (!read_oneliner(&buf, rebase_path_onto(), 0))
> > + return error(_("could not read 'onto'"));
> > + if (get_sha1(buf.buf, onto_oid.hash)) {
>
> I missed this last time but we could also replace `get_sha1` with `get_oid`
Good point!
I replaced this locally and force-pushed, but there is actually little
chance of this patch series being integrated in a form with which I would
be comfortable.
Ciao,
Dscho
Re: [PATCH v5 08/10] rebase -i: skip unnecessary picks using the rebase--helper
Hi,
On 14/06/17 09:08 AM, Johannes Schindelin wrote:
> diff --git a/sequencer.c b/sequencer.c
> index a697906d463..a0e020dab09 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2640,3 +2640,110 @@ int check_todo_list(void)
>
> return res;
> }
> +
> +/* skip picking commits whose parents are unchanged */
> +int skip_unnecessary_picks(void)
> +{
> + const char *todo_file = rebase_path_todo();
> + struct strbuf buf = STRBUF_INIT;
> + struct todo_list todo_list = TODO_LIST_INIT;
> + struct object_id onto_oid, *oid = &onto_oid, *parent_oid;
> + int fd, i;
> +
> + if (!read_oneliner(&buf, rebase_path_onto(), 0))
> + return error(_("could not read 'onto'"));
> + if (get_sha1(buf.buf, onto_oid.hash)) {
I missed this last time but we could also replace `get_sha1` with `get_oid`
> + strbuf_release(&buf);
> + return error(_("need a HEAD to fixup"));
> + }
> + strbuf_release(&buf);
> +
> + fd = open(todo_file, O_RDONLY);
> + if (fd < 0) {
> + return error_errno(_("could not open '%s'"), todo_file);
> + }
> + if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
> + close(fd);
> + return error(_("could not read '%s'."), todo_file);
> + }
> + close(fd);
> + if (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {
> + todo_list_release(&todo_list);
> + return -1;
> + }
> +
> + for (i = 0; i < todo_list.nr; i++) {
> + struct todo_item *item = todo_list.items + i;
> +
> + if (item->command >= TODO_NOOP)
> + continue;
> + if (item->command != TODO_PICK)
> + break;
> + if (parse_commit(item->commit)) {
> + todo_list_release(&todo_list);
> + return error(_("could not parse commit '%s'"),
> + oid_to_hex(&item->commit->object.oid));
> + }
> + if (!item->commit->parents)
> + break; /* root commit */
> + if (item->commit->parents->next)
> + break; /* merge commit */
> + parent_oid = &item->commit->parents->item->object.oid;
> + if (hashcmp(parent_oid->hash, oid->hash))
> + break;
> + oid = &item->commit->object.oid;
> + }
> + if (i > 0) {
> + int offset = i < todo_list.nr ?
> + todo_list.items[i].offset_in_buf : todo_list.buf.len;
> + const char *done_path = rebase_path_done();
> +
> + fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666);
> + if (fd < 0) {
> + error_errno(_("could not open '%s' for writing"),
> + done_path);
> + todo_list_release(&todo_list);
> + return -1;
> + }
> + if (write_in_full(fd, todo_list.buf.buf, offset) < 0) {
> + error_errno(_("could not write to '%s'"), done_path);
> + todo_list_release(&todo_list);
> + close(fd);
> + return -1;
> + }
> + close(fd);
> +
> + fd = open(rebase_path_todo(), O_WRONLY, 0666);
> + if (fd < 0) {
> + error_errno(_("could not open '%s' for writing"),
> + rebase_path_todo());
> + todo_list_release(&todo_list);
> + return -1;
> + }
> + if (write_in_full(fd, todo_list.buf.buf + offset,
> + todo_list.buf.len - offset) < 0) {
> + error_errno(_("could not write to '%s'"),
> + rebase_path_todo());
> + close(fd);
> + todo_list_release(&todo_list);
> + return -1;
> + }
> + if (ftruncate(fd, todo_list.buf.len - offset) < 0) {
> + error_errno(_("could not truncate '%s'"),
> + rebase_path_todo());
> + todo_list_release(&todo_list);
> + close(fd);
> + return -1;
> + }
> + close(fd);
> +
> + todo_list.current = i;
> + if (is_fixup(peek_command(&todo_list, 0)))
> + record_in_rewritten(oid, peek_command(&todo_list, 0));
> + }
> +
> + todo_list_release(&todo_list);
> + printf("%s\n", oid_to_hex(oid));
> +
> + return 0;
> +}
> diff --git a/sequencer.h b/sequencer.h
> index 878dd296f8c..04a57e09a1d 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -50,6 +50,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
>
> int transform_todo_ids(int shorten_ids);
> int check_todo_list(void);
> +int skip_unnecessary_picks(void);
>
> extern const ch
