[PATCH] "not uptodate" changed to "has local changes"

[PATCH] "not uptodate" changed to "has local changes"

[Tim Harper]

When doing a merge, the message says "file.txt: needs update", or "file.txt: not uptodate, cannot merge".   While internally 'uptodate' makes sense, from the outside it's a mystery.

This patch will make git a little more human friendly, reporting "file.txt: has local changes".
---
 read-cache.c   |    2 +-
 unpack-trees.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index a92b25b..e890b27 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -999,7 +999,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
 			}
 			if (quiet)
 				continue;
-			printf("%s: needs update\n", ce->name);
+			printf("%s: has local changes\n", ce->name);
 			has_errors = 1;
 			continue;
 		}
diff --git a/unpack-trees.c b/unpack-trees.c
index a59f475..1d67e08 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -427,7 +427,7 @@ static int verify_uptodate(struct cache_entry *ce,
 	if (errno == ENOENT)
 		return 0;
 	return o->gently ? -1 :
-		error("Entry '%s' not uptodate. Cannot merge.", ce->name);
+		error("Entry '%s' has local changes. Cannot merge.", ce->name);
 }
 
 static void invalidate_ce_path(struct cache_entry *ce, struct unpack_trees_options *o)
-- 
1.5.5.1

Re: [PATCH] "not uptodate" changed to "has local changes"

[Mike Ralphson]

2008/5/3 Tim Harper <timcharper@gmail.com>:
> When doing a merge, the message says "file.txt: needs update", or "file.txt: not uptodate, cannot merge".   While internally 'uptodate' makes sense, from the outside it's a mystery.
>
>  This patch will make git a little more human friendly, reporting "file.txt: has local changes".

Documentation/git-checkout.txt should also change in this case,
otherwise users will see different output to that described and
possibly get confused if following along with the examples.

Mike

Re: [PATCH] "not uptodate" changed to "has local changes"

[André Goddard Rosa]

>>  This patch will make git a little more human friendly, reporting "file.txt: has local changes".
>
> Documentation/git-checkout.txt should also change in this case,
> otherwise users will see different output to that described and
> possibly get confused if following along with the examples.
>

I like the idea too.

---
[PATCH] "not uptodate" changed to "has local changes"

Use more straightforward message for regular user.

Signed-off-by: Andre Goddard Rosa <andre.goddard@gmail.com>

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index a644173..624dea6 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -168,7 +168,7 @@ the above checkout would fail like this:
 +
 ------------
 $ git checkout mytopic
-fatal: Entry 'frotz' not uptodate. Cannot merge.
+fatal: Entry 'frotz' has local changes. Cannot merge.
 ------------
 +
 You can give the `-m` flag to the command, which would try a

Re: [PATCH] "not uptodate" changed to "has local changes"

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 474 bytes --]

Hi,

On Thu, 15 May 2008, André Goddard Rosa wrote:

> >>  This patch will make git a little more human friendly, reporting "file.txt: has local changes".
> >
> > Documentation/git-checkout.txt should also change in this case,
> > otherwise users will see different output to that described and
> > possibly get confused if following along with the examples.
> >
> 
> I like the idea too.

No comment on the concern that it might break people's scripts?  None?

Ciao,
Dscho

Re: [PATCH] "not uptodate" changed to "has local changes"

[Holger Schurig]

> No comment on the concern that it might break people's
> scripts?  None?

Scripts should look for exit values :-)

Re: [PATCH] "not uptodate" changed to "has local changes"

[Francis Moreau]

On Fri, May 16, 2008 at 1:02 PM, Holger Schurig
<hs4233@mail.mn-solutions.de> wrote:
>> No comment on the concern that it might break people's
>> scripts?  None?
>
> Scripts should look for exit values :-)

except that they're not documented ;)

-- 
Francis

Re: [PATCH] "not uptodate" changed to "has local changes"

[Johannes Schindelin]

