* Re: help reverting a merge
From: Justin P. Mattock @ 2009-12-01 0:06 UTC (permalink / raw)
To: Jeff King; +Cc: Christian Couder, git
In-Reply-To: <20091130081315.GA587@coredump.intra.peff.net>
First things first is I owe you a great thanks
for teaching me how to do a rebase.. :-)
took a while, and still a bit confusing
but I managed to do exactly what you
had written down.
bad thing is somehow the bisect came up
with no results. probably will do another bisect,
just to make sure things are in the right
direction, and the try the rebase again.
Again Thanks for the help/info on this.
Justin P. Mattock
^ permalink raw reply
* Re: [StGit RFC PATCH 0/6] add support for git send-email
From: Alex Chiang @ 2009-12-01 0:02 UTC (permalink / raw)
To: Karl Wiberg; +Cc: catalin.marinas, git
In-Reply-To: <b8197bcb0911291405i6f052216q8717c34063320592@mail.gmail.com>
* Karl Wiberg <kha@treskal.com>:
> On Sat, Nov 28, 2009 at 8:50 PM, Alex Chiang <achiang@hp.com> wrote:
>
> > stg mail still has some nice features over git send-email, such
> > as the -v command line parameter and --prefix. Maybe at some point
> > in the future, we can migrate those features into git send-email and
> > continue thinning out stg mail.
>
> Yes. But note that we tend to be conservative and not require a
> too-new git, so a patch adding such a dependency would have to wait a
> while. (I'm currently carrying two such patches in my experimental
> branch.)
Understood. For now, of course, all the changes that I'm
proposing should work with bog-standard, oldish git, since I
don't think the git send-email interface has changed in a while.
> > But I wanted to get some feedback first to make sure I'm going in the
> > right direction before going too much further.
>
> I've read the patches, and it looks about right from where I stand.
Thank you very much for the review.
> Did you remember to run the regression tests? It's very helpful when
> reviewing to know that the regression suite passes at every point in
> the series.
Good idea. I've been running t/t1900-mail.sh at each stage since
my changes seem rather localized to sending mail.
Should I be running the entire suite?
/ac
^ permalink raw reply
* Re: [PATCH 5/6] stg mail: add basic support for git send-email
From: Alex Chiang @ 2009-12-01 0:00 UTC (permalink / raw)
To: Karl Wiberg; +Cc: catalin.marinas, git
In-Reply-To: <b8197bcb0911291354m674d3698m929a1d542a59ed9f@mail.gmail.com>
* Karl Wiberg <kha@treskal.com>:
> On Sat, Nov 28, 2009 at 8:50 PM, Alex Chiang <achiang@hp.com> wrote:
>
> > + # XXX: yuck, there's gotta be a more pythonic way. Ideally we'd like
> > + # to use the git_opts dictionary as our mapping between stg mail and
> > + # git send-email; extract k, v pairs from git_opts, and use those
> > + # to iterate across options somehow.
> > + git_opts = { 'to': '--to=', 'cc': '--cc=', 'bcc': '--bcc=' }
> > + if options.to:
> > + for a in options.to:
> > + cmd.append("--to=%s" % a)
> > + if options.cc:
> > + for a in options.cc:
> > + cmd.append("--cc=%s" % a)
> > + if options.bcc:
> > + for a in options.bcc:
> > + cmd.append("--bcc=%s" % a)
> > + if not options.auto:
> > + cmd.append("--suppress-cc=body")
>
> Like this?
>
> for x in ['to', 'cc', 'bcc']:
> if getattr(options, x):
> cmd.extend('--%s=%s' % (x, a) for a in getattr(options, x))
Yeah, that looks nice. Re-implemented with your suggestion.
> > + (fd, path) = mkstemp()
> > + os.write(fd, msg.as_string(options.mbox))
> > + os.close(fd)
> > +
> > + try:
> > + cmd.append(path)
> > + call(cmd)
> > + except Exception, err:
> > + os.unlink(path)
> > + raise CmdException, str(err)
> > +
> > + os.unlink(path)
>
> To avoid having to remember to call unlink in all paths, you can write
>
> try:
> try:
> cmd.append(path)
> call(cmd)
> except Exception, e:
> raise CmdException(str(e))
> finally:
> os.unlink(path)
>
> (The combined try...except...finally statement didn't appear until
> python 2.5, but we'd like to stay compatible with 2.4.)
This statement confuses me a bit. The way I read it, I shouldn't
use your suggestion due to compat reasons?
Thanks,
/ac
^ permalink raw reply
* Re: [PATCH 3/6] stg mail: make __send_message do more
From: Alex Chiang @ 2009-11-30 23:59 UTC (permalink / raw)
To: Karl Wiberg; +Cc: catalin.marinas, git
In-Reply-To: <b8197bcb0911291323l35cb3624td3cbc393bf4513b3@mail.gmail.com>
* Karl Wiberg <kha@treskal.com>:
> On Sat, Nov 28, 2009 at 8:50 PM, Alex Chiang <achiang@hp.com> wrote:
>
> > Factor out the common code required to send either a cover mail
> > or patch, and implement it in __send_message.
>
> Nice code size reduction.
Thanks.
> > + msg_id = email.Utils.make_msgid('stgit')
> > + build = { 1: __build_cover, 4: __build_message }
> > + msg = build[len(args)](tmpl, msg_id, options, *args)
> > +
> > + from_addr, to_addrs = __parse_addresses(msg)
> > + msg_str = msg.as_string(options.mbox)
> > + if options.mbox:
> > + out.stdout_raw(msg_str + '\n')
> > + return msg_id
> > +
> > + outstr = { 1: 'the cover message', 4: 'patch "%s"' % args[0] }
> > + out.start('Sending ' + outstr[len(args)])
>
> You could consolidate the two dictionaries like this, to avoid making
> the same choice twice and make the code more pleasant to read:
>
> (build, outstr) = { 1: (__build_cover, 'the cover message'), 4:
> (__build_message, 'patch "%s"' % args[0]) }
Hm, I don't think that's valid. I ended up doing something like
this:
d = { 'cover': (__build_cover, 'the cover message'),
'patch': (__build_message, 'patch "%s"' % args[0]) }
(build, outstr) = d[type]
> > + # give recipients a chance of receiving related patches in correct order
> > + # patch_nr < total_nr
> > + if len(args) == 1 or (len(args) == 4 and args[1] < args[2]):
> > + sleep = options.sleep or config.getint('stgit.smtpdelay')
> > + time.sleep(sleep)
>
> Hmm. I must say I find all the args[x] a bit hard to read. I'd prefer
> symbolic names.
Ok, I changed this up.
Thanks for the review.
/ac
^ permalink raw reply
* Re: [PATCH 1/6] stg mail: Refactor __send_message and friends
From: Alex Chiang @ 2009-11-30 23:58 UTC (permalink / raw)
To: Karl Wiberg; +Cc: catalin.marinas, git
In-Reply-To: <4B123B48.4050405@treskal.com>
* Karl Wiberg <kha@treskal.com>:
> Alex Chiang wrote:
>
>> + if (smtppassword and not smtpuser):
>> + raise CmdException, 'SMTP password supplied, username needed'
>> + if (smtpusetls and not smtpuser):
>> + raise CmdException, 'SMTP over TLS requested, username needed'
>
> Python style nit: Use "raise Exception('message')" in new code. (And
> yes, I know you just moved these lines around.)
Changed, thanks.
^ permalink raw reply
* [PATCH] get_ref_states: strdup entries and free util in stale list
From: Bert Wesarg @ 2009-11-30 23:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Jay Soffian, git, Bert Wesarg
Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
builtin-remote.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/builtin-remote.c b/builtin-remote.c
index 7916626..bb72e27 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -272,7 +272,9 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
die("Could not get fetch map for refspec %s",
states->remote->fetch_refspec[i]);
- states->new.strdup_strings = states->tracked.strdup_strings = 1;
+ states->new.strdup_strings =
+ states->tracked.strdup_strings =
+ states->stale.strdup_strings = 1;
for (ref = fetch_map; ref; ref = ref->next) {
unsigned char sha1[20];
if (!ref->peer_ref || read_ref(ref->peer_ref->name, sha1))
@@ -768,7 +770,7 @@ static void clear_push_info(void *util, const char *string)
static void free_remote_ref_states(struct ref_states *states)
{
string_list_clear(&states->new, 0);
- string_list_clear(&states->stale, 0);
+ string_list_clear(&states->stale, 1);
string_list_clear(&states->tracked, 0);
string_list_clear(&states->heads, 0);
string_list_clear_func(&states->push, clear_push_info);
--
1.6.6.rc0.253.g1ec3
^ permalink raw reply related
* Git open calls fail on NFS-mounted NTFS volume
From: Kai Lanz @ 2009-11-30 23:09 UTC (permalink / raw)
To: git
There are several places in the code where git calls open() with the
flag
O_EXCL. This causes a problem for us when the current directory is on an
NFS-mounted NTFS volume shared by an NAS fileserver. The open fails with
"EACCES: Permission denied", even though the user has full read/write
permissions on the target directory.
I used a tiny C program to confirm that the O_EXCL flag is the culprit;
removing that flag from the open() call allows the open to succeed. Of
course, the call also succeeds, with the O_EXCL flag in place, if the
current directory is on an NFS-mounted UNIX filesystem.
We're looking into ways to work around this; one way would be to hack
the git
source and remove all the O_EXCL flags (are they really needed?), but
I'm
afraid that might break git in horrible ways. Another way might be to
mount
the NTFS volume via CIFS instead of NFS. But first I wanted to ask if
anyone
here can offer a solution based on changing our git configuration or
our NFS
mount options or the NTFS volume settings. (At present we have no
gitconfig
file, so all settings are default).
Git version is 1.6.5.3, running on x86_64 RHEL-4, kernel 2.6.9-89.
The error
the user sees is:
> cd /WWW
> git init
fatal: cannot copy '/usr/local/share/git-core/templates/info/exclude' to
'/WWW/.git/info/exclude': Permission denied
Using strace, we can see the offending call:
open("/WWW/.git/info/exclude", O_WRONLY|O_CREAT|O_EXCL, 0666) = -1
EACCES
(Permission denied)
We know the user has permission to create this file because he can cd
into
/WWW/.git/info and copy exclude there from the templates directory by
hand.
--
Kai Lanz Stanford University School of Earth Sciences
^ permalink raw reply
* Re: [PATCH] tests: handle NO_PYTHON setting
From: Johan Herland @ 2009-11-30 22:31 UTC (permalink / raw)
To: Jeff King; +Cc: git, Brandon Casey, Junio C Hamano, Sverre Rabbelier
In-Reply-To: <tzCtKYIbFlbIbn30IPnqgVxpN1o3NxDP88NXFHHufBYXMjZJIG1Gyw@cipher.nrlssc.navy.mil>
On Monday 30 November 2009, Brandon Casey wrote:
> Jeff King wrote:
> > On Mon, Nov 30, 2009 at 12:07:40PM -0600, Brandon Casey wrote:
> >> ps. There's something eerily familiar about this patch.
> >
> > Hmmm. Yes, I didn't search before writing it, but you probably mean:
> >
> > http://article.gmane.org/gmane.comp.version-control.git/127172
> >
> :) yeah, that was it, nbd.
Oops. I got Brandon's patches in my local tree, but I never got around to
resend the series until Sverre picked up and refactored it. Sorry for the
screwup.
> > But that is missing the NO-PYTHON bit in GIT-BUILD-OPTIONS (did you
> > forget it there, or was it part of some other patch that also didn't
> > get applied?).
>
> It was 1/2 of that series.
Indeed.
> > Also, I am tempted to move the GIT-BUILD-OPTIONS invocation _up_. It
> > is about reading config and should probably come before we start doing
> > _anything_.
> >
> > So maybe this instead:
>
> <snip the patch>
>
> Looks fine to me.
As with Brandon's original patch, this is of course
Acked-by: Johan Herland <johan@herland.net>
> No strong opinion on whether the BUILD-OPTIONS thing should be
> at the beginning of the script, or in the place where you placed
> it.
Me neither.
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: equal-tree-merges as way to make rebases fast-forward-able
From: Nanako Shiraishi @ 2009-11-30 22:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Bernhard R. Link, git
In-Reply-To: <7v8wdnooza.fsf@alter.siamese.dyndns.org>
Quoting Junio C Hamano <gitster@pobox.com>
> To avoid that, I think (1) the marker has to be more reliable than just
> "happens to have the same tree", and (2) the traversal done by Porcelains
> (your patches 3 thru 5) by default should be unaware of eqt.
>
> I don't know what a suitable marker should look like, though. The marker
> must be easily identifiable by the lowest level rev-list machinery, so it
> needs to be a sign left somewhere in the commit object. Perhaps making it
> require to have the same tree as all its parents _and_ a well-known marker
> string in the log message (and nothing else) would be a good start.
I think you can record a merge commit that has an unusual
list of parents for this. For example, you can record the
latest version twice, as the first and the second parents,
and make the previous version the third parent. Because
such a merge can't be created with git-merge command, you
can reliably tell that it is an unusual 'marker' merge.
No matter what techinique is used to mark the special
'marker', if it happens in real life for two or more people
who worked independantly to arrive at the same conclusion,
I don't think dismissing it as 'by chance' and discarding
the contribution from the second branch is a good solution.
If git is meant to work smoothly in projects where more than
one person see and accept patches from the same origin, the
condition is not met 'by chance'; the tool is by design
supposed to handle it as a regular situation.
On the other hand, if you made the marker reliable, I think
you don't have to disable this feature by default like you
said in your (2).
As a side note, I have a bug to report. I tried this sequence
of commands to make sure git-merge doesn't record the same
parent twice (the last git-merge is made on the slave branch
and tries to have slave, master and slave as its three
parents).
% git init
% echo hello >world
% git add . ; git commit -m first
% echo again >world
% git commit -a -m master
% git checkout -b slave master^
% echo again >world
% git commit -a -m slave
% git merge master slave
But I got the "usage: ..." error message from git-merge.
--
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
^ permalink raw reply
* Re: [PATCH] reset: add --quiet option
From: Stephen Boyd @ 2009-11-30 21:45 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git
In-Reply-To: <94a0d4530911300219j51e21e2cwae17d4248400a345@mail.gmail.com>
On Mon, Nov 30, 2009 at 2:19 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> I thought somebody would complain about loosing that string. In any
> case, first step is adding --query to 'git reset', second step is
> moving all OPT_BOOLEAN('q' to OPT__QUIET; there are other commands
> doing the same.
>
If you're already touching the line why not just do it once? I agree a
follow-up patch to cover the other commands would be good.
^ permalink raw reply
* [PATCH/RFC] Add a --bouquet option to git rev-list
From: Nathan W. Panike @ 2009-11-30 20:55 UTC (permalink / raw)
To: git
Add a command line option to rev-list so the command 'git rev-list --bouquet'
shows all revisions that are ancestors of refs which share history with HEAD.
Signed-off-by: Nathan W. Panike <nathan.panike@gmail.com>
---
I have a repository with the following structure:
B
/
A'--A--C
\
D
E'--E
Thus the command 'git merge base E A' returns nothing, as there is no common
history. The E history contains stuff that is derived from the other history
(A, B, C, or D). Often I find myself doing the following:
git checkout C
gitk $(include_forks) &
<View history, make changes, merges, et cetera>
git checkout E
<go back to gitk, only see history for B, C, etc>
Now the 'include_forks' command is a bash function in my .bashrc:
include_forks ()
{
local head="$(git show -s --pretty=format:'%H' HEAD)";
echo "HEAD $(git for-each-ref --format='%(refname)' \
refs/heads refs/remotes | while read ref; do \
if test "$(git merge-base HEAD ${ref}^{commit})" != ""; \
then echo ${ref}; fi; done)"
}
The shell thus intercepts my command and I must restart gitk to see the history
of E.
With this patch, I can issue the command 'gitk --bouquet' and when I checkout
E, I can 'reload' in gitk and see the history of E automatically.
If there is an easier way to do this in git, please let me know. Otherwise,
please let me know how to improve this patch.
revision.c | 38 ++++++++++++++++++++++++++++++++++++++
1 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/revision.c b/revision.c
index a8a3c3a..ba367cc 100644
--- a/revision.c
+++ b/revision.c
@@ -699,6 +699,31 @@ static int handle_one_ref(const char *path, const unsigned char *sha1, int flag,
return 0;
}
+static int handle_one_connected_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)
+{
+ struct all_refs_cb *cb = cb_data;
+ struct object *object = get_reference(cb->all_revs, path, sha1,
+ cb->all_flags);
+ struct commit *r;
+ static int got_head = 1;
+ static struct commit *head_commit;
+ static int head_nr = 0;
+ if(got_head) {
+ got_head=head_ref(handle_one_ref,cb);
+ if(got_head)
+ return 1;
+ if(cb && cb->all_revs && cb->all_revs->pending.nr > 0) {
+ head_nr = cb->all_revs->pending.nr - 1;
+ head_commit = (struct commit*)&cb->all_revs->pending.objects->item[head_nr];
+ }
+ }
+ r = lookup_commit_reference_gently(sha1,1);
+ if(r != NULL && head_commit)
+ if(get_merge_bases_many(head_commit,1,&r,1))
+ add_pending_object(cb->all_revs, object, path);
+ return 0;
+}
+
static void handle_refs(struct rev_info *revs, unsigned flags,
int (*for_each)(each_ref_fn, void *))
{
@@ -708,6 +733,15 @@ static void handle_refs(struct rev_info *revs, unsigned flags,
for_each(handle_one_ref, &cb);
}
+static void handle_connected_refs(struct rev_info *revs, unsigned flags,
+ int (*for_each)(each_ref_fn, void *))
+{
+ struct all_refs_cb cb;
+ cb.all_revs = revs;
+ cb.all_flags = flags;
+ for_each(handle_one_connected_ref, &cb);
+}
+
static void handle_one_reflog_commit(unsigned char *sha1, void *cb_data)
{
struct all_refs_cb *cb = cb_data;
@@ -1352,6 +1386,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
handle_refs(revs, flags, for_each_remote_ref);
continue;
}
+ if (!strcmp(arg, "--bouquet")) {
+ handle_connected_refs(revs, flags, for_each_ref);
+ continue;
+ }
if (!strcmp(arg, "--reflog")) {
handle_reflog(revs, flags);
continue;
--
1.6.5.3
^ permalink raw reply related
* Re: "git merge" merges too much!
From: Dmitry Potapov @ 2009-11-30 21:17 UTC (permalink / raw)
To: The Git Mailing List; +Cc: Junio C Hamano
In-Reply-To: <m1NFBAx-000kmgC@most.weird.com>
On Mon, Nov 30, 2009 at 01:40:38PM -0500, Greg A. Woods wrote:
>
> While it may be quite convenient in small projects to quickly move a
> single working directory from one branch to another and do various
> builds and tests from the result, large projects (say where a compile
> takes the better part of a working day or more and where testing
> requires multi-day processes) demand that working directories remain
> "stable", and multiple lines of development therefore demand multiple
> working directories.
It depends on the project and what tools are used, but using ccache and
proper dependencies help a lot to reduce the cost of switching. In fact,
it may be faster to switch to another branch and have to recompile a few
files than to go into another working directory, because when you go to
another working directory, you hit cold cache and things get very slow.
And then if a project is huge and takes a lot of time to compile and
test everything, I do not think, it is a good idea to build that in your
work tree. Instead, you make a shanshot using git-archive and then run
full build and test on it. In this way, you know that you test exactly
what you have committed (you can amend any commit later until you
publish it).
Dmitry
^ permalink raw reply
* Re: [PATCH] tests: handle NO_PYTHON setting
From: Brandon Casey @ 2009-11-30 21:17 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Sverre Rabbelier, git
In-Reply-To: <20091130205453.GA20348@coredump.intra.peff.net>
Jeff King wrote:
> On Mon, Nov 30, 2009 at 12:07:40PM -0600, Brandon Casey wrote:
>> ps. There's something eerily familiar about this patch.
>
> Hmmm. Yes, I didn't search before writing it, but you probably mean:
>
> http://article.gmane.org/gmane.comp.version-control.git/127172
:) yeah, that was it, nbd.
> But that is missing the NO-PYTHON bit in GIT-BUILD-OPTIONS (did you
> forget it there, or was it part of some other patch that also didn't get
> applied?).
It was 1/2 of that series.
> Also, I am tempted to move the GIT-BUILD-OPTIONS invocation _up_. It
> is about reading config and should probably come before we start doing
> _anything_.
>
> So maybe this instead:
<snip the patch>
Looks fine to me.
No strong opinion on whether the BUILD-OPTIONS thing should be
at the beginning of the script, or in the place where you placed
it.
-brandon
^ permalink raw reply
* Re: [RFC/PATCH] Detailed diagnostic when parsing an object name fails.
From: Matthieu Moy @ 2009-11-30 20:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vtywbkc05.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> $ ../git show HEAD:test-lib.sh
>> fatal: Path 't/test-lib.sh' exists, but not 'test-lib.sh'.
>> Did you mean 'HEAD:t/test-lib.sh'?
>
> The first thought that comes to mind is that if it makes more sense to
> just fall back to the interpretation of the input when the tool has
> already figured out to second guess the intention of the user like the
> above message does.
>
> That would obviously break scripts that try to make sure the _absense_ of
> a path in a tree-ish (or in the index if you lack "HEAD" in your example),
> expecting:
>
> git rev-parse HEAD:test-lib.sh ||
> echo "test-lib.sh shouldn't be at the top level"
There's another (more important IMO) problem: if HEAD contains both
file.txt and subdir/file.txt, then what should
cd subdir
git show HEAD:file.txt
do? Allowing HEAD:file-in-a-subdirectory.txt would indirectly mean
teaching users to use in, and they'd be bitten by an ambiguous
behavior one day or another ...
> Perhaps the second step would be to teach the machinery to understand a
> syntax like "<tree-ish>:./<path>" and have it prefix the path to the
> current subdirectory from the root of the work tree, and with such an
> enhancement, the suggestion given by this patch would probably change to
> "Did you mean 'HEAD:./test-lib.sh'?", but that would be a separate
> topic.
Exactly. I think this HEAD:./relative-path syntax has been discussed
here already, but I don't remember the outcome of the discussion. If
it's ever implemented, my patch, modified as you suggest will help
users to discover the feature ;-).
BTW, I finally had time to write a test. Current version below FYI
(fixed a segfault discovered by testing too), but I'll resend properly
when I get enough feedback.
>From 1bc1c8d114b538f57a5ba4a501436229d644d587 Mon Sep 17 00:00:00 2001
From: Matthieu Moy <Matthieu.Moy@imag.fr>
Date: Mon, 30 Nov 2009 17:32:39 +0100
Subject: [PATCH] Detailed diagnosis when parsing an object name fails.
The previous error message was the same in many situations (unknown
revision or path not in the working tree). We try to help the user as
much as possible to understand the error, especially with the
sha1:filename notation. In this case, we say whether the sha1 or the
filename is problematic, and diagnose the confusion between
relative-to-root and relative-to-$PWD confusion precisely.
The 6 new error messages are tested.
---
cache.h | 6 ++-
setup.c | 15 +++++-
sha1_name.c | 95 ++++++++++++++++++++++++++++++++++++++--
t/t1506-rev-parse-diagnosis.sh | 67 ++++++++++++++++++++++++++++
4 files changed, 176 insertions(+), 7 deletions(-)
create mode 100755 t/t1506-rev-parse-diagnosis.sh
diff --git a/cache.h b/cache.h
index 0e69384..5c8cb5f 100644
--- a/cache.h
+++ b/cache.h
@@ -708,7 +708,11 @@ static inline unsigned int hexval(unsigned char c)
#define DEFAULT_ABBREV 7
extern int get_sha1(const char *str, unsigned char *sha1);
-extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);
+static inline get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
+{
+ return get_sha1_with_mode_1(str, sha1, mode, 0, NULL);
+}
+extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int fatal, const char *prefix);
extern int get_sha1_hex(const char *hex, unsigned char *sha1);
extern char *sha1_to_hex(const unsigned char *sha1); /* static buffer result! */
extern int read_ref(const char *filename, unsigned char *sha1);
diff --git a/setup.c b/setup.c
index f67250b..3094e8b 100644
--- a/setup.c
+++ b/setup.c
@@ -74,6 +74,18 @@ int check_filename(const char *prefix, const char *arg)
die_errno("failed to stat '%s'", arg);
}
+static void NORETURN die_verify_filename(const char *prefix, const char *arg)
+{
+ unsigned char sha1[20];
+ unsigned mode;
+ /* try a detailed diagnostic ... */
+ get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);
+ /* ... or fall back the most general message. */
+ die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
+ "Use '--' to separate paths from revisions", arg);
+
+}
+
/*
* Verify a filename that we got as an argument for a pathspec
* entry. Note that a filename that begins with "-" never verifies
@@ -87,8 +99,7 @@ void verify_filename(const char *prefix, const char *arg)
die("bad flag '%s' used after filename", arg);
if (check_filename(prefix, arg))
return;
- die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
- "Use '--' to separate paths from revisions", arg);
+ die_verify_filename(prefix, arg);
}
/*
diff --git a/sha1_name.c b/sha1_name.c
index 44bb62d..030e2ac 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -804,7 +804,77 @@ int get_sha1(const char *name, unsigned char *sha1)
return get_sha1_with_mode(name, sha1, &unused);
}
-int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
+static void diagnose_invalid_sha1_path(const char *prefix,
+ const char *filename,
+ const char *tree_sha1,
+ const char *object_name)
+{
+ struct stat st;
+ unsigned char sha1[20];
+ unsigned mode;
+
+ if (!prefix)
+ prefix = "";
+
+ if (!lstat(filename, &st))
+ die("Path '%s' exists on disk, but not in '%s'.",
+ filename, object_name);
+ if (errno == ENOENT || errno == ENOTDIR) {
+ char *fullname = malloc(strlen(filename)
+ + strlen(prefix) + 1);
+ strcpy(fullname, prefix);
+ strcat(fullname, filename);
+
+ if (!get_tree_entry(tree_sha1, fullname,
+ sha1, &mode)) {
+ die("Path '%s' exists, but not '%s'.\n"
+ "Did you mean '%s:%s'?",
+ fullname,
+ filename,
+ object_name,
+ fullname);
+ }
+ die("Path '%s' does not exist in '%s'",
+ filename, object_name);
+ }
+}
+
+static void diagnose_invalid_index_path(int stage,
+ const char *prefix,
+ const char *filename)
+{
+ struct stat st;
+
+ if (!prefix)
+ prefix = "";
+
+ if (!lstat(filename, &st))
+ die("Path '%s' exists on disk, but not in the index.", filename);
+ if (errno == ENOENT || errno == ENOTDIR) {
+ struct cache_entry *ce;
+ int pos;
+ int namelen = strlen(filename) + strlen(prefix);
+ char *fullname = malloc(namelen + 1);
+ strcpy(fullname, prefix);
+ strcat(fullname, filename);
+ pos = cache_name_pos(fullname, namelen);
+ if (pos < 0)
+ pos = -pos - 1;
+ ce = active_cache[pos];
+ if (ce_namelen(ce) == namelen &&
+ !memcmp(ce->name, fullname, namelen))
+ die("Path '%s' is in the index, but not '%s'.\n"
+ "Did you mean ':%d:%s'?",
+ fullname, filename,
+ stage, fullname);
+
+ die("Path '%s' does not exist (neither on disk nor in the index).",
+ filename);
+ }
+}
+
+
+int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int fatal, const char *prefix)
{
int ret, bracket_depth;
int namelen = strlen(name);
@@ -850,6 +920,8 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
}
pos++;
}
+ if (fatal)
+ diagnose_invalid_index_path(stage, prefix, cp);
return -1;
}
for (cp = name, bracket_depth = 0; *cp; cp++) {
@@ -862,9 +934,24 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
}
if (*cp == ':') {
unsigned char tree_sha1[20];
- if (!get_sha1_1(name, cp-name, tree_sha1))
- return get_tree_entry(tree_sha1, cp+1, sha1,
- mode);
+ char *object_name;
+ if (fatal) {
+ object_name = malloc(cp-name+1);
+ strncpy(object_name, name, cp-name);
+ object_name[cp-name] = '\0';
+ }
+ if (!get_sha1_1(name, cp-name, tree_sha1)) {
+ const char *filename = cp+1;
+ ret = get_tree_entry(tree_sha1, filename, sha1, mode);
+ if (fatal)
+ diagnose_invalid_sha1_path(prefix, filename,
+ tree_sha1, object_name);
+
+ return ret;
+ } else {
+ if (fatal)
+ die("Invalid object name '%s'.", object_name);
+ }
}
return ret;
}
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
new file mode 100755
index 0000000..8112d56
--- /dev/null
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+
+test_description='test git rev-parse diagnosis for invalid argument'
+
+exec </dev/null
+
+. ./test-lib.sh
+
+HASH_file=
+
+test_expect_success 'set up basic repo' '
+ echo one > file.txt &&
+ mkdir subdir &&
+ echo two > subdir/file.txt &&
+ echo three > subdir/file2.txt &&
+ git add . &&
+ git commit -m init &&
+ echo four > index-only.txt &&
+ git add index-only.txt &&
+ echo five > disk-only.txt
+'
+
+test_expect_success 'correct file objects' '
+ HASH_file=$(git rev-parse HEAD:file.txt) &&
+ git rev-parse HEAD:subdir/file.txt &&
+ git rev-parse :index-only.txt &&
+ cd subdir &&
+ git rev-parse HEAD:file.txt &&
+ git rev-parse HEAD:subdir/file2.txt &&
+ test $HASH_file = $(git rev-parse HEAD:file.txt) &&
+ test $HASH_file = $(git rev-parse :file.txt) &&
+ test $HASH_file = $(git rev-parse :0:file.txt) &&
+ cd ..
+'
+
+test_expect_success 'incorrect revision id' '
+ test_must_fail git rev-parse foobar:file.txt 2>&1 |
+ grep "Invalid object name '"'"'foobar'"'"'." &&
+ test_must_fail git rev-parse foobar 2>&1 |
+ grep "unknown revision or path not in the working tree."
+'
+
+test_expect_success 'incorrect file in sha1:path' '
+ test_must_fail git rev-parse HEAD:nothing.txt 2>&1 |
+ grep "fatal: Path '"'"'nothing.txt'"'"' does not exist in '"'"'HEAD'"'"'" &&
+ test_must_fail git rev-parse HEAD:index-only.txt 2>&1 |
+ grep "fatal: Path '"'"'index-only.txt'"'"' exists on disk, but not in '"'"'HEAD'"'"'." &&
+ cd subdir &&
+ test_must_fail git rev-parse HEAD:file2.txt 2>&1 |
+ grep "Did you mean '"'"'HEAD:subdir/file2.txt'"'"'?" &&
+ cd ..
+'
+
+test_expect_success 'incorrect file in :path and :0:path' '
+ test_must_fail git rev-parse :nothing.txt 2>&1 |
+ grep "fatal: Path '"'"'nothing.txt'"'"' does not exist (neither on disk nor in the index)." &&
+ test_must_fail git rev-parse :1:nothing.txt 2>&1 |
+ grep "Path '"'"'nothing.txt'"'"' does not exist (neither on disk nor in the index)." &&
+ cd subdir &&
+ test_must_fail git rev-parse :file2.txt 2>&1 |
+ grep "Did you mean '"'"':0:subdir/file2.txt'"'"'?" &&
+ cd .. &&
+ test_must_fail git rev-parse :disk-only.txt 2>&1 |
+ grep "fatal: Path '"'"'disk-only.txt'"'"' exists on disk, but not in the index."
+'
+
+test_done
--
1.6.6.rc0.256.g6060
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply related
* Re: [PATCH] tests: handle NO_PYTHON setting
From: Jeff King @ 2009-11-30 20:54 UTC (permalink / raw)
To: Brandon Casey; +Cc: Junio C Hamano, Sverre Rabbelier, git
In-Reply-To: <0OMCWm31DRbUGCfPIS6SSJn59HQIM9AKtSDgVSNdiSR99clfmJ3D-Q@cipher.nrlssc.navy.mil>
On Mon, Nov 30, 2009 at 12:07:40PM -0600, Brandon Casey wrote:
> Shouldn't this section be moved down below the sourcing of ../GIT-BUILD-OPTIONS
> on line 656 so that the value of NO_PYTHON will be available when running the
> test scripts directly?
Oops, good catch. I stupidly tested with "make NO_PYTHON=1 test" instead
of actually checking that GIT-BUILD-OPTIONS was propagating it
correctly.
> ps. There's something eerily familiar about this patch.
Hmmm. Yes, I didn't search before writing it, but you probably mean:
http://article.gmane.org/gmane.comp.version-control.git/127172
But that is missing the NO-PYTHON bit in GIT-BUILD-OPTIONS (did you
forget it there, or was it part of some other patch that also didn't get
applied?).
Also, I am tempted to move the GIT-BUILD-OPTIONS invocation _up_. It
is about reading config and should probably come before we start doing
_anything_.
So maybe this instead:
-- >8 --
Subject: [PATCH] tests: handle NO_PYTHON setting
Without this, test-lib checks that the git_remote_helpers
directory has been built. However, if we are building
without python, we will not have done anything at all in
that directory, and test-lib's sanity check will fail.
We bump the inclusion of GIT-BUILD-OPTIONS further up in
test-lib; it contains configuration, and as such should be
read before we do any checks (and in this particular case,
we need its value to do our check properly).
Signed-off-by: Jeff King <peff@peff.net>
---
I moved the BUILD-OPTIONS thing to just above the beginning of the
"have you built anything" checks, but after all of the function
definitions. But perhaps it should simply go at the very top of the
script. After all, in the case of "make NO_PYTHON=1 test", those
variables will already be defined at the very beginning of the script.
Makefile | 1 +
t/test-lib.sh | 6 +++---
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index 42744a4..443565e 100644
--- a/Makefile
+++ b/Makefile
@@ -1743,6 +1743,7 @@ GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
@echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@
@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@
+ @echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@
### Detect Tck/Tk interpreter path changes
ifndef NO_TCLTK
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4a40520..2d523fe 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -632,13 +632,15 @@ GIT_CONFIG_NOSYSTEM=1
GIT_CONFIG_NOGLOBAL=1
export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_CONFIG_NOGLOBAL
+. ../GIT-BUILD-OPTIONS
+
GITPERLLIB=$(pwd)/../perl/blib/lib:$(pwd)/../perl/blib/arch/auto/Git
export GITPERLLIB
test -d ../templates/blt || {
error "You haven't built things yet, have you?"
}
-if test -z "$GIT_TEST_INSTALLED"
+if test -z "$GIT_TEST_INSTALLED" && test -z "$NO_PYTHON"
then
GITPYTHONLIB="$(pwd)/../git_remote_helpers/build/lib"
export GITPYTHONLIB
@@ -653,8 +655,6 @@ if ! test -x ../test-chmtime; then
exit 1
fi
-. ../GIT-BUILD-OPTIONS
-
# Test repository
test="trash directory.$(basename "$0" .sh)"
test -n "$root" && test="$root/$test"
--
1.6.6.rc0.327.gd49b
^ permalink raw reply related
* Re: "git merge" merges too much!
From: Junio C Hamano @ 2009-11-30 20:50 UTC (permalink / raw)
To: The Git Mailing List; +Cc: Junio C Hamano
In-Reply-To: <m1NFBAx-000kmgC@most.weird.com>
"Greg A. Woods" <woods@planix.com> writes:
> .... This isn't really a Git problem I suppose, except
> for the fact that it means the lack of easy support for multiple working
> directories that track different branches makes this kind of development
> somewhat more difficult to do with Git than with, say, CVS.
You want to google for git-new-workdir then; it is found in
contrib/workdir and fairly widely used.
^ permalink raw reply
* Re: equal-tree-merges as way to make rebases fast-forward-able
From: Junio C Hamano @ 2009-11-30 20:32 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Bernhard R. Link, git
In-Reply-To: <200911302026.53933.j6t@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> On Montag, 30. November 2009, Junio C Hamano wrote:
>> To avoid that, I think (1) the marker has to be more reliable than just
>> "happens to have the same tree", and (2) the traversal done by Porcelains
>> (your patches 3 thru 5) by default should be unaware of eqt.
>>
>> I don't know what a suitable marker should look like, though. The marker
>> must be easily identifiable by the lowest level rev-list machinery, so it
>> needs to be a sign left somewhere in the commit object.
>
> Wouldn't the pathspec . be the marker:
>
> git rev-list HEAD -- .
>
> follows only one of the branches that have identical trees.
Because I am saying that "this commit has two parents and they record the
identical trees" is a condition that is too weak to mark a special-purpose
merge to bind the latest and an earlier version of a series, your rev-list
example command line should not be the way to identify such a mark commit
and act differently upon seeing one.
Actually your command line is even weaker, I think, although it would not
make much difference in real-life. The marker as currently Bernhard
implements not only has parents with identical trees, but the tree it has
also matches those of its parents.
You can make a commit that merges two branches that independently reached
the same conclusion (which git is designed to handle as an ordinary event
in real life), and amend that commit into an evil merge that has different
contents from its parents (which, I suspect, does not have much use in
practice), and your rev-list will drop one of the branches for even such a
commit, mistaking it as a marker when it is clearly not one.
My "it would not make much difference in real-life" in the two paragraphs
above comes purely from "such an evil merge would not have much use in
practice". We should make sure that "two branches that reached the same
conclusion" is not mistaken with a marker this series introduces.
^ permalink raw reply
* Re: [RFC/PATCH] Detailed diagnostic when parsing an object name fails.
From: Junio C Hamano @ 2009-11-30 20:12 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <1259603421-10055-1-git-send-email-Matthieu.Moy@imag.fr>
Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> The original motivation is actually to anwser the FAQ of "git show
> HEAD:foo.txt" being relative to the root, while some users would
> expect it relative to $PWD but the patch ended up being far more
> general. At least,
>
> $ cd git
> $ cd t
> $ ../git show HEAD:test-lib.sh
> fatal: Path 't/test-lib.sh' exists, but not 'test-lib.sh'.
> Did you mean 'HEAD:t/test-lib.sh'?
The first thought that comes to mind is that if it makes more sense to
just fall back to the interpretation of the input when the tool has
already figured out to second guess the intention of the user like the
above message does.
That would obviously break scripts that try to make sure the _absense_ of
a path in a tree-ish (or in the index if you lack "HEAD" in your example),
expecting:
git rev-parse HEAD:test-lib.sh ||
echo "test-lib.sh shouldn't be at the top level"
to work regardless of where you are in the work tree, so it won't fly
well.
Perhaps the second step would be to teach the machinery to understand a
syntax like "<tree-ish>:./<path>" and have it prefix the path to the
current subdirectory from the root of the work tree, and with such an
enhancement, the suggestion given by this patch would probably change to
"Did you mean 'HEAD:./test-lib.sh'?", but that would be a separate topic.
^ permalink raw reply
* Re: [PATCH 3/8] git-merge-recursive-{ours,theirs}
From: Avery Pennarun @ 2009-11-30 20:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7viqcrlrb8.fsf@alter.siamese.dyndns.org>
On Mon, Nov 30, 2009 at 2:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Avery Pennarun <apenwarr@gmail.com> writes:
>>> I've queued the series with minor fixes to 'pu' and pushed it out.
>>
>> Since I see you didn't change a couple of things you mentioned in
>> earlier comments (the NEEDSWORK comment and the sq-then-eval trick) do
>> you still want me to respin this series?
>
> The commit still is NEEDSWORK and shouldn't be in 'next' in its current
> shape. I don't think the topic is 1.6.6 material yet, and we will be in
> pre-release feature freeze any minute now, so there is no urgency.
>
> As I did the sq-then-eval in many places in our Porcelain scripts (and
> many of them are converted to C and lost the need for the trick), I may
> get tempted to fix it up when I am bored ;-). But no promises.
I'll interpret that as "no, I should not respin the series because
Junio plans to deal with it" :)
Do let me know if there's anything I should do to help this advance
from pu->next sooner (if they delay is not simply because of the code
freeze).
Have fun,
Avery
^ permalink raw reply
* Re: [PATCH 3/8] git-merge-recursive-{ours,theirs}
From: Junio C Hamano @ 2009-11-30 20:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Avery Pennarun, git
In-Reply-To: <7viqcrlrb8.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Avery Pennarun <apenwarr@gmail.com> writes:
>
>>> I've queued the series with minor fixes to 'pu' and pushed it out.
>>
>> Since I see you didn't change a couple of things you mentioned in
>> earlier comments (the NEEDSWORK comment and the sq-then-eval trick) do
>> you still want me to respin this series?
>
> The commit still is NEEDSWORK and shouldn't be in 'next' in its current
> shape.
Oh, I think you meant the "NEEDSWORK -- we limit to depth 2 when we
guess" and that has been with us ever since we added subtree merge, and it
is no reason to block the topic. I had the sq-then-eval stuff in mind
when I wrote above.
Sorry for the confusion.
^ permalink raw reply
* Re: [PATCH 3/8] git-merge-recursive-{ours,theirs}
From: Junio C Hamano @ 2009-11-30 19:56 UTC (permalink / raw)
To: Avery Pennarun; +Cc: git
In-Reply-To: <32541b130911301008v4156f0c6ge9f30952565392f9@mail.gmail.com>
Avery Pennarun <apenwarr@gmail.com> writes:
>> I've queued the series with minor fixes to 'pu' and pushed it out.
>
> Since I see you didn't change a couple of things you mentioned in
> earlier comments (the NEEDSWORK comment and the sq-then-eval trick) do
> you still want me to respin this series?
The commit still is NEEDSWORK and shouldn't be in 'next' in its current
shape. I don't think the topic is 1.6.6 material yet, and we will be in
pre-release feature freeze any minute now, so there is no urgency.
As I did the sq-then-eval in many places in our Porcelain scripts (and
many of them are converted to C and lost the need for the trick), I may
get tempted to fix it up when I am bored ;-). But no promises.
Thanks.
^ permalink raw reply
* Re: equal-tree-merges as way to make rebases fast-forward-able
From: Johannes Sixt @ 2009-11-30 19:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Bernhard R. Link, git
In-Reply-To: <7v8wdnooza.fsf@alter.siamese.dyndns.org>
On Montag, 30. November 2009, Junio C Hamano wrote:
> To avoid that, I think (1) the marker has to be more reliable than just
> "happens to have the same tree", and (2) the traversal done by Porcelains
> (your patches 3 thru 5) by default should be unaware of eqt.
>
> I don't know what a suitable marker should look like, though. The marker
> must be easily identifiable by the lowest level rev-list machinery, so it
> needs to be a sign left somewhere in the commit object.
Wouldn't the pathspec . be the marker:
git rev-list HEAD -- .
follows only one of the branches that have identical trees.
-- Hannes
^ permalink raw reply
* Re: "git merge" merges too much!
From: Dmitry Potapov @ 2009-11-30 19:22 UTC (permalink / raw)
To: The Git Mailing List; +Cc: Jeff King
In-Reply-To: <m1NFAji-000kn2C@most.weird.com>
On Mon, Nov 30, 2009 at 01:12:31PM -0500, Greg A. Woods wrote:
>
> The way "git merge" works does concern me somewhat though as I try to
> figure out how I might use "topic" branches to develop local features
> and then merge them onto each supported release branch.
The basic idea of using topic branches is development is done on
separate branches are merged to the release branch only when they are
ready to be released. These branches are based on the oldest branch in
what they may be included. It means that fixes are normally based on the
stable branch and new feature are based on the master branch, i.e. the
branch that contains changes for the next new feature release. Not all
branches got merged immediately into master. For instance, the git
project has 'pu' (proposed updates) and 'next' branches. Only when a new
feature proved itself to be useful and reliable, it is "graduated" to
the master branch. Thus the master branch is rather stable and it is
released on regular intervals (no need for a long stabilization period).
The key difference comparing to what you may got used is that branches
are normally based on the oldest branch in what this feature may be
included. Thus normally changes are not backported to old branches,
because you can merge them directly.
> > Yes, you must cherry-pick or use rebase (which is a more featureful
> > version of the pipeline you mentioned).
>
> "git rebase" will not work for me unless it grows a "copy" option ,
> i.e. one which does not delete the original branch (i.e. avoids the
> "reset" phase of its operation).
There is no reset phase... It is just reassigning the head of branch to
point to a different commit-id. If you want to copy a branch instead of
rebasing the old one, you create a new branch (a new name) that points
to the same commit as the branch that you want to copy, after that you
rebase this new branch. You can do that like this:
$ git branch new-foo foo
$ git rebase --onto newbase oldbase new-foo
> It likely wouldn't make sense to base this new "copy" feature directly
> on "git rebase" though, especially in light of all the warnings about
> how "git rebase" isn't friendly when applied to already published
> branches. I think in theory this "copy" feature won't cause problems
> for already-published branches.
The "copy" does not have the problem of rebase, but it has a different
problem: You have two series of commits instead of one. If you found
a bug in one of those commits, you will have to patch each series
separately. Also, git merge may produce additional conflicts... So,
copying commits is not something that I would recommend to do often.
Dmitry
^ permalink raw reply
* Re: What is the best way to backport a feature?
From: Greg A. Woods @ 2009-11-30 19:08 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Peter Weseloh, The Git Mailing List
In-Reply-To: <4B12A928.2000401@drmicha.warpmail.net>
[-- Attachment #1: Type: text/plain, Size: 3457 bytes --]
Hmmm.... this topic seems in part to be very close to my thread
"git merge" merges too much!
At Sun, 29 Nov 2009 18:02:32 +0100, Michael J Gruber <git@drmicha.warpmail.net> wrote:
Subject: Re: What is the best way to backport a feature?
>
> Seriously, I suggest reading up on "topic branches". Feature_A should
> have been based off the common merge base of Mainline and Release_1.0,
> and, even more importantly, there should not have been any merges from
> Mainline into Feature_A. So, that branch is not at all what one would
> call a feature branch/topic branch. Hopefully, this scenario is very
> uncommon :)
IFF I understand the original post correctly then actually in my
experience this scenario is VERY common in some environments!
This is exactly how large projects which use the likes of CVS (and SVN?)
manage longer-running development branches for important new features.
One excellent example is NetBSD (and perhaps the other BSDs too).
A developer creates a "working" branch from the trunk, then begins to
make changes and commits to that branch. Periodically the (entire)
trunk is merged again to the working branch. I think this is somewhat
equivalent to using "git rebase" to re-apply the feature branch to a new
fork point from the trunk. However the actual branch base point remains
at the original point -- it is only the delta between the last merge
from trunk and the current head of the trunk which is merged onto the
feature working branch. I think this is what people in the CVS world
mean when they say they want the tool to remember the point on the
source branch from where they did the last merge. They've got their
work-flow "backwards", but this is the best they can do with CVS.
These periodic merges from the trunk mean that once the feature is
finished the delta between the trunk and the new feature branch is going
to be just the new feature, and so merging that delta alone to the trunk
as one commit adds the new feature to the trunk with few or no
conflicts, and the feature working branch can finally be "closed".
I'm guessing that people moving to Git from CVS may choose to stick with
this pattern where they periodically merge-from-master to keep
long-running feature branches as close to in-sync with the master branch
as possible (to avoid final merge conflicts). Ideally, IIUC, perhaps
they should use rebase instead.
Perhaps this "mess" can indeed be cleaned up using "git rebase -i" so
that the final version of the feature branch can be back-ported more
easily (though one will still need to use git-cherry-pick or git-am to
do the back-port to the previous release branch). The result of the
cleanup, before the merge of Feature_A to 1.0 might look more like this:
o--o--o Release_1.0
/ \ \
o-o-o--o--o-o-o-o-o-o-o-o---------------o Mainline
\ /
F1'--F2'--F3' Feature_A
and then after the merge of Feature_A to Release_1.0:
o--o--o--F1''--F2''--F3'' Release_1.0
/ \ \
o-o-o--o--o-o-o-o-o-o-o-o---------------o Mainline
\ /
F1'--F2'--F3' Feature_A
--
Greg A. Woods
+1 416 218-0098 VE3TCP RoboHack <woods@robohack.ca>
Planix, Inc. <woods@planix.com> Secrets of the Weird <woods@weird.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 186 bytes --]
^ permalink raw reply
* Re: equal-tree-merges as way to make rebases fast-forward-able
From: Bernhard R. Link @ 2009-11-30 18:55 UTC (permalink / raw)
To: git
In-Reply-To: <7v8wdnooza.fsf@alter.siamese.dyndns.org>
* Junio C Hamano <gitster@pobox.com> [091130 19:19]:
> "Bernhard R. Link" <brlink@debian.org> writes:
>
> > My idea to solve this is combining both histories, the rebased/revised
> > history and the actualy history, marking with some "equal-tree-merge"
> > the point where they have the same result.
>
> If you rewrite a series twice, your RFC will work like this, IIUC:
>
> * You have commit 1 and rewrite it to 2. You record the difference
> between 1 and 2 on top of 1 as commit X and record a same-tree merge as
> A. Here, A^1 == 2, A^2 == X, and 2^{tree} == A^{tree}.
>
> 2-------A
> / /
> 0---1---X
>
> * You then rewrite it to 3. You record the difference between A and 3
> (which is the same as between 2 and 3, because 2^{tree} == A^{tree})
> as commit Y, and record a same-tree merge as B. B^1 == 3, B^2 == Y and
> 3^{tree} == B^{tree}.
>
> Y---------------B
> / /
> 2-------A-------3
> / /
> 0---1---X
I think it rather looks like this:
3---------------B
| /
| 2-------A---Y
|/ /
0---1---X
>
> 3-------.
> / \
> 0---2---W---B
> \ /
> 1-------Z
>
> That is, Z and W records the interdifff between 1 to 3 and 2 to 3
> respectively, and B is a same-tree merge of 3, W and Z.
I think changing it to get this would be easy (though only in the case
where the very last commit was such an equal tree merge), but I do not
think it would be actually better:
- it is no longer possible to see the history of changes by just walking
right on every equal-tree-merge.
- commit a no longer exists. If some downstream already has
cloned/pulled, no fast-forward is possible any more.
> While I find the primary idea (i.e. keeping the old and new equivalents by
> recording a merge of it, and using the first-parent to traverse when you
> find such a special merge) reasonable (and as Dscho has pointed out, this
> technique is widely used, I suspect---it is an obvious thing to do), I
> think we need something stronger than just "this commit merges commits
> that happen to have the same trees" as the marker.
I've considered adding a new header or only a magic description text for those
commits, but I think it is not necessary.
Because the actual programs making it useful to treat this special
(format-patch producing too many patches, rebases possibly showing conflicts
already resolved and bisect walking too many branches) will be the same when
two branches only resulting in the same tree by pure chance show up.
> To avoid that, I think (1) the marker has to be more reliable than just
> "happens to have the same tree", and (2) the traversal done by Porcelains
> (your patches 3 thru 5) by default should be unaware of eqt.
I think for patch 3 (format-patch) and 4 (rebase -i) it is always better to
have the new behaviour even when only hitting equal trees by chance.
I'm unsure about 5 (rebase -m), but guess it still is.
> I don't know what a suitable marker should look like, though. The marker
> must be easily identifiable by the lowest level rev-list machinery, so it
> needs to be a sign left somewhere in the commit object. Perhaps making it
> require to have the same tree as all its parents _and_ a well-known marker
> string in the log message (and nothing else) would be a good start.
It already does always create a unique log message. So one could also
have one more strict and one less strict mode (and some option to decide
on the default).
Hochachtungsvoll,
Bernhard R. Link
--
"Never contain programs so few bugs, as when no debugging tools are available!"
Niklaus Wirth
^ 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