git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Thomas Gummerer <t.gummerer@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Paul-Sebastian Ungureanu" <ungureanupaulsebastian@gmail.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>
Subject: Re: [PATCH v13 00/27] Convert "git stash" to C builtin
Date: Tue, 26 Feb 2019 20:48:28 +0000	[thread overview]
Message-ID: <20190226204828.GD27711@hank.intra.tgummerer.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1902261330560.41@tvgsbejvaqbjf.bet>

On 02/26, Johannes Schindelin wrote:
> Hi Thomas,
> 
> On Mon, 25 Feb 2019, Thomas Gummerer wrote:
> > 22:  51809c70ca ! 23:  56a5ce2aeb stash: convert `stash--helper.c` into `stash.c`
> >     @@ -13,7 +13,7 @@
> >          called directly and not by a shell script.
> >      
> >          Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
> >     -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
> >     +    Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> >      
> >       diff --git a/.gitignore b/.gitignore
> >       --- a/.gitignore
> >     @@ -273,9 +273,11 @@
> >       		return 0;
> >       
> >      -	strbuf_addstr(&stash_msg_buf, stash_msg);
> >     - 	if (!(ret = do_create_stash(ps, &stash_msg_buf, 0, 0, &info, NULL, 0)))
> >     +-	ret = do_create_stash(ps, &stash_msg_buf, include_untracked, 0, &info,
> >     ++	ret = do_create_stash(ps, &stash_msg_buf, 0, 0, &info,
> >     + 			      NULL, 0);
> 
> I do not quite understand this change, though. The end result seems to
> look similar enough to the previous iteration (except for the kept line
> wrapping which I would have undone in this patch), but how come that the
> `include_untracked` crept into some earlier patch in this iteration?

The reason for this is that previously we stopped passing
include_untracked through in patch 17.  As the option parsing in the
create_stash function was still in place at that point, it felt
incorrect to not pass it through to 'do_create_stash()'.

None of the tests failed at that point though, as nothing was actually
passing the command line options through anymore.  The last user of
that was removed in the "convert save to builtin" patch.

Arguably that would also be the right place to remove the option
parsing, rather than in this patch (convert `stash--helper.c` into
`stash.c`), but I tried to refrain from too much re-ordering to make
the changes more easily reviewable through range-diff.

Maybe I should have held back on these changes as well, or made the
other changes, dunno.  I wasn't always sure where to draw the line
between changing too much, which might hurt the chances of advancing
to next, because of the review work required, or doing too little,
which might hurt the chances because the series is not deemed in good
enough shape overall.

So what I ended up with is what I felt were the changes necessary to
advance the series, so we can start working on top, without the fear
of stepping on others peoples toes as much.

> The rest was obvious enough, thank you.
> 
> As I said before, I was very much in favor of getting this `stash-in-c`
> business moving again by advancing it to `next`.
> 
> From my perspective (which is not informed by any official statement about
> the role of `pu` or `next` because there is none as far as I know), the
> purpose of `pu` is to get patches into a shape where the original author
> is no longer the only one working on them. In my mind, that moment has
> long passed, and the fact that `ps/stash-in-c` is still stuck in `pu`
> really held me back on working more on this.

I mainly held back on doing much with it, because I didn't want to
step on Paul-Sebastians toes (and I thought we'd be okay with
advancing the series with the fixups on top).  But hopefully this is
now getting us into a state where we get this to next, and get further
work on top unblocked.

> So if you manage to get this finally cooking again, all hail to you!

I hope we can :)

> Thanks,
> Dscho

  reply	other threads:[~2019-02-26 20:48 UTC|newest]

