* Re: [RFCv4 1/3] gitweb: add patch view
From: Jakub Narebski @ 2008-12-15 13:58 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <cb7bb73a0812150548w526a8c0eu13ec95785e0ab824@mail.gmail.com>
Giuseppe Bilotta wrote:
> On Mon, Dec 15, 2008 at 2:17 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> + my $patch_max;
>>> + if ($format eq 'patch') {
>>> + $patch_max = gitweb_check_feature('patches');
>>> + die_error(403, "Patch view not allowed") unless $patch_max;
>>> + }
>>> +
>>
>> Hmmm... gitweb_check_feature
>
> You're right, it's an abuse. I'll make it gitweb_get_feature()[0]
Or better
+ ($patch_max) = gitweb_get_feature('patches');
[...]
> As soon as you finish the patchset review I'll have a new version
> taking into consideration all the other suggestions and remarks I
> snipped from this reply.
Thank you very much for keeping this patch series alive.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: Announce: TortoiseGit 0.1 preview version
From: Jakub Narebski @ 2008-12-15 13:56 UTC (permalink / raw)
To: Li Frank; +Cc: git
In-Reply-To: <1976ea660812140529k1499b410u4437645be0dc7dfc@mail.gmail.com>
On Sun, 14 Dec 2008, Li Frank wrote:
>
> 2008/12/14 Jakub Narebski <jnareb@gmail.com>:
>> "李智" <lznuaa@gmail.com> writes:
>>
>>> TortoiseGit is porting from TortoiseSVN. It is explore extension.
>>> This version just finish a minimum set of TortoiseSVN porting
>>
>> How it differs from GitCheetah project?
> GitCheetah just show git-gui &git-bash at context menu!
>
> TortoiseGit is full porting from TortoiseSVN. Overlay icon can show
> modified file, add file and conflicted file whith different icon.
>
> TortoiseGit can commit change, show log, checkout ... at Context menu!
> It is full git fontend.
>> [...]
>>> Project Home Page at:
>>> http://code.google.com/p/tortoisegit/
>>>
>>> Source code at
>>> http://repo.or.cz/w/TortoiseGit.git
>>>
>>> It need msysgit 1.6.0.2.
>>
>> Do you feel it is mature enough to be added to git wiki page
>> http://git.or.cz/gitwiki/InterfacesFrontendsAndTools
>> just above "Git Extensions" entry? Would you do it, or should
>> I do this?
>
> TortoiseGit is in very early stage. I just implement min set feature
> and not mature . I hope there are more and more people involve this
> project and make it mature as TortoiseSVN.
I have added information about TortoiseGit to git wiki at
http://git.or.cz/gitwiki/InterfacesFrontendsAndTools#head-803ff1eef7a32fd1d8fe86713de343af18605047
Please correct it if the information there (copy below) is wrong.
Feel free to extend this short info.
Hopefully this way more people would find your project, and be able
to contribute to it.
=== TortoiseGit ===
TortoiseGit[1] (gitweb[2]) by Li Frank is a port of TortoiseSVN[3]
to Git. It is Microsoft Windows shell (Explorer) extension, written
in C++. TortoiseGit is in very early stages of development,
implementing currently only minimal set of features.
[1] http://code.google.com/p/tortoisegit/
[2] http://repo.or.cz/w/TortoiseGit.git
[3] http://tortoisesvn.tigris.org/
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [RFCv4 1/3] gitweb: add patch view
From: Giuseppe Bilotta @ 2008-12-15 13:48 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <200812151417.16372.jnareb@gmail.com>
On Mon, Dec 15, 2008 at 2:17 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> + my $patch_max;
>> + if ($format eq 'patch') {
>> + $patch_max = gitweb_check_feature('patches');
>> + die_error(403, "Patch view not allowed") unless $patch_max;
>> + }
>> +
>
> Hmmm... gitweb_check_feature
You're right, it's an abuse. I'll make it gitweb_get_feature()[0]
> I am wondering if we could somehow mark (encode) either $hash_parent
> or number of patches in proposed filename... but I don't think it is
> worth it.
Including hash_parent if defined is quite possible. I'm not sure it's
really worth it considering that the typical usage would be to publish
a patchset for a particular feature (in which case the hash/branch
name would be enough).
>> + } elsif ($format eq 'patch') {
>> + local $/ = undef;
>> + print <$fd>;
>> + close $fd
>> + or print "Reading git-format-patch failed\n";
>
> Nice, although... I'd prefer for Perl expert to say if it is better
> to dump file as a whole in such way (it might be quite large), or
> to do it line by line, i.e. without "local $/ = undef;", or using
> "print while <$fd>;" also without "local $/ = undef;".
I'm just sticking to whatever the existing code does :-)
As soon as you finish the patchset review I'll have a new version
taking into consideration all the other suggestions and remarks I
snipped from this reply.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: "git gc" doesn't seem to remove loose objects any more
From: Mikael Magnusson @ 2008-12-15 13:38 UTC (permalink / raw)
To: Bruce Stephens; +Cc: git
In-Reply-To: <808wqhzjl9.fsf@tiny.isode.net>
2008/12/15 Bruce Stephens <bruce.stephens@isode.com>:
> I couldn't see a test for this, but perhaps I'm just missing it?
>
> brs% git count-objects
> 161 objects, 1552 kilobytes
> brs% git gc
> Counting objects: 80621, done.
> Compressing objects: 100% (22372/22372), done.
> Writing objects: 100% (80621/80621), done.
> Total 80621 (delta 57160), reused 80305 (delta 56884)
> brs% git count-objects
> 207 objects, 2048 kilobytes
>
>
> And I see lots of directories under .git/objects which confirms
> things.
>
> I don't think I've changed any relevant configuration.
>
> This is with 8befc50c49e8a271fd3cd7fb34258fe88d1dfcad (also whatever
> version I used before, erm, probably
> de0db422782ddaf7754ac5b03fdc6dc5de1a9ae4), and possibly earlier
> versions---I've just started noticing now that the number of loose
> objects has started causing git gui to complain.
>
> (Hmm, I note that git gui reports a larger number of loose objects
> than git count-objects. Ah, OK, it really is just an approximation,
> so no surprise.)
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
IIRC git gc only removes loose objects older than two weeks, if you
really want to remove them now, run git prune. But make sure no other
git process can be active when you run it, or it could possibly step
on something.
--
Mikael Magnusson
^ permalink raw reply
* Re: [RFCv4 1/3] gitweb: add patch view
From: Jakub Narebski @ 2008-12-15 13:17 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <1228575755-13432-2-git-send-email-giuseppe.bilotta@gmail.com>
On Sat, 6 Dec 2008, Giuseppe Bilotta wrote:
> The output of commitdiff_plain is not intended for git-am:
> * when given a range of commits, commitdiff_plain publishes a single
> patch with the message from the first commit, instead of a patchset
> * the hand-built email format replicates the commit summary both as
> email subject and as first line of the email itself, resulting in
> a duplication if the output is used with git-am.
>
> We thus create a new view that can be fed to git-am directly, allowing
> patch exchange via gitweb. The new view exposes the output of git
> format-patch directly, limiting it to a single patch in the case of a
> single commit.
>
> A configurable upper limit is imposed on the number of commits which
> will be included in a patchset, to prevent DoS attacks on the server.
> Setting the limit to 0 will disable the patch view, setting it to a
> negative number will remove the limit.
It would be good to have in commit mesage what is the default upper
limit (or if we decide to have this feature turned off by default,
proposed limit to choose when enabling this feature).
Does limit of 16 patches have any numbers behind it? We use page size
of 100 commits for 'shortlog', 'log' and 'history' views, but for
those views we don't need to generate patches (which is slower). From
a few tests "git log -100" is faster than "git format-patch -100 --stdout"
about 30 times in warm cache case, and about 1.7 times in cold cache
case.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Other than that minor detail I like this patch
> ---
> gitweb/gitweb.perl | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 64 insertions(+), 1 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 95988fb..71d5af4 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -329,6 +329,14 @@ our %feature = (
> 'ctags' => {
> 'override' => 0,
> 'default' => [0]},
> +
> + # The maximum number of patches in a patchset generated in patch
> + # view. Set this to 0 or undef to disable patch view, or to a
> + # negative number to remove any limit.
I think it would be nice to have standard boilerplate explanation how
to change it, and how to override it, with the addition on how to
disable it from repository config, because it is not very obvious.
Something like:
+ # To disable system wide have in $GITWEB_CONFIG
+ # $feature{'patches'}{'default'} = [];
+ # To have project specific config enable override in $GITWEB_CONFIG
+ # $feature{'patches'}{'override'} = 1;
+ # and in project config, maximum number of patches in a patchset
+ # or 0 to disable. Example: gitweb.patches = 0
> + 'patches' => {
> + 'sub' => \&feature_patches,
> + 'override' => 0,
> + 'default' => [16]},
> );
>
> sub gitweb_get_feature {
> @@ -410,6 +418,19 @@ sub feature_pickaxe {
> return ($_[0]);
> }
>
> +sub feature_patches {
> + my @val = (git_get_project_config('patches', '--int'));
> +
> + # if @val is empty, the config is not (properly)
> + # overriding the feature, so we return the default,
> + # otherwise we pick the override
Very similar feature_snapshot subroutine doesn't have such comment.
I don't think it is necessary, and its wording might cause confusion.
> + if (@val) {
> + return @val;
> + }
> +
> + return ($_[0]);
> +}
> +
Nice.
> # checking HEAD file with -e is fragile if the repository was
> # initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
> # and then pruned.
> @@ -503,6 +524,7 @@ our %actions = (
> "heads" => \&git_heads,
> "history" => \&git_history,
> "log" => \&git_log,
> + "patch" => \&git_patch,
> "rss" => \&git_rss,
> "atom" => \&git_atom,
> "search" => \&git_search,
> @@ -5386,6 +5408,13 @@ sub git_blobdiff_plain {
>
> sub git_commitdiff {
> my $format = shift || 'html';
> +
> + my $patch_max;
> + if ($format eq 'patch') {
> + $patch_max = gitweb_check_feature('patches');
> + die_error(403, "Patch view not allowed") unless $patch_max;
> + }
> +
Hmmm... gitweb_check_feature
> $hash ||= $hash_base || "HEAD";
> my %co = parse_commit($hash)
> or die_error(404, "Unknown commit object");
> @@ -5483,7 +5512,23 @@ sub git_commitdiff {
> open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
> '-p', $hash_parent_param, $hash, "--"
> or die_error(500, "Open git-diff-tree failed");
> -
> + } elsif ($format eq 'patch') {
> + # For commit ranges, we limit the output to the number of
> + # patches specified in the 'patches' feature.
> + # For single commits, we limit the output to a single patch,
> + # diverging from the git format-patch default.
I think it would be more clear to use
+ # diverging from the git-format-patch default.
> + my @commit_spec = ();
> + if ($hash_parent) {
> + if ($patch_max > 0) {
> + push @commit_spec, "-$patch_max";
> + }
> + push @commit_spec, '-n', "$hash_parent..$hash";
> + } else {
> + push @commit_spec, '-1', '--root', $hash;
> + }
Nice solution. I like it.
> + open $fd, "-|", git_cmd(), "format-patch", '--encoding=utf8',
> + '--stdout', @commit_spec
> + or die_error(500, "Open git-format-patch failed");
> } else {
> die_error(400, "Unknown commitdiff format");
> }
> @@ -5532,6 +5577,14 @@ sub git_commitdiff {
> print to_utf8($line) . "\n";
> }
> print "---\n\n";
> + } elsif ($format eq 'patch') {
> + my $filename = basename($project) . "-$hash.patch";
I am wondering if we could somehow mark (encode) either $hash_parent
or number of patches in proposed filename... but I don't think it is
worth it.
> +
> + print $cgi->header(
> + -type => 'text/plain',
> + -charset => 'utf-8',
> + -expires => $expires,
> + -content_disposition => 'inline; filename="' . "$filename" . '"');
> }
>
> # write patch
> @@ -5553,6 +5606,11 @@ sub git_commitdiff {
> print <$fd>;
> close $fd
> or print "Reading git-diff-tree failed\n";
> + } elsif ($format eq 'patch') {
> + local $/ = undef;
> + print <$fd>;
> + close $fd
> + or print "Reading git-format-patch failed\n";
Nice, although... I'd prefer for Perl expert to say if it is better
to dump file as a whole in such way (it might be quite large), or
to do it line by line, i.e. without "local $/ = undef;", or using
"print while <$fd>;" also without "local $/ = undef;".
> }
> }
>
> @@ -5560,6 +5618,11 @@ sub git_commitdiff_plain {
> git_commitdiff('plain');
> }
>
> +# format-patch-style patches
> +sub git_patch {
> + git_commitdiff('patch');
> +}
> +
Nice.
> sub git_history {
> if (!defined $hash_base) {
> $hash_base = git_get_head_hash($project);
> --
> 1.5.6.5
>
>
--
Jakub Narebski
Poland
^ permalink raw reply
* "git gc" doesn't seem to remove loose objects any more
From: Bruce Stephens @ 2008-12-15 12:52 UTC (permalink / raw)
To: git
I couldn't see a test for this, but perhaps I'm just missing it?
brs% git count-objects
161 objects, 1552 kilobytes
brs% git gc
Counting objects: 80621, done.
Compressing objects: 100% (22372/22372), done.
Writing objects: 100% (80621/80621), done.
Total 80621 (delta 57160), reused 80305 (delta 56884)
brs% git count-objects
207 objects, 2048 kilobytes
And I see lots of directories under .git/objects which confirms
things.
I don't think I've changed any relevant configuration.
This is with 8befc50c49e8a271fd3cd7fb34258fe88d1dfcad (also whatever
version I used before, erm, probably
de0db422782ddaf7754ac5b03fdc6dc5de1a9ae4), and possibly earlier
versions---I've just started noticing now that the number of loose
objects has started causing git gui to complain.
(Hmm, I note that git gui reports a larger number of loose objects
than git count-objects. Ah, OK, it really is just an approximation,
so no surprise.)
^ permalink raw reply
* Interactive rebase encoding
From: Constantine Plotnikov @ 2008-12-15 12:42 UTC (permalink / raw)
To: git
The interactive rebase command builds a text file and passes it to
editor specified as environment variable or as configuration
parameter. However the man page for rebase operation says nothing
about which encoding will be used for that file. Apparently
i18n.logoutputencoding is used. I think that the man page for rebase
operation should explicitly specify it.
Also it might make sense to use explicit encoding parameter for
interactive rebase operation.
Constantine
^ permalink raw reply
* Re: [PATCH] modify/delete conflict resolution overwrites untracked file
From: Mike Ralphson @ 2008-12-15 11:45 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Clemens Buchacher, git, raa.lkml
In-Reply-To: <alpine.DEB.1.00.0812151206360.30933@intel-tinevez-2-302>
2008/12/15 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> On Mon, 15 Dec 2008, Mike Ralphson wrote:
>> I wonder if another approach is workable... to read 'vulnerable'
>> untracked working tree files into a new (temporary, uncommittable) stage
>> in the index, perform whatever merging is required, then reinstate all
>> entries from the new stage.
>
> I think the solution is not in making things more complicated, but
> simpler. I agree with Junio that the recursive merge needs a major
> rewrite which respects d/f conflicts and renames in the _design_, not as
> an afterthought.
Yes, it might also score low on not surprising the user. I bow to more
experienced heads on matters of clean design and merge strategies.
> Besides, I really do not want untracked files to be inserted into a stage.
> Remember, adding something to the index means to hash it, and I do have
> half-a-gigabyte untracked data in some of my worktrees.
Granted, but here we are only talking about files (or clashing
file-names) which someone [else] has already added/removed/modified in
another branch etc.
Mike
^ permalink raw reply
* Re: [PATCH] modify/delete conflict resolution overwrites untracked file
From: Johannes Schindelin @ 2008-12-15 11:09 UTC (permalink / raw)
To: Mike Ralphson; +Cc: Junio C Hamano, Clemens Buchacher, git, raa.lkml
In-Reply-To: <e2b179460812150250t6e028330xf0e0ff626c1b6b3c@mail.gmail.com>
Hi,
On Mon, 15 Dec 2008, Mike Ralphson wrote:
> 2008/12/15 Junio C Hamano <gitster@pobox.com>:
> > Clemens Buchacher <drizzd@aon.at> writes:
> >
> >> On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:
> >>> merge-recursive: do not clobber untracked working tree garbage
> >>>
> >>> When merge-recursive wanted to create a new file in the work tree
> >>> (either as the final result, or a hint for reference purposes while
> >>> delete/modify conflicts), it unconditionally overwrote an untracked
> >>> file in the working tree. Be careful not to lose whatever the user
> >>> has that is not tracked.
> >>
> >> This leaves the index in an unmerged state, however, so that a
> >> subsequent git reset --hard still kills the file. And I just realized
> >> that the same goes for merge-resolve. So I'd prefer to abort the
> >> merge, leave everything unchanged and tell the user to clean up
> >> first.
> >
> > That is unfortunately asking for a moon, I am afraid.
> >
> > It needs a major restructuring of the code so that the recursive works
> > more like the way resolve works, namely, changing the final "writeout"
> > into two phase thing (the first phase making sure nothing is clobbered
> > in the work tree, and then the second phase actually touching the work
> > tree).
>
> I wonder if another approach is workable... to read 'vulnerable'
> untracked working tree files into a new (temporary, uncommittable) stage
> in the index, perform whatever merging is required, then reinstate all
> entries from the new stage.
I think the solution is not in making things more complicated, but
simpler. I agree with Junio that the recursive merge needs a major
rewrite which respects d/f conflicts and renames in the _design_, not as
an afterthought.
Besides, I really do not want untracked files to be inserted into a stage.
Remember, adding something to the index means to hash it, and I do have
half-a-gigabyte untracked data in some of my worktrees.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] modify/delete conflict resolution overwrites untracked file
From: Johannes Schindelin @ 2008-12-15 11:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Clemens Buchacher, git
In-Reply-To: <7vljuhwwtg.fsf@gitster.siamese.dyndns.org>
Hi,
On Mon, 15 Dec 2008, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> merge-recursive: do not clobber untracked working tree garbage
> >> ...
> >> +static int would_lose_untracked(const char *path)
> >> +{
> >> + int pos = cache_name_pos(path, strlen(path));
> >> +
> >> + if (pos < 0)
> >> + pos = -1 - pos;
> >> + while (pos < active_nr &&
> >> + !strcmp(path, active_cache[pos]->name)) {
> >> + /*
> >> + * If stage #0, it is definitely tracked.
> >> + * If it has stage #2 then it was tracked
> >> + * before this merge started. All other
> >> + * cases the path was not tracked.
> >> + */
> >> + switch (ce_stage(active_cache[pos])) {
> >> + case 0:
> >> + case 2:
> >> + return 0;
> >> + }
> >> + pos++;
> >> + }
> >> + return file_exists(path);
> >
> > I wonder if it is cheaper to test file_exists() when the index contains a
> > lot of files...
>
> "cheaper" than what?
Oops. I meant "cheaper to test file_exists() _first_". But thinking
about it again, it is probably way more expensive, especially in the cold
cache case.
Sorry for the noise,
Dscho
^ permalink raw reply
* Re: [patch] Fix a corner case in git update-index --index-info
From: Thomas Jarosch @ 2008-12-15 10:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7viqpn6fhz.fsf@gitster.siamese.dyndns.org>
On Saturday, 13. December 2008 20:29:12 you wrote:
> If you are doing a filter-branch and the commits near the beginning of the
> history did not have any path you are interested in, I do not think you
> would want to even create corresponding commits for them that record an
> empty tree to begin with, so I do not necessarily agree with the above
> command line. The mv would fail due to absense of index.new file, and you
> can take it as a sign that you can skip that commit.
True. I killed the empty commit later using rebase -i. Great tool :-)
> Outside the context of your command line above, I am slightly more
> sympathetic than neutral to the argument that "update-index --index-info"
> (and "update-index --stdin", which I suspect would have the same issue,
> but I did not check) should create an output file if one did not exist.
>
> You should note however that such a change would rob from you a way to
> detect that you did not feed anything to the command by checking the lack
> of the output. Such a change would break people's existing scripts that
> relied on the existing behaviour; one example is that the above "The mv
> would fail...and you can" would be made impossible.
That is also true. OTOH there would be no way to create an empty tree,
f.e. if you do positive filtering like --subdirectory-filter
just with multiple subdirs:
git filter-branch --tag-name-filter cat --index-filter \
'git ls-files -s |grep -P "\t(DIR1|DIR2)" \
|GIT_INDEX_FILE=$GIT_INDEX_FILE.new git update-index --index-info &&
mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE' -- --all
Later on I removed all empty commits in a second run.
> > + if (!found_something)
> > + active_cache_changed = 1;
> > +
> > strbuf_release(&buf);
> > strbuf_release(&uq);
> > }
>
> I think this implementation is conceptually wrong, even if we assume it is
> the right thing to always create a new file. The --index-info mode may
> well be fed with the same information as it already records, in which case
> active_cache_changed shouldn't be toggled, and if it is fed something
> different from what is recorded, active_cache_changed should be marked as
> changed, and that decision should be left to the add_cache_entry() that is
> called from add_cacheinfo(). What you did is to make it _always_ write
> the new index out, even if we started with an existing index, and there
> was no change, or even if we started with missing index, and there was no
> input. You only wanted the latter but you did both.
The idea was to toggle the active_cache_changed variable only if we didn't get
a single line of input from stdin. If you feed back the same index information
f.e. via "git ls-tree -s", the active_cache_changed=1 code shouldn't be
executed. Though I didn't explicitly test this case, so I guess you are right.
> But again, this would break people who have been relying on the existing
> behaviour that no resulting file, when GIT_INDEX_FILE points at a
> nonexistent file, signals no operation.
See my remark about "positive list" filtering above.
> I think it is a bad idea to do this in -rc period, even if we were to
> change the semantics.
Yes, this is something one doesn't want in a -rc :-)
Thanks for your implementation.
btw: I sent a small documentation update to the list and forgot to add you
to the CC: list. The subject line was
"[patch] documentation: Explain how to free up space after filter-branch"
Cheers,
Thomas
^ permalink raw reply
* Re: [PATCH] modify/delete conflict resolution overwrites untracked file
From: Mike Ralphson @ 2008-12-15 10:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Clemens Buchacher, git, johannes.schindelin, raa.lkml
In-Reply-To: <7vskopwxej.fsf@gitster.siamese.dyndns.org>
2008/12/15 Junio C Hamano <gitster@pobox.com>:
> Clemens Buchacher <drizzd@aon.at> writes:
>
>> On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:
>>> merge-recursive: do not clobber untracked working tree garbage
>>>
>>> When merge-recursive wanted to create a new file in the work tree (either
>>> as the final result, or a hint for reference purposes while delete/modify
>>> conflicts), it unconditionally overwrote an untracked file in the working
>>> tree. Be careful not to lose whatever the user has that is not tracked.
>>
>> This leaves the index in an unmerged state, however, so that a subsequent
>> git reset --hard still kills the file. And I just realized that the same
>> goes for merge-resolve. So I'd prefer to abort the merge, leave everything
>> unchanged and tell the user to clean up first.
>
> That is unfortunately asking for a moon, I am afraid.
>
> It needs a major restructuring of the code so that the recursive works
> more like the way resolve works, namely, changing the final "writeout"
> into two phase thing (the first phase making sure nothing is clobbered in
> the work tree, and then the second phase actually touching the work tree).
I wonder if another approach is workable... to read 'vulnerable'
untracked working tree files into a new (temporary, uncommittable)
stage in the index, perform whatever merging is required, then
reinstate all entries from the new stage.
Thus the merge should normally succeed under the covers, and the
previously untracked file(s) would now show up as modified against the
tracked content.
Two problems I foresee - potential loss of untracked metadata, and
ensuring we reinstate the untracked contents in all possible paths the
user can use to abort or resolve the merge.
Mike
^ permalink raw reply
* Re: [PATCH] modify/delete conflict resolution overwrites untracked file
From: Junio C Hamano @ 2008-12-15 10:35 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Clemens Buchacher, git
In-Reply-To: <alpine.DEB.1.00.0812151032230.14632@racer>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> merge-recursive: do not clobber untracked working tree garbage
>> ...
>> +static int would_lose_untracked(const char *path)
>> +{
>> + int pos = cache_name_pos(path, strlen(path));
>> +
>> + if (pos < 0)
>> + pos = -1 - pos;
>> + while (pos < active_nr &&
>> + !strcmp(path, active_cache[pos]->name)) {
>> + /*
>> + * If stage #0, it is definitely tracked.
>> + * If it has stage #2 then it was tracked
>> + * before this merge started. All other
>> + * cases the path was not tracked.
>> + */
>> + switch (ce_stage(active_cache[pos])) {
>> + case 0:
>> + case 2:
>> + return 0;
>> + }
>> + pos++;
>> + }
>> + return file_exists(path);
>
> I wonder if it is cheaper to test file_exists() when the index contains a
> lot of files...
"cheaper" than what?
The test with the index is not about efficiency, but is all about
correctness.
If the file in the work tree came from the index that represents "our"
branch, we do not want to say "yes" from this function, even when
(actually, "especially when") the path exists in the work tree.
unpack-trees decided that the path matches the index (otherwise you are
already guaranteeing that we wouldn't have come this far, right?) and we
are about to write out the (potentially partial) merge result to the
working tree file.
^ permalink raw reply
* Re: [PATCH] modify/delete conflict resolution overwrites untracked file
From: Junio C Hamano @ 2008-12-15 10:22 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git, johannes.schindelin, raa.lkml
In-Reply-To: <20081215095949.GA7403@localhost>
Clemens Buchacher <drizzd@aon.at> writes:
> On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:
>> merge-recursive: do not clobber untracked working tree garbage
>>
>> When merge-recursive wanted to create a new file in the work tree (either
>> as the final result, or a hint for reference purposes while delete/modify
>> conflicts), it unconditionally overwrote an untracked file in the working
>> tree. Be careful not to lose whatever the user has that is not tracked.
>
> This leaves the index in an unmerged state, however, so that a subsequent
> git reset --hard still kills the file. And I just realized that the same
> goes for merge-resolve. So I'd prefer to abort the merge, leave everything
> unchanged and tell the user to clean up first.
That is unfortunately asking for a moon, I am afraid.
It needs a major restructuring of the code so that the recursive works
more like the way resolve works, namely, changing the final "writeout"
into two phase thing (the first phase making sure nothing is clobbered in
the work tree, and then the second phase actually touching the work tree).
^ permalink raw reply
* Re: [ANNOUNCE] GIT 1.6.1-rc3
From: Junio C Hamano @ 2008-12-15 10:17 UTC (permalink / raw)
To: Anders Melchiorsen; +Cc: Junio C Hamano, raa.lkml, git
In-Reply-To: <34543.bFoQE3daRhY=.1229335460.squirrel@webmail.hotelhot.dk>
"Anders Melchiorsen" <mail@cup.kalibalik.dk> writes:
> The patch in that post improved things somewhat, by making a correct
> commit. Of course, the working tree then silently loses a file, which
> could be seen as being worse than a fatal failure.
Thanks for a reminder. When resending for discussion, please make it
accompanied with a test case. In any case, let's leave anything iffy
after the final.
^ permalink raw reply
* Re: [PATCH 2/2] rebase -i -p: Fix --continue after a merge could not be redone
From: Junio C Hamano @ 2008-12-15 10:16 UTC (permalink / raw)
To: Johannes Sixt
Cc: Johannes Schindelin, Andreas Ericsson, Stephen Haberman, git
In-Reply-To: <1229335531-32707-2-git-send-email-j6t@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> When a merge that has a conflict was rebased, then rebase stopped to let
> the user resolve the conflicts. However, thereafter --continue failed
> because the author-script was not saved. (This is rebase -i's way to
> preserve a commit's authorship.) This fixes it by doing taking the same
> failure route after a merge that is also taken after a normal cherry-pick.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> This is an attempt at fixing the failure. I don't know whether it is
> problematic to leave a "patch" behind if there was actually a merge.
> Nevertheless, all rebase tests pass.
Interesting approach. Dscho?
^ permalink raw reply
* [PATCH 1/2 updated] Show a failure of rebase -p if the merge had a conflict
From: Johannes Sixt @ 2008-12-15 10:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Andreas Ericsson, Stephen Haberman, git,
Johannes Sixt
In-Reply-To: <1229012461-31377-1-git-send-email-j6t@kdbg.org>
This extends t3409-rebase-preserve-merges by a case where the merge that
is rebased has a conflict. Therefore, the rebase stops and expects that
the user resolves the conflict. However, currently rebase --continue
fails because .git/rebase-merge/author-script is missing.
The test script had allocated two identical clones, but only one of them
(clone2) was used. Now we use both as indicated in the comment. Also,
two instances of && was missing in the setup part.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
This is a resend of the earlier patch with a minor update: I missed one
instance of && in the setup part, so there were actually two missing.
-- Hannes
t/t3409-rebase-preserve-merges.sh | 55 +++++++++++++++++++++++++++++-------
1 files changed, 44 insertions(+), 11 deletions(-)
diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index 8cde40f..5ddd1d1 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh
@@ -11,15 +11,23 @@ Run "git rebase -p" and check that merges are properly carried along
GIT_AUTHOR_EMAIL=bogus_email_address
export GIT_AUTHOR_EMAIL
-#echo 'Setting up:
+# Clone 1 (trivial merge):
#
-#A1--A2 <-- origin/master
-# \ \
-# B1--M <-- topic
-# \
-# B2 <-- origin/topic
+# A1--A2 <-- origin/master
+# \ \
+# B1--M <-- topic
+# \
+# B2 <-- origin/topic
#
-#'
+# Clone 2 (conflicting merge):
+#
+# A1--A2--B3 <-- origin/master
+# \ \
+# B1------M <-- topic
+# \
+# B2 <-- origin/topic
+#
+# In both cases, 'topic' is rebased onto 'origin/topic'.
test_expect_success 'setup for merge-preserving rebase' \
'echo First > A &&
@@ -37,12 +45,19 @@ test_expect_success 'setup for merge-preserving rebase' \
cd clone1 &&
git checkout -b topic origin/topic &&
git merge origin/master &&
- cd ..
+ cd .. &&
+
+ echo Fifth > B &&
+ git add B &&
+ git commit -m "Add different B" &&
- git clone ./. clone2
+ git clone ./. clone2 &&
cd clone2 &&
git checkout -b topic origin/topic &&
- git merge origin/master &&
+ test_must_fail git merge origin/master &&
+ echo Resolved > B &&
+ git add B &&
+ git commit -m "Merge origin/master into topic" &&
cd .. &&
git checkout topic &&
@@ -51,11 +66,29 @@ test_expect_success 'setup for merge-preserving rebase' \
'
test_expect_success 'rebase -p fakes interactive rebase' '
- cd clone2 &&
+ (
+ cd clone1 &&
git fetch &&
git rebase -p origin/topic &&
test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
test 1 = $(git rev-list --all --pretty=oneline | grep "Merge commit" | wc -l)
+ )
+'
+
+test_expect_failure '--continue works after a conflict' '
+ (
+ cd clone2 &&
+ git fetch &&
+ test_must_fail git rebase -p origin/topic &&
+ test 2 = $(git ls-files B | wc -l) &&
+ echo Resolved again > B &&
+ test_must_fail git rebase --continue &&
+ git add B &&
+ git rebase --continue &&
+ test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
+ test 1 = $(git rev-list --all --pretty=oneline | grep "Add different" | wc -l) &&
+ test 1 = $(git rev-list --all --pretty=oneline | grep "Merge origin" | wc -l)
+ )
'
test_done
--
1.6.1.rc2.22.gf3bf84
^ permalink raw reply related
* [PATCH 2/2] rebase -i -p: Fix --continue after a merge could not be redone
From: Johannes Sixt @ 2008-12-15 10:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Andreas Ericsson, Stephen Haberman, git,
Johannes Sixt
In-Reply-To: <1229335531-32707-1-git-send-email-j6t@kdbg.org>
When a merge that has a conflict was rebased, then rebase stopped to let
the user resolve the conflicts. However, thereafter --continue failed
because the author-script was not saved. (This is rebase -i's way to
preserve a commit's authorship.) This fixes it by doing taking the same
failure route after a merge that is also taken after a normal cherry-pick.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
This is an attempt at fixing the failure. I don't know whether it is
problematic to leave a "patch" behind if there was actually a merge.
Nevertheless, all rebase tests pass.
-- Hannes
git-rebase--interactive.sh | 3 +--
t/t3409-rebase-preserve-merges.sh | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 1172e47..89c39eb 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -253,15 +253,14 @@ pick_one_preserving_merges () {
if ! GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
output git merge $STRATEGY -m "$msg" \
$new_parents
then
- git rerere
printf "%s\n" "$msg" > "$GIT_DIR"/MERGE_MSG
- die Error redoing merge $sha1
+ die_with_patch $sha1 "Error redoing merge $sha1"
fi
;;
*)
output git cherry-pick "$@" ||
die_with_patch $sha1 "Could not pick $sha1"
;;
diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index 5ddd1d1..820e010 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh
@@ -72,13 +72,13 @@ test_expect_success 'rebase -p fakes interactive rebase' '
git rebase -p origin/topic &&
test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
test 1 = $(git rev-list --all --pretty=oneline | grep "Merge commit" | wc -l)
)
'
-test_expect_failure '--continue works after a conflict' '
+test_expect_success '--continue works after a conflict' '
(
cd clone2 &&
git fetch &&
test_must_fail git rebase -p origin/topic &&
test 2 = $(git ls-files B | wc -l) &&
echo Resolved again > B &&
--
1.6.1.rc2.22.gf3bf84
^ permalink raw reply related
* Re: [ANNOUNCE] GIT 1.6.1-rc3
From: Anders Melchiorsen @ 2008-12-15 10:04 UTC (permalink / raw)
To: Junio C Hamano, raa.lkml; +Cc: git
In-Reply-To: <7voczdyh23.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> I have fixed a few more smallish (old) bugs after I tagged this, which
> will be in 'master' shortly, but it seems that this cycle is stabilizing
> fairly nicely. Let's have a successful 1.6.1 tagged on 20th. Please hunt
> and fix bugs until then.
Even though it is not a 1.6.1 regression, I wonder about the merge
conflict issue in
http://thread.gmane.org/gmane.comp.version-control.git/100859/focus=101182
The patch in that post improved things somewhat, by making a correct
commit. Of course, the working tree then silently loses a file, which
could be seen as being worse than a fatal failure.
Anders.
^ permalink raw reply
* Re: [PATCH] modify/delete conflict resolution overwrites untracked file
From: Clemens Buchacher @ 2008-12-15 9:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, johannes.schindelin, raa.lkml
In-Reply-To: <7vmyeyyuuh.fsf@gitster.siamese.dyndns.org>
On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:
> merge-recursive: do not clobber untracked working tree garbage
>
> When merge-recursive wanted to create a new file in the work tree (either
> as the final result, or a hint for reference purposes while delete/modify
> conflicts), it unconditionally overwrote an untracked file in the working
> tree. Be careful not to lose whatever the user has that is not tracked.
This leaves the index in an unmerged state, however, so that a subsequent
git reset --hard still kills the file. And I just realized that the same
goes for merge-resolve. So I'd prefer to abort the merge, leave everything
unchanged and tell the user to clean up first.
^ permalink raw reply
* Re: git log --numstat disagrees with git apply --numstat
From: Junio C Hamano @ 2008-12-15 9:57 UTC (permalink / raw)
To: Jeff King; +Cc: Shawn O. Pearce, git
In-Reply-To: <20081212022156.GC23128@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> BTW, I got a little confused looking at the parameters to xdi_diff_outf,
> since ecb gets passed in full of random garbage. I don't know if this
> cleanup is worth applying:
I think this makes sense, but let's do this after 1.6.1 final. It does
not fix anything, and I'd rather avoid distraction.
^ permalink raw reply
* Re: [PATCH] Get rid of the last remnants of GIT_CONFIG_LOCAL
From: Johannes Schindelin @ 2008-12-15 9:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd4fu1d4w.fsf@gitster.siamese.dyndns.org>
Hi,
On Sun, 14 Dec 2008, Junio C Hamano wrote:
> Thanks.
Heh, am I that rare a contributor now that you start to thank me for my
patches? ;-)
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] modify/delete conflict resolution overwrites untracked file
From: Johannes Schindelin @ 2008-12-15 9:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Clemens Buchacher, git
In-Reply-To: <7vmyeyyuuh.fsf@gitster.siamese.dyndns.org>
Hi,
On Sun, 14 Dec 2008, Junio C Hamano wrote:
> merge-recursive: do not clobber untracked working tree garbage
>
> When merge-recursive wanted to create a new file in the work tree (either
> as the final result, or a hint for reference purposes while delete/modify
> conflicts), it unconditionally overwrote an untracked file in the working
> tree. Be careful not to lose whatever the user has that is not tracked.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
Thanks, I had no time at all to look into this issue.
> diff --git c/merge-recursive.c w/merge-recursive.c
> index a0c804c..2da4333 100644
> --- c/merge-recursive.c
> +++ w/merge-recursive.c
> @@ -447,6 +447,30 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)
> }
> }
>
> +static int would_lose_untracked(const char *path)
> +{
> + int pos = cache_name_pos(path, strlen(path));
> +
> + if (pos < 0)
> + pos = -1 - pos;
> + while (pos < active_nr &&
> + !strcmp(path, active_cache[pos]->name)) {
> + /*
> + * If stage #0, it is definitely tracked.
> + * If it has stage #2 then it was tracked
> + * before this merge started. All other
> + * cases the path was not tracked.
> + */
> + switch (ce_stage(active_cache[pos])) {
> + case 0:
> + case 2:
> + return 0;
> + }
> + pos++;
> + }
> + return file_exists(path);
I wonder if it is cheaper to test file_exists() when the index contains a
lot of files...
Thanks,
Dscho
^ permalink raw reply
* Re: is gitosis secure?
From: Mike Hommey @ 2008-12-15 8:35 UTC (permalink / raw)
To: david; +Cc: martin, git
In-Reply-To: <alpine.DEB.1.10.0812150024080.17688@asgard.lang.hm>
On Mon, Dec 15, 2008 at 12:25:53AM -0800, david@lang.hm wrote:
> On Mon, 15 Dec 2008, Mike Hommey wrote:
>
>> On Sun, Dec 14, 2008 at 05:00:14PM -0800, david@lang.hm wrote:
>>> 1. if you are running multiple different applications that all want to be
>>> exposed via port 22 (like git for 'git push') then you may need to expose
>>> numerous machines. tools that use SSH don't tend to have the ability to
>>> use a gateway box before they start executing commands, they assume that
>>> you will SSH directly into the destination box.
>>
>> But ssh itself allows you to do proxying. See ProxyCommand in
>> ssh_config's manpage.
>
> I was not aware of that option, but it looks like it's designed to be one
> setting for all your ssh communications, so unless you always use the
> same gateway box to get to your destination you would need to tweak your
> ssh config for each different thing that you are doing.
Take a look at Host in the same man page.
Mike
^ permalink raw reply
* [ANNOUNCE] GIT 1.6.1-rc3
From: Junio C Hamano @ 2008-12-15 8:32 UTC (permalink / raw)
To: git; +Cc: linux-kernel
In-Reply-To: <7vbpvnbcoa.fsf@gitster.siamese.dyndns.org>
I have fixed a few more smallish (old) bugs after I tagged this, which
will be in 'master' shortly, but it seems that this cycle is stabilizing
fairly nicely. Let's have a successful 1.6.1 tagged on 20th. Please hunt
and fix bugs until then.
http://www.kernel.org/pub/software/scm/git/
git-1.6.1-rc3.tar.{gz,bz2} (source tarball)
git-htmldocs-1.6.1-rc3.tar.{gz,bz2} (preformatted docs)
git-manpages-1.6.1-rc3.tar.{gz,bz2} (preformatted docs)
The RPM binary packages for a few architectures are also there.
testing/git-*-1.6.1-rc3-1.fc9.$arch.rpm (RPM)
----------------------------------------------------------------
Changes since v1.6.1-rc2 are as follows:
Alexander Gavrilov (1):
Documentation: Describe git-gui Tools menu configuration options.
Alexander Potashev (2):
Fix typos in documentation
Fix typo in comment in builtin-add.c
Alexey Borzenkov (1):
Define linkgit macro in [macros] section
Brandon Casey (1):
git-branch: display sha1 on branch deletion
Deskin Miller (1):
git-svn: Make following parents atomic
Jakub Narebski (1):
gitweb: Fix bug in insert_file() subroutine
Jeff King (5):
reorder ALLOW_TEXTCONV option setting
diff: allow turning on textconv explicitly for plumbing
diff: fix handling of binary rewrite diffs
diff: respect textconv in rewrite diffs
rebase: improve error messages about dirty state
Jim Meyering (1):
git-config.txt: fix a typo
Johannes Schindelin (1):
Get rid of the last remnants of GIT_CONFIG_LOCAL
Junio C Hamano (4):
builtin-checkout.c: check error return from read_cache()
read-cache.c: typofix in comment
work around Python warnings from AsciiDoc
Fix t4031
Linus Torvalds (1):
fsck: reduce stack footprint
Markus Heidelberg (1):
builtin-commit: remove unused message variable
Nicolas Pitre (1):
make sure packs to be replaced are closed beforehand
Ralf Wildenhues (1):
Improve language in git-merge.txt and related docs
Tor Arvid Lund (1):
git-p4: Fix regression in p4Where method.
YONETANI Tomokazu (1):
git-fast-import possible memory corruption problem
^ 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