Hi,

On Fri, 16 May 2008, Holger Schurig wrote:

> > No comment on the concern that it might break people's
> > scripts?  None?
> 
> Scripts should look for exit values :-)

Clever.  And which exit value would you exactly check to see _which_ file 
needs an update?

Hth,
Dscho

Re: [PATCH] "not uptodate" changed to "has local changes"

[Kevin Ballard]

On May 16, 2008, at 6:25 AM, Johannes Schindelin wrote:

> On Thu, 15 May 2008, André Goddard Rosa wrote:
>
>>>> This patch will make git a little more human friendly, reporting  
>>>> "file.txt: has local changes".
>>>
>>> Documentation/git-checkout.txt should also change in this case,
>>> otherwise users will see different output to that described and
>>> possibly get confused if following along with the examples.
>>>
>>
>> I like the idea too.
>
> No comment on the concern that it might break people's scripts?  None?


How about an ugly hack? Look to see if stdout is a tty, if so spit out  
the more human-readable version, otherwise spit out the old version >:-)

-Kevin

-- 
Kevin Ballard
http://kevin.sb.org
kevin@sb.org
http://www.tildesoft.com

Re: [PATCH] "not uptodate" changed to "has local changes"

[André Goddard Rosa]

On Fri, May 16, 2008 at 2:12 PM, Kevin Ballard <kevin@sb.org> wrote:
> On May 16, 2008, at 6:25 AM, Johannes Schindelin wrote:
>
>> On Thu, 15 May 2008, André Goddard Rosa wrote:
>>
>>>>> This patch will make git a little more human friendly, reporting
>>>>> "file.txt: has local changes".
>>>>
>>>> Documentation/git-checkout.txt should also change in this case,
>>>> otherwise users will see different output to that described and
>>>> possibly get confused if following along with the examples.
>>>>
>>>
>>> I like the idea too.
>>
>> No comment on the concern that it might break people's scripts?  None?
>
>
> How about an ugly hack? Look to see if stdout is a tty, if so spit out the
> more human-readable version, otherwise spit out the old version >:-)

Is this user interface set on stone? I think we should reserve the
right to improve always.

I would deprecate the current message, but I think that most users
cannot find so much of a sense in the former message,
although the script developer can easily change his scripts to search
for ´cannot merge´ instead.

Do you have a better idea?

---
[PATCH] "not uptodate" changed to "has local changes"

Use more straightforward message for regular user.

Signed-off-by: Andre Goddard Rosa <andre.goddard@gmail.com>

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index a644173..624dea6 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -168,7 +168,7 @@ the above checkout would fail like this:
 +
 ------------
 $ git checkout mytopic
-fatal: Entry 'frotz' not uptodate. Cannot merge.
+fatal: Entry 'frotz' not uptodate, it has local changes. Cannot merge.
 ------------
 +
 You can give the `-m` flag to the command, which would try a

Re: [PATCH] "not uptodate" changed to "has local changes"

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1302 bytes --]

Hi,

On Sat, 17 May 2008, André Goddard Rosa wrote:

> On Fri, May 16, 2008 at 2:12 PM, Kevin Ballard <kevin@sb.org> wrote:
> > On May 16, 2008, at 6:25 AM, Johannes Schindelin wrote:
> >
> >> On Thu, 15 May 2008, André Goddard Rosa wrote:
> >>
> >>>>> This patch will make git a little more human friendly, reporting
> >>>>> "file.txt: has local changes".
> >>>>
> >>>> Documentation/git-checkout.txt should also change in this case,
> >>>> otherwise users will see different output to that described and
> >>>> possibly get confused if following along with the examples.
> >>>>
> >>>
> >>> I like the idea too.
> >>
> >> No comment on the concern that it might break people's scripts?  None?
> >
> >
> > How about an ugly hack? Look to see if stdout is a tty, if so spit out the
> > more human-readable version, otherwise spit out the old version >:-)
> 
> Is this user interface set on stone? I think we should reserve the right 
> to improve always.

