* Re: [PATCH] When exec'ing sub-commands, fall back on execvp (the PATH)
From: Johannes Schindelin @ 2007-10-20 7:30 UTC (permalink / raw)
To: Scott Parish; +Cc: git
In-Reply-To: <20071020064459.GB2237@srparish.net>
Hi,
On Fri, 19 Oct 2007, Scott Parish wrote:
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 9b74ed2..674c9f3 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -34,15 +34,15 @@ int execv_git_cmd(const char **argv)
> {
> char git_command[PATH_MAX + 1];
> int i;
> + int rc;
> const char *paths[] = { current_exec_path,
> getenv(EXEC_PATH_ENVIRONMENT),
> builtin_exec_path };
> + const char *tmp;
> + size_t len;
>
> for (i = 0; i < ARRAY_SIZE(paths); ++i) {
> - size_t len;
> - int rc;
> const char *exec_dir = paths[i];
> - const char *tmp;
>
> if (!exec_dir || !*exec_dir) continue;
>
> @@ -106,8 +106,26 @@ int execv_git_cmd(const char **argv)
>
> argv[0] = tmp;
> }
> - return -1;
>
> + rc = snprintf(git_command, sizeof(git_command), "git-%s", argv[0]);
> + if (rc < 0 || rc >= sizeof(git_command) - len) {
> + fprintf(stderr, "git: command name given is too long.\n");
> + return -1;
> + }
> +
> + tmp = argv[0];
> + argv[0] = git_command;
> +
> + trace_argv_printf(argv, -1, "trace: exec:");
> +
> + /* execve() can only ever return if it fails */
> + execvp(git_command, (char **)argv);
> +
> + trace_printf("trace: exec failed: %s\n", strerror(errno));
> +
> + argv[0] = tmp;
> +
> + return -1;
> }
I am not sure that this is elegant enough: Something like this (completely
untested) might be better:
diff --git a/exec_cmd.c b/exec_cmd.c
index 9b74ed2..c928f37 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -36,7 +36,8 @@ int execv_git_cmd(const char **argv)
int i;
const char *paths[] = { current_exec_path,
getenv(EXEC_PATH_ENVIRONMENT),
- builtin_exec_path };
+ builtin_exec_path,
+ "" };
for (i = 0; i < ARRAY_SIZE(paths); ++i) {
size_t len;
@@ -44,9 +45,12 @@ int execv_git_cmd(const char **argv)
const char *exec_dir = paths[i];
const char *tmp;
- if (!exec_dir || !*exec_dir) continue;
+ if (!exec_dir) continue;
- if (*exec_dir != '/') {
+ if (!*exec_dir)
+ /* try PATH */
+ *git_command = '\0';
+ else if (*exec_dir != '/') {
if (!getcwd(git_command, sizeof(git_command))) {
fprintf(stderr, "git: cannot determine "
"current directory: %s\n",
@@ -81,7 +85,7 @@ int execv_git_cmd(const char **argv)
len = strlen(git_command);
rc = snprintf(git_command + len, sizeof(git_command) - len,
- "/git-%s", argv[0]);
+ "%sgit-%s", *exec_dir ? "/" : "", argv[0]);
if (rc < 0 || rc >= sizeof(git_command) - len) {
fprintf(stderr,
"git: command name given is too long.\n");
Ciao,
Dscho
^ permalink raw reply related
* Issues for git-submodule
From: Yin Ping @ 2007-10-20 7:31 UTC (permalink / raw)
To: git
In-Reply-To: <46dff0320710200027o5fe434b4i9bd4f3ffc17f03f6@mail.gmail.com>
1. gti-submodule status
As the manual says, '+' is shown if the currently checked out
submodule commit does not match the SHA-1 found in the index of the
containing repository.
However, not matching has two cases: one is a new commit in the
submodule, the other is update of index of the submodule after a
command such as "git-pull".
So which is the case when a '+' is seen? Should i run 'git-commit'
or 'git-submodule update'? As a suggestion, I think git should tell
the user which commit is newer (the one in supermodule index or the
HEAD of the submodule) and even give the log entry between the two
commits.
2. As I first saw 'git-submodule status submoduepath', I thought it
should do the following thing
cd submoduepath && git-status && cd -
This is actually what i need, espacially when i have a lot of
submodules and has made changes in some modules. How do i know which
modules have been changed and how they have been changed?
So, In some degree, I even think current 'git-submodule status'
should be replaced by git-status and the right thing 'git-submodule
status' should do is to show the status of submodule itself as i just
said above.
--
franky
^ permalink raw reply
* [PATCH] Deduce exec_path also from calls to git with a relative path
From: Johannes Schindelin @ 2007-10-20 7:21 UTC (permalink / raw)
To: Scott R Parish; +Cc: git, spearce, gitster
In-Reply-To: <20071020064617.GC2237@srparish.net>
There is already logic in the git wrapper to deduce the exec_path from
argv[0], when the git wrapper was called with an absolute path. Extend
that logic to handle relative paths as well.
For example, when you call "../../hello/world/git", it will not turn
"../../hello/world" into an absolute path, and use that.
Initial implementation by Scott R Parish.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
On Fri, 19 Oct 2007, Scott R Parish wrote:
> Signed-off-by: Scott R Parish <srp@srparish.net>
That is a little short for a commit message ;-)
> git.c | 35 +++++++++++++++++++++++++++++++++--
> 1 files changed, 33 insertions(+), 2 deletions(-)
I had commented on this before. Probably I did a very bad job
at explaining things, so hopefully this is better:
git.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/git.c b/git.c
index d7c6bca..1dad764 100644
--- a/git.c
+++ b/git.c
@@ -414,13 +414,14 @@ int main(int argc, const char **argv)
/*
* Take the basename of argv[0] as the command
* name, and the dirname as the default exec_path
- * if it's an absolute path and we don't have
- * anything better.
+ * if we don't have anything better.
*/
if (slash) {
*slash++ = 0;
if (*cmd == '/')
exec_path = cmd;
+ else
+ exec_path = xstrdup(make_absolute_path(cmd));
cmd = slash;
}
--
1.5.3.4.1287.g8b31e
^ permalink raw reply related
* Re: Proposed git mv behavioral change
From: Shawn O. Pearce @ 2007-10-20 7:02 UTC (permalink / raw)
To: Michael Witten; +Cc: git
In-Reply-To: <8D972813-2D7F-4D6A-958F-B76E947E7BC3@MIT.EDU>
Michael Witten <mfwitten@MIT.EDU> wrote:
>
> On 20 Oct 2007, at 2:36:28 AM, Shawn O. Pearce wrote:
>
> >Today I move the file, then unstage the hunks I'm not sure about,
> >then go back and restage them. Annoying. It really disrupts
> >my workflow.
>
> I know it's against policy, but the proposed change should be set
> as the default at some point, in my opinion.
Its not so much policy as a timing issue.
Git 1.5.4 shouldn't have major distruptions to end-users in terms
of changing existing behavior to be different than in 1.5.3.
Especially default behavior.
Git 1.6.0 we're a little bit more willing to change default
behavior as it is a full major release. This is probably quite a
bit off still, unless we started to accumulate a lot of really good
changes that required breaking something in the user interface.
That's what happened with the 1.4.4 series vs. 1.5.0. Right now
I don't see that happening anytime soon.
> I have a feeling that my suggestion will not go far,
> but I also think that backwards compatibility can
> overstay its welcome.
I agree.
--
Shawn.
^ permalink raw reply
* Re: [RFC/PATCH] git-fetch: mega-terse fetch output
From: Shawn O. Pearce @ 2007-10-20 6:58 UTC (permalink / raw)
To: Jeff King
Cc: Nicolas Pitre, Johannes Sixt, Theodore Tso, Santi Béjar,
David Symonds, git
In-Reply-To: <20071020050019.GA27282@coredump.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> On Fri, Oct 19, 2007 at 10:14:59AM -0400, Nicolas Pitre wrote:
>
> > > ==> git://repo.or.cz/git/spearce.git
> > > * (new) gitk -> spearce/gitk
> > > * 1aa3d01..e7187e4 maint -> spearce/maint
> > > * de61e42..7840ce6 master -> spearce/master
> > > * 895be02..2fe5433 next -> spearce/next
> > > + 89fa332...1e4c517 pu -> spearce/pu
> > > * (new) todo -> spearce/todo
> >
> > Actually I think this is the best format so far: one line per branch, no
> > terminal width issue (long branch names are simply wrapped), the
> > old..new info is there also with the single character marker to quickly
> > notice the type of update.
Yea, I think this is almost the right format.
Nicolas Pitre <nico@cam.org> wrote:
> Agreed. ' ' = fast forward, '+' = forced update, and '!' = refused.
We're probably looking at something like this:
>From git://repo.or.cz/git/spearce.git
1aa3d01..e7187e4 maint -> spearce/maint
de61e42..7840ce6 master -> spearce/master
895be02..2fe5433 next -> spearce/next
(new) todo -> spearce/todo
(new) tag v1.6.0
+ 89fa332...1e4c517 pu -> spearce/pu (forced update)
! 2b5afb...289840 gitk -> spearce/gitk (non-fast forward)
Notice the sorting order by *type* of update. I think it makes
the code slightly more complicated in builtin-fetch as we need to
classify each ref into a type of update, then sort them by that
type, but it allows the end-user to see the most "important" (not
simple fast-forward updates) at the end of their terminal window,
especially if there were many fast-forward branches. Within a
class of update we still sort by ref name.
> Technically speaking, the hash IDs can be up to 80 characters long,
> since they are meant to be unique abbreviations. But in practice, I
> think leaving enough space for 10 + '...' + 10 should accomodate just
> about any project (IIRC, the kernel's longest non-unique is around 9).
Which nicely solves the issue with the window size as we aren't
really worring about it here in this display.
--
Shawn.
^ permalink raw reply
* [PATCH] If git is ran with a relative path, try building an absolute exec_path from it
From: Scott R Parish @ 2007-10-20 6:46 UTC (permalink / raw)
To: git
Signed-off-by: Scott R Parish <srp@srparish.net>
---
git.c | 35 +++++++++++++++++++++++++++++++++--
1 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/git.c b/git.c
index 9eaca1d..d129ecc 100644
--- a/git.c
+++ b/git.c
@@ -28,6 +28,35 @@ static void prepend_to_path(const char *dir, int len)
free(path);
}
+static char *rel_to_abs_exec_path(const char *cmd) {
+ int len, rc;
+ char *exec_path = xmalloc(PATH_MAX + 1);
+
+ if (!getcwd(exec_path, PATH_MAX)) {
+ fprintf(stderr, "git: cannot determine current directory: %s\n",
+ strerror(errno));
+ free(exec_path);
+ return NULL;
+ }
+ len = strlen(exec_path);
+
+ /* Trivial cleanup */
+ while (!prefixcmp(cmd, "./")) {
+ cmd += 2;
+ while (*cmd == '/')
+ cmd++;
+ }
+
+ rc = snprintf(exec_path + len, PATH_MAX - len, "/%s", cmd);
+ if (rc < 0 || rc >= PATH_MAX - len) {
+ fprintf(stderr, "git: command name given is too long.\n");
+ free(exec_path);
+ return NULL;
+ }
+
+ return exec_path;
+}
+
static int handle_options(const char*** argv, int* argc, int* envchanged)
{
int handled = 0;
@@ -409,13 +438,15 @@ int main(int argc, const char **argv)
/*
* Take the basename of argv[0] as the command
* name, and the dirname as the default exec_path
- * if it's an absolute path and we don't have
- * anything better.
+ * if we don't have anything better.
*/
if (slash) {
*slash++ = 0;
if (*cmd == '/')
exec_path = cmd;
+ else
+ exec_path = rel_to_abs_exec_path(cmd);
+
cmd = slash;
}
--
1.5.3.GIT
^ permalink raw reply related
* Re: Proposed git mv behavioral change
From: Michael Witten @ 2007-10-20 6:45 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
In-Reply-To: <20071020063628.GV14735@spearce.org>
On 20 Oct 2007, at 2:36:28 AM, Shawn O. Pearce wrote:
> Today I move the file, then unstage the hunks I'm not sure about,
> then go back and restage them. Annoying. It really disrupts
> my workflow.
I know it's against policy, but the proposed change should be set
as the default at some point, in my opinion.
Perhaps when the -u flagged is not used, there can be a warning that
states -u will become the default at a certain time.
In fact, -u speaks "update" to me, and I would expect it to signal
the current behavior.
I have a feeling that my suggestion will not go far,
but I also think that backwards compatibility can
overstay its welcome.
^ permalink raw reply
* [PATCH] When exec'ing sub-commands, fall back on execvp (the PATH)
From: Scott Parish @ 2007-10-20 6:44 UTC (permalink / raw)
To: git
Signed-off-by: Scott R Parish <srp@srparish.net>
---
exec_cmd.c | 26 ++++++++++++++++++++++----
1 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/exec_cmd.c b/exec_cmd.c
index 9b74ed2..674c9f3 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -34,15 +34,15 @@ int execv_git_cmd(const char **argv)
{
char git_command[PATH_MAX + 1];
int i;
+ int rc;
const char *paths[] = { current_exec_path,
getenv(EXEC_PATH_ENVIRONMENT),
builtin_exec_path };
+ const char *tmp;
+ size_t len;
for (i = 0; i < ARRAY_SIZE(paths); ++i) {
- size_t len;
- int rc;
const char *exec_dir = paths[i];
- const char *tmp;
if (!exec_dir || !*exec_dir) continue;
@@ -106,8 +106,26 @@ int execv_git_cmd(const char **argv)
argv[0] = tmp;
}
- return -1;
+ rc = snprintf(git_command, sizeof(git_command), "git-%s", argv[0]);
+ if (rc < 0 || rc >= sizeof(git_command) - len) {
+ fprintf(stderr, "git: command name given is too long.\n");
+ return -1;
+ }
+
+ tmp = argv[0];
+ argv[0] = git_command;
+
+ trace_argv_printf(argv, -1, "trace: exec:");
+
+ /* execve() can only ever return if it fails */
+ execvp(git_command, (char **)argv);
+
+ trace_printf("trace: exec failed: %s\n", strerror(errno));
+
+ argv[0] = tmp;
+
+ return -1;
}
--
1.5.3.GIT
^ permalink raw reply related
* Re: [PATCH] allow git to use the PATH for finding subcommands and help docs
From: Scott Parish @ 2007-10-20 6:42 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Sixt, git
In-Reply-To: <Pine.LNX.4.64.0710191616490.16728@wbgn129.biozentrum.uni-wuerzburg.de>
On Fri, Oct 19, 2007 at 04:27:39PM +0200, Johannes Schindelin wrote:
> While reading this, I have to wonder why it is not just simpler to try
> with builtin_exec_path first, and if that fails, just let exec() find the
> program in the PATH?
I think you're right; that is a much better way to do this. I've
rewritten this as two patches i'll post shortly. I have mixed
feelings about the MANPATH/PERL5LIB hack, so i'm leaving it out for
now.
sRp
--
Scott Parish
http://srparish.net/
^ permalink raw reply
* Re: Proposed git mv behavioral change
From: Jeff King @ 2007-10-20 6:40 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Ari Entlich, Michael Witten, git
In-Reply-To: <20071020063628.GV14735@spearce.org>
On Sat, Oct 20, 2007 at 02:36:28AM -0400, Shawn O. Pearce wrote:
> So lets say I make a change in my Makefile that changes the name
> of a source file to be more descriptive of that file's contents.
> I've reviewed the Makefile change *and* done the file rename,
> but I'm still not done reviewing the stuff in the file. (Yea,
> maybe that should be two different commits, but maybe not, lets
> not get into that as it depends very much on context.)
Right. So the exact state you have in your index never actually existed
in your working tree. But that's OK if:
- the changes are trivial and obviously correct
- you're not actually planning on _committing_ that, you just want to
build the commit using the index
And in those cases, git-stash is either, respectively, overkill or
totally useless.
Please ignore everything I said on this subject before today...I
obviously hadn't figured out what was going on.
-Peff
^ permalink raw reply
* Re: Proposed git mv behavioral change
From: Shawn O. Pearce @ 2007-10-20 6:36 UTC (permalink / raw)
To: Jeff King; +Cc: Ari Entlich, Michael Witten, git
In-Reply-To: <20071020062400.GA30388@coredump.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> No, I meant more that the state you have staged will never have actually
> existed in your working tree, so you could not possibly have tested it.
> However, it may be that the changes you are trying to avoid staging are
> trivial, and you're willing to accept that. And git shouldn't stand in
> your way.
>
> And it sounds like you don't necessarily want to make a _commit_ out of
> this, you just want to keep working and not have git-mv munge your
> staged state.
Yea, I want the same thing. :-)
And not because I commit things that weren't tested. Its because
I do mini code reviews for small hunks at a time. I stage things
I have looked at, and keep things I'm still unsure about unstaged.
But before I commit I make sure my `git diff` output is empty,
so I know that any tests I do now will match what I have when I
run git-commit.
So lets say I make a change in my Makefile that changes the name
of a source file to be more descriptive of that file's contents.
I've reviewed the Makefile change *and* done the file rename,
but I'm still not done reviewing the stuff in the file. (Yea,
maybe that should be two different commits, but maybe not, lets
not get into that as it depends very much on context.)
Today I move the file, then unstage the hunks I'm not sure about,
then go back and restage them. Annoying. It really disrupts
my workflow.
The killer feature in Git for me is the index. That sucker saves
me on a daily basis as it lets me organize "this i'm happy with,
this i'm not" while I'm in the middle of making a single new commit.
Today git-mv robs me of it.
--
Shawn.
^ permalink raw reply
* Re: Proposed git mv behavioral change
From: Jeff King @ 2007-10-20 6:36 UTC (permalink / raw)
To: Michael Witten; +Cc: git
In-Reply-To: <FF5A82CA-8AE3-4E1C-872D-26D938933E1B@MIT.EDU>
On Sat, Oct 20, 2007 at 02:34:49AM -0400, Michael Witten wrote:
> The first method is wrong, because I wasn't considering the fact that
> he may have already staged something.
Yes, I also didn't consider that, which is what led me to wonder what
his use case could possibly be. And thus we were both confused.
-Peff
^ permalink raw reply
* Re: Proposed git mv behavioral change
From: Michael Witten @ 2007-10-20 6:34 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20071020062400.GA30388@coredump.intra.peff.net>
On 20 Oct 2007, at 2:24:00 AM, Jeff King wrote:
> Because I was having trouble understanding what Michael was trying to
> accomplish with his "here are two methods, and one is better" since
> they
> seemed to do different things. But that is perhaps my fault for
> joining
> the thread in the middle. I may have simply caused more confusion by
> getting involved. :)
The first method is wrong, because I wasn't considering the fact that
he may have already staged something.
Because I thought both methods achieve the same result, I called the
latter
"better", because it is more efficient.
Michael Witten
mfwitten
^ permalink raw reply
* Re: Proposed git mv behavioral change
From: Jeff King @ 2007-10-20 6:34 UTC (permalink / raw)
To: Ari Entlich; +Cc: Shawn O. Pearce, git
In-Reply-To: <1192859758.13347.148.camel@g4mdd.entnet>
On Sat, Oct 20, 2007 at 01:55:58AM -0400, Ari Entlich wrote:
> Might it be possible to simply get the struct cache_entry for the file
> which we want to rename, change its name property (would this involve
> xreallocing it?), and change the ce_namelen field of ce_flags?
The sort order of entries in the cache is important, so you would have
to move the entry, as well.
-Peff
^ permalink raw reply
* Re: Proposed git mv behavioral change
From: Jeff King @ 2007-10-20 6:31 UTC (permalink / raw)
To: Ari Entlich; +Cc: Wincent Colaiuta, Michael Witten, git
In-Reply-To: <1192859753.13347.147.camel@g4mdd.entnet>
On Sat, Oct 20, 2007 at 01:55:53AM -0400, Ari Entlich wrote:
> git-stash has been suggested to me numerous times, but I really feel
> that there's no need to use it in this case - if the git mv command gave
> adequate control to the user, it would be unnecessary.
Yes, now that I understand what you're doing, git-stash is probably
overkill. It's great if you really want to put aside your current state
and do some other, totally unrelated work.
But in this case, you are talking about tools for managing the index
correctly, which is a totally different use case.
> > git mv --cached foo bar
> Are you talking about REALLY only changing the index? I can't think of
> why you'd want to do this either... After all, wouldn't there be no bar
> file to do git add --interactive on? In addition, I don't think giving
> --interactive a filename is meaningful...
Originally I had thought git-mv --cached would be useful (and a nice
orthogonal design), but it really _does_ seem useless, since it's
awkward to get your working tree changes into the index without actually
moving the working tree file.
> It might be interesting to do some sort of survey of whether people
> depend on this behavior. It seems pretty inconsistent with how git works
> otherwise, and I'd be surprised if a lot of people expect it (kinda like
> the Spanish Inquisition :-P).
The best way to get comments is to make a patch.
As for inconsitency of git-mv, it has always been a bit of an odd duck.
I'm fairly certain neither Linus nor Junio actually use it, preferring
instead to make all changes to the working tree and then update the
index manually. I even seem to recall Linus arguing that git-mv was
stupid and unnecessary a long time ago, but I could be wrong.
-Peff
^ permalink raw reply
* Re: Proposed git mv behavioral change
From: Shawn O. Pearce @ 2007-10-20 6:30 UTC (permalink / raw)
To: Ari Entlich; +Cc: git
In-Reply-To: <1192859758.13347.148.camel@g4mdd.entnet>
Ari Entlich <lmage11@twcny.rr.com> wrote:
> On Thu, 2007-10-18 at 21:54 -0400, Shawn O. Pearce wrote:
> > --index is used in Git for places were we update *both* the index
> > and the working directory (git-apply --index). So actually I should
> > have suggested "git-mv --index". Whoops.
>
> Alright then, I don't know about that particular convention. If this
> behavior can't be made default, git mv --index should activate it? I
> there anything else that might be more descriptive?
That's always the hard part. I actually think the current behavior
should be called --index as it does not only the working tree
update but also stages the whole file into the index, which is what
git-apply --index does.
What about just -u for "keep unstaged"?
> Might it be possible to simply get the struct cache_entry for the file
> which we want to rename, change its name property (would this involve
> xreallocing it?), and change the ce_namelen field of ce_flags?
Yes.
> In any case, I think the ce_flags would need to be changed to reflect
> the new name length. Also, it seems that the old ce_mtime, ce_dev,
> ce_ino, ce_uid, ce_gid, and ce_size could be used too... ce_ctime would
> need to be updated...
Correct. The ce_size shouldn't change as a result of the rename but
the other properties of a struct stat may. I'd like to think that
ce_uid and ce_gid wouldn't change as a result of a rename but there's
probably some bastard filesystem somewhere that that won't hold true.
Just call fill_stat_cache_info() on your xrealloc'd entry and pass
it a stat buffer for the file after the rename and you should be
covered here.
> > I'm sure Junio could probably give you a better starting point
> > than I can, as he's more familiar with this sort of code, but that
> > should still get you looking in the right direction and maybe get
> > a working implementation together that you can share for discussion.
>
> Yeah, I'd appreciate any help I can get. The people on #git were
> invoking the "the code is the documentation" ideology. :)
It really is in this case. You are talking about hacking on
Git itself. For the most part we believe in "the code is the
documentation" for the guts as otherwise the documentation gets
out of sync with the code when the code changes. User level docs
are different as the user interface needs to stay stable.
> So... Ping Junio...? I actually haven't seen any messages from him since
> I subscribed to this list (I've only gotten about 300 messages so far,
> but I'd expect to see a lot from him as he's the maintainer...). Is he
> away at the moment or something?
Yes. We run like 100 messages a day so I guess you've only joined
in the past few days. Welcome to the list. :-)
Junio has been away for about 2 weeks now. He's basically off
on leave as he needed some time to take care of some things.
I'm filling in for him until he returns.
--
Shawn.
^ permalink raw reply
* Re: Proposed git mv behavioral change
From: Jeff King @ 2007-10-20 6:24 UTC (permalink / raw)
To: Ari Entlich; +Cc: Michael Witten, git
In-Reply-To: <1192859748.13347.146.camel@g4mdd.entnet>
On Sat, Oct 20, 2007 at 01:55:48AM -0400, Ari Entlich wrote:
> Well, I also want to move the working directory file so that the index
> and the working directory still match up and so that their differences
> are preserved. Therefore, --cached isn't quite right (see Shawn's
> messages and my response to them).
Ah, OK. I understand now. So what you would want could be accomplished
with something like:
mv A B
(git-ls-files -s | sed s/A$/B/;
echo 0 0000000000000000000000000000000000000000 A) \
| git-update-index --index-info
IOW:
1. move working tree A to B
2. create staged entry B, same as staged entry A
3. remove staged entry A
> Hmm, that's an interesting point. Are you talking about situations in
> which your changes after (1) leave the program in an uncompilable state?
No, I meant more that the state you have staged will never have actually
existed in your working tree, so you could not possibly have tested it.
However, it may be that the changes you are trying to avoid staging are
trivial, and you're willing to accept that. And git shouldn't stand in
your way.
And it sounds like you don't necessarily want to make a _commit_ out of
this, you just want to keep working and not have git-mv munge your
staged state.
> I don't really see why you're analyzing the situations in which this
> would be used. I think it should be obvious from my descriptions of my
Because I was having trouble understanding what Michael was trying to
accomplish with his "here are two methods, and one is better" since they
seemed to do different things. But that is perhaps my fault for joining
the thread in the middle. I may have simply caused more confusion by
getting involved. :)
Now I think I understand exactly what you're trying to accomplish,
and I agree that it makes sense. Shawn is right that "git-mv --cached"
doesn't do what you want, since to be consistent with other tools it
wouldn't touch the working tree (though I wonder if you would be
satisfied with "git-mv --cached A B; mv A B"). I think at this point the
best way forward is to produce a patch and try to solicit comments (in
particular, which behaviors should be supported (current, --cached, or
what you are proposing) and which should be the default).
> But... that's not even the point I was going to make. I think the
> questions you should be asking are things like "Does this fit with the
> overall architecture?", "Does this or doesn't this provide power and
> flexibility to the user?", etc. Is git being made for "the 80%" that use
There seems to be a strong sentiment among the various gits that you
shouldn't build in flexibility for the sake of flexibility if there's no
good use for it. I tend to be less of that sentiment than some others
here, but I think when proposing a feature it is still worth saying "and
here's why it's useful."
> Sorry if anything I've said here sounded confrontational; that was not
> not my intent at all. I'm just raising some points I think are
> important. I don't want any flame wars...
I think you stated your position fine. And we've had enough flame wars
around here lately to last us a while.
-Peff
^ permalink raw reply
* [BUG] git-mv submodule failure
From: Yin Ping @ 2007-10-20 6:01 UTC (permalink / raw)
To: git
project
.git
file1
submoudle
.git
file2
$ cd project
$ git-mv submodule submodule1
fatal: source directory is empty, source=submodule, destination=submodule1
However, the following is ok and rename is automatically detected
$ cd project
$ mv submodule submodule1
$ git-add submodule1
$ git-commit -a
which gives in vim:
# Please enter the commit message for your changes.
# (Comment lines starting with '#' will not be included)
# On branch master
# Changes to be committed:
# (use "git reset HEAD <file>..." to unstage)
#
# renamed: submodule -> submodule1
#
--
franky
^ permalink raw reply
* Re: Proposed git mv behavioral change
From: Ari Entlich @ 2007-10-20 5:55 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
In-Reply-To: <20071019015419.GV14735@spearce.org>
On Thu, 2007-10-18 at 21:54 -0400, Shawn O. Pearce wrote:
> Line wrapping email at under 80 columns would be nice. It makes it
> easier to read the message, and more importantly, easier to quote
> a few times during discussion.
Yeah, sorry about that. As I said, the crappy webmail client I was using
was screwing up. I'm using a real email client now.
> I'm somewhat hesistant to change existing behavior, as users may
> be used to it or relying upon it within their scripts. But you
> make an excellent argument about why the current git-mv behavior
> is perhaps less than ideal.
See my response to Wincent's message for what I said about this.
One minor issue (since you talk about programming stuff below) is that,
from my cursory survey of builtin-mv.c, a slight restructuring might be
required for this to work and for the solution to satisfy my coding
standards. This would be because you'd need to change the names of
individual cache entries as opposed to removing one and adding one. This
is a minor issue though, I'm sure I can figure out how to get it to work
somehow.
> Elsewhere in git we use the --cached command line option to mean
> "only make the change in the index". For example the git-apply
> --cached option. You could start a patch that uses --cached to
> trigger the new behavior you propose and see if people are interested
> in changing the default once the feature is working and available
> for experimentation.
>From a later email...
> But I was originally *way* wrong to propose --cached for this usage
> in git-mv. --cached means "apply *ONLY* to the index" and "do *NOT*
> touch the working tree". Here we want to touch the working tree
> in the sense of moving the file. So --cached is not the correct
> option name.
Hmm yeah, that did occur to me after I read your original message...
> --index is used in Git for places were we update *both* the index
> and the working directory (git-apply --index). So actually I should
> have suggested "git-mv --index". Whoops.
Alright then, I don't know about that particular convention. If this
behavior can't be made default, git mv --index should activate it? I
there anything else that might be more descriptive?
> > I'm willing to look into what changes would need to be made to the
> > code for this change to happen; I'm not asking for someone to do
> > all the work for me. :)
> >
> > So... Yeah. I'd like to know what people think about this before
> > I put a significant amount of effort into it. After all, we know
> > how lazy programmers are... :)
>
> See builtin-mv.c around l.264-283. This is where we are removing
> the old names from the index (in memory) and inserting the new
> names. Instead of calling add_file_to_cache() you would want
> to use something like add_cacheinfo() in builtin-update-index.c,
> specifying the old sha1, ce_flags and ce_mode.
Might it be possible to simply get the struct cache_entry for the file
which we want to rename, change its name property (would this involve
xreallocing it?), and change the ce_namelen field of ce_flags?
In any case, I think the ce_flags would need to be changed to reflect
the new name length. Also, it seems that the old ce_mtime, ce_dev,
ce_ino, ce_uid, ce_gid, and ce_size could be used too... ce_ctime would
need to be updated...
> I'm sure Junio could probably give you a better starting point
> than I can, as he's more familiar with this sort of code, but that
> should still get you looking in the right direction and maybe get
> a working implementation together that you can share for discussion.
Yeah, I'd appreciate any help I can get. The people on #git were
invoking the "the code is the documentation" ideology. :)
So... Ping Junio...? I actually haven't seen any messages from him since
I subscribed to this list (I've only gotten about 300 messages so far,
but I'd expect to see a lot from him as he's the maintainer...). Is he
away at the moment or something?
Thanks,
Ari
^ permalink raw reply
* Re: Proposed git mv behavioral change
From: Ari Entlich @ 2007-10-20 5:55 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: Michael Witten, Jeff King, git
On Fri, 2007-10-19 at 13:33 +0200, Wincent Colaiuta wrote:
> El 19/10/2007, a las 4:29, Michael Witten escribió:
> > Ah. Basically my 'pseudo-code' is correct, but redundant.
>
> If I understood the original poster's proposal then I don't think your
> code does what he asked for:
>
> > What you want to happen is the following:
> >
> > git show HEAD:A.txt > path/B.txt
> > git add path/B.txt
> > mv A.txt B.txt
> > git rm A.txt
> >
> > Is this correct?
>
> Here you're copying the content of A.txt as it was in the last (HEAD)
> commit, but from what the poster said he wants the content of A.txt as
> it is staged in the index (that is, there may be staged but uncomitted
> changes).
You took the words right out of my mouth! :)
Yeah, I noticed the problem with using HEAD, but the other problem is
that this would change the contents of the file in the working directory
file, which I don't want. Thus, putting the contents of the file as it
is in the index into the working directory wouldn't be correct either.
In addition, I'm not quite sure where that "mv A.txt B.txt" came from,
since we're supposed to be moving A.txt to path/B.txt...
> > Better:
> >
> > > mv A.txt path/B.txt
> > > Point the index entry for A.txt to path/B.txt
>
> Yes, that is basically what he was asking for, as I read it.
Yep! I was going to respond to your (Michael's) original message saying
exactly that. :)
> El 19/10/2007, a las 5:47, Jeff King escribió:
> > Hrm. So you _do_ want to do an index-only move of A to B, in which
> > case the suggestion of a "git-mv --cached" seems sensible. Though
> > I'm curious why you want that.
>
> I agree that git-stash can be used in this workflow but I can also
> imagine cases where the proposed "git-mv --cached" might be a bit
> nicer.
As Shawn said, --cached wouldn't be entirely accurate as the file in the
working directory is being moved as well.
git-stash has been suggested to me numerous times, but I really feel
that there's no need to use it in this case - if the git mv command gave
adequate control to the user, it would be unnecessary.
> I'm thinking of occasions where you just want to do something
> like:
>
> git mv --cached foo bar
> git add --interactive bar
I think it would be the other way around, since the only time this
change would effect anything is when there are changes still waiting to
be staged.
Are you talking about REALLY only changing the index? I can't think of
why you'd want to do this either... After all, wouldn't there be no bar
file to do git add --interactive on? In addition, I don't think giving
--interactive a filename is meaningful...
> I'm not sure the proposed "--cached" switch should ever be the
> default -- would need to ponder that one -- but I do think the switch
> would be a nice addition.
Yeah, that is one thing I was wondering. It would break compatibility,
but would it be enough to put a note about that in the announcements for
a release? Could you make the change at a release when the interface
isn't guaranteed to be the same, or is this practice only done with
libraries?
It might be interesting to do some sort of survey of whether people
depend on this behavior. It seems pretty inconsistent with how git works
otherwise, and I'd be surprised if a lot of people expect it (kinda like
the Spanish Inquisition :-P).
Thanks,
Ari
^ permalink raw reply
* Re: Proposed git mv behavioral change
From: Ari Entlich @ 2007-10-20 5:55 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Witten, git
In-Reply-To: <20071019034704.GB11095@coredump.intra.peff.net>
On Thu, 2007-10-18 at 23:47 -0400, Jeff King wrote:
> On Thu, Oct 18, 2007 at 11:40:47PM -0400, Michael Witten wrote:
>
> > Anyway, succinctly:
> >
> >> What you want to happen is the following:
> >>
> >> git show HEAD:A.txt > path/B.txt
> >> git add path/B.txt
> >> mv A.txt B.txt
> >> git rm A.txt
The one above is not only "worse" than the one below, it's wrong. See
Wincent's message and my response to it.
> >
> > Better:
> >
> >> mv A.txt path/B.txt
> >> Point the index entry for A.txt to path/B.txt
> >
> > I hope that's right.
>
> Hrm. So you _do_ want to do an index-only move of A to B, in which case
> the suggestion of a "git-mv --cached" seems sensible. Though I'm curious
> why you want that.
Well, I also want to move the working directory file so that the index
and the working directory still match up and so that their differences
are preserved. Therefore, --cached isn't quite right (see Shawn's
messages and my response to them).
> The only workflow I can think of is:
> 1. you modify a.c
> 2. your boss comes in and tells you to make some unrelated change,
> which involves moving a.c to b.c
> 3. You don't want to commit your changes, so you git-mv in the index
> only without involving your dirty working tree file.
> 4. You commit the index (which doesn't have your changes from (1)
>
> I think that is sort of a bogus workflow, though, since you will never
> have actually compiled or tested the changes in (2). You are much better
> to git-stash your current work, fulfill the boss's request, then
> unstash.
Hmm, that's an interesting point. Are you talking about situations in
which your changes after (1) leave the program in an uncompilable state?
In this situation, I could imagine git stash being a better solution. In
my situation, however, the project was compilable. The reason I didn't
want to commit the changes was because I wasn't entirely satisfied with
how I implemented the change I was trying to make; I thought there might
be a better way to do it. I wanted to get some comparatively
straightforward changes out of the way before I tackled it.
On some level, the reason I want this change isn't entirely because it's
preventing me from doing something I want to do. I've come to really
like git and how the index can be used to separate all of the changes
you have made from the changes that you want to commit in individual
commits. I've come to really like how much access to the lower-level
interfaces git provides. It seems to me that people should support this
simply because it provides more power to the user. If the user wants
their unstaged changes to be staged, they can explicitly do it with git
update-index or some such command.
If the issue is whether this would be the default, that's a completely
separate issue, and one which I don't really have a strong opinion on.
If it were up to me, I'd probably choose to make it the default, but
this might break some people's expectations. I want to keep git
accessible to as many people as possible, not force it into the mold
that I feel is the only correct one. If the functionality is available
but only used when you want it, what's the harm in including it?
I don't really see why you're analyzing the situations in which this
would be used. I think it should be obvious from my descriptions of my
situation that there is an application for this functionality. Unless
you're going to start calling my process bogus (as you did to the one
involving a boss), which I would not appreciate, I'd argue that the same
mentality should be applied to this that is applied to kernel drivers -
if it's useful to even only one person, it should be supported. This
functionality of git wouldn't even be that extreme an example, though -
there's no telling whether somebody might be in a situation like mine
and find that they want to do what I want to do.
But... that's not even the point I was going to make. I think the
questions you should be asking are things like "Does this fit with the
overall architecture?", "Does this or doesn't this provide power and
flexibility to the user?", etc. Is git being made for "the 80%" that use
Subversion and need to be supported by a cushy GUI, or "the 20%" that
thrive on power and are in touch with future trends?
So... yeah. Sorry, I know this got to sounding incredibly
propagandistic, but this is what propaganda's for, right? :)
Sorry if anything I've said here sounded confrontational; that was not
not my intent at all. I'm just raising some points I think are
important. I don't want any flame wars...
On Thu, 2007-10-18 at 23:58 -0400, Jeff King wrote:
> On Thu, Oct 18, 2007 at 11:53:05PM -0400, Michael Witten wrote:
> > It's not unreasonable.
>
> I guess. I really think git-stash is your friend here. But you can
> still do step (3) with git-update-index (I'll leave the exact details
> as an exercise for the reader).
See my response to Wincent's message for my opinion on using git stash.
I seems to me that using it is more of a workaround than a fix.
Thanks,
Ari
^ permalink raw reply
* Re: git on afs
From: Shawn O. Pearce @ 2007-10-20 5:29 UTC (permalink / raw)
To: Todd T. Fries; +Cc: git, Brandon Casey
In-Reply-To: <200710190742.08174.todd@fries.net>
"Todd T. Fries" <todd@fries.net> wrote:
> It should not matter, but I'm using arla's afs client on OpenBSD; the errno
> is 17 (EEXIST), the very errno that bypasses the coda hack's rename():
...
> I can assure you that the 2nd argument to link does not exist ;-)
...
> The only downside is that either on coda or if the file already exists, it
> will try a spurrous rename(), in which case it will fail with EEXIST again,
> so I hope this is not a big negative.
Actually there is a really big downside. rename() is defined to
unlink the destination if it already exists, so you'll never get
EEXIST from a rename() call.
This means that an existing (known to be good) object can be
overwritten as a result of a rename with another copy of the object.
We've never really had that behavior as part of our security model
has been to always trust what is already in the repository and
refuse to replace something we already have.
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2004,8 +2004,13 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
> *
> * When this succeeds, we just return 0. We have nothing
> * left to unlink.
> + *
> + * AFS hack - afs is similar to coda, but inconveniently
> + * set errno to EEXIST, so call rename() if the link()
> + * above fails unconditionally. Small bit of extra work
> + * so afs functions properly.
> */
> - if (ret && ret != EEXIST) {
> + if (ret) {
> if (!rename(tmpfile, filename))
> return 0;
> ret = errno;
This is very unfortunate. There's no way to tell that the file
already exists. This whole AFS link() returning EEXIST is sort of
like the unlink() call on Solaris UFS working on directories as root
and leaving corrupted filesystems. At some point the application
just cannot be reasonably expected to work on a system that acts
this insane.
I think I would rather change sha1_file.c to write temporary files
into the destination directory, rather than one level up and try to
hardlink them into position. At least there we can rely on hardlinks
within Coda and AFS, and only need this rename special case for FAT.
--
Shawn.
^ permalink raw reply
* Re: [RFC/PATCH] git-fetch: mega-terse fetch output
From: Jeff King @ 2007-10-20 5:00 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Johannes Sixt, Theodore Tso, Santi Béjar, Shawn O. Pearce,
David Symonds, git
In-Reply-To: <alpine.LFD.0.9999.0710191009330.19446@xanadu.home>
On Fri, Oct 19, 2007 at 10:14:59AM -0400, Nicolas Pitre wrote:
> > ==> git://repo.or.cz/git/spearce.git
> > * (new) gitk -> spearce/gitk
> > * 1aa3d01..e7187e4 maint -> spearce/maint
> > * de61e42..7840ce6 master -> spearce/master
> > * 895be02..2fe5433 next -> spearce/next
> > + 89fa332...1e4c517 pu -> spearce/pu
> > * (new) todo -> spearce/todo
>
> Actually I think this is the best format so far: one line per branch, no
> terminal width issue (long branch names are simply wrapped), the
> old..new info is there also with the single character marker to quickly
> notice the type of update.
Technically speaking, the hash IDs can be up to 80 characters long,
since they are meant to be unique abbreviations. But in practice, I
think leaving enough space for 10 + '...' + 10 should accomodate just
about any project (IIRC, the kernel's longest non-unique is around 9).
-Peff
^ permalink raw reply
* Re: [PATCH] send-pack: respect '+' on wildcard refspecs
From: Shawn O. Pearce @ 2007-10-20 4:57 UTC (permalink / raw)
To: Jeff King; +Cc: Dan McGee, git
In-Reply-To: <20071020042257.GA26755@coredump.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> On Fri, Oct 19, 2007 at 08:00:37PM -0500, Dan McGee wrote:
>
> > Turns out I didn't have GIT_EXEC_PATH set up right. Once I do that,
> > everything seems to work just fine.
> >
> > Thanks for looking into this Jeff, and git-bisect just won me over. It
> > made easy work of finding the commit that broke this.
>
> Huzzah, success! Shawn, this should probably go on 'maint'. Although it
> is probably not high priority (it has been broken since May; I think
> wildcard push refspecs must not be that common), it is a fairly trivial
> fix that shouldn't impact anyone else.
Yea, its already queued on maint. Both the patch and the
tests are obviously correct. Its broken without the patch.
Its documented/expected behavior fixed by the patch. IMHO it
belongs in maint.
--
Shawn.
^ permalink raw reply
* Re: gitk patch collection pull request
From: Linus Torvalds @ 2007-10-20 4:51 UTC (permalink / raw)
To: Paul Mackerras; +Cc: Shawn O. Pearce, git
In-Reply-To: <18201.34779.27836.531742@cargo.ozlabs.ibm.com>
On Sat, 20 Oct 2007, Paul Mackerras wrote:
>
> Do you mean that when you have a file limit, the diff window should
> just show the diffs for those files, not any other files the commit
> might have modified?
Yes. The same way "git log -p" works by default.
With perhaps a checkbox to toggle the "--full-diff" behaviour.
> That would be easy enough to implement in gitk.
Well, the "--merged" case is slightly trickier, since git will figure out
the pathnames on its own (it limits pathnames to the intersection of the
names you give one the command line *and* the list of unmerged files, ie
the "filter" becomes "git ls-files -u [pathspec]".
But goodie. I look forward to it ;)
Linus
^ 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