* Re: [RFC/PATCH] commit: make the error message on unmerged entries user-friendly.
From: Matthieu Moy @ 2010-01-06 17:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7veim3yx5o.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> Two questions:
>>
>> * Did anyone actually use the 3 "file: unmerged (sha1)" lines?
>
> I don't think anybody does these days, and even if the question were about
> the historical practice, I doubt anybody used "git merge-file" using these
> blob object names to come up with a resolution. Besides, if they really
> want to, they can ask "ls-files -u" for that information themselves.
OK, that's what I thought, but 'wanted to be sure.
>I imagine you would say
> something like this?
>
> With this patch, the error looks like this:
>
> $ git commit
> U foo.txt
> fatal: Commit is not possible because you have unmerged files.
> Please, resolve the conflicts listed above, and then mark them
> as resolved with 'git add <filename>', or use 'git commit -a'
Yes, sorry for not making it more explicit.
> Do I like it? Hmmm.
>
> - "Please, ", especially with the comma, looks superfluous;
I wouldn't call it mandatory, but it doesn't harm, and
$ grep -i -n -e '"Please' *.c
builtin-commit.c:246: "Please, resolve the conflicts listed above, and then mark them\n"
builtin-commit.c:698: "Please supply the message using either -m or -F option.\n");
builtin-help.c:209: "Please consider using 'man.<tool>.cmd' instead.",
builtin-help.c:221: "Please consider using 'man.<tool>.path' instead.",
builtin-remote.c:1124: "Please choose one explicitly with:");
builtin-tag.c:321: "Please supply the message using either -m or -F option.\n");
merge-recursive.c:1191: "Please, commit your changes or stash them before you can merge.";
setup.c:241: warning("Please upgrade Git");
> - Some people consider "please resolve the conflicts" to mean the whole
> process of "fix them up in the work tree, and mark them resolved in the
> index", and others consider it to mean only the first step to "fix them
> up in the work tree". I happen to be in the former camp, and to me
> ",and then mark them as..., add <filename>'," look superfluous because
> of that.
>
> I however think it is more helpful to new people who benefit from this
> advice message if we spell out these two steps.
Yes. Someone having "resolved" (i.e. removed markers), and trying to
commit should get a hint on the commands he can use to achieve this
goal.
> Unfortunately, for that purpose, the description of the latter
> "mark them resolved in the index" step you have looks inadequate;
> e.g. sometimes it needs to be done with "git rm <filename>".
Maybe "sometimes", but to me, that's sufficiently rare to be omited
here (I don't think I ever used 'git rm' to resolve a conflict). The
user manual says this:
,----
| Each time you resolve the conflicts in a file and update the index:
|
| $ git add file.txt
|
| the different stages of that file will be "collapsed", after which git
| diff will (by default) no longer show diffs for that file.
`----
and I don't think it makes sense to try to be more exhaustive here
than in the user-manual.
> The need to give this advice on how to resolve conflicts is shared among
> commands like 'git merge', 'git cherry-pick', and 'git status', to name a
> few.
Not sure 'status' needs anything more. It already says
# Unmerged paths:
# (use "git add/rm <file>..." as appropriate to mark resolution)
#
# both modified: foo.txt
and the big difference between 'git status' and the others is that
it's legitimate to run it while resolving conflicts, while calling
'git merge' or 'git commit' can be done only by mistake.
It's not serious to eat 3 or 4 lines of the screen to display a
message to tell the user he shouldn't have done this, but it'd be
disturbing to eat more than 1 line for a common case.
> I think we should consolidate them all.
Right, although "commit" is definitely the most important (dumb users
don't need "git merge").
> - Decide if we go one-step (i.e. just say "resolve conflicts" and nothing
> else) or two-step (i.e. say "Fix them up in the work tree" and "mark
> them resolved in the index") approach; I am leaning toward the latter.
Meetoo.
> - Decide the exact language used. I think "Fix them up in the work tree"
> is passably okay, but "Mark them resolved in the index" needs to be
> made more concrete, and "git add <filename>" is not quite it, I am
> afraid.
See above. To me, pointing to "git commit -a" and "git add" is
sufficient.
Pointing to "git commit -a" is also important to me, because Git
newbies may have been told to always use "git commit" with "-a"
(common use-case: "I have to use Git, I know SVN and I don't want to
learn anything new").
> - Omit issuing the advisory message when advice.resolveConflict is set to
> false.
Sensible, yes.
> - Perhaps have a common helper function to do this from the commands that
> need to give the advice.
Probably not, because the advice will be different:
git merge => please resolve and commit before you can merge
git commit => please resolve before you can commit
...
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: submodules' shortcomings, was Re: RFC: display dirty submodule working directory in git gui and gitk
From: Junio C Hamano @ 2010-01-06 17:55 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Jens Lehmann, Johannes Schindelin, Git Mailing List,
Shawn O. Pearce, Paul Mackerras, Heiko Voigt, Lars Hjemli,
Avery Pennarun
In-Reply-To: <fcaeb9bf1001060923m6559f00bp794bb5fdd4af704c@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> On 1/7/10, Junio C Hamano <gitster@pobox.com> wrote:
>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>
>>
>> > Am 06.01.2010 02:04, schrieb Junio C Hamano:
>> >> After all we _were_ in agreement. We both think the user should be told
>> >> about untracked files in submodule directory when inspecting the status to
>> >> make a commit in the superproject.
>> >
>> > Thanks. So i'll take a closer look at the diff core (but i suspect i'll
>> > need some time until i can come up with some patches because i don't know
>> > this part of git very well).
>>
>>
>> I don't see a direct connection between "the user should be told about
>> untracked in the submodule before committing" and diffcore. It is just
>> the matter of "git status" and "git commit" running another instance of
>> "git status" via run_command() interface in the submodule directory, no?
>
> You would need to rewrite file paths so that files in submodules are
> also relative to the same directory as files in supermodule (I tried
> to do that with GIT_WORK_TREE and needed to change a bit). Or you
> could show each "git status" output separately, which does not look as
> nice as the former in my opinion.
You could show output separately if you want, but I think that is a
separate issue.
I was envisioning that the "git status" in submodule will be run with its
recent --porcelain option, and "git status" or "git commit" would read it
to postprocess and incorporate into its own output.
^ permalink raw reply
* gc.auto bug in 1.6.5.7
From: Jon Nelson @ 2010-01-06 17:59 UTC (permalink / raw)
To: git
It would appear that there is a bug in git, at least as of 1.6.5.7, in
the gc command.
When gc.auto is non-zero, it appears to consider packfiles as "loose"
- this conflicts with pack.packSizeLimit, insofar as I have
pack.packSizeLimit set to 2M (which appears to work). If gc.auto is,
say, 50 - once I exceed 50 2MB packs then gc --auto repacks *every
single time* it is run. This is unexpected, at least.
--
Jon
^ permalink raw reply
* Re: [RFC/PATCH] commit: make the error message on unmerged entries user-friendly.
From: Junio C Hamano @ 2010-01-06 18:15 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <vpqtyuz86ny.fsf@bauges.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Maybe "sometimes", but to me, that's sufficiently rare to be omited
> here (I don't think I ever used 'git rm' to resolve a conflict). The
> user manual says this:
>
> ,----
> | Each time you resolve the conflicts in a file and update the index:
> |
> | $ git add file.txt
> |
> | the different stages of that file will be "collapsed", after which git
> | diff will (by default) no longer show diffs for that file.
> `----
>
> and I don't think it makes sense to try to be more exhaustive here
> than in the user-manual.
I tend to disagree. The user-manual, as it currently stands, is an
introductory document that covers the concepts and the surface of git.
It's purpose is not to replace the main manual, but to give the necessary
foundation _before_ people read the manual.
>> The need to give this advice on how to resolve conflicts is shared among
>> commands like 'git merge', 'git cherry-pick', and 'git status', to name a
>> few.
>
> Not sure 'status' needs anything more. It already says
>
> # Unmerged paths:
> # (use "git add/rm <file>..." as appropriate to mark resolution)
> #
> # both modified: foo.txt
This message was exactly what I was getting at. The message in the
parentheses is what I called "advice". As I was suggesting to share the
advice messages used by commands that deal with mergy situations (like the
one you added in your patch), looking at existing examples to find the
best one and using that in other places would be the most effective
approach. That one line we see above is concise and does mention "rm" as
well. Why not use it?
>> I think we should consolidate them all.
>
> Right, although "commit" is definitely the most important (dumb users
> don't need "git merge").
Your "dumb users" don't get the unmerged error from commit, either, if
they don't need "git merge".
^ permalink raw reply
* Re: [PATCH v2] Makefile: make ppc/sha1ppc.o depend on GIT-CFLAGS
From: Nicolas Pitre @ 2010-01-06 18:17 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git, Nanako Shiraishi
In-Reply-To: <20100106063759.GA882@progeny.tock>
On Wed, 6 Jan 2010, Jonathan Nieder wrote:
> The %.o: %.S pattern rule should depend on GIT-CFLAGS to avoid
> trouble when ALL_CFLAGS changes.
>
> The pattern only applies to one file (ppc/sha1ppc.S) and that
> file does not use any #ifdefs, so leaving the dependency out is
> probably harmless. Nevertheless, it is safer to include the
> dependency in case future code's behavior does depend on the
> build flags.
Is the PPC SHA1 code still needed? Is it actually ever used?
We have block-sha1/* which is already about just as fast if not faster
on most targets. So I'd simply remove the ppc directory and its content
which is the only remaining platform specific assembly code in the
source tree.
Nicolas
^ permalink raw reply
* Re: submodules' shortcomings, was Re: RFC: display dirty submodule working directory in git gui and gitk
From: Jens Lehmann @ 2010-01-06 18:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Git Mailing List, Shawn O. Pearce,
Paul Mackerras, Heiko Voigt, Lars Hjemli, Avery Pennarun
In-Reply-To: <7vbph7181x.fsf@alter.siamese.dyndns.org>
Am 06.01.2010 18:01, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
>> Am 06.01.2010 02:04, schrieb Junio C Hamano:
>>> After all we _were_ in agreement. We both think the user should be told
>>> about untracked files in submodule directory when inspecting the status to
>>> make a commit in the superproject.
>>
>> Thanks. So i'll take a closer look at the diff core (but i suspect i'll
>> need some time until i can come up with some patches because i don't know
>> this part of git very well).
>
> I don't see a direct connection between "the user should be told about
> untracked in the submodule before committing" and diffcore. It is just
> the matter of "git status" and "git commit" running another instance of
> "git status" via run_command() interface in the submodule directory, no?
Basically yes. But i also would like to teach "git diff" (when diffing
against the working directory of the superproject) to show these
submodule states too so that git gui and gitk will display them.
^ permalink raw reply
* Re: submodules' shortcomings, was Re: RFC: display dirty submodule working directory in git gui and gitk
From: Nguyen Thai Ngoc Duy @ 2010-01-06 18:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jens Lehmann, Johannes Schindelin, Git Mailing List,
Shawn O. Pearce, Paul Mackerras, Heiko Voigt, Lars Hjemli,
Avery Pennarun
In-Reply-To: <7vljgbw21x.fsf@alter.siamese.dyndns.org>
On 1/7/10, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> > You would need to rewrite file paths so that files in submodules are
> > also relative to the same directory as files in supermodule (I tried
> > to do that with GIT_WORK_TREE and needed to change a bit). Or you
> > could show each "git status" output separately, which does not look as
> > nice as the former in my opinion.
>
>
> You could show output separately if you want, but I think that is a
> separate issue.
>
> I was envisioning that the "git status" in submodule will be run with its
> recent --porcelain option, and "git status" or "git commit" would read it
> to postprocess and incorporate into its own output.
Nice option! I had to call a few "git diff" for that just because I
did not catch up with recent Git development :-(
--
Duy
^ permalink raw reply
* Re: gc.auto bug in 1.6.5.7
From: Brandon Casey @ 2010-01-06 18:31 UTC (permalink / raw)
To: Jon Nelson; +Cc: git
In-Reply-To: <cccedfc61001060959h72a07eb6g78f2ca231867df63@mail.gmail.com>
Jon Nelson wrote:
> It would appear that there is a bug in git, at least as of 1.6.5.7, in
> the gc command.
> When gc.auto is non-zero, it appears to consider packfiles as "loose"
> - this conflicts with pack.packSizeLimit, insofar as I have
> pack.packSizeLimit set to 2M (which appears to work). If gc.auto is,
> say, 50 - once I exceed 50 2MB packs then gc --auto repacks *every
> single time* it is run. This is unexpected, at least.
There is also a gc.autopacklimit which controls auto-gc'ing based on
the number of packs. The default is 50 and it does not consider the
pack.packSizeLimit setting when deciding whether or not to auto-gc.
It can be raised, or it can be disabled by setting it to zero.
The ideal number of packs is one, but if you limit the size of packs
to be very small, then you will quickly grow the number of packs.
Perhaps autogc should only count packs that are much smaller than
pack.packSizeLimit? Looks like there is already a comment in
builtin-gc.c suggesting something similar.
-brandon
^ permalink raw reply
* Re: submodules' shortcomings, was Re: RFC: display dirty submodule working directory in git gui and gitk
From: Jens Lehmann @ 2010-01-06 18:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyen Thai Ngoc Duy, Johannes Schindelin, Git Mailing List,
Shawn O. Pearce, Paul Mackerras, Heiko Voigt, Lars Hjemli,
Avery Pennarun
In-Reply-To: <7vljgbw21x.fsf@alter.siamese.dyndns.org>
Am 06.01.2010 18:55, schrieb Junio C Hamano:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> On 1/7/10, Junio C Hamano <gitster@pobox.com> wrote:
>>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>>
>>>
>>>> Am 06.01.2010 02:04, schrieb Junio C Hamano:
>>> >> After all we _were_ in agreement. We both think the user should be told
>>> >> about untracked files in submodule directory when inspecting the status to
>>> >> make a commit in the superproject.
>>> >
>>> > Thanks. So i'll take a closer look at the diff core (but i suspect i'll
>>> > need some time until i can come up with some patches because i don't know
>>> > this part of git very well).
>>>
>>>
>>> I don't see a direct connection between "the user should be told about
>>> untracked in the submodule before committing" and diffcore. It is just
>>> the matter of "git status" and "git commit" running another instance of
>>> "git status" via run_command() interface in the submodule directory, no?
>>
>> You would need to rewrite file paths so that files in submodules are
>> also relative to the same directory as files in supermodule (I tried
>> to do that with GIT_WORK_TREE and needed to change a bit). Or you
>> could show each "git status" output separately, which does not look as
>> nice as the former in my opinion.
>
> You could show output separately if you want, but I think that is a
> separate issue.
>
> I was envisioning that the "git status" in submodule will be run with its
> recent --porcelain option, and "git status" or "git commit" would read it
> to postprocess and incorporate into its own output.
And i thought about printing just one line for each dirty submodule that
contains uncommitted and/or new files. I did not intend to list every
file, for the same reason a "git diff --submodule" only shows the first
line of the commit messages, not the actual differences of all changed
files in the submodule. I am not against being able to show all files
too, but i really would want to have an option to get a short output for
git gui and gitk.
^ permalink raw reply
* Re: [RFC/PATCH] lockfile: show absolute filename in unable_to_lock_message.
From: Johannes Sixt @ 2010-01-06 18:50 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <1262797288-23004-1-git-send-email-Matthieu.Moy@imag.fr>
Matthieu Moy schrieb:
> - path, strerror(err));
> + make_absolute_path(path), strerror(err));
> } else
> - strbuf_addf(&buf, "Unable to create '%s.lock': %s", path, strerror(err));
> + strbuf_addf(&buf, "Unable to create '%s.lock': %s",
> + make_absolute_path(path), strerror(err));
Since these are merely informative, wouldn't it be more suitable to use
make_nonrelative_path()?
-- Hannes
^ permalink raw reply
* git pull --all fails (possible options handling bug in git pull?)
From: Zing @ 2010-01-06 19:05 UTC (permalink / raw)
To: git
In 1.6.6, if I do:
$ git pull --all
Fetching origin
Fetching gnome
You asked to pull from the remote '--all', but did not specify
a branch. Because this is not the default configured remote
for your current branch, you must specify a branch on the command line.
The fetch all remotes works, but shouldn't the "--all" option only apply
to the fetch operation? It looks like it's getting noticed by the merge
part of the pull.
^ permalink raw reply
* Re: git file sharing/versioning.
From: Nicolas Sebrecht @ 2010-01-06 19:42 UTC (permalink / raw)
To: Bas Driessen; +Cc: Nicolas Sebrecht, git
In-Reply-To: <1262732791.14714.7.camel@ams.xobas.net>
[ Please, don't top post and don't answer to me in private for public
questions. ]
The 06/01/10, Bas Driessen wrote:
> Btw, this list does not appear to be a "user" list, more a "developer"
> list. Is there a list specifically for users only?
They are the manual pages... for end-users.
--
Nicolas Sebrecht
^ permalink raw reply
* Re: [RFC/PATCH] commit: make the error message on unmerged entries user-friendly.
From: Matthieu Moy @ 2010-01-06 19:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8wcbw14f.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> # Unmerged paths:
>> # (use "git add/rm <file>..." as appropriate to mark resolution)
[...]
> That one line we see above is concise and does mention "rm" as
> well. Why not use it?
Fine, you convinced me. I was thinking of a longer sentence, but "/rm"
doesn't harm.
>>> I think we should consolidate them all.
>>
>> Right, although "commit" is definitely the most important (dumb users
>> don't need "git merge").
>
> Your "dumb users" don't get the unmerged error from commit, either, if
> they don't need "git merge".
They'd use "pull", not merge. Anyway, I did it for commit, merge,
pull, revert, cherry-pick. I guess we covered the common cases.
The patch seems to have a lot of redundancies, but I think trying to
factor this into helper functions would be much more effort than the
few cut-and-paste that I had to do, since each instance is a slight
variant of each other ...
Patch follows. Let me know in case you prefer to split it.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: submodules' shortcomings, was Re: RFC: display dirty submodule working directory in git gui and gitk
From: Junio C Hamano @ 2010-01-06 20:01 UTC (permalink / raw)
To: Jens Lehmann
Cc: Nguyen Thai Ngoc Duy, Johannes Schindelin, Git Mailing List,
Shawn O. Pearce, Paul Mackerras, Heiko Voigt, Lars Hjemli,
Avery Pennarun
In-Reply-To: <4B44D73F.6000607@web.de>
Jens Lehmann <Jens.Lehmann@web.de> writes:
> Am 06.01.2010 18:55, schrieb Junio C Hamano:
>> I was envisioning that the "git status" in submodule will be run with its
>> recent --porcelain option, and "git status" or "git commit" would read it
>> to postprocess and incorporate into its own output.
>
> And i thought about printing just one line for each dirty submodule that
> contains uncommitted and/or new files. I did not intend to list every
> file, for the same reason a "git diff --submodule" only shows the first
> line of the commit messages, not the actual differences of all changed
> files in the submodule. I am not against being able to show all files
> too, but i really would want to have an option to get a short output for
> git gui and gitk.
I don't think what you are saying is inconsistent with "git status/commit
that reads from 'git status --porcelain' it runs in a submodule directory,
postprocesses it and incorporates it into its own output." When the
sub-status reports changes, your "postprocess" would condense it down to
"this has a potential change that user could want to commit". How the
dirtiness is shown is entirely up to the caller that detected that change.
Let's explain it in another way.
The original "diff" for a submodule entry was implemented by preparing a
"Subproject commit %s\n"
line for the submodule commit recorded in the preimage and postimage, and
compare these as if they are one-line files. When the postimage was work
tree, it looked at submodule's .git/HEAD to learn what to stuff in %s
there.
But nobody forced you to limit the check only to .git/HEAD in the
submodule. To make the comparison richer, you could check if the
submodule directory is dirty (and we have already discussed the potential
definition of dirtiness earlier), and add "-dirty" in the string as well.
With such a change, if you make some changes to a file in the work tree of
the submodule after a clean "clone", "git diff" between the index and the
work tree would report:
-Subproject commit 37bae10e38a66e4f1ddd5350daded00b21735126
+Subproject commit 37bae10e38a66e4f1ddd5350daded00b21735126-dirty
The suggestion to read from "status --porcelain" that is run in the
submodule directory was about how to implement the part that determines
this "dirtiness" information, and not about how that dirtiness is
expressed in the output. The above is an illustration that even the
traditional output format can be made aware of this submodule dirtiness
check. "diff --submodule" can express that dirtiness information in any
way it wants.
^ permalink raw reply
* Re: [RFC/PATCH] commit: make the error message on unmerged entries user-friendly.
From: Junio C Hamano @ 2010-01-06 20:05 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <vpqocl76mdo.fsf@bauges.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> They'd use "pull", not merge. Anyway, I did it for commit, merge,
> pull, revert, cherry-pick. I guess we covered the common cases.
> The patch seems to have a lot of redundancies, but I think trying to
> factor this into helper functions would be much more effort than the
> few cut-and-paste that I had to do, since each instance is a slight
> variant of each other ...
I'd be more worried about longer term maintainability than one time
expediency of producing your single patch to add these messages. If the
messages are cast in stone, we can just verify they are consistent _now_
and forget about them, but I suspect not even you are perfect to predict
that we won't come up with different/better ways to resolve and mark them
resolved in the future and write a set of messages that won't have to
change.
^ permalink raw reply
* [PATCH 1/2] git-svn: ignore changeless commits when checking for a cherry-pick
From: Andrew Myrick @ 2010-01-06 20:08 UTC (permalink / raw)
To: git; +Cc: sam, normalperson, Andrew Myrick
Update git-svn to ignore commits that do not change the tree when it is
deciding if an svn merge ticket represents a real branch merge or just a
cherry-pick.
Consider the following integration model in the svn repository:
F---G branch1
/ \
D tag1 \ E tag2
/ \ /
A---B C trunk
branch1 is merged to trunk in commit C.
With this patch, git-svn will correctly identify branch1 as a proper merge
parent, instead of incorrectly ignoring it as a cherry-pick.
Signed-off-by: Andrew Myrick <amyrick@apple.com>
---
git-svn.perl | 24 ++++++++++++++++++++++++
1 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 650c9e5..8c7c034 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3052,12 +3052,36 @@ sub check_cherry_pick {
for my $range ( @ranges ) {
delete @commits{_rev_list($range)};
}
+ for my $commit (keys %commits) {
+ if (has_no_changes($commit)) {
+ delete $commits{$commit};
+ }
+ }
return (keys %commits);
}
+sub has_no_changes {
+ my $commit = shift;
+
+ my @revs = split / /, command_oneline(
+ qw(rev-list --parents -1 -m), $commit);
+
+ # Commits with no parents, e.g. the start of a partial branch,
+ # have changes by definition.
+ return 1 if (@revs < 2);
+
+ # Commits with multiple parents, e.g a merge, have no changes
+ # by definition.
+ return 0 if (@revs > 2);
+
+ return (command_oneline("rev-parse", "$commit^{tree}") eq
+ command_oneline("rev-parse", "$commit~1^{tree}"));
+}
+
BEGIN {
memoize 'lookup_svn_merge';
memoize 'check_cherry_pick';
+ memoize 'has_no_changes';
}
sub parents_exclude {
--
1.6.6.2.g18c9a
^ permalink raw reply related
* [PATCH 2/2] git-svn: handle merge-base failures
From: Andrew Myrick @ 2010-01-06 20:08 UTC (permalink / raw)
To: git; +Cc: sam, normalperson, Andrew Myrick
In-Reply-To: <1262808529-1575-1-git-send-email-amyrick@apple.com>
Change git-svn to warn and continue when merge-base fails while processing svn
merge tickets.
merge-base can fail when a partial branch is created and merged back to trunk
in svn, because it cannot find a common ancestor between the partial branch and
trunk.
Signed-off-by: Andrew Myrick <amyrick@apple.com>
---
git-svn.perl | 19 +++++++++++++++----
1 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 8c7c034..d58982b 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3158,10 +3158,21 @@ sub find_extra_svn_parents {
my $ranges = $ranges{$merge_tip};
# check out 'new' tips
- my $merge_base = command_oneline(
- "merge-base",
- @$parents, $merge_tip,
- );
+ my $merge_base;
+ eval {
+ $merge_base = command_oneline(
+ "merge-base",
+ @$parents, $merge_tip,
+ );
+ };
+ if ($@) {
+ die "An error occurred during merge-base"
+ unless $@->isa("Git::Error::Command");
+
+ warn "W: Cannot find common ancestor between ".
+ "@$parents and $merge_tip. Ignoring merge info.\n";
+ next;
+ }
# double check that there are no missing non-merge commits
my (@incomplete) = check_cherry_pick(
--
1.6.6.2.g18c9a
^ permalink raw reply related
* Re: [RFC/PATCH] commit: make the error message on unmerged entries user-friendly.
From: Matthieu Moy @ 2010-01-06 20:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vy6kbt2ww.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> They'd use "pull", not merge. Anyway, I did it for commit, merge,
>> pull, revert, cherry-pick. I guess we covered the common cases.
>> The patch seems to have a lot of redundancies, but I think trying to
>> factor this into helper functions would be much more effort than the
>> few cut-and-paste that I had to do, since each instance is a slight
>> variant of each other ...
>
> I'd be more worried about longer term maintainability than one time
> expediency of producing your single patch to add these messages. If the
> messages are cast in stone, we can just verify they are consistent _now_
> and forget about them, but I suspect not even you are perfect to predict
> that we won't come up with different/better ways to resolve and mark them
> resolved in the future and write a set of messages that won't have to
> change.
Sorry, I sent the message a bit too early. Re-reading the patch, there
were actually pieces to factor. There's still duplication between C
and shell, and sentences which are actually reworded from a place to
another.
OTOH, maybe this reveals some differences that should be eliminated.
For example, 'git cherry-pick' has no problem with
$GIT_DIR/MERGE_HEAD, while 'git merge' will immediately if it exists.
Maybe we should write a helper like
ensure_everything_is_ok_for_a_merge_or_die() called by both
cherry-pick and merge?
This time, patch actually follows ;-).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* [PATCH v2] Be more user-friendly when refusing to do something because of conflict.
From: Matthieu Moy @ 2010-01-06 20:17 UTC (permalink / raw)
To: git, gitster; +Cc: Matthieu Moy
In-Reply-To: <vpqhbqz6lcr.fsf@bauges.imag.fr>
Various commands refuse to run in the presence of conflicts (commit,
merge, pull, cherry-pick/revert). They all used to provide rough, and
inconsistant error messages.
A new variable advice.resolveconflict is introduced, and allows more
verbose messages, pointing the user to the appropriate solution.
For commit, the error message used to look like this:
$ git commit
foo.txt: needs merge
foo.txt: unmerged (c34a92682e0394bc0d6f4d4a67a8e2d32395c169)
foo.txt: unmerged (3afcd75de8de0bb5076942fcb17446be50451030)
foo.txt: unmerged (c9785d77b76dfe4fb038bf927ee518f6ae45ede4)
error: Error building trees
The "need merge" line is given by refresh_cache. We add the IN_PORCELAIN
option to make the output more consistant with the other porcelain
commands, and catch the error in return, to stop with a clean error
message. The next lines were displayed by a call to cache_tree_update(),
which is not reached anymore if we noticed the conflict.
Pull is slightly modified to abort immediately if $GIT_DIR/MERGE_HEAD
exists instead of waiting for merge to complain.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Documentation/config.txt | 4 ++++
advice.c | 12 ++++++++++++
advice.h | 3 +++
builtin-commit.c | 15 +++++++++++++--
builtin-merge.c | 15 ++++++++++-----
builtin-revert.c | 15 ++++++++++++++-
git-pull.sh | 25 +++++++++++++++++++++++--
7 files changed, 79 insertions(+), 10 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 23a965e..5078d26 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -130,6 +130,10 @@ advice.*::
Advice shown when linkgit:git-merge[1] refuses to
merge to avoid overwritting local changes.
Default: true.
+ resolveConflict::
+ Advices shown by various commands when conflicts
+ prevent the operation from being performed.
+ Default: true.
--
core.fileMode::
diff --git a/advice.c b/advice.c
index cb666ac..ec2bd82 100644
--- a/advice.c
+++ b/advice.c
@@ -3,6 +3,7 @@
int advice_push_nonfastforward = 1;
int advice_status_hints = 1;
int advice_commit_before_merge = 1;
+int advice_resolve_conflict = 1;
static struct {
const char *name;
@@ -11,6 +12,7 @@ static struct {
{ "pushnonfastforward", &advice_push_nonfastforward },
{ "statushints", &advice_status_hints },
{ "commitbeforemerge", &advice_commit_before_merge },
+ { "resolveconflict", &advice_resolve_conflict },
};
int git_default_advice_config(const char *var, const char *value)
@@ -27,3 +29,13 @@ int git_default_advice_config(const char *var, const char *value)
return 0;
}
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+ if (advice_resolve_conflict)
+ die("'%s' is not possible because you have unmerged files.\n"
+ "Please, fix them up in the work tree, and then use 'git add/rm <file>'\n"
+ "as appropriate to mark resolution, or use 'git commit -a'.", me);
+ else
+ die("'%s' is not possible because you have unmerged files.", me);
+}
diff --git a/advice.h b/advice.h
index 3de5000..a6b4422 100644
--- a/advice.h
+++ b/advice.h
@@ -4,7 +4,10 @@
extern int advice_push_nonfastforward;
extern int advice_status_hints;
extern int advice_commit_before_merge;
+extern int advice_resolve_conflict;
int git_default_advice_config(const char *var, const char *value);
+extern void NORETURN die_resolve_conflict(const char *me);
+
#endif /* ADVICE_H */
diff --git a/builtin-commit.c b/builtin-commit.c
index 3dfcd77..a4977ac 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -235,6 +235,17 @@ static void create_base_index(void)
exit(128); /* We've already reported the error, finish dying */
}
+static void refresh_cache_or_die(int refresh_flags)
+{
+ /*
+ * refresh_flags contains REFRESH_QUIET, so the only errors
+ * are for unmerged entries.
+ */
+ if(refresh_cache(refresh_flags | REFRESH_IN_PORCELAIN)) {
+ die_resolve_conflict("commit");
+ }
+}
+
static char *prepare_index(int argc, const char **argv, const char *prefix, int is_status)
{
int fd;
@@ -274,7 +285,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
if (all || (also && pathspec && *pathspec)) {
int fd = hold_locked_index(&index_lock, 1);
add_files_to_cache(also ? prefix : NULL, pathspec, 0);
- refresh_cache(refresh_flags);
+ refresh_cache_or_die(refresh_flags);
if (write_cache(fd, active_cache, active_nr) ||
close_lock_file(&index_lock))
die("unable to write new_index file");
@@ -293,7 +304,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
*/
if (!pathspec || !*pathspec) {
fd = hold_locked_index(&index_lock, 1);
- refresh_cache(refresh_flags);
+ refresh_cache_or_die(refresh_flags);
if (write_cache(fd, active_cache, active_nr) ||
commit_locked_index(&index_lock))
die("unable to write new_index file");
diff --git a/builtin-merge.c b/builtin-merge.c
index f1c84d7..abe6c03 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -847,11 +847,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
const char *best_strategy = NULL, *wt_strategy = NULL;
struct commit_list **remotes = &remoteheads;
- if (file_exists(git_path("MERGE_HEAD")))
- die("You have not concluded your merge. (MERGE_HEAD exists)");
- if (read_cache_unmerged())
- die("You are in the middle of a conflicted merge."
- " (index unmerged)");
+ if (read_cache_unmerged()) {
+ die_resolve_conflict("merge");
+ }
+ if (file_exists(git_path("MERGE_HEAD"))) {
+ if (advice_resolve_conflict)
+ die("You have not concluded your merge (MERGE_HEAD exists).\n"
+ "Please, commit your changes before you can merge.");
+ else
+ die("You have not concluded your merge (MERGE_HEAD exists).");
+ }
/*
* Check if we are _not_ on a detached HEAD, i.e. if there is a
diff --git a/builtin-revert.c b/builtin-revert.c
index 151aa6a..d14dde3 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -233,6 +233,19 @@ static struct tree *empty_tree(void)
return tree;
}
+static NORETURN void die_dirty_index(const char *me)
+{
+ if (read_cache_unmerged()) {
+ die_resolve_conflict(me);
+ } else {
+ if (advice_commit_before_merge)
+ die("Your local changes would be overwritten by %s.\n"
+ "Please, commit your changes or stash them to proceed.", me);
+ else
+ die("Your local changes would be overwritten by %s.\n", me);
+ }
+}
+
static int revert_or_cherry_pick(int argc, const char **argv)
{
unsigned char head[20];
@@ -269,7 +282,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
if (get_sha1("HEAD", head))
die ("You do not have a valid HEAD");
if (index_differs_from("HEAD", 0))
- die ("Dirty index: cannot %s", me);
+ die_dirty_index(me);
}
discard_cache();
diff --git a/git-pull.sh b/git-pull.sh
index 9e69ada..54ce0af 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -13,8 +13,29 @@ set_reflog_action "pull $*"
require_work_tree
cd_to_toplevel
-test -z "$(git ls-files -u)" ||
- die "You are in the middle of a conflicted merge."
+
+die_conflict () {
+ git diff-index --cached --name-status -r --ignore-submodules HEAD --
+ if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+ die "Pull is not possible because you have unmerged files.
+Please, fix them up in the work tree, and then use 'git add/rm <file>'
+as appropriate to mark resolution, or use 'git commit -a'."
+ else
+ die "Pull is not possible because you have unmerged files."
+ fi
+}
+
+die_merge () {
+ if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+ die "You have not concluded your merge (MERGE_HEAD exists).
+Please, commit your changes before you can merge."
+ else
+ die "You have not concluded your merge (MERGE_HEAD exists)."
+ fi
+}
+
+test -z "$(git ls-files -u)" || die_conflict
+test -f "$GIT_DIR/MERGE_HEAD" && die_merge
strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
log_arg= verbosity=
--
1.6.6.81.g35ec3e.dirty
^ permalink raw reply related
* [PATCH v3] Be more user-friendly when refusing to do something because of conflict.
From: Matthieu Moy @ 2010-01-06 20:36 UTC (permalink / raw)
To: git, gitster; +Cc: Matthieu Moy
In-Reply-To: <1262809074-27137-1-git-send-email-Matthieu.Moy@imag.fr>
Various commands refuse to run in the presence of conflicts (commit,
merge, pull, cherry-pick/revert). They all used to provide rough, and
inconsistant error messages.
A new variable advice.resolveconflict is introduced, and allows more
verbose messages, pointing the user to the appropriate solution.
For commit, the error message used to look like this:
$ git commit
foo.txt: needs merge
foo.txt: unmerged (c34a92682e0394bc0d6f4d4a67a8e2d32395c169)
foo.txt: unmerged (3afcd75de8de0bb5076942fcb17446be50451030)
foo.txt: unmerged (c9785d77b76dfe4fb038bf927ee518f6ae45ede4)
error: Error building trees
The "need merge" line is given by refresh_cache. We add the IN_PORCELAIN
option to make the output more consistant with the other porcelain
commands, and catch the error in return, to stop with a clean error
message. The next lines were displayed by a call to cache_tree_update(),
which is not reached anymore if we noticed the conflict.
Pull is slightly modified to abort immediately if $GIT_DIR/MERGE_HEAD
exists instead of waiting for merge to complain.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Sorry, v2 missed a #include "git-compat-utils.h", it's not even compilable.
Documentation/config.txt | 4 ++++
advice.c | 12 ++++++++++++
advice.h | 5 +++++
builtin-commit.c | 15 +++++++++++++--
builtin-merge.c | 15 ++++++++++-----
builtin-revert.c | 15 ++++++++++++++-
git-pull.sh | 25 +++++++++++++++++++++++--
7 files changed, 81 insertions(+), 10 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 23a965e..5078d26 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -130,6 +130,10 @@ advice.*::
Advice shown when linkgit:git-merge[1] refuses to
merge to avoid overwritting local changes.
Default: true.
+ resolveConflict::
+ Advices shown by various commands when conflicts
+ prevent the operation from being performed.
+ Default: true.
--
core.fileMode::
diff --git a/advice.c b/advice.c
index cb666ac..ec2bd82 100644
--- a/advice.c
+++ b/advice.c
@@ -3,6 +3,7 @@
int advice_push_nonfastforward = 1;
int advice_status_hints = 1;
int advice_commit_before_merge = 1;
+int advice_resolve_conflict = 1;
static struct {
const char *name;
@@ -11,6 +12,7 @@ static struct {
{ "pushnonfastforward", &advice_push_nonfastforward },
{ "statushints", &advice_status_hints },
{ "commitbeforemerge", &advice_commit_before_merge },
+ { "resolveconflict", &advice_resolve_conflict },
};
int git_default_advice_config(const char *var, const char *value)
@@ -27,3 +29,13 @@ int git_default_advice_config(const char *var, const char *value)
return 0;
}
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+ if (advice_resolve_conflict)
+ die("'%s' is not possible because you have unmerged files.\n"
+ "Please, fix them up in the work tree, and then use 'git add/rm <file>'\n"
+ "as appropriate to mark resolution, or use 'git commit -a'.", me);
+ else
+ die("'%s' is not possible because you have unmerged files.", me);
+}
diff --git a/advice.h b/advice.h
index 3de5000..acd5fdd 100644
--- a/advice.h
+++ b/advice.h
@@ -1,10 +1,15 @@
#ifndef ADVICE_H
#define ADVICE_H
+#include "git-compat-util.h"
+
extern int advice_push_nonfastforward;
extern int advice_status_hints;
extern int advice_commit_before_merge;
+extern int advice_resolve_conflict;
int git_default_advice_config(const char *var, const char *value);
+extern void NORETURN die_resolve_conflict(const char *me);
+
#endif /* ADVICE_H */
diff --git a/builtin-commit.c b/builtin-commit.c
index 3dfcd77..a4977ac 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -235,6 +235,17 @@ static void create_base_index(void)
exit(128); /* We've already reported the error, finish dying */
}
+static void refresh_cache_or_die(int refresh_flags)
+{
+ /*
+ * refresh_flags contains REFRESH_QUIET, so the only errors
+ * are for unmerged entries.
+ */
+ if(refresh_cache(refresh_flags | REFRESH_IN_PORCELAIN)) {
+ die_resolve_conflict("commit");
+ }
+}
+
static char *prepare_index(int argc, const char **argv, const char *prefix, int is_status)
{
int fd;
@@ -274,7 +285,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
if (all || (also && pathspec && *pathspec)) {
int fd = hold_locked_index(&index_lock, 1);
add_files_to_cache(also ? prefix : NULL, pathspec, 0);
- refresh_cache(refresh_flags);
+ refresh_cache_or_die(refresh_flags);
if (write_cache(fd, active_cache, active_nr) ||
close_lock_file(&index_lock))
die("unable to write new_index file");
@@ -293,7 +304,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix, int
*/
if (!pathspec || !*pathspec) {
fd = hold_locked_index(&index_lock, 1);
- refresh_cache(refresh_flags);
+ refresh_cache_or_die(refresh_flags);
if (write_cache(fd, active_cache, active_nr) ||
commit_locked_index(&index_lock))
die("unable to write new_index file");
diff --git a/builtin-merge.c b/builtin-merge.c
index f1c84d7..abe6c03 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -847,11 +847,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
const char *best_strategy = NULL, *wt_strategy = NULL;
struct commit_list **remotes = &remoteheads;
- if (file_exists(git_path("MERGE_HEAD")))
- die("You have not concluded your merge. (MERGE_HEAD exists)");
- if (read_cache_unmerged())
- die("You are in the middle of a conflicted merge."
- " (index unmerged)");
+ if (read_cache_unmerged()) {
+ die_resolve_conflict("merge");
+ }
+ if (file_exists(git_path("MERGE_HEAD"))) {
+ if (advice_resolve_conflict)
+ die("You have not concluded your merge (MERGE_HEAD exists).\n"
+ "Please, commit your changes before you can merge.");
+ else
+ die("You have not concluded your merge (MERGE_HEAD exists).");
+ }
/*
* Check if we are _not_ on a detached HEAD, i.e. if there is a
diff --git a/builtin-revert.c b/builtin-revert.c
index 151aa6a..d14dde3 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -233,6 +233,19 @@ static struct tree *empty_tree(void)
return tree;
}
+static NORETURN void die_dirty_index(const char *me)
+{
+ if (read_cache_unmerged()) {
+ die_resolve_conflict(me);
+ } else {
+ if (advice_commit_before_merge)
+ die("Your local changes would be overwritten by %s.\n"
+ "Please, commit your changes or stash them to proceed.", me);
+ else
+ die("Your local changes would be overwritten by %s.\n", me);
+ }
+}
+
static int revert_or_cherry_pick(int argc, const char **argv)
{
unsigned char head[20];
@@ -269,7 +282,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
if (get_sha1("HEAD", head))
die ("You do not have a valid HEAD");
if (index_differs_from("HEAD", 0))
- die ("Dirty index: cannot %s", me);
+ die_dirty_index(me);
}
discard_cache();
diff --git a/git-pull.sh b/git-pull.sh
index 9e69ada..54ce0af 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -13,8 +13,29 @@ set_reflog_action "pull $*"
require_work_tree
cd_to_toplevel
-test -z "$(git ls-files -u)" ||
- die "You are in the middle of a conflicted merge."
+
+die_conflict () {
+ git diff-index --cached --name-status -r --ignore-submodules HEAD --
+ if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+ die "Pull is not possible because you have unmerged files.
+Please, fix them up in the work tree, and then use 'git add/rm <file>'
+as appropriate to mark resolution, or use 'git commit -a'."
+ else
+ die "Pull is not possible because you have unmerged files."
+ fi
+}
+
+die_merge () {
+ if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
+ die "You have not concluded your merge (MERGE_HEAD exists).
+Please, commit your changes before you can merge."
+ else
+ die "You have not concluded your merge (MERGE_HEAD exists)."
+ fi
+}
+
+test -z "$(git ls-files -u)" || die_conflict
+test -f "$GIT_DIR/MERGE_HEAD" && die_merge
strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
log_arg= verbosity=
--
1.6.6.81.g2955.dirty
^ permalink raw reply related
* Re: [PATCH v2] Be more user-friendly when refusing to do something because of conflict.
From: Junio C Hamano @ 2010-01-06 21:04 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <1262809074-27137-1-git-send-email-Matthieu.Moy@imag.fr>
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> Various commands refuse to run in the presence of conflicts (commit,
> merge, pull, cherry-pick/revert). They all used to provide rough, and
> inconsistant error messages.
>
> A new variable advice.resolveconflict is introduced, and allows more
> verbose messages, pointing the user to the appropriate solution.
>
> For commit, the error message used to look like this:
>
> $ git commit
> foo.txt: needs merge
> foo.txt: unmerged (c34a92682e0394bc0d6f4d4a67a8e2d32395c169)
> foo.txt: unmerged (3afcd75de8de0bb5076942fcb17446be50451030)
> foo.txt: unmerged (c9785d77b76dfe4fb038bf927ee518f6ae45ede4)
> error: Error building trees
>
> The "need merge" line is given by refresh_cache. We add the IN_PORCELAIN
> option to make the output more consistant with the other porcelain
> commands, and catch the error in return, to stop with a clean error
> message. The next lines were displayed by a call to cache_tree_update(),
> which is not reached anymore if we noticed the conflict.
"The new output looks like this, which is much better..." is missing
here.
> @@ -27,3 +29,13 @@ int git_default_advice_config(const char *var, const char *value)
>
> return 0;
> }
> +
> +void NORETURN die_resolve_conflict(const char *me)
> +{
> + if (advice_resolve_conflict)
> + die("'%s' is not possible because you have unmerged files.\n"
> + "Please, fix them up in the work tree, and then use 'git add/rm <file>'\n"
> + "as appropriate to mark resolution, or use 'git commit -a'.", me);
> + else
> + die("'%s' is not possible because you have unmerged files.", me);
> +}
Nice, but the advice contrasts between squirrels and oranges.
One advice, "use add/rm as appropriate to mark resolution" goes only as
far as making the index in order, without recording it in a commit. The
other, "commit -a", will make a commit, I suspect that "commit -a" needs
to be matched with "commit" if the user chooses to take the first advice
of resolving paths incrementally. IOW
use 'git add/rm <file>' as appropriate to mark resolution and make a
commit, or use 'commit -a', before running me again.
might be more appropriate.
> diff --git a/builtin-commit.c b/builtin-commit.c
> index 3dfcd77..a4977ac 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -235,6 +235,17 @@ static void create_base_index(void)
> exit(128); /* We've already reported the error, finish dying */
> }
>
> +static void refresh_cache_or_die(int refresh_flags)
> +{
> + /*
> + * refresh_flags contains REFRESH_QUIET, so the only errors
> + * are for unmerged entries.
> + */
Mixed indentation.
> + if(refresh_cache(refresh_flags | REFRESH_IN_PORCELAIN)) {
SP after "if".
What should we see upon "commit --dry-run" and what does the code after
the patch produce? Are we sure refresh_flags always lack REFRESH_UNMERGED
that allows unmerged entries and produces the unmerged error messages when
needed?
> diff --git a/builtin-merge.c b/builtin-merge.c
> index f1c84d7..abe6c03 100644
> --- a/builtin-merge.c
> +++ b/builtin-merge.c
> @@ -847,11 +847,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> const char *best_strategy = NULL, *wt_strategy = NULL;
> struct commit_list **remotes = &remoteheads;
>
> - if (file_exists(git_path("MERGE_HEAD")))
> - die("You have not concluded your merge. (MERGE_HEAD exists)");
> - if (read_cache_unmerged())
> - die("You are in the middle of a conflicted merge."
> - " (index unmerged)");
> + if (read_cache_unmerged()) {
> + die_resolve_conflict("merge");
> + }
> + if (file_exists(git_path("MERGE_HEAD"))) {
> + if (advice_resolve_conflict)
> + die("You have not concluded your merge (MERGE_HEAD exists).\n"
> + "Please, commit your changes before you can merge.");
> + else
> + die("You have not concluded your merge (MERGE_HEAD exists).");
> + }
It is not a very big deal, but why are these checked in different order
after the patch? Unless the new order is justifiably better, I'd rather
see the order kept as before. Otherwise please justify it in the proposed
commit log message.
Note that the user might have already run "add -u" to mark everything
resolved, in which case MERGE_HEAD will still exist even though the index
is free of ummerged entries.
> diff --git a/builtin-revert.c b/builtin-revert.c
Nice.
> diff --git a/git-pull.sh b/git-pull.sh
> index 9e69ada..54ce0af 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -13,8 +13,29 @@ set_reflog_action "pull $*"
> require_work_tree
> cd_to_toplevel
>
> -test -z "$(git ls-files -u)" ||
> - die "You are in the middle of a conflicted merge."
> +
> +die_conflict () {
> + git diff-index --cached --name-status -r --ignore-submodules HEAD --
> + if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
> + die "Pull is not possible because you have unmerged files.
> +Please, fix them up in the work tree, and then use 'git add/rm <file>'
> +as appropriate to mark resolution, or use 'git commit -a'."
> + else
> + die "Pull is not possible because you have unmerged files."
> + fi
> +}
> +
> +die_merge () {
> + if [ $(git config --bool --get advice.resolveConflict || echo true) = "true" ]; then
> + die "You have not concluded your merge (MERGE_HEAD exists).
> +Please, commit your changes before you can merge."
> + else
> + die "You have not concluded your merge (MERGE_HEAD exists)."
> + fi
> +}
> +
> +test -z "$(git ls-files -u)" || die_conflict
> +test -f "$GIT_DIR/MERGE_HEAD" && die_merge
Nice. Maybe we want to handle other cases like:
$ git rebase master
... conflicted
... called away for a 30-minutes meeting
... forgot the user was in the middle of the rebase
$ git pull
and the "pull" refused to run because the earlier "rebase" hasn't been
concluded (I suspect an earlier "am" failure would be the same issue).
Thanks.
^ permalink raw reply
* Re: submodules' shortcomings, was Re: RFC: display dirty submodule working directory in git gui and gitk
From: Jens Lehmann @ 2010-01-06 21:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyen Thai Ngoc Duy, Johannes Schindelin, Git Mailing List,
Shawn O. Pearce, Paul Mackerras, Heiko Voigt, Lars Hjemli,
Avery Pennarun
In-Reply-To: <7vbph7uhn9.fsf@alter.siamese.dyndns.org>
Am 06.01.2010 21:01, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
>> Am 06.01.2010 18:55, schrieb Junio C Hamano:
>>> I was envisioning that the "git status" in submodule will be run with its
>>> recent --porcelain option, and "git status" or "git commit" would read it
>>> to postprocess and incorporate into its own output.
>>
>> And i thought about printing just one line for each dirty submodule that
>> contains uncommitted and/or new files. I did not intend to list every
>> file, for the same reason a "git diff --submodule" only shows the first
>> line of the commit messages, not the actual differences of all changed
>> files in the submodule. I am not against being able to show all files
>> too, but i really would want to have an option to get a short output for
>> git gui and gitk.
>
> I don't think what you are saying is inconsistent with "git status/commit
> that reads from 'git status --porcelain' it runs in a submodule directory,
> postprocesses it and incorporates it into its own output." When the
> sub-status reports changes, your "postprocess" would condense it down to
> "this has a potential change that user could want to commit". How the
> dirtiness is shown is entirely up to the caller that detected that change.
>
> Let's explain it in another way.
>
> The original "diff" for a submodule entry was implemented by preparing a
>
> "Subproject commit %s\n"
>
> line for the submodule commit recorded in the preimage and postimage, and
> compare these as if they are one-line files. When the postimage was work
> tree, it looked at submodule's .git/HEAD to learn what to stuff in %s
> there.
>
> But nobody forced you to limit the check only to .git/HEAD in the
> submodule. To make the comparison richer, you could check if the
> submodule directory is dirty (and we have already discussed the potential
> definition of dirtiness earlier), and add "-dirty" in the string as well.
> With such a change, if you make some changes to a file in the work tree of
> the submodule after a clean "clone", "git diff" between the index and the
> work tree would report:
>
> -Subproject commit 37bae10e38a66e4f1ddd5350daded00b21735126
> +Subproject commit 37bae10e38a66e4f1ddd5350daded00b21735126-dirty
>
> The suggestion to read from "status --porcelain" that is run in the
> submodule directory was about how to implement the part that determines
> this "dirtiness" information, and not about how that dirtiness is
> expressed in the output. The above is an illustration that even the
> traditional output format can be made aware of this submodule dirtiness
> check. "diff --submodule" can express that dirtiness information in any
> way it wants.
I see, we seem to agree again :-)
While looking into "git status" in the last hours i became aware that
there is some infrastructure for calling "git submodule summary" (when
that is enabled via "git config status.submodulesummary"). I think this
can be extended to transfer the dirty information from "git diff
--submodule" (which can and should replace "git submodule summary" IMO)
into "git status".
Will send a patch for discussion tomorrow, i have to get some sleep now.
^ permalink raw reply
* Re: [PATCH v3 5/6] transport-helper.c::push_refs(): ignore helper-reported status if ref is not to be pushed
From: Tay Ray Chuan @ 2010-01-06 21:41 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce, Daniel Barkalow, Junio C Hamano
In-Reply-To: <20100106120456.GA7221@coredump.intra.peff.net>
Hi,
On Wed, Jan 6, 2010 at 8:04 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jan 05, 2010 at 06:01:13PM +0800, Tay Ray Chuan wrote:
>
> > > It seems like this should be checking for REF_STATUS_NONE explicitly
> > > instead of trying to enumerate the reasons we might not have tried to
> > > push. Shouldn't helpers _only_ be pushing REF_STATUS_NONE refs?
> > >
> > > I think right now the two cases are equivalent, since non-ff and
> > > uptodate are the only two states set before the helper is invoked. But
> > > we have discussed in the past (and I still have a patch floating around
> > > for) a REF_STATUS_REWIND which would treat strict rewinds differently
> > > (silently ignoring them instead of making an error). Explicitly checking
> > > REF_STATUS_NONE future-proofs against new states being added.
> >
> > I'm not really sure if this is true (ie. that if status is not non-ff
> > or uptodate, then it is REF_STATUS_NONE), but we could step around this
>
> Well, consider it this way. If it's _not_ REF_STATUS_NONE, then what is
> it, and what does it mean to be overwriting it?
Ok, I'll take your suggestion from your previous email and do this:
@@ -429,8 +429,16 @@ static int push_refs(struct transport *transport,
continue;
}
- ref->status = status;
- ref->remote_status = msg;
+ if (ref->status == REF_STATUS_NONE) {
+ ref->status = status;
+ ref->remote_status = msg;
+ } else {
+ /*
+ * Earlier, the ref was marked not to be pushed, so ignore what
+ * the remote helper said about the ref.
+ */
+ continue;
+ }
}
strbuf_release(&buf);
return 0;
Going by this principle (only refs with status of none will be pushed),
I think I should also squash the below into patch 3 (refactor ref status
logic for pushing):
@@ -336,11 +336,10 @@ static int push_refs(struct transport *transport,
continue;
switch (ref->status) {
- case REF_STATUS_REJECT_NONFASTFORWARD:
- case REF_STATUS_UPTODATE:
- continue;
+ case REF_STATUS_NONE:
+ ; /* carry on with pushing */
default:
- ; /* do nothing */
+ continue;
}
if (force_all)
> Maybe I am misunderstanding the problem the patch is addressing, but the
> point of these REF_STATUS feels was to act as a small state machine.
> Everything starts as NONE, and then:
>
> - we compare locally against remote refs. We may transition:
> NONE -> UPTODATE
> NONE -> REJECT_NONFASTFORWARD
> NONE -> REJECT_NODELETE
>
> - we send the push list
> NONE -> EXPECTING_REPORT (if the remote supports individual status)
> NONE -> OK (otherwise)
>
> - we get back status responses
> EXPECTING_REPORT -> OK
> EXPECTING_REPORT -> REMOTE_REJECT
>
> I haven't looked closely at the new transport helper code, but I would
> think it should stick more or less to those transitions. The exception
> would be that some transports don't necessarily handle EXPECTING_REPORT
> in the same way, and may transition directly from NONE to
> OK/REMOTE_REJECT.
minor nit: yes, this may differ from transport-to-transport, but
EXPECTING_REPORT is not used at all in the top-level transport (the
level above the helper).
There's also something I'd like to point out for accuracy: it's that
this sequence of transitions occur at two levels, separately: one at
the top-level transport/transport-helper, and another at the helper.
So, for certain non-ff refs (the type this patch series is looking at),
the sequence of state transitions stops and doesn't continue to step 2
in the top-level transport (sending the push list); but separately, in
the helper, the ref goes through another sequence of state transitions.
What this patch touches is the part in the top-level transport that
syncs the ref status between the helper and the top-level transport: do
we take and present to the user what the helper has done, or not?
Regarding this point, I now think that we should ignore the
helper-reported status only if that status is none, and continue
updating the ref status in the top-level transport if the helper did
push successfully/failed, even if we didn't tell it to push:
@@ -429,7 +429,7 @@ static int push_refs(struct transport *transport,
ref->status = status;
ref->remote_status = msg;
- if (ref->status == REF_STATUS_NONE) {
+ if (ref->status == REF_STATUS_NONE && status == REF_STATUS_NONE) {
ref->status = status;
ref->remote_status = msg;
} else {
> So offhand, I would say that your list should also probably include
> REJECT_NODELETE. However, I think that status is just for old servers
> which didn't support the delete-refs protocol extension. So presumably
> that is none of the new helpers, as they all post-date the addition of
> that feature by quite a few years.
You're right, AFAIK, for the smart http protocol; I don't think it
supports NODELETE.
> > by introducing a property, say, ref->should_push, that is set to 1,
> > after all the vetting has been carried out and just before we talk to
> > the server.
>
> I'd rather not introduce new state. The point of the status flag was to
> encapsulate all of that information, and a new state variable just seems
> like introducing extra complexity. If we are not in the NONE state, I
> don't see why we would tell the helper about a ref at all.
Noted.
--
Cheers,
Ray Chuan
^ permalink raw reply
* Re: [PATCH 1/2] git-svn: ignore changeless commits when checking for a cherry-pick
From: Eric Wong @ 2010-01-06 21:43 UTC (permalink / raw)
To: Andrew Myrick; +Cc: git, sam
In-Reply-To: <1262808529-1575-1-git-send-email-amyrick@apple.com>
Andrew Myrick <amyrick@apple.com> wrote:
> diff --git a/git-svn.perl b/git-svn.perl
> index 650c9e5..8c7c034 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -3052,12 +3052,36 @@ sub check_cherry_pick {
> for my $range ( @ranges ) {
> delete @commits{_rev_list($range)};
> }
> + for my $commit (keys %commits) {
Hi Andrew,
I'll again defer to Sam for Acks on these. Test cases would be nice to
have, too.
A few notes:
The prevailing standard for indentation in git is with hard tabs and
that's certainly the case with the rest of git-svn.perl.
> +sub has_no_changes {
> + my $commit = shift;
> +
> + my @revs = split / /, command_oneline(
> + qw(rev-list --parents -1 -m), $commit);
> +
> + # Commits with no parents, e.g. the start of a partial branch,
> + # have changes by definition.
> + return 1 if (@revs < 2);
> +
I've become very picky about trailing whitespace (from Junio :), too.
"git diff --check" is helpful for this, especially in the pre-commit
hook.
Thanks!
--
Eric Wong
^ permalink raw reply
* Re: [PATCH 9/9] rerere forget path: forget recorded resolution
From: Junio C Hamano @ 2010-01-06 21:59 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <7vr5q31871.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> However, I think you need this fix for "checkout -m" to work as intended,
Here is a cleaner reroll with tests, meant to be applied on v1.6.1-rc1 or
later.
-- >8 --
Subject: [PATCH] checkout -m path: fix recreating conflicts
We should tell ll_merge() that the 3-way merge between stages #2 and #3 is
an outermost merge, not a virtual-ancestor creation.
Back when this code was originally written, users couldn't write custom
merge drivers easily, so the bug didn't matter, but these days it does.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin-checkout.c | 2 +-
t/t7201-co.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+), 1 deletions(-)
diff --git a/builtin-checkout.c b/builtin-checkout.c
index 7f3bd7b..e41e73b 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -179,7 +179,7 @@ static int checkout_merged(int pos, struct checkout *state)
fill_mm(active_cache[pos+2]->sha1, &theirs);
status = ll_merge(&result_buf, path, &ancestor,
- &ours, "ours", &theirs, "theirs", 1);
+ &ours, "ours", &theirs, "theirs", 0);
free(ancestor.ptr);
free(ours.ptr);
free(theirs.ptr);
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 0e21632..3214ad2 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -534,4 +534,61 @@ test_expect_success 'failing checkout -b should not break working tree' '
'
+(
+ echo "#!$SHELL_PATH"
+ cat <<\EOF
+O=$1 A=$2 B=$3
+cat "$A" >.tmp
+exec >"$A"
+echo '<<<<<<< filfre-theirs'
+cat "$B"
+echo '||||||| filfre-common'
+cat "$O"
+echo '======='
+cat ".tmp"
+echo '>>>>>>> filfre-ours'
+rm -f .tmp
+exit 1
+EOF
+) >filfre.sh
+chmod +x filfre.sh
+
+test_expect_success 'custom merge driver with checkout -m' '
+ git reset --hard &&
+
+ git config merge.filfre.driver "./filfre.sh %O %A %B" &&
+ git config merge.filfre.name "Feel-free merge driver" &&
+ git config merge.filfre.recursive binary &&
+ echo "arm merge=filfre" >.gitattributes &&
+
+ git checkout -b left &&
+ echo neutral >arm &&
+ git add arm .gitattributes &&
+ test_tick &&
+ git commit -m neutral &&
+ git branch right &&
+
+ echo left >arm &&
+ test_tick &&
+ git commit -a -m left &&
+ git checkout right &&
+
+ echo right >arm &&
+ test_tick &&
+ git commit -a -m right &&
+
+ test_must_fail git merge left &&
+ (
+ for t in filfre-common left right
+ do
+ grep $t arm || exit 1
+ done
+ exit 0
+ ) &&
+
+ mv arm expect &&
+ git checkout -m arm &&
+ test_cmp expect arm
+'
+
test_done
--
1.6.6.184.ge163d
^ permalink raw reply related
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