Umm.

As has been mentioned, this is not a "user interface".  The message you 
are seeing comes from a _plumbing_ program, i.e. something _not_ meant for 
human consumption.

I still think that it might be better to add a command line option with a 
custom message, because that would _not_ break backwards-compatibility.

Thankyouverymuch,
Dscho

Re: [PATCH] "not uptodate" changed to "has local changes"

[Steven Walter]

On Sat, May 17, 2008 at 6:04 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>> Is this user interface set on stone? I think we should reserve the right
>> to improve always.
>
> Umm.
>
> As has been mentioned, this is not a "user interface".  The message you
> are seeing comes from a _plumbing_ program, i.e. something _not_ meant for
> human consumption.
>
> I still think that it might be better to add a command line option with a
> custom message, because that would _not_ break backwards-compatibility.

With this dedication to backwards-compatibility, we'll be at Windows
Vista quality in no time.
-- 
-Steven Walter <stevenrwalter@gmail.com>

Re: [PATCH] "not uptodate" changed to "has local changes"

[Sverre Rabbelier]

On Sat, May 17, 2008 at 4:44 PM, Steven Walter <stevenrwalter@gmail.com> wrote:
> With this dedication to backwards-compatibility, we'll be at Windows
> Vista quality in no time.

I very much agree here, given the nature of scripts (that is, being
very easy to update), I think we should try not to be too strict in
backwards-compatibility or we'll lose the flexibility that is very
much needed when developing a Good Product (tm) As long as such
compatibility breaking changes are marked (in BIG LETTERS) in the
changelog/release notes I think that would be a 'sacrifice' we should
consider making.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] "not uptodate" changed to "has local changes"

[Johannes Schindelin]

Hi,

On Sat, 17 May 2008, Sverre Rabbelier wrote:

> On Sat, May 17, 2008 at 4:44 PM, Steven Walter <stevenrwalter@gmail.com> 
> wrote:
>
> > With this dedication to backwards-compatibility, we'll be at Windows 
> > Vista quality in no time.

That is silly at best, especially given that Vista is _not_ 
backwards-compatible.  Not to mention that it is not forkable, because it 
is not Open Source.

> I very much agree here, given the nature of scripts (that is, being very 
> easy to update), I think we should try not to be too strict in 
> backwards-compatibility or we'll lose the flexibility that is very much 
> needed when developing a Good Product (tm) As long as such compatibility 
> breaking changes are marked (in BIG LETTERS) in the changelog/release 
> notes I think that would be a 'sacrifice' we should consider making.

Had you (one of our GSoc students) not replied, I would not even have 
bothered to say anything.

But I strongly disagree with the notion that it is okay to fsck with 
old-timers (who would be harmed by breaking backwards-incompatibility, 
and nobody else), especially given that it is mostly old-timers who turned 
Git into the Good Product(tm) it is.

Ciao,
Dscho

Re: [PATCH] "not uptodate" changed to "has local changes"

[Sverre Rabbelier]

On Sat, May 17, 2008 at 8:44 PM, Johannes Schindelin > But I strongly
disagree with the notion that it is okay to fsck with
> old-timers (who would be harmed by breaking backwards-incompatibility,
> and nobody else), especially given that it is mostly old-timers who turned
> Git into the Good Product(tm) it is.

But those old-timers can be updated can't they? Also, shouldn't we
have tests to see if they 'break' because of changes?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] "not uptodate" changed to "has local changes"

[Sverre Rabbelier]

On Sun, May 18, 2008 at 12:31 PM, しらいしななこ <nanako3@bluebottle.com> wrote:
> What advantage are you bringing to the table for them to be worth bothering to update, other than "if they update then they can get their scripts working again after you break them by rewording messages for no good reason"? Why do you punish the old-timers for using the well established API?

