* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Stefan Beller @ 2016-12-06 18:22 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Junio C Hamano, git@vger.kernel.org,
David Aguilar, Dennis Kaarsemaker
In-Reply-To: <20161206150955.mvq4ocamaei52bap@sigill.intra.peff.net>
On Tue, Dec 6, 2016 at 7:09 AM, Jeff King <peff@peff.net> wrote:
>>
>> Maybe even go a step further and say that the config code needs a context
>> "object".
>
> If I were writing git from scratch, I'd consider making a "struct
> repository" object. I'm not sure how painful it would be to retro-fit it
> at this point.
Would it be possible to introduce "the repo" struct similar to "the index"
in cache.h?
From a submodule perspective I would very much welcome this
object oriented approach to repositories.
^ permalink raw reply
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Junio C Hamano @ 2016-12-06 18:17 UTC (permalink / raw)
To: Jeff King; +Cc: Ramsay Jones, GIT Mailing-list
In-Reply-To: <20161206140259.lly76xkvsj7su3om@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Dec 05, 2016 at 10:01:19AM -0800, Junio C Hamano wrote:
>
>> > That said, I think the right patch may be to just drop --abbrev
>> > entirely.
>> > ...
>> > I think at that point it was a noop, as 7 should have been the default.
>> > And now we probably ought to drop it, so that we can use the
>> > auto-scaling default.
>>
>> Yeah, I agree.
>>
>> It does mean that snapshot binaries built out of the same commit in
>> the same repository before and after a repack have higher chances of
>> getting named differently, which may surprise people, but that
>> already is possible with a fixed length if the repacking involves
>> pruning (albeit with lower probabilities), and I do not think it is
>> a problem.
>
> I think that the number is already unstable, even with --abbrev, as it
> just specifies a minimum. So any operation that creates objects has a
> possibility of increasing the length. Or more likely, two people
> describing the same version may end up with different strings because
> they have different objects in their repositories (e.g., I used to
> carry's trast's git-notes archive of the mailing list which added quite
> a few objects).
>
> I agree that having it change over the course of a repack is slightly
> _more_ surprising than those cases, but ultimately the value just isn't
> stable.
Yup, that is what I meant to say with "that is already possible" and
we are on the same page. As all three of us seem to be happy with
just dropping --abbrev and letting describe do its default thing (as
configured by whoever is doing the build), let's do so.
-- >8 --
From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Sun, 4 Dec 2016 20:45:59 +0000
Subject: [PATCH] GIT-VERSION-GEN: do not force abbreviation length used by 'describe'
The default version name for a Git binary is computed by running
"git describe" on the commit the binary is made out of, basing on a
tag whose name matches "v[0-9]*", e.g. v2.11.0-rc2-2-g7f1dc9.
In the very early days, with 9b88fcef7d ("Makefile: use git-describe
to mark the git version.", 2005-12-27), we used "--abbrev=4" to get
absolute minimum number of abbreviated commit object name. This was
later changed to match the default minimum of 7 with bf505158d0
("Git 1.7.10.1", 2012-05-01).
These days, the "default minimum" scales automatically depending on
the size of the repository, and there is no point in specifying a
particular abbreviation length; all we wanted since Git 1.7.10.1
days was to get "something reasonable we would use by default".
Just drop "--abbrev=<number>" from the invocation of "git describe"
and let the command pick what it thinks is appropriate, taking the
end user's configuration and the repository contents into account.
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
GIT-VERSION-GEN | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 556fbfc104..f95b04bb36 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -12,7 +12,7 @@ if test -f version
then
VN=$(cat version) || VN="$DEF_VER"
elif test -d ${GIT_DIR:-.git} -o -f .git &&
- VN=$(git describe --match "v[0-9]*" --abbrev=7 HEAD 2>/dev/null) &&
+ VN=$(git describe --match "v[0-9]*" HEAD 2>/dev/null) &&
case "$VN" in
*$LF*) (exit 1) ;;
v[0-9]*)
--
2.11.0-263-gd89495280e
^ permalink raw reply related
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Jeff King @ 2016-12-06 18:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <xmqq60mx2bbi.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 06, 2016 at 09:53:53AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I don't know if that makes things any easier. I feel funny saying "no,
> > no, mine preempts yours because it is more maint-worthy", but I think
> > that order does make sense.
> >
> > I think it would be OK to put Brandon's on maint, too, though. It is a
> > refactor of an existing security feature to make it more featureful, but
> > the way it is implemented could not cause security regressions unless
> > you use the new feature (IOW, we still respect the whitelist environment
> > exactly as before).
>
> I think I merged yours and then Brandon's on jch/pu branches in that
> order, and the conflict resolution should look OK.
>
> I however forked yours on v2.11.0-rc1, which would need to be
> rebased to one of the earlier maintenance tracks, before we can
> merge it to 'next'.
Yeah, I built it on top of master.
It does depend on some of the http-walker changes Eric made a few months
ago. In particular, 17966c0a6 (http: avoid disconnecting on 404s for
loose objects, 2016-07-11) added some checks against the HTTP status
code, and my series modifies the checks (mostly so that ">= 400" becomes
">= 300").
Rebasing on maint-2.9 means omitting those changes. That preserves the
security properties, but means that the error handling is worse when we
see an illegal redirect. That may be OK, though.
Since the resolution is to omit the changes entirely from my series,
merging up to v2.11 wouldn't produce any conflicts. We'd need to have a
separate set of patches adding those changes back in.
-Peff
^ permalink raw reply
* Re: "git add -p ." raises an unexpected "warning: empty strings as pathspecs will be made invalid in upcoming releases. please use . instead if you meant to match all paths"
From: Brandon Williams @ 2016-12-06 18:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Emily Xie
In-Reply-To: <xmqqtwaobni5.fsf@gitster.mtv.corp.google.com>
On 11/30, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> forgot to Cc: the author of the
> most relevant change to the issue, d426430e6e ("pathspec: warn on
> empty strings as pathspec", 2016-06-22).
>
> > Kevin Daudt <me@ikke.info> writes:
> >
> >> On Wed, Nov 30, 2016 at 12:31:49PM -0800, Peter Urda wrote:
> >>> After upgrading to version 2.11.0 I am getting a warning about empty
> >>> strings as pathspecs while using 'patch'
> >>>
> >>> - Ran 'git add -p .' from the root of my git repository.
> >>>
> >>> - I was able to normally stage my changes, but was presented with a
> >>> "warning: empty strings as pathspecs will be made invalid in upcoming
> >>> releases. please use . instead if you meant to match all paths"
> >>> message.
> >>>
> >>> - I expected no warning message since I included a "." with my original command.
> >>>
> >>> I believe that I should not be seeing this warning message as I
> >>> included the requested "." pathspec.
> >
> > Yes, this seems to be caused by pathspec.c::prefix_pathspec()
> > overwriting the original pathspec "." into "". The callchain
> > looks like this:
> >
> > builtin/add.c::interactive_add()
> > -> parse_pathspec()
> > passes argv[] that has "." to the caller,
> > receives pathspec whose pathspec->items[].original
> > is supposed to point at the unmolested original,
> > but prefix_pathspec() munges "." into ""
> > -> run_add_interactive()
> > which runs "git add--interactive" with
> > pathspec->items[].original as pathspecs
> >
> >
> > Perhaps this would work it around, but there should be a better way
> > to fix it (like, making sure that what we call "original" indeed
> > stays "original").
> >
> > builtin/add.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/add.c b/builtin/add.c
> > index e8fb80b36e..137097192d 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -167,9 +167,18 @@ int run_add_interactive(const char *revision, const char *patch_mode,
> > if (revision)
> > argv_array_push(&argv, revision);
> > argv_array_push(&argv, "--");
> > - for (i = 0; i < pathspec->nr; i++)
> > + for (i = 0; i < pathspec->nr; i++) {
> > /* pass original pathspec, to be re-parsed */
> > + if (!*pathspec->items[i].original) {
> > + /*
> > + * work around a misfeature in parse_pathspecs()
> > + * that munges "." into "".
> > + */
> > + argv_array_push(&argv, ".");
> > + continue;
> > + }
> > argv_array_push(&argv, pathspec->items[i].original);
> > + }
> >
> > status = run_command_v_opt(argv.argv, RUN_GIT_CMD);
> > argv_array_clear(&argv);
> > @@ -180,7 +189,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)
> > {
> > struct pathspec pathspec;
> >
> > - parse_pathspec(&pathspec, 0,
> > + parse_pathspec(&pathspec, 0,
> > PATHSPEC_PREFER_FULL |
> > PATHSPEC_SYMLINK_LEADING_PATH |
> > PATHSPEC_PREFIX_ORIGIN,
I've been doing a bit of work trying to clean up the pathspec
initialization code and I believe this can be fixed without
having to add in this work around. The code which does the munging is
always trying to prefix the pathspec regardless if there is a prefix or
not. If instead its changed to only try and prefix the original if
there is indeed a prefix, then it should fix the munging.
I'll try to get the series I'm working on out in the next day.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Junio C Hamano @ 2016-12-06 17:53 UTC (permalink / raw)
To: Jeff King; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <20161206135113.i7nlr45vg7uzgfcn@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I don't know if that makes things any easier. I feel funny saying "no,
> no, mine preempts yours because it is more maint-worthy", but I think
> that order does make sense.
>
> I think it would be OK to put Brandon's on maint, too, though. It is a
> refactor of an existing security feature to make it more featureful, but
> the way it is implemented could not cause security regressions unless
> you use the new feature (IOW, we still respect the whitelist environment
> exactly as before).
I think I merged yours and then Brandon's on jch/pu branches in that
order, and the conflict resolution should look OK.
I however forked yours on v2.11.0-rc1, which would need to be
rebased to one of the earlier maintenance tracks, before we can
merge it to 'next'.
^ permalink raw reply
* Re: Re: Feature request: Set git svn options in .git/config file
From: Juergen Kosel @ 2016-12-06 17:24 UTC (permalink / raw)
To: git
In-Reply-To: <20161205225424.GA29771@starla>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hello,
Am 05.12.2016 um 23:54 schrieb Eric Wong:
> So, can you confirm that svn.addAuthorFrom and svn.useLogAuthor
> config keys work and can be documented?
yes, I can confirm, that adding this configuration keys works with git
2.1.4 work.
I have added the config keys as follows:
git config --add --bool svn.useLogAuthor true
git config --add --bool svn.addAuthorFrom true
>
> Even better would be for you to provide a patch to the
> documentation :) Otherwise, I can write one up sometime this week.
My English is not that well. So I prefer, if you update the
documentation :-)
Greetings
Juergen
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Icedove - http://www.enigmail.net/
iD8DBQFYRvRhYZbcPghIM9IRAjtHAJ0QaqbUxcgoPXmidIg9WALXmg1JAQCfTHFj
wgPLKXK5Ny0bP/K9vpE9KzY=
=A+Yy
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH v4] diff: handle --no-abbrev in no-index case
From: Jack Bates @ 2016-12-06 17:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20161206165614.22921-1-jack@nottheoilrig.com>
On 06/12/16 09:56 AM, Jack Bates wrote:
> There are two different places where the --no-abbrev option is parsed,
> and two different places where SHA-1s are abbreviated. We normally parse
> --no-abbrev with setup_revisions(), but in the no-index case, "git diff"
> calls diff_opt_parse() directly, and diff_opt_parse() didn't handle
> --no-abbrev until now. (It did handle --abbrev, however.) We normally
> abbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 ("diff:
> handle sha1 abbreviations outside of repository, 2016-10-20) recently
> introduced a special case when you run "git diff" outside of a
> repository.
>
> setup_revisions() does also call diff_opt_parse(), but not for --abbrev
> or --no-abbrev, which it handles itself. setup_revisions() sets
> rev_info->abbrev, and later copies that to diff_options->abbrev. It
> handles --no-abbrev by setting abbrev to zero. (This change doesn't
> touch that.)
>
> Setting abbrev to zero was broken in the outside-of-a-repository special
> case, which until now resulted in a truly zero-length SHA-1, rather than
> taking zero to mean do not abbreviate. The only way to trigger this bug,
> however, was by running "git diff --raw" without either the --abbrev or
> --no-abbrev options, because 1) without --raw it doesn't respect abbrev
> (which is bizarre, but has been that way forever), 2) we silently clamp
> --abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until
> now.
>
> The outside-of-a-repository case is one of three no-index cases. The
> other two are when one of the files you're comparing is outside of the
> repository you're in, and the --no-index option.
>
> Signed-off-by: Jack Bates <jack@nottheoilrig.com>
> ---
> diff.c | 6 +++++-
> t/t4013-diff-various.sh | 7 +++++++
> t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir | 3 +++
> t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++
> t/t4013/diff.diff_--no-index_--raw_dir2_dir | 3 +++
> t/t4013/diff.diff_--raw_--abbrev=4_initial | 6 ++++++
> t/t4013/diff.diff_--raw_--no-abbrev_initial | 6 ++++++
> t/t4013/diff.diff_--raw_initial | 6 ++++++
> 8 files changed, 39 insertions(+), 1 deletion(-)
> create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
> create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
> create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
> create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
> create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
> create mode 100644 t/t4013/diff.diff_--raw_initial
>
> diff --git a/diff.c b/diff.c
> index ec87283..84dba60 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
> abbrev = FALLBACK_DEFAULT_ABBREV;
> if (abbrev > GIT_SHA1_HEXSZ)
> die("BUG: oid abbreviation out of range: %d", abbrev);
> - hex[abbrev] = '\0';
> + if (abbrev)
> + hex[abbrev] = '\0';
> return hex;
> }
> }
> @@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
>
> options->file = stdout;
>
> + options->abbrev = DEFAULT_ABBREV;
> options->line_termination = '\n';
> options->break_opt = -1;
> options->rename_limit = -1;
> @@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
> offending, optarg);
> return argcount;
> }
> + else if (!strcmp(arg, "--no-abbrev"))
> + options->abbrev = 0;
> else if (!strcmp(arg, "--abbrev"))
> options->abbrev = DEFAULT_ABBREV;
> else if (skip_prefix(arg, "--abbrev=", &arg)) {
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 566817e..d09acfe 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
> diff --dirstat master~1 master~2
> diff --dirstat initial rearrange
> diff --dirstat-by-file initial rearrange
> +# No-index --abbrev and --no-abbrev
I updated this comment to be consistent with the no-index vs.
outside-of-a-repository distinction in the commit message.
> +diff --raw initial
> +diff --raw --abbrev=4 initial
> +diff --raw --no-abbrev initial
> +diff --no-index --raw dir2 dir
> +diff --no-index --raw --abbrev=4 dir2 dir
> +diff --no-index --raw --no-abbrev dir2 dir
> EOF
>
> test_expect_success 'log -S requires an argument' '
> diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
> new file mode 100644
> index 0000000..a71b38a
> --- /dev/null
> +++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
> @@ -0,0 +1,3 @@
> +$ git diff --no-index --raw --abbrev=4 dir2 dir
> +:000000 100644 0000... 0000... A dir/sub
> +$
> diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
> new file mode 100644
> index 0000000..e0f0097
> --- /dev/null
> +++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
> @@ -0,0 +1,3 @@
> +$ git diff --no-index --raw --no-abbrev dir2 dir
> +:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A dir/sub
> +$
> diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
> new file mode 100644
> index 0000000..3cb4ee7
> --- /dev/null
> +++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
> @@ -0,0 +1,3 @@
> +$ git diff --no-index --raw dir2 dir
> +:000000 100644 0000000... 0000000... A dir/sub
> +$
> diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
> new file mode 100644
> index 0000000..c3641db
> --- /dev/null
> +++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
> @@ -0,0 +1,6 @@
> +$ git diff --raw --abbrev=4 initial
> +:100644 100644 35d2... 9929... M dir/sub
> +:100644 100644 01e7... 10a8... M file0
> +:000000 100644 0000... b1e6... A file1
> +:100644 000000 01e7... 0000... D file2
> +$
> diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
> new file mode 100644
> index 0000000..c87a125
> --- /dev/null
> +++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
> @@ -0,0 +1,6 @@
> +$ git diff --raw --no-abbrev initial
> +:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M dir/sub
> +:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M file0
> +:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A file1
> +:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D file2
> +$
> diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
> new file mode 100644
> index 0000000..a3e9780
> --- /dev/null
> +++ b/t/t4013/diff.diff_--raw_initial
> @@ -0,0 +1,6 @@
> +$ git diff --raw initial
> +:100644 100644 35d242b... 992913c... M dir/sub
> +:100644 100644 01e79c3... 10a8a9f... M file0
> +:000000 100644 0000000... b1e6722... A file1
> +:100644 000000 01e79c3... 0000000... D file2
> +$
>
^ permalink raw reply
* [PATCH v4] diff: handle --no-abbrev in no-index case
From: Jack Bates @ 2016-12-06 16:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jack Bates
In-Reply-To: <20161206010134.21856-1-jack@nottheoilrig.com>
There are two different places where the --no-abbrev option is parsed,
and two different places where SHA-1s are abbreviated. We normally parse
--no-abbrev with setup_revisions(), but in the no-index case, "git diff"
calls diff_opt_parse() directly, and diff_opt_parse() didn't handle
--no-abbrev until now. (It did handle --abbrev, however.) We normally
abbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 ("diff:
handle sha1 abbreviations outside of repository, 2016-10-20) recently
introduced a special case when you run "git diff" outside of a
repository.
setup_revisions() does also call diff_opt_parse(), but not for --abbrev
or --no-abbrev, which it handles itself. setup_revisions() sets
rev_info->abbrev, and later copies that to diff_options->abbrev. It
handles --no-abbrev by setting abbrev to zero. (This change doesn't
touch that.)
Setting abbrev to zero was broken in the outside-of-a-repository special
case, which until now resulted in a truly zero-length SHA-1, rather than
taking zero to mean do not abbreviate. The only way to trigger this bug,
however, was by running "git diff --raw" without either the --abbrev or
--no-abbrev options, because 1) without --raw it doesn't respect abbrev
(which is bizarre, but has been that way forever), 2) we silently clamp
--abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until
now.
The outside-of-a-repository case is one of three no-index cases. The
other two are when one of the files you're comparing is outside of the
repository you're in, and the --no-index option.
Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
diff.c | 6 +++++-
t/t4013-diff-various.sh | 7 +++++++
t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_dir2_dir | 3 +++
t/t4013/diff.diff_--raw_--abbrev=4_initial | 6 ++++++
t/t4013/diff.diff_--raw_--no-abbrev_initial | 6 ++++++
t/t4013/diff.diff_--raw_initial | 6 ++++++
8 files changed, 39 insertions(+), 1 deletion(-)
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
create mode 100644 t/t4013/diff.diff_--raw_initial
diff --git a/diff.c b/diff.c
index ec87283..84dba60 100644
--- a/diff.c
+++ b/diff.c
@@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
abbrev = FALLBACK_DEFAULT_ABBREV;
if (abbrev > GIT_SHA1_HEXSZ)
die("BUG: oid abbreviation out of range: %d", abbrev);
- hex[abbrev] = '\0';
+ if (abbrev)
+ hex[abbrev] = '\0';
return hex;
}
}
@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
options->file = stdout;
+ options->abbrev = DEFAULT_ABBREV;
options->line_termination = '\n';
options->break_opt = -1;
options->rename_limit = -1;
@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
offending, optarg);
return argcount;
}
+ else if (!strcmp(arg, "--no-abbrev"))
+ options->abbrev = 0;
else if (!strcmp(arg, "--abbrev"))
options->abbrev = DEFAULT_ABBREV;
else if (skip_prefix(arg, "--abbrev=", &arg)) {
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 566817e..d09acfe 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
diff --dirstat master~1 master~2
diff --dirstat initial rearrange
diff --dirstat-by-file initial rearrange
+# No-index --abbrev and --no-abbrev
+diff --raw initial
+diff --raw --abbrev=4 initial
+diff --raw --no-abbrev initial
+diff --no-index --raw dir2 dir
+diff --no-index --raw --abbrev=4 dir2 dir
+diff --no-index --raw --no-abbrev dir2 dir
EOF
test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
new file mode 100644
index 0000000..a71b38a
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --abbrev=4 dir2 dir
+:000000 100644 0000... 0000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
new file mode 100644
index 0000000..e0f0097
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --no-abbrev dir2 dir
+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
new file mode 100644
index 0000000..3cb4ee7
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw dir2 dir
+:000000 100644 0000000... 0000000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
new file mode 100644
index 0000000..c3641db
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --abbrev=4 initial
+:100644 100644 35d2... 9929... M dir/sub
+:100644 100644 01e7... 10a8... M file0
+:000000 100644 0000... b1e6... A file1
+:100644 000000 01e7... 0000... D file2
+$
diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
new file mode 100644
index 0000000..c87a125
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --no-abbrev initial
+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M dir/sub
+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D file2
+$
diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
new file mode 100644
index 0000000..a3e9780
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_initial
@@ -0,0 +1,6 @@
+$ git diff --raw initial
+:100644 100644 35d242b... 992913c... M dir/sub
+:100644 100644 01e79c3... 10a8a9f... M file0
+:000000 100644 0000000... b1e6722... A file1
+:100644 000000 01e79c3... 0000000... D file2
+$
--
2.10.2
^ permalink raw reply related
* [PATCH v4] diff: handle --no-abbrev in no-index case
From: Jack Bates @ 2016-12-06 16:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jack Bates
In-Reply-To: <20161206010134.21856-1-jack@nottheoilrig.com>
There are two different places where the --no-abbrev option is parsed,
and two different places where SHA-1s are abbreviated. We normally parse
--no-abbrev with setup_revisions(), but in the no-index case, "git diff"
calls diff_opt_parse() directly, and diff_opt_parse() didn't handle
--no-abbrev until now. (It did handle --abbrev, however.) We normally
abbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 ("diff:
handle sha1 abbreviations outside of repository, 2016-10-20) recently
introduced a special case when you run "git diff" outside of a
repository.
setup_revisions() does also call diff_opt_parse(), but not for --abbrev
or --no-abbrev, which it handles itself. setup_revisions() sets
rev_info->abbrev, and later copies that to diff_options->abbrev. It
handles --no-abbrev by setting abbrev to zero. (This change doesn't
touch that.)
Setting abbrev to zero was broken in the outside-of-a-repository special
case, which until now resulted in a truly zero-length SHA-1, rather than
taking zero to mean do not abbreviate. The only way to trigger this bug,
however, was by running "git diff --raw" without either the --abbrev or
--no-abbrev options, because 1) without --raw it doesn't respect abbrev
(which is bizarre, but has been that way forever), 2) we silently clamp
--abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until
now.
The outside-of-a-repository case is one of three no-index cases. The
other two are when one of the files you're comparing is outside of the
repository you're in, and the --no-index option.
Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
diff.c | 6 +++++-
t/t4013-diff-various.sh | 7 +++++++
t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_dir2_dir | 3 +++
t/t4013/diff.diff_--raw_--abbrev=4_initial | 6 ++++++
t/t4013/diff.diff_--raw_--no-abbrev_initial | 6 ++++++
t/t4013/diff.diff_--raw_initial | 6 ++++++
8 files changed, 39 insertions(+), 1 deletion(-)
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
create mode 100644 t/t4013/diff.diff_--raw_initial
diff --git a/diff.c b/diff.c
index ec87283..84dba60 100644
--- a/diff.c
+++ b/diff.c
@@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
abbrev = FALLBACK_DEFAULT_ABBREV;
if (abbrev > GIT_SHA1_HEXSZ)
die("BUG: oid abbreviation out of range: %d", abbrev);
- hex[abbrev] = '\0';
+ if (abbrev)
+ hex[abbrev] = '\0';
return hex;
}
}
@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
options->file = stdout;
+ options->abbrev = DEFAULT_ABBREV;
options->line_termination = '\n';
options->break_opt = -1;
options->rename_limit = -1;
@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
offending, optarg);
return argcount;
}
+ else if (!strcmp(arg, "--no-abbrev"))
+ options->abbrev = 0;
else if (!strcmp(arg, "--abbrev"))
options->abbrev = DEFAULT_ABBREV;
else if (skip_prefix(arg, "--abbrev=", &arg)) {
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 566817e..d7b71a0 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
diff --dirstat master~1 master~2
diff --dirstat initial rearrange
diff --dirstat-by-file initial rearrange
+# --abbrev and --no-abbrev outside of repository
+diff --raw initial
+diff --raw --abbrev=4 initial
+diff --raw --no-abbrev initial
+diff --no-index --raw dir2 dir
+diff --no-index --raw --abbrev=4 dir2 dir
+diff --no-index --raw --no-abbrev dir2 dir
EOF
test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
new file mode 100644
index 0000000..a71b38a
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --abbrev=4 dir2 dir
+:000000 100644 0000... 0000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
new file mode 100644
index 0000000..e0f0097
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --no-abbrev dir2 dir
+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
new file mode 100644
index 0000000..3cb4ee7
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw dir2 dir
+:000000 100644 0000000... 0000000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
new file mode 100644
index 0000000..c3641db
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --abbrev=4 initial
+:100644 100644 35d2... 9929... M dir/sub
+:100644 100644 01e7... 10a8... M file0
+:000000 100644 0000... b1e6... A file1
+:100644 000000 01e7... 0000... D file2
+$
diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
new file mode 100644
index 0000000..c87a125
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --no-abbrev initial
+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M dir/sub
+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D file2
+$
diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
new file mode 100644
index 0000000..a3e9780
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_initial
@@ -0,0 +1,6 @@
+$ git diff --raw initial
+:100644 100644 35d242b... 992913c... M dir/sub
+:100644 100644 01e79c3... 10a8a9f... M file0
+:000000 100644 0000000... b1e6722... A file1
+:100644 000000 01e79c3... 0000000... D file2
+$
--
2.10.2
^ permalink raw reply related
* Re: [PATCH] git-p4: add p4 shelf support
From: Vinicius Kursancew @ 2016-12-06 16:03 UTC (permalink / raw)
To: Luke Diamand; +Cc: Nuno Subtil, Git Users, Junio C Hamano, Lars Schneider
In-Reply-To: <CAE5ih79S+SFt-fsQ_2c4eXMankoXvoSE3zhxw39Y4XeQqQ9nMg@mail.gmail.com>
It seems if you do shelve and then later submit the p4 workspace is
left dirty (just like --prepare-p4-only would've done).
Vinicius Alexandre Kursancew <viniciusalexandre@gmail.com>
On Tue, Dec 6, 2016 at 8:36 AM, Luke Diamand <luke@diamand.org> wrote:
> On 6 December 2016 at 02:02, Nuno Subtil <subtil@gmail.com> wrote:
>> Extends the submit command to support shelving a commit instead of
>> submitting it to p4 (similar to --prepare-p4-only).
>
> Is this just the same as these two changes?
>
> http://www.spinics.net/lists/git/msg290755.html
> http://www.spinics.net/lists/git/msg291103.html
>
> Thanks,
> Luke
>
>>
>> Signed-off-by: Nuno Subtil <subtil@gmail.com>
>> ---
>> git-p4.py | 36 ++++++++++++++++++++++++++++++------
>> 1 file changed, 30 insertions(+), 6 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index fd5ca52..3c4be22 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1286,6 +1286,8 @@ def __init__(self):
>> optparse.make_option("--export-labels", dest="exportLabels", action="store_true"),
>> optparse.make_option("--dry-run", "-n", dest="dry_run", action="store_true"),
>> optparse.make_option("--prepare-p4-only", dest="prepare_p4_only", action="store_true"),
>> + optparse.make_option("--shelve-only", dest="shelve_only", action="store_true", help="Create P4 shelf for first change that would be submitted (using a new CL)"),
>> + optparse.make_option("--shelve-cl", dest="shelve_cl", help="Replace shelf under existing CL number (previously shelved files will be deleted)"),
>> optparse.make_option("--conflict", dest="conflict_behavior",
>> choices=self.conflict_behavior_choices),
>> optparse.make_option("--branch", dest="branch"),
>> @@ -1297,6 +1299,8 @@ def __init__(self):
>> self.preserveUser = gitConfigBool("git-p4.preserveUser")
>> self.dry_run = False
>> self.prepare_p4_only = False
>> + self.shelve_only = False
>> + self.shelve_cl = None
>> self.conflict_behavior = None
>> self.isWindows = (platform.system() == "Windows")
>> self.exportLabels = False
>> @@ -1496,6 +1500,12 @@ def prepareSubmitTemplate(self):
>> else:
>> inFilesSection = False
>> else:
>> + if self.shelve_only and self.shelve_cl:
>> + if line.startswith("Change:"):
>> + line = "Change: %s\n" % self.shelve_cl
>> + if line.startswith("Status:"):
>> + line = "Status: pending\n"
>> +
>> if line.startswith("Files:"):
>> inFilesSection = True
>>
>> @@ -1785,7 +1795,11 @@ def applyCommit(self, id):
>> if self.isWindows:
>> message = message.replace("\r\n", "\n")
>> submitTemplate = message[:message.index(separatorLine)]
>> - p4_write_pipe(['submit', '-i'], submitTemplate)
>> +
>> + if self.shelve_only:
>> + p4_write_pipe(['shelve', '-i', '-r'], submitTemplate)
>> + else:
>> + p4_write_pipe(['submit', '-i'], submitTemplate)
>>
>> if self.preserveUser:
>> if p4User:
>> @@ -1799,12 +1813,17 @@ def applyCommit(self, id):
>> # new file. This leaves it writable, which confuses p4.
>> for f in pureRenameCopy:
>> p4_sync(f, "-f")
>> - submitted = True
>> +
>> + if not self.shelve_only:
>> + submitted = True
>>
>> finally:
>> # skip this patch
>> if not submitted:
>> - print "Submission cancelled, undoing p4 changes."
>> + if not self.shelve_only:
>> + print "Submission cancelled, undoing p4 changes."
>> + else:
>> + print "Change shelved, undoing p4 changes."
>> for f in editedFiles:
>> p4_revert(f)
>> for f in filesToAdd:
>> @@ -2034,9 +2053,13 @@ def run(self, args):
>> if ok:
>> applied.append(commit)
>> else:
>> - if self.prepare_p4_only and i < last:
>> - print "Processing only the first commit due to option" \
>> - " --prepare-p4-only"
>> + if (self.prepare_p4_only or self.shelve_only) and i < last:
>> + if self.prepare_p4_only:
>> + print "Processing only the first commit due to option" \
>> + " --prepare-p4-only"
>> + else:
>> + print "Processing only the first commit due to option" \
>> + " --shelve-only"
>> break
>> if i < last:
>> quit = False
>> @@ -3638,6 +3661,7 @@ def printUsage(commands):
>> "debug" : P4Debug,
>> "submit" : P4Submit,
>> "commit" : P4Submit,
>> + "shelve" : P4Submit,
>> "sync" : P4Sync,
>> "rebase" : P4Rebase,
>> "clone" : P4Clone,
>>
>> --
>> https://github.com/git/git/pull/309
^ permalink raw reply
* [PATCH] stash: disable renames when calling git-diff
From: Jeff King @ 2016-12-06 15:41 UTC (permalink / raw)
To: Matthew Patey; +Cc: git
In-Reply-To: <20161206152530.snccf7buiosst3e4@sigill.intra.peff.net>
On Tue, Dec 06, 2016 at 10:25:30AM -0500, Jeff King wrote:
> I agree that the second stash not saving the deletion seems like a bug.
> Oddly, just stashing a deletion, like:
>
> rm a
> git stash
> git stash apply
>
> does store it. So it's not like we can't represent the deletion. Somehow
> the presence of "b" in the index matters.
>
> It looks like the culprit may be this part of create_stash():
>
> git diff --name-only -z HEAD -- >"$TMP-stagenames"
>
> That is using the "git diff" porcelain, which will respect renames, and
> the --name-only bit mentions only "b".
>
> If you run:
>
> git -c diff.renames=false stash
>
> then it works.
And here's a patch to fix it.
-- >8 --
Subject: [PATCH] stash: disable renames when calling git-diff
When creating a stash, we need to look at the diff between
the working tree and HEAD. There's no plumbing command that
covers this case, so we do so by calling the git-diff
porcelain. This means we also inherit the usual list of
porcelain options, which can impact the output in confusing
ways.
There's at least one known problem: --name-only will not
mention the source side of a rename, meaning that we will
fail to stash a deletion in such a case.
The correct solution is to move to an appropriate plumbing
command. But since there isn't one available, in the short
term we can plug this particular problem by manually asking
git-diff not to respect renames.
Reported-by: Matthew Patey <matthew.patey2167@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
There may be other similar problems lurking depending on the various
config options you have set, but I don't think so. Most of the options
only affect --patch output.
I do find it interesting that --name-only does not mention the source
side of a rename. The longer forms like --name-status mention both
sides. Certainly --name-status does not have any way of saying "this is
the source side of a rename", but I'd think it would simply mention both
sides. Anyway, even if that were changed (which would fix this bug), I
think adding --no-renames is still a good thing to be doing here.
git-stash.sh | 2 +-
t/t3903-stash.sh | 9 +++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/git-stash.sh b/git-stash.sh
index 4546abaae..40ab2710e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -115,7 +115,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
- git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
+ git diff --name-only -z --no-renames HEAD -- >"$TMP-stagenames" &&
git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
git write-tree &&
rm -f "$TMPindex"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index e1a6ccc00..2de3e18ce 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -766,4 +766,13 @@ test_expect_success 'stash list --cc shows combined diff' '
test_cmp expect actual
'
+test_expect_success 'stash is not confused by partial renames' '
+ mv file renamed &&
+ git add renamed &&
+ git stash &&
+ git stash apply &&
+ test_path_is_file renamed &&
+ test_path_is_missing file
+'
+
test_done
--
2.11.0.191.gdb26c57
^ permalink raw reply related
* Re: Bug: stash staged file move loses original file deletion
From: Jeff King @ 2016-12-06 15:25 UTC (permalink / raw)
To: Matthew Patey; +Cc: git
In-Reply-To: <CAFQpxx+PJ3FSoH9DFWyEw+ZLagji9Qou+aY9EB8A+=t+QX0o2A@mail.gmail.com>
On Tue, Dec 06, 2016 at 02:45:05PM +0000, Matthew Patey wrote:
> Thanks for the reply! I agree that it is weird that applying a stash with a
> move only puts the addition back in the index, and thanks for the tip on
> "index" to properly apply that. For this case I was focusing on the
> behavior of the second stash/apply, with the first stash/apply as an
> example of how to get into a weird state that triggers the odd behavior of
> the second.
Oh, heh, I totally missed that.
I agree that the second stash not saving the deletion seems like a bug.
Oddly, just stashing a deletion, like:
rm a
git stash
git stash apply
does store it. So it's not like we can't represent the deletion. Somehow
the presence of "b" in the index matters.
It looks like the culprit may be this part of create_stash():
git diff --name-only -z HEAD -- >"$TMP-stagenames"
That is using the "git diff" porcelain, which will respect renames, and
the --name-only bit mentions only "b".
If you run:
git -c diff.renames=false stash
then it works.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Jeff King @ 2016-12-06 15:09 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1612061512190.117539@virtualbox>
On Tue, Dec 06, 2016 at 03:48:38PM +0100, Johannes Schindelin wrote:
> > Should it blindly look at ".git/config"?
>
> Absolutely not, of course. You did not need me to say that.
>
> > Now your program behaves differently depending on whether you are in the
> > top-level of the working tree.
>
> Exactly. This, BTW, is already how the code would behave if anybody called
> `git_path()` before `setup_git_directory()`, as the former function
> implicitly calls `setup_git_env()` which does *not* call
> `setup_git_directory()` but *does* set up `git_dir` which is then used by
> `do_git_config_sequence()`..
>
> We have a few of these nasty surprises in our code base, where code
> silently assumes that global state is set up correctly, and succeeds in
> sometimes surprising ways if it is not.
Right. I have been working on fixing this. v2.11 has a ton of tweaks in
this area, and my patch to die() rather than default to ".git" is
cooking in next to catch any stragglers.
> > Should it speculatively do repo discovery, and use any discovered config
> > file?
>
> Personally, I find the way we discover the repository most irritating. It
> seems that we have multiple, mutually incompatible code paths
> (`setup_git_directory()` and `setup_git_env()` come to mind already, and
> it does not help that consecutive calls to `setup_git_directory()` will
> yield a very unexpected outcome).
I agree. We should be killing off setup_git_env(), which is something
I've been slowly working towards over the years.
There are some annoyances with setup_git_directory(), too (like the fact
that you cannot ask "is there a git repository you can find" without
making un-recoverable changes to the process state). I think we should
fix those, too.
> > Now some commands respect config that they shouldn't (e.g., running "git
> > init foo.git" from inside another repository will accidentally pick up
> > the value of core.sharedrepository from wherever you happen to run it).
>
> Right. That points to another problem with the way we do things: we leak
> global state from discovering a git_dir, which means that we can neither
> undo nor override it.
>
> If we discovered our git_dir in a robust manner, `git init` could say:
> hey, this git_dir is actually not what I wanted, here is what I want.
>
> Likewise, `git submodule` would eventually be able to run in the very same
> process as the calling `git`, as would a local fetch.
Yep, I agree with all that. I do think things _have_ been improving over
the years, and the code is way less tangled than it used to be. But
there are so many corner cases, and the code is so fundamental, that it
has been slow going. I'd be happy to review patches if you want to push
it along.
> > So I think the caller of the config code has to provide some kind of
> > context about how it is expecting to run and how the value will be used.
>
> Yep.
>
> Maybe even go a step further and say that the config code needs a context
> "object".
If I were writing git from scratch, I'd consider making a "struct
repository" object. I'm not sure how painful it would be to retro-fit it
at this point.
> > Right now if setup_git_directory() or similar hasn't been called, the
> > config code does not look.
>
> Correct.
>
> Actually, half correct. If setup_git_directory() has not been called, but,
> say, git_path() (and thereby implicitly setup_git_env()), the config code
> *does* look. At a hard-coded .git/config.
Not since b9605bc4f (config: only read .git/config from configured
repos, 2016-09-12). That's why pager.c needs its little hack.
I guess you could see that as a step backwards, but I think it is
necessary one on the road to doing it right.
> > Ideally there would be a way for a caller to say "I am running early and
> > not even sure yet if we are in a repo; please speculatively try to find
> > repo config for me".
>
> And ideally, it would not roll *yet another* way to discover the git_dir,
> but it would reuse the same function (fixing it not to chdir()
> unilaterally).
Yes, absolutely.
> Of course, not using `chdir()` means that we have to figure out symbolic
> links somehow, in case somebody works from a symlinked subdirectory, e.g.:
>
> ln -s $PWD/t/ ~/test-directory
> cd ~/test-directory
> git log
There's work happening elsewhere[1] on making real_path() work without
calling chdir(). Which necessarily involves resolving symlinks
ourselves. I wonder if we could leverage that work here (ideally by
using real_path() under the hood, and not reimplementing the same
readlink() logic ourselves).
[1] http://public-inbox.org/git/1480964316-99305-1-git-send-email-bmwill@google.com/
> > The pager code does this manually, and without great accuracy; see the
> > hack in pager.c's read_early_config().
>
> I saw it. And that is what triggered the mail you are responding to (you
> probably saw my eye-rolling between the lines).
>
> The real question is: can we fix this? Or is there simply too great
> reluctance to change the current code?
The code in pager.c is only a month or two old. Like I said, it's ugly,
but I think it's a necessary step on the way forward. So I don't think
there's reluctance at all. The next steps (which I outlined) just
haven't been taken yet.
> > I think the way forward is:
> >
> > 1. Make that an optional behavior in git_config_with_options() so
> > other spots can reuse it (probably alias lookup, and something like
> > your difftool config).
>
> Ideally: *any* early call to `git_config_get_value()`. Make things less
> surprising.
I'm not convinced that's a good idea. The changes in b9605bc4f were
motivated by a real bug, which your suggestion would reintroduce (namely
low-level code run by git-init ending up with config variables from a
repo that _should_ be unrelated).
In my mental model, the cases are:
1. We are "early" in the process, before we know if we have a repo or
not. These early looks should speculatively look at repo config,
which is confined to generic things like pager config, alias
config, etc.
2. We are in a repo. Obviously look at $GIT_DIR/config.
3. We are in a program which has done setup and determined we are
_not_ in a repo. Definitely do not look at .git/config or anything
else.
My plan was for the config code to default to (3) when we are not in a
repo, but let some lookups ask specifically for (1).
If you want to default to (1), you need some way for programs to say "I
am really case (3); do not look for a repo". And it needs to be global,
as the config lookup may be done by much lower-level code. That could be
by turning startup_info->have_repository into a tri-state. It just
wasn't the way I was planning on it.
> > 2. Make it more accurate. Right now it blindly looks in .git/config,
> > but it should be able to do the usual repo-detection (_without_
> > actually entering the repo) to try to find a possible config file.
>
> The real trick will be to convince Junio to have a single function for
> git_dir discovery, I guess, lest we have multiple, slightly incompatible
> ways to discover it. I expect a lot of resistance here, because we would
> have to change tried-and-tested (if POLA-violating) code.
Personally, I haven't seen any resistance from Junio on refactoring this
area. I'm sure he is concerned that we do not regress, but it's not like
the area has been unchanged over the years. It has been slow going
because we want to do it carefully, but I think we are actually at the
point now where the next step is making setup_git_directory() more sane.
-Peff
^ permalink raw reply
* inconsistent output from git add about ignored files
From: Yaroslav Halchenko @ 2016-12-06 14:18 UTC (permalink / raw)
To: git; +Cc: Benjamin Poldrack
Dear Git gurus,
It seems that there is some inconsistency in output of git while it is
ignoring files:
if a single path within ignored directory is provided -- git outputs
the file path. If multiple files are provided (some of which aren't
ignored) -- git outputs only the ignored directory name:
% git --version
git version 2.10.2
% git status
On branch master
Untracked files:
(use "git add <file>..." to include in what will be committed)
bu
nothing added to commit but untracked files present (use "git add" to track)
% cat .gitignore
*.me
% git add blah.me/bu
The following paths are ignored by one of your .gitignore files:
blah.me/bu
Use -f if you really want to add them.
% git add blah.me/bu bu
The following paths are ignored by one of your .gitignore files:
blah.me
Use -f if you really want to add them.
% git status
On branch master
Changes to be committed:
(use "git reset HEAD <file>..." to unstage)
new file: bu
So note that in the first case it reports "blah.me/bu" whenever in the
second -- only "blah.me".
P.S. Please CC us in your replies.
With best regards,
--
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2016-12-06 14:48 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <20161206133650.t7gkg4f6wzw3zxki@sigill.intra.peff.net>
Hi Peff,
On Tue, 6 Dec 2016, Jeff King wrote:
> On Tue, Dec 06, 2016 at 02:16:35PM +0100, Johannes Schindelin wrote:
>
> > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > >
> > > > Seriously, do you really think it is a good idea to have
> > > > git_config_get_value() *ignore* any value in .git/config?
> > >
> > > When we do not know ".git/" directory we see is the repository we
> > > were told to work on, then we should ignore ".git/config" file. So
> > > allowing git_config_get_value() to _ignore_ ".git/config" before the
> > > program calls setup_git_directory() does have its uses.
> >
> > I think you are wrong. This is yet another inconsistent behavior that
> > violates the Law of Least Surprises.
>
> There are surprises in this code any way you turn. If we have not
> called setup_git_directory(), then how does git_config_get_value() know
> if we are in a repository or not?
My biggest surprise, frankly, would be that `git init` and `git clone` are
not special-cased.
> Should it blindly look at ".git/config"?
Absolutely not, of course. You did not need me to say that.
> Now your program behaves differently depending on whether you are in the
> top-level of the working tree.
Exactly. This, BTW, is already how the code would behave if anybody called
`git_path()` before `setup_git_directory()`, as the former function
implicitly calls `setup_git_env()` which does *not* call
`setup_git_directory()` but *does* set up `git_dir` which is then used by
`do_git_config_sequence()`..
We have a few of these nasty surprises in our code base, where code
silently assumes that global state is set up correctly, and succeeds in
sometimes surprising ways if it is not.
> Should it speculatively do repo discovery, and use any discovered config
> file?
Personally, I find the way we discover the repository most irritating. It
seems that we have multiple, mutually incompatible code paths
(`setup_git_directory()` and `setup_git_env()` come to mind already, and
it does not help that consecutive calls to `setup_git_directory()` will
yield a very unexpected outcome).
Just try to explain to a veteran software engineer why you cannot call
`setup_git_directory_gently()` multiple times and expect the very same
return value every time.
Another irritation is that some commands that clearly would like to use a
repository if there is one (such as `git diff`) are *not* marked with
RUN_SETUP_GENTLY, due to these unfortunate implementation details.
> Now some commands respect config that they shouldn't (e.g., running "git
> init foo.git" from inside another repository will accidentally pick up
> the value of core.sharedrepository from wherever you happen to run it).
Right. That points to another problem with the way we do things: we leak
global state from discovering a git_dir, which means that we can neither
undo nor override it.
If we discovered our git_dir in a robust manner, `git init` could say:
hey, this git_dir is actually not what I wanted, here is what I want.
Likewise, `git submodule` would eventually be able to run in the very same
process as the calling `git`, as would a local fetch.
> So I think the caller of the config code has to provide some kind of
> context about how it is expecting to run and how the value will be used.
Yep.
Maybe even go a step further and say that the config code needs a context
"object".
> Right now if setup_git_directory() or similar hasn't been called, the
> config code does not look.
Correct.
Actually, half correct. If setup_git_directory() has not been called, but,
say, git_path() (and thereby implicitly setup_git_env()), the config code
*does* look. At a hard-coded .git/config.
> Ideally there would be a way for a caller to say "I am running early and
> not even sure yet if we are in a repo; please speculatively try to find
> repo config for me".
And ideally, it would not roll *yet another* way to discover the git_dir,
but it would reuse the same function (fixing it not to chdir()
unilaterally).
Of course, not using `chdir()` means that we have to figure out symbolic
links somehow, in case somebody works from a symlinked subdirectory, e.g.:
ln -s $PWD/t/ ~/test-directory
cd ~/test-directory
git log
> The pager code does this manually, and without great accuracy; see the
> hack in pager.c's read_early_config().
I saw it. And that is what triggered the mail you are responding to (you
probably saw my eye-rolling between the lines).
The real question is: can we fix this? Or is there simply too great
reluctance to change the current code?
> I think the way forward is:
>
> 1. Make that an optional behavior in git_config_with_options() so
> other spots can reuse it (probably alias lookup, and something like
> your difftool config).
Ideally: *any* early call to `git_config_get_value()`. Make things less
surprising.
> 2. Make it more accurate. Right now it blindly looks in .git/config,
> but it should be able to do the usual repo-detection (_without_
> actually entering the repo) to try to find a possible config file.
The real trick will be to convince Junio to have a single function for
git_dir discovery, I guess, lest we have multiple, slightly incompatible
ways to discover it. I expect a lot of resistance here, because we would
have to change tried-and-tested (if POLA-violating) code.
Ciao,
Dscho
^ permalink raw reply
* Re: Bug: stash staged file move loses original file deletion
From: Jeff King @ 2016-12-06 14:24 UTC (permalink / raw)
To: Matthew Patey; +Cc: git
In-Reply-To: <CAFQpxxKbn4vBMzVcLZgBVvuL2fsOGNMHR1WC+aTOG_RAWkZ_Gg@mail.gmail.com>
On Mon, Dec 05, 2016 at 09:37:51AM -0500, Matthew Patey wrote:
> Git version 2.8.0 (sorry can't update to more recent on my machine) on Ubuntu.
The behavior is the same on more recent versions of git, too. The short
answer is "use --index". The longer one is below.
> After moving a file, if the new file is in the index but the deletion
> of the old file is not staged, git stash loses the deletion operation.
> Repro:
>
> 1. git mv a b
> This will have both the "deletion" and the "file added" in the index
>
> 2. git stash; git stash pop
> Now file a is still in the index, but file b deletion is not staged.
>
> 3. git stash; git stash show -v
> This will show that the deletion operation is not in the stash
>
> 4. git stash pop
> Again confirms the issue, file a is in the index, but file b is
> present and unmodified in the working directory.
Thanks for a clear reproduction case. I think the oddball, though, is
not that "b" is not staged for deletion, but that the addition of "a"
_is_ staged.
Applying a stash usually does not re-stage index contents, unless you
specify --index. For example, try:
# Make a staged change
echo change >>a
git add a
# This puts the change back into the working tree, but does _not_
# put it into the index.
git stash apply
# Now reset to try again.
git reset --hard
# This does restore the index.
git stash apply --index
So in your case, the deletion of "b" is following that same rule. What's
unusual is that "a" is staged. There's code specifically in git-stash
specifically to make sure this is the case, but I don't remember offhand
why this is so. The code comes from the original f2c66ed19 (Add
git-stash script, 2007-06-30), which in turn seems to come from Junio's
comments in:
http://public-inbox.org/git/7vmyyq2zrz.fsf@assigned-by-dhcp.pobox.com/
I don't see any specific reasoning, but I think it is simply that it is
confusing for the files to become untracked totally. These days we have
intent-to-add via "git add -N", so that might actually be a better
choice for this case.
Anyway, that history digression is neither here nor there for your case.
If you want to restore the index contents, use "--index". That does what
you were expecting:
$ git mv a b
$ git stash && git stash apply --index
Saved working directory and index state WIP on master: 5355755 foo
HEAD is now at 5355755 foo
On branch master
Changes to be committed:
renamed: a -> b
-Peff
^ permalink raw reply
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Jeff King @ 2016-12-06 14:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, GIT Mailing-list
In-Reply-To: <xmqqbmwq5k7k.fsf@gitster.mtv.corp.google.com>
On Mon, Dec 05, 2016 at 10:01:19AM -0800, Junio C Hamano wrote:
> > That said, I think the right patch may be to just drop --abbrev
> > entirely.
> > ...
> > I think at that point it was a noop, as 7 should have been the default.
> > And now we probably ought to drop it, so that we can use the
> > auto-scaling default.
>
> Yeah, I agree.
>
> It does mean that snapshot binaries built out of the same commit in
> the same repository before and after a repack have higher chances of
> getting named differently, which may surprise people, but that
> already is possible with a fixed length if the repacking involves
> pruning (albeit with lower probabilities), and I do not think it is
> a problem.
I think that the number is already unstable, even with --abbrev, as it
just specifies a minimum. So any operation that creates objects has a
possibility of increasing the length. Or more likely, two people
describing the same version may end up with different strings because
they have different objects in their repositories (e.g., I used to
carry's trast's git-notes archive of the mailing list which added quite
a few objects).
I agree that having it change over the course of a repack is slightly
_more_ surprising than those cases, but ultimately the value just isn't
stable.
-Peff
^ permalink raw reply
* Re: git add -p doesn't honor diff.noprefix config
From: Jeff King @ 2016-12-06 13:56 UTC (permalink / raw)
To: paddor; +Cc: git
In-Reply-To: <E1D7329A-A54B-4D09-A72A-62ECA8005752@gmail.com>
On Sat, Dec 03, 2016 at 07:45:18AM +0100, paddor wrote:
> I set the config diff.noprefix = true because I don't like the a/ and
> b/ prefixes, which nicely changed the output of `git diff`.
> Unfortunately, the filenames in the output of `git add --patch` are
> still prefixed.
>
> To me, this seems like a bug. Or there's a config option missing.
The interactive-add process is a perl script built around plumbing
commands like diff-tree, diff-files, etc. Plumbing commands do not
respect some config options, so that the output remains stable or
scripts built around them. And diff.noprefix is one of these. So scripts
have to get the value themselves and decide whether to pass it along to
the plumbing.
In this case, I think there are two steps needed:
1. Confirm that git-add--interactive.perl can actually handle
no-prefix patches. It feeds the patches back to git-apply, which
may be a complication (so it may need, for example, to pass a
special flag to git-apply).
2. git-add--interactive.perl needs to parse the config value, and if
set, pass the appropriate option to the diff plumbing. This should
only be one or two lines; see how $diff_algorithm is handled in
that script.
-Peff
^ permalink raw reply
* Re: [PATCH v2 0/6] shallow.c improvements
From: Duy Nguyen @ 2016-12-06 13:47 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List, Rasmus Villemoes
In-Reply-To: <20161206134212.mttcb75dov2jvqu5@sigill.intra.peff.net>
On Tue, Dec 6, 2016 at 8:42 PM, Jeff King <peff@peff.net> wrote:
> The final one _seems_ reasonable after reading your explanation, but I
> lack enough context to know whether or not there might be a corner case
> that you're missing. I'm inclined to trust your assessment on it.
Yeah I basically just wrote down my thoughts so somebody could maybe
spot something wrong. I'm going to think about it some more in the
next few days.
--
Duy
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Jeff King @ 2016-12-06 13:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <xmqqr35m3zx7.fsf@gitster.mtv.corp.google.com>
On Mon, Dec 05, 2016 at 12:04:52PM -0800, Junio C Hamano wrote:
> > I'm sending out another reroll of this series so that in Jeff's he can
> > just call 'get_curl_allowed_protocols(-1)' for the non-redirection curl
> > option, which should make this test stop barfing.
>
> I was hoping to eventually merge Peff's series to older maintenance
> tracks. How bad would it be if we rebased the v8 of this series
> together with Peff's series to say v2.9 (or even older if it does
> not look too bad)?
My series actually fixes existing security problems, so I'd consider it
a bug-fix. I _think_ Brandon's series is purely about allowing more
expressiveness in the whitelist policy, and so could be considered more
of a feature.
So one option is to apply my series for older 'maint', and then just
rebase Brandon's on top of that for 'master'.
I don't know if that makes things any easier. I feel funny saying "no,
no, mine preempts yours because it is more maint-worthy", but I think
that order does make sense.
I think it would be OK to put Brandon's on maint, too, though. It is a
refactor of an existing security feature to make it more featureful, but
the way it is implemented could not cause security regressions unless
you use the new feature (IOW, we still respect the whitelist environment
exactly as before).
-Peff
^ permalink raw reply
* Re: [PATCH] difftool: fix dir-diff index creation when in a subdirectory
From: Johannes Schindelin @ 2016-12-06 13:46 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, Git ML, Frank Becker, John Keeping
In-Reply-To: <20161205222600.29914-1-davvid@gmail.com>
Hi David,
On Mon, 5 Dec 2016, David Aguilar wrote:
> 9ec26e797781239b36ebccb87c590e5778358007 corrected how path arguments
How about using the "whatis" instead, i.e. "9ec26e7977 (difftool: fix
argument handling in subdirs, 2016-07-18)"?
Ciao,
Dscho
^ permalink raw reply
* [PATCH v2 0/6] shallow.c improvements
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <1480710664-26290-1-git-send-email-rv@rasmusvillemoes.dk>
After staring not-so-hard and not-for-so-long at the code. This is
what I come up with. Rasmus I replaced two of your commits with my
own (and thank you for giving me an opportunity to refresh my memory
with this stuff). The first two commits are new and the result of
Jeff's observation on COMMIT_SLAB_SIZE.
You may find the description here a bit different from my explanation
previously (about "exclude/shallow requests"). Well.. I was wrong.
I had the recent --exclude-tag and friends in mind, but this is about
clone/fetch/push from/to a shallow repository since 2013, no wonder I
don't remember much about it :-D
Nguyễn Thái Ngọc Duy (4):
shallow.c: rename fields in paint_info to better express their purposes
shallow.c: stop abusing COMMIT_SLAB_SIZE for paint_info's memory pools
shallow.c: make paint_alloc slightly more robust
shallow.c: remove useless code
Rasmus Villemoes (2):
shallow.c: avoid theoretical pointer wrap-around
shallow.c: bit manipulation tweaks
shallow.c | 39 ++++++++++++++++++++-------------------
1 file changed, 20 insertions(+), 19 deletions(-)
--
2.8.2.524.g6ff3d78
^ permalink raw reply
* Re: [PATCH v2 0/6] shallow.c improvements
From: Jeff King @ 2016-12-06 13:42 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, rv
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
On Tue, Dec 06, 2016 at 07:53:33PM +0700, Nguyễn Thái Ngọc Duy wrote:
> Nguyễn Thái Ngọc Duy (4):
> shallow.c: rename fields in paint_info to better express their purposes
> shallow.c: stop abusing COMMIT_SLAB_SIZE for paint_info's memory pools
> shallow.c: make paint_alloc slightly more robust
> shallow.c: remove useless code
>
> Rasmus Villemoes (2):
> shallow.c: avoid theoretical pointer wrap-around
> shallow.c: bit manipulation tweaks
The first 5 patches look obviously good to me. The naming changes in
paint_alloc() make things much clearer, and the fixes retained from
Rasmus are all obvious improvements.
The final one _seems_ reasonable after reading your explanation, but I
lack enough context to know whether or not there might be a corner case
that you're missing. I'm inclined to trust your assessment on it.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Jeff King @ 2016-12-06 13:36 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Junio C Hamano, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <alpine.DEB.2.20.1612061411000.117539@virtualbox>
On Tue, Dec 06, 2016 at 02:16:35PM +0100, Johannes Schindelin wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> > > Seriously, do you really think it is a good idea to have
> > > git_config_get_value() *ignore* any value in .git/config?
> >
> > When we do not know ".git/" directory we see is the repository we were
> > told to work on, then we should ignore ".git/config" file. So allowing
> > git_config_get_value() to _ignore_ ".git/config" before the program
> > calls setup_git_directory() does have its uses.
>
> I think you are wrong. This is yet another inconsistent behavior that
> violates the Law of Least Surprises.
There are surprises in this code any way you turn. If we have not
called setup_git_directory(), then how does git_config_get_value() know
if we are in a repository or not?
Should it blindly look at ".git/config"? Now your program behaves
differently depending on whether you are in the top-level of the working
tree.
Should it speculatively do repo discovery, and use any discovered config
file? Now some commands respect config that they shouldn't (e.g.,
running "git init foo.git" from inside another repository will
accidentally pick up the value of core.sharedrepository from wherever
you happen to run it).
So I think the caller of the config code has to provide some kind of
context about how it is expecting to run and how the value will be used.
Right now if setup_git_directory() or similar hasn't been called, the
config code does not look. Ideally there would be a way for a caller to
say "I am running early and not even sure yet if we are in a repo;
please speculatively try to find repo config for me".
The pager code does this manually, and without great accuracy; see the
hack in pager.c's read_early_config(). I think the way forward is:
1. Make that an optional behavior in git_config_with_options() so
other spots can reuse it (probably alias lookup, and something like
your difftool config).
2. Make it more accurate. Right now it blindly looks in .git/config,
but it should be able to do the usual repo-detection (_without_
actually entering the repo) to try to find a possible config file.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2016-12-06 13:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <xmqqy3zu43yk.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 5 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Seriously, do you really think it is a good idea to have
> > git_config_get_value() *ignore* any value in .git/config?
>
> When we do not know ".git/" directory we see is the repository we were
> told to work on, then we should ignore ".git/config" file. So allowing
> git_config_get_value() to _ignore_ ".git/config" before the program
> calls setup_git_directory() does have its uses.
I think you are wrong. This is yet another inconsistent behavior that
violates the Law of Least Surprises.
> > We need to fix this.
>
> I have a feeling that "environment modifications that cannot be undone"
> we used as the rationale in 73c2779f42 ("builtin-am: implement skeletal
> builtin am", 2015-08-04) might be overly pessimistic and in order to
> implement undo_setup_git_directory(), all we need to do may just be
> matter of doing a chdir(2) back to prefix and unsetting GIT_PREFIX
> environment, but I haven't looked into details of the setup sequence
> recently.
The problem is that we may not know enough anymore to "undo
setup_git_directory()", as some environment variables may have been set
before calling Git. If we simply unset the environment variables, we do it
incorrectly. On the other hand, the environment variables *may* have been
set by setup_git_directory(). In which case we *do* have to unset them.
The entire setup_git_directory() logic is a bit of a historically-grown
garden.
Ciao,
Dscho
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox