Re: [PATCH 0/1] rebase: understand -C again, refactor
Hi Peff, On Wed, 14 Nov 2018, Jeff King wrote: > On Tue, Nov 13, 2018 at 04:38:24AM -0800, Johannes Schindelin via > GitGitGadget wrote: > > > 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). > > I was unaware of it, too. Thanks, that makes me feel better ;-) > Looking at the OPT_PASSTHRU and its ARGV counterpart, I think the > original intent was that you'd pass through normal last-one-wins > individual options with OPT_PASSTHRU, and then list-like options with > OPT_PASSTHRU_ARGV. But here you've used the latter to pass sets of > individual last-one-wins options. > > That said, I think what you've done here is way simpler and more > readable than using a bunch of OPT_PASSTHRUs would have been. And even > if it was not the original intent of the ARGV variant, I can't see any > reason to avoid doing it this way. Thank you, that makes me feel *even* better ;-) Together with the extra-early error checking for incorrect -C and --whitespace options, I think we're in a much better place now. Ciao, Dscho
Re: [PATCH 1/1] rebase: really just passthru the `git am` options
Hi Phillip,
On Tue, 13 Nov 2018, Phillip Wood wrote:
> On 13/11/2018 19:21, Johannes Schindelin wrote:
> > Hi Phillip,
> >
> > On Tue, 13 Nov 2018, Phillip Wood wrote:
> >
> > > Thanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems to
> > > break the error reporting
> > >
> > > Running
> > >bin/wrappers/git rebase --onto @ @^^ -Cbad
> > >
> > > Gives
> > >git encountered an error while preparing the patches to replay
> > >these revisions:
> > >
> > >
> > > 67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c
> > >
> > >As a result, git cannot rebase them.
> > >
>
> git 2.19.1 gives
>
> First, rewinding head to replay your work on top of it...
> Applying: Ninth batch for 2.20
> error: switch `C' expects a numerical value
>
> So it has a clear message as to what the error is, this patch regresses that.
> It would be better if rebase detected the error before starting though.
I agree that that would make most sense: why start something when you can
know that it will fail later...
Let me try to add that test case that Junio wanted, and some early error
handling.
> > > If I do
> > >
> > >bin/wrappers/git rebase @^^ -Cbad
> > >
> > > I get no error, it just tells me that it does not need to rebase (which is
> > > true)
> >
> > Hmm. Isn't this the same behavior as with the scripted version?
>
> Ah you're right the script does not check if the option argument is valid.
>
> > Also: are
> > we sure that we want to allow options to come *after* the ``
> > argument?
>
> Maybe not but the scripted version does. I'm not sure if that is a good idea
> or not.
That behavior was never documented, though, was it?
Ciao,
Dscho
>
> Best Wishes
>
> Phillip
>
> > Ciao,
> > Dscho
> >
> > > Best Wishes
> > >
> > > Phillip
> > >
> > >
> > > On 13/11/2018 12:38, Johannes Schindelin via GitGitGadget wrote:
> > > > 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_t
Re: [PATCH 0/2] rebase.useBuiltin doc & test mode
Hi Ævar, On Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > On Wed, Nov 14 2018, Stefan Beller wrote: > > >> But maybe I'm being overly paranoid. What do those more familiar with > >> this think? > > > > I am not too worried, > > * as rebase is a main porcelain, that is even hard to use in a script. > > so any failures are not deep down in some automation, > > but when found exposed quickly (and hopefully reported). > > * 5541bd5b8f was merged to next a month ago; internally we > >distribute the next branch to Googlers (on a weekly basis) > >and we have not had any bug reports regarding rebase. > >(Maybe our environment is too strict for the wide range > > of bugs reported) > > I do the same at Booking.com (although at a more ad-hoc schedule) and > got the report whose fix is now sitting in "pu" noted upthread. > > I fear that these sorts of corporate environments, both Google's and > Booking's, end up testing a relatively narrow featureset. Most people > have similar enough workflows, e.g. just using "git pull --rebase", > I'd be surprised if we have more than 2-3 internal users who ever use > the --onto option for example. > > > * Johannes reported that the rebase is used in GfW > > > > https://public-inbox.org/git/nycvar.qro.7.76.6.1808241320540...@tvgsbejvaqbjf.bet/ > >https://github.com/git-for-windows/git/pull/1800 > >and from my cursory reading it is part of > >2.19.windows, which has a large user base. > > > >> (and would re-enable rebase.useBuiltin=true in > >> master right after 2.20 is out the door). > > > > That would be fine with me as well, but I'd rather > > document rebase.useBuiltin instead of flip-flopping > > the switch around the release. > > > > Have there been any fixes that are only in > > the C version (has the shell version already bitrotted)? > > That's a good question, one which I don't think we knew the answer to > before the following patches. I pay close attention to `git rebase` bug reports and patches (obviously), and there have not been any changes going into the built-in rebase/rebase -i that necessitated changes also in the scripted version. Ciao, Dscho > As it turns out no, we still run the tests without failures with > GIT_TEST_REBASE_USE_BUILTIN=false. > > Ævar Arnfjörð Bjarmason (2): > rebase doc: document rebase.useBuiltin > tests: add a special setup where rebase.useBuiltin is off > > Documentation/config/rebase.txt | 14 ++ > builtin/rebase.c| 5 - > t/README| 3 +++ > 3 files changed, 21 insertions(+), 1 deletion(-) > > -- > 2.19.1.1182.g4ecb1133ce > >
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 agree that the scenario is under-explained. Of course, I have to say that this is not Tanushree's problem; They only copied what is in https://github.com/git-for-windows/git/issues/1854 and @chucklu did not grace us with an explanation, either. Based on historical context, I would wager a bet that the scenario is that some commits that may or may not have been in a different SCM originally and that may or may not have been empty and/or squashed in `master` need to be cherry-picked. But I agree that this should be clarified. I prodded the original wish-haver. Ciao, Dscho
[PATCH v2 0/1] Some left-over add-on for bw/config-h
Back when bw/config-h was developed (and backported to Git for Windows), I
came up with a patch to use git_dir if commondir is NULL, and contributed
that as v1 of this patch. However, it was deemed a bug if that happens, so
let's instead detect that condition and report it.
Change since v1:
* Be loud about this bug instead of papering over it.
Johannes Schindelin (1):
config: report a bug if git_dir exists without commondir
config.c | 2 ++
1 file changed, 2 insertions(+)
base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-78%2Fdscho%2Fbw%2Fconfig-h-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-78/dscho/bw/config-h-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/78
Range-diff vs v1:
1: a3854e4ed8 ! 1: 0767f98378 do_git_config_sequence(): fall back to git_dir
if commondir is NULL
@@ -1,8 +1,9 @@
Author: Johannes Schindelin
-do_git_config_sequence(): fall back to git_dir if commondir is NULL
+config: report a bug if git_dir exists without commondir
-Just some defensive programming.
+This did happen at some stage, and was fixed relatively quickly. Make
+sure that we detect very quickly, too, should that happen again.
Signed-off-by: Johannes Schindelin
@@ -14,7 +15,7 @@
if (opts->commondir)
repo_config = mkpathdup("%s/config", opts->commondir);
+ else if (opts->git_dir)
-+ repo_config = mkpathdup("%s/config", opts->git_dir);
++ BUG("git_dir without commondir");
else
repo_config = NULL;
--
gitgitgadget
[PATCH v2 1/1] config: report a bug if git_dir exists without commondir
From: Johannes Schindelin
This did happen at some stage, and was fixed relatively quickly. Make
sure that we detect very quickly, too, should that happen again.
Signed-off-by: Johannes Schindelin
---
config.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/config.c b/config.c
index 4051e38823..db6b0167c6 100644
--- a/config.c
+++ b/config.c
@@ -1676,6 +1676,8 @@ static int do_git_config_sequence(const struct
config_options *opts,
if (opts->commondir)
repo_config = mkpathdup("%s/config", opts->commondir);
+ else if (opts->git_dir)
+ BUG("git_dir without commondir");
else
repo_config = NULL;
--
gitgitgadget
Re: [PATCH v2 2/2] tests: add a special setup where rebase.useBuiltin is off
Hi Ævar,
On Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> Add a GIT_TEST_REBASE_USE_BUILTIN=false test mode which is equivalent
> to running with rebase.useBuiltin=false. This is needed to spot that
> we're not introducing any regressions in the legacy rebase version
> while we're carrying both it and the new builtin version.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> builtin/rebase.c | 5 -
> t/README | 4
> 2 files changed, 8 insertions(+), 1 deletion(-)
I am slightly surprised not to see any ci/ change in this diffstat. Did
you mean to add a test axis for Travis, or not?
Ciao,
Dscho
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 0ee06aa363..68ad8c1149 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -48,7 +48,10 @@ static int use_builtin_rebase(void)
> {
> struct child_process cp = CHILD_PROCESS_INIT;
> struct strbuf out = STRBUF_INIT;
> - int ret;
> + int ret, env = git_env_bool("GIT_TEST_REBASE_USE_BUILTIN", -1);
> +
> + if (env != -1)
> + return env;
>
> argv_array_pushl(,
>"config", "--bool", "rebase.usebuiltin", NULL);
> diff --git a/t/README b/t/README
> index 242497455f..3df5d12e46 100644
> --- a/t/README
> +++ b/t/README
> @@ -339,6 +339,10 @@ for the index version specified. Can be set to any
> valid version
> GIT_TEST_PRELOAD_INDEX= exercises the preload-index code path
> by overriding the minimum number of cache entries required per thread.
>
> +GIT_TEST_REBASE_USE_BUILTIN=, when false, disables the
> +builtin version of git-rebase. See 'rebase.useBuiltin' in
> +git-config(1).
> +
> GIT_TEST_INDEX_THREADS= enables exercising the multi-threaded loading
> of the index for the whole test suite by bypassing the default number of
> cache entries and thread minimums. Setting this to 1 will make the
> --
> 2.19.1.1182.g4ecb1133ce
>
>
Re: [PATCH v2 1/2] rebase doc: document rebase.useBuiltin
Hi Ævar,
On Wed, 14 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> 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.
>
> Let's document it so that users who run into any stability issues with
> the C rewrite know there's an escape hatch[1], and make it clear that
> needing to turn off builtin rebase means you've found a bug in git.
>
> 1. https://public-inbox.org/git/87y39w1wc2@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
Makes sense.
Thanks,
Dscho
> ---
> Documentation/config/rebase.txt | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index 42e1ba7575..f079bf6b7e 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -1,3 +1,17 @@
> +rebase.useBuiltin::
> + Set to `false` to use the legacy shellscript implementation of
> + linkgit:git-rebase[1]. Is `true` by default, which means use
> + the built-in rewrite of it in C.
> ++
> +The C rewrite is first included with Git version 2.20. This option
> +serves an an escape hatch to re-enable the legacy version in case any
> +bugs are found in the rewrite. This option and the shellscript version
> +of linkgit:git-rebase[1] will be removed in some future release.
> ++
> +If you find some reason to set this option to `false` other than
> +one-off testing you should report the behavior difference as a bug in
> +git.
> +
> rebase.stat::
> Whether to show a diffstat of what changed upstream since the last
> rebase. False by default.
> --
> 2.19.1.1182.g4ecb1133ce
>
>
Re: [PATCH 4/5] tests: do not require Git to be built when testing an installed Git
Hi Peff,
On Wed, 14 Nov 2018, Jeff King wrote:
> On Mon, Nov 12, 2018 at 05:48:37AM -0800, Johannes Schindelin via
> GitGitGadget wrote:
>
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 832ede5099..1ea20dc2dc 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -51,7 +51,7 @@ export LSAN_OPTIONS
> >
> >
> > # It appears that people try to run tests without building...
> > -"$GIT_BUILD_DIR/git" >/dev/null
> > +test -n "$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.'
>
> The original runs the command independently and then checks $?. Your
> replacement chains the ||. I think it works, because the only case that
> is different is if running git returns 0 (in which case we currently
> complain, but the new code would quietly accept this).
>
> That should never happen, but if it does we'd probably want to complain.
> And it's pretty subtle all around. Maybe this would be a bit clearer:
>
> if test -n "$GIT_TEST_INSTALLED"
> then
> : assume installed git is OK
> else
> "$GIT_BUILD_DIR/git" >/dev/null
> if test $? != 1
> then
> ... die ...
> fi
> fi
>
> Though arguably we should be checking that there is a git in our path in
> the first instance. So maybe:
>
> if test -n "$GIT_TEST_INSTALLED"
> "$GIT_TEST_INSTALLED/git" >/dev/null
> else
> "$GIT_BUILD_DIR/git" >/dev/null
> fi
> if test $? != 1 ...
You're right. I did it using `"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git"`
and I also adjust the error message now.
Will be fixed in the next iteration,
Dscho
>
> -Peff
>
Re: [PATCH 5/5] tests: explicitly use `git.exe` on Windows
Hi Junio,
On Wed, 14 Nov 2018, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget"
> writes:
>
> > diff --git a/Makefile b/Makefile
> > index bbfbb4292d..5df0118ce9 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2590,6 +2590,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)\' >>$@+
>
> Made me wonder if a single letter $(X) is a bit too cute to expose
> to the outside world; as a narrowly confined local convention in
> this single Makefile, it was perfectly fine. But it should do for
> now. Its terseness is attractive, and our eyes (I do not speak for
> those new to our codebase and build structure) are already used to
> seeing $X suffix. Somebody may later come and complain but I am OK
> to rename it to something like $EXE at that time, not now.
>
> > 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 801cc9b2ef..c167b2e1af 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -900,7 +900,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 \
>
> Good.
>
> > "--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 1ea20dc2dc..3e2a9ce76d 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -49,18 +49,23 @@ 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
>
> OK, this tells us that we at least attempted to build git once, even
> under test-installed mode, and hopefully people won't run $(MAKE) and
> immediately ^C it only to fool us by leaving this file while keeping
> git binary and t/helpers/ binary unbuilt.
>
> > +. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> > +export PERL_PATH SHELL_PATH
> > +
> >
> > # 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 ||
>
> The latter half of this change is a good one. Given what the
> proposed log message of this patch says
>
> 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.
>
> which I read as "path-prefixed invocation, i.e. some/path/to/git, is
> bad, it must be spelled some/path/to/git.exe", I am surprised that
> tests ever worked on Windows without these five patches, as this
> line used to read like this:
>
> "$GIT_BUILD_DIR/git" >/dev/null
> if test $? != 1
> then
> ...
>
> Wouldn't "$GIT_BUILD_DIR/git" have failed (and "executable not
> found" hopefully won't produce exit code 1) and stopped the test
> suite from running even after you built git and not under the
> test-installed-git mode?
"$GIT_BUILD_DIR/git" did not fail until I regularly built with Visual
Studio (and my Visual Studio project generator generates a directory named
"git" to live alongside "git.exe").
And when it failed, I patched Git for Windows. Fast-forward, years later I
managed to contribute the patch we are discussing right now ;-)
So yes, it is primarily a concern when testing Git in specific setups
where a "git" directory can live next to the "git.exe" that we want to
test. Not necessarily a big deal for most developers on Windows.
Ciao,
Dscho
>
> > if test $? != 1
> > then
> > echo >&2 'error: you do not seem to have built git yet.'
> > 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
>
Re: [PATCH 4/5] tests: do not require Git to be built when testing an installed Git
Hi Junio, On Wed, 14 Nov 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > From: Johannes Schindelin > > > > We really only need the test helpers in that case, but that is not what > > we test for. So let's skip the test for now when we know that we want to > > test an installed Git. > > True, but... hopefully we are making sure t/helpers/ has been built > in some other ways, though, right? We do it implicitly, in the test cases that use the helpers. However, t/test-lib.sh does not particularly verify that the test helpers have been built. And I think that is a good thing: we do have several test scripts, I would think, that do not require any test helper to begin with. These scripts can work pretty well without having to build anything (read: on a machine where there is not even a toolchain available to build things). Besides, it is pretty much only t/helper/test-tool these days, and it is unlikely that anybody has a `test-tool` in their `PATH`. If they do, they'll find out soon enough iff they test with `GIT_TEST_INSTALLED` and iff they do not have the test helper(s) in t/helper/ ;-) Ciao, Dscho > I do not offhand see how tests would work in a pristine checkout with > "cd t && sh t-*.sh" as t/test-lib.sh is not running $(MAKE) itself > (and the design of the test-lib.sh, as can be seen in the check this > part of it makes, is to just abort if we cannot test) with this patch > (and PATCH 1/5) under the test-installed mode, though. > > > 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 832ede5099..1ea20dc2dc 100644 > > --- a/t/test-lib.sh > > +++ b/t/test-lib.sh > > @@ -51,7 +51,7 @@ export LSAN_OPTIONS > > > > > > # It appears that people try to run tests without building... > > -"$GIT_BUILD_DIR/git" >/dev/null > > +test -n "$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.' >
Re: [PATCH 2/5] tests: respect GIT_TEST_INSTALLED when initializing repositories
Hi Junio,
On Wed, 14 Nov 2018, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget"
> writes:
>
> > 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.
>
> 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).
Ciao,
Dscho
> >
> > 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 78d8c3783b..801cc9b2ef 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -900,7 +900,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
>
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
Hi,
On Tue, 13 Nov 2018, Tanushree Tumane wrote:
> From: tanushree27
>
> when we cherrypick an existing commit it doesn't change anything and
> therefore it fails prompting to reset (skip commit) or commit using
> --allow-empty attribute and then continue.
This is a nice paragraph, but it might make sense to connect it to the
commit's oneline somehow. I, for one, was surprised to see the oneline
talk about `git commit` and the commit message about `git cherry-pick`.
I could imagine that an introductory paragraph, talking about why one
might want to commit empty commits, might be the best lead into the
subject, and the paragraph about `cherry-pick` could follow (and be
introduced by saying something along the lines that this config setting
has more reach than just `git commit`; it also affects `git cherry-pick`)?
Ciao,
Johannes
>
> Add commit.allowEmpty configuration variable as a convenience to skip
> this process.
>
> Add tests to check the behavior introduced by this commit.
>
> This closes https://github.com/git-for-windows/git/issues/1854
>
> Signed-off-by: tanushree27
> Signed-off-by: Tanushree Tumane
> ---
> Documentation/config.txt | 5 +
> Documentation/git-commit.txt | 3 ++-
> builtin/commit.c | 8
> t/t3500-cherry.sh| 10 ++
> 4 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c0727b7866..f3828518a5 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1467,6 +1467,11 @@ commit.verbose::
> A boolean or int to specify the level of verbose with `git commit`.
> See linkgit:git-commit[1].
>
> +commit.allowEmpty::
> + A boolean to specify whether empty commits are allowed with `git
> + commit`. See linkgit:git-commit[1].
> + Defaults to false.
> +
> credential.helper::
> Specify an external helper to be called when a username or
> password credential is needed; the helper may consult external
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index f970a43422..5d3bbf017a 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -176,7 +176,8 @@ The `-m` option is mutually exclusive with `-c`, `-C`,
> and `-F`.
> Usually recording a commit that has the exact same tree as its
> sole parent commit is a mistake, and the command prevents you
> from making such a commit. This option bypasses the safety, and
> - is primarily for use by foreign SCM interface scripts.
> + is primarily for use by foreign SCM interface scripts. See
> + `commit.allowEmpty` in linkgit:git-config[1].
>
> --allow-empty-message::
> Like --allow-empty this command is primarily for use by foreign
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 67fa949204..4516309ac2 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -101,6 +101,7 @@ static int all, also, interactive, patch_interactive,
> only, amend, signoff;
> static int edit_flag = -1; /* unspecified */
> static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
> static int config_commit_verbose = -1; /* unspecified */
> +static int config_commit_allow_empty = -1; /* unspecified */
> static int no_post_rewrite, allow_empty_message;
> static char *untracked_files_arg, *force_date, *ignore_submodule_arg,
> *ignored_arg;
> static char *sign_commit;
> @@ -1435,6 +1436,10 @@ static int git_commit_config(const char *k, const char
> *v, void *cb)
> config_commit_verbose = git_config_bool_or_int(k, v, _bool);
> return 0;
> }
> + if (!strcmp(k, "commit.allowempty")) {
> + config_commit_allow_empty = git_config_bool(k, v);
> + return 0;
> + }
>
> status = git_gpg_config(k, v, NULL);
> if (status)
> @@ -1556,6 +1561,9 @@ int cmd_commit(int argc, const char **argv, const char
> *prefix)
> if (verbose == -1)
> verbose = (config_commit_verbose < 0) ? 0 :
> config_commit_verbose;
>
> + if (config_commit_allow_empty >= 0) /* if allowEmpty is allowed in
> config*/
> + allow_empty = config_commit_allow_empty;
> +
> if (dry_run)
> return dry_run_commit(argc, argv, prefix, current_head, );
> index_file = prepare_index(argc, argv, prefix, current_head, 0);
> diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh
> index f038f34b7c..11504e2d9f 100755
> --- a/t/t3500-cherry.sh
> +++ b/t/t3500-cherry.sh
> @@ -55,4 +55,14 @@ test_expect_success \
> expr "$(echo $(git cherry master my-topic-branch) )" : "+ [^ ]* - .*"
> '
>
> +
> +# Tests for commit.allowEmpty config
> +
> +test_expect_success 'cherry-pick existing commit with commit.allowEmpty' '
> +test_tick &&
> + test_commit "first" &&
> + test_commit "second" &&
> + git -c commit.allowEmpty=true cherry-pick HEAD~1
> +'
> +
>
Re: [PATCH 1/1] rebase: really just passthru the `git am` options
Hi Phillip,
On Tue, 13 Nov 2018, Phillip Wood wrote:
> Thanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems to
> break the error reporting
>
> Running
> bin/wrappers/git rebase --onto @ @^^ -Cbad
>
> Gives
> git encountered an error while preparing the patches to replay
> these revisions:
>
>
> 67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c
>
> As a result, git cannot rebase them.
>
> If I do
>
> bin/wrappers/git rebase @^^ -Cbad
>
> I get no error, it just tells me that it does not need to rebase (which is
> true)
Hmm. Isn't this the same behavior as with the scripted version? Also: are
we sure that we want to allow options to come *after* the ``
argument?
Ciao,
Dscho
> Best Wishes
>
> Phillip
>
>
> On 13/11/2018 12:38, Johannes Schindelin via GitGitGadget wrote:
> > 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-st
[PATCH 0/1] win32: modernize pthread_cond_*() shims
And yet another patch from Git for Windows' cache of treasures. It was challenging to emulate the functions related to pthread_cond_t as long as we tried to maintain support for Windows XP, which has nothing close to that feature. Now that we require Windows Vista or later, we can make use of the very nice functions associated with the CONDITION_VARIABLE data type. Look at how much code this deletes, and how little it introduces. This is a maintainer's dream. Loo Rong Jie (1): win32: replace pthread_cond_*() with much simpler code compat/win32/pthread.c | 138 - compat/win32/pthread.h | 28 +++-- 2 files changed, 7 insertions(+), 159 deletions(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-80%2Fdscho%2Fmingw-modernize-pthread_cond_t-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-80/dscho/mingw-modernize-pthread_cond_t-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/80 -- gitgitgadget
[PATCH 0/1] bundle: fix issue when bundles would be empty
And yet another patch coming through Git for Windows... Gaël Lhez (1): bundle: refuse to create empty bundle 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-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-79/dscho/create-empty-bundle-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/79 -- gitgitgadget
[PATCH 0/1] Some left-over add-on for bw/config-h
Back when bw/config-h was developed (and backported to Git for Windows), I came up with this patch. It seems to not be strictly necessary, but I like the safety of falling back to the Git directory when no common directory is configured (for whatever reason). Johannes Schindelin (1): do_git_config_sequence(): fall back to git_dir if commondir is NULL config.c | 2 ++ 1 file changed, 2 insertions(+) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-78%2Fdscho%2Fbw%2Fconfig-h-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-78/dscho/bw/config-h-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/78 -- gitgitgadget
[PATCH 1/1] do_git_config_sequence(): fall back to git_dir if commondir is NULL
From: Johannes Schindelin
Just some defensive programming.
Signed-off-by: Johannes Schindelin
---
config.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/config.c b/config.c
index 4051e38823..279d0d7077 100644
--- a/config.c
+++ b/config.c
@@ -1676,6 +1676,8 @@ static int do_git_config_sequence(const struct
config_options *opts,
if (opts->commondir)
repo_config = mkpathdup("%s/config", opts->commondir);
+ else if (opts->git_dir)
+ repo_config = mkpathdup("%s/config", opts->git_dir);
else
repo_config = NULL;
--
gitgitgadget
[PATCH 0/1] mingw: use CreateHardLink() directly
This is another tidbit from the stash of Git for Windows' patches: it avoids loading the function address of CreateHardLink() at runtime. This was done in case we were running on a Windows version that does not support that function, but we no longer support any of these Windows versions. Johannes Schindelin (1): mingw: use `CreateHardLink()` directly compat/mingw.c | 14 +- 1 file changed, 1 insertion(+), 13 deletions(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-77%2Fdscho%2Fmingw-CreateHardLink-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-77/dscho/mingw-CreateHardLink-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/77 -- gitgitgadget
[PATCH 1/1] mingw: use `CreateHardLink()` directly
From: Johannes Schindelin
The function `CreateHardLink()` is available in all supported Windows
versions (even since Windows XP), so there is no more need to resolve it
at runtime.
Helped-by: Max Kirillov
Signed-off-by: Johannes Schindelin
---
compat/mingw.c | 14 +-
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 3b44dd99d7..fdcf946275 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2084,24 +2084,12 @@ int mingw_raise(int sig)
int link(const char *oldpath, const char *newpath)
{
- typedef BOOL (WINAPI *T)(LPCWSTR, LPCWSTR, LPSECURITY_ATTRIBUTES);
- static T create_hard_link = NULL;
wchar_t woldpath[MAX_PATH], wnewpath[MAX_PATH];
if (xutftowcs_path(woldpath, oldpath) < 0 ||
xutftowcs_path(wnewpath, newpath) < 0)
return -1;
- if (!create_hard_link) {
- create_hard_link = (T) GetProcAddress(
- GetModuleHandle("kernel32.dll"), "CreateHardLinkW");
- if (!create_hard_link)
- create_hard_link = (T)-1;
- }
- if (create_hard_link == (T)-1) {
- errno = ENOSYS;
- return -1;
- }
- if (!create_hard_link(wnewpath, woldpath, NULL)) {
+ if (!CreateHardLinkW(wnewpath, woldpath, NULL)) {
errno = err_win_to_posix(GetLastError());
return -1;
}
--
gitgitgadget
Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
Hi Junio, On Tue, 13 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > >> For a trivially small change/fix like this, it is OK and even > >> preferrable to make 1+2 a single step, as applying t/ part only to > >> try to see the breakage (or "am"ing everything and then "diff | > >> apply -R" the part outside t/ for the same purpose) is easy enough. > > > > I disagree. It helps both development and porting to different branches to > > be able to cherry-pick the regression test individually. Please do not ask > > me to violate this hard-learned principle. > > A trivially small change/fix like this, by definition (of "trivial" > and "small" ness), it is trivial to develop and port to different > branches a single patch, and test with just one half (either the > test part or the code-change part) of the change reversed, to ensure > that the codebase is broken without the code-change and to > demonstrate that the code-change does fix the problem revealed by > the test change. And "porting" by cherry-picking a single patch is > always easier than two patch series. > > So you may disagree all you want in your project, but do not make > reviewer's lives unnecessarily harder in this project. You misunderstand. In this case it is crucial to read the regression test first. The fix does not make much sense before one understands the condition under which the order of the code statements matters. By trying to force me to smoosh them together, you are trying to force me to combine them in one patch where you would read the (now seemingly non-sensical) fix first, and only then the test. That's just really unhelpful. If I were a reviewer, I would want it presented in the way it *was* presented. I firmly believe most reviewers would. Ciao, Dscho
[PATCH 0/1] 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). Johannes Schindelin (1): rebase: really just passthru the `git am` options builtin/rebase.c | 98 +--- 1 file changed, 35 insertions(+), 63 deletions(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-76%2Fdscho%2Frebase-Cn-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-76/dscho/rebase-Cn-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/76 -- gitgitgadget
[PATCH 1/1] 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),
+
Re: [PATCH 1/2] refs: show --exclude failure with --branches/tags/remotes=glob
Hi Junio, On Tue, 13 Nov 2018, Junio C Hamano wrote: > Rafael Ascensão writes: > > > The documentation of `--exclude=` option from rev-list and rev-parse > > explicitly states that exclude patterns *should not* start with 'refs/' > > when used with `--branches`, `--tags` or `--remotes`. > > > > However, following this advice results in refereces not being excluded > > if the next `--branches`, `--tags`, `--remotes` use the optional > > inclusive glob. > > > > Demonstrate this failure. > > > > Signed-off-by: Rafael Ascensão > > --- > > t/t6018-rev-list-glob.sh | 60 ++-- > > 1 file changed, 57 insertions(+), 3 deletions(-) > > For a trivially small change/fix like this (i.e. the real fix in 2/2 > is just 4 lines), it is OK and even preferrable to make 1+2 a single > step, as applying t/ part only to try to see the breakage (or > "am"ing everything and then "diff | apply -R" the part outside t/ > for the same purpose) is easy enough. I wish you were not so adamant about this. I really consider it poor style to smoosh those together, and there is nothing easy about disentangling changes that have been thrown into the same commit. Please stop saying that this is easy. It is as easy as maintaining Linux kernel development using .tar files and patches. It is possible, yes, and Linus Torvalds did it for years. It is also error-prone and the entire reason we have Git. And nobody wants to go back anymore to .tar files and patches. Likewise, I do not want to read anybody recommending some semi-understandable diff|apply-R dance when the alternative would be a simple cherry-pick. I do not even want to read such a recommendation from you. I respect you a lot for what you do, and for your knowledge, but this is simply bad advice and I would wish you stopped giving it. Besides, we spent a decade trying to come up with clear-cut rules how to organize commits, and we ended up pretty quickly with recommending logically-separate changes belonging to separate commits. A typo fix should not be thrown in with a regression fix, they are two different things. Likewise, demonstrating a bug is a different thing from fixing it. If you need more arguments to make the case, here is another one: it is reflecting the reality a lot better if the regression test comes first, and then the fix. This is how Rafael did it, too, according to what he said on IRC. And reflecting this in the commit history is a good thing, not a bad thing. It goes further: obviously, Rafael had really good success with this strategy, even figuring out part of the bug while trying to write the regression test. I, myself wrote a regression test yesterday that completely short-circuited the bug hunt: originally, I thought the left-over MERGE_HEAD files in the rebase -r stemmed from mere conflicts during a `merge` command, and somehow `git commit` not cleaning it properly. But when I wrote that regression test and ran it, it failed to show a regression. So then I took my (rather lengthy: >200 todo commands) real-world example, and condensed it into the regression test that you saw yesterday. I would estimate that this saved me about 1-3 hours of debugging in vain. So it is a very, very good idea to start with the regression test, and only then analyze the bug. Reading the commit history this way makes therefore not only sense, but also sets a good example for new contributors to follow. For these reasons, and many more, I implore you to stop suggesting to conflate the demonstration of a bug with the fix. Instead, we should be happy to see good practices in action and encourage more of the same. Thank you, Dscho > Often the patch 2 with your method ends up showing only the test > set-up part in the context by changing _failure to _success, without > showing what end-user visible breakage the step fixed, which usually > comes near the end of the added test piece. For this particular > test, s/_failure/_success/ shows everything in the verification > phase, but the entire set-up for these tests cannot be seen while > reviewing 2/2. Unlike that, a single patch that gives tests that > ought to succeed would not force the readers to switch between > patches 1 and 2 while reading the fix. > > Of course, the above would not apply for a more involved case where > the actual fix to the code needs to span multiple patches. > > > diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh > > index 0bf10d0686..8e2b136356 100755 > > --- a/t/t6018-rev-list-glob.sh > > +++ b/t/t6018-rev-list-glob.sh > > @@ -36,7 +36,13 @@ test_expect_success 'setup' ' > > git tag foo/bar master && > > commit master3 && > > git update-ref refs/remotes/foo/baz master && > > - commit master4 > > + commit master4 && > > + git update-ref refs/remotes/upstream/one subspace/one && > > + git update-ref refs/remotes/upstream/two subspace/two && > > + git update-ref refs/remotes/upstream/x subspace-x && > > + git tag
Re: [PATCH 4/5] built-in rebase --skip/--abort: clean up stale .git/ files
Hi Junio,
On Tue, 13 Nov 2018, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget"
> writes:
>
> > From: Johannes Schindelin
> >
> > The scripted version of the rebase used to execute `git reset --hard`
> > when skipping or aborting. When we ported this to C, we did update the
> > worktree and some reflogs, but we failed to imitate `git reset --hard`'s
> > behavior regarding files in .git/ such as MERGE_HEAD.
> >
> > Let's address this oversight.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > builtin/rebase.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 0ee06aa363..017dce1b50 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -23,6 +23,7 @@
> > #include "revision.h"
> > #include "commit-reach.h"
> > #include "rerere.h"
> > +#include "branch.h"
> >
> > static char const * const builtin_rebase_usage[] = {
> > N_("git rebase [-i] [options] [--exec ] [--onto ] "
> > @@ -1002,6 +1003,7 @@ int cmd_rebase(int argc, const char **argv, const
> > char *prefix)
> >
> > if (reset_head(NULL, "reset", NULL, 0, NULL, NULL) < 0)
> > die(_("could not discard worktree changes"));
> > + remove_branch_state();
> > if (read_basic_state())
> > exit(1);
> > goto run_rebase;
> > @@ -1019,6 +1021,7 @@ int cmd_rebase(int argc, const char **argv, const
> > char *prefix)
> >options.head_name, 0, NULL, NULL) < 0)
> > die(_("could not move back to %s"),
> > oid_to_hex(_head));
> > + remove_branch_state();
>
> Hmph. Among 5 or so callsites of reset_head(), only these two
> places need it, so reset_head() is clearly not a substitute for
> "reset --hard HEAD", and it sometimes used to switch branches or
> something?
Indeed. There is also the `git reset --hard` call in the scripted
version's autostash code path. But that definitely does not need to remove
the branch state, as it is not recovering from a merge or cherry-pick or
revert.
> Perhaps we may need to rename it to avoid confusion but it can wait
> until we actually decide to make it externally available. Until then,
> it's OK as-is, I would think.
Okay.
Ciao,
Dscho
>
> > ret = finish_rebase();
> > goto cleanup;
> > }
>
Re: [PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
Hi Junio, On Tue, 13 Nov 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > From: Johannes Schindelin > > > > When calling `merge` on a branch that has already been merged, that > > `merge` is skipped quietly, but currently a MERGE_HEAD file is being > > left behind and will then be grabbed by the next `pick` (that did > > not want to create a *merge* commit). > > > > Demonstrate this. > > > > Signed-off-by: Johannes Schindelin > > --- > > t/t3430-rebase-merges.sh | 16 > > 1 file changed, 16 insertions(+) > > For a trivially small change/fix like this, it is OK and even > preferrable to make 1+2 a single step, as applying t/ part only to > try to see the breakage (or "am"ing everything and then "diff | > apply -R" the part outside t/ for the same purpose) is easy enough. I disagree. It helps both development and porting to different branches to be able to cherry-pick the regression test individually. Please do not ask me to violate this hard-learned principle. > Because the patch 2 with your method ends up showing only the test > set-up part in the context by changing _failure to _success, without > showing what end-user visible breakage the step fixed, which usually > comes near the end of the added test piece. A single patch that > gives tests that ought to succeed would not force the readers to > switch between patches 1 and 2 while reading the fix. That is why I put in a verbose commit message, so that you do not have to guess. And even the test title talks about this. Seriously, I am very much opposed to changing the patches in the direction you suggested. In my mind, they would make the story substantially worse. Thank you for your review, Dscho > > Of course, the above would not apply for a more involved case where > the actual fix to the code needs to span multiple patches. > > Thanks. > > > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > > index aa7bfc88ec..1f08a33687 100755 > > --- a/t/t3430-rebase-merges.sh > > +++ b/t/t3430-rebase-merges.sh > > @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' ' > > grep "G: +G" actual > > ' > > > > +test_expect_failure '--continue after resolving conflicts after a merge' ' > > + git checkout -b already-has-g E && > > + git cherry-pick E..G && > > + test_commit H2 && > > + > > + git checkout -b conflicts-in-merge H && > > + test_commit H2 H2.t conflicts H2-conflict && > > + test_must_fail git rebase -r already-has-g && > > + grep conflicts H2.t && > > Is this making sure that the above test_must_fail succeeded because > of a conflict and not due to any other failure? I would have used > "ls-files -u H2.t" to see if the index is unmerged, which probably > is a more direct way to test what this is trying to test, but if we > are in the conflicted state, the one side of << == >> has this > string (the other has "H2" in it, presumably?), so in practice this > should be good enough. > > > + echo resolved >H2.t && > > + git add -u && > > and we resolve to continue. > > > + git rebase --continue && > > + test_must_fail git rev-parse --verify HEAD^2 && > > Even if we made an octopus by mistake, the above will catch it, > which is good. > > > + test_path_is_missing .git/MERGE_HEAD > > +' > > + > > test_done > > And from the proposed log message, I am reading that the last two > things (i.e. resulting tip is a child with a single parent and there > is no leftover MERGE_HEAD file) fail without the fix. > > This is enough material to convince me or anybody that the bug is > worth fixing. Thanks for being careful noticing a glitch during > your real (and otherwise unrelated to the bug) work and following > through. >
Re: Regression: rebase -C1 fails in master
Hi Phillip, On Mon, 12 Nov 2018, Phillip Wood wrote: > I've just tried running > > bin-wrappers/git rebase -C1 @^ > > and I get > > error: unknown switch `1' Darn. I think this is the same problem as the `-S` switch problem, but in reverse: the built-in rebase used to require an argument for `-S` (it should be optional, fixed in the meantime), but `-C` also has an optional argument. Let me see whether I can fix this quickly. Ciao, Dscho > usage: git rebase [-i] [options] [--exec ] [--onto ] > [] [] >or: git rebase [-i] [options] [--exec ] [--onto ] > --root [] >or: git rebase --continue | --abort | --skip | --edit-todo > ... > > bin-wrappers/git --version gives > git version 2.19.1.856.g8858448bb4 > > Unfortunately I've not got time it investigate properly at the moment. > > Best Wishes > > Phillip >
Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
Hi Phillip, On Mon, 12 Nov 2018, Phillip Wood wrote: > It's good to see these patches progressing, I've just got a couple of > comments related to Johannes' points below > > On 12/11/2018 16:21, Johannes Schindelin wrote: > > Hi Elijah, > > > > On Wed, 7 Nov 2018, Elijah Newren wrote: > > > >> Interactive rebases are implemented in terms of cherry-pick rather than > >> the merge-recursive builtin, but cherry-pick also calls into the recursive > >> merge machinery by default and can accept special merge strategies and/or > >> special strategy options. As such, there really is not any need for > >> having both git-rebase--merge and git-rebase--interactive anymore. > >> > >> Delete git-rebase--merge.sh and have the --merge option be implemented > >> by the now built-in interactive machinery. > > > > Okay. > > > >> Note that this change fixes a few known test failures (see t3421). > > > > Nice! > > > >> testcase modification notes: > >> t3406: --interactive and --merge had slightly different progress output > >> while running; adjust a test to match > >> t3420: these test precise output while running, but rebase--am, > >> rebase--merge, and rebase--interactive all were built on very > >> different commands (am, merge-recursive, cherry-pick), so the > >> tests expected different output for each type. Now we expect > >> --merge and --interactive to have the same output. > >> t3421: --interactive fixes some bugs in --merge! Wahoo! > >> 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. > > > >> 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? > > > >> 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.) > > > >> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > >> index 3407d835bd..35084f5681 100644 > >> --- a/Documentation/git-rebase.txt > >> +++ b/Documentation/git-rebase.txt > >> @@ -504,15 +504,7 @@ See also INCOMPATIBLE OPTIONS below. > >> INCOMPATIBLE OPTIONS > >> > >> > >> -git-rebase has many flags that are incompatible with each other, > >> -predominantly due to the fact that it has three different underlying > >> -implementations: > >> - > >> - * one based on linkgit:git-am[1] (the default) > >> - * one based on git-merge-recursive (merge backend) > >> - * one based on linkgit:git-cherry-pick[1] (interactive backend) > >> - > > > > Could we retain this part, with `s/three/two/` and `/git-merge/d`? *Maybe* > > `s/interactive backend/interactive\/merge backend/` > > I was hoping we could get rid of that, I'm not sure how useful it is to > users. That's a good point. If the commit message makes the case for that (it is an implementation detail that confuses users), I am fine with removing it, too. Thanks
Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long
Hi,
On Mon, 12 Nov 2018, Carlo Marcelo Arenas Belón wrote:
> There are still some more possible improvements around this code but
> they are orthogonal to this change :
>
> * migrate to approxidate_careful or parse_expiry_date
> * maybe make sure only approxidate are used for expiration
>
> Changes in v2:
> * improved commit message as suggested by Eric
> * failsafe against time_t truncation as suggested by Junio
>
> -- >8 --
> Subject: [PATCH v2 2/2] read-cache: use time specific types instead of
> unsigned long
>
> b968372279 ("read-cache: unlink old sharedindex files", 2017-03-06)
> introduced get_shared_index_expire_date using unsigned long to track
> the modification times of a shared index.
>
> dddbad728c ("timestamp_t: a new data type for timestamps", 2017-04-26)
> shows why that might be problematic so move to timestamp_t/time_t.
>
> if time_t can't represent a valid time keep the indexes for failsafe
Is this sentence incomplete? What are those "indexes"?
Thanks,
Johannes
>
> Signed-off-by: Carlo Marcelo Arenas Belón
> ---
> read-cache.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 7b1354d759..7d322f11c8 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2625,9 +2625,9 @@ static int write_split_index(struct index_state *istate,
>
> static const char *shared_index_expire = "2.weeks.ago";
>
> -static unsigned long get_shared_index_expire_date(void)
> +static timestamp_t get_shared_index_expire_date(void)
> {
> - static unsigned long shared_index_expire_date;
> + static timestamp_t shared_index_expire_date;
> static int shared_index_expire_date_prepared;
>
> if (!shared_index_expire_date_prepared) {
> @@ -2643,12 +2643,12 @@ static unsigned long
> get_shared_index_expire_date(void)
> static int should_delete_shared_index(const char *shared_index_path)
> {
> struct stat st;
> - unsigned long expiration;
> + time_t expiration;
> + timestamp_t t = get_shared_index_expire_date();
>
> - /* Check timestamp */
> - expiration = get_shared_index_expire_date();
> - if (!expiration)
> + if (!t || date_overflows(t))
> return 0;
> + expiration = t;
> if (stat(shared_index_path, ))
> return error_errno(_("could not stat '%s'"), shared_index_path);
> if (st.st_mtime > expiration)
> --
> 2.19.1.856.g8858448bb
>
>
[PATCH 2/5] rebase -r: do not write MERGE_HEAD unless needed
From: Johannes Schindelin
When we detect that a `merge` can be skipped because the merged commit
is already an ancestor of HEAD, we do not need to commit, therefore
writing the MERGE_HEAD file is useless.
It is actually worse than useless: a subsequent `git commit` will pick
it up and think that we want to merge that commit, still.
To avoid that, move the code that writes the MERGE_HEAD file to a
location where we already know that the `merge` cannot be skipped.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 8
t/t3430-rebase-merges.sh | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 9e1ab3a2a7..7a9cd81afb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3191,10 +3191,6 @@ static int do_merge(struct commit *commit, const char
*arg, int arg_len,
}
merge_commit = to_merge->item;
- write_message(oid_to_hex(_commit->object.oid), GIT_SHA1_HEXSZ,
- git_path_merge_head(the_repository), 0);
- write_message("no-ff", 5, git_path_merge_mode(the_repository), 0);
-
bases = get_merge_bases(head_commit, merge_commit);
if (bases && oideq(_commit->object.oid,
>item->object.oid)) {
@@ -3203,6 +3199,10 @@ static int do_merge(struct commit *commit, const char
*arg, int arg_len,
goto leave_merge;
}
+ write_message(oid_to_hex(_commit->object.oid), GIT_SHA1_HEXSZ,
+ git_path_merge_head(the_repository), 0);
+ write_message("no-ff", 5, git_path_merge_mode(the_repository), 0);
+
for (j = bases; j; j = j->next)
commit_list_insert(j->item, );
free_commit_list(bases);
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 1f08a33687..cc5646836f 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -396,7 +396,7 @@ test_expect_success 'with --autosquash and --exec' '
grep "G: +G" actual
'
-test_expect_failure '--continue after resolving conflicts after a merge' '
+test_expect_success '--continue after resolving conflicts after a merge' '
git checkout -b already-has-g E &&
git cherry-pick E..G &&
test_commit H2 &&
--
gitgitgadget
[PATCH 3/5] rebase -i: include MERGE_HEAD into files to clean up
From: Johannes Schindelin
Every once in a while, the interactive rebase makes sure that no stale
files are lying around. These days, we need to include MERGE_HEAD into
that set of files, as the `merge` command will generate them.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 7a9cd81afb..2f526390ac 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3459,6 +3459,7 @@ static int pick_commits(struct todo_list *todo_list,
struct replay_opts *opts)
unlink(rebase_path_author_script());
unlink(rebase_path_stopped_sha());
unlink(rebase_path_amend());
+ unlink(git_path_merge_head(the_repository));
delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
if (item->command == TODO_BREAK)
@@ -3829,6 +3830,7 @@ static int commit_staged_changes(struct replay_opts *opts,
opts, flags))
return error(_("could not commit staged changes."));
unlink(rebase_path_amend());
+ unlink(git_path_merge_head(the_repository));
if (final_fixup) {
unlink(rebase_path_fixup_msg());
unlink(rebase_path_squash_msg());
--
gitgitgadget
[PATCH 5/5] status: rebase and merge can be in progress at the same time
From: Johannes Schindelin
Since `git rebase -r` was introduced, that is possible. But our
machinery did not think that possible, and failed to say anything about
the rebase in progress when in the middle of a merge.
Let's work around that in the minimal fashion.
Signed-off-by: Johannes Schindelin
---
wt-status.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 187568a112..a24711374c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1559,6 +1559,7 @@ void wt_status_get_state(struct wt_status_state *state,
struct object_id oid;
if (!stat(git_path_merge_head(the_repository), )) {
+ wt_status_check_rebase(NULL, state);
state->merge_in_progress = 1;
} else if (wt_status_check_rebase(NULL, state)) {
; /* all set */
@@ -1583,9 +1584,13 @@ static void wt_longstatus_print_state(struct wt_status
*s)
const char *state_color = color(WT_STATUS_HEADER, s);
struct wt_status_state *state = >state;
- if (state->merge_in_progress)
+ if (state->merge_in_progress) {
+ if (state->rebase_interactive_in_progress) {
+ show_rebase_information(s, state_color);
+ fputs("\n", s->fp);
+ }
show_merge_in_progress(s, state_color);
- else if (state->am_in_progress)
+ } else if (state->am_in_progress)
show_am_in_progress(s, state_color);
else if (state->rebase_in_progress ||
state->rebase_interactive_in_progress)
show_rebase_in_progress(s, state_color);
--
gitgitgadget
[PATCH 1/5] rebase -r: demonstrate bug with conflicting merges
From: Johannes Schindelin When calling `merge` on a branch that has already been merged, that `merge` is skipped quietly, but currently a MERGE_HEAD file is being left behind and will then be grabbed by the next `pick` (that did not want to create a *merge* commit). Demonstrate this. Signed-off-by: Johannes Schindelin --- t/t3430-rebase-merges.sh | 16 1 file changed, 16 insertions(+) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index aa7bfc88ec..1f08a33687 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -396,4 +396,20 @@ test_expect_success 'with --autosquash and --exec' ' grep "G: +G" actual ' +test_expect_failure '--continue after resolving conflicts after a merge' ' + git checkout -b already-has-g E && + git cherry-pick E..G && + test_commit H2 && + + git checkout -b conflicts-in-merge H && + test_commit H2 H2.t conflicts H2-conflict && + test_must_fail git rebase -r already-has-g && + grep conflicts H2.t && + echo resolved >H2.t && + git add -u && + git rebase --continue && + test_must_fail git rev-parse --verify HEAD^2 && + test_path_is_missing .git/MERGE_HEAD +' + test_done -- gitgitgadget
[PATCH 4/5] built-in rebase --skip/--abort: clean up stale .git/ files
From: Johannes Schindelin
The scripted version of the rebase used to execute `git reset --hard`
when skipping or aborting. When we ported this to C, we did update the
worktree and some reflogs, but we failed to imitate `git reset --hard`'s
behavior regarding files in .git/ such as MERGE_HEAD.
Let's address this oversight.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..017dce1b50 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -23,6 +23,7 @@
#include "revision.h"
#include "commit-reach.h"
#include "rerere.h"
+#include "branch.h"
static char const * const builtin_rebase_usage[] = {
N_("git rebase [-i] [options] [--exec ] [--onto ] "
@@ -1002,6 +1003,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
if (reset_head(NULL, "reset", NULL, 0, NULL, NULL) < 0)
die(_("could not discard worktree changes"));
+ remove_branch_state();
if (read_basic_state())
exit(1);
goto run_rebase;
@@ -1019,6 +1021,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
options.head_name, 0, NULL, NULL) < 0)
die(_("could not move back to %s"),
oid_to_hex(_head));
+ remove_branch_state();
ret = finish_rebase();
goto cleanup;
}
--
gitgitgadget
[PATCH 0/5] Assorted fixes revolving around rebase and merges
I noticed a couple of weeks ago that I had bogus merge commits after my rebases, where the original commits had been regular commits. This set me out on the adventure that is reflected in this patch series. Of course, the thing I wanted to fix is demonstrated by 1/5 and fixed in 2/5. But while at it, I ran into other issues and fixed them since I was at it anyway. Johannes Schindelin (5): rebase -r: demonstrate bug with conflicting merges rebase -r: do not write MERGE_HEAD unless needed rebase -i: include MERGE_HEAD into files to clean up built-in rebase --skip/--abort: clean up stale .git/ files status: rebase and merge can be in progress at the same time builtin/rebase.c | 3 +++ sequencer.c | 10 ++ t/t3430-rebase-merges.sh | 16 wt-status.c | 9 +++-- 4 files changed, 32 insertions(+), 6 deletions(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-75%2Fdscho%2Frebase-r-and-merge-head-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-75/dscho/rebase-r-and-merge-head-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/75 -- gitgitgadget
[PATCH 1/1] apply --recount: allow "no-op hunks"
From: Johannes Schindelin When editing patches e.g. in `git add -e`, it is quite common that a hunk ends up having no -/+ lines, i.e. it is now supposed to do nothing. This use case was broken by ad6e8ed37bc1 (apply: reject a hunk that does not do anything, 2015-06-01) with the good intention of catching a very real, different issue in hand-edited patches. So let's use the `--recount` option as the tell-tale whether the user would actually be okay with no-op hunks. Add a test case to make sure that this use case does not regress again. Signed-off-by: Johannes Schindelin --- apply.c| 2 +- t/t4136-apply-check.sh | 12 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/apply.c b/apply.c index 073d5f0451..76955afb00 100644 --- a/apply.c +++ b/apply.c @@ -1748,7 +1748,7 @@ static int parse_fragment(struct apply_state *state, } if (oldlines || newlines) return -1; - if (!deleted && !added) + if (!patch->recount && !deleted && !added) return -1; fragment->leading = leading; diff --git a/t/t4136-apply-check.sh b/t/t4136-apply-check.sh index 6d92872318..4c3f264a63 100755 --- a/t/t4136-apply-check.sh +++ b/t/t4136-apply-check.sh @@ -29,6 +29,18 @@ test_expect_success 'apply exits non-zero with no-op patch' ' test_must_fail git apply --check input ' +test_expect_success '`apply --recount` allows no-op patch' ' + echo 1 >1 && + git apply --recount --check <<-\EOF + diff --get a/1 b/1 + index 6696ea4..606eddd 100644 + --- a/1 + +++ b/1 + @@ -1,1 +1,1 @@ +1 + EOF +' + test_expect_success 'invalid combination: create and copy' ' test_must_fail git apply --check - <<-\EOF diff --git a/1 b/2 -- gitgitgadget
[PATCH 0/1] Allow "no-op hunks" when editing the diff in git add -e
I use git add -e frequently. Often there are multiple hunks and I end up deleting the + lines and converting the - lines to context lines, as I like to stage massive changes in an incremental fashion (and commit those staged changes incrementally, too). Some time after I invented git add -e, this feature was broken, and I had to start identifying and deleting those hunks with no changes. I finally got around to find the regression, and to fix it. Here is the outcome of this effort. Johannes Schindelin (1): apply --recount: allow "no-op hunks" apply.c| 2 +- t/t4136-apply-check.sh | 12 2 files changed, 13 insertions(+), 1 deletion(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-74%2Fdscho%2Fapply-recount-empty-hunk-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-74/dscho/apply-recount-empty-hunk-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/74 -- gitgitgadget
Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper
Hi Ævar,
On Mon, 12 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> On Mon, Nov 12 2018, Johannes Schindelin wrote:
>
> > On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> >> Move a 37 line for-loop mess out of "install" and into a helper
> >> script. This started out fairly innocent but over the years has grown
> >> into a hard-to-maintain monster, and my recent ad874608d8 ("Makefile:
> >> optionally symlink libexec/git-core binaries to bin/git", 2018-03-13)
> >> certainly didn't help.
> >>
> >> The shell code is ported pretty much as-is (with getopts added), it'll
> >> be fixed & prettified in subsequent commits.
> >>
> >> Signed-off-by: Ævar Arnfjörð Bjarmason
> >> ---
> >> Makefile | 52
> >> install_programs | 89
> >> 2 files changed, 103 insertions(+), 38 deletions(-)
> >> create mode 100755 install_programs
> >>
> >> diff --git a/Makefile b/Makefile
> >> index bbfbb4292d..aa6ca1fa68 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -2808,44 +2808,20 @@ endif
> >>bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
> >>execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
> >>destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e
> >> 's|[^/][^/]*|..|g') && \
> >> - { test "$$bindir/" = "$$execdir/" || \
> >> -for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS));
> >> do \
> >> - $(RM) "$$execdir/$$p" && \
> >> - test -n "$(INSTALL_SYMLINKS)" && \
> >> - ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p"
> >> "$$execdir/$$p" || \
> >> - { test -z
> >> "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
> >> -ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
> >> -cp "$$bindir/$$p" "$$execdir/$$p" || exit; } \
> >> -done; \
> >> - } && \
> >> - for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
> >> - $(RM) "$$bindir/$$p" && \
> >> - test -n "$(INSTALL_SYMLINKS)" && \
> >> - ln -s "git$X" "$$bindir/$$p" || \
> >> - { test -z "$(NO_INSTALL_HARDLINKS)" && \
> >> -ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
> >> -ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
> >> -cp "$$bindir/git$X" "$$bindir/$$p" || exit; } \
> >> - done && \
> >> - for p in $(BUILT_INS); do \
> >> - $(RM) "$$execdir/$$p" && \
> >> - test -n "$(INSTALL_SYMLINKS)" && \
> >> - ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X"
> >> "$$execdir/$$p" || \
> >> - { test -z "$(NO_INSTALL_HARDLINKS)" && \
> >> -ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
> >> -ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
> >> -cp "$$execdir/git$X" "$$execdir/$$p" || exit; } \
> >> - done && \
> >> - remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
> >> - for p in $$remote_curl_aliases; do \
> >> - $(RM) "$$execdir/$$p" && \
> >> - test -n "$(INSTALL_SYMLINKS)" && \
> >> - ln -s "git-remote-http$X" "$$execdir/$$p" || \
> >> - { test -z "$(NO_INSTALL_HARDLINKS)" && \
> >> -ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null
> >> || \
> >> -ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
> >> -cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \
> >> - done && \
> >
> > This indeed looks like a mess...
> >
> >> + ./install_programs \
> >> + --X="$$X" \
> >> +
Re: [PATCH v2 2/2] rebase: Implement --merge via git-rebase--interactive
Hi Elijah, On Wed, 7 Nov 2018, Elijah Newren wrote: > Interactive rebases are implemented in terms of cherry-pick rather than > the merge-recursive builtin, but cherry-pick also calls into the recursive > merge machinery by default and can accept special merge strategies and/or > special strategy options. As such, there really is not any need for > having both git-rebase--merge and git-rebase--interactive anymore. > > Delete git-rebase--merge.sh and have the --merge option be implemented > by the now built-in interactive machinery. Okay. > Note that this change fixes a few known test failures (see t3421). Nice! > testcase modification notes: > t3406: --interactive and --merge had slightly different progress output > while running; adjust a test to match > t3420: these test precise output while running, but rebase--am, > rebase--merge, and rebase--interactive all were built on very > different commands (am, merge-recursive, cherry-pick), so the > tests expected different output for each type. Now we expect > --merge and --interactive to have the same output. > t3421: --interactive fixes some bugs in --merge! Wahoo! > 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. > 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? > 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.) > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 3407d835bd..35084f5681 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -504,15 +504,7 @@ See also INCOMPATIBLE OPTIONS below. > INCOMPATIBLE OPTIONS > > > -git-rebase has many flags that are incompatible with each other, > -predominantly due to the fact that it has three different underlying > -implementations: > - > - * one based on linkgit:git-am[1] (the default) > - * one based on git-merge-recursive (merge backend) > - * one based on linkgit:git-cherry-pick[1] (interactive backend) > - Could we retain this part, with `s/three/two/` and `/git-merge/d`? *Maybe* `s/interactive backend/interactive\/merge backend/` > -Flags only understood by the am backend: > +The following options: > > * --committer-date-is-author-date > * --ignore-date > @@ -520,15 +512,12 @@ Flags only understood by the am backend: > * --ignore-whitespace > * -C > > -Flags understood by both merge and interactive backends: > +are incompatible with the following options: > > * --merge > * --strategy > * --strategy-option > * --allow-empty-message > - > -Flags only understood by the interactive backend: > - > * --[no-]autosquash > * --rebase-merges > * --preserve-merges > @@ -539,7 +528,7 @@ Flags only understood by the interactive backend: > * --edit-todo > * --root when used in combination with --onto > > -Other incompatible flag pairs: > +In addition, the following pairs of options are incompatible: > > * --preserve-merges and --interactive > * --preserve-merges and --signoff The rest of the diff is funny to read, but the post image makes a lot of sense. > diff --git a/builtin/rebase.c b/builtin/rebase.c > index be004406a6..d55bbb9eb9 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -118,7 +118,7 @@ static void imply_interactive(struct rebase_options > *opts, const char *option) > case REBASE_PRESERVE_MERGES: > break; > case REBASE_MERGE: > - /* we silently *upgrade* --merge to --interactive if needed */ > +
Re: [PATCH v2 1/2] git-rebase, sequencer: extend --quiet option for the interactive machinery
Hi Elijah,
On Wed, 7 Nov 2018, Elijah Newren wrote:
> While 'quiet' and 'interactive' may sound like antonyms, the interactive
> machinery actually has logic that implements several
> interactive_rebase=implied cases (--exec, --keep-empty, --rebase-merges)
> which won't pop up an editor. The rewrite of interactive rebase in C
> added a quiet option, though it only turns stats off. Since we want to
> make the interactive machinery also take over for git-rebase--merge, it
> should fully implement the --quiet option.
>
> git-rebase--interactive was already somewhat quieter than
> git-rebase--merge and git-rebase--am, possibly because cherry-pick has
> just traditionally been quieter. As such, we only drop a few
> informational messages -- "Rebasing (n/m)" and "Succesfully rebased..."
Makes sense.
> Also, for simplicity, remove the differences in how quiet and verbose
> options were recorded. Having one be signalled by the presence of a
> "verbose" file in the state_dir, while the other was signalled by the
> contents of a "quiet" file was just weirdly inconsistent. (This
> inconsistency pre-dated the rewrite into C.) Make them consistent by
> having them both key off the presence of the file.
I am slightly concerned that some creative power user could have written
scripts that rely on this behavior.
But only *slightly* concerned.
The patch looks correct.
Ciao,
Dscho
>
> Signed-off-by: Elijah Newren
> ---
> builtin/rebase.c | 5 +
> git-legacy-rebase.sh | 2 +-
> git-rebase--common.sh | 2 +-
> sequencer.c | 23 +--
> sequencer.h | 1 +
> 5 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 0ee06aa363..be004406a6 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -181,10 +181,7 @@ static int read_basic_state(struct rebase_options *opts)
> if (get_oid(buf.buf, >orig_head))
> return error(_("invalid orig-head: '%s'"), buf.buf);
>
> - strbuf_reset();
> - if (read_one(state_dir_path("quiet", opts), ))
> - return -1;
> - if (buf.len)
> + if (file_exists(state_dir_path("quiet", opts)))
> opts->flags &= ~REBASE_NO_QUIET;
> else
> opts->flags |= REBASE_NO_QUIET;
> diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
> index 75a08b2683..da27bfca5a 100755
> --- a/git-legacy-rebase.sh
> +++ b/git-legacy-rebase.sh
> @@ -113,7 +113,7 @@ read_basic_state () {
> else
> orig_head=$(cat "$state_dir"/head)
> fi &&
> - GIT_QUIET=$(cat "$state_dir"/quiet) &&
> + test -f "$state_dir"/quiet && GIT_QUIET=t
> test -f "$state_dir"/verbose && verbose=t
> test -f "$state_dir"/strategy && strategy="$(cat "$state_dir"/strategy)"
> test -f "$state_dir"/strategy_opts &&
> diff --git a/git-rebase--common.sh b/git-rebase--common.sh
> index 7e39d22871..dc18c682fa 100644
> --- a/git-rebase--common.sh
> +++ b/git-rebase--common.sh
> @@ -10,7 +10,7 @@ write_basic_state () {
> echo "$head_name" > "$state_dir"/head-name &&
> echo "$onto" > "$state_dir"/onto &&
> echo "$orig_head" > "$state_dir"/orig-head &&
> - echo "$GIT_QUIET" > "$state_dir"/quiet &&
> + test t = "$GIT_QUIET" && : > "$state_dir"/quiet
> test t = "$verbose" && : > "$state_dir"/verbose
> test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
> test -n "$strategy_opts" && echo "$strategy_opts" > \
> diff --git a/sequencer.c b/sequencer.c
> index 9e1ab3a2a7..bd8337dbf1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -150,6 +150,7 @@ static GIT_PATH_FUNC(rebase_path_refs_to_delete,
> "rebase-merge/refs-to-delete")
> static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
> static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
> static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
> +static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
> static GIT_PATH_FUNC(rebase_path_signoff, "rebase-merge/signoff")
> static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
> static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
> @@ -157,7 +158,6 @@ static GIT_PATH_FUNC(rebase_path_autostash,
> "rebase-merge/autostash")
> 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 int git_sequencer_config(const char *k, const char *v, void *cb)
> {
> @@ -2307,6 +2307,9 @@ static int read_populate_opts(struct replay_opts *opts)
> if (file_exists(rebase_path_verbose()))
> opts->verbose = 1;
>
> + if (file_exists(rebase_path_quiet()))
> +
Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long
Hi,
On Mon, 12 Nov 2018, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón writes:
>
> > b968372279 ("read-cache: unlink old sharedindex files", 2017-03-06)
> > introduced get_shared_index_expire_date using unsigned long to track
> > the modification times of a shared index.
> >
> > dddbad728c ("timestamp_t: a new data type for timestamps", 2017-04-26)
> > shows why that might problematic so move to time_t instead.
> >
> > Signed-off-by: Carlo Marcelo Arenas Belón
> > ---
> > read-cache.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/read-cache.c b/read-cache.c
> > index 7b1354d759..5525d8e679 100644
> > --- a/read-cache.c
> > +++ b/read-cache.c
> > @@ -2625,9 +2625,9 @@ static int write_split_index(struct index_state
> > *istate,
> >
> > static const char *shared_index_expire = "2.weeks.ago";
> >
> > -static unsigned long get_shared_index_expire_date(void)
> > +static time_t get_shared_index_expire_date(void)
> > {
> > - static unsigned long shared_index_expire_date;
> > + static time_t shared_index_expire_date;
> > static int shared_index_expire_date_prepared;
> >
> > if (!shared_index_expire_date_prepared) {
>
> After this line, the post-context reads like this:
>
> git_config_get_expiry("splitindex.sharedindexexpire",
> _index_expire);
> shared_index_expire_date = approxidate(shared_index_expire);
> shared_index_expire_date_prepared = 1;
> }
>
> return shared_index_expire_date;
>
> Given that the function returns the value obtained from
> approxidate(), which is approxidate_careful() in disguise, time_t is
> not as appropriate as timestamp_t, no?
>
> IOW, what if time_t were narrower than timestamp_t?
Rght. From the patch, I had assumed that the return type of
`approxidate()` is `time_t`, but it is `timestamp_t`.
Ciao,
Johannes
>
>
> > @@ -2643,7 +2643,7 @@ static unsigned long
> > get_shared_index_expire_date(void)
> > static int should_delete_shared_index(const char *shared_index_path)
> > {
> > struct stat st;
> > - unsigned long expiration;
> > + time_t expiration;
> >
> > /* Check timestamp */
> > expiration = get_shared_index_expire_date();
>
Re: [PATCH v2 0/3] Fix built-in rebase perf regression
Hi,
On Mon, 12 Nov 2018, Johannes Schindelin via GitGitGadget wrote:
> In our tests with large repositories, we noticed a serious regression of the
> performance of git rebase when using the built-in vs the shell script
> version. It boils down to an incorrect conversion of a git checkout -q:
> instead of using a twoway_merge as git checkout does, we used a oneway_merge
> as git reset does. The latter, however, calls lstat() on all files listed in
> the index, while the former essentially looks only at the files that are
> different between the given two revisions.
>
> Let's reinstate the original behavior by introducing a flag to the
> reset_head() function to indicate whether we want to emulate reset --hard
> (in which case we use the oneway_merge, otherwise we use twoway_merge).
>
> Johannes Schindelin (3):
> rebase: consolidate clean-up code before leaving reset_head()
> rebase: prepare reset_head() for more flags
> built-in rebase: reinstate `checkout -q` behavior where appropriate
>
> builtin/rebase.c | 79
> 1 file changed, 46 insertions(+), 33 deletions(-)
I forgot to specify the changes vs v1:
- More error paths are not consolidated via `goto leave_reset_head`.
- The `desc` array is not initialized to all-zero, to avoid bogus
addresses being passed to `free()`.
- The `detach_head` and `reset_hard` parameters have been consolidated
into a `flags` parameter.
- The `reset_head()` function once again only initializes `head_oid`
when needed.
Sorry for the omission,
Johannes
>
>
> base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
> Published-As:
> https://github.com/gitgitgadget/git/releases/tags/pr-72%2Fdscho%2Fbuiltin-rebase-perf-regression-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
> pr-72/dscho/builtin-rebase-perf-regression-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/72
>
> Range-diff vs v1:
>
> 1: 64597fe827 ! 1: 28e24d98ab rebase: consolidate clean-up code before
> leaving reset_head()
> @@ -11,6 +11,33 @@
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@
> +if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
> +BUG("Not a fully qualified branch: '%s'",
> switch_to_branch);
> +
> +- if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0)
> +- return -1;
> ++ if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0) {
> ++ ret = -1;
> ++ goto leave_reset_head;
> ++ }
> +
> +if (!oid) {
> +if (get_oid("HEAD", _oid)) {
> +- rollback_lock_file();
> +- return error(_("could not determine HEAD
> revision"));
> ++ ret = error(_("could not determine HEAD
> revision"));
> ++ goto leave_reset_head;
> +}
> +oid = _oid;
> +}
> +@@
> +unpack_tree_opts.reset = 1;
> +
> +if (read_index_unmerged(the_repository->index) < 0) {
> +- rollback_lock_file();
> +- return error(_("could not read index"));
> ++ ret = error(_("could not read index"));
> ++ goto leave_reset_head;
> }
>
> if (!fill_tree_descriptor(, oid)) {
> @@ -31,15 +58,17 @@
> }
>
> tree = parse_tree_indirect(oid);
> -@@
> +prime_cache_tree(the_repository->index, tree);
>
> -if (write_locked_index(the_repository->index, ,
> COMMIT_LOCK) < 0)
> +- if (write_locked_index(the_repository->index, ,
> COMMIT_LOCK) < 0)
> ++ if (write_locked_index(the_repository->index, ,
> COMMIT_LOCK) < 0) {
> ret = error(_("could not write index"));
> - free((void *)desc.buffer);
> -
> -if (ret)
> +-
> +- if (ret)
> - return ret;
> + goto leave_reset_head;
> ++ }
>
> reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
> strbuf_addf(, "%s: ", reflog_action ? reflog_action :
> "rebase");
> -: -- > 2: db963b2094 rebase: prepare reset_head() for more flags
> 2: 070092b430 ! 3: a7360b856f built-in rebase: reinstate `checkout -q`
> behav
Re: [RFC/PATCH 5/5] Makefile: Add a NO_INSTALL_BUILTIN_EXECDIR_ALIASES flag
Hi Ævar,
On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> Back when git was initially written the likes of "git-add", "git-init"
> etc. were installed in the user's $PATH. A few years later everything,
> with a few exceptions like git-upload-pack and git-receive-pack, was
> expected to be invoked as "git $cmd".
>
> Now something like a decade later we're still installing these old
> commands in gitexecdir. This is so someone with a shellscript that
> still targets e.g. "git-init" can add $(git --exec-path) to their
> $PATH and not have to change their script.
>
> Let's add an option to break this backwards compatibility. Now with
> NO_INSTALL_BUILTIN_EXECDIR_ALIASES=YesPlease there's only 3 programs
> in the bindir that are hardlinked to "git" (receive-pack,
> upload-archive & upload-pack), and 3 in the
> gitexecdir (git-remote-{ftp,ftps,https} linked to git-remote-http).
>
> There's no cross-directory links anymore, so the
> "NO_CROSS_DIRECTORY_HARDLINKS" flag becomes redundant under this new
> option.
>
> 1. https://public-inbox.org/git/87woyfdkoi@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
I like the idea.
With my suggested refactoring that avoids the non-DRY code, this patch
would also become much simpler (as would 2/5 -- 4/5).
However, I would not call these "aliases". That's just confusing. Maybe
NO_INSTALL_DASHED_BUILTINS would be better? It certainly would not have
confused me.
Ciao,
Dscho
> Makefile | 8
> install_programs | 36 +---
> 2 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 07c8b74353..a849a7b6d1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -346,6 +346,13 @@ all::
> # INSTALL_SYMLINKS if you'd prefer not to have the install procedure
> # fallack on hardlinking or copying if "ln -s" fails.
> #
> +# Define NO_INSTALL_BUILTIN_EXECDIR_ALIASES if you'd like to skip
> +# installing legacy such as "git-init" and "git-add" in the
> +# gitexecdir. Unless you're on a system where "which git-init" is
> +# expected to returns something set this. Users have been expected to
> +# use the likes of "git init" for ages now, these programs were only
> +# provided for legacy compatibility.
> +#
> # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
> # programs as a tar, where bin/ and libexec/ might be on different file
> systems.
> #
> @@ -2823,6 +2830,7 @@ endif
> --flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
>
> --flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
>
> --flag-no-install-symlinks-fallback="$(NO_INSTALL_SYMLINKS_FALLBACK)" \
> +
> --flag-no-install-builtin-execdir-aliases="$(NO_INSTALL_BUILTIN_EXECDIR_ALIASES)"
> \
> --list-bindir-standalone="git$X $(filter
> $(install_bindir_programs),$(ALL_PROGRAMS))" \
> --list-bindir-git-dashed="$(filter
> $(install_bindir_programs),$(BUILT_INS))" \
> --list-execdir-git-dashed="$(BUILT_INS)" \
> diff --git a/install_programs b/install_programs
> index 51e08019dd..8d89cd9984 100755
> --- a/install_programs
> +++ b/install_programs
> @@ -33,6 +33,9 @@ do
> --flag-no-install-symlinks-fallback=*)
>
> NO_INSTALL_SYMLINKS_FALLBACK="${1#--flag-no-install-symlinks-fallback=}"
> ;;
> + --flag-no-install-builtin-execdir-aliases=*)
> +
> NO_INSTALL_BUILTIN_EXECDIR_ALIASES="${1#--flag-no-install-builtin-execdir-aliases=}"
> + ;;
> --list-bindir-standalone=*)
> list_bindir_standalone="${1#--list-bindir-standalone=}"
> ;;
> @@ -54,7 +57,7 @@ do
> shift
> done &&
>
> -if test "$bindir/" != "$execdir/"
> +if test "$bindir/" != "$execdir/" -a -z "$NO_INSTALL_BUILTIN_EXECDIR_ALIASES"
> then
> for p in $list_bindir_standalone; do
> $RM "$execdir/$p" &&
> @@ -87,20 +90,23 @@ do
> fi
> done &&
>
> -for p in $list_execdir_git_dashed; do
> - $RM "$execdir/$p" &&
> - if test -n "$INSTALL_SYMLINKS" -a -n "$NO_INSTALL_SYMLINKS_FALLBACK"
> - then
> - ln -s "$destdir_from_execdir/$bindir_relative/git$X"
> "$execdir/$p"
> - else
> - test -n "$INSTALL_SYMLINKS" &&
> - ln -s "$destdir_from_execdir/$bindir_relative/git$X"
> "$execdir/$p" ||
> - { test -z "$NO_INSTALL_HARDLINKS" &&
> - ln "$execdir/git$X" "$execdir/$p" ||
> - ln -s "git$X" "$execdir/$p" ||
> - cp "$execdir/git$X" "$execdir/$p" || exit; }
> - fi
> -done &&
> +if test -z "$NO_INSTALL_BUILTIN_EXECDIR_ALIASES"
> +then
> + for p in $list_execdir_git_dashed; do
> + $RM "$execdir/$p" &&
> + if test -n "$INSTALL_SYMLINKS" -a -n
> "$NO_INSTALL_SYMLINKS_FALLBACK"
> + then
> + ln -s
Re: [RFC/PATCH 1/5] Makefile: move long inline shell loops in "install" into helper
Hi,
On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> Move a 37 line for-loop mess out of "install" and into a helper
> script. This started out fairly innocent but over the years has grown
> into a hard-to-maintain monster, and my recent ad874608d8 ("Makefile:
> optionally symlink libexec/git-core binaries to bin/git", 2018-03-13)
> certainly didn't help.
>
> The shell code is ported pretty much as-is (with getopts added), it'll
> be fixed & prettified in subsequent commits.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> Makefile | 52
> install_programs | 89
> 2 files changed, 103 insertions(+), 38 deletions(-)
> create mode 100755 install_programs
>
> diff --git a/Makefile b/Makefile
> index bbfbb4292d..aa6ca1fa68 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2808,44 +2808,20 @@ endif
> bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
> execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
> destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e
> 's|[^/][^/]*|..|g') && \
> - { test "$$bindir/" = "$$execdir/" || \
> - for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS));
> do \
> - $(RM) "$$execdir/$$p" && \
> - test -n "$(INSTALL_SYMLINKS)" && \
> - ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p"
> "$$execdir/$$p" || \
> - { test -z
> "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
> - ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
> - cp "$$bindir/$$p" "$$execdir/$$p" || exit; } \
> - done; \
> - } && \
> - for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
> - $(RM) "$$bindir/$$p" && \
> - test -n "$(INSTALL_SYMLINKS)" && \
> - ln -s "git$X" "$$bindir/$$p" || \
> - { test -z "$(NO_INSTALL_HARDLINKS)" && \
> - ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
> - ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
> - cp "$$bindir/git$X" "$$bindir/$$p" || exit; } \
> - done && \
> - for p in $(BUILT_INS); do \
> - $(RM) "$$execdir/$$p" && \
> - test -n "$(INSTALL_SYMLINKS)" && \
> - ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/git$X"
> "$$execdir/$$p" || \
> - { test -z "$(NO_INSTALL_HARDLINKS)" && \
> - ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
> - ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
> - cp "$$execdir/git$X" "$$execdir/$$p" || exit; } \
> - done && \
> - remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
> - for p in $$remote_curl_aliases; do \
> - $(RM) "$$execdir/$$p" && \
> - test -n "$(INSTALL_SYMLINKS)" && \
> - ln -s "git-remote-http$X" "$$execdir/$$p" || \
> - { test -z "$(NO_INSTALL_HARDLINKS)" && \
> - ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null
> || \
> - ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
> - cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; } \
> - done && \
This indeed looks like a mess...
> + ./install_programs \
> + --X="$$X" \
> + --RM="$(RM)" \
> + --bindir="$$bindir" \
> + --bindir-relative="$(bindir_relative_SQ)" \
> + --execdir="$$execdir" \
> + --destdir-from-execdir="$$destdir_from_execdir_SQ" \
> + --flag-install-symlinks="$(INSTALL_SYMLINKS)" \
> + --flag-no-install-hardlinks="$(NO_INSTALL_HARDLINKS)" \
> +
> --flag-no-cross-directory-hardlinks="$(NO_CROSS_DIRECTORY_HARDLINKS)" \
> + --list-bindir-standalone="git$X $(filter
> $(install_bindir_programs),$(ALL_PROGRAMS))" \
> + --list-bindir-git-dashed="$(filter
> $(install_bindir_programs),$(BUILT_INS))" \
> + --list-execdir-git-dashed="$(BUILT_INS)" \
> + --list-execdir-curl-aliases="$(REMOTE_CURL_ALIASES)" && \
> ./check_bindir "z$$bindir" "z$$execdir" "$$bindir/git-add$X"
>
> .PHONY: install-gitweb install-doc install-man install-man-perl install-html
> install-info install-pdf
> diff --git a/install_programs b/install_programs
> new file mode 100755
> index 00..e287108112
> --- /dev/null
> +++ b/install_programs
> @@ -0,0 +1,89 @@
> +#!/bin/sh
> +
> +while test $# != 0
> +do
> + case "$1" in
> + --X=*)
> + X="${1#--X=}"
> + ;;
> + --RM=*)
> + RM="${1#--RM=}"
> + ;;
> + --bindir=*)
> + bindir="${1#--bindir=}"
> + ;;
> + --bindir-relative=*)
> + bindir_relative="${1#--bindir-relative=}"
> + ;;
> + --execdir=*)
> + execdir="${1#--execdir=}"
> +
[PATCH 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 in that case, but that is not what we test for. So let's skip the test for now when we know that we want to test an installed Git. 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 832ede5099..1ea20dc2dc 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -51,7 +51,7 @@ export LSAN_OPTIONS # It appears that people try to run tests without building... -"$GIT_BUILD_DIR/git" >/dev/null +test -n "$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.' -- gitgitgadget
[PATCH 5/5] tests: explicitly use `git.exe` on Windows
From: Johannes Schindelin
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 bbfbb4292d..5df0118ce9 100644
--- a/Makefile
+++ b/Makefile
@@ -2590,6 +2590,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 801cc9b2ef..c167b2e1af 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -900,7 +900,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 1ea20dc2dc..3e2a9ce76d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -49,18 +49,23 @@ 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...
-test -n "$GIT_TEST_INSTALLED" || "$GIT_BUILD_DIR/git" >/dev/null ||
+test -n "$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.'
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 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 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 78d8c3783b..801cc9b2ef 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -900,7 +900,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 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 47a99aa0ed..832ede5099 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 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. 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 | 15 ++- 4 files changed, 19 insertions(+), 7 deletions(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-73%2Fdscho%2Ftest-git-installed-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-73/dscho/test-git-installed-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/73 -- gitgitgadget
Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"
Hi Ævar, On Fri, 2 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > * GIT_TEST_INSTALLED breaks entirely under this, as early as the >heuristic for "are we built?" being "do we have git-init in >libexecdir?". I tried a bit to make this work, but there's a lot of >dependencies there. I have a couple of patches in the pipeline to improve `GIT_TEST_INSTALLED`, as I needed it to work without hardlinked copies of the built-ins. These patches might help this here isue. > * We still (and this is also true of my ad874608d8) hardlink >everything in the build dir via a different part of the Makefile, >ideally we should do exactly the same thing there so also normal >tests and not just GIT_TEST_INSTALLED (if that worked) would test >in the same mode. > >I gave making that work a bit of a try and gave up in the Makefile >jungle. Yep. Ciao, Dscho
Re: [PATCH 0/2] avoid unsigned long for timestamps
Hi Carlo, On Mon, 12 Nov 2018, Carlo Marcelo Arenas Belón wrote: > specially problematic in Windows where unsigned long is only 32bit wide > and therefore the assumption that a time_t would fit will lead to loss > of precision in a 64bit OS. Both patches look correct to me. Thanks! Dscho > > builtin/commit.c | 4 ++-- > read-cache.c | 6 +++--- > 2 files changed, 5 insertions(+), 5 deletions(-) > > >
Re: [RFC PATCH 1/5] ref-filter: add objectsize:disk option
Hi,
On Mon, 12 Nov 2018, Junio C Hamano wrote:
> Olga Telezhnaya writes:
>
> > @@ -876,11 +882,13 @@ static void grab_common_values(struct atom_value
> > *val, int deref, struct expand_
> > name++;
> > if (!strcmp(name, "objecttype"))
> > v->s = xstrdup(type_name(oi->type));
> > - else if (!strcmp(name, "objectsize")) {
> > + else if (!strcmp(name, "objectsize:disk")) {
> > + v->value = oi->disk_size;
> > + v->s = xstrfmt("%lld", (long long)oi->disk_size);
>
> oi.disk_size is off_t; do we know "long long"
>
>(1) is available widely enough (I think it is from c99)?
>
>(2) is sufficiently wide so that we can safely cast off_t to?
>
>(3) will stay to be sufficiently wide as machines get larger
>together with standard types like off_t in the future?
>
> I'd rather use intmax_t (as off_t is a signed integral type) so that
> we do not have to worry about these questions in the first place.
You mean something like
v->s = xstrfmt("%"PRIdMAX, (intmax_t)oi->disk_size);
If so, I agree.
Ciao,
Dscho
P.S.: I wondered whether we have precedent for PRIdMAX, as we used to use
only PRIuMAX, but yes: JeffH's json-writer uses PRIdMAX.
>
> > + } else if (!strcmp(name, "objectsize")) {
> > v->value = oi->size;
> > v->s = xstrfmt("%lu", oi->size);
>
> This is not a suggestion but is a genuine question, but I wonder if
> two years down the road somebody who meets this API for the first
> time find the asymmetry between "objectsize" and "objectsize:disk" a
> tad strange and suggest the former to have "objectsize:real" or some
> synonym. Or we can consider "objectsize" the primary thing (hence
> needing no colon-plus-modifier to clarify what kind of size we are
> asking) and leave these two deliberatly asymmetric. I am leaning
> towards the latter myself.
>
> > - }
> > - else if (deref)
> > + } else if (deref)
> > grab_objectname(name, >oid, v, _atom[i]);
> > }
> > }
> >
> > --
> > https://github.com/git/git/pull/552
>
[PATCH v2 2/3] rebase: prepare reset_head() for more flags
From: Johannes Schindelin
Currently, we only accept the flag indicating whether the HEAD should be
detached not. In the next commit, we want to introduce another flag: to
toggle between emulating `reset --hard` vs `checkout -q`.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index e173654d56..074594cf10 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -522,10 +522,13 @@ finished_rebase:
#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
+#define RESET_HEAD_DETACH (1<<0)
+
static int reset_head(struct object_id *oid, const char *action,
- const char *switch_to_branch, int detach_head,
+ const char *switch_to_branch, unsigned flags,
const char *reflog_orig_head, const char *reflog_head)
{
+ unsigned detach_head = flags & RESET_HEAD_DETACH;
struct object_id head_oid;
struct tree_desc desc;
struct lock_file lock = LOCK_INIT;
@@ -1500,8 +1503,8 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
"it...\n"));
strbuf_addf(, "rebase: checkout %s", options.onto_name);
- if (reset_head(>object.oid, "checkout", NULL, 1,
- NULL, msg.buf))
+ if (reset_head(>object.oid, "checkout", NULL,
+ RESET_HEAD_DETACH, NULL, msg.buf))
die(_("Could not detach HEAD"));
strbuf_release();
--
gitgitgadget
[PATCH v2 0/3] Fix built-in rebase perf regression
In our tests with large repositories, we noticed a serious regression of the
performance of git rebase when using the built-in vs the shell script
version. It boils down to an incorrect conversion of a git checkout -q:
instead of using a twoway_merge as git checkout does, we used a oneway_merge
as git reset does. The latter, however, calls lstat() on all files listed in
the index, while the former essentially looks only at the files that are
different between the given two revisions.
Let's reinstate the original behavior by introducing a flag to the
reset_head() function to indicate whether we want to emulate reset --hard
(in which case we use the oneway_merge, otherwise we use twoway_merge).
Johannes Schindelin (3):
rebase: consolidate clean-up code before leaving reset_head()
rebase: prepare reset_head() for more flags
built-in rebase: reinstate `checkout -q` behavior where appropriate
builtin/rebase.c | 79
1 file changed, 46 insertions(+), 33 deletions(-)
base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-72%2Fdscho%2Fbuiltin-rebase-perf-regression-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-72/dscho/builtin-rebase-perf-regression-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/72
Range-diff vs v1:
1: 64597fe827 ! 1: 28e24d98ab rebase: consolidate clean-up code before
leaving reset_head()
@@ -11,6 +11,33 @@
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@
+ if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
+ BUG("Not a fully qualified branch: '%s'", switch_to_branch);
+
+- if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0)
+- return -1;
++ if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0) {
++ ret = -1;
++ goto leave_reset_head;
++ }
+
+ if (!oid) {
+ if (get_oid("HEAD", _oid)) {
+- rollback_lock_file();
+- return error(_("could not determine HEAD revision"));
++ ret = error(_("could not determine HEAD revision"));
++ goto leave_reset_head;
+ }
+ oid = _oid;
+ }
+@@
+ unpack_tree_opts.reset = 1;
+
+ if (read_index_unmerged(the_repository->index) < 0) {
+- rollback_lock_file();
+- return error(_("could not read index"));
++ ret = error(_("could not read index"));
++ goto leave_reset_head;
}
if (!fill_tree_descriptor(, oid)) {
@@ -31,15 +58,17 @@
}
tree = parse_tree_indirect(oid);
-@@
+ prime_cache_tree(the_repository->index, tree);
- if (write_locked_index(the_repository->index, , COMMIT_LOCK) < 0)
+- if (write_locked_index(the_repository->index, , COMMIT_LOCK) < 0)
++ if (write_locked_index(the_repository->index, , COMMIT_LOCK) < 0) {
ret = error(_("could not write index"));
- free((void *)desc.buffer);
-
- if (ret)
+-
+- if (ret)
- return ret;
+ goto leave_reset_head;
++ }
reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
strbuf_addf(, "%s: ", reflog_action ? reflog_action : "rebase");
-: -- > 2: db963b2094 rebase: prepare reset_head() for more flags
2: 070092b430 ! 3: a7360b856f built-in rebase: reinstate `checkout -q`
behavior where appropriate
@@ -20,15 +20,18 @@
@@
#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
+ #define RESET_HEAD_DETACH (1<<0)
++#define RESET_HEAD_HARD (1<<1)
+
static int reset_head(struct object_id *oid, const char *action,
-- const char *switch_to_branch, int detach_head,
-+ const char *switch_to_branch,
-+ int detach_head, int reset_hard,
+const char *switch_to_branch, unsigned flags,
const char *reflog_orig_head, const char *reflog_head)
{
+ unsigned detach_head = flags & RESET_HEAD_DETACH;
++ unsigned reset_hard = flags & RESET_HEAD_HARD;
struct object_id head_oid;
- struct tree_desc desc;
-+ struct tree_desc desc[2];
++ struct tree_desc desc[2] = { { NULL }, { NULL } };
struct lock_file lock = LOCK_INIT;
struct unpack_trees_options unpack_tree_opts;
struct tree *tree;
@@ -42,18 +45,18 @@
if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
BUG("Not a fully qualified branch: '%s'", switch_to_branch
[PATCH v2 1/3] rebase: consolidate clean-up code before leaving reset_head()
From: Johannes Schindelin
The same clean-up code is repeated quite a few times; Let's DRY up the
code some.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..e173654d56 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -541,13 +541,15 @@ static int reset_head(struct object_id *oid, const char
*action,
if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
BUG("Not a fully qualified branch: '%s'", switch_to_branch);
- if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0)
- return -1;
+ if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0) {
+ ret = -1;
+ goto leave_reset_head;
+ }
if (!oid) {
if (get_oid("HEAD", _oid)) {
- rollback_lock_file();
- return error(_("could not determine HEAD revision"));
+ ret = error(_("could not determine HEAD revision"));
+ goto leave_reset_head;
}
oid = _oid;
}
@@ -564,32 +566,27 @@ static int reset_head(struct object_id *oid, const char
*action,
unpack_tree_opts.reset = 1;
if (read_index_unmerged(the_repository->index) < 0) {
- rollback_lock_file();
- return error(_("could not read index"));
+ ret = error(_("could not read index"));
+ goto leave_reset_head;
}
if (!fill_tree_descriptor(, oid)) {
- error(_("failed to find tree of %s"), oid_to_hex(oid));
- rollback_lock_file();
- free((void *)desc.buffer);
- return -1;
+ ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
+ goto leave_reset_head;
}
if (unpack_trees(1, , _tree_opts)) {
- rollback_lock_file();
- free((void *)desc.buffer);
- return -1;
+ ret = -1;
+ goto leave_reset_head;
}
tree = parse_tree_indirect(oid);
prime_cache_tree(the_repository->index, tree);
- if (write_locked_index(the_repository->index, , COMMIT_LOCK) < 0)
+ if (write_locked_index(the_repository->index, , COMMIT_LOCK) < 0) {
ret = error(_("could not write index"));
- free((void *)desc.buffer);
-
- if (ret)
- return ret;
+ goto leave_reset_head;
+ }
reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
strbuf_addf(, "%s: ", reflog_action ? reflog_action : "rebase");
@@ -622,7 +619,10 @@ static int reset_head(struct object_id *oid, const char
*action,
UPDATE_REFS_MSG_ON_ERR);
}
+leave_reset_head:
strbuf_release();
+ rollback_lock_file();
+ free((void *)desc.buffer);
return ret;
}
--
gitgitgadget
[PATCH v2 3/3] built-in rebase: reinstate `checkout -q` behavior where appropriate
From: Johannes Schindelin
When we converted a `git checkout -q $onto^0` call to use
`reset_head()`, we inadvertently incurred a change from a twoway_merge
to a oneway_merge, as if we wanted a `git reset --hard` instead.
This has performance ramifications under certain, though, as the
oneway_merge needs to lstat() every single index entry whereas
twoway_merge does not.
So let's go back to the old behavior.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 40 +---
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 074594cf10..dc78c1497d 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -523,14 +523,16 @@ finished_rebase:
#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
#define RESET_HEAD_DETACH (1<<0)
+#define RESET_HEAD_HARD (1<<1)
static int reset_head(struct object_id *oid, const char *action,
const char *switch_to_branch, unsigned flags,
const char *reflog_orig_head, const char *reflog_head)
{
unsigned detach_head = flags & RESET_HEAD_DETACH;
+ unsigned reset_hard = flags & RESET_HEAD_HARD;
struct object_id head_oid;
- struct tree_desc desc;
+ struct tree_desc desc[2] = { { NULL }, { NULL } };
struct lock_file lock = LOCK_INIT;
struct unpack_trees_options unpack_tree_opts;
struct tree *tree;
@@ -539,7 +541,7 @@ static int reset_head(struct object_id *oid, const char
*action,
size_t prefix_len;
struct object_id *orig = NULL, oid_orig,
*old_orig = NULL, oid_old_orig;
- int ret = 0;
+ int ret = 0, nr = 0;
if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
BUG("Not a fully qualified branch: '%s'", switch_to_branch);
@@ -549,20 +551,20 @@ static int reset_head(struct object_id *oid, const char
*action,
goto leave_reset_head;
}
- if (!oid) {
- if (get_oid("HEAD", _oid)) {
- ret = error(_("could not determine HEAD revision"));
- goto leave_reset_head;
- }
- oid = _oid;
+ if ((!oid || !reset_hard) && get_oid("HEAD", _oid)) {
+ ret = error(_("could not determine HEAD revision"));
+ goto leave_reset_head;
}
+ if (!oid)
+ oid = _oid;
+
memset(_tree_opts, 0, sizeof(unpack_tree_opts));
setup_unpack_trees_porcelain(_tree_opts, action);
unpack_tree_opts.head_idx = 1;
unpack_tree_opts.src_index = the_repository->index;
unpack_tree_opts.dst_index = the_repository->index;
- unpack_tree_opts.fn = oneway_merge;
+ unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
unpack_tree_opts.update = 1;
unpack_tree_opts.merge = 1;
if (!detach_head)
@@ -573,12 +575,17 @@ static int reset_head(struct object_id *oid, const char
*action,
goto leave_reset_head;
}
- if (!fill_tree_descriptor(, oid)) {
+ if (!reset_hard && !fill_tree_descriptor([nr++], _oid)) {
+ ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
+ goto leave_reset_head;
+ }
+
+ if (!fill_tree_descriptor([nr++], oid)) {
ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
goto leave_reset_head;
}
- if (unpack_trees(1, , _tree_opts)) {
+ if (unpack_trees(nr, desc, _tree_opts)) {
ret = -1;
goto leave_reset_head;
}
@@ -625,7 +632,8 @@ static int reset_head(struct object_id *oid, const char
*action,
leave_reset_head:
strbuf_release();
rollback_lock_file();
- free((void *)desc.buffer);
+ while (nr)
+ free((void *)desc[--nr].buffer);
return ret;
}
@@ -1003,7 +1011,8 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
rerere_clear(_rr);
string_list_clear(_rr, 1);
- if (reset_head(NULL, "reset", NULL, 0, NULL, NULL) < 0)
+ if (reset_head(NULL, "reset", NULL, RESET_HEAD_HARD,
+ NULL, NULL) < 0)
die(_("could not discard worktree changes"));
if (read_basic_state())
exit(1);
@@ -1019,7 +1028,8 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
if (read_basic_state())
exit(1);
if (reset_head(_head, "reset",
- options.head_name, 0, NULL, NULL) < 0)
+ options.head_name, RES
Re: [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate
Hi Junio, On Mon, 12 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > >> > static int reset_head(struct object_id *oid, const char *action, > >> > - const char *switch_to_branch, int detach_head, > >> > + const char *switch_to_branch, > >> > + int detach_head, int reset_hard, > >> > >> It might be worth switching to a single flag variable here. It would > >> make calls like this: > >> > >> > -if (reset_head(>object.oid, "checkout", NULL, 1, > >> > +if (reset_head(>object.oid, "checkout", NULL, 1, > >> > 0, > >> > NULL, msg.buf)) > >> > >> a little more self-documenting (if a little more verbose). > > > > I thought about that, but for just two flags? Well, let me try it and see > > whether I like the result ;-) > > My rule of thumb is that repeating three times is definitely when we > should consolidate separate ones into a single flag word, and twice > is a borderline. > > For two-time repetition, it is often worth fixing when somebody > notices it during the review, as that is a sign that repetition, > even a minor one, disturbed a reader enough to point out. That's my thought exactly, hence I looked into it. The end result is actually easier to read, in particular the commit that re-introduces the `reset --hard` behavior: it no longer touches *all* callsites of `reset_head()` but only the relevant ones. > On the other hand, for a file-scope static that is likely to stay as a > non-API function but a local helper, it may not be worth it. Oh, do you think that `reset_head()` is likely to stay as non-API function? I found myself in the need of repeating this tedious unpack_trees() dance quite a few times over the past two years, and it is *always* daunting because the API is *that* unintuitive. So I *do* hope that `reset_head()` will actually be moved to reset.[ch] eventually, and callsites e.g. in `sequencer.c` will be converted from calling `unpack_trees()` to calling `reset_head()` instead. v2 on the way, Dscho > So I am OK either way, slightly in favor of fixing it while we > remember. > > > >> This one could actually turn into: > >> > >> ret = error(...); > >> goto leave_reset_head; > >> > >> now. We don't have to worry about an uninitialized desc.buffer anymore > >> (as I mentioned in the previous email), because "nr" would be 0. > >> > >> It doesn't save any lines, though (but maybe having a single > >> cleanup/exit point would make things easier to read; I dunno). > > > > But you're right, of course. Consistency makes for easier-to-read code. > > Yup, that does sound good. > > Thanks. >
Re: [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate
Hi Peff, On Fri, 9 Nov 2018, Jeff King wrote: > On Fri, Nov 09, 2018 at 06:21:41PM +0100, Johannes Schindelin wrote: > > > Actually, you got me thinking about the desc.buffer. And I think there is > > one corner case where it could cause a problem: `struct tree_desc desc[2]` > > does not initialize the buffers to NULL. And what if > > fill_tree_descriptor() function returns NULL? Then the buffer is still > > uninitialized. > > > > In practice, our *current* version of fill_tree_descriptor() never returns > > NULL if the oid parameter is non-NULL. It would die() in the error case > > instead (bad!). So to prepare for a less bad version, I'd rather > > initialize the `desc` array and be on the safe (and easier to reason > > about) side. > > Yeah, I agree with all of that. > > One solution would just be to increment only after success: > > if (fill_tree_descriptor([nr], ..) < 0) > goto error; > nr++; /* now we know it's valid! */ > > But there are lots of alternatives. :) True. I simply prefer to initialize it and be done with it. ;-) Ciao, Dscho
Re: [PATCH] p3400: replace calls to `git checkout -b' by `git checkout -B'
Hi Alban, On Fri, 9 Nov 2018, Alban Gruin wrote: > p3400 makes a copy of the current repository to test git-rebase > performance, and creates new branches in the copy with `git checkout > -b'. If the original repository has branches with the same name as the > script is trying to create, this operation will fail. > > This replaces these calls by `git checkout -B' to force the creation and > update of these branches. Both the explanation and the patch make sense to me. Thanks, Dscho > > Signed-off-by: Alban Gruin > --- > t/perf/p3400-rebase.sh | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/t/perf/p3400-rebase.sh b/t/perf/p3400-rebase.sh > index ce271ca4c1..d202aaed06 100755 > --- a/t/perf/p3400-rebase.sh > +++ b/t/perf/p3400-rebase.sh > @@ -6,9 +6,9 @@ test_description='Tests rebase performance' > test_perf_default_repo > > test_expect_success 'setup rebasing on top of a lot of changes' ' > - git checkout -f -b base && > - git checkout -b to-rebase && > - git checkout -b upstream && > + git checkout -f -B base && > + git checkout -B to-rebase && > + git checkout -B upstream && > for i in $(seq 100) > do > # simulate huge diffs > @@ -35,8 +35,8 @@ test_perf 'rebase on top of a lot of unrelated changes' ' > > test_expect_success 'setup rebasing many changes without split-index' ' > git config core.splitIndex false && > - git checkout -b upstream2 to-rebase && > - git checkout -b to-rebase2 upstream > + git checkout -B upstream2 to-rebase && > + git checkout -B to-rebase2 upstream > ' > > test_perf 'rebase a lot of unrelated changes without split-index' ' > -- > 2.19.1 > >
Re: [PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate
Hi Peff,
On Fri, 9 Nov 2018, Jeff King wrote:
> On Fri, Nov 09, 2018 at 01:34:19AM -0800, Johannes Schindelin via
> GitGitGadget wrote:
>
> > From: Johannes Schindelin
> >
> > When we converted a `git checkout -q $onto^0` call to use
> > `reset_head()`, we inadvertently incurred a change from a twoway_merge
> > to a oneway_merge, as if we wanted a `git reset --hard` instead.
> >
> > This has performance ramifications under certain, though, as the
> > oneway_merge needs to lstat() every single index entry whereas
> > twoway_merge does not.
> >
> > So let's go back to the old behavior.
>
> Makes sense. I didn't think too hard about any possible gotchas with the
> twoway/oneway switch, but if that's what git-checkout was doing before,
> it seems obviously safe.
Right.
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 6f6d7de156..c1cc50f3f8 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -523,11 +523,12 @@ finished_rebase:
> > #define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
> >
> > static int reset_head(struct object_id *oid, const char *action,
> > - const char *switch_to_branch, int detach_head,
> > + const char *switch_to_branch,
> > + int detach_head, int reset_hard,
>
> It might be worth switching to a single flag variable here. It would
> make calls like this:
>
> > - if (reset_head(>object.oid, "checkout", NULL, 1,
> > + if (reset_head(>object.oid, "checkout", NULL, 1, 0,
> > NULL, msg.buf))
>
> a little more self-documenting (if a little more verbose).
I thought about that, but for just two flags? Well, let me try it and see
whether I like the result ;-)
> > - if (!oid) {
> > - if (get_oid("HEAD", _oid)) {
> > - rollback_lock_file();
> > - return error(_("could not determine HEAD revision"));
> > - }
> > - oid = _oid;
> > + if (get_oid("HEAD", _oid)) {
> > + rollback_lock_file();
> > + return error(_("could not determine HEAD revision"));
> > }
>
> This one could actually turn into:
>
> ret = error(...);
> goto leave_reset_head;
>
> now. We don't have to worry about an uninitialized desc.buffer anymore
> (as I mentioned in the previous email), because "nr" would be 0.
>
> It doesn't save any lines, though (but maybe having a single
> cleanup/exit point would make things easier to read; I dunno).
But you're right, of course. Consistency makes for easier-to-read code.
> Take all my comments as observations, not objections. This looks OK to
> me either way.
Actually, you got me thinking about the desc.buffer. And I think there is
one corner case where it could cause a problem: `struct tree_desc desc[2]`
does not initialize the buffers to NULL. And what if
fill_tree_descriptor() function returns NULL? Then the buffer is still
uninitialized.
In practice, our *current* version of fill_tree_descriptor() never returns
NULL if the oid parameter is non-NULL. It would die() in the error case
instead (bad!). So to prepare for a less bad version, I'd rather
initialize the `desc` array and be on the safe (and easier to reason
about) side.
Thank you for your helpful review,
Dscho
Re: [PATCH 1/2] rebase: consolidate clean-up code before leaving reset_head()
Hi Peff,
On Fri, 9 Nov 2018, Jeff King wrote:
> On Fri, Nov 09, 2018 at 01:34:17AM -0800, Johannes Schindelin via
> GitGitGadget wrote:
>
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 0ee06aa363..6f6d7de156 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -569,16 +569,13 @@ static int reset_head(struct object_id *oid, const
> > char *action,
> > }
> >
> > if (!fill_tree_descriptor(, oid)) {
> > - error(_("failed to find tree of %s"), oid_to_hex(oid));
> > - rollback_lock_file();
> > - free((void *)desc.buffer);
> > - return -1;
> > + ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
> > + goto leave_reset_head;
> > }
>
> If fill_tree_descriptor() fails, what is left in desc.buffer? Looking at
> the implementation, I think it's always NULL or a valid buffer. But I
> think all code paths actually die() unless we pass a NULL oid (and in
> that case desc.buffer would be NULL, too).
>
> So I think the original here that calls free() doesn't ever do anything
> but it did not hurt. After your patch, the leave_reset_head code would
> continue to call free(), and that's OK.
Right, that was my thinking, too.
> There are a few earlier conditionals in reset_head() that do only
> rollback_lock_file() that could similarly be converted to use the goto.
> But they would need desc.buffer to be initialized to NULL. I could go
> either way on converting them or not.
Whoops. I should have checked more carefully?
> > @@ -586,10 +583,9 @@ static int reset_head(struct object_id *oid, const
> > char *action,
> >
> > if (write_locked_index(the_repository->index, , COMMIT_LOCK) < 0)
> > ret = error(_("could not write index"));
> > - free((void *)desc.buffer);
> >
> > if (ret)
> > - return ret;
> > + goto leave_reset_head;
> >
> > reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
> > strbuf_addf(, "%s: ", reflog_action ? reflog_action : "rebase");
> > @@ -622,7 +618,10 @@ static int reset_head(struct object_id *oid, const
> > char *action,
> > UPDATE_REFS_MSG_ON_ERR);
> > }
> >
> > +leave_reset_head:
> > strbuf_release();
> > + rollback_lock_file();
> > + free((void *)desc.buffer);
> > return ret;
>
> We get here on success, too. So we may call rollback_lock_file() on an
> already-committed lock. This is explicitly documented as a no-op by the
> lock code, so that's OK.
Indeed. I did not check the documentation, but the code, and came to the
same conclusion.
> So overall looks good to me.
Thanks!
Dscho
Re: [PATCH v4 0/3] range-diff fixes
Hi Ævar, On Fri, 9 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > Addresses feedback on v3, especially Eric's suggestion to split out > the behavior change (which I was not aware of) into a 3/3. For the record, I am fine with this iteration, too. Ciao, Dscho
Re: Git Evolve
Hi,
On Tue, 2 Oct 2018, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Oct 02 2018, Taylor Blau wrote:
>
> > Hi Stefan,
> >
> > On Sat, Sep 29, 2018 at 04:00:04PM -0700, Stefan Xenos wrote:
> >> Hello, List!
> >>
> >> I'm interested in porting something like Mercurial's evolve command to
> >> Git.
> >
> > Welcome to Git :-). I think that the discussion in this thread is good,
> > but it's not why I'm replying. I have also wanted a Mercurial feature in
> > Git, but a different one than yours.
> >
> > Specifically, I've wanted the 'hg absorb' command. My understanding of
> > the commands functionality is that it builds a sort of flamegraph-esque
> > view of the blame, and then cascades downwards parts of a change. I am
> > sure that I'm not doing the command justice, so I'll defer to [1] where
> > it is explained in more detail.
> >
> > The benefit of this command is that it gives you a way to--without
> > ambiguity--absorb changes into earlier commits, and in fact, the
> > earliest commit that they make sense to belong to.
> >
> > This would simplify my workflow greatly when re-rolling patches, as I
> > often want to rewrite a part of an earlier commit. This is certainly
> > possible by a number of different `git rebase` invocations (e.g., (1)
> > create fixup commits, and then re-order them, or (2) mark points in your
> > history as 'edit', and rewrite them in a detached state, and I'm sure
> > many more).
> >
> > I'm curious if you or anyone else has thought about how this might work
> > in Git.
>
> I've wanted a "git absorb" for a while, but have done no actual work on
> it, I just found out about it.
>
> I think a combination of these two heuristics would probably do the
> trick:
>
> 1. If a change in your "git diff" output has a hunk whose lines overlap
> with an earlier commit in the @{u}.. range, we do the equivalent of
> "git add -p", select that hunk, and "git commit --fixup commit>". We fixup the most recent commit that matches (otherwise
> commit>we'd conflict).
>
> 2. Have some mode where we fall back from #1 and consider changes to
> entire files, if that's unambiguous.
>
> The neat thing about this would be that you could tweak how promiscuous
> #1 would be via the -U option to git-diff, and #2 would just be a
> special case of -U9 (we should really add a -Uinf...).
>
> Then once you ran this you could run "git rebase -i --autosquash" to see
> how the TODO list would look, and optionally have some "git absorb
> --now" or whatever to do the "git add -p", "git commit --fixup" and "git
> rebase --autosquash" all in one go.
This is essentially what the script does that I sent to this here mailing
list back in May:
https://public-inbox.org/git/nycvar.qro.7.76.6.1805052220360...@tvgsbejvaqbjf.bet/
I used this quite a lot, but I still find it slow (especially on Windows),
so I am still in search for a better solution. And yes, I was intrigued by
`hg absorb` when it was presented at GitMerge last year, and wanted to
have the same.
In the meantime, what I often do is to call `git log -L :`
where and are taken from the hunk(s) of the current diff.
Actually, I have a script to do that, hidden behind a Git alias. Then I
inspect the diffs in that log and call `git commit --fixup` with the one I
deem most appropriate.
Note that it sometimes fails because of semantic dependencies. That is,
even if my current change overlaps with an earlier change, it may be too
early to be squashed in.
As an example: imagine that I moved a git_config() call from some function
into the cmd_() function. Only: I intended to move it, but in
fact, I just added the call to the latter function. Eventually, I figure
it out! So I want to make a fixup! commit. My script, as well as Ævar's
suggestion, as well as literally all the other attempts to solve this that
I am aware of, would try to squash this change into whichever commit
introduced the function that originally called git_config(). But that is
wrong. It should be squashed into the commit that added the git_config()
call to the cmd_() function.
Ciao,
Dscho
>
> > [1]: http://files.lihdd.net/hgabsorb-note.pdf
>
Re: GPG signing is bent on WIndows
Hi Jeff, On Wed, 26 Sep 2018, Jeffrey Walton wrote: > Several weeks ago I updated to the latest Git for Windows (when > prompted by the version check). At the time I noticed: > > $ git commit -S -am "Fix unset MAKE variable in test scripts" > gpg: signing failed: No pinentry > gpg: signing failed: No pinentry > error: gpg failed to sign the data > fatal: failed to write commit object > > I got to look at it today. On Windows: > > $ cat ~/.gnupg/gpg-agent.conf > pinentry-program > /usr/local/MacGPG2/libexec/pinentry-mac.app/Contents/MacOS/pinentry-mac Git for Windows is partially based off of MSYS2, which upgraded GPG from v1 to v2, and one of the consequences is that v2 handles interaction with the user differently. My guess is that you copied your config from a Mac, and the path is simply incorrect. I would wager a bet that it starts working when you remove that line with the incorrect path, as the default should work plenty fine for you. Ciao, Johannes
Re: [GSoC][PATCH v8 14/20] stash: convert create to builtin
Hi Paul, On Wed, 26 Sep 2018, Paul-Sebastian Ungureanu wrote: > Sorry for the late reply. I had a lot on my plate for the last couple of > weeks. I understand. University can be busy times. > > > + > > > + git_config(git_diff_basic_config, NULL); > > > > Is this not called in as part of `git_config(git_default_config, NULL);` > > in cmd_stash() already? > > > > *clicketyclick* > > > > I guess not. But then, maybe it would make sense to run with > > `git_diff_basic_config` from the get go, to avoid having to run > > `git_config()` twice. > > I am not sure I got it right, but if I did: > > Running `git_config` with `git_diff_basic_config` from the > beginning wouldn't be pointless when we would use any other > command than `create`, `push` and `save`? Although it might > confuse the reader a little bit, the stash should run without > problems. In my mind, this would simplify the code. Which is always easier on the reader... ;-) Ciao, Dscho
Re: [GSoC][PATCH v8 18/20] stash: convert `stash--helper.c` into `stash.c`
Hi Paul,
On Wed, 26 Sep 2018, Paul-Sebastian Ungureanu wrote:
> Hi,
>
> > > @@ -1443,9 +1448,10 @@ static int push_stash(int argc, const char
> > > **argv, const char *prefix)
> > > OPT_END()
> > >};
> > > - argc = parse_options(argc, argv, prefix, options,
> > > - git_stash_helper_push_usage,
> > > - 0);
> > > + if (argc)
> > > + argc = parse_options(argc, argv, prefix, options,
> > > + git_stash_push_usage,
> > > + 0);
> >
> > Is this `if (argc)` guard necessary?
>
> Yes because without it, there would be a seg fault when
> calling `git stash`. Why?
>
> After spawning `git stash`, in `push_stash()`: `argc` would be
> 0 (and `argv` would be `NULL`).
I *think* what you want to do, then, is to pass PARSE_OPT_KEEP_ARGV0 in
the first parse_options() call. In general, this needs to be done any time
you might want to call `parse_options()` on the remaining options.
Looking forward to v10,
Dscho
> `parse_options()` calls `parse_options_start()` which does the following:
>
> ctx->argc = ctx->total = argc - 1;
> ctx->argv = argv + 1;
>
> So, `ctx->argc` would be `-1`. After we are back to `parse_options()`,
> `parse_options_step()` would be called. In `parse_options_step()`:
>
> for (; ctx->argc; ctx->argc--, ctx->argv++)
>
> Which is an infinite loop, because `ctx->argc` is already -1.
>
> This check is also done for `git notes list` (function `list()`
> inside `builtin/notes.c`). Now, that I relook at it, it seems to me
> that this is a bug. Probably a better solution would be to check at the
> beginning of `parse_options()` and return early if `argc` is less then one.
>
> > > @@ -1536,7 +1544,44 @@ int cmd_stash__helper(int argc, const char **argv,
> > > const char *prefix)
> > > return !!push_stash(argc, argv, prefix);
> > >else if (!strcmp(argv[0], "save"))
> > > return !!save_stash(argc, argv, prefix);
> > > + else if (*argv[0] != '-')
> > > + usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
> > > + git_stash_usage, options);
> > > +
> > > + if (strcmp(argv[0], "-p")) {
> > > + while (++i < argc && strcmp(argv[i], "--")) {
> > > + /*
> > > + * `akpqu` is a string which contains all short
> > > options,
> > > + * except `-m` which is verified separately.
> > > + */
> > > + if ((strlen(argv[i]) == 2) && *argv[i] == '-' &&
> > > + strchr("akpqu", argv[i][1]))
> >
> > I *think* this is missing the `n`.
>
> I guess that by `n` you are referring to the short option of
> `--no-keep-index`, which is missing because it was also omitted
> in `stash.sh`. I am not sure whether it is worth adding it. In
> case `stash` will learn any other option starting with `n`, this
> might create confusion amongst users.
>
> Best regards,
> Paul
>
Re: [PATCH] travis-ci: install packages in 'ci/install-dependencies.sh'
Hi, On Fri, 9 Nov 2018, Junio C Hamano wrote: > SZEDER Gábor writes: > > >> > I'm not sure about the last paragraph, because: > >> > > >> > - It talks about presumed benefits for a currently still > >> > work-in-progress patch series of an other contributor, and I'm not > >> > really sure that that's a good thing. Perhaps I should have > >> > rather put it below the '---'. > >> > > >> > - I'm confused about the name of this Azure thing. The cover letter > >> > mentions "Azure Pipelines", the file is called > >> > 'azure-pipelines.yml', but the relevant patch I link to talks > >> > about "Azure DevOps" in the commit message. > >> > > >> > Anyway, keep that last paragraph or drop it as you see fit. > >> > >> I hope we'll hear from Dscho in one or two revolutions of the Earth > >> ;-) > > > > ... revolutions around what? :) > > Originally I meant its own axis, but perhaps the moon. I see, you had fun talking about a revolution [*1*]... I am much in favor of this patch, as it indeed will simplify the integration into Azure Pipelines. As to the DevOps vs Pipelines thing: DevOps is the umbrella, it consists of much more than just the automated builds (Pipelines), it also has Boards, Packages, etc, but I don't think we will ever use that in the Git project, as our workflow is based out of a mailing list. Ciao, Dscho Footnote *1*: https://www.youtube.com/watch?v=Xv8FBjo1Y8I
Re: What's cooking in git.git (Nov 2018, #03; Wed, 7)
Hi Phillip, On Thu, 8 Nov 2018, Phillip Wood wrote: > On 07/11/2018 09:41, Junio C Hamano wrote: > > Here are the topics that have been cooking. Commits prefixed with > > '-' are only in 'pu' (proposed updates) while commits prefixed with > > '+' are in 'next'. The ones marked with '.' do not appear in any of > > the integration branches, but I am still holding onto them. > > > > You can find the changes described here in the integration branches > > of the repositories listed at > > > > http://git-blame.blogspot.com/p/git-public-repositories.html > > > > -- > > > > * pw/add-p-select (2018-07-26) 4 commits > > - add -p: optimize line selection for short hunks > > - add -p: allow line selection to be inverted > > - add -p: select modified lines correctly > > - add -p: select individual hunk lines > > > > "git add -p" interactive interface learned to let users choose > > individual added/removed lines to be used in the operation, instead > > of accepting or rejecting a whole hunk. > > > > Will discard. > > No further feedbacks on the topic for quite some time. > > Unfortunately I've not found time to work on this recently and don't see > myself doing so before the new year so I think it makes sense to drop it. > Hopefully I can come up with something next year, it would be nice for users > to avoid having to edit patches where possible. If I understand correctly, this patch series essentially imitates on the console what you can do in Git GUI by selecting multiple lines in the diff and then "Stage Selected Lines"? If my understanding is correct, then yes, I would be very much in favor of having this feature, too. Thanks, Dscho
[PATCH v2 0/1] Update .mailmap
I noticed that there were a couple of duplicate entries in git shortlog -nse v2.19.1.., and then continued a bit to remove the duplicate entries even for v2.10.0.. Changes relative to v1: * Fixed the commit message (it talked about the opposite commit range than intended). * Added a formerly missing space between the email addresses of Masaya. Johannes Schindelin (1): Update .mailmap .mailmap | 13 + 1 file changed, 13 insertions(+) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-71%2Fdscho%2Fmailmap-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-71/dscho/mailmap-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/71 Range-diff vs v1: 1: b590a9bebf ! 1: c121ebc474 Update .mailmap @@ -2,7 +2,7 @@ Update .mailmap -This patch makes the output of `git shortlog -nse v2.10.0` +This patch makes the output of `git shortlog -nse v2.10.0..master` duplicate-free. Signed-off-by: Johannes Schindelin @@ -47,7 +47,7 @@ Mark Rada Martin Langhoff Martin von Zweigbergk -+Masaya Suzuki ++Masaya Suzuki Matt Draisey Matt Kraai Matt McCutchen -- gitgitgadget
[PATCH v2 1/1] Update .mailmap
From: Johannes Schindelin This patch makes the output of `git shortlog -nse v2.10.0..master` duplicate-free. Signed-off-by: Johannes Schindelin --- .mailmap | 13 + 1 file changed, 13 insertions(+) diff --git a/.mailmap b/.mailmap index bef3352b0d..eb7b5fc7b9 100644 --- a/.mailmap +++ b/.mailmap @@ -21,6 +21,8 @@ Anders Kaseorg Aneesh Kumar K.V Amos Waterland Amos Waterland +Ben Peart +Ben Peart Ben Walton Benoit Sigoure Bernt Hansen @@ -32,6 +34,7 @@ Bryan Larsen Cheng Renquan Chris Shoemaker Chris Wright +Christian Ludwig Cord Seele Christian Couder Christian Stimming @@ -51,6 +54,7 @@ David Reiss David S. Miller David Turner David Turner +Derrick Stolee Deskin Miller Dirk Süsserott Eric Blake @@ -98,6 +102,7 @@ Jens Axboe Jens Lindström Jens Lindstrom Jim Meyering Joachim Berdal Haga +Joachim Jablon Johannes Schindelin Johannes Sixt Johannes Sixt @@ -150,6 +155,7 @@ Mark Levedahl Mark Rada Martin Langhoff Martin von Zweigbergk +Masaya Suzuki Matt Draisey Matt Kraai Matt McCutchen @@ -157,6 +163,7 @@ Matthias Kestenholz Matthias Rüster Matthias Ruester Matthias Urlichs Matthias Urlichs +Matthieu Moy Michael Coleman Michael J Gruber Michael J Gruber @@ -180,7 +187,11 @@ Nick Stokoe Nick Woolley Nick Stokoe Nick Woolley Nicolas Morey-Chaisemartin Nicolas Morey-Chaisemartin +Nicolas Morey-Chaisemartin +Nicolas Morey-Chaisemartin +Nicolas Morey-Chaisemartin Nicolas Sebrecht +Orgad Shaneh Paolo Bonzini Pascal Obry Pascal Obry @@ -200,6 +211,7 @@ Philipp A. Hartmann Philippe Bruhat Ralf Thielow Ramsay Jones +Randall S. Becker René Scharfe René Scharfe Rene Scharfe Richard Hansen @@ -238,6 +250,7 @@ Steven Walter Sven Verdoolaege Sven Verdoolaege SZEDER Gábor +Tao Qingyun <845767...@qq.com> Tay Ray Chuan Ted Percival Theodore Ts'o -- gitgitgadget
Re: [PATCH 1/1] Update .mailmap
Hi Junio, On Fri, 9 Nov 2018, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > > > From: Johannes Schindelin > > > > This patch makes the output of `git shortlog -nse v2.10.0` > > duplicate-free. > > Did you mean "v2.10.0..master" or did you really mean this covers > authors recorded up to v2.10.0? Judging from the cover letter, I > think you meant the former. D'oh. Yes, I meant v2.10.0..master. > Can you say a bit more about how one among multiple addresses for > each person was chosen in the log message? E.g. "After asking each > author which one is the preferred one", "Picked the one with the > most recent committer timestamps", "There were two for each but one > of them were bouncing", etc. I looked at each of the authors' mails and tried to determine which one was the preferred one. For example, Masaya sent a patch from their gmail account but signed off using their google account, so I figured the latter was preferable. Nicolas, on the other hand, already had a couple of entries in .mailmap, so I picked the one that seemed to be preferred judging by the .mailmap even if it was not in use lately. For Christian, it was gmail vs googlemail, and I picked the former, as it is more common these days. For Ben and Stolee, I used their Microsoft accounts (preferring the one Ben used originally). For Joachim, Matthieu, Randall and Tao, I used the more personalized email address. For Orgad, I used his personal one rather than his work email. > > @@ -150,6 +155,7 @@ Mark Levedahl > > Mark Rada > > Martin Langhoff > > Martin von Zweigbergk > > > > +Masaya Suzuki > > It is a bit surprising that we can take an entry without SP in > between two addresses and still behave sensibley, but it probably > makes more sense to add one just to look similar to other entries. Whoops. Thanks, fixed! > Thanks for working on this. You're welcome. v2 about to be sent, Dscho
[PATCH 0/2] Fix built-in rebase perf regression
In our tests with large repositories, we noticed a serious regression of the performance of git rebase when using the built-in vs the shell script version. It boils down to an incorrect conversion of a git checkout -q: instead of using a twoway_merge as git checkout does, we used a oneway_merge as git reset does. The latter, however, calls lstat() on all files listed in the index, while the former essentially looks only at the files that are different between the given two revisions. Let's reinstate the original behavior by introducing a flag to the reset_head() function to indicate whether we want to emulate reset --hard (in which case we use the oneway_merge, otherwise we use twoway_merge). Johannes Schindelin (2): rebase: consolidate clean-up code before leaving reset_head() built-in rebase: reinstate `checkout -q` behavior where appropriate builtin/rebase.c | 60 ++-- 1 file changed, 33 insertions(+), 27 deletions(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-72%2Fdscho%2Fbuiltin-rebase-perf-regression-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-72/dscho/builtin-rebase-perf-regression-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/72 -- gitgitgadget
[PATCH 2/2] built-in rebase: reinstate `checkout -q` behavior where appropriate
From: Johannes Schindelin
When we converted a `git checkout -q $onto^0` call to use
`reset_head()`, we inadvertently incurred a change from a twoway_merge
to a oneway_merge, as if we wanted a `git reset --hard` instead.
This has performance ramifications under certain, though, as the
oneway_merge needs to lstat() every single index entry whereas
twoway_merge does not.
So let's go back to the old behavior.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 45 ++---
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6f6d7de156..c1cc50f3f8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -523,11 +523,12 @@ finished_rebase:
#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
static int reset_head(struct object_id *oid, const char *action,
- const char *switch_to_branch, int detach_head,
+ const char *switch_to_branch,
+ int detach_head, int reset_hard,
const char *reflog_orig_head, const char *reflog_head)
{
struct object_id head_oid;
- struct tree_desc desc;
+ struct tree_desc desc[2];
struct lock_file lock = LOCK_INIT;
struct unpack_trees_options unpack_tree_opts;
struct tree *tree;
@@ -536,7 +537,7 @@ static int reset_head(struct object_id *oid, const char
*action,
size_t prefix_len;
struct object_id *orig = NULL, oid_orig,
*old_orig = NULL, oid_old_orig;
- int ret = 0;
+ int ret = 0, nr = 0;
if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
BUG("Not a fully qualified branch: '%s'", switch_to_branch);
@@ -544,20 +545,20 @@ static int reset_head(struct object_id *oid, const char
*action,
if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0)
return -1;
- if (!oid) {
- if (get_oid("HEAD", _oid)) {
- rollback_lock_file();
- return error(_("could not determine HEAD revision"));
- }
- oid = _oid;
+ if (get_oid("HEAD", _oid)) {
+ rollback_lock_file();
+ return error(_("could not determine HEAD revision"));
}
+ if (!oid)
+ oid = _oid;
+
memset(_tree_opts, 0, sizeof(unpack_tree_opts));
setup_unpack_trees_porcelain(_tree_opts, action);
unpack_tree_opts.head_idx = 1;
unpack_tree_opts.src_index = the_repository->index;
unpack_tree_opts.dst_index = the_repository->index;
- unpack_tree_opts.fn = oneway_merge;
+ unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge;
unpack_tree_opts.update = 1;
unpack_tree_opts.merge = 1;
if (!detach_head)
@@ -568,12 +569,17 @@ static int reset_head(struct object_id *oid, const char
*action,
return error(_("could not read index"));
}
- if (!fill_tree_descriptor(, oid)) {
+ if (!reset_hard && !fill_tree_descriptor([nr++], _oid)) {
+ ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
+ goto leave_reset_head;
+ }
+
+ if (!fill_tree_descriptor([nr++], oid)) {
ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
goto leave_reset_head;
}
- if (unpack_trees(1, , _tree_opts)) {
+ if (unpack_trees(nr, desc, _tree_opts)) {
ret = -1;
goto leave_reset_head;
}
@@ -621,7 +627,8 @@ static int reset_head(struct object_id *oid, const char
*action,
leave_reset_head:
strbuf_release();
rollback_lock_file();
- free((void *)desc.buffer);
+ while (nr)
+ free((void *)desc[--nr].buffer);
return ret;
}
@@ -999,7 +1006,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
rerere_clear(_rr);
string_list_clear(_rr, 1);
- if (reset_head(NULL, "reset", NULL, 0, NULL, NULL) < 0)
+ if (reset_head(NULL, "reset", NULL, 0, 1, NULL, NULL) < 0)
die(_("could not discard worktree changes"));
if (read_basic_state())
exit(1);
@@ -1015,7 +1022,7 @@ int cmd_rebase(int argc, const char **argv, const char
*prefix)
if (read_basic_state())
exit(1);
if (reset_head(_head, "reset",
- options.head_name, 0, NULL, NULL) < 0)
+ options.head_name, 0, 1, NULL, NULL) < 0)
die(_("could not move back to %s"),
oid_to_h
[PATCH 1/2] rebase: consolidate clean-up code before leaving reset_head()
From: Johannes Schindelin
The same clean-up code is repeated quite a few times; Let's DRY up the
code some.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..6f6d7de156 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -569,16 +569,13 @@ static int reset_head(struct object_id *oid, const char
*action,
}
if (!fill_tree_descriptor(, oid)) {
- error(_("failed to find tree of %s"), oid_to_hex(oid));
- rollback_lock_file();
- free((void *)desc.buffer);
- return -1;
+ ret = error(_("failed to find tree of %s"), oid_to_hex(oid));
+ goto leave_reset_head;
}
if (unpack_trees(1, , _tree_opts)) {
- rollback_lock_file();
- free((void *)desc.buffer);
- return -1;
+ ret = -1;
+ goto leave_reset_head;
}
tree = parse_tree_indirect(oid);
@@ -586,10 +583,9 @@ static int reset_head(struct object_id *oid, const char
*action,
if (write_locked_index(the_repository->index, , COMMIT_LOCK) < 0)
ret = error(_("could not write index"));
- free((void *)desc.buffer);
if (ret)
- return ret;
+ goto leave_reset_head;
reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
strbuf_addf(, "%s: ", reflog_action ? reflog_action : "rebase");
@@ -622,7 +618,10 @@ static int reset_head(struct object_id *oid, const char
*action,
UPDATE_REFS_MSG_ON_ERR);
}
+leave_reset_head:
strbuf_release();
+ rollback_lock_file();
+ free((void *)desc.buffer);
return ret;
}
--
gitgitgadget
[PATCH 1/1] Update .mailmap
From: Johannes Schindelin This patch makes the output of `git shortlog -nse v2.10.0` duplicate-free. Signed-off-by: Johannes Schindelin --- .mailmap | 13 + 1 file changed, 13 insertions(+) diff --git a/.mailmap b/.mailmap index bef3352b0d..1d8310073a 100644 --- a/.mailmap +++ b/.mailmap @@ -21,6 +21,8 @@ Anders Kaseorg Aneesh Kumar K.V Amos Waterland Amos Waterland +Ben Peart +Ben Peart Ben Walton Benoit Sigoure Bernt Hansen @@ -32,6 +34,7 @@ Bryan Larsen Cheng Renquan Chris Shoemaker Chris Wright +Christian Ludwig Cord Seele Christian Couder Christian Stimming @@ -51,6 +54,7 @@ David Reiss David S. Miller David Turner David Turner +Derrick Stolee Deskin Miller Dirk Süsserott Eric Blake @@ -98,6 +102,7 @@ Jens Axboe Jens Lindström Jens Lindstrom Jim Meyering Joachim Berdal Haga +Joachim Jablon Johannes Schindelin Johannes Sixt Johannes Sixt @@ -150,6 +155,7 @@ Mark Levedahl Mark Rada Martin Langhoff Martin von Zweigbergk +Masaya Suzuki Matt Draisey Matt Kraai Matt McCutchen @@ -157,6 +163,7 @@ Matthias Kestenholz Matthias Rüster Matthias Ruester Matthias Urlichs Matthias Urlichs +Matthieu Moy Michael Coleman Michael J Gruber Michael J Gruber @@ -180,7 +187,11 @@ Nick Stokoe Nick Woolley Nick Stokoe Nick Woolley Nicolas Morey-Chaisemartin Nicolas Morey-Chaisemartin +Nicolas Morey-Chaisemartin +Nicolas Morey-Chaisemartin +Nicolas Morey-Chaisemartin Nicolas Sebrecht +Orgad Shaneh Paolo Bonzini Pascal Obry Pascal Obry @@ -200,6 +211,7 @@ Philipp A. Hartmann Philippe Bruhat Ralf Thielow Ramsay Jones +Randall S. Becker René Scharfe René Scharfe Rene Scharfe Richard Hansen @@ -238,6 +250,7 @@ Steven Walter Sven Verdoolaege Sven Verdoolaege SZEDER Gábor +Tao Qingyun <845767...@qq.com> Tay Ray Chuan Ted Percival Theodore Ts'o -- gitgitgadget
[PATCH 0/1] Update .mailmap
I noticed that there were a couple of duplicate entries in git shortlog -nse v2.19.1.., and then continued a bit to remove the duplicate entries even for v2.10.0.. Johannes Schindelin (1): Update .mailmap .mailmap | 13 + 1 file changed, 13 insertions(+) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-71%2Fdscho%2Fmailmap-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-71/dscho/mailmap-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/71 -- gitgitgadget
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Hi Junio, On Thu, 8 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt? > > The `~` prefix is *already* a reserved character,... > > We would need to prepare for a future where we need yet another > special thing to be expanded, and it will quickly become cryptic if > you said "~/ is HOME, ~USER/ is USER's HOME, ~~/ is runtime prefix, > and ~~~/ is this new thing...". ~runtime_prefix~/ (i.e. carve out > the namespace for USERNAME by reserving any names that ends with a > tilde) may be a viable way to do this, though. It is just as good > as your other idea, , in an earlier message. Indeed. Your "cryptic" matches my "unintuitive". Ciao, Dscho
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Hi Duy, On Thu, 8 Nov 2018, Duy Nguyen wrote: > On Thu, Nov 8, 2018 at 2:14 PM Johannes Schindelin > wrote: > > > > On Wed, 7 Nov 2018, Jeff King wrote: > > > > > All that said, if we're just interested in allowing this for config, > > > then we already have such a wrapper function: git_config_pathname(). > > > > Good point. I agree that `git_config_pathname()` is a better home for this > > feature than `expand_user_path()`. > > > > But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt? > > The `~` prefix is *already* a reserved character, and while it would > > probably not be super intuitive to have `~~` be expanded to the runtime > > prefix, at least it would be less ambiguous than `$RUNTIME_PREFIX` (which > > *is* a valid directory name). > > One thing I had in mind when proposing $VARIABLE is that it opens up a > namespace for us to expand more things (*) for example $GIT_DIR (from > ~/.gitconfig). > > (*) but in a controlled way, it may look like an environment variable, > but we only accept a selected few. And it's only expanded at the > beginning of a path. I understand that desire, and I even agree. But the fact that it looks like an environment variable, but is not, is actually a point in favor of *not* doing this. It would violate the principle of least astonishment. The form `/abc/def` would not be confused with anything that it is not, I would think. The only thing against this form (at least that I can think of) is that some people use this way to talk about paths that vary between different setups, with the implicit assumption that the reader will "interpolate" this *before* applying. So for example, when I tell a user to please edit their /config, I implicitly assume that they know to not type out, literally, but instead fill in the correct path. Ciao, Dscho > > Of course, `~~` is also a valid directory name, but then, so is `~` and we > > do not have any way to specify *that* because `expand_user_path()` will > > always expand it to the home directory. Or am I wrong? Do we have a way to > > disable the expansion? > > > > Ciao, > > Dscho > > > > -- > Duy >
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Hi Junio, On Thu, 8 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > On Thu, 8 Nov 2018, Junio C Hamano wrote: > > > >> I am tempted to say "///" might also be such a > >> way, even in the POSIX world, but am not brave enough to do so, as I > >> suspect that may have a fallout in the Windows world X-<. > > > > It does. //server/share is the way we refer to UNC paths (AKA network > > drives). > > Shucks. That would mean the patch that started this thread would > not be a good idea, as an end-user could already be writing > "//server/share/some/path" and the code with the patch would see '/' > that begins it, and start treating it differently than the code > before the patch X-<. Ouch. You're right! > > Granted, this is a highly unlikely scenario, but I would feel a bit more > > comfortable with something like > > > > /ssl/certs/ca-bundle.crt > > > > Of course, `` is *also* a perfectly valid directory name, > > but I would argue that it is even less likely to exist than > > `$RUNTIME_PREFIX` because the user would have to escape *two* characters > > rather than one. > > Yes, and it is naturally extensible by allowing > inside the special bra-ket pair (just like $OTHER_THINGS can be a > way to extend the system if we used a special variable syntax). True. > >> Are there security implications if we started allowing references to > >> environment varibables in strings we pass expand_user_path()? > > > > Probably. But then, the runtime prefix is not even available as > > environment variable... > > Ah, sorry. I thought it was clear that I would next be suggesting to > add an environmet variable for it, _if_ the approach to allow env > references turns out to be viable. Of course, I should have assumed that. Sorry! Ciao, Dscho
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Hi Peff, On Wed, 7 Nov 2018, Jeff King wrote: > All that said, if we're just interested in allowing this for config, > then we already have such a wrapper function: git_config_pathname(). Good point. I agree that `git_config_pathname()` is a better home for this feature than `expand_user_path()`. But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt? The `~` prefix is *already* a reserved character, and while it would probably not be super intuitive to have `~~` be expanded to the runtime prefix, at least it would be less ambiguous than `$RUNTIME_PREFIX` (which *is* a valid directory name). Of course, `~~` is also a valid directory name, but then, so is `~` and we do not have any way to specify *that* because `expand_user_path()` will always expand it to the home directory. Or am I wrong? Do we have a way to disable the expansion? Ciao, Dscho
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Hi, On Thu, 8 Nov 2018, Junio C Hamano wrote: > I am tempted to say "///" might also be such a > way, even in the POSIX world, but am not brave enough to do so, as I > suspect that may have a fallout in the Windows world X-<. It does. //server/share is the way we refer to UNC paths (AKA network drives). > An earlier suggestion by Duy in [*1*] to pretend as if we take > $VARIABLE and define special variables might be a better direction. My only qualm with this is that `$VARIABLE` is a perfectly allowed part of a path. That is, you *could* create a directory called `$VARIABLE` and reference that, and then the expand_user_path() function (or whatever name we will give it) would always expand this, with no way to switch it off. Granted, this is a highly unlikely scenario, but I would feel a bit more comfortable with something like /ssl/certs/ca-bundle.crt Of course, `` is *also* a perfectly valid directory name, but I would argue that it is even less likely to exist than `$RUNTIME_PREFIX` because the user would have to escape *two* characters rather than one. > Are there security implications if we started allowing references to > environment varibables in strings we pass expand_user_path()? Probably. But then, the runtime prefix is not even available as environment variable... Ciao, Dscho > If we cannot convince ourselves that it is safe to allow access to any > and all environment variables, then we probably can start by specific > "pseudo variables" under our control, like GIT_EXEC_PATH and > GIT_INSTALL_ROOT, that do not even have to be tied to environment > variables, perhaps with further restriction to allow it only at the > beginning of the string, or something like that, if necessary. > > [References] > > *1* >
[PATCH 0/2] built-in rebase --autostash: fix regression
It was reported that the new, shiny built-in git rebase has a bug where it would detach the HEAD when it was not even necessary to detach it. Keeping with my fine tradition to first demonstrate what is the actual problem (and making it easy to verify my claim), this patch series first introduces the regression test, and then the (quite simple) fix. AEvar, sorry for the ASCII-fication of your name, I still did not find the time to look at the GitGitGadget bug closely where it does the wrong thing when Cc:ing with non-ASCII names. Johannes Schindelin (2): built-in rebase: demonstrate regression with --autostash built-in rebase --autostash: leave the current branch alone if possible builtin/rebase.c| 3 ++- t/t3420-rebase-autostash.sh | 8 2 files changed, 10 insertions(+), 1 deletion(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-70%2Fdscho%2Ffix-built-in-rebase-autostash-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-70/dscho/fix-built-in-rebase-autostash-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/70 -- gitgitgadget
[PATCH 1/2] built-in rebase: demonstrate regression with --autostash
From: Johannes Schindelin An unnamed colleague of Ævar Arnfjörð Bjarmason reported a breakage where a `pull --rebase` (which did not really need to do anything but stash, see that nothing was changed, and apply the stash again) also detached the HEAD. This patch adds a minimal reproducer for this regression. Signed-off-by: Johannes Schindelin --- t/t3420-rebase-autostash.sh | 8 1 file changed, 8 insertions(+) diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh index f355c6825a..d4e2520bcb 100755 --- a/t/t3420-rebase-autostash.sh +++ b/t/t3420-rebase-autostash.sh @@ -361,4 +361,12 @@ test_expect_success 'autostash with dirty submodules' ' git rebase -i --autostash HEAD ' +test_expect_failure 'branch is left alone when possible' ' + git checkout -b unchanged-branch && + echo changed >file0 && + git rebase --autostash unchanged-branch && + test changed = "$(cat file0)" && + test unchanged-branch = "$(git rev-parse --abbrev-ref HEAD)" +' + test_done -- gitgitgadget
[PATCH 2/2] built-in rebase --autostash: leave the current branch alone if possible
From: Johannes Schindelin
When we converted a `git reset --hard` call in the original Unix shell
script to built-in code, we asked to reset the worktree and the index
and explicitly *not* to detach the HEAD. By mistake, though, we still
did. Let's fix this.
Signed-off-by: Johannes Schindelin
---
builtin/rebase.c| 3 ++-
t/t3420-rebase-autostash.sh | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ee06aa363..4a608d0a78 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -613,7 +613,8 @@ static int reset_head(struct object_id *oid, const char
*action,
reflog_head = msg.buf;
}
if (!switch_to_branch)
- ret = update_ref(reflog_head, "HEAD", oid, orig, REF_NO_DEREF,
+ ret = update_ref(reflog_head, "HEAD", oid, orig,
+detach_head ? REF_NO_DEREF : 0,
UPDATE_REFS_MSG_ON_ERR);
else {
ret = create_symref("HEAD", switch_to_branch, msg.buf);
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index d4e2520bcb..4c7494cc8f 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -361,7 +361,7 @@ test_expect_success 'autostash with dirty submodules' '
git rebase -i --autostash HEAD
'
-test_expect_failure 'branch is left alone when possible' '
+test_expect_success 'branch is left alone when possible' '
git checkout -b unchanged-branch &&
echo changed >file0 &&
git rebase --autostash unchanged-branch &&
--
gitgitgadget
Re: Regression in rebase-in-C with rebase.autoStash=true
Hi Ævar,
On Tue, 6 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Oct 23 2018, Johannes Schindelin via GitGitGadget wrote:
>
> > Johannes Schindelin (2):
> > rebase --autostash: demonstrate a problem with dirty submodules
> > rebase --autostash: fix issue with dirty submodules
> >
> > builtin/rebase.c| 2 +-
> > t/t3420-rebase-autostash.sh | 10 ++
> > 2 files changed, 11 insertions(+), 1 deletion(-)
>
> There's another bug with rebase.autoStash in master (and next/pu) but
> not v2.19.0. I tried to bisect bit it just comes down to 5541bd5b8f
> ("rebase: default to using the builtin rebase", 2018-08-08).
>
> Credit to a co-worker of mine who wishes to remain anonymous for
> discovering this. I narrowed down his test-case to (any repo will do):
>
> (
> rm -rf /tmp/todo &&
> git clone --single-branch --no-tags --branch=todo
> https://github.com/git/git.git /tmp/todo &&
> cd /tmp/todo &&
> rm Make &&
> git rev-parse --abbrev-ref HEAD &&
> git -c rebase.autoStash=true -c pull.rebase=true pull &&
> if test $(git rev-parse --abbrev-ref HEAD) != 'todo'
> then
> echo 'On detached head!' &&
> git status &&
> exit 1
> else
> echo 'We are still on our todo branch!'
> fi
> )
I found the culprit. Patch forthcoming,
Dscho
Re: [PATCH 1/1] Windows: force-recompile git.res for differing architectures
Hi, On Wed, 7 Nov 2018, Junio C Hamano wrote: > Johannes Sixt writes: > > > Am 07.11.18 um 02:32 schrieb Junio C Hamano: > >> Johannes Sixt writes: > >>> On Linux, when I recompile for a different architecture, CFLAGS would > >>> change, so I would have thought that GIT-CFLAGS were the natural > >>> choice for a dependency. Don't they change in this case on Windows, > >>> too? > >> > >> Depending on GIT-CFLAGS would have a better chance of being safe, I > >> guess, even though it can at times be overly safe, than GIT-PREFIX, > >> I suspect. As a single user target in Makefile, which is only used > >> by Dscho, who intends to stick to /mingw(32|64)/ convention til the > >> end of time, I think the posted patch is OK, though. > > > > I think that it's not only Dscho who uses the target (my build > > environment, for example, is different from Dscho's and compiles > > git.res, too). But since the patch helps him most and doesn't hurt > > others, it is good to go. No objection from my side. > > Yeah, that was phrased poorly. What I meant was by "only by Dscho" > was "only by those who share the convention that GIT-PREFIX is > changed if and only if targetting a different arch". > > Anyway, I just wanted to say that GIT-PREFIX may not be precise > enough but would give sufficient signal; GIT-CFLAGS may be a more > cautious choice, but it may change a bit too often ;-). I am fine with GIT-CFLAGS, too. Do you want me to "rick-roll" a v2? Ciao, Dscho
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Hi Hannes,
On Tue, 6 Nov 2018, Johannes Sixt wrote:
> Am 06.11.18 um 15:53 schrieb Johannes Schindelin via GitGitGadget:
> > From: Johannes Schindelin
> >
> > On Windows, an absolute POSIX path needs to be turned into a Windows
> > one.
>
> If I were picky, I would say that in a pure Windows application there cannot
> be POSIX paths to begin with.
>
> Even if a path looks like a POSIX paths, i.e. it starts with a directory
> separator, but not with drive-letter-colon, it still has a particular meaning,
> namely (as far as I know) that the path is anchored at the root of the drive
> of the current working directory.
>
> If a user specifies such a path on Windows, and it must undergo
> expand_user_path(), then that is a user error, or the user knows what they are
> doing.
>
> I think it is wrong to interpret such a path as relative to some random other
> directory, like this patch seems to do.
Okay, now we know everything you find wrong with the current patch. Do you
have any suggestion how to make it right? I.e. what would you suggest as a
way to specify in a gitconfig in a portable Git where the certificate
bundle is?
Thanks,
Dscho
>
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > path.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/path.c b/path.c
> > index 34f0f98349..a72abf0e1f 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -11,6 +11,7 @@
> > #include "path.h"
> > #include "packfile.h"
> > #include "object-store.h"
> > +#include "exec-cmd.h"
> >
> > static int get_st_mode_bits(const char *path, int *mode)
> > {
> > @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
> >
> >if (path == NULL)
> > goto return_null;
> > +#ifdef __MINGW32__
> > + if (path[0] == '/')
> > + return system_path(path + 1);
> > +#endif
> >if (path[0] == '~') {
> > const char *first_slash = strchrnul(path, '/');
> > const char *username = path + 1;
> >
>
>
>
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Hi,
On Tue, 6 Nov 2018, Duy Nguyen wrote:
> On Tue, Nov 6, 2018 at 3:55 PM Johannes Schindelin via GitGitGadget
> wrote:
> >
> > From: Johannes Schindelin
> >
> > On Windows, an absolute POSIX path needs to be turned into a Windows
> > one.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > path.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/path.c b/path.c
> > index 34f0f98349..a72abf0e1f 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -11,6 +11,7 @@
> > #include "path.h"
> > #include "packfile.h"
> > #include "object-store.h"
> > +#include "exec-cmd.h"
> >
> > static int get_st_mode_bits(const char *path, int *mode)
> > {
> > @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
> >
> > if (path == NULL)
> > goto return_null;
> > +#ifdef __MINGW32__
> > + if (path[0] == '/')
> > + return system_path(path + 1);
> > +#endif
>
> Should this behavior be documented somewhere, maybe in config.txt?
First we need to find a consensus how this should work.
Ciao,
Dscho
>
> > if (path[0] == '~') {
> > const char *first_slash = strchrnul(path, '/');
> > const char *username = path + 1;
> > --
> > gitgitgadget
> --
> Duy
>
Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()
Hi,
On Wed, 7 Nov 2018, Junio C Hamano wrote:
> Ramsay Jones writes:
>
> > On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote:
> >> From: Johannes Schindelin
> >>
> >> On Windows, an absolute POSIX path needs to be turned into a Windows
> >> one.
> >>
> >> Signed-off-by: Johannes Schindelin
> >> ---
> >> path.c | 5 +
> >> 1 file changed, 5 insertions(+)
> >>
> >> diff --git a/path.c b/path.c
> >> index 34f0f98349..a72abf0e1f 100644
> >> --- a/path.c
> >> +++ b/path.c
> >> @@ -11,6 +11,7 @@
> >> #include "path.h"
> >> #include "packfile.h"
> >> #include "object-store.h"
> >> +#include "exec-cmd.h"
> >>
> >> static int get_st_mode_bits(const char *path, int *mode)
> >> {
> >> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int
> >> real_home)
> >>
> >>if (path == NULL)
> >>goto return_null;
> >> +#ifdef __MINGW32__
> >> + if (path[0] == '/')
> >> + return system_path(path + 1);
> >> +#endif
> >
> > Hmm, this doesn't quite fit with the intended use of this
> > function! ;-) (even on windows!)
> >
> > I haven't looked very deeply, but doesn't this affect all
> > absolute paths in the config read by git_config_pathname(),
> > along with all 'included config' files?
>
> I think so. I have not thought things through to say if replacing a
> "full path in the current drive" with system_path() is a sensible
> thing to do in the first place, but I am getting the impression from
> review comments that it probably is not.
>
> > I am pretty sure that I would not want the absolute paths
> > in my config file(s) magically 'moved' depending on whether
> > git has been compiled with 'runtime prefix' support or not!
The cute thing is: your absolute paths would not be moved because we are
talking about Windows. Therefore your absolute paths would not start with
a forward slash.
> In any case, the helper is about expanding ~/foo and ~who/foo to
> absolute paths, without touching other paths, so it is a wrong
> function to implement it in, even if the motivation were sensible.
It could be renamed. In any case, for this feature we would need to expand
a path that is not the final path, and this here location is the most
logical place to do so.
Ciao,
Dscho
Re: [PATCH v2] range-diff: add a --no-patch option to show a summary
Hi,
On Wed, 7 Nov 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
>
> > diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> > index f01a0be851..05d1f6b6b6 100644
> > --- a/builtin/range-diff.c
> > +++ b/builtin/range-diff.c
> > @@ -16,11 +16,14 @@ int cmd_range_diff(int argc, const char **argv, const
> > char *prefix)
> > int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
> > struct diff_options diffopt = { NULL };
> > int simple_color = -1;
> > + int no_patch = 0;
> > struct option options[] = {
> > OPT_INTEGER(0, "creation-factor", _factor,
> > N_("Percentage by which creation is weighted")),
> > OPT_BOOL(0, "no-dual-color", _color,
> > N_("use simple diff colors")),
> > + OPT_BOOL_F('s', "no-patch", _patch,
> > +N_("show patch output"), PARSE_OPT_NONEG),
>
> As OPT_BOOL("patch") natively takes "--no-patch" to flip the bool
> off, an int variable "patch" that is initialized to 1 would make it
> more readable by avoiding double negation !no_patch like the one we
> see below. I guess the reason behind the contortion you wanted to
> give the synonym --silent to it?
In light of my investigation that revealed that the original behavior
(which is still documented in the manual page of range-diff) was broken,
and I would much rather see that fixed than adding a workaround.
I would be fine with my patch being combined with the update to the manual
page and the regression test, as a v3.
Ciao,
Dscho
>
> > @@ -92,7 +95,7 @@ int cmd_range_diff(int argc, const char **argv, const
> > char *prefix)
> > }
> >
> > res = show_range_diff(range1.buf, range2.buf, creation_factor,
> > - simple_color < 1, );
> > + simple_color < 1, !no_patch, );
>
> > strbuf_release();
> > strbuf_release();
>
> > @@ -7,6 +7,7 @@
> >
> > int show_range_diff(const char *range1, const char *range2,
> > int creation_factor, int dual_color,
> > + int patch,
> > struct diff_options *diffopt);
>
> Other than that small "Huh?", the code looks good to me.
>
> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> > index 6aae364171..27e071650b 100755
> > --- a/t/t3206-range-diff.sh
> > +++ b/t/t3206-range-diff.sh
> > @@ -122,6 +122,26 @@ test_expect_success 'changed commit' '
> > test_cmp expected actual
> > '
> >
> > +test_expect_success 'changed commit -p & --patch' '
> > + git range-diff --no-color -p topic...changed >actual &&
> > + test_cmp expected actual &&
> > + git range-diff --no-color --patch topic...changed >actual &&
> > + test_cmp expected actual
>
> This makes sure that -p and --patch produces the same output as the
> default case? I am not sure who in the parseopt API groks the
> single letter "-p" in this case offhand. Care to explain how?
>
> The other side of the test to see -s/--no-patch we see below also
> makes sense.
>
> > +test_expect_success 'changed commit -s & --no-patch' '
> > +...
> > + cat >expected <<-EOF &&
>
> Quote EOF to allow readers skim the contents without looking for and
> worrying about $substitutions in there, unless there are tons of
> offending code in the same script already in which case we should
> leave the clean-up outside this primary change.
>
>
Re: [PATCH] range-diff: add a --no-patch option to show a summary
Me again, On Wed, 7 Nov 2018, Johannes Schindelin wrote: > On Wed, 7 Nov 2018, Johannes Schindelin wrote: > > > On Wed, 7 Nov 2018, Johannes Schindelin wrote: > > > > > On Tue, 6 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > > > > > > > On Tue, Nov 06 2018, Johannes Schindelin wrote: > > > > > > > > > On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > > > > > > > > > >> On Mon, Nov 05 2018, Eric Sunshine wrote: > > > > >> > > > > >> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason > > > > >> > wrote: > > > > >> >> Add a --no-patch option which shows which changes got removed, > > > > >> >> added > > > > >> >> or moved etc., without showing the diff associated with them. > > > > >> > > > > > >> > This option existed in the very first version[1] of range-diff > > > > >> > (then > > > > >> > called branch-diff) implemented by Dscho, although it was called > > > > >> > --no-patches (with an "es"), which it inherited from tbdiff. I > > > > >> > think > > > > >> > someone (possibly me) pointed out that --no-patch (sans "es") > > > > >> > would be > > > > >> > more consistent with existing Git options. I don't recall why Dscho > > > > >> > removed the option during the re-rolls, but the explanation may be > > > > >> > in > > > > >> > that thread. > > > > >> > > > > >> Thanks for digging. Big thread, not going to re-read it now. I'd just > > > > >> like to have this. > > > > > > > > > > In my hands, the well-documented `-s` option works (see e.g. > > > > > https://git-scm.com/docs/git-diff#git-diff--s), although I have to > > > > > admit > > > > > that the `git-range-diff` manual does not talk about the diff-options. > > > > > > > > > > And for the record, for me, `git range-diff A...B --no-patch` > > > > > *already* > > > > > works. > > > > > > > > Neither of those works for me without my patch. E.g. > > > > > > > > ./git-range-diff -s 711aaa392f...a5ba8f2101 > > > > ./git-range-diff --no-patch 711aaa392f...a5ba8f2101 > > > > > > > > This is on current next, 2.19.1.1182.g4ecb1133ce. What version are you > > > > on? > > > > > > I tried it with git version 2.19.0.windows.1. > > > > > > To verify, I repeated this with `next` (git version > > > 2.19.1.1215.g8438c0b2453a): > > > > > > ./git range-diff -s 711aaa392f...a5ba8f2101 > > > fatal: unrecognized argument: --output-indicator-new=> > > > error: could not parse log for 'a5ba8f2101..711aaa392f' > > > > > > Which means that something broke rather dramatically between > > > v2.19.0.windows.1 and 8438c0b2453a. > > > > Nevermind, this was solved by passing `--exec-path=$PWD`. And *now* I can > > reproduce your finding. > > I've bisected this breakage down to 73a834e9e279 (range-diff: relieve > callers of low-level configuration burden, 2018-07-22). Eric, that's one > of your commits. Okay, so I would really like to point you to this particular paragraph in the manual page of `git range-diff` (just below https://git-scm.com/docs/git-range-diff#git-range-diff-ltbasegtltrev1gtltrev2gt): `git range-diff` also accepts the regular diff options (see linkgit:git-diff[1]), most notably the `--color=[]` and `--no-color` options. These options are used when generating the "diff between patches", i.e. to compare the author, commit message and diff of corresponding old/new commits. There is currently no means to tweak the diff options passed to `git log` when generating those patches. So this was quite intentional, in particular with an eye on `--no-patch`. Note also the commit message of c8c5e43ac3f9 (range-diff: also show the diff between patches, 2018-08-13): Note also: while tbdiff accepts the `--no-patches` option to suppress these diffs between patches, we prefer the `-s` (or `--no-patch`) option that is automatically supported via our use of diff_opt_parse(). This was very, very intentional, as you can see, and it was pretty broken by 73a834e. This here patch papers over that breakage, sadly I have too much on my plate as it is, so I cannot wrap it up in a nice commit (nor add a regression test, but you did that, nor investigate what else is broken) and therefore I would be indebted if you could take this in your custody: diff --git a/range-diff.c b/range-diff.c index 3dd2edda0176..014112ee401f 100644 --- a/range-diff.c +++ b/range-diff.c @@ -433,7 +433,8 @@ int show_range_diff(const char *range1, const char *range2, struct strbuf indent = STRBUF_INIT; memcpy(, diffopt, sizeof(opts)); - opts.output_format = DIFF_FORMAT_PATCH; + if (!opts.output_format) + 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; Ciao, Dscho
Re: [PATCH] range-diff: add a --no-patch option to show a summary
Hi Ævar, On Wed, 7 Nov 2018, Johannes Schindelin wrote: > On Wed, 7 Nov 2018, Johannes Schindelin wrote: > > > On Tue, 6 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > > > > > On Tue, Nov 06 2018, Johannes Schindelin wrote: > > > > > > > On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > > > > > > > >> On Mon, Nov 05 2018, Eric Sunshine wrote: > > > >> > > > >> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason > > > >> > wrote: > > > >> >> Add a --no-patch option which shows which changes got removed, added > > > >> >> or moved etc., without showing the diff associated with them. > > > >> > > > > >> > This option existed in the very first version[1] of range-diff (then > > > >> > called branch-diff) implemented by Dscho, although it was called > > > >> > --no-patches (with an "es"), which it inherited from tbdiff. I think > > > >> > someone (possibly me) pointed out that --no-patch (sans "es") would > > > >> > be > > > >> > more consistent with existing Git options. I don't recall why Dscho > > > >> > removed the option during the re-rolls, but the explanation may be in > > > >> > that thread. > > > >> > > > >> Thanks for digging. Big thread, not going to re-read it now. I'd just > > > >> like to have this. > > > > > > > > In my hands, the well-documented `-s` option works (see e.g. > > > > https://git-scm.com/docs/git-diff#git-diff--s), although I have to admit > > > > that the `git-range-diff` manual does not talk about the diff-options. > > > > > > > > And for the record, for me, `git range-diff A...B --no-patch` *already* > > > > works. > > > > > > Neither of those works for me without my patch. E.g. > > > > > > ./git-range-diff -s 711aaa392f...a5ba8f2101 > > > ./git-range-diff --no-patch 711aaa392f...a5ba8f2101 > > > > > > This is on current next, 2.19.1.1182.g4ecb1133ce. What version are you > > > on? > > > > I tried it with git version 2.19.0.windows.1. > > > > To verify, I repeated this with `next` (git version > > 2.19.1.1215.g8438c0b2453a): > > > > ./git range-diff -s 711aaa392f...a5ba8f2101 > > fatal: unrecognized argument: --output-indicator-new=> > > error: could not parse log for 'a5ba8f2101..711aaa392f' > > > > Which means that something broke rather dramatically between > > v2.19.0.windows.1 and 8438c0b2453a. > > Nevermind, this was solved by passing `--exec-path=$PWD`. And *now* I can > reproduce your finding. I've bisected this breakage down to 73a834e9e279 (range-diff: relieve callers of low-level configuration burden, 2018-07-22). Eric, that's one of your commits. Ciao, Dscho
Re: [PATCH] range-diff: add a --no-patch option to show a summary
Hi Ævar, On Wed, 7 Nov 2018, Johannes Schindelin wrote: > On Tue, 6 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > > > On Tue, Nov 06 2018, Johannes Schindelin wrote: > > > > > On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > > > > > >> On Mon, Nov 05 2018, Eric Sunshine wrote: > > >> > > >> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason > > >> > wrote: > > >> >> Add a --no-patch option which shows which changes got removed, added > > >> >> or moved etc., without showing the diff associated with them. > > >> > > > >> > This option existed in the very first version[1] of range-diff (then > > >> > called branch-diff) implemented by Dscho, although it was called > > >> > --no-patches (with an "es"), which it inherited from tbdiff. I think > > >> > someone (possibly me) pointed out that --no-patch (sans "es") would be > > >> > more consistent with existing Git options. I don't recall why Dscho > > >> > removed the option during the re-rolls, but the explanation may be in > > >> > that thread. > > >> > > >> Thanks for digging. Big thread, not going to re-read it now. I'd just > > >> like to have this. > > > > > > In my hands, the well-documented `-s` option works (see e.g. > > > https://git-scm.com/docs/git-diff#git-diff--s), although I have to admit > > > that the `git-range-diff` manual does not talk about the diff-options. > > > > > > And for the record, for me, `git range-diff A...B --no-patch` *already* > > > works. > > > > Neither of those works for me without my patch. E.g. > > > > ./git-range-diff -s 711aaa392f...a5ba8f2101 > > ./git-range-diff --no-patch 711aaa392f...a5ba8f2101 > > > > This is on current next, 2.19.1.1182.g4ecb1133ce. What version are you > > on? > > I tried it with git version 2.19.0.windows.1. > > To verify, I repeated this with `next` (git version > 2.19.1.1215.g8438c0b2453a): > > ./git range-diff -s 711aaa392f...a5ba8f2101 > fatal: unrecognized argument: --output-indicator-new=> > error: could not parse log for 'a5ba8f2101..711aaa392f' > > Which means that something broke rather dramatically between > v2.19.0.windows.1 and 8438c0b2453a. Nevermind, this was solved by passing `--exec-path=$PWD`. And *now* I can reproduce your finding. Ciao, Dscho
Re: [PATCH] range-diff: add a --no-patch option to show a summary
Hi Ævar, On Tue, 6 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > On Tue, Nov 06 2018, Johannes Schindelin wrote: > > > On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > > > >> On Mon, Nov 05 2018, Eric Sunshine wrote: > >> > >> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason > >> > wrote: > >> >> Add a --no-patch option which shows which changes got removed, added > >> >> or moved etc., without showing the diff associated with them. > >> > > >> > This option existed in the very first version[1] of range-diff (then > >> > called branch-diff) implemented by Dscho, although it was called > >> > --no-patches (with an "es"), which it inherited from tbdiff. I think > >> > someone (possibly me) pointed out that --no-patch (sans "es") would be > >> > more consistent with existing Git options. I don't recall why Dscho > >> > removed the option during the re-rolls, but the explanation may be in > >> > that thread. > >> > >> Thanks for digging. Big thread, not going to re-read it now. I'd just > >> like to have this. > > > > In my hands, the well-documented `-s` option works (see e.g. > > https://git-scm.com/docs/git-diff#git-diff--s), although I have to admit > > that the `git-range-diff` manual does not talk about the diff-options. > > > > And for the record, for me, `git range-diff A...B --no-patch` *already* > > works. > > Neither of those works for me without my patch. E.g. > > ./git-range-diff -s 711aaa392f...a5ba8f2101 > ./git-range-diff --no-patch 711aaa392f...a5ba8f2101 > > This is on current next, 2.19.1.1182.g4ecb1133ce. What version are you > on? I tried it with git version 2.19.0.windows.1. To verify, I repeated this with `next` (git version 2.19.1.1215.g8438c0b2453a): ./git range-diff -s 711aaa392f...a5ba8f2101 fatal: unrecognized argument: --output-indicator-new=> error: could not parse log for 'a5ba8f2101..711aaa392f' Which means that something broke rather dramatically between v2.19.0.windows.1 and 8438c0b2453a. Ciao, Dscho > > >> > >> > I was also wondering if --summarize or --summary-only might be a > >> > better name, describing the behavior at a higher level, but since > >> > there is precedent for --no-patch (or --no-patches in tbdiff), perhaps > >> > the name is fine as is. > >> > >> I think we should aim to keep a 1=1 mapping between range-diff and > >> log/show options when possible, even though the output might have a > >> slightly different flavor as my 4th paragraph discussing a potential > >> --stat talks about. > >> > >> E.g. I can imagine that range-diff --no-patch --stat --summary would not > >> show the patch, but a stat as described there, plus e.g. a "create > >> mode..." if applicable. > >> > >> This change implements only a tiny fraction of that, but it would be > >> very neat if we supported more stuff, and showed it in range-diff-y way, > >> e.g. some compact format showing: > >> > >> 1 file changed, 3->2 insertions(+), 10->9 deletions(-) > >> create mode 100(6 -> 7)44 new-executable > >> > >> > The patch itself looks okay. > >> > > >> > [1]: > >> > https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schinde...@gmx.de/ > >> >
[PATCH 0/1] Windows: force-recompile git.res for differing architectures
This is a patch designed to help maintaining Git for Windows better: when the same source code is "cross-compiled" for i686 as well as x86_64, we want to rebuild the whole thing, including the resource file git.res. Note: regular C files are re-compiled appropriately, as the default prefix in Git for Windows is /mingw32 or /mingw64 depending on the architecture, and this difference is manifested in the CFLAGS (which, upon change, trigger a complete rebuild). As non-Windows platforms do not even compile these .res files, this patch should have exactly no effect on non-Windows builds. Johannes Schindelin (1): Windows: force-recompile git.res for differing architectures Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: cd69ec8cde54af1817630331fc441f493866f0d4 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-67%2Fdscho%2Fmingw-git.res-bitness-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-67/dscho/mingw-git.res-bitness-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/67 -- gitgitgadget
[PATCH 1/1] Windows: force-recompile git.res for differing architectures
From: Johannes Schindelin When git.rc is compiled into git.res, the result is actually dependent on the architecture. That is, you cannot simply link a 32-bit git.res into a 64-bit git.exe. Therefore, to allow 32-bit and 64-bit builds in the same directory, we let git.res depend on GIT-PREFIX so that it gets recompiled when compiling for a different architecture (this works because the exec path changes based on the architecture: /mingw32/libexec/git-core for 32-bit and /mingw64/libexec/git-core for 64-bit). Signed-off-by: Johannes Schindelin --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index bbfbb4292d..8375736c32 100644 --- a/Makefile +++ b/Makefile @@ -2110,7 +2110,7 @@ $(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES $(QUIET_GEN)$(cmd_munge_script) && \ mv $@+ $@ -git.res: git.rc GIT-VERSION-FILE +git.res: git.rc GIT-VERSION-FILE GIT-PREFIX $(QUIET_RC)$(RC) \ $(join -DMAJOR= -DMINOR= -DMICRO= -DPATCHLEVEL=, $(wordlist 1, 4, \ $(shell echo $(GIT_VERSION) 0 0 0 0 | tr '.a-zA-Z-' ' '))) \ -- gitgitgadget
[PATCH 1/1] mingw: handle absolute paths in expand_user_path()
From: Johannes Schindelin
On Windows, an absolute POSIX path needs to be turned into a Windows
one.
Signed-off-by: Johannes Schindelin
---
path.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/path.c b/path.c
index 34f0f98349..a72abf0e1f 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@
#include "path.h"
#include "packfile.h"
#include "object-store.h"
+#include "exec-cmd.h"
static int get_st_mode_bits(const char *path, int *mode)
{
@@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
if (path == NULL)
goto return_null;
+#ifdef __MINGW32__
+ if (path[0] == '/')
+ return system_path(path + 1);
+#endif
if (path[0] == '~') {
const char *first_slash = strchrnul(path, '/');
const char *username = path + 1;
--
gitgitgadget
[PATCH 0/1] mingw: handle absolute paths in expand_user_path()
While this patch has been "in production" in Git for Windows for a good while, this patch series is half meant as a request for comments. The reason is this: something like this (make paths specified e.g. via http.sslCAInfo relative to the runtime prefix) is potentially useful also in the non-Windows context, as long as Git was built with the runtime prefix feature. The main problem with non-Windows platforms is that paths starting with a single slash are unambiguously absolute, whereas we can say with certainty that they are not absolute Windows paths. So maybe someone on this list has a clever idea how we could specify paths (unambiguously, even on non-Windows platforms) that Git should interpret as relative to the runtime prefix? Johannes Schindelin (1): mingw: handle absolute paths in expand_user_path() path.c | 5 + 1 file changed, 5 insertions(+) base-commit: cd69ec8cde54af1817630331fc441f493866f0d4 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-66%2Fdscho%2Fmingw-expand-absolute-user-path-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-66/dscho/mingw-expand-absolute-user-path-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/66 -- gitgitgadget
Re: [PATCH v2 1/1] diff-highlight: Use correct /dev/null for UNIX and Windows
List,
I have no idea why this mail made it to GitGitGadget's email account but
not to the Git mailing list... Sorry about that.
Ciao,
Johannes
On Wed, 31 Oct 2018, Chris. Webster via GitGitGadget wrote:
> From: "Chris. Webster"
>
> Use File::Spec->devnull() for output redirection to avoid messages
> when Windows version of Perl is first in path. The message 'The
> system cannot find the path specified.' is displayed each time git is
> run to get colors.
>
> Signed-off-by: Chris. Webster
> ---
> contrib/diff-highlight/DiffHighlight.pm | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/diff-highlight/DiffHighlight.pm
> b/contrib/diff-highlight/DiffHighlight.pm
> index 536754583b..7440aa1c46 100644
> --- a/contrib/diff-highlight/DiffHighlight.pm
> +++ b/contrib/diff-highlight/DiffHighlight.pm
> @@ -4,6 +4,11 @@ use 5.008;
> use warnings FATAL => 'all';
> use strict;
>
> +# Use the correct value for both UNIX and Windows (/dev/null vs nul)
> +use File::Spec;
> +
> +my $NULL = File::Spec->devnull();
> +
> # Highlight by reversing foreground and background. You could do
> # other things like bold or underline if you prefer.
> my @OLD_HIGHLIGHT = (
> @@ -134,7 +139,7 @@ sub highlight_stdin {
> # fallback, which means we will work even if git can't be run.
> sub color_config {
> my ($key, $default) = @_;
> - my $s = `git config --get-color $key 2>/dev/null`;
> + my $s = `git config --get-color $key 2>$NULL`;
> return length($s) ? $s : $default;
> }
>
> --
> gitgitgadget
>