Thread overview: 106+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <https://public-inbox.org/git/cover.1542925164.git.ungureanupaulsebastian@gmail.com/>
2018-12-20 19:44 ` [PATCH v12 00/26] Convert "git stash" to C builtin Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 01/26] sha1-name.c: add `get_oidf()` which acts like `get_oid()` Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 02/26] strbuf.c: add `strbuf_join_argv()` Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 03/26] strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()` Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 04/26] ident: add the ability to provide a "fallback identity" Paul-Sebastian Ungureanu
2018-12-26 21:21     ` Junio C Hamano
2018-12-27 21:24       ` Johannes Schindelin
2018-12-28 19:40         ` Junio C Hamano
2018-12-20 19:44   ` [PATCH v12 05/26] stash: improve option parsing test coverage Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 06/26] t3903: modernize style Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 07/26] stash: rename test cases to be more descriptive Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 08/26] stash: add tests for `git stash show` config Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 09/26] stash: mention options in `show` synopsis Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 10/26] stash: convert apply to builtin Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 11/26] stash: convert drop and clear " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 12/26] stash: convert branch " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 13/26] stash: convert pop " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 14/26] stash: convert list " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 15/26] stash: convert show " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 16/26] stash: convert store " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 17/26] stash: convert create " Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 18/26] stash: convert push " Paul-Sebastian Ungureanu
2019-02-08 11:30     ` SZEDER Gábor
2019-02-10 22:17       ` Thomas Gummerer
2019-02-11  1:13         ` SZEDER Gábor
2019-02-12 23:18           ` Thomas Gummerer
2019-02-19  0:23             ` SZEDER Gábor
2019-02-19 10:47               ` Johannes Schindelin
2019-02-19 19:59                 ` Junio C Hamano
2019-02-20 21:01                   ` Johannes Schindelin
2019-02-19 23:59                 ` Thomas Gummerer
2019-02-20  4:37                   ` Junio C Hamano
2019-02-20 21:10                     ` Johannes Schindelin
2019-02-20 22:30                     ` Thomas Gummerer
2019-02-25 23:16                 ` [PATCH v13 00/27] Convert "git stash" to C builtin Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 01/27] sha1-name.c: add `get_oidf()` which acts like `get_oid()` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 02/27] strbuf.c: add `strbuf_join_argv()` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 03/27] strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 04/27] ident: add the ability to provide a "fallback identity" Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 05/27] stash: improve option parsing test coverage Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 06/27] t3903: modernize style Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 07/27] t3903: add test for --intent-to-add file Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 08/27] stash: rename test cases to be more descriptive Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 09/27] stash: add tests for `git stash show` config Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 10/27] stash: mention options in `show` synopsis Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 11/27] stash: convert apply to builtin Thomas Gummerer
2019-03-14 13:19                     ` regression in new built-in stash + fsmonitor (was Re: [PATCH v13 11/27] stash: convert apply to builtin) Ævar Arnfjörð Bjarmason
2019-03-14 15:20                       ` Johannes Schindelin
2019-03-14 15:40                         ` Ævar Arnfjörð Bjarmason
2019-03-14 22:45                           ` Johannes Schindelin
2019-03-14 23:39                             ` Ævar Arnfjörð Bjarmason
2019-03-15  2:23                               ` Ben Peart
2019-02-25 23:16                   ` [PATCH v13 12/27] stash: convert drop and clear to builtin Thomas Gummerer
2019-03-07 19:15                     ` Jeff King
2019-03-09 18:30                       ` Thomas Gummerer
2019-03-10 23:26                         ` Jeff King
2019-03-11  1:40                         ` Junio C Hamano
2019-03-11 21:40                           ` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 13/27] stash: convert branch " Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 14/27] stash: convert pop " Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 15/27] stash: convert list " Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 16/27] stash: convert show " Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 17/27] stash: convert store " Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 18/27] stash: convert create " Thomas Gummerer
2019-03-07 19:18                     ` Jeff King
2019-03-08 15:30                       ` Johannes Schindelin
2019-03-09 18:26                         ` Thomas Gummerer
2019-03-11  1:47                           ` Junio C Hamano
2019-03-11  7:30                             ` Junio C Hamano
2019-03-11 21:42                               ` Thomas Gummerer
2019-03-11 22:16                                 ` [PATCH v2] stash: pass pathspec as pointer Thomas Gummerer
2019-03-12  6:50                                   ` Junio C Hamano
2019-03-12 22:35                                   ` Johannes Schindelin
2019-03-12 23:40                                     ` Thomas Gummerer
2019-03-13  1:47                                       ` Junio C Hamano
2019-03-13 22:14                                       ` Johannes Schindelin
2019-03-15 22:33                                         ` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 19/27] stash: convert push to builtin Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 20/27] stash: make push -q quiet Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 21/27] stash: convert save to builtin Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 22/27] stash: optimize `get_untracked_files()` and `check_changes()` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 23/27] stash: replace all `write-tree` child processes with API calls Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 24/27] stash: convert `stash--helper.c` into `stash.c` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 25/27] stash: add back the original, scripted `git stash` Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 26/27] stash: optionally use the scripted version again Thomas Gummerer
2019-02-25 23:16                   ` [PATCH v13 27/27] tests: add a special setup where stash.useBuiltin is off Thomas Gummerer
2019-02-26 12:40                   ` [PATCH v13 00/27] Convert "git stash" to C builtin Johannes Schindelin
2019-02-26 20:48                     ` Thomas Gummerer [this message]
2019-02-26 21:45                   ` Ævar Arnfjörð Bjarmason
2019-02-26 22:37                     ` Johannes Schindelin
2019-03-03  1:24                   ` Junio C Hamano
2019-03-03  1:25                   ` Junio C Hamano
2019-03-03 10:03                     ` Thomas Gummerer
2018-12-20 19:44   ` [PATCH v12 19/26] stash: make push -q quiet Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 20/26] stash: convert save to builtin Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 21/26] stash: optimize `get_untracked_files()` and `check_changes()` Paul-Sebastian Ungureanu
2019-01-06 22:47     ` Thomas Gummerer
2018-12-20 19:44   ` [PATCH v12 22/26] stash: replace all `write-tree` child processes with API calls Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 23/26] stash: convert `stash--helper.c` into `stash.c` Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 24/26] stash: add back the original, scripted `git stash` Paul-Sebastian Ungureanu
2018-12-20 19:44   ` [PATCH v12 25/26] stash: optionally use the scripted version again Paul-Sebastian Ungureanu
2019-01-06 22:59     ` Thomas Gummerer
2018-12-20 19:44   ` [PATCH v12 26/26] tests: add a special setup where stash.useBuiltin is off Paul-Sebastian Ungureanu
2019-01-03 23:39   ` [PATCH v12 00/26] Convert "git stash" to C builtin Junio C Hamano
2019-01-18 12:06     ` Johannes Schindelin
2019-01-18 17:49       ` Junio C Hamano

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20190226204828.GD27711@hank.intra.tgummerer.com \
    --to=t.gummerer@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=szeder.dev@gmail.com \
    --cc=ungureanupaulsebastian@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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

	https://80x24.org/mirrors/git.git

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).