My reason was that (perhaps not as much in this case, but I meant to
speak in general) the message is confusing/not clear enough.

> Did you study Junio's patch before you responded to his message? The point of the suggestion was that he reworded the message given by git-checkout that is a porcelain command without breaking output from read-tree that is a plumbing command. In other words, you can improve output from porcelain commands without making unnecessary changes to plumbing.

I hadn't read his e-mail when I was replying, my Internet has been
playing up lately (same for about half of the Netherlands due to a
problem at one of our biggest ISPs). I did reply to it though:
http://thread.gmane.org/gmane.comp.version-control.git/81100

-- 
Cheers,

Sverre Rabbelier

Re* [PATCH] "not uptodate" changed to "has local changes"

[Junio C Hamano]

"Sverre Rabbelier" <alturin@gmail.com> writes:

> On Sat, May 17, 2008 at 4:44 PM, Steven Walter <stevenrwalter@gmail.com> wrote:
>> With this dedication to backwards-compatibility, we'll be at Windows
>> Vista quality in no time.
>
> I very much agree here, given the nature of scripts (that is, being
> very easy to update), I think we should try not to be too strict in
> backwards-compatibility or we'll lose the flexibility that is very
> much needed when developing a Good Product (tm) As long as such
> compatibility breaking changes are marked (in BIG LETTERS) in the
> changelog/release notes I think that would be a 'sacrifice' we should
> consider making.

Don't feed the troll by responding to a cheap shot.

The plumbing output is sacred as it is an API.  We _could_ change it if it
is broken in such a way that it cannot convey necessary information fully,
but we just do not _reword_ for the sake of rewording.  If somebody does
not like it, s/he is complaining too late.  S/he should have been here in
early May 2005 and make the language used by the API closer to what humans
read.  S/he wasn't here.  Too bad, and it is too late.  

And people who complain should look at a bigger picture.  Look at what was
suggested by one of them and think for five seconds:

     $ git checkout mytopic
    -fatal: Entry 'frotz' not uptodate. Cannot merge.
    +fatal: Entry 'frotz' has local changes. Cannot merge.

If you do not see something wrong with this output, your brain has already
been rotten with use of git for too long a time.  Nobody asked us to
"merge" but why are we talking about "Cannot merge"?

Try a different approach along this patch instead.

    $ git-checkout pu
    error: You have local changes to 'Makefile'; cannot switch branches.

There are other places that ask unpack_trees() to n-way merge, detect
issues _and_ let it issue error message on its own, which people who
complained in this thread can identify and improve, but I did this as a
demonstration and replaced only one message.

Yes I know about C99 structure initializers.  I'd love to use them but we
try to be nice to compilers without it.

 builtin-checkout.c |    2 ++
 unpack-trees.c     |   47 +++++++++++++++++++++++++++++++++--------------
 unpack-trees.h     |    9 +++++++++
 3 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 10ec137..83da7ca 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -236,6 +236,8 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.src_index = &the_index;
 		topts.dst_index = &the_index;
 
