* Re: [PATCH v2 1/4] config: factor out config file stack management
From: Jeff King @ 2013-03-12 19:04 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130312154434.GB3021@sandbox-ub.fritz.box>
On Tue, Mar 12, 2013 at 04:44:35PM +0100, Heiko Voigt wrote:
> > Can we throw in a comment at the top here with the expected usage? In
> > particular, do_config_from is expecting the caller to have filled in
> > certain fields (at this point, top->f and top->name), but there is
> > nothing to make that clear.
>
> Of course. Will do that in the next iteration. How about I squash this in:
> [...]
> +/* The fields data, name and the source specific callbacks of top need
> + * to be initialized before calling this function.
> + */
> static int do_config_from_source(struct config_source *top, config_fn_t fn, voi
I think that is OK, but it may be even better to list the fields by
name. Also, our multi-line comment style is:
/*
* Multi-line comment.
*/
> I would add that to the third patch:
>
> config: make parsing stack struct independent from actual data source
>
> because that contains the final modification to config_file/config_source.
It does not matter to the end result, but I find it helps with reviewing
when the comment is added along with the function, and then expanded as
the function is changed. It helps to understand the effects of later
patches if they need to tweak comments.
I do not care that much in this instance, since we have already
discussed it, and I know what is going on, though.
-Peff
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: John Keeping @ 2013-03-12 19:09 UTC (permalink / raw)
To: Matt McClure; +Cc: David Aguilar, git@vger.kernel.org, Tim Henigan
In-Reply-To: <CAJELnLGOK5m-JLwgfUdmQcS1exZMQdf1QR_g-GB_UhryDw3C9w@mail.gmail.com>
On Tue, Mar 12, 2013 at 02:12:29PM -0400, Matt McClure wrote:
> On Tue, Nov 27, 2012 at 7:41 AM, Matt McClure <matthewlmcclure@gmail.com> wrote:
> Your thoughts on the change?
Please include the patch in your message so that interested parties can
comment on it here, especially since the compare view on GitHub seems to
mangle the tabs.
For others' reference the patch is:
-- >8 --
From: Matt McClure <matt.mcclure@mapmyfitness.com>
Subject: [PATCH] difftool: Make directory diff symlink work tree
difftool -d formerly knew how to symlink to the work tree when the work
tree contains uncommitted changes. In practice, prior to this change, it
would not symlink to the work tree in case there were no uncommitted
changes, even when the user invoked difftool with the form:
git difftool -d [--options] <commit> [--] [<path>...]
This form is to view the changes you have in your working tree
relative to the named <commit>. You can use HEAD to compare it
with the latest commit, or a branch name to compare with the tip
of a different branch.
Instead, prior to this change, difftool would use the file's HEAD blob
sha1 to find its content rather than the work tree content. This change
teaches `git diff --raw` to emit the null SHA1 for consumption by
difftool -d, so that difftool -d will use a symlink rather than a copy
of the file.
Before:
$ git diff --raw HEAD^ -- diff-lib.c
:100644 100644 f35de0f... ead9399... M diff-lib.c
After:
$ ./git diff --raw HEAD^ -- diff-lib.c
:100644 100644 f35de0f... 0000000... M diff-lib.c
---
diff-lib.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/diff-lib.c b/diff-lib.c
index f35de0f..ead9399 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -319,6 +319,10 @@ static int show_modified(struct rev_info *revs,
return -1;
}
+ if (!cached && hashcmp(old->sha1, new->sha1)) {
+ sha1 = null_sha1;
+ }
+
if (revs->combine_merges && !cached &&
(hashcmp(sha1, old->sha1) || hashcmp(old->sha1, new->sha1))) {
struct combine_diff_path *p;
--
1.8.2.rc2.4.g7799588
^ permalink raw reply related
* Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()
From: Jeff King @ 2013-03-12 19:18 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130312160056.GB4472@sandbox-ub.fritz.box>
On Tue, Mar 12, 2013 at 05:00:56PM +0100, Heiko Voigt wrote:
> > That is, every path to get_next_char happens while we are in
> > git_config_from_file, and that function guarantees that cf = &top, and
> > that top.f != NULL. We do not have to even do any analysis of the
> > conditions for each call, because we never change "cf" nor "top.f"
> > except when we set them in git_config_from_file.
>
> Ok if you say so I will do that :-). I was thinking about adding a patch
> that would remove cf as a global variable and explicitely pass it down
> to get_next_char. That makes it more obvious that it actually is != NULL.
> Looking at your callgraph I do not think its that much work. What do you
> think?
Yeah, I think that makes it more obvious what is going on, but you will
run into trouble if you ever want that information to cross the "void *"
boundary of a config callback, as adding a new parameter there is hard.
The only place we do that now is for git_config_include, and I think you
could get by with stuffing it into the config_include_data struct (which
is already there to deal with this problem).
It would also make something like this patch hard or impossible:
http://article.gmane.org/gmane.comp.version-control.git/190267
as it wants to access "cf" from arbitrary callbacks. That is not a patch
that is actively under consideration, but I had considered polishing it
up at some point.
> BTW, how did you generate this callgraph? Do you have a nice tool for that?
I just did it manually in vi, working backwards from every instance of
get_next_char, as it is not that big a graph. I suspect something like
cscope could make it easier, but I don't know of any tool that will
build the graph automatically (it would not be that hard from the data
cscope has, though, so I wouldn't be surprised if such a tool exists).
-Peff
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: David Aguilar @ 2013-03-12 19:23 UTC (permalink / raw)
To: Matt McClure; +Cc: git@vger.kernel.org, Tim Henigan, John Keeping
In-Reply-To: <20130312190956.GC2317@serenity.lan>
On Tue, Mar 12, 2013 at 12:09 PM, John Keeping <john@keeping.me.uk> wrote:
> On Tue, Mar 12, 2013 at 02:12:29PM -0400, Matt McClure wrote:
>> On Tue, Nov 27, 2012 at 7:41 AM, Matt McClure <matthewlmcclure@gmail.com> wrote:
>> Your thoughts on the change?
>
> Please include the patch in your message so that interested parties can
> comment on it here, especially since the compare view on GitHub seems to
> mangle the tabs.
>
> For others' reference the patch is:
>
> -- >8 --
> From: Matt McClure <matt.mcclure@mapmyfitness.com>
> Subject: [PATCH] difftool: Make directory diff symlink work tree
>
> difftool -d formerly knew how to symlink to the work tree when the work
> tree contains uncommitted changes. In practice, prior to this change, it
> would not symlink to the work tree in case there were no uncommitted
> changes, even when the user invoked difftool with the form:
>
> git difftool -d [--options] <commit> [--] [<path>...]
> This form is to view the changes you have in your working tree
> relative to the named <commit>. You can use HEAD to compare it
> with the latest commit, or a branch name to compare with the tip
> of a different branch.
>
> Instead, prior to this change, difftool would use the file's HEAD blob
> sha1 to find its content rather than the work tree content. This change
> teaches `git diff --raw` to emit the null SHA1 for consumption by
> difftool -d, so that difftool -d will use a symlink rather than a copy
> of the file.
>
> Before:
>
> $ git diff --raw HEAD^ -- diff-lib.c
> :100644 100644 f35de0f... ead9399... M diff-lib.c
>
> After:
>
> $ ./git diff --raw HEAD^ -- diff-lib.c
> :100644 100644 f35de0f... 0000000... M diff-lib.c
Interesting approach. While this does get the intended behavior
for difftool, I'm afraid this would be a grave regression for
existing "git diff --raw" users who cannot have such behavior.
I don't think we could do this without adding an additional flag
to trigger this change in behavior (e.g. --null-sha1-for-....?)
so that existing users are unaffected by the change.
It feels like forcing the null SHA-1 is heavy-handed, but I
haven't thought it through enough.
While this may be a quick way to get this behavior,
I wonder if there is a better way.
Does anybody else have any comments/suggestions on how to
better accomplish this?
> ---
> diff-lib.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/diff-lib.c b/diff-lib.c
> index f35de0f..ead9399 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -319,6 +319,10 @@ static int show_modified(struct rev_info *revs,
> return -1;
> }
>
> + if (!cached && hashcmp(old->sha1, new->sha1)) {
> + sha1 = null_sha1;
> + }
> +
> if (revs->combine_merges && !cached &&
> (hashcmp(sha1, old->sha1) || hashcmp(old->sha1, new->sha1))) {
> struct combine_diff_path *p;
> --
> 1.8.2.rc2.4.g7799588
>
--
David
^ permalink raw reply
* Re: [PATCH] difftool: Make directory diff symlink work tree
From: John Keeping @ 2013-03-12 19:24 UTC (permalink / raw)
To: Matt McClure; +Cc: David Aguilar, git@vger.kernel.org, Tim Henigan
In-Reply-To: <20130312190956.GC2317@serenity.lan>
> difftool -d formerly knew how to symlink to the work tree when the work
> tree contains uncommitted changes. In practice, prior to this change, it
> would not symlink to the work tree in case there were no uncommitted
> changes, even when the user invoked difftool with the form:
>
> git difftool -d [--options] <commit> [--] [<path>...]
> This form is to view the changes you have in your working tree
> relative to the named <commit>. You can use HEAD to compare it
> with the latest commit, or a branch name to compare with the tip
> of a different branch.
>
> Instead, prior to this change, difftool would use the file's HEAD blob
> sha1 to find its content rather than the work tree content. This change
> teaches `git diff --raw` to emit the null SHA1 for consumption by
> difftool -d, so that difftool -d will use a symlink rather than a copy
> of the file.
>
> Before:
>
> $ git diff --raw HEAD^ -- diff-lib.c
> :100644 100644 f35de0f... ead9399... M diff-lib.c
>
> After:
>
> $ ./git diff --raw HEAD^ -- diff-lib.c
> :100644 100644 f35de0f... 0000000... M diff-lib.c
When I tried this I got the expected behaviour even without this patch.
It turns out that an uncommitted, but *staged* change emits the SHA1 of
the blob rather than the null SHA1. Do you get the desired behaviour if
you "git reset" before using difftool?
If so I think you want some new mode of operation for difftool instead
of this patch which will also affect unrelated commands.
> ---
> diff-lib.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/diff-lib.c b/diff-lib.c
> index f35de0f..ead9399 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -319,6 +319,10 @@ static int show_modified(struct rev_info *revs,
> return -1;
> }
>
> + if (!cached && hashcmp(old->sha1, new->sha1)) {
> + sha1 = null_sha1;
> + }
> +
> if (revs->combine_merges && !cached &&
> (hashcmp(sha1, old->sha1) || hashcmp(old->sha1, new->sha1))) {
> struct combine_diff_path *p;
> --
> 1.8.2.rc2.4.g7799588
>
^ permalink raw reply
* Re: [PATCH v2 2/4] config: drop file pointer validity check in get_next_char()
From: Jeff King @ 2013-03-12 19:26 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130312161620.GA4551@sandbox-ub.fritz.box>
On Tue, Mar 12, 2013 at 05:16:20PM +0100, Heiko Voigt wrote:
> > Ok if you say so I will do that :-). I was thinking about adding a patch
> > that would remove cf as a global variable and explicitely pass it down
> > to get_next_char. That makes it more obvious that it actually is != NULL.
> > Looking at your callgraph I do not think its that much work. What do you
> > think?
>
> I just had a look and unfortunately there are other functions that use
> this variable (namely handle_path_include) for which its not that easy
> to pass this in. So I will just drop the check here.
Ah, I should have read all your responses before answering the last
email. I think handle_path_include is surmountable, and I started to do
a proof-of-concept patch, but there's a harder one: die_bad_config,
which is called from git_config_int and friends. Getting the "cf"
pointer there is hard, because you would have to update every config
callback that uses git_config_int. Not worth it, I think.
Thanks for looking, though.
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/4] config: make parsing stack struct independent from actual data source
From: Jeff King @ 2013-03-12 19:27 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130312162734.GB4704@sandbox-ub.fritz.box>
On Tue, Mar 12, 2013 at 05:27:35PM +0100, Heiko Voigt wrote:
> > Would a union be more appropriate here? We do not ever have to pass it
> > directly as a parameter, since we pass the "struct config_source" to the
> > method functions.
> >
> > It's still possible to screw up using a union, but it's slightly harder
> > than screwing up using a void pointer. And I do not think we need the
> > run-time flexibility offered by the void pointer in this case.
>
> No we do not need the void pointer flexibility. But that means every new
> source would need to add to this union. Junio complained about that fact
> when I first added the extra members directly to the struct. A union
> does not waste that much space and we get some seperation using the
> union members. Since this struct is local only I think that should be
> ok.
Right. I think that is not a big deal, as we are not exposing this
struct outside of the config.c; any additions would need to add a new
git_config_from_foo function, anyway.
-Peff
^ permalink raw reply
* Re: [PATCH v2 4/4] teach config parsing to read from strbuf
From: Jeff King @ 2013-03-12 19:29 UTC (permalink / raw)
To: Heiko Voigt; +Cc: Junio C Hamano, git, Jens Lehmann, Ramsay Jones
In-Reply-To: <20130312164254.GB4752@sandbox-ub.fritz.box>
On Tue, Mar 12, 2013 at 05:42:54PM +0100, Heiko Voigt wrote:
> > Your series does not actually add any callers of the new function. The
> > obvious "patch 5/4" would be to plumb it into "git config --blob", and
> > then we can just directly test it there (there could be other callers
> > besides reading from a blob, of course, but I think the point of the
> > series is to head in that direction).
>
> Since this is a split of the series mentioned above there are no real
> callers yet. The main reason for the split was that I wanted to reduce
> the review burden of one big series into multiple reviews of smaller
> chunks. If you think it is useful to add the --blob option I can also
> test from there. It could actually be useful to look at certain
> .gitmodules options from the submodule script.
I am on the fence. I do not want to create more work for you, but I do
think it may come in handy, if only for doing submodule things from
shell. And it is hopefully not a very large patch. I'd say try it, and
if starts looking like it will be very ugly, the right thing may be to
leave it until somebody really wants it.
-Peff
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: John Keeping @ 2013-03-12 19:43 UTC (permalink / raw)
To: David Aguilar; +Cc: Matt McClure, git@vger.kernel.org, Tim Henigan
In-Reply-To: <CAJDDKr7S0ex1RvZS0QeBXxAuqcKrQJzhZeJP0MoMGmpGXyMOrA@mail.gmail.com>
On Tue, Mar 12, 2013 at 12:23:52PM -0700, David Aguilar wrote:
> I don't think we could do this without adding an additional flag
> to trigger this change in behavior (e.g. --null-sha1-for-....?)
> so that existing users are unaffected by the change.
>
> It feels like forcing the null SHA-1 is heavy-handed, but I
> haven't thought it through enough.
>
> While this may be a quick way to get this behavior,
> I wonder if there is a better way.
>
> Does anybody else have any comments/suggestions on how to
> better accomplish this?
How about something like "--symlink-all" where the everything in the
right-hand tree is symlink'd?
Something like this perhaps:
-- >8 --
diff --git a/git-difftool.perl b/git-difftool.perl
index 0a90de4..cab7c45 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -85,7 +85,7 @@ sub exit_cleanup
sub setup_dir_diff
{
- my ($repo, $workdir, $symlinks) = @_;
+ my ($repo, $workdir, $symlinks, $symlink_all) = @_;
# Run the diff; exit immediately if no diff found
# 'Repository' and 'WorkingCopy' must be explicitly set to insure that
@@ -159,10 +159,10 @@ EOF
}
if ($rmode ne $null_mode) {
- if ($rsha1 ne $null_sha1) {
- $rindex .= "$rmode $rsha1\t$dst_path\0";
- } else {
+ if ($symlink_all or $rsha1 eq $null_sha1) {
push(@working_tree, $dst_path);
+ } else {
+ $rindex .= "$rmode $rsha1\t$dst_path\0";
}
}
}
@@ -299,6 +299,7 @@ sub main
prompt => undef,
symlinks => $^O ne 'cygwin' &&
$^O ne 'MSWin32' && $^O ne 'msys',
+ symlink_all => undef,
tool_help => undef,
);
GetOptions('g|gui!' => \$opts{gui},
@@ -308,6 +309,7 @@ sub main
'y' => sub { $opts{prompt} = 0; },
'symlinks' => \$opts{symlinks},
'no-symlinks' => sub { $opts{symlinks} = 0; },
+ 'symlink-all' => \$opts{symlink_all},
't|tool:s' => \$opts{difftool_cmd},
'tool-help' => \$opts{tool_help},
'x|extcmd:s' => \$opts{extcmd});
@@ -346,7 +348,7 @@ sub main
# will invoke a separate instance of 'git-difftool--helper' for
# each file that changed.
if (defined($opts{dirdiff})) {
- dir_diff($opts{extcmd}, $opts{symlinks});
+ dir_diff($opts{extcmd}, $opts{symlinks}, $opts{symlink_all});
} else {
file_diff($opts{prompt});
}
@@ -354,13 +356,13 @@ sub main
sub dir_diff
{
- my ($extcmd, $symlinks) = @_;
+ my ($extcmd, $symlinks, $symlink_all) = @_;
my $rc;
my $error = 0;
my $repo = Git->repository();
my $workdir = find_worktree($repo);
my ($a, $b, $tmpdir, @worktree) =
- setup_dir_diff($repo, $workdir, $symlinks);
+ setup_dir_diff($repo, $workdir, $symlinks, $symlink_all);
if (defined($extcmd)) {
$rc = system($extcmd, $a, $b);
^ permalink raw reply related
* Re: linux-next: unneeded merge in the security tree
From: Junio C Hamano @ 2013-03-12 19:49 UTC (permalink / raw)
To: Linus Torvalds
Cc: Theodore Ts'o, James Morris, Stephen Rothwell, linux-next,
Linux Kernel Mailing List, Git Mailing List
In-Reply-To: <CA+55aFzFLDcN-1GKae6Xqrns59K1xOD_HPzuv2Lv1__fZpqFMw@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> One is simple:
>
> git config alias.sync="pull --ff-only"
Heh, I just wrote that myself after reading the early part of this
message ;-)
> which works fine, but forces submaintainers to be careful when doing
> things like this, and using a special command to do back-merges.
> And maybe that's the right thing to do? Back-merges *are* special,
Yes.
> after all. But the above alias is particularly fragile, in that
> there's both "pull" and "merge" that people want to use this for, and
> it doesn't really handle both. And --ff-only will obviously fail if
> you actually have some work in your tree, and want to do a real merge,
> so then you have to do that differently. So I'm mentioning this as a
> better model than "git reset", but not really a *solution*.
> That said, the fact that "--ff-only" errors out if you have local
> development may actually be a big bonus - because you really shouldn't
> do merges at all if you have local development, but syncing up to my
> tree if you don't have it (and are going to start it) may be something
> reasonable.
Yes, that's the reasoning behind all the behaviours you described
above.
> Now, the other approach - and perhaps preferable, but requiring actual
> changes to git itself - is to do the non-fast-forward merge *only* for
> FETCH_HEAD, which already has magic semantics in other ways. So if
> somebody does
>
> git fetch linus
> git merge v3.8
>
> to sync with me, they would *not* get a merge commit with a signature,
> just a fast-forward. But if you do
>
> git pull linus v3.8
>
> or a
>
> git fetch linus v3.8
> git merge FETCH_HEAD
>
> it would look like a "maintainer merge"....
I am not sure I follow. Are you solving the real problem, the
pointeless merge in the "security tree" that started this thread?
I would imagine it was made by somebody thinking that pulling a
tagged stable point from you is a good idea, like this:
git pull linus v3.9-rc2
which under your FETCH_HEAD rule would look like a maintainer merge,
no?
An alternative may be to activate the magic "mergetag" thing only
when you give "--no-ff" explicitly; otherwise merge would unwrap the
tag, whether it comes from FETCH_HEAD.
The following examples all assume that your HEAD is somewhere
behind v3.9-rc2, perhaps done by
git checkout -b test v3.8^0
Then under the "--no-ff activates the magic" rule:
git merge v3.9-rc2
will fast-forward, but this
git merge --no-ff v3.9-rc2
creates a real merge with the "mergetag" signature block. The one
that caused trouble in the "security tree", i.e.
git pull linus v3.9-rc2
or its equivalent
git fetch linus v3.9-rc2
git merge FETCH_HEAD
would still fast-forward under this rule. The maintainer needs to
do
git pull --no-ff git://git.kernel.org/... for-linus
if the pull could fast-forward under this rule, though.
Having thought this up to this point, I am not sure it generally is
a good change. It feels that "pull --ff-only" that prevents people
from creating pointless back-merges may still be a better mechanism.
I dunno.
^ permalink raw reply
* Re: linux-next: unneeded merge in the security tree
From: Junio C Hamano @ 2013-03-12 20:02 UTC (permalink / raw)
To: Linus Torvalds
Cc: Theodore Ts'o, James Morris, Stephen Rothwell, linux-next,
Linux Kernel Mailing List, Git Mailing List
In-Reply-To: <7vsj40760d.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Then under the "--no-ff activates the magic" rule:
>
> git merge v3.9-rc2
>
> will fast-forward, but this
>
> git merge --no-ff v3.9-rc2
>
> creates a real merge with the "mergetag" signature block. The one
> that caused trouble in the "security tree", i.e.
>
> git pull linus v3.9-rc2
>
> or its equivalent
>
> git fetch linus v3.9-rc2
> git merge FETCH_HEAD
>
> would still fast-forward under this rule. The maintainer needs to
> do
>
> git pull --no-ff git://git.kernel.org/... for-linus
>
> if the pull could fast-forward under this rule, though.
Scratch the last sentence. It should have been
"whether the pull fast-forwards or not". You'd always need to.
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: Junio C Hamano @ 2013-03-12 20:38 UTC (permalink / raw)
To: David Aguilar
Cc: Matt McClure, git@vger.kernel.org, Tim Henigan, John Keeping
In-Reply-To: <CAJDDKr7S0ex1RvZS0QeBXxAuqcKrQJzhZeJP0MoMGmpGXyMOrA@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
> Interesting approach. While this does get the intended behavior
> for difftool, I'm afraid this would be a grave regression for
> existing "git diff --raw" users who cannot have such behavior.
The 0{40} in RHS of --raw output is to say that we do not know what
object name the contents at the path hashes to.
If you run "git diff HEAD^" for a path that is different between
HEAD and the index for which you do not have a local change in the
working tree, we have to show the path (because it is different
between the working tree and HEAD^), but we know the object name for
copy in the working tree, simply because we know it matches what is
in the index. Showing 0{40} on the RHS in such a case loses
information, making us say "We don't know" when we perfectly well
know. That is a regression.
If the user is allowed to touch any random file in the working tree,
I do not see a workable solution other than John Keeping's follow-up
patch to make symlinks of all paths involved.
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: Junio C Hamano @ 2013-03-12 20:39 UTC (permalink / raw)
To: John Keeping
Cc: David Aguilar, Matt McClure, git@vger.kernel.org, Tim Henigan
In-Reply-To: <20130312194306.GE2317@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> How about something like "--symlink-all" where the everything in the
> right-hand tree is symlink'd?
Does it even have to be conditional? What is the situation when you
do not want symbolic links?
^ permalink raw reply
* Re: [PATCH v2 3/6] match_basename: use strncmp instead of strcmp
From: Junio C Hamano @ 2013-03-12 20:59 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <CACsJy8A_4SqLu5L6P0PJ78Lwy12fjL7T2p-KbVEVLJmKNqhyRw@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> glibc's C strncmp version does 4-byte comparison at a time when n >=4,
> then fall back to 1-byte for the rest. I don't know if it's faster
> than a plain always 1-byte comparison though. There's also the hand
> written assembly version that compares n from 1..16, not exactly sure
> how this version works yet.
It sounds to me more like "a very popular implementation of
strcmp/strncmp pair found to have more optimized strncmp than
strcmp". While that may be a good reason to justify this patch,
I do not think it justifies this:
>> strncmp is provided length information which could be taken advantage
>> by the underlying implementation.
After all, strcmp() could also be optimized to fetch word-at-a-time
while being careful about not overstepping the page boundary.
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: John Keeping @ 2013-03-12 21:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Aguilar, Matt McClure, git@vger.kernel.org, Tim Henigan
In-Reply-To: <7vfw0073pm.fsf@alter.siamese.dyndns.org>
On Tue, Mar 12, 2013 at 01:39:17PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > How about something like "--symlink-all" where the everything in the
> > right-hand tree is symlink'd?
>
> Does it even have to be conditional? What is the situation when you
> do not want symbolic links?
When you're not comparing the working tree.
If we can reliably say "the RHS is the working tree" then it could be
unconditional, but I haven't thought about how to do that - I can't see
a particularly easy way to check for that; is it sufficient to say
"there is no more than one non-option to the left of '--' and '--cached'
is not among the options"?
John
^ permalink raw reply
* Re: linux-next: unneeded merge in the security tree
From: Theodore Ts'o @ 2013-03-12 21:20 UTC (permalink / raw)
To: Linus Torvalds
Cc: James Morris, Stephen Rothwell, linux-next,
Linux Kernel Mailing List, Git Mailing List, Junio C Hamano
In-Reply-To: <CA+55aFzFLDcN-1GKae6Xqrns59K1xOD_HPzuv2Lv1__fZpqFMw@mail.gmail.com>
What if we added the ability to do something like this:
[remote "origin"]
url = git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
fetch = +refs/heads/master:refs/heads/master
mergeoptions = --ff-only
This would be an analog to branch.<name>.mergeoptions, but it would
apply to the source of the pull request, instead of the destination.
That way, people who do a "git pull" from Linus's tree would get the
protection of --ff-only, while pulls from submaintainer trees would
automatically get a merge commit, which is what we want.
It doesn't handle the case of a submaintainer pulling from a
maintainer in a back-merge scenario, but that should be a pretty rare
case, so maybe that's OK.
- Ted
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: Junio C Hamano @ 2013-03-12 21:26 UTC (permalink / raw)
To: John Keeping
Cc: David Aguilar, Matt McClure, git@vger.kernel.org, Tim Henigan
In-Reply-To: <20130312210630.GF2317@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
>> Does it even have to be conditional? What is the situation when you
>> do not want symbolic links?
>
> When you're not comparing the working tree.
OK, so what you want is essentially:
* If you see 0{40} in "diff --raw", you *know* you are showing the working tree
file on the RHS, and you want to symlink, so that the edit made
by the user will be reflected back to theh working tree copy.
* If your working tree file match what is in the index, you won't
see 0{40} but you still want to symlink, for the same reason.
* If you are comparing two trees, and especially if your RHS is not
HEAD, you will send everything to a temporary without
symlinks. Any edit made by the user will be lost.
If that is the case, perhaps the safest way to go may be to write
the object out when you see non 0{40}, compare it with the working
tree version and then turn that into symlink? That way, you not
only cover the second bullet point, but also cover half of the third
one where the user may find a bug in the RHS, update it in difftool.
I am assuming that you are write-protecting the non-symlink files in
the temporary tree (i.e. those that do not match the working tree)
to prevent users from accidentally modifying something there is no
place to save back to.
Hrm?
^ permalink raw reply
* Re: linux-next: unneeded merge in the security tree
From: Linus Torvalds @ 2013-03-12 21:28 UTC (permalink / raw)
To: Theodore Ts'o, Linus Torvalds, James Morris, Stephen Rothwell,
linux-next, Linux Kernel Mailing List, Git Mailing List,
Junio C Hamano
In-Reply-To: <20130312212027.GE14792@thunk.org>
On Tue, Mar 12, 2013 at 2:20 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> What if we added the ability to do something like this:
>
> [remote "origin"]
> url = git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
> fetch = +refs/heads/master:refs/heads/master
> mergeoptions = --ff-only
Hmm. Something like this could be interesting for other things:
- use "--rebase" when pulling (this is common for people who maintain
a set of patches and do *not* export their git tree - I use it for
projects like git and subsurface where there is an upstream maintainer
and I usually send patches by email rather than git)
- "--no-summary". As a maintainer, you people probably do want to
enable summaries for people they pull from, but *not* from upstream.
So this might even make sense to do by default when you clone a new
repository.
- I do think that we might want a "--no-signatures" for the specific
case of merging signed tags without actually taking the signature
(because it's a "upstream" repo). The "--ff-only" thing is *too*
strict. Sometimes you really do want to merge in new code, disallowing
it entirely is tough.
Of course, I'm not really sure if we want to list the flags. Maybe
it's better to just introduce the notion of "upstream" directly, and
make that a flag, and make "origin" default to that when you clone.
And then have git use different heurstics for pulling upstream (like
warning by default when doing a back-merge, perhaps?)
Linus
^ permalink raw reply
* Re: linux-next: unneeded merge in the security tree
From: Junio C Hamano @ 2013-03-12 21:30 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Linus Torvalds, James Morris, Stephen Rothwell, linux-next,
Linux Kernel Mailing List, Git Mailing List
In-Reply-To: <20130312212027.GE14792@thunk.org>
Theodore Ts'o <tytso@mit.edu> writes:
> What if we added the ability to do something like this:
>
> [remote "origin"]
> url = git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
> fetch = +refs/heads/master:refs/heads/master
> mergeoptions = --ff-only
>
> This would be an analog to branch.<name>.mergeoptions, but it would
> apply to the source of the pull request, instead of the destination.
>
> That way, people who do a "git pull" from Linus's tree would get the
> protection of --ff-only, while pulls from submaintainer trees would
> automatically get a merge commit, which is what we want.
>
> It doesn't handle the case of a submaintainer pulling from a
> maintainer in a back-merge scenario, but that should be a pretty rare
> case, so maybe that's OK.
Is there an escape hatch for that rare case? IOW, how does a
submaintainer who configured the above to override --ff-only?
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: Matt McClure @ 2013-03-12 21:43 UTC (permalink / raw)
To: John Keeping
Cc: Junio C Hamano, David Aguilar, git@vger.kernel.org, Tim Henigan
In-Reply-To: <20130312210630.GF2317@serenity.lan>
On Tue, Mar 12, 2013 at 5:06 PM, John Keeping <john@keeping.me.uk> wrote:
> On Tue, Mar 12, 2013 at 01:39:17PM -0700, Junio C Hamano wrote:
>>
>> What is the situation when you do not want symbolic links?
>
> When you're not comparing the working tree.
>
> If we can reliably say "the RHS is the working tree" then it could be
> unconditional, but I haven't thought about how to do that - I can't see
> a particularly easy way to check for that;
Agreed. From what I can see, the only form of the diff options that
compares against the working tree is
git difftool -d [--options] <commit> [--] [<path>...]
At first, I thought that the following cases were also working tree
cases, but actually they use the HEAD commit.
git difftool -d commit1..
git difftool -d commit1...
> is it sufficient to say
> "there is no more than one non-option to the left of '--' and '--cached'
> is not among the options"?
An alternative approach would be to reuse git-diff's option parsing
and make it tell git-difftool when git-diff sees the working tree
case. At this point, I haven't seen an obvious place in the source
where git-diff makes that choice, but if someone could point me in the
right direction, I think I'd actually prefer that approach. What do
you think?
--
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure
^ permalink raw reply
* Re: linux-next: unneeded merge in the security tree
From: Junio C Hamano @ 2013-03-12 21:47 UTC (permalink / raw)
To: Linus Torvalds
Cc: Theodore Ts'o, James Morris, Stephen Rothwell, linux-next,
Linux Kernel Mailing List, Git Mailing List
In-Reply-To: <CA+55aFwHJtOU4Qzt3XZsER165kTc5P0ATQP2wPHvuUiVic8bnA@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> - I do think that we might want a "--no-signatures" for the specific
> case of merging signed tags without actually taking the signature
> (because it's a "upstream" repo). The "--ff-only" thing is *too*
> strict. Sometimes you really do want to merge in new code, disallowing
> it entirely is tough.
I agree that "--ff-only" thing is too strict and sometimes you would
want to allow back-merges, but when you do allow such a back-merge,
is there a reason you want it to be --no-signatures merge? When a
subtree maintainer decides to merge a stable release point from you
with a good reason, I do not see anything wrong in recording that
the resulting commit _did_ merge what you released with a signature.
^ permalink raw reply
* Re: linux-next: unneeded merge in the security tree
From: Linus Torvalds @ 2013-03-12 21:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Theodore Ts'o, James Morris, Stephen Rothwell, linux-next,
Linux Kernel Mailing List, Git Mailing List
In-Reply-To: <7vppz45lz9.fsf@alter.siamese.dyndns.org>
On Tue, Mar 12, 2013 at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I agree that "--ff-only" thing is too strict and sometimes you would
> want to allow back-merges, but when you do allow such a back-merge,
> is there a reason you want it to be --no-signatures merge? When a
> subtree maintainer decides to merge a stable release point from you
> with a good reason, I do not see anything wrong in recording that
> the resulting commit _did_ merge what you released with a signature.
No, there's nothing really bad with adding the signature to the merge
commit if you do make a merge. It's the fact that it currently makes a
non-ff merge when that is pointless that hurts.
That said, adding the signature from an upstream tag doesn't really
seem to be hugely useful. I'm not seeing much of an upside, in other
words. I'd *expect* that people would pick up upstream tags
regardless, no?
Linus
^ permalink raw reply
* Re: linux-next: unneeded merge in the security tree
From: Junio C Hamano @ 2013-03-12 22:00 UTC (permalink / raw)
To: Linus Torvalds
Cc: Theodore Ts'o, James Morris, Stephen Rothwell, linux-next,
Linux Kernel Mailing List, Git Mailing List
In-Reply-To: <CA+55aFzWjfFjcRZXBO+edO7f66REA0pOsC3iZ2vYdHrkcovnHA@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> That said, adding the signature from an upstream tag doesn't really
> seem to be hugely useful. I'm not seeing much of an upside, in other
> words. I'd *expect* that people would pick up upstream tags
> regardless, no?
Yes, their "git fetch" will auto-follow, but mergetag embedded in
the commit objects will give the history auditable trail the same
way as the merges you make your downstream. You of course could
match out-of-line tags against back-merges and verify your merges
with mergetags, but you do not have to.
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: Matt McClure @ 2013-03-12 22:11 UTC (permalink / raw)
To: John Keeping
Cc: Junio C Hamano, David Aguilar, git@vger.kernel.org, Tim Henigan
In-Reply-To: <CAJELnLGBr1wOX4-3rCNjPpPLezc_6FgyeuPqty268JR0==qtvQ@mail.gmail.com>
On Tue, Mar 12, 2013 at 5:43 PM, Matt McClure <matthewlmcclure@gmail.com> wrote:
> On Tue, Mar 12, 2013 at 5:06 PM, John Keeping <john@keeping.me.uk> wrote:
>>
>> is it sufficient to say
>> "there is no more than one non-option to the left of '--' and '--cached'
>> is not among the options"?
>
> An alternative approach would be to reuse git-diff's option parsing
> and make it tell git-difftool when git-diff sees the working tree
> case. At this point, I haven't seen an obvious place in the source
> where git-diff makes that choice, but if someone could point me in the
> right direction, I think I'd actually prefer that approach. What do
> you think?
There's an interesting comment in cmd_diff:
/*
* We could get N tree-ish in the rev.pending_objects list.
* Also there could be M blobs there, and P pathspecs.
*
* N=0, M=0:
* cache vs files (diff-files)
* N=0, M=2:
* compare two random blobs. P must be zero.
* N=0, M=1, P=1:
* compare a blob with a working tree file.
*
* N=1, M=0:
* tree vs cache (diff-index --cached)
*
* N=2, M=0:
* tree vs tree (diff-tree)
*
* N=0, M=0, P=2:
* compare two filesystem entities (aka --no-index).
*
* Other cases are errors.
*/
whereas inspecting rev.pending in the "compare against working tree"
case, I see:
(gdb) p rev.pending
$3 = {
nr = 1,
alloc = 64,
objects = 0x100807a00
}
(gdb) p *rev.pending.objects
$4 = {
item = 0x100831a48,
name = 0x7fff5fbff8f8 "HEAD^",
mode = 12288
}
Given the cases listed in the comment, I assume cmd_diff must
interpret this case as:
* N=1, M=0:
* tree vs cache (diff-index --cached)
The description of that case is confusing or wrong given that
git-diff-index(1) says:
--cached
do not consider the on-disk file at all
***
cmd_diff executes this case:
else if (ents == 1)
result = builtin_diff_index(&rev, argc, argv);
So it looks like I could short-circuit in builtin_diff_index or
something it calls -- e.g., run_diff_index -- to get git-diff to tell
git-difftool that it's the working tree case. I see that
run_diff_index does:
diff_set_mnemonic_prefix(&revs->diffopt, "c/", cached ? "i/" : "w/");
So that looks like a good place where the code is already deciding
that it's the working tree case -- "w/", though surprisingly to me:
(gdb) p revs->diffopt
$12 = {
...
a_prefix = 0x1001c25aa "a/",
b_prefix = 0x1001c25ad "b/",
...
So diff_set_mnemonic_prefix doesn't actually use the "w/" value passed
to it because:
if (!options->b_prefix)
options->b_prefix = b;
Maybe if I could prevent b_prefix from getting set earlier, I could
get some variant of git-diff to emit the "w/" for git-difftool.
--
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure
^ permalink raw reply
* Re: difftool -d symlinks, under what conditions
From: Junio C Hamano @ 2013-03-12 22:16 UTC (permalink / raw)
To: Matt McClure
Cc: John Keeping, David Aguilar, git@vger.kernel.org, Tim Henigan
In-Reply-To: <CAJELnLGBr1wOX4-3rCNjPpPLezc_6FgyeuPqty268JR0==qtvQ@mail.gmail.com>
Matt McClure <matthewlmcclure@gmail.com> writes:
> An alternative approach would be to reuse git-diff's option parsing
> and make it tell git-difftool when git-diff sees the working tree
> case. At this point, I haven't seen an obvious place in the source
> where git-diff makes that choice, but if someone could point me in the
> right direction, I think I'd actually prefer that approach. What do
> you think?
I do not think you want to go there. That wouldn't solve the third
case in my previous message, no?
^ 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