Re: [PATCH v2 3/3] Refactor mingw_cygwin_offset_1st_component()
Hi Torsten,
On Fri, 7 Dec 2018, tbo...@web.de wrote:
> diff --git a/compat/mingw-cygwin.c b/compat/mingw-cygwin.c
> index 5552c3ac20..c379a72775 100644
> --- a/compat/mingw-cygwin.c
> +++ b/compat/mingw-cygwin.c
> @@ -10,10 +10,8 @@ size_t mingw_cygwin_skip_dos_drive_prefix(char **path)
> size_t mingw_cygwin_offset_1st_component(const char *path)
> {
> char *pos = (char *)path;
> -
> - /* unc paths */
This comment is still useful (and now even more correct), and should stay.
> - if (!skip_dos_drive_prefix() &&
> - is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
> + if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
> + /* unc path */
> /* skip server name */
> pos = strpbrk(pos + 2, "\\/");
> if (!pos)
> @@ -22,7 +20,8 @@ size_t mingw_cygwin_offset_1st_component(const char *path)
> do {
> pos++;
> } while (*pos && !is_dir_sep(*pos));
> + } else {
> + skip_dos_drive_prefix();
> }
> -
Why remove this empty line? It structures the code quite nicely.
The rest looks correct to me,
Johannes
> return pos + is_dir_sep(*pos) - path;
> }
> --
> 2.19.0.271.gfe8321ec05
>
>
Re: [PATCH v2 1/3] git clone C:\cygwin\home\USER\repo' is working (again)
Hi Torsten, On Fri, 7 Dec 2018, tbo...@web.de wrote: > compat/mingw-cygwin.c | 28 > compat/mingw-cygwin.h | 20 Please use compat/win32/path.c (or .../path-utils.c) instead. This is not so much about MINGW or Cygwin or MSys or MSYS2 or Visual C++, but about Windows. Thanks, Johannes
Re: enhancement: support for author.email and author.name in "git config"
Hi William, On Thu, 6 Dec 2018, William Hubbs wrote: > On Thu, Dec 06, 2018 at 07:38:08PM +0100, Martin Ågren wrote: > > Hi William, > > > > [...] > > > > This idea was floated a couple of months ago [1]. Junio seemed to find > > the request sensible and outlined a design. No patches materialized, as > > far as I know, but that could be because Eric suggested a tool called > > direnv. Maybe that would work for you. > > Yes, this design would solve the issue. There *is* a way to get what you want that is super easy and will definitely work: if you sit down and do it ;-) Please let us know if you need any additional information before you can start. Ciao, Johannes
Re: git, monorepos, and access control
Hi, On Wed, 5 Dec 2018, Jeff King wrote: > The model that fits more naturally with how Git is implemented would be > to use submodules. There you leak the hash of the commit from the > private submodule, but that's probably obscure enough (and if you're > really worried, you can add a random nonce to the commit messages in the > submodule to make their hashes unguessable). I hear myself frequently saying: "Friends don't let friends use submodules". It's almost like: "Some people think their problem is solved by using submodules. Only now they have two problems." There are big reasons, after all, why some companies go for monorepos: it is not for lack of trying to go with submodules, it is the problems that were incurred by trying to treat entire repositories the same as single files (or even trees): they are just too different. In a previous life, I also tried to go for submodules, was burned, and had to restart the whole thing. We ended up with something that might work in this instance, too, although our use case was not need-to-know type of encapsulation. What we went for was straight up modularization. What I mean is that we split the project up into over 100 individual projects that are now all maintained in individual repositories, and they are connected completely outside of Git, via a dependency management system (in this case, Maven, although that is probably too Java-centric for AMD's needs). I just wanted to throw that out here: if you can split up your project into individual projects, it might make sense not to maintain them as submodules but instead as individual repositories whose artifacts are uploaded into a central, versioned artifact store (Maven, NuGet, etc). And those artifacts would then be retrieved by the projects that need them. I figure that that scheme might work for you better than submodules: I could imagine that you need to make the build artifacts available even to people who are not permitted to look at the corresponding source code, anyway. Ciao, Johannes
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Peff, On Mon, 3 Dec 2018, Jeff King wrote: > On Mon, Dec 03, 2018 at 08:01:44PM +0100, Johannes Schindelin wrote: > > > > In this sort of situation, I often whish to be able to do nested rebases. > > > Even more because it happen relatively often that I forget that I'm > > > working in a rebase and not on the head, and then it's quite natural > > > to me to type things like 'git rebase -i @^^^' while already rebasing. > > > But I suppose this has already been discussed. > > > > Varieties of this have been discussed, but no, not nested rebases. > > > > The closest we thought about was re-scheduling the latest commits, > > which is now harder because of the `--rebase-merges` mode. > > > > But I think it would be doable. Your idea of a "nested" rebase actually > > opens that door quite nicely. It would not *really* be a nested rebase, > > and it would still only be possible in interactive mode, but I could > > totally see > > > > git rebase --nested -i HEAD~3 > > > > to generate and prepend the following lines to the `git-rebase-todo` file: > > > > reset abcdef01 # This is HEAD~3 > > pick abcdef02 # This is HEAD~2 > > pick abcdef03 # This is HEAD~ > > pick abcdef04 # This is HEAD > > > > (assuming that the latest 3 commits were non-merge commits; It would look > > quite a bit more complicated in other situations.) > > Yeah, I would probably use that if it existed. I kind of use it, even if it does not exist ;-) > It would be nicer to have real nested sequencer operations, I think, for > other situations. I agree. But for the moment, our data format is too married to the exact layout of .git/, thanks to `git rebase`'s evolution from a Unix shell script. Alban has this really great patch series to work on the todo list in-memory, and that paves the way to decouple the entire sequencer thing from the file system. The most notably thing that still would need to be encapsulated would be the options: currently, there is a plethora of inconsistent options files being saved into the state directory (for some, the mere presence indicates `true`, some contain `true` or `false`, others contain text, etc). > E.g., cherry-picking a sequence of commits while you're in the middle of > a rebase. You will be delighted to learn that you can cherry-pick a sequence of commits in the middle of a rebase already. I do `exec git cherry-pick ` *all* the time. > But I suspect getting that right would be _loads_ more work, and > probably would involve some funky UI corner cases to handle the stack of > operations (so truly aborting a rebase may mean an arbitrary number of > "rebase --abort" calls to pop the stack). Your suggestion is probably a > reasonable trick in the meantime. You know what is an even more reasonable trick? Worktrees. I only thought about that this morning, but I should have mentioned it right away, as I use it quite frequently. When I have tricky nested rebases to perform, I do use throw-away worktrees where I check out unnamed branches, work on those, and then integrate them back into the "outer rebase" via the `reset` command in the todo list. Ciao, Dscho
Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences
Hi Hannes, On Mon, 3 Dec 2018, Johannes Sixt wrote: > The text body of section Behavioral Differences is typeset as code, > but should be regular text. Remove the indentation to achieve that. > > While here, prettify the language: > > - use "the x backend" instead of "x-based rebase"; > - use present tense instead of future tense; > > and use subsections instead of a list. > > Signed-off-by: Johannes Sixt > --- The changes look sensible to me. Thanks, Dscho > Cf. https://git-scm.com/docs/git-rebase#_behavioral_differences > > I actually did not test the result, because I don't have the > infrastructure. > > The sentence "The am backend sometimes does not" is not very useful, > but is not my fault ;) It would be great if it could be made more > specific, but I do not know the details. > > Documentation/git-rebase.txt | 30 +- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 80793bad8d..dff17b3178 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -550,24 +550,28 @@ Other incompatible flag pairs: > BEHAVIORAL DIFFERENCES > --- > > - * empty commits: > +There are some subtle differences how the backends behave. > > -am-based rebase will drop any "empty" commits, whether the > -commit started empty (had no changes relative to its parent to > -start with) or ended empty (all changes were already applied > -upstream in other commits). > +Empty commits > +~ > + > +The am backend drops any "empty" commits, regardless of whether the > +commit started empty (had no changes relative to its parent to > +start with) or ended empty (all changes were already applied > +upstream in other commits). > > -merge-based rebase does the same. > +The merge backend does the same. > > -interactive-based rebase will by default drop commits that > -started empty and halt if it hits a commit that ended up empty. > -The `--keep-empty` option exists for interactive rebases to allow > -it to keep commits that started empty. > +The interactive backend drops commits by default that > +started empty and halts if it hits a commit that ended up empty. > +The `--keep-empty` option exists for the interactive backend to allow > +it to keep commits that started empty. > > - * directory rename detection: > +Directory rename detection > +~~ > > -merge-based and interactive-based rebases work fine with > -directory rename detection. am-based rebases sometimes do not. > +The merge and interactive backends work fine with > +directory rename detection. The am backend sometimes does not. > > include::merge-strategies.txt[] > > -- > 2.19.1.1133.g2dd3d172d2 >
Re: [ANNOUNCE] Git v2.20.0-rc2
Team, Git for Windows v2.20.0-rc2 is available here: https://github.com/git-for-windows/git/releases/tag/v2.20.0-rc2.windows.1 There is already one known issue: the size of the installer increased (see https://github.com/git-for-windows/git/issues/1963). This is in the process of being addressed. Ciao, Johannes On Sat, 1 Dec 2018, Junio C Hamano wrote: > A release candidate Git v2.20.0-rc2 is now available for testing > at the usual places. It is comprised of 934 non-merge commits > since v2.19.0, contributed by 76 people, 25 of which are new faces. > > The tarballs are found at: > > https://www.kernel.org/pub/software/scm/git/testing/ > > The following public repositories all have a copy of the > 'v2.20.0-rc2' tag and the 'master' branch that the tag points at: > > url = https://kernel.googlesource.com/pub/scm/git/git > url = git://repo.or.cz/alt-git.git > url = https://github.com/gitster/git > > New contributors whose contributions weren't in v2.19.0 are as follows. > Welcome to the Git development community! > > Aaron Lindsay, Alexander Pyhalov, Anton Serbulov, Brendan > Forster, Carlo Marcelo Arenas Belón, Daniels Umanovskis, David > Zych, Đoàn Trần Công Danh, Frederick Eaton, Greg Hurrell, > James Knight, Jann Horn, Joshua Watt, Loo Rong Jie, Lucas > De Marchi, Matthew DeVore, Mihir Mehta, Nickolai Belakovski, > Roger Strain, Sam McKelvie, Saulius Gurklys, Shulhan, Steven > Fernandez, Strain, Roger L, and Tim Schumacher. > > Returning contributors who helped this release are as follows. > Thanks for your continued support. > > Ævar Arnfjörð Bjarmason, Alban Gruin, Andreas Gruenbacher, > Andreas Heiduk, Antonio Ospite, Ben Peart, Brandon Williams, > brian m. carlson, Christian Couder, Christian Hesse, Denton Liu, > Derrick Stolee, Elijah Newren, Eric Sunshine, Jean-Noël Avila, > Jeff Hostetler, Jeff King, Johannes Schindelin, Johannes Sixt, > Jonathan Nieder, Jonathan Tan, Josh Steadmon, Junio C Hamano, > Karsten Blees, Luke Diamand, Martin Ågren, Max Kirillov, > Michael Witten, Michał Górny, Nguyễn Thái Ngọc Duy, Noam > Postavsky, Olga Telezhnaya, Phillip Wood, Pratik Karki, Rafael > Ascensão, Ralf Thielow, Ramsay Jones, Rasmus Villemoes, René > Scharfe, Sebastian Staudt, Stefan Beller, Stephen P. Smith, Steve > Hoelzer, Sven Strickroth, SZEDER Gábor, Tao Qingyun, Taylor > Blau, Thomas Gummerer, Todd Zullinger, Torsten Bögershausen, > and Uwe Kleine-König. > > > > Git 2.20 Release Notes (draft) > == > > Backward Compatibility Notes > > > * "git branch -l " used to be a way to ask a reflog to be >created while creating a new branch, but that is no longer the >case. It is a short-hand for "git branch --list " now. > > * "git push" into refs/tags/* hierarchy is rejected without getting >forced, but "git fetch" (misguidedly) used the "fast forwarding" >rule used for the refs/heads/* hierarchy; this has been corrected, >which means some fetches of tags that did not fail with older >version of Git will fail without "--force" with this version. > > * "git help -a" now gives verbose output (same as "git help -av"). >Those who want the old output may say "git help --no-verbose -a".. > > * "git cpn --help", when "cpn" is an alias to, say, "cherry-pick -n", >reported only the alias expansion of "cpn" in earlier versions of >Git. It now runs "git cherry-pick --help" to show the manual page >of the command, while sending the alias expansion to the standard >error stream. > > * "git send-email" learned to grab address-looking string on any >trailer whose name ends with "-by". This is a backward-incompatible >change. Adding "--suppress-cc=misc-by" on the command line, or >setting sendemail.suppresscc configuration variable to "misc-by", >can be used to disable this behaviour. > > > Updates since v2.19 > --- > > UI, Workflows & Features > > * Running "git clone" against a project that contain two files with >pathnames that differ only in cases on a case insensitive >filesystem would result in one of the files lost because the >underlying filesystem is incapable of holding both at the same >time. An attempt is made to detect such a case and warn. > > * "git checkout -b newbranch [HEAD]" should not have to do as much as >checking out a commit diff
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Duy, On Mon, 3 Dec 2018, Duy Nguyen wrote: > On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote: > > I sometimes add "x false" to the top of the todo list to stop and create > > new commits before the first one. > > And here I've been doing the same by "edit" the first commit, add a > new commit then reorder them in the second interactive rebase :P > > This made me look at git-rebase.txt to really learn about interactive > rebase. I think the interactive rebase section could use some > improvements. Its style looks.. umm.. more story telling than a > reference. Perhaps something like this to at least highlight the > commands. And maybe, just maybe, that "story telling" is more useful for users who want to learn about the interactive rebase, just like yourself, when compared to a mere "reference". Ciao, Johannes
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Luc, On Mon, 3 Dec 2018, Luc Van Oostenryck wrote: > On Sat, Dec 01, 2018 at 03:02:09PM -0500, Jeff King wrote: > > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: > > > > > > > Would it not make more sense to add a command-line option (and a > > > > > config > > > > > setting) to re-schedule failed `exec` commands? Like so: > > > > > > > > Your proposition would do in most cases, however it is not possible to > > > > make a distinction between reschedulable and non-reschedulable commands. > > > > > > True. But I don't think that's so terrible. > > > > > > What I think is something to avoid is two commands that do something very, > > > very similar, but with two very, very different names. > > > > > > In reality, I think that it would even make sense to change the default to > > > reschedule failed `exec` commands. Which is why I suggested to also add a > > > config option. > > > > I sometimes add "x false" to the top of the todo list to stop and create > > new commits before the first one. That would be awkward if I could never > > get past that line. However, I think elsewhere a "pause" line has been > > discussed, which would serve the same purpose. > > > > I wonder how often this kind of "yes, I know it fails, but keep going > > anyway" situation would come up. And what the interface is like for > > getting past it. E.g., what if you fixed a bunch of stuff but your tests > > still fail? You may not want to abandon the changes you've made, but you > > need to "rebase --continue" to move forward. I encounter this often when > > the correct fix is actually in an earlier commit than the one that > > yields the test failure. You can't rewind an interactive rebase, so I > > complete and restart it, adding an "e"dit at the earlier commit. > > In this sort of situation, I often whish to be able to do nested rebases. > Even more because it happen relatively often that I forget that I'm > working in a rebase and not on the head, and then it's quite natural > to me to type things like 'git rebase -i @^^^' while already rebasing. > But I suppose this has already been discussed. Varieties of this have been discussed, but no, not nested rebases. The closest we thought about was re-scheduling the latest commits, which is now harder because of the `--rebase-merges` mode. But I think it would be doable. Your idea of a "nested" rebase actually opens that door quite nicely. It would not *really* be a nested rebase, and it would still only be possible in interactive mode, but I could totally see git rebase --nested -i HEAD~3 to generate and prepend the following lines to the `git-rebase-todo` file: reset abcdef01 # This is HEAD~3 pick abcdef02 # This is HEAD~2 pick abcdef03 # This is HEAD~ pick abcdef04 # This is HEAD (assuming that the latest 3 commits were non-merge commits; It would look quite a bit more complicated in other situations.) Ciao, Dscho
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Peff, On Sat, 1 Dec 2018, Jeff King wrote: > On Thu, Nov 29, 2018 at 09:32:48AM +0100, Johannes Schindelin wrote: > > > > > Would it not make more sense to add a command-line option (and a config > > > > setting) to re-schedule failed `exec` commands? Like so: > > > > > > Your proposition would do in most cases, however it is not possible to > > > make a distinction between reschedulable and non-reschedulable commands. > > > > True. But I don't think that's so terrible. > > > > What I think is something to avoid is two commands that do something very, > > very similar, but with two very, very different names. > > > > In reality, I think that it would even make sense to change the default to > > reschedule failed `exec` commands. Which is why I suggested to also add a > > config option. > > I sometimes add "x false" to the top of the todo list to stop and create > new commits before the first one. That would be awkward if I could never > get past that line. However, I think elsewhere a "pause" line has been > discussed, which would serve the same purpose. Yep, `break`, as Eric pointed out. After all, you did not really want a command to fail, you just wanted the interactive rebase to give you a break. > I wonder how often this kind of "yes, I know it fails, but keep going > anyway" situation would come up. And what the interface is like for > getting past it. E.g., what if you fixed a bunch of stuff but your tests > still fail? You may not want to abandon the changes you've made, but you > need to "rebase --continue" to move forward. I encounter this often when > the correct fix is actually in an earlier commit than the one that > yields the test failure. You can't rewind an interactive rebase, so I > complete and restart it, adding an "e"dit at the earlier commit. > > How would I move past the test that fails to continue? I guess "git > rebase --edit-todo" and then manually remove it (and any other remaining > test lines)? Yes, the current way would be to use `git rebase --edit-todo`. > That's not too bad, but I wonder if people would find it more awkward > than the current way (which is to just "rebase --continue" until you get > to the end). > > I dunno. I am not sure if I am for or against changing the default, so > these are just some musings. :) It's good that you chimed in with your side of things. If you missed the `break` command, so will many others have missed it. And continue to miss it. Besides, Junio mentioned elsewhere that he is in the camp of people who wait for enough users to complain why some config option isn't the default to actually change the default. So... I guess we'll leave the default where it is for now. Ciao, Dscho
Re: [PATCH v3 06/16] sequencer: refactor sequencer_add_exec_commands() to work on a todo_list
Hi,
On Fri, 30 Nov 2018, Phillip Wood wrote:
> > diff --git a/sequencer.c b/sequencer.c
> > index 900899ef20..11692d0b98 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -4394,24 +4394,29 @@ int sequencer_make_script(FILE *out, int argc, const
> > char **argv,
> > return 0;
> > }
> >
> > -/*
> > - * Add commands after pick and (series of) squash/fixup commands
> > - * in the todo list.
> > - */
> > -int sequencer_add_exec_commands(const char *commands)
> > +static void todo_list_add_exec_commands(struct todo_list *todo_list,
> > + struct string_list *commands)
> > {
> > - const char *todo_file = rebase_path_todo();
> > - struct todo_list todo_list = TODO_LIST_INIT;
> > - struct strbuf *buf = _list.buf;
> > - size_t offset = 0, commands_len = strlen(commands);
> > - int i, insert;
> > + struct strbuf *buf = _list->buf;
> > + const char *old_buf = buf->buf;
> > + size_t base_length = buf->len;
> > + int i, insert, nr = 0, alloc = 0;
> > + struct todo_item *items = NULL, *base_items = NULL;
> >
> > - if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
> > - return error(_("could not read '%s'."), todo_file);
> > + for (i = 0; i < commands->nr; ++i) {
> > + strbuf_addstr(buf, commands->items[i].string);
> > + strbuf_addch(buf, '\n');
> > + }
>
> This could cause buf->buf to be reallocated in which case all the
> todo_list_item.arg pointers will be invalidated.
I guess it is a good time for me to admit that the `arg` idea was not my
best. Maybe it would be good to convert that from a pointer to an offset,
e.g. `size_t arg_offset_in_buf;`? Or maybe we should just drop the
`_in_buf` suffix and clarify in a comment next to the declaration of the
fields that they are offsets in the strbuf?
Ciao,
Dscho
Re: [PATCH] builtin/rebase.c: remove superfluous space in messages
Hi Ralf,
On Fri, 30 Nov 2018, Ralf Thielow wrote:
> Signed-off-by: Ralf Thielow
ACK.
The commit message could state that the scripted rebase does not have
those whitespace issues, and that this aligns the built-in rebase with it,
but I won't insist.
Ciao,
Johannes
> ---
> builtin/rebase.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5b3e5baec8..a6acba76b4 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -871,7 +871,7 @@ int cmd_rebase(int argc, const char **argv, const char
> *prefix)
> "them"), REBASE_PRESERVE_MERGES),
> OPT_BOOL(0, "rerere-autoupdate",
>_rerere_autoupdate,
> - N_("allow rerere to update index with resolved "
> + N_("allow rerere to update index with resolved "
> "conflict")),
> OPT_BOOL('k', "keep-empty", _empty,
>N_("preserve empty commits during rebase")),
> @@ -1520,7 +1520,7 @@ int cmd_rebase(int argc, const char **argv, const char
> *prefix)
>*/
> strbuf_reset();
> if (!oidcmp(_base, _head)) {
> - printf(_("Fast-forwarded %s to %s. \n"),
> + printf(_("Fast-forwarded %s to %s.\n"),
> branch_name, options.onto_name);
> strbuf_addf(, "rebase finished: %s onto %s",
> options.head_name ? options.head_name : "detached HEAD",
> --
> 2.20.0.rc1.379.g1dd7ef354c
>
>
Re: en/rebase-merge-on-sequencer, was Re: What's cooking in git.git (Nov 2018, #07; Fri, 30)
Hi Junio, On Fri, 30 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > On Fri, 30 Nov 2018, Junio C Hamano wrote: > > > >> * en/rebase-merge-on-sequencer (2018-11-08) 2 commits > >> - rebase: implement --merge via git-rebase--interactive > >> - git-rebase, sequencer: extend --quiet option for the interactive > >> machinery > >> > >> "git rebase --merge" as been reimplemented by reusing the internal > >> machinery used for "git rebase -i". > > > > I *think* a new iteration has landed (which has 7 instead of 2 commits): > > https://public-inbox.org/git/20181122044841.20993-1-new...@gmail.com/ > > "Landed" as opposed to "be in-flight"? > > You got me worried by implying that I merged them to either 'master' > or 'next' where it is harder to back out ;-). > > During the freeze, especially after -rc1, I stop paying attention to > anything other than regression fixes and fixes to the addition since > the previous releases, unless I have too much time and get bored and > the new topic is trivial (which often means a single patch). I thought so, but then I thought I had seen you commenting on the (distinctly non-single patch distinctly non-regression fix) `split-checkout-into-two-new-commands` patch series ;-) > I'll mark the topic with the following, and continue ignoring it (or > any other topics) for now. Thanks. > > * en/rebase-merge-on-sequencer (2018-11-08) 2 commits > - rebase: implement --merge via git-rebase--interactive > - git-rebase, sequencer: extend --quiet option for the interactive machinery > > "git rebase --merge" as been reimplemented by reusing the internal > machinery used for "git rebase -i". > > Reroll exists. > cf. <20181122044841.20993-1-new...@gmail.com> Thank you, much appreciated. Dscho
en/rebase-merge-on-sequencer, was Re: What's cooking in git.git (Nov 2018, #07; Fri, 30)
Hi Junio, On Fri, 30 Nov 2018, Junio C Hamano wrote: > * en/rebase-merge-on-sequencer (2018-11-08) 2 commits > - rebase: implement --merge via git-rebase--interactive > - git-rebase, sequencer: extend --quiet option for the interactive machinery > > "git rebase --merge" as been reimplemented by reusing the internal > machinery used for "git rebase -i". I *think* a new iteration has landed (which has 7 instead of 2 commits): https://public-inbox.org/git/20181122044841.20993-1-new...@gmail.com/ Assuming that the -rc2 work has been interfering, I guess you wanted to pick that up some time after -rc2 or even after -final? Ciao, Dscho
Re: [PATCH] format-patch: do not let its diff-options affect --range-diff (was Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options)
Hi Junio, On Fri, 30 Nov 2018, Junio C Hamano wrote: > Junio C Hamano writes: > > >> I had to delay -rc2 to see these last minute tweaks come to some > >> reasonable place to stop at, and I do not think we want to delay the > >> final any longer or destablizing it further by piling last minute > >> undercooked changes on top. > > > > So how about doing this on top of 'master' instead? As this leaks > > *no* information wrt how range-diff machinery should behave from the > > format-patch side by not passing any diffopt, as long as the new > > code I added to show_range_diff() comes up with a reasonable default > > diffopts (for which I really would appreciate extra sets of eyes to > > make sure), this change by definition cannot be wrong (famous last > > words). > > As listed in today's "What's cooking" report, I've merged this to > 'next' in today's pushout and planning to have it in the -rc2. I am > not married to this exact implementation, and I'd welcome to have an > even simpler and less disruptive solution if exists, but I am hoping > that this is a good-enough interim measure for the upcoming release, > until we decide what to do with the customizability of range-diff > driven by format-patch. > > In addition to this, I am planning the "rebase --stat" and "reflog > that does not say 'rebase -i' but 'rebase'" fixes merged to 'master' > before cutting -rc2. Thank you for integrating them. That way, we have an -rc2 with no issues in the built-in rebase/rebase -i that we know of. Ciao, Dscho
[PATCH 1/1] rebase: fix GIT_REFLOG_ACTION regression
From: Johannes Schindelin
The scripted version (partially) heeded the `GIT_REFLOG_ACTION` and when
we converted to a built-in, this regressed.
Fix that, and add a regression test, both with `GIT_REFLOG_ACTION` set
and unset.
Note: the reflog message for "rebase finished" did *not* heed
GIT_REFLOG_ACTION, and as we are very late in the v2.20.0-rcN phase, we
leave that bug for later (as it seems that that bug has been with us
from the very beginning).
Reported by Ian Jackson.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 29 ++---
t/t3406-rebase-message.sh | 26 ++
2 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b3e5baec8..ba0c3c954b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -776,6 +776,23 @@ static void NORETURN
error_on_missing_default_upstream(void)
exit(1);
}
+static void set_reflog_action(struct rebase_options *options)
+{
+ const char *env;
+ struct strbuf buf = STRBUF_INIT;
+
+ if (!is_interactive(options))
+ return;
+
+ env = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
+ if (env && strcmp("rebase", env))
+ return; /* only override it if it is "rebase" */
+
+ strbuf_addf(, "rebase -i (%s)", options->action);
+ setenv(GIT_REFLOG_ACTION_ENVIRONMENT, buf.buf, 1);
+ strbuf_release();
+}
+
int cmd_rebase(int argc, const char **argv, const char *prefix)
{
struct rebase_options options = {
@@ -978,6 +995,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
if (action != NO_ACTION && !in_progress)
die(_("No rebase in progress?"));
+ setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
if (action == ACTION_EDIT_TODO && !is_interactive())
die(_("The --edit-todo action can only be used during "
@@ -990,6 +1008,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
int fd;
options.action = "continue";
+ set_reflog_action();
/* Sanity check */
if (get_oid("HEAD", ))
@@ -1018,6 +1037,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
struct string_list merge_rr = STRING_LIST_INIT_DUP;
options.action = "skip";
+ set_reflog_action();
rerere_clear(_rr);
string_list_clear(_rr, 1);
@@ -1033,6 +1053,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
case ACTION_ABORT: {
struct string_list merge_rr = STRING_LIST_INIT_DUP;
options.action = "abort";
+ set_reflog_action();
rerere_clear(_rr);
string_list_clear(_rr, 1);
@@ -1440,11 +1461,12 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
}
strbuf_reset();
- strbuf_addf(, "rebase: checkout %s",
+ strbuf_addf(, "%s: checkout %s",
+
getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
options.switch_to);
if (reset_head(, "checkout",
options.head_name, 0,
- NULL, NULL) < 0) {
+ NULL, buf.buf) < 0) {
ret = !!error(_("could not switch to "
"%s"),
options.switch_to);
@@ -1508,7 +1530,8 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
printf(_("First, rewinding head to replay your work on top of "
"it...\n"));
- strbuf_addf(, "rebase: checkout %s", options.onto_name);
+ strbuf_addf(, "%s: checkout %s",
+ getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
if (reset_head(>object.oid, "checkout", NULL,
RESET_HEAD_DETACH, NULL, msg.buf))
die(_("Could not detach HEAD"));
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 38bd876cab..db8505eb86 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -91,4 +91,30 @@ test_expect_success 'error out early upon -C or
--whitespace=' '
test_i18ngrep "Invalid whitespace option" err
'
+test_expect_success 'GIT_REFLOG_ACTION' '
+ git checkout
[PATCH 0/1] Fix built-in rebase regression noticed by Debian's dgit
It has been reported on the Debian bug tracker [https://bugs.debian.org/914695] that the built-in rebase regresses on the scripted version, and later details emerged that this has something to do with the reflog messages: they were different with the built-in rebase than with the scripted one. This here patch fixes that. Johannes Schindelin (1): rebase: fix GIT_REFLOG_ACTION regression builtin/rebase.c | 29 ++--- t/t3406-rebase-message.sh | 26 ++ 2 files changed, 52 insertions(+), 3 deletions(-) base-commit: 7068cbc4abac53d9c3675dfba81c1e97d25e8eeb Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-91%2Fdscho%2Ffix-reflog-action-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-91/dscho/fix-reflog-action-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/91 -- gitgitgadget
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
Hi Ævar, On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > On Thu, Nov 29 2018, Johannes Schindelin wrote: > > > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > > > >> On Thu, Nov 29 2018, Johannes Schindelin wrote: > >> > >> > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > >> > > >> >> On Thu, Nov 29 2018, Johannes Schindelin wrote: > >> >> > >> >> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > >> >> > > >> >> >> Change the semantics of the "--range-diff" option so that the regular > >> >> >> diff options can be provided separately for the range-diff and the > >> >> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to > >> >> >> "format-patch" to provide different context for the range-diff and > >> >> >> the > >> >> >> patch. This wasn't possible before. > >> >> > > >> >> > I really, really dislike the `--range-diff-`. We have > >> >> > precedent for passing optional arguments that are passed to some other > >> >> > command, so a much more logical and consistent convention would be to > >> >> > use > >> >> > `--range-diff[=..]`, allowing all of the diff options > >> >> > that > >> >> > you might want to pass to the outer diff in one go rather than having > >> >> > a > >> >> > lengthy string of `--range-diff-this` and `--range-diff-that` options. > >> >> > >> >> Where do we pass those sorts of arguments? > >> >> > >> >> Reasons I did it this way: > >> >> > >> >> a) Passing it as one option will require the user to double-quote those > >> >> options that take quoted arguments (e.g. --word-diff-regex), which I > >> >> thought sucked more than the prefix. On the implementation side we > >> >> couldn't leave the parsing of the command-line to the shell anymore. > >> >> > >> >> b) I think people will want to tweak this very rarely, much more rarely > >> >> than e.g. -U10 in format-patch itself, so having something long-ish > >> >> doesn't sound bad. > >> > > >> > Hmm. I still don't like it. It sets a precedent, and we simply do not do > >> > it that way in other circumstances (most obvious would be the -X merge > >> > options). The more divergent user interfaces for the same sort of thing > >> > are, the more brain cycles you force users to spend on navigating said > >> > interfaces. > >> > >> Yeah it sucks, I just think it sucks less than the alternative :) > >> I.e. I'm not picky about --range-diff-* prefix the name, but I think > >> doing our own shell parsing would be nasty. > > > > What prevents you from using `sq_dequote_to_argv()`? > > I mean not just nasty in terms of implementation, yeah we could do it, > but also a nasty UX for things like --word-diff-regex. I.e. instead of: > > --range-diff-word-diff-regex='[0-9"]' > > You need: > > --range-diff-opts="--word-diff-regex='[0-9\"]'" Really? I think that would not work. It would pass the single quotes as part of the regex to the diff machinery. Or maybe not. But the extra quotes do not strike me as necessary, as there is no shell script involved (thank deity!) after `git range-diff` parsed the options. > Now admittedly that in itself isn't very painful *in this case*, but in > terms of precedent I really dislike that option, i.e. git having some > mode where I need to work to escape input to pass to another command. > > Not saying that this --range-diff-* thing is what we should go for, but > surely we can find some way to do deal with this that doesn't involve > the user needing to escape stuff like this. > > It also has other downstream effects in the UI, e.g. it's presumably > easy to teach the bash completion that a --foo=XYZ option is also called > --some-prefix--foo=XYZ and to enable completion for that, less so for > making it smart enough to complete "--some-prefix-opts="--foo=". These are all good points, and need proper discussion. Sadly, all that time needed for a proper discussion is not left before v2.20.0 is supposed to come out. Quite honestly, I think what we will have to do is to describe in the documentation of `format-patch`'s `--range-diff` option that the exact user interface how to pass diff options down to `range-diff` is in flux and not final. That way, we can give your design the proper treatment, and work together on making a user interface we all can be happy with. Ciao, Dscho
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
Hi Ian,
On Thu, 29 Nov 2018, Ian Jackson wrote:
> Johannes Schindelin writes ("Re: [PATCH] rebase: mark the C reimplementation
> as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)"):
> > > In a successful run with older git I get a reflog like this:
> > >
> > >4833d74 HEAD@{0}: rebase finished: returning to
> > > refs/heads/with-preexisting
> > >4833d74 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another
> > > new upstream file
> > >cabd5ec HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c
> > > file
> > >0b362ce HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new
> > > upstream file
> > >29653e5 HEAD@{4}: debrebase new-upstream 2.1-1: rebase: checkout
> > > 29653e5a17bee4ac23a68bba3e12bc1f52858ac3
> > >85e0c46 HEAD@{5}: debrebase: launder for new upstream
> ...
> > > This breaks the test because my test suite is checking that I set
> > > GIT_REFLOG_ACTION appropriately.
> > >
> > > If you want I can provide a minimal test case but this should suffice
> > > to see the bug I hope...
> >
> > This should be plenty for me to get going. Thank you!
>
> Happy hunting.
I'll have to take a (lengthy) dinner break now, but this is what I have so
far: a regression test that verifies the breakage (see the
`fix-reflog-action` branch at https://github.com/dscho/git). I'll continue
after dinner and am confident that this bug will be fixed within the next
four hours.
> While you're looking at this, I observe that the fact that the `rebase
> finished' message also does not honour GIT_REFLOG_ACTION appears to be
> a pre-existing bug.
I noticed that, too, but at this point I am only fixing regressions. We
can try to fix this long-standing bug in the v2.20 cycle.
Ciao,
Johannes
> (In general one often can't rely on GIT_REFLOG_ACTION still being set
> because the rebase might have been interrupted and restarted, which I
> think is why my test case looks for it in the initial `checkout'
> message.)
>
> Regards,
> Ian.
>
> --
> Ian JacksonThese opinions are my own.
>
> If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
> a private address which bypasses my fierce spamfilter.
>
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
Hi Ævar,
On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> On Thu, Nov 29 2018, Johannes Schindelin wrote:
>
> > On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> >> On Thu, Nov 29 2018, Johannes Schindelin wrote:
> >>
> >> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >> >
> >> >> Change the semantics of the "--range-diff" option so that the regular
> >> >> diff options can be provided separately for the range-diff and the
> >> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
> >> >> "format-patch" to provide different context for the range-diff and the
> >> >> patch. This wasn't possible before.
> >> >
> >> > I really, really dislike the `--range-diff-`. We have
> >> > precedent for passing optional arguments that are passed to some other
> >> > command, so a much more logical and consistent convention would be to use
> >> > `--range-diff[=..]`, allowing all of the diff options that
> >> > you might want to pass to the outer diff in one go rather than having a
> >> > lengthy string of `--range-diff-this` and `--range-diff-that` options.
> >>
> >> Where do we pass those sorts of arguments?
> >>
> >> Reasons I did it this way:
> >>
> >> a) Passing it as one option will require the user to double-quote those
> >> options that take quoted arguments (e.g. --word-diff-regex), which I
> >> thought sucked more than the prefix. On the implementation side we
> >> couldn't leave the parsing of the command-line to the shell anymore.
> >>
> >> b) I think people will want to tweak this very rarely, much more rarely
> >> than e.g. -U10 in format-patch itself, so having something long-ish
> >> doesn't sound bad.
> >
> > Hmm. I still don't like it. It sets a precedent, and we simply do not do
> > it that way in other circumstances (most obvious would be the -X merge
> > options). The more divergent user interfaces for the same sort of thing
> > are, the more brain cycles you force users to spend on navigating said
> > interfaces.
>
> Yeah it sucks, I just think it sucks less than the alternative :)
> I.e. I'm not picky about --range-diff-* prefix the name, but I think
> doing our own shell parsing would be nasty.
What prevents you from using `sq_dequote_to_argv()`?
> >> > I only had time to skim the patch, and I have to wonder why you pass
> >> > around full-blown `rev_info` structs for range diff (and with that really
> >> > awful name `rd_rev`) rather than just the `diff_options` that you
> >> > *actually* care about?
> >>
> >> Because setup_revisions() which does all the command-line parsing needs
> >> a rev_info, so even if we only need the diffopt in the end we need to
> >> initiate the whole thing.
> >>
> >> Suggestions for a better varibale name most welcome.
> >
> > `range_diff_revs`
> >
> > And you do not need to pass around the whole thing. You can easily pass
> > `_diff_revs.diffopt`.
> >
> > Don't pass around what you do not need to pass around.
>
> Ah, you mean internally in log.c, yes that makes sense. I thought you
> meant just pass diffopt to setup_revisions() (which needs the containing
> struct). Willdo.
Thanks,
Dscho
>
> > Ciao,
> > Dscho
> >
> >>
> >> > Ciao,
> >> > Dscho
> >> >
> >> >>
> >> >> Ever since the "--range-diff" option was added in
> >> >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
> >> >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
> >> >> machinery has been the one we get from "format-patch"'s own
> >> >> setup_revisions().
> >> >>
> >> >> This sort of thing is unique among the log-like commands in
> >> >> builtin/log.c, no command than format-patch will embed the output of
> >> >> another log-like command. Since the "rev->diffopt" is reused we need
> >> >> to munge it before we pass it to show_range_diff(). See
> >> >> 43dafc4172 ("format-patch: don't include --stat with --range-diff
> >> >> output", 2018-11-22) for a related regression fix which is being
> >> >> mostly reverted here.
> >> >>
> >> >> Implementation notes: 1) W
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
Hi Ian,
On Thu, 29 Nov 2018, Ian Jackson wrote:
> Johannes Schindelin writes ("Re: [PATCH] rebase: mark the C reimplementation
> as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)"):
> > if you could pry more information (or better information) out of that bug
> > reporter, that would be nice. Apparently my email address is blacklisted
> > by his mail provider, so he is unlikely to have received my previous mail
> > (nor will he receive this one, I am sure).
>
> (I did receive this mail. Sorry for the inconvenience, which sadly is
> inevitable occasionally in the modern email world. FTR in future feel
> free to send the bounce to postmaster@chiark and I will make a
> you-shaped hole in my spamfilter. Also with Debian bugs you can
> launder your messages by, eg, emailing 914695-submitter@bugs.)
Right. I myself have plenty of email-related problems that seem to crop up
this year in particular.
> > > > At https://bugs.debian.org/914695 is a report of a test regression in
> > > > an outside project that is very likely to have been triggered by the
> > > > new faster rebase code.
>
> As I wrote in the bug report last night:
>
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914695#15
>
> I have investigated and the bug seems to be that git-rebase --onto now
> fails to honour GIT_REFLOG_ACTION for the initial checkout.
>
> In a successful run with older git I get a reflog like this:
>
>4833d74 HEAD@{0}: rebase finished: returning to refs/heads/with-preexisting
>4833d74 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new
> upstream file
>cabd5ec HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
>0b362ce HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream
> file
>29653e5 HEAD@{4}: debrebase new-upstream 2.1-1: rebase: checkout
> 29653e5a17bee4ac23a68bba3e12bc1f52858ac3
>85e0c46 HEAD@{5}: debrebase: launder for new upstream
>
> With a newer git I get this:
>
>6d3fb91 HEAD@{0}: rebase finished: returning to refs/heads/master
>6d3fb91 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new
> upstream file
>86c0721 HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
>50ba56c HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream
> file
>8272825 HEAD@{4}: rebase: checkout 8272825bb4ff6eba89afa936e32b6460f963a36a
>c78db55 HEAD@{5}: debrebase: launder for new upstream
>
> This breaks the test because my test suite is checking that I set
> GIT_REFLOG_ACTION appropriately.
>
> If you want I can provide a minimal test case but this should suffice
> to see the bug I hope...
This should be plenty for me to get going. Thank you!
Ciao,
Johannes
>
> Regards
> Ian.
>
> --
> Ian JacksonThese opinions are my own.
>
> If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
> a private address which bypasses my fierce spamfilter.
>
Re: [PATCH v1] mem_pool: add GIT_TRACE_MEMPOOL support
Hi Ben,
On Thu, 29 Nov 2018, Ben Peart wrote:
> On 11/28/2018 4:37 AM, Johannes Schindelin wrote:
> > Hi Ben,
> >
> > On Tue, 27 Nov 2018, Ben Peart wrote:
> >
> > > From: Ben Peart
> > >
> > > Add tracing around initializing and discarding mempools. In discard report
> > > on the amount of memory unused in the current block to help tune setting
> > > the initial_size.
> > >
> > > Signed-off-by: Ben Peart
> > > ---
> > Looks good.
> >
> > My only question: should we also trace calls to _alloc(), _calloc() and
> > _combine()?
>
> I was trying to tune the initial size in my use of the mem_pool and so found
> this tracing useful to see how much memory was actually being used. I'm
> inclined to only add tracing as it is needed rather that proactively because
> we think it _might_ be needed. I suspect _alloc() and _calloc() would get
> very noisy and not add much value.
In other words, YAGNI. Makes sense.
Thanks,
Johannes
>
> >
> > Ciao,
> > Johannes
> >
> > > Notes:
> > > Base Ref: * git-trace-mempool
> > > Web-Diff: https://github.com/benpeart/git/commit/9ac84bbca2
> > > Checkout: git fetch https://github.com/benpeart/git
> > > git-trace-mempool-v1 && git checkout 9ac84bbca2
> > >
> > > mem-pool.c | 5 +
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/mem-pool.c b/mem-pool.c
> > > index a2841a4a9a..065389aaec 100644
> > > --- a/mem-pool.c
> > > +++ b/mem-pool.c
> > > @@ -5,6 +5,7 @@
> > > #include "cache.h"
> > > #include "mem-pool.h"
> > >
> > > +static struct trace_key trace_mem_pool = TRACE_KEY_INIT(MEMPOOL);
> > > #define BLOCK_GROWTH_SIZE 1024*1024 - sizeof(struct mp_block);
> > >
> > > /*
> > > @@ -48,12 +49,16 @@ void mem_pool_init(struct mem_pool **mem_pool, size_t
> > > initial_size)
> > > mem_pool_alloc_block(pool, initial_size, NULL);
> > >
> > > *mem_pool = pool;
> > > + trace_printf_key(_mem_pool, "mem_pool (%p): init (%"PRIuMAX")
> > > initial size\n",
> > > + pool, (uintmax_t)initial_size);
> > > }
> > >
> > > void mem_pool_discard(struct mem_pool *mem_pool, int invalidate_memory)
> > > {
> > >struct mp_block *block, *block_to_free;
> > > + trace_printf_key(_mem_pool, "mem_pool (%p): discard
> > > (%"PRIuMAX")
> > > unused\n",
> > > + mem_pool, (uintmax_t)(mem_pool->mp_block->end -
> > > mem_pool->mp_block->next_free));
> > >block = mem_pool->mp_block;
> > >while (block)
> > >{
> > >
> > > base-commit: bb75be6cb916297f271c846f2f9caa3daaaec718
> > > --
> > > 2.18.0.windows.1
> > >
> > >
>
>
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
Hi Jonathan, if you could pry more information (or better information) out of that bug reporter, that would be nice. Apparently my email address is blacklisted by his mail provider, so he is unlikely to have received my previous mail (nor will he receive this one, I am sure). Thanks, Dscho On Wed, 28 Nov 2018, Johannes Schindelin wrote: > Hi Jonathan, > > On Tue, 27 Nov 2018, Jonathan Nieder wrote: > > > At https://bugs.debian.org/914695 is a report of a test regression in > > an outside project that is very likely to have been triggered by the > > new faster rebase code. > > From looking through that log.gz (without having a clue where the test > code lives, so I cannot say what it is supposed to do, and also: this is > the first time I hear about dgit...), it would appear that this must be a > regression in the reflog messages produced by `git rebase`. > > > The issue has not been triaged, so I don't know yet whether it's a > > problem in rebase-in-c or a manifestation of a bug in the test. > > It ends thusly: > > -- snip -- > [...] > + git reflog > + egrep 'debrebase new-upstream.*checkout' > + test 1 = 0 > + t-report-failure > + set +x > TEST FAILED > -- snap -- > > Which makes me think that the reflog we produce in *some* code path that > originally called `git checkout` differs from the scripted rebase's > generated reflog. > > > That said, Google has been running with the new rebase since ~1 month > > ago when it became the default, with no issues reported by users. As a > > result, I am confident that it can cope with what most users of "next" > > throw at it, which means that if we are to find more issues to polish it > > better, it will need all the exposure it can get. > > Right. And there are a few weeks before the holidays, which should give me > time to fix whatever bugs are discovered (I only half mind being the only > one who fixes these bugs). > > > In the Google deployment, we will keep using rebase-in-c even if it > > gets disabled by default, in order to help with that. > > > > From the Debian point of view, it's only a matter of time before > > rebase-in-c becomes the default: even if it's not the default in 2.20, > > it would presumably be so in 2.21 or 2.22. That means the community's > > attention when resolving security and reliability bugs would be on the > > rebase-in-c implementation. As a result, the Debian package will most > > likely enable rebase-in-c by default even if upstream disables it, in > > order to increase the package's shelf life (i.e. to ease the > > maintenance burden of supporting whichever version of the package ends > > up in the next Debian stable). > > > > So with either hat on, it doesn't matter whether you apply this patch > > upstream. > > > > Having two pretty different deployments end up with the same > > conclusion leads me to suspect that it's best for upstream not to > > apply the revert patch, unless either > > > > (a) we have a concrete regression to address and then try again, or > > (b) we have a test or other plan to follow before trying again. > > In this instance, I am more a fan of the "let's move fast and break > things, then move even faster fixing them" approach. > > Besides, the bug that Ævar discovered was a bug already in the scripted > rebase, but hidden by yet another bug (the missing error checking). > > I get the pretty firm impression that the common code paths are now pretty > robust, and only lesser-exercised features may expose a bug (or > regression, as in the case of the reflogs, where one could argue that the > exact reflog message is not something we promise not to fiddle with). > > Ciao, > Dscho
Re: [PATCH v11 00/22] Convert "git stash" to C builtin
Hi Junio, On Thu, 29 Nov 2018, Johannes Schindelin wrote: > On Mon, 26 Nov 2018, Junio C Hamano wrote: > > > Junio C Hamano writes: > > > > > Thomas Gummerer writes: > > > > > >> Thanks for your work on this! I have read through the range-diff and > > >> the new patch of this last round, and this addresses all the comments > > >> I had on v10 (and some more :)). I consider it > > >> Reviewed-by: Thomas Gummerer > > > > > > Thanks. > > > > > > One thing that bothers me is that this seems to have been rebased on > > > 'master', but as long as we are rebasing, the updated series must > > > also take into account of the sd/stash-wo-user-name topic, i.e. if > > > we are rebasing it, it should be rebased on top of the result of > > > > > > git checkout -B ps/rebase-in-c master > > > git merge --no-ff sd/stash-wo-user-name > > > > > > I think. > > > > https://travis-ci.org/git/git/builds/459619672 would show that this > > C reimplementation now regresses from the scripted version due to > > lack of such rebasing (i.e. porting a correction from scripted one). > > Oh, you know, at first I *mis-read* your mail to mean "don't you rebase > all the time!", but in this case (in contrast to earlier statements about > rebasing between iterations of patch series), you *do* want Paul to > rebase. > > Let me see what I can come up with in my `git-stash` branch on > https://github.com/dscho/git There. I force-pushed an update that is based on sd/stash-wo-user-name and adds a `prepare_fallback_ident(name, email)` to `ident.c` for use in the built-in stash: https://github.com/dscho/git/commit/d37ce623fbd32e4345c701dea822e56de1a5417f It passes t3903 in a little over a minute with GIT_TEST_STASH_USE_BUILTIN=true and in a little less than seven minutes with GIT_TEST_STASH_USE_BUILTIN=false. Ciao, Dscho
[PATCH v2 0/1] Fix git rebase --stat -i
We're really entering obscure territory here, I would say.
To trigger the bug, two things have to come together: the user must have
asked for a diffstat afterwards, and the commits need to have been rebased
onto a completely unrelated commit history (i.e. there must exist no merge
base between the pre-rebase HEAD and the post-rebase HEAD).
Please note that this bug existed already in the scripted rebase, but it was
never detected because the scripted version would not even perform any error
checking.
It will make Junio very happy that I squashed the regression test in to the
patch that fixes it. The reason, however, was not to make Junio happy (I
hope to make him happy by fixing bugs), but simply that an earlier iteration
of test would only fail with the built-in rebase, but not with the scripted
version. The current version would fail with the scripted version, but I'll
save the time to split the patch again now.
Changes since v1:
* The commit message now talks more about what we should do in case that
there is no merge base, rather than stressing the differences between the
way scripted vs built-in rebase handled it (both buggy, and fixed by this
patch).
* In case that there is no merge base, it no longer reports "Changes from
(empty) to ..." but "Changes to ...", which should be a lot less
controversial.
Johannes Schindelin (1):
rebase --stat: fix when rebasing to an unrelated history
builtin/rebase.c | 18 --
git-legacy-rebase.sh | 10 --
t/t3406-rebase-message.sh | 10 ++
3 files changed, 30 insertions(+), 8 deletions(-)
base-commit: a1598010f775d82b5adf12c29d0f5bc9b41434c6
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-88%2Fdscho%2Frebase-stat-fix-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-88/dscho/rebase-stat-fix-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/88
Range-diff vs v1:
1: 680385e4bd ! 1: 190c7856ad rebase --stat: fix when rebasing to an
unrelated history
@@ -3,22 +3,21 @@
rebase --stat: fix when rebasing to an unrelated history
When rebasing to a commit history that has no common commits with the
-current branch, there is no merge base. The scripted version of the
`git
-rebase` command was not prepared for that and spewed out
+current branch, there is no merge base. In diffstat mode, this means
+that we cannot compare to the merge base, but we have to compare to
the
+empty tree instead.
-fatal: ambiguous argument '': unknown revision or path not in
-the working tree.
+Also, if running in verbose diffstat mode, we should not output
-but then continued (due to lack of error checking).
+Changes from to
-The built-in version of the `git rebase` command blindly translated
that
-shell script code, assuming that there is no need to test whether
there
-*was* a merge base, and due to its better error checking, exited with
a
-fatal error (because it tried to read the object with hash ...
-as a tree).
+as that does not make sense without any merge base.
-Fix both scripted and built-in versions to output something sensibly,
-and add a regression test to keep this working in all eternity.
+Note: neither scripted nor built-in versoin of `git rebase` were
+prepared for this situation well. We use this opportunity not only to
+fix the bug(s), but also to make both versions' output consistent in
+this instance. And add a regression test to keep this working in all
+eternity.
Reported-by: Ævar Arnfjörð Bjarmason
Signed-off-by: Johannes Schindelin
@@ -27,15 +26,25 @@
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@
+ if (options.flags & REBASE_DIFFSTAT) {
+ struct diff_options opts;
- if (options.flags & REBASE_VERBOSE)
- printf(_("Changes from %s to %s:\n"),
+- if (options.flags & REBASE_VERBOSE)
+- printf(_("Changes from %s to %s:\n"),
- oid_to_hex(_base),
-+ is_null_oid(_base) ?
-+ "(empty)" : oid_to_hex(_base),
- oid_to_hex(>object.oid));
+- oid_to_hex(>object.oid));
++ if (options.flags & REBASE_VERBOSE) {
++ if (is_null_oid(_base))
++ printf(_("Changes to %s:\n"),
++oid_to_hex(>object.oid));
++ else
++ printf(_
[PATCH v2 1/1] rebase --stat: fix when rebasing to an unrelated history
From: Johannes Schindelin
When rebasing to a commit history that has no common commits with the
current branch, there is no merge base. In diffstat mode, this means
that we cannot compare to the merge base, but we have to compare to the
empty tree instead.
Also, if running in verbose diffstat mode, we should not output
Changes from to
as that does not make sense without any merge base.
Note: neither scripted nor built-in versoin of `git rebase` were
prepared for this situation well. We use this opportunity not only to
fix the bug(s), but also to make both versions' output consistent in
this instance. And add a regression test to keep this working in all
eternity.
Reported-by: Ævar Arnfjörð Bjarmason
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 18 --
git-legacy-rebase.sh | 10 --
t/t3406-rebase-message.sh | 10 ++
3 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b3e5baec8..1c6f817f4b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1481,10 +1481,15 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
if (options.flags & REBASE_DIFFSTAT) {
struct diff_options opts;
- if (options.flags & REBASE_VERBOSE)
- printf(_("Changes from %s to %s:\n"),
- oid_to_hex(_base),
- oid_to_hex(>object.oid));
+ if (options.flags & REBASE_VERBOSE) {
+ if (is_null_oid(_base))
+ printf(_("Changes to %s:\n"),
+ oid_to_hex(>object.oid));
+ else
+ printf(_("Changes from %s to %s:\n"),
+ oid_to_hex(_base),
+ oid_to_hex(>object.oid));
+ }
/* We want color (if set), but no pager */
diff_setup();
@@ -1494,8 +1499,9 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
opts.detect_rename = DIFF_DETECT_RENAME;
diff_setup_done();
- diff_tree_oid(_base, >object.oid,
- "", );
+ diff_tree_oid(is_null_oid(_base) ?
+ the_hash_algo->empty_tree : _base,
+ >object.oid, "", );
diffcore_std();
diff_flush();
}
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index b97ffdc9dd..b4c7dbfa57 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -718,10 +718,16 @@ if test -n "$diffstat"
then
if test -n "$verbose"
then
- echo "$(eval_gettext "Changes from \$mb to \$onto:")"
+ if test -z "$mb"
+ then
+ echo "$(eval_gettext "Changes to \$onto:")"
+ else
+ echo "$(eval_gettext "Changes from \$mb to \$onto:")"
+ fi
fi
+ mb_tree="${mb:-$(git hash-object -t tree /dev/null)}"
# We want color (if set), but no pager
- GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
+ GIT_PAGER='' git diff --stat --summary "$mb_tree" "$onto"
fi
test -n "$interactive_rebase" && run_specific_rebase
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 38bd876cab..c2c9950568 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -91,4 +91,14 @@ test_expect_success 'error out early upon -C or
--whitespace=' '
test_i18ngrep "Invalid whitespace option" err
'
+test_expect_success 'rebase -i onto unrelated history' '
+ git init unrelated &&
+ test_commit -C unrelated 1 &&
+ git -C unrelated remote add -f origin "$PWD" &&
+ git -C unrelated branch --set-upstream-to=origin/master &&
+ git -C unrelated -c core.editor=true rebase -i -v --stat >actual &&
+ test_i18ngrep "Changes to " actual &&
+ test_i18ngrep "5 files changed" actual
+'
+
test_done
--
gitgitgadget
Re: [PATCH v11 00/22] Convert "git stash" to C builtin
Hi Junio, On Mon, 26 Nov 2018, Junio C Hamano wrote: > Junio C Hamano writes: > > > Thomas Gummerer writes: > > > >> Thanks for your work on this! I have read through the range-diff and > >> the new patch of this last round, and this addresses all the comments > >> I had on v10 (and some more :)). I consider it > >> Reviewed-by: Thomas Gummerer > > > > Thanks. > > > > One thing that bothers me is that this seems to have been rebased on > > 'master', but as long as we are rebasing, the updated series must > > also take into account of the sd/stash-wo-user-name topic, i.e. if > > we are rebasing it, it should be rebased on top of the result of > > > > git checkout -B ps/rebase-in-c master > > git merge --no-ff sd/stash-wo-user-name > > > > I think. > > https://travis-ci.org/git/git/builds/459619672 would show that this > C reimplementation now regresses from the scripted version due to > lack of such rebasing (i.e. porting a correction from scripted one). Oh, you know, at first I *mis-read* your mail to mean "don't you rebase all the time!", but in this case (in contrast to earlier statements about rebasing between iterations of patch series), you *do* want Paul to rebase. Let me see what I can come up with in my `git-stash` branch on https://github.com/dscho/git Ciao, Dscho
Re: [PATCH 1/1] rebase --stat: fix when rebasing to an unrelated history
Hi Junio,
On Thu, 29 Nov 2018, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget"
> writes:
>
> > The built-in version of the `git rebase` command blindly translated that
> > shell script code, assuming that there is no need to test whether there
> > *was* a merge base, and due to its better error checking, exited with a
> > fatal error (because it tried to read the object with hash ...
> > as a tree).
>
> And the scripted version produced a wrong result because it did not
> check the lack of merge base and fed an empty string (presumably, as
> that is what you would get from mb=$(merge-base A B) when A and B
> are unrelated) to later steps? If that is the case, then it *is* a
> very significant thing to record here. As it was not merely "failed
> to stop due to lack of error checking", but a lot worse. It was
> producing a wrong result.
Indeed. But it was only in the `--stat` reporting, so it did not produce
an incorrect rebased history.
> A more faithful reimplementation would not have exited with fatal
> error, but would have produced the same wrong result, so "a better
> error checking caused the reimplementation die where the original
> did not die" may be correct but is much less important than the fact
> that "the logic used in the original to produce diffstat forgot that
> there may not be a merge base and would not have worked correctly in
> such a case, and that is what we are correcting" is more important.
True.
> Please descibe the issue as such, if that is the case (I cannot read
> from the description in the proposed log message if that is the
> case---but I came to the conclusion that it is what you wanted to
> fix reading the code).
Indeed, my commit message is a bit too close to what I fixed, and not
enough about what needed to be fixed.
> > if (options.flags & REBASE_VERBOSE)
> > printf(_("Changes from %s to %s:\n"),
> > - oid_to_hex(_base),
> > + is_null_oid(_base) ?
> > + "(empty)" : oid_to_hex(_base),
>
> I am not sure (empty) is a good idea for two reasons. It is going
> to be inserted into an translated string without translation.
Oooops.
> Also it is too similar to the fallback string used by some printf
> implementations when NULL was given to %s by mistake.
You mean `(null)`? That was actually intentional, I just thought that
`(empty)` would be different enough...
> If there weren't i18n issues, I would suggest to use "empty merge
> base" or "empty tree" instead, but the old one would have shown
> 0{40}, so perhaps empty_tree_oid_hex() is a good substitute?
As this is a user-facing message, I'd rather avoid something like
`empty_tree_oid_hex()`, which to every Git user who does not happen to be
a Git developer, too, must sound very cryptic.
But I guess that I should not be so lazy and really use two different
messages here:
Changes from to
and if there is no merge base,
Changes in
Will fix.
> > @@ -1494,8 +1495,9 @@ int cmd_rebase(int argc, const char **argv, const
> > char *prefix)
> > DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> > opts.detect_rename = DIFF_DETECT_RENAME;
> > diff_setup_done();
> > - diff_tree_oid(_base, >object.oid,
> > - "", );
> > + diff_tree_oid(is_null_oid(_base) ?
> > + the_hash_algo->empty_tree : _base,
> > + >object.oid, "", );
>
> The original would have run "git diff '' $onto", and died with an
> error message, so both the original and the reimplementation were
> failing. Just in different ways ;-)
Right.
> > diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> > index b97ffdc9dd..be3b241676 100755
> > --- a/git-legacy-rebase.sh
> > +++ b/git-legacy-rebase.sh
> > @@ -718,10 +718,12 @@ if test -n "$diffstat"
> > then
> > if test -n "$verbose"
> > then
> > - echo "$(eval_gettext "Changes from \$mb to \$onto:")"
> > + mb_display="${mb:-(empty)}"
> > + echo "$(eval_gettext "Changes from \$mb_display to \$onto:")"
> > fi
> > + mb_tree="${mb:-$(git hash-object -t tree /dev/null)}"
> > # We want color (if set), but no pager
> > - GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
> > + GIT_PAGER='' git diff --stat --summary "$mb_tree" "$onto"
>
> Code fix for diff-tree invocation, both in the builtin/ version and
> this one, looks correct.
Okay. I fixed the two things you pointed out, just waiting for the Linux
phase to finish (I don't think there is anything OS-specific in this
patch, so I trust macOS and Windows to pass if Linux passes):
https://git-for-windows.visualstudio.com/git/_build/results?buildId=26116
Ciao,
Dscho
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
Hi Ævar,
On Thu, 29 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> On Thu, Nov 29 2018, Johannes Schindelin wrote:
>
> > On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> >> Change the semantics of the "--range-diff" option so that the regular
> >> diff options can be provided separately for the range-diff and the
> >> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
> >> "format-patch" to provide different context for the range-diff and the
> >> patch. This wasn't possible before.
> >
> > I really, really dislike the `--range-diff-`. We have
> > precedent for passing optional arguments that are passed to some other
> > command, so a much more logical and consistent convention would be to use
> > `--range-diff[=..]`, allowing all of the diff options that
> > you might want to pass to the outer diff in one go rather than having a
> > lengthy string of `--range-diff-this` and `--range-diff-that` options.
>
> Where do we pass those sorts of arguments?
>
> Reasons I did it this way:
>
> a) Passing it as one option will require the user to double-quote those
> options that take quoted arguments (e.g. --word-diff-regex), which I
> thought sucked more than the prefix. On the implementation side we
> couldn't leave the parsing of the command-line to the shell anymore.
>
> b) I think people will want to tweak this very rarely, much more rarely
> than e.g. -U10 in format-patch itself, so having something long-ish
> doesn't sound bad.
Hmm. I still don't like it. It sets a precedent, and we simply do not do
it that way in other circumstances (most obvious would be the -X merge
options). The more divergent user interfaces for the same sort of thing
are, the more brain cycles you force users to spend on navigating said
interfaces.
> > I only had time to skim the patch, and I have to wonder why you pass
> > around full-blown `rev_info` structs for range diff (and with that really
> > awful name `rd_rev`) rather than just the `diff_options` that you
> > *actually* care about?
>
> Because setup_revisions() which does all the command-line parsing needs
> a rev_info, so even if we only need the diffopt in the end we need to
> initiate the whole thing.
>
> Suggestions for a better varibale name most welcome.
`range_diff_revs`
And you do not need to pass around the whole thing. You can easily pass
`_diff_revs.diffopt`.
Don't pass around what you do not need to pass around.
Ciao,
Dscho
>
> > Ciao,
> > Dscho
> >
> >>
> >> Ever since the "--range-diff" option was added in
> >> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
> >> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
> >> machinery has been the one we get from "format-patch"'s own
> >> setup_revisions().
> >>
> >> This sort of thing is unique among the log-like commands in
> >> builtin/log.c, no command than format-patch will embed the output of
> >> another log-like command. Since the "rev->diffopt" is reused we need
> >> to munge it before we pass it to show_range_diff(). See
> >> 43dafc4172 ("format-patch: don't include --stat with --range-diff
> >> output", 2018-11-22) for a related regression fix which is being
> >> mostly reverted here.
> >>
> >> Implementation notes: 1) We're not bothering with the full teardown
> >> around die() and will leak memory, but it's too much boilerplate to do
> >> all the frees with/without the die() and not worth it. 2) We call
> >> repo_init_revisions() for "rd_rev" even though we could get away with
> >> a shallow copy like the code we're replacing (and which
> >> show_range_diff() itself does). This is to make this code more easily
> >> understood.
> >>
> >> Signed-off-by: Ævar Arnfjörð Bjarmason
> >> ---
> >> Documentation/git-format-patch.txt | 10 ++-
> >> builtin/log.c | 42 +++---
> >> t/t3206-range-diff.sh | 41 +
> >> 3 files changed, 82 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/Documentation/git-format-patch.txt
> >> b/Documentation/git-format-patch.txt
> >> index aba4c5febe..6c048f415f 100644
> >> --- a/Documentation/git-format-patch.txt
> >> +++ b/Documentation/git-format-patch.txt
> >> @@ -24,7 +24,8 @@ SYNOPSIS
> >> [--to=] [--cc=]
> >> [--[no-]cover-lett
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Paul, On Thu, 29 Nov 2018, Johannes Schindelin wrote: > I already added a test... See the reschedule-failed-exec branch on > https://github.com/dscho/git. And now I put up a proper Pull Request, to be submitted via GitGitGadget right after Git v2.20.0 will be released (technically, we are in a feature freeze period right now, I just could no resist, as I found your reasoning for rescheduling failed `exec` commands compelling, and there were no `rebase` bugs for me to fix). https://github.com/gitgitgadget/git/pull/90 Ciao, Johannes
Re: in 2.19.2 t0061-run-command FAILs if . is in $PATH
Hi Merijn and Junio, On Thu, 29 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > -test_expect_success 'run_command is restricted to PATH' ' > > +test_lazy_prereq DOT_IN_PATH ' > > + case ":$PATH:" in > > + *:.:*) true;; > > + *) false;; > > + esac > > +' > > An empty element in the colon-separated list also serves as an > instruction to pick up executable from $cwd, so > > case ":$PATH:" in > *:.:** | *::*) true ;; > *) false ;; > esac > > perhaps. Good point. Merijn, please be sure to squash this fix in before you submit the final thing. Thanks, Johannes > > > +test_expect_success !DOT_IN_PATH 'run_command is restricted to PATH' ' > > write_script should-not-run <<-\EOF && > > echo yikes > > EOF > > -- snap -- > > > > If so, can you please provide a commit message for it (you can add my > > Signed-off-by: line and your Tested-by: line). > > > > Thanks, > > Johannes >
Re: [PATCH 2/2] format-patch: allow for independent diff & range-diff options
Hi Ævar,
On Wed, 28 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> Change the semantics of the "--range-diff" option so that the regular
> diff options can be provided separately for the range-diff and the
> patch. This allows for supplying e.g. --range-diff-U0 and -U1 to
> "format-patch" to provide different context for the range-diff and the
> patch. This wasn't possible before.
I really, really dislike the `--range-diff-`. We have
precedent for passing optional arguments that are passed to some other
command, so a much more logical and consistent convention would be to use
`--range-diff[=..]`, allowing all of the diff options that
you might want to pass to the outer diff in one go rather than having a
lengthy string of `--range-diff-this` and `--range-diff-that` options.
I only had time to skim the patch, and I have to wonder why you pass
around full-blown `rev_info` structs for range diff (and with that really
awful name `rd_rev`) rather than just the `diff_options` that you
*actually* care about?
Ciao,
Dscho
>
> Ever since the "--range-diff" option was added in
> 31e2617a5f ("format-patch: add --range-diff option to embed diff in
> cover letter", 2018-07-22) the "rev->diffopt" we pass down to the diff
> machinery has been the one we get from "format-patch"'s own
> setup_revisions().
>
> This sort of thing is unique among the log-like commands in
> builtin/log.c, no command than format-patch will embed the output of
> another log-like command. Since the "rev->diffopt" is reused we need
> to munge it before we pass it to show_range_diff(). See
> 43dafc4172 ("format-patch: don't include --stat with --range-diff
> output", 2018-11-22) for a related regression fix which is being
> mostly reverted here.
>
> Implementation notes: 1) We're not bothering with the full teardown
> around die() and will leak memory, but it's too much boilerplate to do
> all the frees with/without the die() and not worth it. 2) We call
> repo_init_revisions() for "rd_rev" even though we could get away with
> a shallow copy like the code we're replacing (and which
> show_range_diff() itself does). This is to make this code more easily
> understood.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> Documentation/git-format-patch.txt | 10 ++-
> builtin/log.c | 42 +++---
> t/t3206-range-diff.sh | 41 +
> 3 files changed, 82 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/git-format-patch.txt
> b/Documentation/git-format-patch.txt
> index aba4c5febe..6c048f415f 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -24,7 +24,8 @@ SYNOPSIS
> [--to=] [--cc=]
> [--[no-]cover-letter] [--quiet] [--notes[=]]
> [--interdiff=]
> -[--range-diff= [--creation-factor=]]
> +[--range-diff= [--creation-factor=]
> + [--range-diff]]
> [--progress]
> []
> [ | ]
> @@ -257,6 +258,13 @@ feeding the result to `git send-email`.
> creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
> for details.
>
> +--range-diff::
> + Other options prefixed with `--range-diff` are stripped of
> + that prefix and passed as-is to the diff machinery used to
> + generate the range-diff, e.g. `--range-diff-U0` and
> + `--range-diff--no-color`. This allows for adjusting the format
> + of the range-diff independently from the patch itself.
> +
> --notes[=]::
> Append the notes (see linkgit:git-notes[1]) for the commit
> after the three-dash line.
> diff --git a/builtin/log.c b/builtin/log.c
> index 02d88fa233..7658e56ecc 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1023,7 +1023,8 @@ static void show_diffstat(struct rev_info *rev,
> fprintf(rev->diffopt.file, "\n");
> }
>
> -static void make_cover_letter(struct rev_info *rev, int use_stdout,
> +static void make_cover_letter(struct rev_info *rev, struct rev_info *rd_rev,
> + int use_stdout,
> struct commit *origin,
> int nr, struct commit **list,
> const char *branch_name,
> @@ -1095,13 +1096,9 @@ static void make_cover_letter(struct rev_info *rev,
> int use_stdout,
> }
>
> if (rev->rdiff1) {
> - struct diff_options opts;
> - memcpy(, >diffopt, sizeof(opts));
> - opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT |
> DIFF_FORMAT_SUMMARY);
> -
> fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
> show_range_diff(rev->rdiff1, rev->rdiff2,
> - rev->creation_factor, 1, );
> + rev->creation_factor, 1, _rev->diffopt);
> }
> }
>
> @@ -1485,6 +1482,7 @@ int cmd_format_patch(int argc, const char
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Paul,
On Wed, 28 Nov 2018, Paul Morelle wrote:
> On 28/11/18 16:19, Johannes Schindelin wrote:
> >
> > On Wed, 28 Nov 2018, Paul Morelle wrote:
> >
> >> The 'exec' command can be used to run tests on a set of commits,
> >> interrupting on failing commits to let the user fix the tests.
> >>
> >> However, the 'exec' line has been consumed, so it won't be ran again by
> >> 'git rebase --continue' is ran, even if the tests weren't fixed.
> >>
> >> This commit introduces a new command 'test' equivalent to 'exec', except
> >> that it is automatically rescheduled in the todo list if it fails.
> >>
> >> Signed-off-by: Paul Morelle
> > Would it not make more sense to add a command-line option (and a config
> > setting) to re-schedule failed `exec` commands? Like so:
>
> Your proposition would do in most cases, however it is not possible to
> make a distinction between reschedulable and non-reschedulable commands.
True. But I don't think that's so terrible.
What I think is something to avoid is two commands that do something very,
very similar, but with two very, very different names.
In reality, I think that it would even make sense to change the default to
reschedule failed `exec` commands. Which is why I suggested to also add a
config option.
> Do you think that we can ignore the distinction between reschedulable
> and non-reschedulable commands in the same script?
Yes, I think that there *should not* be any distinction. It would just
make it harder to use the feature (users would have to keep in mind that
one version reschedules, one version does not).
> In this case, I would add some tests to your patch below, and propose
> the result on this mailing list.
I already added a test... See the reschedule-failed-exec branch on
https://github.com/dscho/git.
And I had another idea. To make this feature more useful for *myself*, I
would like to introduce the `-X` option as a shortcut for
`--reschedule-failed-exec -x`, e.g.
git rebase -X "make -j15 test"
What do you think?
Johannes
>
> > -- snip --
> > diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
> > index a2ab68ed0632..a9ab009fdbca 100644
> > --- a/builtin/rebase--interactive.c
> > +++ b/builtin/rebase--interactive.c
> > @@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char
> > **argv, const char *prefix)
> > OPT_STRING(0, "onto-name", _name, N_("onto-name"),
> > N_("onto name")),
> > OPT_STRING(0, "cmd", , N_("cmd"), N_("the command to run")),
> > OPT_RERERE_AUTOUPDATE(_rerere_auto),
> > + OPT_BOOL(0, "reschedule-failed-exec",
> > _failed_exec,
> > +N_("automatically re-schedule any `exec` that fails")),
> > OPT_END()
> > };
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 5b3e5baec8a0..700cbc4e150e 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -104,6 +104,7 @@ struct rebase_options {
> > int rebase_merges, rebase_cousins;
> > char *strategy, *strategy_opts;
> > struct strbuf git_format_patch_opt;
> > + int reschedule_failed_exec;
> > };
> >
> > static int is_interactive(struct rebase_options *opts)
> > @@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options
> > *opts)
> > argv_array_push(, opts->gpg_sign_opt);
> > if (opts->signoff)
> > argv_array_push(, "--signoff");
> > + if (opts->reschedule_failed_exec)
> > + argv_array_push(,
> > "--reschedule-failed-exec");
> >
> > status = run_command();
> > goto finished_rebase;
> > @@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char
> > *prefix)
> >"strategy")),
> > OPT_BOOL(0, "root", ,
> > N_("rebase all reachable commits up to the root(s)")),
> > + OPT_BOOL(0, "reschedule-failed-exec",
> > +_failed_exec,
> > +N_("automatically re-schedule any `exec` that fails")),
> > OPT_END(),
> > };
> > int i;
> > @@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const
> > char *prefix)
> > break;
> > }
> >
> > + if (options.resched
Re: [PATCH] rebase -i: introduce the 'test' command
Hi Paul,
On Wed, 28 Nov 2018, Paul Morelle wrote:
> The 'exec' command can be used to run tests on a set of commits,
> interrupting on failing commits to let the user fix the tests.
>
> However, the 'exec' line has been consumed, so it won't be ran again by
> 'git rebase --continue' is ran, even if the tests weren't fixed.
>
> This commit introduces a new command 'test' equivalent to 'exec', except
> that it is automatically rescheduled in the todo list if it fails.
>
> Signed-off-by: Paul Morelle
Would it not make more sense to add a command-line option (and a config
setting) to re-schedule failed `exec` commands? Like so:
-- snip --
diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index a2ab68ed0632..a9ab009fdbca 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -192,6 +192,8 @@ int cmd_rebase__interactive(int argc, const char **argv,
const char *prefix)
OPT_STRING(0, "onto-name", _name, N_("onto-name"),
N_("onto name")),
OPT_STRING(0, "cmd", , N_("cmd"), N_("the command to run")),
OPT_RERERE_AUTOUPDATE(_rerere_auto),
+ OPT_BOOL(0, "reschedule-failed-exec",
_failed_exec,
+N_("automatically re-schedule any `exec` that fails")),
OPT_END()
};
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b3e5baec8a0..700cbc4e150e 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -104,6 +104,7 @@ struct rebase_options {
int rebase_merges, rebase_cousins;
char *strategy, *strategy_opts;
struct strbuf git_format_patch_opt;
+ int reschedule_failed_exec;
};
static int is_interactive(struct rebase_options *opts)
@@ -415,6 +416,8 @@ static int run_specific_rebase(struct rebase_options *opts)
argv_array_push(, opts->gpg_sign_opt);
if (opts->signoff)
argv_array_push(, "--signoff");
+ if (opts->reschedule_failed_exec)
+ argv_array_push(,
"--reschedule-failed-exec");
status = run_command();
goto finished_rebase;
@@ -903,6 +906,9 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
"strategy")),
OPT_BOOL(0, "root", ,
N_("rebase all reachable commits up to the root(s)")),
+ OPT_BOOL(0, "reschedule-failed-exec",
+_failed_exec,
+N_("automatically re-schedule any `exec` that fails")),
OPT_END(),
};
int i;
@@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
break;
}
+ if (options.reschedule_failed_exec && !is_interactive())
+ die(_("--reschedule-failed-exec requires an interactive
rebase"));
+
if (options.git_am_opts.argc) {
/* all am options except -q are compatible only with --am */
for (i = options.git_am_opts.argc - 1; i >= 0; i--)
diff --git a/sequencer.c b/sequencer.c
index e1a4dd15f1a8..69bee63e116f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -158,6 +158,7 @@ static GIT_PATH_FUNC(rebase_path_strategy,
"rebase-merge/strategy")
static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate,
"rebase-merge/allow_rerere_autoupdate")
static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
+static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec,
"rebase-merge/reschedule-failed-exec")
static int git_sequencer_config(const char *k, const char *v, void *cb)
{
@@ -2362,6 +2363,9 @@ static int read_populate_opts(struct replay_opts *opts)
opts->signoff = 1;
}
+ if (file_exists(rebase_path_reschedule_failed_exec()))
+ opts->reschedule_failed_exec = 1;
+
read_strategy_opts(opts, );
strbuf_release();
@@ -2443,6 +2447,8 @@ int write_basic_state(struct replay_opts *opts, const
char *head_name,
write_file(rebase_path_gpg_sign_opt(), "-S%s\n",
opts->gpg_sign);
if (opts->signoff)
write_file(rebase_path_signoff(), "--signoff\n");
+ if (opts->reschedule_failed_exec)
+ write_file(rebase_path_reschedule_failed_exec(), "%s", "");
return 0;
}
@@ -3586,9 +3592,10 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
*end_of_arg = saved;
/* Reread the todo file if it has changed. */
- if (res)
- ; /* fall through */
- else if (stat(get_todo_path(opts), ))
+ if (res) {
+ if (opts->reschedule_failed_exec)
+
Re: in 2.19.2 t0061-run-command FAILs if . is in $PATH
Hi, On Wed, 28 Nov 2018, H.Merijn Brand wrote: > the test is explicitely checking that it should not find runnable > scripts outside $PATH, *assuming* $PATH does not have . in it Does this fix it for you? -- snip -- diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index f3f308920f04..4949fdfde88b 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -33,7 +33,14 @@ test_expect_success 'run_command can run a command' ' test_must_be_empty err ' -test_expect_success 'run_command is restricted to PATH' ' +test_lazy_prereq DOT_IN_PATH ' + case ":$PATH:" in + *:.:*) true;; + *) false;; + esac +' + +test_expect_success !DOT_IN_PATH 'run_command is restricted to PATH' ' write_script should-not-run <<-\EOF && echo yikes EOF -- snap -- If so, can you please provide a commit message for it (you can add my Signed-off-by: line and your Tested-by: line). Thanks, Johannes > > Having '.' in $PATH can be seen as a bad idea (and it most likely is), > but the tests should either remove '.' from $PATH before testing or > ignore that fail if $PATH does have '.', as it is not illegal > > $ git-2.19.2/t 504 > prove -v t0061-run-command.sh > t0061-run-command.sh .. > ok 1 - start_command reports ENOENT (slash) > ok 2 - start_command reports ENOENT (no slash) > ok 3 - run_command can run a command > ok 4 - run_command is restricted to PATH > ok 5 - run_command can run a script without a #! line > ok 6 - run_command does not try to execute a directory > ok 7 - run_command passes over non-executable file > ok 8 - run_command reports EACCES > ok 9 - unreadable directory in PATH > ok 10 - run_command runs in parallel with more jobs available than tasks > ok 11 - run_command runs in parallel with as many jobs as tasks > ok 12 - run_command runs in parallel with more tasks than jobs available > ok 13 - run_command is asked to abort gracefully > ok 14 - run_command outputs > ok 15 - GIT_TRACE with environment variables > # passed all 15 test(s) > 1..15 > ok > All tests successful. > Files=1, Tests=15, 1 wallclock secs ( 0.04 usr 0.01 sys + 0.26 cusr 0.07 > csys = 0.38 CPU) > Result: PASS > > $ env PATH="$PATH"":." prove -v t0061-run-command.sh > t0061-run-command.sh .. > ok 1 - start_command reports ENOENT (slash) > ok 2 - start_command reports ENOENT (no slash) > ok 3 - run_command can run a command > not ok 4 - run_command is restricted to PATH > # > # write_script should-not-run <<-\EOF && > # echo yikes > # EOF > # test_must_fail test-tool run-command run-command > should-not-run > # > ok 5 - run_command can run a script without a #! line > ok 6 - run_command does not try to execute a directory > ok 7 - run_command passes over non-executable file > ok 8 - run_command reports EACCES > ok 9 - unreadable directory in PATH > ok 10 - run_command runs in parallel with more jobs available than tasks > ok 11 - run_command runs in parallel with as many jobs as tasks > ok 12 - run_command runs in parallel with more tasks than jobs available > ok 13 - run_command is asked to abort gracefully > ok 14 - run_command outputs > ok 15 - GIT_TRACE with environment variables > # failed 1 among 15 test(s) > 1..15 > Dubious, test returned 1 (wstat 256, 0x100) > Failed 1/15 subtests > > Test Summary Report > --- > t0061-run-command.sh (Wstat: 256 Tests: 15 Failed: 1) > Failed test: 4 > Non-zero exit status: 1 > Files=1, Tests=15, 1 wallclock secs ( 0.03 usr 0.00 sys + 0.24 cusr 0.07 > csys = 0.34 CPU) > Result: FAIL > > -- > H.Merijn Brand http://tux.nl Perl Monger http://amsterdam.pm.org/ > using perl5.00307 .. 5.29 porting perl5 on HP-UX, AIX, and openSUSE > http://mirrors.develooper.com/hpux/http://www.test-smoke.org/ > http://qa.perl.org http://www.goldmark.org/jeff/stupid-disclaimers/ >
Re: [PATCH v1] mem_pool: add GIT_TRACE_MEMPOOL support
Hi Ben,
On Tue, 27 Nov 2018, Ben Peart wrote:
> From: Ben Peart
>
> Add tracing around initializing and discarding mempools. In discard report
> on the amount of memory unused in the current block to help tune setting
> the initial_size.
>
> Signed-off-by: Ben Peart
> ---
Looks good.
My only question: should we also trace calls to _alloc(), _calloc() and
_combine()?
Ciao,
Johannes
>
> Notes:
> Base Ref: * git-trace-mempool
> Web-Diff: https://github.com/benpeart/git/commit/9ac84bbca2
> Checkout: git fetch https://github.com/benpeart/git git-trace-mempool-v1
> && git checkout 9ac84bbca2
>
> mem-pool.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/mem-pool.c b/mem-pool.c
> index a2841a4a9a..065389aaec 100644
> --- a/mem-pool.c
> +++ b/mem-pool.c
> @@ -5,6 +5,7 @@
> #include "cache.h"
> #include "mem-pool.h"
>
> +static struct trace_key trace_mem_pool = TRACE_KEY_INIT(MEMPOOL);
> #define BLOCK_GROWTH_SIZE 1024*1024 - sizeof(struct mp_block);
>
> /*
> @@ -48,12 +49,16 @@ void mem_pool_init(struct mem_pool **mem_pool, size_t
> initial_size)
> mem_pool_alloc_block(pool, initial_size, NULL);
>
> *mem_pool = pool;
> + trace_printf_key(_mem_pool, "mem_pool (%p): init (%"PRIuMAX")
> initial size\n",
> + pool, (uintmax_t)initial_size);
> }
>
> void mem_pool_discard(struct mem_pool *mem_pool, int invalidate_memory)
> {
> struct mp_block *block, *block_to_free;
>
> + trace_printf_key(_mem_pool, "mem_pool (%p): discard (%"PRIuMAX")
> unused\n",
> + mem_pool, (uintmax_t)(mem_pool->mp_block->end -
> mem_pool->mp_block->next_free));
> block = mem_pool->mp_block;
> while (block)
> {
>
> base-commit: bb75be6cb916297f271c846f2f9caa3daaaec718
> --
> 2.18.0.windows.1
>
>
Re: [PATCH v1/RFC 1/1] 'git clone C:\cygwin\home\USER\repo' is working (again)
Hi J.H. On Wed, 28 Nov 2018, Houder wrote: > On 2018-11-28 09:46, Johannes Schindelin wrote: > > > > On Wed, 28 Nov 2018, J.H. van de Water wrote: > > > > > > > me@work /cygdrive > > > > > $ ls > > > > > c d > > > > > > > > > > So `/cygdrive` *is* a valid directory in Cygwin. > > > > > > > > That supports the code that does not special case a path that begins > > > > with /cygdrive/ and simply treats it as a full path and freely use > > > > relative path, I guess. Very good point. > > > > > > Please read > > > > > > https://cygwin.com/cygwin-ug-net/using.html#cygdrive > > > ( The cygdrive path prefix ) > > > > > > you can access arbitary drives on your system by using the cygdrive > > > path > > > prefix. The default value for this prefix is /cygdrive ... > > > > > > > > > The cygdrive prefix is a >>> virtual directory <<< under which all drives > > > on > > > a system are subsumed ... > > > > > > > > > The cygdrive prefix may be CHANGED in the fstab file as outlined above > > > ! > > > > > > > > > To simplify scripting, Cygwin also provides a /proc/cygdrive symlink, ... > > > > > > = > > > > That's all very interesting, but I fail to see the relevance with regards > > to the issue at hand, namely whether to special-case `/cygdrive` as a > > special prefix that cannot be treated as directory in Git. > > > > I still maintain that it should not be special-cased, no matter whether it > > is a virtual directory or whether it can be renamed to `/jh-likes-cygwin` > > or whatever. > > Ok. Sorry about the noise. > > From your post I got the impression that you believed that there will always > be a directory called /cygdrive on Cygwin. I know it can be different. In MSYS2 it is set to `/` via this line in `/etc/fstab`: none / cygdrive binary,posix=0,noacl,user 0 0 Which is just to say that I am fully aware of the option to rename it. > My point: it can have a different name. Indeed. And whatever name you give it, Cygwin can handle it as if it were a regular directory. So we *must not* special-case it in Git. Ciao, Johannes
Re: [ANNOUNCE] Git v2.20.0-rc1
Hi Junio, On Wed, 28 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > ... > > In short, even a thorough study of the code (keeping in mind the few > > tidbits of information provided by you) leaves me really wondering which > > code you run, because it sure does not look like current `master` to me. > > > > And if it is not `master`, then I have to ask why you keep suggesting to > > turn off the built-in rebase by default in `master`. > > > > Ciao, > > Johannes > > > > P.S.: Maybe you have a hook you forgot to mention? > > Any response? Or can I retract jc/postpone-rebase-in-c that was > prepared as a reaction to this? I worked with Ævar via IRC and we figured out the root cause and I submitted https://public-inbox.org/git/pull.88.git.gitgitgad...@gmail.com/ to fix it. Given that this is a really obscure edge case (`git rebase --stat -v -i` onto an unrelated commit history, if you take away one of these, the bug does not trigger), and that it was only discovered to be a bug *because* of the built-in rebase (the scripted version had the same bug, but simply forgot to do proper error checking), I would not think that the reported bug is a strong argument in favor of turning off the built-in rebase by defauly. In other words, after understanding the bug I am even more confident than before that the built-in rebase is actually in a pretty good shape. I do not expect any major regressions, and if any happen: we do have that escape hatch for corner cases while I fix those bugs. Ciao, Dscho
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
Hi Jonathan, On Tue, 27 Nov 2018, Jonathan Nieder wrote: > At https://bugs.debian.org/914695 is a report of a test regression in > an outside project that is very likely to have been triggered by the > new faster rebase code. >From looking through that log.gz (without having a clue where the test code lives, so I cannot say what it is supposed to do, and also: this is the first time I hear about dgit...), it would appear that this must be a regression in the reflog messages produced by `git rebase`. > The issue has not been triaged, so I don't know yet whether it's a > problem in rebase-in-c or a manifestation of a bug in the test. It ends thusly: -- snip -- [...] + git reflog + egrep 'debrebase new-upstream.*checkout' + test 1 = 0 + t-report-failure + set +x TEST FAILED -- snap -- Which makes me think that the reflog we produce in *some* code path that originally called `git checkout` differs from the scripted rebase's generated reflog. > That said, Google has been running with the new rebase since ~1 month > ago when it became the default, with no issues reported by users. As a > result, I am confident that it can cope with what most users of "next" > throw at it, which means that if we are to find more issues to polish it > better, it will need all the exposure it can get. Right. And there are a few weeks before the holidays, which should give me time to fix whatever bugs are discovered (I only half mind being the only one who fixes these bugs). > In the Google deployment, we will keep using rebase-in-c even if it > gets disabled by default, in order to help with that. > > From the Debian point of view, it's only a matter of time before > rebase-in-c becomes the default: even if it's not the default in 2.20, > it would presumably be so in 2.21 or 2.22. That means the community's > attention when resolving security and reliability bugs would be on the > rebase-in-c implementation. As a result, the Debian package will most > likely enable rebase-in-c by default even if upstream disables it, in > order to increase the package's shelf life (i.e. to ease the > maintenance burden of supporting whichever version of the package ends > up in the next Debian stable). > > So with either hat on, it doesn't matter whether you apply this patch > upstream. > > Having two pretty different deployments end up with the same > conclusion leads me to suspect that it's best for upstream not to > apply the revert patch, unless either > > (a) we have a concrete regression to address and then try again, or > (b) we have a test or other plan to follow before trying again. In this instance, I am more a fan of the "let's move fast and break things, then move even faster fixing them" approach. Besides, the bug that Ævar discovered was a bug already in the scripted rebase, but hidden by yet another bug (the missing error checking). I get the pretty firm impression that the common code paths are now pretty robust, and only lesser-exercised features may expose a bug (or regression, as in the case of the reflogs, where one could argue that the exact reflog message is not something we promise not to fiddle with). Ciao, Dscho
Re: [PATCH] advice: don't pointlessly suggest --convert-graft-file
Hi Ævar,
On Tue, 27 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> The advice to run 'git replace --convert-graft-file' added in
> f9f99b3f7d ("Deprecate support for .git/info/grafts", 2018-04-29)
> didn't add an exception for the 'git replace --convert-graft-file'
> codepath itself.
>
> As a result we'd suggest running --convert-graft-file while the user
> was running --convert-graft-file, which makes no sense. Before:
>
> $ git replace --convert-graft-file
> hint: Support for /info/grafts is deprecated
> hint: and will be removed in a future Git version.
> hint:
> hint: Please use "git replace --convert-graft-file"
> hint: to convert the grafts into replace refs.
> hint:
> hint: Turn this message off by running
> hint: "git config advice.graftFileDeprecated false"
>
> Add a check for that case and skip printing the advice while the user
> is busy following our advice.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> builtin/replace.c | 1 +
> t/t6050-replace.sh | 5 -
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index a58b9c6d13..affcdfb416 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -495,6 +495,7 @@ static int convert_graft_file(int force)
> if (!fp)
> return -1;
>
> + advice_graft_file_deprecated = 0;
> while (strbuf_getline(, fp) != EOF) {
> if (*buf.buf == '#')
> continue;
As long as we keep this code in the one-shot code path, it is probably
okay. Otherwise we'd have to save the original value and restoring it
before returning from this function.
But then, we might never move `convert_graft_file()` into `libgit.a`.
Thanks,
Dscho
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index 86374a9c52..5d6d3184ac 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -461,7 +461,10 @@ test_expect_success '--convert-graft-file' '
> printf "%s\n%s %s\n\n# comment\n%s\n" \
> $(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \
> >.git/info/grafts &&
> - git replace --convert-graft-file &&
> + git status 2>stderr &&
> + test_i18ngrep "hint:.*grafts is deprecated" stderr &&
> + git replace --convert-graft-file 2>stderr &&
> + test_i18ngrep ! "hint:.*grafts is deprecated" stderr &&
> test_path_is_missing .git/info/grafts &&
>
> : verify that the history is now "grafted" &&
> --
> 2.20.0.rc1.379.g1dd7ef354c
>
>
Re: [PATCH v1/RFC 1/1] 'git clone C:\cygwin\home\USER\repo' is working (again)
Hi J.H., On Wed, 28 Nov 2018, J.H. van de Water wrote: > > > me@work /cygdrive > > > $ ls > > > c d > > > > > > So `/cygdrive` *is* a valid directory in Cygwin. > > > > That supports the code that does not special case a path that begins > > with /cygdrive/ and simply treats it as a full path and freely use > > relative path, I guess. Very good point. > > Please read > > https://cygwin.com/cygwin-ug-net/using.html#cygdrive > ( The cygdrive path prefix ) > > you can access arbitary drives on your system by using the cygdrive path > prefix. The default value for this prefix is /cygdrive ... > > > The cygdrive prefix is a >>> virtual directory <<< under which all drives on > a system are subsumed ... > > > The cygdrive prefix may be CHANGED in the fstab file as outlined above ! > > > To simplify scripting, Cygwin also provides a /proc/cygdrive symlink, ... > > = That's all very interesting, but I fail to see the relevance with regards to the issue at hand, namely whether to special-case `/cygdrive` as a special prefix that cannot be treated as directory in Git. I still maintain that it should not be special-cased, no matter whether it is a virtual directory or whether it can be renamed to `/jh-likes-cygwin` or whatever. Ciao, Dscho
Re: [PATCH v3 1/7] rebase: fix incompatible options error message
Hi Elijah,
On Wed, 21 Nov 2018, Elijah Newren wrote:
> In commit f57696802c30 ("rebase: really just passthru the `git am`
> options", 2018-11-14), the handling of `git am` options was simplified
> dramatically (and an option parsing bug was fixed), but it introduced
> a small regression in the error message shown when options only
> understood by separate backends were used:
>
> $ git rebase --keep --ignore-whitespace
> fatal: error: cannot combine interactive options (--interactive, --exec,
> --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with
> am options (.git/rebase-apply/applying)
>
> $ git rebase --merge --ignore-whitespace
> fatal: error: cannot combine merge options (--merge, --strategy,
> --strategy-option) with am options (.git/rebase-apply/applying)
>
> Note that in both cases, the list of "am options" is
> ".git/rebase-apply/applying", which makes no sense. Since the lists of
> backend-specific options is documented pretty thoroughly in the rebase
> man page (in the "Incompatible Options" section, with multiple links
> throughout the document), and since I expect this list to change over
> time, just simplify the error message.
>
> Signed-off-by: Elijah Newren
> ---
This patch is obviously good.
Given that you embedded it in the patch series that makes the sequencer
the work horse also for the `merge` backend of `git rebase` in addition to
the `interactive` one, may I assume that you intend this patch for post
v2.20.0?
Ciao,
Dscho
> builtin/rebase.c | 11 ---
> git-legacy-rebase.sh | 4 ++--
> 2 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5b3e5baec8..5ece134ae6 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1202,14 +1202,11 @@ int cmd_rebase(int argc, const char **argv, const
> char *prefix)
> break;
>
> if (is_interactive() && i >= 0)
> - die(_("error: cannot combine interactive options "
> - "(--interactive, --exec, --rebase-merges, "
> - "--preserve-merges, --keep-empty, --root + "
> - "--onto) with am options (%s)"), buf.buf);
> + die(_("error: cannot combine am options "
> + "with interactive options"));
> if (options.type == REBASE_MERGE && i >= 0)
> - die(_("error: cannot combine merge options (--merge, "
> - "--strategy, --strategy-option) with am options "
> - "(%s)"), buf.buf);
> + die(_("error: cannot combine am options "
> + "with merge options "));
> }
>
> if (options.signoff) {
> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> index b97ffdc9dd..0a747eb76c 100755
> --- a/git-legacy-rebase.sh
> +++ b/git-legacy-rebase.sh
> @@ -508,13 +508,13 @@ if test -n "$git_am_opt"; then
> then
> if test -n "$incompatible_opts"
> then
> - die "$(gettext "error: cannot combine interactive
> options (--interactive, --exec, --rebase-merges, --preserve-merges,
> --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
> + die "$(gettext "error: cannot combine am options with
> interactive options")"
> fi
> fi
> if test -n "$do_merge"; then
> if test -n "$incompatible_opts"
> then
> - die "$(gettext "error: cannot combine merge options
> (--merge, --strategy, --strategy-option) with am options
> ($incompatible_opts)")"
> + die "$(gettext "error: cannot combine am options with
> merge options")"
> fi
> fi
> fi
> --
> 2.20.0.rc1.7.g58371d377a
>
>
Re: [PATCH v11 20/22] stash: convert `stash--helper.c` into `stash.c`
Hi Junio & Paul,
On Mon, 26 Nov 2018, Junio C Hamano wrote:
> Paul-Sebastian Ungureanu writes:
>
> > The old shell script `git-stash.sh` was removed and replaced
> > entirely by `builtin/stash.c`. In order to do that, `create` and
> > `push` were adapted to work without `stash.sh`. For example, before
> > this commit, `git stash create` called `git stash--helper create
> > --message "$*"`. If it called `git stash--helper create "$@"`, then
> > some of these changes wouldn't have been necessary.
> >
> > This commit also removes the word `helper` since now stash is
> > called directly and not by a shell script.
> >
> > Signed-off-by: Paul-Sebastian Ungureanu
> > ---
> > .gitignore | 1 -
> > Makefile | 3 +-
> > builtin.h| 2 +-
> > builtin/{stash--helper.c => stash.c} | 157 +++
> > git-stash.sh | 153 --
> > git.c| 2 +-
> > 6 files changed, 92 insertions(+), 226 deletions(-)
> > rename builtin/{stash--helper.c => stash.c} (91%)
> > delete mode 100755 git-stash.sh
>
> Seeing the recent trouble in "rebase in C" and how keeping the
> scripted version as "git legacy-rebase" helped us postpone the
> rewritten version without ripping the whole thing out, I wonder if
> we can do the same here.
Feel very free to cherry-pick
https://github.com/git-for-windows/git/commit/004da7e7faa36c872868ae938e06594ea1c2f01c
and
https://github.com/git-for-windows/git/commit/cedfcd39f5a4e4beb33e16fa67c4659fd4bdabf6
which is what we carry in Git for Windows.
Ciao,
Dscho
> Also, the remaining two patches should be done _before_ this step, I
> would think. I can understand it if the reason you have those two
> after this step is because you found the opportunity for these
> improvements after you wrote this step, but in the larger picture
> seen by the end users of the "stash in C" and those developers who
> follow the evolution of the code, the logical place for this "now we
> have everything in C, we retire the scripted version" step to happen
> is at the very end.
>
> Thanks.
>
Re: [PATCH v11 03/22] strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`
Hi,
On Sun, 25 Nov 2018, Thomas Gummerer wrote:
> On 11/23, Paul-Sebastian Ungureanu wrote:
> > Implement `strbuf_insertf()` and `strbuf_vinsertf()` to
> > insert data using a printf format string.
> >
> > Original-idea-by: Johannes Schindelin
> > Signed-off-by: Paul-Sebastian Ungureanu
> > ---
> > strbuf.c | 36
> > strbuf.h | 9 +
> > 2 files changed, 45 insertions(+)
> >
> > diff --git a/strbuf.c b/strbuf.c
> > index 82e90f1dfe..bfbbdadbf3 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -249,6 +249,42 @@ void strbuf_insert(struct strbuf *sb, size_t pos,
> > const void *data, size_t len)
> > strbuf_splice(sb, pos, 0, data, len);
> > }
> >
> > +void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt,
> > va_list ap)
> > +{
> > + int len, len2;
> > + char save;
> > + va_list cp;
> > +
> > + if (pos > sb->len)
> > + die("`pos' is too far after the end of the buffer");
>
> I was going to ask about translation of this and other messages in
> 'die()' calls, but I see other messages in strbuf.c are not marked for
> translation either. It may make sense to mark them all for
> translation at some point in the future, but having them all
> untranslated for now makes sense.
>
> In the long run it may even be better to return an error here rather
> than 'die()'ing, but again this is consistent with the rest of the
> API, so this wouldn't be a good time to take that on.
I guess I was too overzealous in my copying. These conditions really
indicate bugs in the caller... So I'd actually rather change that die() to
BUG().
But then, the original code in strbuf_vaddf() calls die() and would have
to be changed, too.
> > + va_copy(cp, ap);
> > + len = vsnprintf(sb->buf + sb->len, 0, fmt, cp);
>
> Here we're just getting the length of what we're trying to format
> (excluding the final NUL). As the second argument is 0, we do not
> modify the strbuf at this point...
>
> > + va_end(cp);
> > + if (len < 0)
> > + BUG("your vsnprintf is broken (returned %d)", len);
> > + if (!len)
> > + return; /* nothing to do */
> > + if (unsigned_add_overflows(sb->len, len))
> > + die("you want to use way too much memory");
> > + strbuf_grow(sb, len);
>
> ... and then we grow the strbuf by the length we previously, which
> excludes the NUL character, plus one extra character, so even if pos
> == len we are sure to have enough space in the strbuf ...
>
> > + memmove(sb->buf + pos + len, sb->buf + pos, sb->len - pos);
> > + /* vsnprintf() will append a NUL, overwriting one of our characters */
> > + save = sb->buf[pos + len];
> > + len2 = vsnprintf(sb->buf + pos, sb->alloc - sb->len, fmt, ap);
>
> ... and we use vsnprintf to write the formatted string to the
> beginning of the buffer.
It is not actually the beginning of the buffer, but possibly the middle of
the buffer ;-)
> sb->alloc - sb->len can be larger than 'len', which is fine as vsnprintf
> doesn't write anything after the NUL character. And as 'strbuf_grow'
> adds len + 1 bytes to the strbuf we'll always have enough space for
> adding the formatted string ...
>
> > + sb->buf[pos + len] = save;
> > + if (len2 != len)
> > + BUG("your vsnprintf is broken (returns inconsistent lengths)");
> > + strbuf_setlen(sb, sb->len + len);
>
> And finally we set the strbuf to the new length. So all this is just
> a very roundabout way to say that this function does the right thing
> according to my reading (and tests).
:-)
It seems that Junio likes this way of reviewing, giving him confidence
that the review was thorough.
Thanks!
Dscho
> > +}
> > +
> > +void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...)
> > +{
> > + va_list ap;
> > + va_start(ap, fmt);
> > + strbuf_vinsertf(sb, pos, fmt, ap);
> > + va_end(ap);
> > +}
> > +
> > void strbuf_remove(struct strbuf *sb, size_t pos, size_t len)
> > {
> > strbuf_splice(sb, pos, len, "", 0);
> > diff --git a/strbuf.h b/strbuf.h
> > index be02150df3..8f8fe01e68 100644
> > --- a/strbuf.h
> > +++ b/strbuf.h
> > @@ -244,6 +244,15 @@ void strbuf_addchars(struct strbuf *sb, int c, size_t
> > n);
> > */
> > void strbuf_insert(struct strbuf *sb, size_t pos, const void *, size_t);
> >
> > +/**
> > + * Insert data to the given position of the buffer giving a printf format
> > + * string. The contents will be shifted, not overwritten.
> > + */
> > +void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt,
> > +va_list ap);
> > +
> > +void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...);
> > +
> > /**
> > * Remove given amount of data from a given position of the buffer.
> > */
> > --
> > 2.19.1.878.g0482332a22
> >
>
Git for Windows v2.19.2 demoted to "pre-release"
Team, sorry for the inconvenience, but I had to "pull" Git for Windows v2.19.2 because of two major bugs: 32-bit Git Bash simply failed to start, and all operations using git:// URLs were broken. This close to v2.20.0, I was uncomfortable pushing yet another upgrade to users that would have been made obsolete a mere week from now. I apologize for the trouble, Johannes
Re: [PATCH v1/RFC 1/1] 'git clone C:\cygwin\home\USER\repo' is working (again)
Hi Junio, On Tue, 27 Nov 2018, Junio C Hamano wrote: > Steven Penny writes: > > > If you strip the drive, you can still navigate within the same drive: > > > > $ cd 'C:\Users' > > $ pwd > > /cygdrive/c/Users > > > > $ cd '\Windows' > > $ pwd > > /cygdrive/c/Windows > > > > but you can no longer traverse drives: > > > > $ cd '\Testing' > > sh: cd: \Testing: No such file or directory > > Sorry, but I fail to see the point the last example wants to make. I agree. For me, the real test is this: me@work ~ $ cd /cygdrive me@work /cygdrive $ ls c d So `/cygdrive` *is* a valid directory in Cygwin. > > I would say these could be merged into a "win.h" or similar. Cygwin > > typically > > leans toward the "/unix/style" while MINGW has been more tolerant of > > "C:\Windows\Style" and "C:/Mixed/Style" paths, i dont see that changing. > > I'd defer to Windows folks to decide if a unified win.h is a good > idea. We already have such a thing, but it is not just `win.h`, it is `compat/win32/`. I would think that the best idea would be to move the MINGW variants to `compat/win32/path-utils.c` and declare them in `compat/win32/path-utils.h`, renaming them from `mingw_*()` to `win32_*()`. Ciao, Dscho
Re: [PATCH v1/RFC 1/1] 'git clone C:\cygwin\home\USER\repo' is working (again)
Hi Torsten,
On Mon, 26 Nov 2018, tbo...@web.de wrote:
> diff --git a/compat/cygwin.c b/compat/cygwin.c
> index b9862d606d..c4a10cb5a1 100644
> --- a/compat/cygwin.c
> +++ b/compat/cygwin.c
> @@ -1,19 +1,29 @@
> #include "../git-compat-util.h"
> #include "../cache.h"
>
> +int cygwin_skip_dos_drive_prefix(char **path)
> +{
> + int ret = has_dos_drive_prefix(*path);
> + *path += ret;
> + return ret;
> +}
> +
> int cygwin_offset_1st_component(const char *path)
> {
> - const char *pos = path;
> + char *pos = (char *)path;
> +
> /* unc paths */
> - if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
> + if (!skip_dos_drive_prefix() &&
> + is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
It takes a little folding and knotting of the brain to understand that
this `!skip_dos_drive_prefix()` has *nothing* to do with the comment
`unc paths` nor with the test whether the paths starts with two directory
separators.
As a consequence, I would highly suggest to turn this into:
if (skip_dos_drive_prefix())
; /* absolute path with DOS drive prefix */
/* unc paths */
else if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
That makes the code a lot easier to understand, and as a consequence a lot
harder to mess up in the future.
Thanks,
Dscho
> /* skip server name */
> - pos = strchr(pos + 2, '/');
> + pos = strpbrk(pos + 2, "\\/");
> if (!pos)
> return 0; /* Error: malformed unc path */
>
> do {
> pos++;
> - } while (*pos && pos[0] != '/');
> + } while (*pos && !is_dir_sep(*pos));
> }
> +
> return pos + is_dir_sep(*pos) - path;
> }
> diff --git a/compat/cygwin.h b/compat/cygwin.h
> index 8e52de4644..46f29c0a90 100644
> --- a/compat/cygwin.h
> +++ b/compat/cygwin.h
> @@ -1,2 +1,34 @@
> +#define has_dos_drive_prefix(path) \
> + (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
> +
> +
> +int cygwin_offset_1st_component(const char *path);
> +#define offset_1st_component cygwin_offset_1st_component
> +
> +
> +#define has_dos_drive_prefix(path) \
> + (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
> +int cygwin_skip_dos_drive_prefix(char **path);
> +#define skip_dos_drive_prefix cygwin_skip_dos_drive_prefix
> +static inline int cygwin_is_dir_sep(int c)
> +{
> + return c == '/' || c == '\\';
> +}
> +#define is_dir_sep cygwin_is_dir_sep
> +static inline char *cygwin_find_last_dir_sep(const char *path)
> +{
> + char *ret = NULL;
> + for (; *path; ++path)
> + if (is_dir_sep(*path))
> + ret = (char *)path;
> + return ret;
> +}
> +static inline void convert_slashes(char *path)
> +{
> + for (; *path; path++)
> + if (*path == '\\')
> + *path = '/';
> +}
> +#define find_last_dir_sep cygwin_find_last_dir_sep
> int cygwin_offset_1st_component(const char *path);
> #define offset_1st_component cygwin_offset_1st_component
> --
> 2.19.0.271.gfe8321ec05
>
>
[PATCH 1/1] rebase --stat: fix when rebasing to an unrelated history
From: Johannes Schindelin
When rebasing to a commit history that has no common commits with the
current branch, there is no merge base. The scripted version of the `git
rebase` command was not prepared for that and spewed out
fatal: ambiguous argument '': unknown revision or path not in
the working tree.
but then continued (due to lack of error checking).
The built-in version of the `git rebase` command blindly translated that
shell script code, assuming that there is no need to test whether there
*was* a merge base, and due to its better error checking, exited with a
fatal error (because it tried to read the object with hash ...
as a tree).
Fix both scripted and built-in versions to output something sensibly,
and add a regression test to keep this working in all eternity.
Reported-by: Ævar Arnfjörð Bjarmason
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 8 +---
git-legacy-rebase.sh | 6 --
t/t3406-rebase-message.sh | 10 ++
3 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b3e5baec8..9e4b0b564f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1483,7 +1483,8 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
if (options.flags & REBASE_VERBOSE)
printf(_("Changes from %s to %s:\n"),
- oid_to_hex(_base),
+ is_null_oid(_base) ?
+ "(empty)" : oid_to_hex(_base),
oid_to_hex(>object.oid));
/* We want color (if set), but no pager */
@@ -1494,8 +1495,9 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
opts.detect_rename = DIFF_DETECT_RENAME;
diff_setup_done();
- diff_tree_oid(_base, >object.oid,
- "", );
+ diff_tree_oid(is_null_oid(_base) ?
+ the_hash_algo->empty_tree : _base,
+ >object.oid, "", );
diffcore_std();
diff_flush();
}
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index b97ffdc9dd..be3b241676 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -718,10 +718,12 @@ if test -n "$diffstat"
then
if test -n "$verbose"
then
- echo "$(eval_gettext "Changes from \$mb to \$onto:")"
+ mb_display="${mb:-(empty)}"
+ echo "$(eval_gettext "Changes from \$mb_display to \$onto:")"
fi
+ mb_tree="${mb:-$(git hash-object -t tree /dev/null)}"
# We want color (if set), but no pager
- GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
+ GIT_PAGER='' git diff --stat --summary "$mb_tree" "$onto"
fi
test -n "$interactive_rebase" && run_specific_rebase
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 38bd876cab..a1ee912118 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -91,4 +91,14 @@ test_expect_success 'error out early upon -C or
--whitespace=' '
test_i18ngrep "Invalid whitespace option" err
'
+test_expect_success 'rebase -i onto unrelated history' '
+ git init unrelated &&
+ test_commit -C unrelated 1 &&
+ git -C unrelated remote add -f origin "$PWD" &&
+ git -C unrelated branch --set-upstream-to=origin/master &&
+ git -C unrelated -c core.editor=true rebase -i -v --stat >actual &&
+ test_i18ngrep "Changes from (empty)" actual &&
+ test_i18ngrep "5 files changed" actual
+'
+
test_done
--
gitgitgadget
[PATCH 0/1] Fix git rebase --stat -i
We're really entering obscure territory here, I would say. To trigger the bug, two things have to come together: the user must have asked for a diffstat afterwards, and the commits need to have been rebased onto a completely unrelated commit history (i.e. there must exist no merge base between the pre-rebase HEAD and the post-rebase HEAD). Please note that this bug existed already in the scripted rebase, but it was never detected because the scripted version would not even perform any error checking. It will make Junio very happy that I squashed the regression test in to the patch that fixes it. The reason, however, was not to make Junio happy (I hope to make him happy by fixing bugs), but simply that an earlier iteration of test would only fail with the built-in rebase, but not with the scripted version. The current version would fail with the scripted version, but I'll save the time to split the patch again now. Johannes Schindelin (1): rebase --stat: fix when rebasing to an unrelated history builtin/rebase.c | 8 +--- git-legacy-rebase.sh | 6 -- t/t3406-rebase-message.sh | 10 ++ 3 files changed, 19 insertions(+), 5 deletions(-) base-commit: a1598010f775d82b5adf12c29d0f5bc9b41434c6 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-88%2Fdscho%2Frebase-stat-fix-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-88/dscho/rebase-stat-fix-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/88 -- gitgitgadget
Re: [ANNOUNCE] Git v2.20.0-rc1
Hi Ævar,
On Mon, 26 Nov 2018, Johannes Schindelin wrote:
> On Sat, 24 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
> > On Wed, Nov 21 2018, Junio C Hamano wrote:
> >
> > > * "git rebase" and "git rebase -i" have been reimplemented in C.
> >
> > Here's another regression in the C version (and rc1), note: the
> > sha1collisiondetection is just a stand in for "some repo":
> >
> > (
> > rm -rf /tmp/repo &&
> > git init /tmp/repo &&
> > cd /tmp/repo &&
> > for c in 1 2
> > do
> > touch $c &&
> > git add $c &&
> > git commit -m"add $c"
> > done &&
> > git remote add origin
> > https://github.com/cr-marcstevens/sha1collisiondetection.git &&
> > git fetch &&
> > git branch --set-upstream-to origin/master &&
> > git rebase -i
> > )
> >
> > The C version will die with "fatal: unable to read tree
> > ". Running this with
> > rebase.useBuiltin=false does the right thing and rebases as of the merge
> > base of the two (which here is the root of the history).
>
> Sorry, this bug does not reproduce here:
>
> $ git rebase -i
> Successfully rebased and updated refs/heads/master.
>
> > I wasn't trying to stress test rebase. I was just wanting to rebase a
> > history I was about to force-push after cleaning it up, hardly an
> > obscure use-case. So [repeat last transmission in
> > https://public-inbox.org/git/87y39w1wc2@evledraar.gmail.com/ ]
>
> Maybe you can give me the full details so that I can verify that this is
> indeed a bug in the builtin C and not just a regression caused by some
> random branches being merged together?
>
> In short: please provide me with the exact URL and branch of your git.git
> fork to test. Then please make sure to specify the precise revision of the
> sha1collisiondetection/master rev, just in case that it matters.
>
> Ideally, you would reduce the problem to a proper test case, say, for
> t3412 (it seems that you try to rebase onto an unrelated history, so it is
> *vaguely* related to "rebase-root").
So I was getting spooked enough by your half-complete bug report that I
did more digging (it is really quite a bit frustrating to have so little
hard evidence to go on, a wild goose chase is definitely not what I was
looking forward to after a day of fighting other fires, but you know,
built-in rebase is dear to me).
The error message you copied clearly comes from tree-walk.c, from
`fill_tree_descriptor()` (the other "unable to read tree" messages enclose
the hash in parentheses).
There are exactly 3 calls to said function in the built-in rebase/rebase
-i in the current `master`, a1598010f775 (Merge branch
'nd/per-worktree-ref-iteration', 2018-11-26):
$ git grep fill_tree_descriptor -- builtin/rebase*.c sequencer.[ch]
rebase-interactive.[ch]
builtin/rebase.c: if (!reset_hard && !fill_tree_descriptor([nr++],
_oid)) {
builtin/rebase.c: if (!fill_tree_descriptor([nr++], oid)) {
sequencer.c:if (!fill_tree_descriptor(, )) {
The last one of these is in `do_reset()`, i.e. handling a `reset` command
which you did not ask for, as you passed `-i` to `git rebase`, not `-ir`.
The first two *both* are in `reset_head()`. The first of them uses
`head_oid`, which is read directly via `get_oid("HEAD", _oid)`, so if
this is all zeroes for you, then it's not rebase's fault.
The second one uses the parameter `oid` passed into `reset_head()`. The
only calls to that function that do not pass `NULL` as `oid` (which would
trigger `oid` to be replaced by `_oid`, i.e again not all zeroes
unless your setup is broken) are:
- in the `--abort` code path
- in the `--autostash` code path
- in the fast-forwarding code path
- just after the "First, rewinding head" message in the *non*-interactive
rebase
None of these apply to your script snippet.
Under the assumption that you might have forgotten to talk about
rebase.autostash=true and some dirty file, I tried to augment the script
snippet accordingly, but the built-in rebase as of current `master` still
works for me, plus: reading the autostash code path, it is hard to imagine
that the `lookup_commit_reference()` would return a pointer to a commit
object whose oid is all zeroes.
In short, even a thorough study of the code (keeping in mind the few
tidbits of information provided by you) leaves me really wondering which
code you run, because it sure does not look like current `master` to me.
And if it is not `master`, then I have to ask why you keep suggesting to
turn off the built-in rebase by default in `master`.
Ciao,
Johannes
P.S.: Maybe you have a hook you forgot to mention?
Re: [ANNOUNCE] Git v2.20.0-rc1
Hi Ævar, On Sat, 24 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > On Wed, Nov 21 2018, Junio C Hamano wrote: > > > * "git rebase" and "git rebase -i" have been reimplemented in C. > > Here's another regression in the C version (and rc1), note: the > sha1collisiondetection is just a stand in for "some repo": > > ( > rm -rf /tmp/repo && > git init /tmp/repo && > cd /tmp/repo && > for c in 1 2 > do > touch $c && > git add $c && > git commit -m"add $c" > done && > git remote add origin > https://github.com/cr-marcstevens/sha1collisiondetection.git && > git fetch && > git branch --set-upstream-to origin/master && > git rebase -i > ) > > The C version will die with "fatal: unable to read tree > ". Running this with > rebase.useBuiltin=false does the right thing and rebases as of the merge > base of the two (which here is the root of the history). Sorry, this bug does not reproduce here: $ git rebase -i Successfully rebased and updated refs/heads/master. > I wasn't trying to stress test rebase. I was just wanting to rebase a > history I was about to force-push after cleaning it up, hardly an > obscure use-case. So [repeat last transmission in > https://public-inbox.org/git/87y39w1wc2@evledraar.gmail.com/ ] Maybe you can give me the full details so that I can verify that this is indeed a bug in the builtin C and not just a regression caused by some random branches being merged together? In short: please provide me with the exact URL and branch of your git.git fork to test. Then please make sure to specify the precise revision of the sha1collisiondetection/master rev, just in case that it matters. Ideally, you would reduce the problem to a proper test case, say, for t3412 (it seems that you try to rebase onto an unrelated history, so it is *vaguely* related to "rebase-root"). Ciao, Dscho
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
Hi Martin,
On Fri, 23 Nov 2018, Martin Ågren wrote:
> On Fri, 23 Nov 2018 at 11:13, Johannes Schindelin
> wrote:
> > On Mon, 30 Oct 2017, Pranit Bauva wrote:
> >
> > > On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågren
> > > wrote:
> > > > On 27 October 2017 at 17:06, Pranit Bauva
> > > > wrote:
> > > >> +static void free_terms(struct bisect_terms *terms)
> > > >> +{
> > > >> + if (!terms->term_good)
> > > >> + free((void *) terms->term_good);
> > > >> + if (!terms->term_bad)
> > > >> + free((void *) terms->term_bad);
> > > >> +}
>
> > > > You leave the pointers dangling, but you're ok for now since this is the
> > > > last thing that happens in `cmd_bisect__helper()`. Your later patches
> > > > add more users, but they're also ok, since they immediately assign new
> > > > values.
> > > >
> > > > In case you (and others) find it useful, the below is a patch I've been
> > > > sitting on for a while as part of a series to plug various memory-leaks.
> > > > `FREE_AND_NULL_CONST()` would be useful in precisely these situations.
> > >
> > > Honestly, I wouldn't be the best person to judge this.
> >
> > Git's source code implicitly assumes that any `const` pointer refers to
> > memory owned by another code path. It is therefore probably not a good
> > idea to encourage `free()`ing a `const` pointer.
>
> Yeah, I never submitted that patch as part of a real series. I remember
> having a funky feeling about it, and whatever use-case I had for this
> macro, I managed to solve the memory leak in some other way in a
> rewrite.
>
> Thanks for a sanity check.
I am glad you agree, and it's just fair that I contribute a sanity check
on this here list when I have benefitted so many times from the same.
Ciao,
Johannes
Re: [PATCH v1 1/1] t5601-99: Enable colliding file detection for MINGW
Hi Carlo,
On Thu, 22 Nov 2018, Carlo Marcelo Arenas Belón wrote:
> Subject: [PATCH] entry: remove windows fallback to inode checking
>
> this test is really FS specific, so is better to avoid any compiled
> assumptions about the platform and let the user drive the fallback
> through core.checkStat instead
>
> Signed-off-by: Carlo Marcelo Arenas Belón
> ---
> entry.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index 0a3c451f5f..5ae74856e6 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -404,10 +404,6 @@ static void mark_colliding_entries(const struct checkout
> *state,
> {
> int i, trust_ino = check_stat;
>
> -#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)
> - trust_ino = 0;
> -#endif
> -
No, we cannot drop this. You may not see it in git.git's source code, but
in Git for Windows' patches, we have an experimental feature (which had
seemed to stabilize, but Ben Peart is currently doing wonders with it,
improving the performance substantially) for accelerating the file
metadata enumeration in a noticeable manner. The only way we can do that
is by *not* insisting on a correct inode.
Besides, IIRC even our regular stat() now "fails" to fill the inode field.
So no, we cannot do that. We can probably drop the `||
defined(__CYGINW__)` part (Cygwin even generates a fake inode for FAT,
where no equivalent is available, by hashing the full normalized path).
But you cannot drop the `GIT_WINDOWS_NATIVE` part.
Ciao,
Johannes
> ce->ce_flags |= CE_MATCHED;
>
> for (i = 0; i < state->istate->cache_nr; i++) {
> --
> 2.20.0.rc1
>
>
Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
Hi Peff, On Thu, 22 Nov 2018, Jeff King wrote: > On Thu, Nov 22, 2018 at 01:48:53PM +0100, Johannes Schindelin wrote: > > > So YMMV with git-s. My rule of thumb is: if I want to use this > > myself only, I'll make it an alias. If I want to ship it (e.g. with Git > > for Windows), I'll make it a git-. > > I have a handful of personal git-* scripts: mostly ones that are big > enough to be unwieldy as an alias. But then, $PATH management is pretty > straightforward on my platforms, so it's easier to drop a script there > than to point an alias to a non-git-* script. Oh, my Git aliases pretty much *all* point to a single script that takes subcommands. Ciao, Dscho
Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C
Hi Pranit,
(Cc:ing Tanushree because they will try to pick up this patch series as
part of the Outreachy program.)
On Mon, 30 Oct 2017, Pranit Bauva wrote:
> On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågren wrote:
> > On 27 October 2017 at 17:06, Pranit Bauva wrote:
> >> +static void free_terms(struct bisect_terms *terms)
> >> +{
> >> + if (!terms->term_good)
> >> + free((void *) terms->term_good);
> >> + if (!terms->term_bad)
> >> + free((void *) terms->term_bad);
> >> +}
> >
> > These look like no-ops. Remove `!` for correctness, or `if (...)` for
> > simplicity, since `free()` can handle NULL.
>
> I probably forgot to do this here. I will make the change.
>
> > You leave the pointers dangling, but you're ok for now since this is the
> > last thing that happens in `cmd_bisect__helper()`. Your later patches
> > add more users, but they're also ok, since they immediately assign new
> > values.
> >
> > In case you (and others) find it useful, the below is a patch I've been
> > sitting on for a while as part of a series to plug various memory-leaks.
> > `FREE_AND_NULL_CONST()` would be useful in precisely these situations.
>
> Honestly, I wouldn't be the best person to judge this.
Git's source code implicitly assumes that any `const` pointer refers to
memory owned by another code path. It is therefore probably not a good
idea to encourage `free()`ing a `const` pointer.
Which brings me back to the question: who really owns that allocated
memory to which `term_good` and `term_bad` refer?
Ciao,
Johannes
Re: 2.19.2 wont launch
Hi Paul, On Thu, 22 Nov 2018, Paul Gureghian wrote: > I installed 2.19.2 on windows 7 , 32 bit and it wont launch. This has been reported on Gitter and fixed in https://github.com/git-for-windows/MINGW-packages/commit/deb0395d031401ffe55024fb066267e2ea8d032b For the time being, please either use v2.19.1, or copy the file `git-bash.exe` from a portable Git v2.19.1 into your v2.19.2 installation. Sorry for the trouble, Johannes
Re: Git for Windows v2.20.0-rc0
Hi Stefan,
On Thu, 22 Nov 2018, stefan.na...@atlas-elektronik.com wrote:
> Just a quick note:
>
> I installed v2.20.0-rc0 with these options:
>
> $ cat /etc/install-options.txt
> Editor Option: VIM
> Custom Editor Path:
> Path Option: Cmd
> SSH Option: OpenSSH
> CURL Option: OpenSSL
> CRLF Option: CRLFCommitAsIs
> Bash Terminal Option: MinTTY
> Performance Tweaks FSCache: Enabled
> Use Credential Manager: Enabled
> Enable Symlinks: Disabled
> Enable Builtin Rebase: Enabled
> Enable Builtin Stash: Enabled
>
>
>
> When starting the git bash two windows pop up instead of one.
> One that's "empty" and the other one containing the real git bash.
Thank you for the report. This has been also reported at
https://github.com/git-for-windows/git/issues/1942, and I fixed it in the
meantime.
Ciao,
Johannes
>
> Thanks,
> Stefan
>
> Am 20.11.2018 um 21:56 schrieb Johannes Schindelin:
> > Team,
> >
> > On Sun, 18 Nov 2018, Junio C Hamano wrote:
> >
> >> An early preview release Git v2.20.0-rc0 is now available for
> >> testing at the usual places. It is comprised of 887 non-merge
> >> commits since v2.19.0, contributed by 71 people, 23 of which are
> >> new faces.
> >
> > The "for Windows" flavor of Git v2.20.0-rc0 is available here:
> >
> > https://github.com/git-for-windows/git/releases/tag/v2.20.0-rc0.windows.1
> >
> > The current change log for v2.20.0 reads like this:
> >
> > Changes since Git for Windows v2.19.1 (Oct 5th 2018)
> >
> > Please note: Git CMD is deprecated as of this Git for Windows version. The
> > default is to have git.exe in the PATH anyway, so there is no noticeable
> > difference between CMD and Git CMD. It is impossible to turn off CMD's
> > behavior where it picks up any git.exe in the current directory, so let's
> > discourage the use of Git CMD. Users who dislike Git Bash should switch to
> > Powershell instead.
> >
> > New Features
> >
> > • Comes with OpenSSH v7.9p1.
> > • The description of the editor option to choose Vim has been clarified
> > to state that this unsets core.editor.
> > • Comes with cURL v7.62.0.
> > • The type of symlinks to create (directory or file) can now be
> > specified via the .gitattributes.
> > • The FSCache feature now uses a faster method to enumerate files,
> > making e.g. git status faster in large repositories.
> > • Comes with Git Credential Manager v1.18.3.
> > • Comes with Git LFS v2.6.0.
> > • Comes with MSYS2 runtime (Git for Windows flavor) based on Cygwin
> > 2.11.2.
> > • The FSCache feature was optimized to become faster.
> >
> > Bug Fixes
> >
> > • The 64-bit Portable Git no longer sets pack.packSizeLimit.
> >
> > Thanks,
> > Johannes
> >
>
>
> --
>
> /dev/random says: To save trouble later, Joe named his cat Roadkill Fred
> python -c "print
> '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
>
> GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
Hi Peff,
On Sat, 17 Nov 2018, Jeff King wrote:
> On Fri, Nov 16, 2018 at 08:22:11PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> > So maybe we should just document this interface better. It seems one
> > implicit dependency is that we expect a manpage for the tool to exist
> > for --help.
>
> Yeah, I think this really the only problematic assumption. The rest of
> "-c", "--git-dir", etc, are just manipulating the environment, and that
> gets passed through to sub-invocations of Git (so if I have a script
> which calls git-config, it will pick up the "-c" config).
>
> It would be nice if there was a way to ask "is there a manpage?", and
> fallback to running "git-cmd --help". But:
>
> 1. I don't think there is a portable way to query that via man. And
> anyway, depending platform and config, it may be opening a browser
> to show HTML documentation (or I think even texinfo?).
>
> 2. We can just ask whether "man git-sizer" (or whatever help display
> command) returned a non-zero exit code, and fall back to "git-sizer
> --help". But there's an infinite-loop possibility here: running
> "git-sizer --help" does what we want. But if "man git-log" failed,
> we'd run "git-log --help", which in turn runs "git help log", which
> runs "man git-log", and so on.
>
> You can break that loop with an environment variable for "I already
> tried to show the manpage", which would I guess convert "--help" to
> "-h".
>
> So it's maybe do-able, but not quite as trivial as one might hope.
A trivial alternative would be to recommend adding a man page for
3rd-party git-s.
In other words, as soon as `git-sizer` is accompanied by `git-sizer.1` in
one of the appropriate locations, it is set.
(Actually, it is not: on Windows, it would have to add git-sizer.html in
the appropriate location, but we can deal with this if needed.)
> > But I don't remember the details, and can't reproduce it now with:
> >
> > $ cat ~/bin/git-sigint
> > #!/usr/bin/env perl
> > $SIG{INT} = sub { warn localtime . " " . $$ };
> > sleep 1 while 1;
> > $ git sigint # or git-sigint
> > [... behaves the same either way ...]
> >
> > So maybe it was something else, or I'm misremembering...
>
> I think that generally works because hitting ^C is going to send SIGINT
> to the whole process group. A more interesting case is:
>
> git sigint &
> kill -INT $!
>
> There $! is a parent "git" process that is just waiting on git-sigint to
> die. But that works, too, because the parent relays common signals due
> to 10c6cddd92 (dashed externals: kill children on exit, 2012-01-08). So
> you might have been remembering issues prior to that commit (or uncommon
> signals; these come from sigchain_push_common).
FWIW I do have a couple of scripts I use that I install into
`$HOME/bin/git-`. Although, granted, I essentially switched to
aliases for most of them, where the aliases still call a script that is
checked out in some folder in my home directory. The reason: this works in
more circumstances, as I do not have to add `$HOME/bin` to the `PATH`,
say, in PowerShell.
So YMMV with git-s. My rule of thumb is: if I want to use this
myself only, I'll make it an alias. If I want to ship it (e.g. with Git
for Windows), I'll make it a git-.
Ciao,
Dscho
[PATCH 0/1] Fix Windows build of next
The topic ot/ref-filter-object-info broke the Windows build since it entered pu, and as a consequence we have no test coverage of the other topics in pu. Sadly, this topic now advanced to next. Junio, I would like to ask you to merge this fix down to next and to advance to master together with ot/ref-filter-object-info. Johannes Schindelin (1): ref-filter: replace unportable `%lld` format ref-filter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: d364d6b33e15dbc6e57d83f9f1457a8e8fe77046 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-87%2Fdscho%2Fno-percent-lld-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-87/dscho/no-percent-lld-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/87 -- gitgitgadget
[PATCH 1/1] ref-filter: replace unportable `%lld` format
From: Johannes Schindelin
The `%lld` format is supported on Linux and macOS, but not on Windows.
This issue has been reported ten days ago (Message-ID:
nycvar.qro.7.76.6.1811121300520...@tvgsbejvaqbjf.bet), but the
corresponding topic still advanced to `next` in the meantime, breaking
the Windows build.
Let's use `PRIdMAX` and a cast to `intmax_t` instead, which unbreaks the
build, and imitates how we do things e.g. in `json-writer.c` already.
Signed-off-by: Johannes Schindelin
---
ref-filter.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ref-filter.c b/ref-filter.c
index 3cfe01a039..69cdf2dbb5 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -897,7 +897,7 @@ static void grab_common_values(struct atom_value *val, int
deref, struct expand_
v->s = xstrdup(type_name(oi->type));
else if (!strcmp(name, "objectsize:disk")) {
v->value = oi->disk_size;
- v->s = xstrfmt("%lld", (long long)oi->disk_size);
+ v->s = xstrfmt("%"PRIdMAX, (intmax_t)oi->disk_size);
} else if (!strcmp(name, "objectsize")) {
v->value = oi->size;
v->s = xstrfmt("%lu", oi->size);
--
gitgitgadget
Re: [RFC PATCH 1/5] ref-filter: add objectsize:disk option
Hi Junio, On Wed, 21 Nov 2018, Junio C Hamano wrote: > Оля Тележная writes: > > >> I am OK if we avoid PRIdMAX and use PRIuMAX instead with a cast to > >> the corresponding size in this codepath, as long as we properly > >> handle negative oi.disk_size field, which may be telling some > >> "unusual" condition to us. > > > > Maybe we want to change the type (from off_t to unsigned) directly in > > struct object_info? That will help us not to make additional > > checkings. Or, at least, I suggest to add check to > > oid_object_info_extended() so that this function will give a guarantee > > that the size is non-negative. > > I am fine with the approach. The potential gain of allowing > oi.disk_size is it would allow the code to say "I'll give these > pieces of info about the object, but the on-disk size is unknown" > without failing the whole object_info_extended() request. And I > personally do not think such an ability is not all that important > or useful. I see that this topic advanced to `next`, which means that the Windows build of `next` is now broken. To fix this, I prepared a GitGitGadget PR (https://github.com/gitgitgadget/git/pull/87) and will submit it as soon as I am satisfied that the build works. Ciao, Dscho
Re: [PATCH 0/1] rebase: warn about the correct tree's OID
Hi Junio, On Wed, 21 Nov 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > A quick fix for a recent topic. Not overly critical, but I would deem this > > v2.20.0-rc1 material. > > > > Johannes Schindelin (1): > > rebase: warn about the correct tree's OID > > > > Yup, it is kind of embarrasing that nobody caught it, but at the > same time, this typo is at so tiny level that I would not be > surprised if it survived for many years. TBH I am quite mortified that it slipped through *my* multiple pre-contribution reviews. > Will apply. Thanks. Ciao, Dscho
js/vsts-ci, was Re: What's cooking in git.git (Nov 2018, #06; Wed, 21)
Hi Junio, On Wed, 21 Nov 2018, Junio C Hamano wrote: > * js/vsts-ci (2018-10-16) 13 commits > . travis: fix skipping tagged releases > . README: add a build badge (status of the Azure Pipelines build) > . tests: record more stderr with --write-junit-xml in case of failure > . tests: include detailed trace logs with --write-junit-xml upon failure > . git-p4: use `test_atexit` to kill the daemon > . git-daemon: use `test_atexit` in the tests > . tests: introduce `test_atexit` > . ci: add a build definition for Azure DevOps > . ci/lib.sh: add support for Azure Pipelines > . tests: optionally write results as JUnit-style .xml > . test-date: add a subcommand to measure times in shell scripts > . ci/lib.sh: encapsulate Travis-specific things > . ci: rename the library of common functions > > Prepare to run test suite on Azure DevOps. > > Ejected out of 'pu', as doing so seems to help other topics get > tested at TravisCI. > > https://travis-ci.org/git/git/builds/452713184 is a sample of a > build whose tests on 4 hang (with this series in). Ejecting it > gave us https://travis-ci.org/git/git/builds/452778963 which still > shows breakages from other topics not yet in 'next', but at least > the tests do not stall. Sorry about that. FWIW my current plan is to work a bit more on the Windows phase (to make it faster), and to split out the `test_atexit` patches (because they cause those hangs). I still think it is the right thing to do, but I lack the time to take care of it within the next weeks. Instead, I will try to run even the Windows phase in --verbose-log mode so that the --junit-xml code can pick up the verbose logs right away (read: no more re-running upon test failures). Hopefully this won't cause a speed regression. Ciao, Dscho
Re: [PATCH 1/1] legacy-rebase: backport -C and --whitespace= checks
Hi Junio,
On Wed, 21 Nov 2018, Junio C Hamano wrote:
> Carlo Arenas writes:
>
> > Tested-by: Carlo Marcelo Arenas Belón
> >
> > the C version prepends: "fatal: " unlike the shell version for both
> > error messages
>
> In addition, "Invalid whitespace option" says 'bad', not
> '--whitespace=bad', in the builtin version.
>
> I think the following would address both issues.
Yes! Thank you. Can you squash it in?
Thanks,
Dscho
>
>
> git-legacy-rebase.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> index ced0635326..b97ffdc9dd 100755
> --- a/git-legacy-rebase.sh
> +++ b/git-legacy-rebase.sh
> @@ -340,7 +340,7 @@ do
> warn|nowarn|error|error-all)
> ;; # okay, known whitespace option
> *)
> - die "Invalid whitespace option: '${1%*=}'"
> + die "fatal: Invalid whitespace option: '${1#*=}'"
> ;;
> esac
> ;;
> @@ -358,7 +358,7 @@ do
> force_rebase=t
> ;;
> -C*[!0-9]*)
> - die "switch \`C' expects a numerical value"
> + die "fatal: switch \`C' expects a numerical value"
> ;;
> -C*)
> git_am_opt="$git_am_opt $1"
>
>
>
Git for Windows v2.20.0-rc0, was Re: [ANNOUNCE] Git v2.20.0-rc0
Team, On Sun, 18 Nov 2018, Junio C Hamano wrote: > An early preview release Git v2.20.0-rc0 is now available for > testing at the usual places. It is comprised of 887 non-merge > commits since v2.19.0, contributed by 71 people, 23 of which are > new faces. The "for Windows" flavor of Git v2.20.0-rc0 is available here: https://github.com/git-for-windows/git/releases/tag/v2.20.0-rc0.windows.1 The current change log for v2.20.0 reads like this: Changes since Git for Windows v2.19.1 (Oct 5th 2018) Please note: Git CMD is deprecated as of this Git for Windows version. The default is to have git.exe in the PATH anyway, so there is no noticeable difference between CMD and Git CMD. It is impossible to turn off CMD's behavior where it picks up any git.exe in the current directory, so let's discourage the use of Git CMD. Users who dislike Git Bash should switch to Powershell instead. New Features • Comes with OpenSSH v7.9p1. • The description of the editor option to choose Vim has been clarified to state that this unsets core.editor. • Comes with cURL v7.62.0. • The type of symlinks to create (directory or file) can now be specified via the .gitattributes. • The FSCache feature now uses a faster method to enumerate files, making e.g. git status faster in large repositories. • Comes with Git Credential Manager v1.18.3. • Comes with Git LFS v2.6.0. • Comes with MSYS2 runtime (Git for Windows flavor) based on Cygwin 2.11.2. • The FSCache feature was optimized to become faster. Bug Fixes • The 64-bit Portable Git no longer sets pack.packSizeLimit. Thanks, Johannes
Re: [PATCH] msvc: Directly use MS version (_stricmp) of strcasecmp
Hi Junio,
On Tue, 20 Nov 2018, Junio C Hamano wrote:
> Sven Strickroth writes:
>
> > This also removes an implicit conversion from size_t (unsigned) to int
> > (signed).
> >
> > _stricmp as well as _strnicmp are both available since VS2012.
Looks good to me.
> > Signed-off-by: Sven Strickroth
> > ---
> > compat/msvc.h | 8 +---
> > 1 file changed, 1 insertion(+), 7 deletions(-)
>
> Will apply, thanks.
>
> The substition from ftello with _ftelli64 does not appear in our
> codebase yet, but it was easy enough to adjust the patch myself, so
> no need to resend this patch.
Indeed, that is only in Git for Windows' code base yet, AFAICT.
For the record: I am currently holding off from contributing those patches
(I am talking about the patch series to make Git compile with MS Visual
C++ on the command line, followed by the patch series to generate project
definitions ready for use with MS Visual Studio) because of the feature
freeze. I had hoped to be able to contribute them sooner, but it took Jeff
Hostetler and myself a combined gargantuan effort to reorder and
disentangle Git for Windows' branch thicket so that those patches apply
cleanly on top of git.git's `master`.
Happily, almost all of the prerequisites made it upstream (e.g. the
nanosecond support for Windows, the patches to require Windows Vista or
later, the patch to use CreateHardLink() directly, etc). By my counting,
only two, relatively small patch series are left, and both are already
under discussion (but on hold, due to the code freeze).
For interested parties: the current shape of the `visual-studio` patch
series can be seen here:
https://github.com/git-for-windows/git/compare/581eb5441089%5E...581eb5441089%5E2
and the current shape of the `msvc` patch series can be seen here:
https://github.com/git-for-windows/git/compare/e9e7bd2a2485%5E...e9e7bd2a2485%5E2
Ciao,
Dscho
>
> > diff --git a/compat/msvc.h b/compat/msvc.h
> > index e6e1a6bbf7..2d558bae14 100644
> > --- a/compat/msvc.h
> > +++ b/compat/msvc.h
> > @@ -14,18 +14,12 @@
> > #define inline __inline
> > #define __inline__ __inline
> > #define __attribute__(x)
> > +#define strcasecmp _stricmp
> > #define strncasecmp _strnicmp
> > #define ftruncate_chsize
> > #define strtoull _strtoui64
> > #define strtoll _strtoi64
> >
> > -static __inline int strcasecmp (const char *s1, const char *s2)
> > -{
> > - int size1 = strlen(s1);
> > - int sisz2 = strlen(s2);
> > - return _strnicmp(s1, s2, sisz2 > size1 ? sisz2 : size1);
> > -}
> > -
> > #undef ERROR
> >
> > #define ftello _ftelli64
>
Re: Git Test Coverage Report (v2.20.0-rc0)
Hi Stolee, On Mon, 19 Nov 2018, Derrick Stolee wrote: > On 11/19/2018 1:33 PM, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Nov 19 2018, Derrick Stolee wrote: > > > > > [...] > > > builtin/rebase.c > > > 62c23938fa 55) return env; > > > [...] > > > Ævar Arnfjörð Bjarmason 62c23938f: tests: add a special setup > > > where rebase.useBuiltin is off > > This one would be covered with > > GIT_TEST_REBASE_USE_BUILTIN=false. Obviously trivial, but I wonder if > > the rest of the coverage would look different when passed through the > > various GIT_TEST_* options. > > > > Thanks for pointing out this GIT_TEST_* variable to me. I had been running > builds with some of them enabled, but didn't know about this one. > > Unfortunately, t3406-rebase-message.sh fails with > GIT_TEST_REBASE_USE_BUILTIN=false and it bisects to 4520c2337: Merge branch > 'ab/rebase-in-c-escape-hatch'. > > The issue is that the commit 04519d72 "rebase: validate -C and > --whitespace= parameters early" introduced the following test that cares > about error messages: > > +test_expect_success 'error out early upon -C or --whitespace=' ' > + test_must_fail git rebase -Cnot-a-number HEAD 2>err && > + test_i18ngrep "numerical value" err && > + test_must_fail git rebase --whitespace=bad HEAD 2>err && > + test_i18ngrep "Invalid whitespace option" err > +' > > The merge commit then was the first place where this test could run with that > variable. > > What's the correct fix here? Force the builtin rebase in this test? Unify the > error message in the non-builtin case? I am obviously biased, but my take is that the correct fix is in https://public-inbox.org/git/pull.86.git.gitgitgad...@gmail.com Ciao, Dscho
[PATCH 0/1] legacy-rebase: fix "regression"
This is a backport, really, to accommodate a new regression test that was introduced when the built-in rebase learned to validate the -C and --whitespace= arguments early. Johannes Schindelin (1): legacy-rebase: backport -C and --whitespace= checks git-legacy-rebase.sh | 8 1 file changed, 8 insertions(+) base-commit: bb75be6cb916297f271c846f2f9caa3daaaec718 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-86%2Fdscho%2Fscripted-rebase-Cn-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-86/dscho/scripted-rebase-Cn-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/86 -- gitgitgadget
[PATCH 1/1] legacy-rebase: backport -C and --whitespace= checks
From: Johannes Schindelin
Since 04519d720114 (rebase: validate -C and --whitespace=
parameters early, 2018-11-14), the built-in rebase validates the -C and
--whitespace arguments early. As this commit also introduced a
regression test for this, and as a later commit introduced the
GIT_TEST_REBASE_USE_BUILTIN mode to run tests, we now have a
"regression" in the scripted version of `git rebase` on our hands.
Backport the validation to fix this.
Reported-by: Ævar Arnfjörð Bjarmason
Signed-off-by: Johannes Schindelin
---
git-legacy-rebase.sh | 8
1 file changed, 8 insertions(+)
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 75a08b2683..ced0635326 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -337,6 +337,11 @@ do
fix|strip)
force_rebase=t
;;
+ warn|nowarn|error|error-all)
+ ;; # okay, known whitespace option
+ *)
+ die "Invalid whitespace option: '${1%*=}'"
+ ;;
esac
;;
--ignore-whitespace)
@@ -352,6 +357,9 @@ do
git_am_opt="$git_am_opt $1"
force_rebase=t
;;
+ -C*[!0-9]*)
+ die "switch \`C' expects a numerical value"
+ ;;
-C*)
git_am_opt="$git_am_opt $1"
;;
--
gitgitgadget
Re: [PATCH] rebase: mark a test as failing with rebase.useBuiltin=false
Hi Ævar,
On Tue, 20 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> Mark a test added in 04519d7201 ("rebase: validate -C and
> --whitespace= parameters early", 2018-11-14) as only succeeding
> with the builtin version of rebase. It would be nice if the
> shellscript version had the same fix, but it's on its way out, and the
> author is not interested in fixing it[1].
It's not that I am not interested in fixing it. It's more like I hoped
that I could work on Git for Windows v2.20.0-rc0 and rely on you to fix
this bug.
Now that Git for Windows v2.20.0-rc0 is out (see
https://github.com/git-for-windows/git/releases/tag/v2.20.0-rc0.windows.1),
expect a patch soon (see https://github.com/gitgitgadget/git/pull/86).
Ciao,
Dscho
>
> This makes the entire test suite pass again with the
> GIT_TEST_REBASE_USE_BUILTIN=false mode added in my 62c23938fa ("tests:
> add a special setup where rebase.useBuiltin is off", 2018-11-14).
>
> 1.
> https://public-inbox.org/git/nycvar.qro.7.76.6.1811201157170...@tvgsbejvaqbjf.bet/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
>
> On Tue, Nov 20 2018, Johannes Schindelin wrote:
>
> > [...]
> > Maybe you can concoct a prereq for this test? Something like
> >
> > test_lazy_prereq BUILTIN_REBASE '
> > test true = "${GIT_TEST_REBASE_USE_BUILTIN:-true}"
> > '
>
> It's better to just mark the test as needing the prereq turned
> off. E.g. this is what we do for the split index tests & now for the
> gettext tests. That way we always run the test, but just indicate that
> it relies on GIT_TEST_REBASE_USE_BUILTIN being unset.
>
> t/t3406-rebase-message.sh | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 38bd876cab..77e5bbb3d5 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -84,7 +84,8 @@ test_expect_success 'rebase --onto outputs the invalid ref'
> '
> test_i18ngrep "invalid-ref" err
> '
>
> -test_expect_success 'error out early upon -C or --whitespace=' '
> +test_expect_success 'builtin rebase: error out early upon -C or
> --whitespace=' '
> + sane_unset GIT_TEST_REBASE_USE_BUILTIN &&
> test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
> test_i18ngrep "numerical value" err &&
> test_must_fail git rebase --whitespace=bad HEAD 2>err &&
> --
> 2.20.0.rc0.387.gc7a69e6b6c
>
>
Re: [PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early
Hi Ævar,
On Mon, 19 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Nov 14 2018, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin
> >
> > It is a good idea to error out early upon seeing, say, `-Cbad`, rather
> > than starting the rebase only to have the `--am` backend complain later.
> >
> > Let's do this.
> >
> > The only options accepting parameters which we pass through to `git am`
> > (which may, or may not, forward them to `git apply`) are `-C` and
> > `--whitespace`. The other options we pass through do not accept
> > parameters, so we do not have to validate them here.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > builtin/rebase.c | 12 +++-
> > t/t3406-rebase-message.sh | 7 +++
> > 2 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 96ffa80b71..571cf899d5 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const
> > char *prefix)
> > }
> >
> > for (i = 0; i < options.git_am_opts.argc; i++) {
> > - const char *option = options.git_am_opts.argv[i];
> > + const char *option = options.git_am_opts.argv[i], *p;
> > if (!strcmp(option, "--committer-date-is-author-date") ||
> > !strcmp(option, "--ignore-date") ||
> > !strcmp(option, "--whitespace=fix") ||
> > !strcmp(option, "--whitespace=strip"))
> > options.flags |= REBASE_FORCE;
> > + else if (skip_prefix(option, "-C", )) {
> > + while (*p)
> > + if (!isdigit(*(p++)))
> > + die(_("switch `C' expects a "
> > + "numerical value"));
> > + } else if (skip_prefix(option, "--whitespace=", )) {
> > + if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
> > + strcmp(p, "error") && strcmp(p, "error-all"))
> > + die("Invalid whitespace option: '%s'", p);
> > + }
> > }
> >
> > if (!(options.flags & REBASE_NO_QUIET))
> > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> > index 0392e36d23..2c79eed4fe 100755
> > --- a/t/t3406-rebase-message.sh
> > +++ b/t/t3406-rebase-message.sh
> > @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid
> > ref' '
> > test_i18ngrep "invalid-ref" err
> > '
> >
> > +test_expect_success 'error out early upon -C or --whitespace=' '
> > + test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
> > + test_i18ngrep "numerical value" err &&
> > + test_must_fail git rebase --whitespace=bad HEAD 2>err &&
> > + test_i18ngrep "Invalid whitespace option" err
> > +'
> > +
>
> The combination of this gitster/js/rebase-am-options and my
> gitster/ab/rebase-in-c-escape-hatch breaks tests under
> GIT_TEST_REBASE_USE_BUILTIN=false for obvious reasons. The C version is
> now more strict.
Maybe you can concoct a prereq for this test? Something like
test_lazy_prereq BUILTIN_REBASE '
test true = "${GIT_TEST_REBASE_USE_BUILTIN:-true}"
'
Ciao,
Dscho
[PATCH 1/1] rebase: warn about the correct tree's OID
From: Johannes Schindelin
This was a simple copy/paste error, and an obvious one at that: if we
cannot fill the tree descriptor, we should show an error message about
*that* tree, not another one.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1a2758756a..5b3e5baec8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -582,7 +582,8 @@ static int reset_head(struct object_id *oid, const char
*action,
}
if (!reset_hard && !fill_tree_descriptor([nr++], _oid)) {
- ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
+ ret = error(_("failed to find tree of %s"),
+ oid_to_hex(_oid));
goto leave_reset_head;
}
--
gitgitgadget
[PATCH 0/1] rebase: warn about the correct tree's OID
A quick fix for a recent topic. Not overly critical, but I would deem this v2.20.0-rc1 material. Johannes Schindelin (1): rebase: warn about the correct tree's OID builtin/rebase.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) base-commit: bb75be6cb916297f271c846f2f9caa3daaaec718 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-85%2Fdscho%2Freset_head-typo-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-85/dscho/reset_head-typo-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/85 -- gitgitgadget
Re: [PATCH] bundle: dup() output descriptor closer to point-of-use
Hi Peff,
On Fri, 16 Nov 2018, Jeff King wrote:
> On Thu, Nov 15, 2018 at 09:01:07PM +0100, Johannes Schindelin wrote:
>
> > > It seems like we should be checking that the stale lockfile isn't left,
> > > which is the real problem (the warning is annoying, but is a symptom of
> > > the same thing). I.e., something like:
> > >
> > > test_must_fail git bundle create foobar.bundle master..master &&
> > > test_path_is_missing foobar.bundle.lock
> > >
> > > That would already pass on non-Windows platforms, but that's OK. It will
> > > never give a false failure.
> > >
> > > If you don't mind, can you confirm that the test above fails without
> > > either of the two patches under discussion?
> >
> > This test succeeds with your patch as well as with Gaël's, and fails when
> > neither patch is applied. So you're right, it is the much better test.
>
> Thanks for checking!
>
> > > > Do you want to integrate this test into your patch and run with it, or
> > > > do you want me to shepherd your patch?
> > >
> > > I'll wrap it up with a commit message and a test.
>
> Actually, I realized there's an even simpler way to do this. Here it is.
>
> -- >8 --
> Subject: [PATCH] bundle: dup() output descriptor closer to point-of-use
>
> When writing a bundle to a file, the bundle code actually creates
> "your.bundle.lock" using our lockfile interface. We feed that output
> descriptor to a child git-pack-objects via run-command, which has the
> quirk that it closes the output descriptor in the parent.
>
> To avoid confusing the lockfile code (which still thinks the descriptor
> is valid), we dup() it, and operate on the duplicate.
>
> However, this has a confusing side effect: after the dup() but before we
> call pack-objects, we have _two_ descriptors open to the lockfile. If we
> call die() during that time, the lockfile code will try to clean up the
> partially-written file. It knows to close() the file before unlinking,
> since on some platforms (i.e., Windows) the open file would block the
> deletion. But it doesn't know about the duplicate descriptor. On
> Windows, triggering an error at the right part of the code will result
> in the cleanup failing and the lockfile being left in the filesystem.
>
> We can solve this by moving the dup() much closer to start_command(),
> shrinking the window in which we have the second descriptor open. It's
> easy to place this in such a way that no die() is possible. We could
> still die due to a signal in the exact wrong moment, but we already
> tolerate races there (e.g., a signal could come before we manage to put
> the file on the cleanup list in the first place).
>
> As a bonus, this shields create_bundle() itself from the duplicate-fd
> trick, and we can simplify its error handling (note that the lock
> rollback now happens unconditionally, but that's OK; it's a noop if we
> didn't open the lock in the first place).
>
> The included test uses an empty bundle to cause a failure at the right
> spot in the code, because that's easy to trigger (the other likely
> errors are write() problems like ENOSPC). Note that it would already
> pass on non-Windows systems (because they are happy to unlink an
> already-open file).
Thanks, this is a very nice explanation (and now that I do not feel so
stressed as I did yesterday, I can easily wrap my head around it).
I can confirm that the test fails without the changes to bundle.c, and
succeeds with the changes.
Thank you so much!
Dscho
> Based-on-a-patch-by: Gaël Lhez
> Signed-off-by: Jeff King
> ---
> bundle.c| 39 ++-
> t/t5607-clone-bundle.sh | 6 ++
> 2 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index 1ef584b93b..6b0f6d8f10 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -243,7 +243,7 @@ static int is_tag_in_date_range(struct object *tag,
> struct rev_info *revs)
> }
>
>
> -/* Write the pack data to bundle_fd, then close it if it is > 1. */
> +/* Write the pack data to bundle_fd */
> static int write_pack_data(int bundle_fd, struct rev_info *revs)
> {
> struct child_process pack_objects = CHILD_PROCESS_INIT;
> @@ -256,6 +256,20 @@ static int write_pack_data(int bundle_fd, struct
> rev_info *revs)
> pack_objects.in = -1;
> pack_objects.out = bundle_fd;
> pack_objects.git_cmd = 1;
> +
> + /*
> + * start_command() will close our descriptor if it's >1. Duplicate it
> + * to avoid surprising the caller.
> + */
> + if (pack_objects.out > 1) {
>
Re: git pull --rebase=preserve is always rebasing something, even on up-to-date branch
Hi Mikhail, On Mon, 17 Sep 2018, Mikhail Matrosov wrote: > Please try the following: > > mmatrosov@Mikhail-PC:~/test$ git init --bare server > Initialized empty Git repository in /home/mmatrosov/test/server/ > mmatrosov@Mikhail-PC:~/test$ git clone server local > Cloning into 'local'... > warning: You appear to have cloned an empty repository. > done. > mmatrosov@Mikhail-PC:~/test$ cd local > mmatrosov@Mikhail-PC:~/test/local$ echo a > a && git add . && git commit -m A > [master (root-commit) a34c21f] A > 1 file changed, 1 insertion(+) > create mode 100644 a > mmatrosov@Mikhail-PC:~/test/local$ git push > Counting objects: 3, done. > Writing objects: 100% (3/3), 205 bytes | 0 bytes/s, done. > Total 3 (delta 0), reused 0 (delta 0) > To /home/mmatrosov/test/server > * [new branch] master -> master > mmatrosov@Mikhail-PC:~/test/local$ git pull > Already up-to-date. > mmatrosov@Mikhail-PC:~/test/local$ git pull --rebase=preserve > Rebasing (1/1) > Successfully rebased and updated refs/heads/master. > > As you can see, running bare "git pull" just tells me everything is up > to date. However, running "git pull --rebase=preserve" triggers > rebasing of something. This is most likely a `noop` command. > It wont be a problem if it didn't take significant time (especially on > Windows). Why this rebase happens? It is completely redundant and slows > down the pull operation. Looks like a bug to me. It is an implementation detail, and if you want to work on fixing it, please let me know about your availability and C skill level. > > Note that it is important to me, because I want to set "git config > --global pull.rebase preserve". Please note that `preserve` is on its way to deprecation, superseded by `pull.rebase = merges`. Which may, or may not, share the performance penalty you reported. Probably not, though. Ciao, Johannes > But because of this issue, pulling on an up-to-date repository takes a > lot of time. Which is very frustrating. > > Tested with: > * git version 2.19.0.windows.1 in Windows 10 Version 1803 > * git version 2.7.4 in Ubuntu 16.04.3 LTS (inside WSL) > > - > Best regards, Mikhail Matrosov >
Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin
Hi Junio,
On Fri, 16 Nov 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
>
> > The rebase.useBuiltin variable introduced in 55071ea248 ("rebase:
> > start implementing it as a builtin", 2018-08-07) was turned on by
> > default in 5541bd5b8f ("rebase: default to using the builtin rebase",
> > 2018-08-08), but had no documentation.
>
> I actually thought that everybody understood that this was merely an
> aid to be used during the development of the feature and never meant
> to be given to the end users.
It may have been from git.git's point of view, but from Git for Windows'
point of view it was always meant to be a real feature flag.
> With my devil's advocate hat on, how much do we trust it as an
> escape hatch?
As you know, only a fraction of the bug reports about the built-in rebase
came in from Git for Windows: the autostash-with-submodules bug and the
perf-regression one. By my counting that is 2 out of 5 bugs coming in via
that route.
One of the reasons for that was that the built-in rebase that was shipped
in Git for Windows v2.19.1 was marked as experimental.
And the way I could mark it experimental was by flipping the default to
executing the scripted version:
https://github.com/git-for-windows/git/commit/cff1a96cfe (you will note
that I added the same escape hatch for `git stash` by adding back
`git-stash.sh` as `git-legacy-stash.sh` and imitating the same dance as
for built-in `rebase`, and I also added back the scripted
`git-rebase--interactive.sh` for use by `git-legacy-rebase.sh`).
Meaning: essentially, `rebase.useBuiltin` was defaulting to `false`, and
if a user installed Git for Windows with the experimental built-in rebase,
it was set to `true` in the system config.
There was not a single complaint about the scripted `git rebase` being
broken in any way.
So we *do* have some real-world testing of that feature. (Obviously I have
no numbers about Git for Windows' usage, apart from download numbers, and
they do not say how many users opted in and how many did not, but Git for
Windows v2.19.1 was downloaded more than 2.7 million times so far and I
think it is safe to assume that some percentage tested that feature.)
> After all, the codepath to hide the "rebase in C" implementation and use
> the scripted version were never in 'master' (or 'next' for that matter)
> with this variable turned off, so I am reasonably sure it had no serious
> exposure to real world usage.
See above for a counter-argument.
> Having said that, assuming that the switching back to scripted
> version works correctly and assuming that we want to expose this to
> end users, I think the patch text makes sense.
Indeed.
I would still love to see the built-in rebase to be used by default in
v2.20.0, and I am reasonably sure that the escape hatch works (because, as
I told you above, it worked in the reverse, making the built-in rebase an
opt-in in Git for Windows v2.19.1).
Ciao,
Dscho
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
Hi Peff, On Thu, 15 Nov 2018, Jeff King wrote: > On Thu, Nov 15, 2018 at 05:32:15PM +0100, Johannes Schindelin wrote: > > > I cannot claim that I wrapped my head around your explanation or your > > diff (I am busy trying to prepare Git for Windows' `master` for > > rebasing to v2.20.0-rc0), but it does fix the problem. Thank you so > > much! > > > > The line `test_expect_code 1 ...` needs to be adjusted to > > `test_expect_code 128`, of course, and to discern from the fixed > > problem (which also exits with code 128), the error output should be > > verified, like so: > > > > -- snip -- > > test_expect_success 'try to create a bundle with empty ref count' ' > > test_must_fail git bundle create foobar.bundle master..master 2>err && > > test_i18ngrep "Refusing to create empty bundle" err > > ' > > -- snap -- > > It seems like we should be checking that the stale lockfile isn't left, > which is the real problem (the warning is annoying, but is a symptom of > the same thing). I.e., something like: > > test_must_fail git bundle create foobar.bundle master..master && > test_path_is_missing foobar.bundle.lock > > That would already pass on non-Windows platforms, but that's OK. It will > never give a false failure. > > If you don't mind, can you confirm that the test above fails without > either of the two patches under discussion? This test succeeds with your patch as well as with Gaël's, and fails when neither patch is applied. So you're right, it is the much better test. > > Do you want to integrate this test into your patch and run with it, or > > do you want me to shepherd your patch? > > I'll wrap it up with a commit message and a test. Thank you so much! Dscho
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
Hi Peff,
On Thu, 15 Nov 2018, Jeff King wrote:
> On Thu, Nov 15, 2018 at 01:57:45PM +0100, Johannes Schindelin wrote:
>
> > > I looked at the test to see if it would pass, but it is not even
> > > checking anything about lockfiles! It just checks that we exit 1 by
> > > returning up the callstack instead of calling die(). And of course it
> > > would not have a problem under Linux either way. But if I run something
> > > similar under strace, I see:
> > >
> > > $ strace ./git bundle create foobar.bundle HEAD..HEAD
> > > [...]
> > > openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock",
> > > O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3
> > > [...]
> > > close(3)= 0
> > > unlink("/home/peff/compile/git/foobar.bundle.lock") = 0
> > > exit_group(128) = ?
> > >
> > > which seems right.
> >
> > Without the fix, the added regression test fails thusly:
> >
> > -- snip --
> > [...]
> > ++ test_expect_code 1 git bundle create foobar.bundle master..master
> > ++ want_code=1
> > ++ shift
> > ++ git bundle create foobar.bundle master..master
> > fatal: Refusing to create empty bundle.
> > warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash
> > directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied
>
> Hmph. So who has it open, and why isn't the tempfile code working as
> designed?
>
> Aha, I see the problem. We dup() the descriptor in create_bundle(). So
> the patch _is_ necessary and (fairly) correct. But the explanation
> probably ought to be something like:
>
> In create_bundle(), we duplicate the lockfile descriptor via dup().
> This means that even though the lockfile code carefully calls close()
> before unlinking the lockfile, we may still have the file open. Unix
> systems don't care, but under Windows, this prevents the unlink
> (causing an annoying warning and a stale lockfile).
>
> But that also means that all of the other places we could die (e.g., in
> write_or_die) are going to have the same problem. We've fixed only one.
> Is there a way we can avoid doing the dup() in the first place?
>
> The comment there explains that we duplicate because write_pack_data()
> will close the descriptor. Unfortunately, that's hard to change because
> it comes from run-command. But we don't actually need the descriptor
> ourselves after it's closed; we're just trying to appease the lockfile
> code; see e54c347c1c (create_bundle(): duplicate file descriptor to
> avoid closing it twice, 2015-08-10).
>
> We just need some reasonable way of telling the lock code what's
> happening. Something like the patch below, which is a moral revert of
> e54c347c1c, but instead wrapping the "lock->fd = -1" in an official API.
>
> Does this make your warning go away?
>
> diff --git a/bundle.c b/bundle.c
> [...]
I cannot claim that I wrapped my head around your explanation or your diff (I am
busy trying to prepare Git for Windows' `master` for rebasing to v2.20.0-rc0),
but it does fix the problem. Thank you so much!
The line `test_expect_code 1 ...` needs to be adjusted to `test_expect_code
128`, of course, and to discern from the fixed problem (which also exits with
code 128), the error output should be verified, like so:
-- snip --
test_expect_success 'try to create a bundle with empty ref count' '
test_must_fail git bundle create foobar.bundle master..master 2>err &&
test_i18ngrep "Refusing to create empty bundle" err
'
-- snap --
Do you want to integrate this test into your patch and run with it, or do you
want me to shepherd your patch?
Ciao,
Dscho
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
Hi Peff, On Thu, 15 Nov 2018, Jeff King wrote: > On Thu, Nov 15, 2018 at 09:40:38AM +0100, Johannes Schindelin wrote: > > > From @chucklu: > > > > > my user case is like this : > > > > > > When I want to cherr-pick commits from A to G (ABCDEFG), image C and E > > > are merge commits. Then I will get lots of popup like: > > > > > >The previous cherry-pick is now empty, possibly due to conflict > > >resolution. > > >If you wish to commit it anyway, use: > > > > > >git commit --allow-empty > > > > > >If you wish to skip this commit, use: > > > > > >git reset > > > > > >Then "git cherry-pick --continue" will resume cherry-picking > > >the remaining commits. > > > > My quick interpretation of this is that the user actually needs a way to > > skip silently commits which are now empty. > > If it's always intended to be used with cherry-pick, shouldn't > cherry-pick learn a --keep-empty (like rebase has)? That would avoid > even stopping for this case in the first place. I'd go for the other way round: --skip-empty. However, given the very unhappy turn in that Git for Windows ticket (somebody asks for a feature, then just sits back, and does not even confirm that the analysis covers their use case, let alone participates in this discussion), I am personally not really interested in driving this one any further. Tanushree proved that they know how to contribute to the Git mailing list, as a pre-requisite for the Outreachy project, and that is the positive outcome of this thread as far as I am concerned. I am pretty happy about that, too. Ciao, Dscho
Re: [PATCH v2 1/1] bundle: cleanup lock files on error
Hi Peff,
On Wed, 14 Nov 2018, Jeff King wrote:
> On Wed, Nov 14, 2018 at 02:08:48PM -0800, Stefan Beller wrote:
>
> > On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren wrote:
> > >
> > > On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget
> > > wrote:
> > > > However, the `.lock` file was still open and on Windows that means
> > > > that it could not be deleted properly. This patch fixes that issue.
> > >
> > > Hmmm, doesn't the tempfile machinery remove the lock file when we die?
> >
> > On Windows this seems not to be the case. (Open files cannot be deleted
> > as the open file is not kept by inode or similar but by the file path
> > there?)
> >
> > Rewording your concern: Could the tempfile machinery be taught to
> > work properly on Windows, e.g. by first closing all files and then deleting
> > them afterwards?
>
> It already tries to do so. See delete_tempfile(), or more likely in the
> die() case, the remove_tempfiles() handler which is called at exit.
>
> Are we sure this is still a problem?
>
> I looked at the test to see if it would pass, but it is not even
> checking anything about lockfiles! It just checks that we exit 1 by
> returning up the callstack instead of calling die(). And of course it
> would not have a problem under Linux either way. But if I run something
> similar under strace, I see:
>
> $ strace ./git bundle create foobar.bundle HEAD..HEAD
> [...]
> openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock",
> O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3
> [...]
> close(3)= 0
> unlink("/home/peff/compile/git/foobar.bundle.lock") = 0
> exit_group(128) = ?
>
> which seems right.
Without the fix, the added regression test fails thusly:
-- snip --
[...]
++ test_expect_code 1 git bundle create foobar.bundle master..master
++ want_code=1
++ shift
++ git bundle create foobar.bundle master..master
fatal: Refusing to create empty bundle.
warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash
directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied
++ exit_code=128
++ test 128 = 1
++ echo 'test_expect_code: command exited with 128, we wanted 1 git bundle
create foobar.bundle master..master'
test_expect_code: command exited with 128, we wanted 1 git bundle create
foobar.bundle master..master
++ return 1
error: last command exited with $?=1
not ok 9 - try to create a bundle with empty ref count
#
# test_expect_code 1 git bundle create foobar.bundle
master..master
#
-- snap --
So yes, we are trying to unlink the `.lock` file, and as far as I can tell that
`unlink()` call comes from the tempfile cleanup asked for by Martin. However, as
we still have a handle open to that file, that call fails.
I do not think that there is any better way to fix this than to close the file
explicitly. If we tried to just close whatever file descriptor is still open to
that file before deleting it, we would possibly cause problems in code that is
still to be executed and assumes that it has a perfectly valid file descriptor.
Besides, trying to do this kind of "automatically" won't work, like, at all,
when it is one child process that holds an open file descriptor while another
process wants to delete the file.
Ciao,
Dscho
Re: [PATCH] technical doc: add a design doc for the evolve command
Hi Stefan, On Wed, 14 Nov 2018, sxe...@google.com wrote: > From: Stefan Xenos > > This document describes what an obsolescence graph for > git would look like, the behavior of the evolve command, > and the changes planned for other commands. Thanks, this is a good discussion starter. > +Objective > +- > +Track the edits to a commit over time in an obsolescence graph. I am not sure that we necessarily need this to be a graph. I think part of the problems with not being able to GC *any* of this is by this requirement to have it stored in a graph, rather than having mappings from which you could reconstruct any non-GC'ed parts of that graph, if you really want. > +Background > +-- > +Imagine you have three dependent changes up for review and you receive > feedback > +that requires editing all three changes. While you're editing one, more > feedback > +arrives on one of the others. What do you do? > + > +The evolve command is a convenient way to work with chains of commits that > are > +under review. Whenever you rebase or amend a commit, the repository remembers > +that the old commit is obsolete and has been replaced by the new one. Then, > at > +some point in the future, you can run "git evolve" and the correct sequence > of > +rebases will occur in the correct order such that no commit has an obsolete > +parent. > + > +Part of making the "evolve" command work involves tracking the edits to a > commit > +over time, which is why we need an obsolescence graph. However, the > obsolescence > +graph will also bring other benefits: > + > +- Users can view the history of a commit directly (the sequence of amends and > + rebases it has undergone, orthogonal to the history of the branch it is > on). > +- It will be possible to quickly locate and list all the changes the user > + currently has in progress. > +- It can be used as part of other high-level commands that combine or split > + changes. > +- It can be used to decorate commits (in git log, gitk, etc) that are either > + obsolete or are the tip of a work in progress. > +- By pushing and pulling the obsolescence graph, users can collaborate more > + easily on changes-in-progress. This is better than pushing and pulling the > + changes themselves since the obsolescence graph can be used to locate a > more > + specific merge base, allowing for better merges between different versions > of > + the same change. > +- It could be used to correctly rebase local changes and other local branches > + after running git-filter-branch. > +- It can replace the change-id footer used by gerrit. Okay. > +Similar technologies > + > +There are some other technologies that address the same end-user problem. > + > +Rebase -i can be used to solve the same problem, but users can't easily > switch > +tasks midway through an interactive rebase or have more than one interactive > +rebase going on at the same time. It can't handle the case where you have > +multiple changes sharing the same parent when that parent needs to be rebased > +and won't let you collaborate with others on resolving a complicated > interactive > +rebase. You can think of rebase -i as a top-down approach and the evolve > command > +as the bottom-up approach to the same problem. > + > +Several patch queue managers have been built on top of git (such as topgit, > +stgit, and quilt). They address the same user need. However they also rely on > +state managed outside git that needs to be kept in sync. Such state can be > +easily damaged when running a git native command that is unaware of the patch > +queue. They also typically require an explicit initialization step to be > done by > +the user which creates workflow problems. > + > +Replacements (refs/replace) are superficially similar to obsolescences in > that > +they describe that one commit should be replaced by another. However, they > +differ in both how they are created and how they are intended to be used. > +Obsolescences are created automatically by the commands a user runs, and they > +describe the user’s intent to perform a future rebase. Obsolete commits still > +appear in branches, logs, etc like normal commits (possibly with an extra > +decoration that marks them as obsolete). Replacements are typically created > +explicitly by the user, they are meant to be kept around for a long time, and > +they describe a replacement to be applied at read-time rather than as the > input > +to a future operation. When a replaced commit is queried, it is typically > hidden > +and swapped out with its replacement as though the replacement has already > +occurred. Why is this missing most notably `hg evolve`? Also, there should be *at least* a brief introduction how `hg evolve` works. They do have the benefit of real-world testing, and probably encountered problems and came up with solutions, and we would be remiss if we did not learn from them. Also, please do not forget `git imerge`.
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Hi Slavica, this looks very good to me. Just one grammar thing: On Wed, 14 Nov 2018, Slavica Djukic wrote: > Add test to document that stash fails if user.name and user.email > are not configured. > In the later commit, test will be updated to expect success. In a later commit [...] Otherwise, I would be totally fine with this version being merged. Ciao, Johannes > > Signed-off-by: Slavica Djukic > --- > t/t3903-stash.sh | 23 +++ > 1 file changed, 23 insertions(+) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index cd216655b..bab8bec67 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -1096,4 +1096,27 @@ test_expect_success 'stash -- works with > binary files' ' > test_path_is_file subdir/untracked > ' > > +test_expect_failure 'stash works when user.name and user.email are not set' ' > + git reset && > + git var GIT_COMMITTER_IDENT >expected && > + >1 && > + git add 1 && > + git stash && > + git var GIT_COMMITTER_IDENT >actual && > + test_cmp expected actual && > + >2 && > + git add 2 && > + test_config user.useconfigonly true && > + test_config stash.usebuiltin true && > + ( > + sane_unset GIT_AUTHOR_NAME && > + sane_unset GIT_AUTHOR_EMAIL && > + sane_unset GIT_COMMITTER_NAME && > + sane_unset GIT_COMMITTER_EMAIL && > + test_unconfig user.email && > + test_unconfig user.name && > + git stash > + ) > +' > + > test_done > -- > 2.19.1.1052.gd166e6afe > >
Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories
Hi Junio, On Wed, 14 Nov 2018, Jeff King wrote: > On Wed, Nov 14, 2018 at 02:16:37PM +0100, Johannes Schindelin wrote: > > > > Makes perfect sense. Shouldn't we be asking where the template > > > directory of the installed version is and using it instead of the > > > freshly built one, no? > > > > It would make sense, but we don't know how to get that information, do we? > > > > $ git grep DEFAULT_GIT_TEMPLATE_DIR > > Makefile: -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"' > > builtin/init-db.c:#ifndef DEFAULT_GIT_TEMPLATE_DIR > > builtin/init-db.c:#define DEFAULT_GIT_TEMPLATE_DIR > > "/usr/share/git-core/templates" > > builtin/init-db.c: template_dir = to_free = > > system_path(DEFAULT_GIT_TEMPLATE_DIR); > > contrib/vscode/init.sh: '-DDEFAULT_GIT_TEMPLATE_DIR="$(template_dir_SQ)"' \ > > > > In other words, the Makefile defines the DEFAULT_GIT_TEMPLATE_DIR, and the > > only user in the code is init-db.c which uses it in copy_templates(). > > > > And changing the code *now* to let us query Git where it thinks its > > templates should be won't work, as this patch is about using the installed > > Git (at whatever pre-compiled version that might be). > > Do we actually care where the templates are? I thought the point was to > override for the built Git to use the built templates instead of the > installed one. For an installed Git, shouldn't we not be overriding the > templates at all? I.e.: > > if test -n "$GIT_TEST_INSTALLED" > then > "$GIT_TEST_INSTALLED/git" init > else > "$GIT_ExEC_PATH/git" init --template="$GIT_BUILD_DIR/templates/blt" > fi >&3 2>&4 > > (That's all leaving aside the question of whether we ought to be using a > clean template dir instead of this). I fear that that might buy us a ton of trouble. Just like we override the system config, we should override the templates. Ciao, Dscho
Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
Hi Elijah, On Wed, 14 Nov 2018, Elijah Newren wrote: > On Mon, Nov 12, 2018 at 8:21 AM Johannes Schindelin > wrote: > > > t3425: topology linearization was inconsistent across flavors of rebase, > > > as already noted in a TODO comment in the testcase. This was not > > > considered a bug before, so getting a different linearization due > > > to switching out backends should not be considered a bug now. > > > > Ideally, the test would be fixed, then. If it fails for other reasons than > > a real regression, it is not a very good regression test, is it. > > I agree, it's not the best regression test. It'd be better if it > defined and tested for "the correct behavior", but I suspect it has > some value in that it's is guaranteeing that the rebase flavors at > least give a "somewhat reasonable" result. Sadly, It just allowed all > three rebase types to have slightly different "somewhat reasonable" > answers. True. I hope, though, that we can address this relatively easily (by toggling the `topo_order` flag). > > > t5407: different rebase types varied slightly in how many times checkout > > > or commit or equivalents were called based on a quick comparison > > > of this tests and previous ones which covered different rebase > > > flavors. I think this is just attributable to this difference. > > > > This concerns me. > > > > In bigger repositories (no, I am not talking about Linux kernel sized > > ones, I consider those small-ish) there are a ton of files, and checkout > > and commit (and even more so reset) sadly do not have a runtime complexity > > growing with the number of modified files, but with the number of tracked > > files (and some commands even with the number of worktree files). > > > > So a larger number of commit/checkout operations makes me expect > > performance regressions. > > > > In this light, could you elaborate a bit on the differences you see > > between rebase -i and rebase -m? > > I wrote this comment months ago and don't remember the full details. > From the wording and as best I remember, I suspect I was at least > partially annoyed by the fact that these and other regression tests > for rebase seemed to not be testing for "correct" behavior, but > "existing" behavior. That wouldn't be so bad, except that existing > behavior differed on the exact same test setup for different rebase > backends and the differences in behavior were checked and enforced in > the regression tests. So, it became a matter of taste as to how much > things should be made identical and to which backend, or whether it > was more important to at least just consolidate the code first and > keep the results at least reasonable. At the time, I figured getting > fewer backends, all with reasonable behavior was a bit more important, > but since this difference is worrisome to you, I will try to dig > further into it. I agree that there is a ton of inconsistency going on, both in the behavior of the different backends as well as in the tests and their quality. The best explanation I have for this is: it grew historically, and we always have to strike a balance between strict rule enforcement and fun. > > > t9903: --merge uses the interactive backend so the prompt expected is > > > now REBASE-i. > > > > We should be able to make that test pass, still, by writing out a special > > file (e.g. $state_dir/opt_m) and testing for that. Users are oddly upset > > when their expectations are broken... (and I actually agree with them.) > > I agree users are upset when expectations are broken, but why would > they expect REBASE-m? In fact, I thought the prompt was a reflection > of what backend was in use, so that if someone reported an issue to > the git mailing list or a power user wanted to know where the control > files were located, they could look for them. As such, I thought that > switching the prompt to REBASE-i was the right thing to do so that it > would match user expectations. Yes, the backend changed; that's part > of the release notes. When you put it that way, I have to agree with you. > Is there documentation somewhere that specifies the meaning of this > prompt differently than the expectations I had somehow built up? I was just looking from my perspective, with my experience: if I was a heavy user of `git rebase -m` (I am not), I would stumble over this prompt. That's all. With your explanation, I agree that this is the best course, though. > > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > > > in
[PATCH 0/1] mingw: update a link to the official documentation
It is a pretty neat thing that the bulky MSDN documentation has been replaced by something a lot more open, something that can be updated via Pull Requests on GitHub. Let's link to this new documentation instead of the obsolete one. I know, it is really close to the code freeze leading up to Git v2.20.0. But this is just an update to a code comment... :-) Johannes Schindelin (1): mingw: replace an obsolete link with the superseding one compat/mingw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) base-commit: d166e6afe5f257217836ef24a73764eba390c58d Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-81%2Fdscho%2Fmingw-update-msdn-link-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-81/dscho/mingw-update-msdn-link-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/81 -- gitgitgadget
[PATCH 1/1] mingw: replace an obsolete link with the superseding one
From: Johannes Schindelin
The MSDN documentation has been superseded by Microsoft Docs (which is
backed by a repository on GitHub containing many, many files in Markdown
format).
Signed-off-by: Johannes Schindelin
---
compat/mingw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index d2f4fabb44..9e42b0ee26 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1028,8 +1028,8 @@ char *mingw_getcwd(char *pointer, int len)
}
/*
- * See http://msdn2.microsoft.com/en-us/library/17w5ykft(vs.71).aspx
- * (Parsing C++ Command-Line Arguments)
+ * See "Parsing C++ Command-Line Arguments" at Microsoft's Docs:
+ * https://docs.microsoft.com/en-us/cpp/cpp/parsing-cpp-command-line-arguments
*/
static const char *quote_arg(const char *arg)
{
--
gitgitgadget
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
Hi, On Wed, 14 Nov 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > > > Agreed. I'm happy to see the test for-loop gone as I noted in > > https://public-inbox.org/git/87d0rm7zeo@evledraar.gmail.com/ but as > > noted in that v3 feedback the whole "why would anyone want this?" > > explanation is still missing, and this still smells like a workaround > > for a bug we should be fixing elsewhere in the sequencing code. > > Thanks. I share the same impression that this is sweeping a bug > under a wrong rug. I asked for clarification at https://github.com/git-for-windows/git/issues/1854 and in my best imitation of Lt Tawney Madison, I report back: >From @chucklu: > my user case is like this : > > When I want to cherr-pick commits from A to G (ABCDEFG), image C and E > are merge commits. Then I will get lots of popup like: > >The previous cherry-pick is now empty, possibly due to conflict >resolution. >If you wish to commit it anyway, use: > >git commit --allow-empty > >If you wish to skip this commit, use: > >git reset > >Then "git cherry-pick --continue" will resume cherry-picking >the remaining commits. My quick interpretation of this is that the user actually needs a way to skip silently commits which are now empty. Ciao, Dscho
Re: [PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early
Hi Phillip,
On Wed, 14 Nov 2018, Phillip Wood wrote:
> Thanks for doing this, I think this patch is good.
Thanks!
> I've not checked the first patch as I think it is the same as before
> judging from the covering letter.
Indeed, that's what the range-diff said. Sorry for not stating this
explicitly.
Thank you for your review,
Dscho
>
> Best Wishes
>
> Phillip
>
> On 14/11/2018 16:25, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin
> >
> > It is a good idea to error out early upon seeing, say, `-Cbad`, rather
> > than starting the rebase only to have the `--am` backend complain later.
> >
> > Let's do this.
> >
> > The only options accepting parameters which we pass through to `git am`
> > (which may, or may not, forward them to `git apply`) are `-C` and
> > `--whitespace`. The other options we pass through do not accept
> > parameters, so we do not have to validate them here.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > builtin/rebase.c | 12 +++-
> > t/t3406-rebase-message.sh | 7 +++
> > 2 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 96ffa80b71..571cf899d5 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv,
> > const char *prefix)
> >}
> >
> > for (i = 0; i < options.git_am_opts.argc; i++) {
> > - const char *option = options.git_am_opts.argv[i];
> > + const char *option = options.git_am_opts.argv[i], *p;
> > if (!strcmp(option, "--committer-date-is-author-date") ||
> > !strcmp(option, "--ignore-date") ||
> > !strcmp(option, "--whitespace=fix") ||
> > !strcmp(option, "--whitespace=strip"))
> > options.flags |= REBASE_FORCE;
> > + else if (skip_prefix(option, "-C", )) {
> > + while (*p)
> > + if (!isdigit(*(p++)))
> > + die(_("switch `C' expects a "
> > + "numerical value"));
> > + } else if (skip_prefix(option, "--whitespace=", )) {
> > + if (*p && strcmp(p, "warn") && strcmp(p, "nowarn")
> > &&
> > + strcmp(p, "error") && strcmp(p, "error-all"))
> > + die("Invalid whitespace option: '%s'", p);
> > + }
> >}
> >
> > if (!(options.flags & REBASE_NO_QUIET))
> > diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> > index 0392e36d23..2c79eed4fe 100755
> > --- a/t/t3406-rebase-message.sh
> > +++ b/t/t3406-rebase-message.sh
> > @@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the
> > invalid ref' '
> > test_i18ngrep "invalid-ref" err
> > '
> >
> > +test_expect_success 'error out early upon -C or --whitespace='
> > '
> > + test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
> > + test_i18ngrep "numerical value" err &&
> > + test_must_fail git rebase --whitespace=bad HEAD 2>err &&
> > + test_i18ngrep "Invalid whitespace option" err
> > +'
> > +
> > test_done
> >
>
>
[PATCH v2 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories
From: Johannes Schindelin
It really makes very, very little sense to use a different git
executable than the one the caller indicated via setting the environment
variable GIT_TEST_INSTALLED.
Signed-off-by: Johannes Schindelin
---
t/test-lib-functions.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index d158c8d0bf..3472716651 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -923,7 +923,8 @@ test_create_repo () {
mkdir -p "$repo"
(
cd "$repo" || error "Cannot setup test environment"
- "$GIT_EXEC_PATH/git-init"
"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
+ "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+ "--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
error "cannot run git init -- have you built things yet?"
mv .git/hooks .git/hooks-disabled
) || exit
--
gitgitgadget
[PATCH v2 4/5] tests: do not require Git to be built when testing an installed Git
From: Johannes Schindelin
We really only need the test helpers to be built in the worktree in that
case, but that is not what we test for.
On the other hand it is a perfect opportunity to verify that
`GIT_TEST_INSTALLED` points to a working Git.
So let's test the appropriate Git executable. While at it, also adjust
the error message in the `GIT_TEST_INSTALLED` case.
This patch is best viewed with `-w --patience`.
Helped-by: Jeff King
Signed-off-by: Johannes Schindelin
---
t/test-lib.sh | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 93883580a8..3d3a65ed0e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -51,10 +51,15 @@ export LSAN_OPTIONS
# It appears that people try to run tests without building...
-"$GIT_BUILD_DIR/git" >/dev/null
+"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git" >/dev/null
if test $? != 1
then
- echo >&2 'error: you do not seem to have built git yet.'
+ if test -n "$GIT_TEST_INSTALLED"
+ then
+ echo >&2 "error: there is no working Git at
'$GIT_TEST_INSTALLED'"
+ else
+ echo >&2 'error: you do not seem to have built git yet.'
+ fi
exit 1
fi
--
gitgitgadget
[PATCH v2 3/5] t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set
From: Johannes Schindelin It makes very, very little sense to test the built git-sh-i18n when the user asked specifically to test another one. Signed-off-by: Johannes Schindelin --- t/lib-gettext.sh | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh index eec757f104..9eb160c997 100644 --- a/t/lib-gettext.sh +++ b/t/lib-gettext.sh @@ -10,7 +10,12 @@ GIT_TEXTDOMAINDIR="$GIT_BUILD_DIR/po/build/locale" GIT_PO_PATH="$GIT_BUILD_DIR/po" export GIT_TEXTDOMAINDIR GIT_PO_PATH -. "$GIT_BUILD_DIR"/git-sh-i18n +if test -n "$GIT_TEST_INSTALLED" +then + . "$(git --exec-path)"/git-sh-i18n +else + . "$GIT_BUILD_DIR"/git-sh-i18n +fi if test_have_prereq GETTEXT && ! test_have_prereq GETTEXT_POISON then -- gitgitgadget
[PATCH v2 5/5] tests: explicitly use `git.exe` on Windows
From: Johannes Schindelin
On Windows, when we refer to `/an/absolute/path/to/git`, it magically
resolves `git.exe` at that location. Except if something of the name
`git` exists next to that `git.exe`. So if we call `$BUILD_DIR/git`, it
will find `$BUILD_DIR/git.exe` *only* if there is not, say, a directory
called `$BUILD_DIR/git`.
Such a directory, however, exists in Git for Windows when building with
Visual Studio (our Visual Studio project generator defaults to putting
the build files into a directory whose name is the base name of the
corresponding `.exe`).
In the bin-wrappers/* scripts, we already take pains to use `git.exe`
rather than `git`, as this could pick up the wrong thing on Windows
(i.e. if there exists a `git` file or directory in the build directory).
Now we do the same in the tests' start-up code.
This also helps when testing an installed Git, as there might be even
more likely some stray file or directory in the way.
Note: the only way we can record whether the `.exe` suffix is by writing
it to the `GIT-BUILD-OPTIONS` file and sourcing it at the beginning of
`t/test-lib.sh`. This is not a requirement introduced by this patch, but
we move the call to be able to use the `$X` variable that holds the file
extension, if any.
Note also: the many, many calls to `git this` and `git that` are
unaffected, as the regular PATH search will find the `.exe` files on
Windows (and not be confused by a directory of the name `git` that is
in one of the directories listed in the `PATH` variable), while
`/path/to/git` would not, per se, know that it is looking for an
executable and happily prefer such a directory.
Signed-off-by: Johannes Schindelin
---
Makefile| 1 +
t/test-lib-functions.sh | 2 +-
t/test-lib.sh | 13 +
3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/Makefile b/Makefile
index 016fdcdb81..21b3978744 100644
--- a/Makefile
+++ b/Makefile
@@ -2591,6 +2591,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst
','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
+ @echo X=\'$(X)\' >>$@+
ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
endif
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3472716651..274cbc2d6e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -923,7 +923,7 @@ test_create_repo () {
mkdir -p "$repo"
(
cd "$repo" || error "Cannot setup test environment"
- "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git" init \
+ "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" init \
"--template=$GIT_BUILD_DIR/templates/blt/" >&3 2>&4 ||
error "cannot run git init -- have you built things yet?"
mv .git/hooks .git/hooks-disabled
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3d3a65ed0e..e12addc324 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -49,9 +49,17 @@ export ASAN_OPTIONS
: ${LSAN_OPTIONS=abort_on_error=1}
export LSAN_OPTIONS
+if test ! -f "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+then
+ echo >&2 'error: GIT-BUILD-OPTIONS missing (has Git been built?).'
+ exit 1
+fi
+. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
+export PERL_PATH SHELL_PATH
+
# It appears that people try to run tests without building...
-"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git" >/dev/null
+"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
if test $? != 1
then
if test -n "$GIT_TEST_INSTALLED"
@@ -63,9 +71,6 @@ then
exit 1
fi
-. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-export PERL_PATH SHELL_PATH
-
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
case "$GIT_TEST_TEE_STARTED, $* " in
--
gitgitgadget
[PATCH v2 1/5] tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/
From: Johannes Schindelin
We really need to be able to find the test helpers... Really. This
change was forgotten when we moved the test helpers into t/helper/
Signed-off-by: Johannes Schindelin
---
t/test-lib.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index aba66cafa2..93883580a8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -957,7 +957,7 @@ elif test -n "$GIT_TEST_INSTALLED"
then
GIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path) ||
error "Cannot run git from $GIT_TEST_INSTALLED."
- PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR:$PATH
+ PATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR/t/helper:$PATH
GIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}
else # normal case, use ../bin-wrappers only unless $with_dashes:
git_bin_dir="$GIT_BUILD_DIR/bin-wrappers"
--
gitgitgadget
[PATCH v2 0/5] tests: various improvements to the GIT_TEST_INSTALLED feature
By setting the GIT_TEST_INSTALLED variable to the path of an installed Git
executable, it is possible to run the test suite also on a specific
installed version (as opposed to a version built from scratch).
The only thing this needs that is unlikely to be installed is the test
helper(s).
However, there have been a few rough edges around that, identified in my
(still ongoing) work to support building Git in Visual Studio (where we do
not want to run GNU Make, and where we have no canonical way to create, say,
hard-linked copies of the built-in commands, and other work to let Git for
Windows play better with BusyBox.
Triggered by a comment of AEvar
[https://public-inbox.org/git/20181102223743.4331-1-ava...@gmail.com/], I
hereby contribute these assorted fixes for the GIT_TEST_INSTALLED feature.
Changes since v1:
* Now we verify in test-lib.sh also in the GIT_TEST_INSTALLED case whether
the Git executable is working (thanks, Peff!).
* The commit message of 5/5 was touched up.
Johannes Schindelin (5):
tests: fix GIT_TEST_INSTALLED's PATH to include t/helper/
tests: respect GIT_TEST_INSTALLED when initializing repositories
t/lib-gettext: test installed git-sh-i18n if GIT_TEST_INSTALLED is set
tests: do not require Git to be built when testing an installed Git
tests: explicitly use `git.exe` on Windows
Makefile| 1 +
t/lib-gettext.sh| 7 ++-
t/test-lib-functions.sh | 3 ++-
t/test-lib.sh | 22 --
4 files changed, 25 insertions(+), 8 deletions(-)
base-commit: d166e6afe5f257217836ef24a73764eba390c58d
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-73%2Fdscho%2Ftest-git-installed-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-73/dscho/test-git-installed-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/73
Range-diff vs v1:
1: 2b04f9f086 = 1: 3b68e0fe8a tests: fix GIT_TEST_INSTALLED's PATH to
include t/helper/
2: 948b3dc146 = 2: 80d50d5932 tests: respect GIT_TEST_INSTALLED when
initializing repositories
3: eddea552e4 = 3: 49e408677a t/lib-gettext: test installed git-sh-i18n if
GIT_TEST_INSTALLED is set
4: 316e215e54 < -: -- tests: do not require Git to be built when
testing an installed Git
-: -- > 4: b801dc8027 tests: do not require Git to be built when
testing an installed Git
5: cd314e1384 ! 5: fbdb659de6 tests: explicitly use `git.exe` on Windows
@@ -2,6 +2,17 @@
tests: explicitly use `git.exe` on Windows
+On Windows, when we refer to `/an/absolute/path/to/git`, it magically
+resolves `git.exe` at that location. Except if something of the name
+`git` exists next to that `git.exe`. So if we call `$BUILD_DIR/git`,
it
+will find `$BUILD_DIR/git.exe` *only* if there is not, say, a
directory
+called `$BUILD_DIR/git`.
+
+Such a directory, however, exists in Git for Windows when building
with
+Visual Studio (our Visual Studio project generator defaults to putting
+the build files into a directory whose name is the base name of the
+corresponding `.exe`).
+
In the bin-wrappers/* scripts, we already take pains to use `git.exe`
rather than `git`, as this could pick up the wrong thing on Windows
(i.e. if there exists a `git` file or directory in the build
directory).
@@ -68,11 +79,12 @@
+
# It appears that people try to run tests without building...
--test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
-+test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git$X" >/dev/null ||
+-"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git" >/dev/null
++"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
if test $? != 1
then
- echo >&2 'error: you do not seem to have built git yet.'
+ if test -n "$GIT_TEST_INSTALLED"
+@@
exit 1
fi
--
gitgitgadget
[PATCH v2 2/2] rebase: validate -C and --whitespace= parameters early
From: Johannes Schindelin
It is a good idea to error out early upon seeing, say, `-Cbad`, rather
than starting the rebase only to have the `--am` backend complain later.
Let's do this.
The only options accepting parameters which we pass through to `git am`
(which may, or may not, forward them to `git apply`) are `-C` and
`--whitespace`. The other options we pass through do not accept
parameters, so we do not have to validate them here.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 12 +++-
t/t3406-rebase-message.sh | 7 +++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 96ffa80b71..571cf899d5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1064,12 +1064,22 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
}
for (i = 0; i < options.git_am_opts.argc; i++) {
- const char *option = options.git_am_opts.argv[i];
+ const char *option = options.git_am_opts.argv[i], *p;
if (!strcmp(option, "--committer-date-is-author-date") ||
!strcmp(option, "--ignore-date") ||
!strcmp(option, "--whitespace=fix") ||
!strcmp(option, "--whitespace=strip"))
options.flags |= REBASE_FORCE;
+ else if (skip_prefix(option, "-C", )) {
+ while (*p)
+ if (!isdigit(*(p++)))
+ die(_("switch `C' expects a "
+ "numerical value"));
+ } else if (skip_prefix(option, "--whitespace=", )) {
+ if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") &&
+ strcmp(p, "error") && strcmp(p, "error-all"))
+ die("Invalid whitespace option: '%s'", p);
+ }
}
if (!(options.flags & REBASE_NO_QUIET))
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 0392e36d23..2c79eed4fe 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -84,4 +84,11 @@ test_expect_success 'rebase --onto outputs the invalid ref' '
test_i18ngrep "invalid-ref" err
'
+test_expect_success 'error out early upon -C or --whitespace=' '
+ test_must_fail git rebase -Cnot-a-number HEAD 2>err &&
+ test_i18ngrep "numerical value" err &&
+ test_must_fail git rebase --whitespace=bad HEAD 2>err &&
+ test_i18ngrep "Invalid whitespace option" err
+'
+
test_done
--
gitgitgadget
[PATCH v2 1/2] rebase: really just passthru the `git am` options
From: Johannes Schindelin
Currently, we parse the options intended for `git am` as if we wanted to
handle them in `git rebase`, and then reconstruct them painstakingly to
define the `git_am_opt` variable.
However, there is a much better way (that I was unaware of, at the time
when I mentored Pratik to implement these options): OPT_PASSTHRU_ARGV.
It is intended for exactly this use case, where command-line options
want to be parsed into a separate `argv_array`.
Let's use this feature.
Incidentally, this also allows us to address a bug discovered by Phillip
Wood, where the built-in rebase failed to understand that the `-C`
option takes an optional argument.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 98 +---
1 file changed, 35 insertions(+), 63 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..96ffa80b71 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -87,7 +87,7 @@ struct rebase_options {
REBASE_FORCE = 1<<3,
REBASE_INTERACTIVE_EXPLICIT = 1<<4,
} flags;
- struct strbuf git_am_opt;
+ struct argv_array git_am_opts;
const char *action;
int signoff;
int allow_rerere_autoupdate;
@@ -339,7 +339,7 @@ N_("Resolve all conflicts manually, mark them as resolved
with\n"
static int run_specific_rebase(struct rebase_options *opts)
{
const char *argv[] = { NULL, NULL };
- struct strbuf script_snippet = STRBUF_INIT;
+ struct strbuf script_snippet = STRBUF_INIT, buf = STRBUF_INIT;
int status;
const char *backend, *backend_func;
@@ -433,7 +433,9 @@ static int run_specific_rebase(struct rebase_options *opts)
oid_to_hex(>restrict_revision->object.oid) : NULL);
add_var(_snippet, "GIT_QUIET",
opts->flags & REBASE_NO_QUIET ? "" : "t");
- add_var(_snippet, "git_am_opt", opts->git_am_opt.buf);
+ sq_quote_argv_pretty(, opts->git_am_opts.argv);
+ add_var(_snippet, "git_am_opt", buf.buf);
+ strbuf_release();
add_var(_snippet, "verbose",
opts->flags & REBASE_VERBOSE ? "t" : "");
add_var(_snippet, "diffstat",
@@ -756,7 +758,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
struct rebase_options options = {
.type = REBASE_UNSPECIFIED,
.flags = REBASE_NO_QUIET,
- .git_am_opt = STRBUF_INIT,
+ .git_am_opts = ARGV_ARRAY_INIT,
.allow_rerere_autoupdate = -1,
.allow_empty_message = 1,
.git_format_patch_opt = STRBUF_INIT,
@@ -777,12 +779,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
ACTION_EDIT_TODO,
ACTION_SHOW_CURRENT_PATCH,
} action = NO_ACTION;
- int committer_date_is_author_date = 0;
- int ignore_date = 0;
- int ignore_whitespace = 0;
const char *gpg_sign = NULL;
- int opt_c = -1;
- struct string_list whitespace = STRING_LIST_INIT_NODUP;
struct string_list exec = STRING_LIST_INIT_NODUP;
const char *rebase_merges = NULL;
int fork_point = -1;
@@ -804,15 +801,20 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
{OPTION_NEGBIT, 'n', "no-stat", , NULL,
N_("do not show diffstat of what changed upstream"),
PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },
- OPT_BOOL(0, "ignore-whitespace", _whitespace,
-N_("passed to 'git apply'")),
OPT_BOOL(0, "signoff", ,
N_("add a Signed-off-by: line to each commit")),
- OPT_BOOL(0, "committer-date-is-author-date",
-_date_is_author_date,
-N_("passed to 'git am'")),
- OPT_BOOL(0, "ignore-date", _date,
-N_("passed to 'git am'")),
+ OPT_PASSTHRU_ARGV(0, "ignore-whitespace", _am_opts,
+ NULL, N_("passed to 'git am'"),
+ PARSE_OPT_NOARG),
+ OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date",
+ _am_opts, NULL,
+ N_("passed to 'git am'"), PARSE_OPT_NOARG),
+ OPT_PASSTHRU_ARGV(0, "ignore-date", _am_opts, NULL,
+ N_("passed to 'git am'"), PARSE_OPT_NOARG),
+ OPT_PASSTHRU_ARGV('C', NULL, _am_opts, N_("n"),
+ N_("passed to 'git apply'"), 0),
+
[PATCH v2 0/2] rebase: understand -C again, refactor
Phillip Wood reported a problem where the built-in rebase did not understand options like -C1, i.e. it did not expect the option argument. While investigating how to address this best, I stumbled upon OPT_PASSTHRU_ARGV (which I was so far happily unaware of). Instead of just fixing the -C bug, I decided to simply convert all of the options intended for git am (or, eventually, for git apply). This happens to fix that bug, and does so much more: it simplifies the entire logic (and removes more lines than it adds). Change since v1: * Introduce early parameter validation for the options passed through to git am. Johannes Schindelin (2): rebase: really just passthru the `git am` options rebase: validate -C and --whitespace= parameters early builtin/rebase.c | 108 -- t/t3406-rebase-message.sh | 7 +++ 2 files changed, 52 insertions(+), 63 deletions(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-76%2Fdscho%2Frebase-Cn-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-76/dscho/rebase-Cn-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/76 Range-diff vs v1: 1: dc36a45068 = 1: dc36a45068 rebase: really just passthru the `git am` options -: -- > 2: 4c2ba52766 rebase: validate -C and --whitespace= parameters early -- gitgitgadget
Re: [PATCH v5 0/3] range-diff fixes
Hi Ævar,
On Tue, 13 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> Trivial updates since v4 addressing the feedback on that
> iteration. Hopefully this is the last one, range-diff with the last
> version:
This range-diff looks good to me.
Thanks,
Dscho
>
> 1: 5399e57513 = 1: f225173f43 range-diff doc: add a section about output
> stability
> 2: e56975df6c = 2: 77804ac641 range-diff: fix regression in passing along
> diff options
> 3: edfef733c7 ! 3: ed67dba073 range-diff: make diff option behavior (e.g.
> --stat) consistent
> @@ -17,8 +17,8 @@
>
> But we should behave consistently with "diff" in anticipation of such
> output being useful in the future, because it would make for
> confusing
> -UI if two "diff" and "range-diff" behaved differently when it came to
> -how they interpret diff options.
> +UI if "diff" and "range-diff" behaved differently when it came to how
> +they interpret diff options.
>
> The new behavior is also consistent with the existing documentation
> added in ba931edd28 ("range-diff: populate the man page",
> @@ -36,7 +36,7 @@
> memcpy(, diffopt, sizeof(opts));
> -opts.output_format |= DIFF_FORMAT_PATCH;
> +if (!opts.output_format)
> -+opts.output_format |= DIFF_FORMAT_PATCH;
> ++opts.output_format = DIFF_FORMAT_PATCH;
> opts.flags.suppress_diff_headers = 1;
> opts.flags.dual_color_diffed_diffs = dual_color;
> opts.output_prefix = output_prefix_cb;
> @@ -45,6 +45,12 @@
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@
> + '
> +
> + test_expect_success 'changed commit with --stat diff option' '
> +-four_spaces="" &&
> + git range-diff --no-color --stat topic...changed >actual &&
> + cat >expected <<-EOF &&
> 1: 4de457d = 1: a4b s/5/A/
>a => b | 0
>1 file changed, 0 insertions(+), 0 deletions(-)
>
> Ævar Arnfjörð Bjarmason (3):
> range-diff doc: add a section about output stability
> range-diff: fix regression in passing along diff options
> range-diff: make diff option behavior (e.g. --stat) consistent
>
> Documentation/git-range-diff.txt | 17 +
> range-diff.c | 3 ++-
> t/t3206-range-diff.sh| 30 ++
> 3 files changed, 49 insertions(+), 1 deletion(-)
>
> --
> 2.19.1.1182.g4ecb1133ce
>
>
[PATCH v2 0/1] bundle: fix issue when bundles would be empty
And yet another patch coming through Git for Windows... Change since v1: * Using a better oneline now. Gaël Lhez (1): bundle: cleanup lock files on error bundle.c| 7 --- t/t5607-clone-bundle.sh | 4 2 files changed, 8 insertions(+), 3 deletions(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-79%2Fdscho%2Fcreate-empty-bundle-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-79/dscho/create-empty-bundle-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/79 Range-diff vs v1: 1: 6276372ad3 ! 1: c7f05a bundle: refuse to create empty bundle @@ -1,6 +1,6 @@ Author: Gaël Lhez -bundle: refuse to create empty bundle +bundle: cleanup lock files on error When an user tries to create an empty bundle via `git bundle create ` where `` resolves to an empty list (for -- gitgitgadget
Re: [PATCH 1/1] bundle: refuse to create empty bundle
Hi Stefan, On Tue, 13 Nov 2018, Stefan Beller wrote: > On Tue, Nov 13, 2018 at 12:33 PM Gaël Lhez wrote: > > > > Hello, > > > > I don't know why I receive these message (and especially now given the time > > at which I pushed this) but I suppose someone (Johannes Schindelin ?) > > probably pushed back my original commit from git for windows github to GIT > > git repository. > > Yes that is pretty much what is happening. Johannes (GfW maintainer) > tries to push a lot of patches upstream to git and cc's people who > originally authored the patch. > Thanks for taking a look, again, even after this long time! > > > > > If you think "bundle: cleanup lock files on error" is better, then no > > problem with me. I'm not a native english speaker and I simply expressed > > the reason for my problem but - after reading back my commit - neither this > > mail' subject and my original commit subject (see > > https://github.com/git-for-windows/git/pull/797/commits/0ef742a1a92ae53188189238adbd16086fabf80a) > > express the core problem. > > I am not a native speaker either, which makes it extra hard to > understand some commits. ;-) So I propose a wording which would have > helped me. > > > As I'm not accustomed to pushing on GIT 'git repository' , please let me > > know if I have something else to do ? > > I don't know how Johannes handles pushing changes upstream, maybe he > will take on the work of resending a reworded patch. He will. > Let's hear his thoughts on it. I would guess you're more than welcome > to take your patches from GitForWindows into Git itself. As I merged the patch into Git for Windows' `master`, I consider it my responsibility to push this upstream (unless the contributor wants to take matters into their own hands). In the future, my hope is that GitGitGadget will make contributing patches to the Git mailing list a more convenient experience, to the point that it will hopefully be pretty much as easy as iterating a PR in https://github.com/git-for-windows/git. At that point, I will ask contributors to do exactly that in order to shepherd their patches into git.git. Ciao, Dscho > > Cheers, > Stefan >