+		topts.msgs.not_uptodate_file = "You have local changes to '%s'; cannot switch branches.";
+
 		refresh_cache(REFRESH_QUIET);
 
 		if (unmerged_cache()) {
diff --git a/unpack-trees.c b/unpack-trees.c
index 1ab28fd..bec12dc 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -8,6 +8,28 @@
 #include "progress.h"
 #include "refs.h"
 
+static struct unpack_trees_error_msgs unpack_default_errors = {
+	/* would_overwrite */
+	"Entry '%s' would be overwritten by merge. Cannot merge.",
+
+	/* not_uptodate_file */ 
+	"Entry '%s' not uptodate. Cannot merge.",
+
+	/* not_uptodate_dir */
+	"Updating '%s' would lose untracked files in it",
+
+	/* would_lose_untracked */
+	"Untracked working tree file '%s' would be %s by merge.",
+
+	/* bind_overlap */
+	"Entry '%s' overlaps with '%s'.  Cannot bind.",
+};
+
+#define ERRORMSG(o,fld) \
+	( ((o) && (o)->msgs.fld) \
+	? ((o)->msgs.fld) \
+	: (unpack_default_errors.fld) )
+
 static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 	unsigned int set, unsigned int clear)
 {
@@ -383,10 +405,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 
 /* Here come the merge functions */
 
-static int reject_merge(struct cache_entry *ce)
+static int reject_merge(struct cache_entry *ce, struct unpack_trees_options *o)
 {
-	return error("Entry '%s' would be overwritten by merge. Cannot merge.",
-		     ce->name);
+	return error(ERRORMSG(o, would_overwrite), ce->name);
 }
 
 static int same(struct cache_entry *a, struct cache_entry *b)
@@ -430,7 +451,7 @@ static int verify_uptodate(struct cache_entry *ce,
 	if (errno == ENOENT)
 		return 0;
 	return o->gently ? -1 :
-		error("Entry '%s' not uptodate. Cannot merge.", ce->name);
+		error(ERRORMSG(o, not_uptodate_file), ce->name);
 }
 
 static void invalidate_ce_path(struct cache_entry *ce, struct unpack_trees_options *o)
@@ -517,8 +538,7 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,
 	i = read_directory(&d, ce->name, pathbuf, namelen+1, NULL);
 	if (i)
 		return o->gently ? -1 :
-			error("Updating '%s' would lose untracked files in it",
-			      ce->name);
+			error(ERRORMSG(o, not_uptodate_dir), ce->name);
 	free(pathbuf);
 	return cnt;
 }
@@ -618,8 +638,7 @@ static int verify_absent(struct cache_entry *ce, const char *action,
 		}
 
 		return o->gently ? -1 :
-			error("Untracked working tree file '%s' "
-			      "would be %s by merge.", ce->name, action);
+			error(ERRORMSG(o, would_lose_untracked), ce->name, action);
 	}
 	return 0;
 }
@@ -751,7 +770,7 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
 	/* #14, #14ALT, #2ALT */
 	if (remote && !df_conflict_head && head_match && !remote_match) {
 		if (index && !same(index, remote) && !same(index, head))
-			return o->gently ? -1 : reject_merge(index);
+			return o->gently ? -1 : reject_merge(index, o);
 		return merged_entry(remote, index, o);
 	}
 	/*
@@ -759,7 +778,7 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
 	 * make sure that it matches head.
 	 */
 	if (index && !same(index, head))
-		return o->gently ? -1 : reject_merge(index);
+		return o->gently ? -1 : reject_merge(index, o);
 
 	if (head) {
 		/* #5ALT, #15 */
@@ -901,11 +920,11 @@ int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)
 		else {
 			/* all other failures */
 			if (oldtree)
-				return o->gently ? -1 : reject_merge(oldtree);
+				return o->gently ? -1 : reject_merge(oldtree, o);
 			if (current)
-				return o->gently ? -1 : reject_merge(current);
+				return o->gently ? -1 : reject_merge(current, o);
 			if (newtree)
-				return o->gently ? -1 : reject_merge(newtree);
+				return o->gently ? -1 : reject_merge(newtree, o);
 			return -1;
 		}
 	}
@@ -931,7 +950,7 @@ int bind_merge(struct cache_entry **src,
 			     o->merge_size);
 	if (a && old)
 		return o->gently ? -1 :
-			error("Entry '%s' overlaps with '%s'.  Cannot bind.", a->name, old->name);
+			error(ERRORMSG(o, bind_overlap), a->name, old->name);
 	if (!a)
 		return keep_entry(old, o);
 	else
diff --git a/unpack-trees.h b/unpack-trees.h
index d436d6c..94e5672 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -8,6 +8,14 @@ struct unpack_trees_options;
 typedef int (*merge_fn_t)(struct cache_entry **src,
 		struct unpack_trees_options *options);
 
+struct unpack_trees_error_msgs {
+	const char *would_overwrite;
+	const char *not_uptodate_file;
+	const char *not_uptodate_dir;
+	const char *would_lose_untracked;
+	const char *bind_overlap;
+};
+
 struct unpack_trees_options {
 	unsigned int reset:1,
 		     merge:1,
@@ -23,6 +31,7 @@ struct unpack_trees_options {
 	int pos;
 	struct dir_struct *dir;
 	merge_fn_t fn;
+	struct unpack_trees_error_msgs msgs;
 
 	int head_idx;
 	int merge_size;

Re: Re* [PATCH] "not uptodate" changed to "has local changes"

[Sverre Rabbelier]

On Sat, May 17, 2008 at 9:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Don't feed the troll by responding to a cheap shot.

Yes, a cheap shot, but I think one trying to make a point.

> The plumbing output is sacred as it is an API.  We _could_ change it if it
> is broken in such a way that it cannot convey necessary information fully,
> but we just do not _reword_ for the sake of rewording.  If somebody does
> not like it, s/he is complaining too late.  S/he should have been here in
> early May 2005 and make the language used by the API closer to what humans
> read.  S/he wasn't here.  Too bad, and it is too late.

Mhh.. I guess I didn't realize how strongly git still is "many
commands that are good at what they do" that together form a coherent
entity.

> If you do not see something wrong with this output, your brain has already
> been rotten with use of git for too long a time.  Nobody asked us to
> "merge" but why are we talking about "Cannot merge"?

Very good point, perhaps we should consider double-checking them all
and improving them for one of the next 1.x releases.

> Try a different approach along this patch instead.
>
>    $ git-checkout pu
>    error: You have local changes to 'Makefile'; cannot switch branches.
>
> There are other places that ask unpack_trees() to n-way merge, detect
> issues _and_ let it issue error message on its own, which people who
> complained in this thread can identify and improve, but I did this as a
> demonstration and replaced only one message.

The patch looks like a step in the right direction, but if there is
interest in improving the error messaging system why not do it right
and make it generic. Instead of each file reinventing the wheel create
a more generic system that uses a configuration file (I think this was
suggested earlier). If we choose something like that, perhaps it would
be nice to add a 'scripting' mode, which provides with a CLI-like
messages instead of the usual human-readable ones (and thus less easy
to parse).

<snip patch>

-- 
Cheers,

Sverre Rabbelier

Re: Re* [PATCH] "not uptodate" changed to "has local changes"

[Wincent Colaiuta]

El 17/5/2008, a las 21:03, Junio C Hamano escribió:
> +	/* not_uptodate_file */
> +	"Entry '%s' not uptodate. Cannot merge.",


Minor nit, "uptodate" is not a word. Should be either "up-to-date" or  
"up to date"; most dictionaries list both.

Cheers,
Wincent

Re: Re* [PATCH] "not uptodate" changed to "has local changes"

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> El 17/5/2008, a las 21:03, Junio C Hamano escribió:
>> +	/* not_uptodate_file */
>> +	"Entry '%s' not uptodate. Cannot merge.",
>
>
> Minor nit, "uptodate" is not a word. Should be either "up-to-date" or
> "up to date"; most dictionaries list both.

Why does *everybody* keep missing the whole point of this patch?

Grumble.

Re: Re* [PATCH] "not uptodate" changed to "has local changes"

[Sverre Rabbelier]

On Mon, May 19, 2008 at 7:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Why does *everybody* keep missing the whole point of this patch?

Isn't it to make it possible to change the error messages at porcelain
level while allowing the plumbing to remain backward compatibility?

-- 
Cheers,

Sverre Rabbelier

Re: Re* [PATCH] "not uptodate" changed to "has local changes"

[Daniel Barkalow]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 893 bytes --]

On Mon, 19 May 2008, Junio C Hamano wrote:

> Wincent Colaiuta <win@wincent.com> writes:
> 
> > El 17/5/2008, a las 21:03, Junio C Hamano escribió:
> >> +	/* not_uptodate_file */
> >> +	"Entry '%s' not uptodate. Cannot merge.",
> >
> >
> > Minor nit, "uptodate" is not a word. Should be either "up-to-date" or
> > "up to date"; most dictionaries list both.
> 
> Why does *everybody* keep missing the whole point of this patch?

That section needs a comment stating that it's the scripting API, not just 
an arbitrary set of messages. For that matter, maybe those shouldn't be 
the default set, but an alternate set used (as a group) by plumbing 
programs; I don't think it's too likely that there will be a whole lot of 
new plumbing programs, and new porcelain programs that don't specify 
anything probably ought to get something more generic.

	-Daniel
*This .sig left intentionally blank*

Re: Re* [PATCH] "not uptodate" changed to "has local changes"

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Mon, 19 May 2008, Junio C Hamano wrote:
>
>> Why does *everybody* keep missing the whole point of this patch?
>
> That section needs a comment stating that it's the scripting API, not just 
> an arbitrary set of messages.

Yeah, that is a very good explanation.  Thanks for a constructive
suggestion for improvements.

Here is an incremental on top of the one I sent out, in case people want
to improve on it.

 unpack-trees.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index da3bdc8..0de5a31 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -8,7 +8,15 @@
 #include "progress.h"
 #include "refs.h"
 
-static struct unpack_trees_error_msgs unpack_default_errors = {
+/*
+ * Error messages expected by scripts out of plumbing commands such as
+ * read-tree.  Non-scripted Porcelain is not required to use these messages
+ * and in fact are encouraged to reword them to better suit their particular
+ * situation better.  See how "git checkout" replaces not_uptodate_file to
+ * explain why it does not allow switching between branches when you have
+ * local changes, for example.
+ */
+static struct unpack_trees_error_msgs unpack_plumbing_errors = {
 	/* would_overwrite */
 	"Entry '%s' would be overwritten by merge. Cannot merge.",
 
@@ -28,7 +36,7 @@ static struct unpack_trees_error_msgs unpack_default_errors = {
 #define ERRORMSG(o,fld) \
 	( ((o) && (o)->msgs.fld) \
 	? ((o)->msgs.fld) \
-	: (unpack_default_errors.fld) )
+	: (unpack_plumbing_errors.fld) )
 
 static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 	unsigned int set, unsigned int clear)

Re: [PATCH] "not uptodate" changed to "has local changes"

[Wincent Colaiuta]

El 17/5/2008, a las 12:04, Johannes Schindelin escribió:
> Hi,
>
> On Sat, 17 May 2008, André Goddard Rosa wrote:
>
>> Is this user interface set on stone? I think we should reserve the  
>> right
>> to improve always.
>
> Umm.
>
> As has been mentioned, this is not a "user interface".  The message  
> you
> are seeing comes from a _plumbing_ program, i.e. something _not_  
> meant for
> human consumption.

That would indicate a problem, if stuff not intended for human  
consumption is being dished up for exactly that: human consumption.

> I still think that it might be better to add a command line option  
> with a
> custom message, because that would _not_ break backwards- 
> compatibility.

Sounds like clutter to me. I'd instead favor just holding back this  
patch until 1.6, when (minor) "compatibility breaking" changes would  
be acceptable.

Wincent

Re: [PATCH] "not uptodate" changed to "has local changes"

[Matthieu Moy]

Wincent Colaiuta <win@wincent.com> writes:

> That would indicate a problem, if stuff not intended for human
> consumption is being dished up for exactly that: human consumption.

There have been other instances of this one, and the output of
git-update-index has been replaced by a call to "git status" in the
porcelain.

-- 
Matthieu