[PATCH 05/23] expire_reflog(): rename "ref" parameter to "refname"

2014-12-04 Thread Michael Haggerty
This is our usual convention.

Signed-off-by: Michael Haggerty 
---
 builtin/reflog.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 3e11bee..df302f4 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -349,7 +349,7 @@ static int push_tip_to_list(const char *refname, const 
unsigned char *sha1, int
return 0;
 }
 
-static int expire_reflog(const char *ref, const unsigned char *sha1, void 
*cb_data)
+static int expire_reflog(const char *refname, const unsigned char *sha1, void 
*cb_data)
 {
struct cmd_reflog_expire_cb *cmd = cb_data;
struct expire_reflog_cb cb;
@@ -365,20 +365,20 @@ static int expire_reflog(const char *ref, const unsigned 
char *sha1, void *cb_da
 * we take the lock for the ref itself to prevent it from
 * getting updated.
 */
-   lock = lock_any_ref_for_update(ref, sha1, 0, NULL);
+   lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
if (!lock)
-   return error("cannot lock ref '%s'", ref);
-   log_file = git_pathdup("logs/%s", ref);
-   if (!reflog_exists(ref))
+   return error("cannot lock ref '%s'", refname);
+   log_file = git_pathdup("logs/%s", refname);
+   if (!reflog_exists(refname))
goto finish;
if (!cmd->dry_run) {
-   newlog_path = git_pathdup("logs/%s.lock", ref);
+   newlog_path = git_pathdup("logs/%s.lock", refname);
cb.newlog = fopen(newlog_path, "w");
}
 
cb.cmd = cmd;
 
-   if (!cmd->expire_unreachable || !strcmp(ref, "HEAD")) {
+   if (!cmd->expire_unreachable || !strcmp(refname, "HEAD")) {
tip_commit = NULL;
cb.unreachable_expire_kind = UE_HEAD;
} else {
@@ -407,7 +407,7 @@ static int expire_reflog(const char *ref, const unsigned 
char *sha1, void *cb_da
mark_reachable(&cb);
}
 
-   for_each_reflog_ent(ref, expire_reflog_ent, &cb);
+   for_each_reflog_ent(refname, expire_reflog_ent, &cb);
 
if (cb.unreachable_expire_kind != UE_ALWAYS) {
if (cb.unreachable_expire_kind == UE_HEAD) {
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 11/23] expire_reflog(): move dry_run to flags argument

2014-12-04 Thread Michael Haggerty
The policy objects don't care about "--dry-run". So move it to
expire_reflog()'s flags parameter.

Signed-off-by: Michael Haggerty 
---
 builtin/reflog.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 319f0d2..a490193 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -22,7 +22,6 @@ static unsigned long default_reflog_expire_unreachable;
 
 struct cmd_reflog_expire_cb {
struct rev_info revs;
-   int dry_run;
int stalefix;
int rewrite;
int updateref;
@@ -415,6 +414,10 @@ static void reflog_expiry_cleanup(struct expire_reflog_cb 
*cb)
 
 static struct lock_file reflog_lock;
 
+enum expire_reflog_flags {
+   EXPIRE_REFLOGS_DRY_RUN = 1 << 0
+};
+
 static int expire_reflog(const char *refname, const unsigned char *sha1,
 unsigned int flags, void *cb_data)
 {
@@ -439,7 +442,7 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1,
}
 
log_file = git_pathdup("logs/%s", refname);
-   if (!cmd->dry_run) {
+   if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
goto failure;
cb.newlog = fdopen_lock_file(&reflog_lock, "w");
@@ -453,7 +456,7 @@ static int expire_reflog(const char *refname, const 
unsigned char *sha1,
for_each_reflog_ent(refname, expire_reflog_ent, &cb);
reflog_expiry_cleanup(&cb);
 
-   if (cb.newlog) {
+   if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
if (close_lock_file(&reflog_lock)) {
status |= error("Couldn't write %s: %s", log_file,
strerror(errno));
@@ -644,7 +647,7 @@ static int cmd_reflog_expire(int argc, const char **argv, 
const char *prefix)
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
-   cb.dry_run = 1;
+   flags |= EXPIRE_REFLOGS_DRY_RUN;
else if (starts_with(arg, "--expire=")) {
if (parse_expiry_date(arg + 9, &cb.expire_total))
die(_("'%s' is not a valid timestamp"), arg);
@@ -738,7 +741,7 @@ static int cmd_reflog_delete(int argc, const char **argv, 
const char *prefix)
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
-   cb.dry_run = 1;
+   flags |= EXPIRE_REFLOGS_DRY_RUN;
else if (!strcmp(arg, "--rewrite"))
cb.rewrite = 1;
else if (!strcmp(arg, "--updateref"))
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 21/23] refs.c: remove unlock_ref/close_ref/commit_ref from the refs api

2014-12-04 Thread Michael Haggerty
From: Ronnie Sahlberg 

unlock|close|commit_ref can be made static since there are no more external
callers.

Signed-off-by: Ronnie Sahlberg 
Signed-off-by: Stefan Beller 
Signed-off-by: Michael Haggerty 
---
 refs.c | 24 
 refs.h |  9 -
 2 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/refs.c b/refs.c
index 5a12d37..c6ee775 100644
--- a/refs.c
+++ b/refs.c
@@ -2090,6 +2090,16 @@ int refname_match(const char *abbrev_name, const char 
*full_name)
return 0;
 }
 
+static void unlock_ref(struct ref_lock *lock)
+{
+   /* Do not free lock->lk -- atexit() still looks at them */
+   if (lock->lk)
+   rollback_lock_file(lock->lk);
+   free(lock->ref_name);
+   free(lock->orig_ref_name);
+   free(lock);
+}
+
 /* This function should make sure errno is meaningful on error */
 static struct ref_lock *verify_lock(struct ref_lock *lock,
const unsigned char *old_sha1, int mustexist)
@@ -2888,7 +2898,7 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
return 1;
 }
 
-int close_ref(struct ref_lock *lock)
+static int close_ref(struct ref_lock *lock)
 {
if (close_lock_file(lock->lk))
return -1;
@@ -2896,7 +2906,7 @@ int close_ref(struct ref_lock *lock)
return 0;
 }
 
-int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock)
 {
if (commit_lock_file(lock->lk))
return -1;
@@ -2904,16 +2914,6 @@ int commit_ref(struct ref_lock *lock)
return 0;
 }
 
-void unlock_ref(struct ref_lock *lock)
-{
-   /* Do not free lock->lk -- atexit() still looks at them */
-   if (lock->lk)
-   rollback_lock_file(lock->lk);
-   free(lock->ref_name);
-   free(lock->orig_ref_name);
-   free(lock);
-}
-
 /*
  * copy the reflog message msg to buf, which has been allocated sufficiently
  * large, while cleaning up the whitespaces.  Especially, convert LF to space,
diff --git a/refs.h b/refs.h
index d777f76..82611b5 100644
--- a/refs.h
+++ b/refs.h
@@ -198,15 +198,6 @@ extern struct ref_lock *lock_any_ref_for_update(const char 
*refname,
const unsigned char *old_sha1,
int flags, int *type_p);
 
-/** Close the file descriptor owned by a lock and return the status */
-extern int close_ref(struct ref_lock *lock);
-
-/** Close and commit the ref locked by the lock */
-extern int commit_ref(struct ref_lock *lock);
-
-/** Release any lock taken but not written. **/
-extern void unlock_ref(struct ref_lock *lock);
-
 /*
  * Setup reflog before using. Set errno to something meaningful on failure.
  */
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 04/23] expire_reflog(): remove unused parameter

2014-12-04 Thread Jonathan Nieder
Michael Haggerty wrote:

> It was called "unused", so at least it was self-consistent.

The missing context is that this was a callback function that had to
match the each_ref_fn signature (where that parameter is 'flags')
until v1.5.4~14 (reflog-expire: avoid creating new files in a
directory inside readdir(3) loop, 2008-01-25).  v1.5.4~14 forgot to
clean up.

With or without a note in the commit message explaining that,
Reviewed-by: Jonathan Nieder 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

2014-12-04 Thread Junio C Hamano
Jonathan Nieder  writes:

> Signed-off-by: Jonathan Nieder 
> ---
> Junio C Hamano wrote:
>> Jonathan Nieder  writes:
>
>>> -extern int copy_fd(int ifd, int ofd);
>>> +extern int copy_fd(int ifd, int ofd, struct strbuf *err);
>>
>> It is not limited to this single function, but what contract do we
>> envision this "error messages are given back to the caller via
>> strbuf" convention should give between the callers and the callee?
>
> Here's a draft for documentation on that.

Thanks; looks reasonable; even if the discussion between you and
Peff took us to a slightly different direction than what you
described here, the earlier description of long established practice
is a welcome addition.




>  Documentation/technical/api-error-handling.txt | 75 
> ++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/technical/api-error-handling.txt
>
> diff --git a/Documentation/technical/api-error-handling.txt
> b/Documentation/technical/api-error-handling.txt
> new file mode 100644
> index 000..fc68db1
> --- /dev/null
> +++ b/Documentation/technical/api-error-handling.txt
> @@ -0,0 +1,75 @@
> +Error reporting in git
> +==
> +
> +`die`, `usage`, `error`, and `warning` report errors of various
> +kinds.
> +
> +- `die` is for fatal application errors.  It prints a message to
> +  the user and exits with status 128.
> +
> +- `usage` is for errors in command line usage.  After printing its
> +  message, it exits with status 129.  (See also `usage_with_options`
> +  in the link:api-parse-options.html[parse-options API].)
> +
> +- `error` is for non-fatal library errors.  It prints a message
> +  to the user and returns -1 for convenience in signaling the error
> +  to the caller.
> +
> +- `warning` is for reporting situations that probably should not
> +  occur but which the user (and Git) can continue to work around
> +  without running into too many problems.  Like `error`, it
> +  returns -1 after reporting the situation to the caller.
> +
> +Customizable error handlers
> +---
> +
> +The default behavior of `die` and `error` is to write a message to
> +stderr and then exit or return as appropriate.  This behavior can be
> +overridden using `set_die_routine` and `set_error_routine`.  For
> +example, "git daemon" uses set_die_routine to write the reason `die`
> +was called to syslog before exiting.
> +
> +Library errors
> +--
> +
> +Functions return a negative integer on error.  Details beyond that
> +vary from function to function:
> +
> +- Some functions return -1 for all errors.  Others return a more
> +  specific value depending on how the caller might want to react
> +  to the error.
> +
> +- Some functions report the error to stderr with `error`,
> +  while others leave that for the caller to do.
> +
> +- errno is not meaningful on return from most functions (except
> +  for thin wrappers for system calls).
> +
> +Check the function's API documentation to be sure.
> +
> +Caller-handled errors
> +-
> +
> +An increasing number of functions take a parameter 'struct strbuf *err'.
> +On error, such functions append a message about what went wrong to the
> +'err' strbuf.  The message is meant to be complete enough to be passed
> +to `die` or `error` as-is.  For example:
> +
> + if (ref_transaction_commit(transaction, &err))
> + die("%s", err.buf);
> +
> +The 'err' parameter will be untouched if no error occured, so multiple
> +function calls can be chained:
> +
> + t = ref_transaction_begin(&err);
> + if (!t ||
> + ref_transaction_update(t, "HEAD", ..., &err) ||
> + ret_transaction_commit(t, &err))
> + die("%s", err.buf);
> +
> +The 'err' parameter must be a pointer to a valid strbuf.  To silence
> +a message, pass a strbuf that is explicitly ignored:
> +
> + if (thing_that_can_fail_in_an_ignorable_way(..., &err))
> + /* This failure is okay. */
> + strbuf_reset(&err);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 04/23] expire_reflog(): remove unused parameter

2014-12-04 Thread Jonathan Nieder
Michael Haggerty wrote:

> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -349,7 +349,7 @@ static int push_tip_to_list(const char *refname, const 
> unsigned char *sha1, int
>   return 0;
>  }
>  
> -static int expire_reflog(const char *ref, const unsigned char *sha1, int 
> unused, void *cb_data)
> +static int expire_reflog(const char *ref, const unsigned char *sha1, void 
> *cb_data)
>  {
>   struct cmd_reflog_expire_cb *cmd = cb_data;

On second thought: why not update the last parameter to be a 'struct
cmd_reflog_expire_cb *' instead of 'void *' while at it, like this?

 builtin/reflog.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git i/builtin/reflog.c w/builtin/reflog.c
index 3e11bee..d860624 100644
--- i/builtin/reflog.c
+++ w/builtin/reflog.c
@@ -349,9 +349,8 @@ static int push_tip_to_list(const char *refname, const 
unsigned char *sha1, int
return 0;
 }
 
-static int expire_reflog(const char *ref, const unsigned char *sha1, void 
*cb_data)
+static int expire_reflog(const char *ref, const unsigned char *sha1, struct 
cmd_reflog_expire_cb *cmd)
 {
-   struct cmd_reflog_expire_cb *cmd = cb_data;
struct expire_reflog_cb cb;
struct ref_lock *lock;
char *log_file, *newlog_path = NULL;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


GIT: ignoring changes in tracked files and bug report

2014-12-04 Thread Sérgio Basto
Hi, I'm trying find a solution where I can change file in a devel
environment , and not commit it into git . 

git update-index --assume-unchanged  

is one solution , not the best solution but one solution. 

I add 2 files that I want ignore on commits 

git update-index --assume-unchanged configurations/local.defs
git update-index --assume-unchanged processor/default.defs

git diff -a 
is clean 
git diff .
is clean

git commit -a 
nothing added to commit
but 
git commit . 
# Changes to be committed:
#   modified:   configurations/local.defs
#   modified:   processor/default.defs

this is a bug ? 

Anyway what is best way to deal with some files where we want change
locally and not commit in git . 
The solution of have one environment variable that says if we are in
devel or in production , and system read the variable and choose the
files to read can't be applied in my case, and is a no solution , is
save a variable outside of source code which I can't. 


Thanks,
-- 
Sérgio M. B.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

2014-12-04 Thread Jonathan Nieder
Hi,

Junio C Hamano wrote:
> Jonathan Nieder  writes:

>> Here's a draft for documentation on that.
>
> Thanks; looks reasonable; even if the discussion between you and
> Peff took us to a slightly different direction than what you
> described here, the earlier description of long established practice
> is a welcome addition.

Thanks.

Can you say more?  I didn't think the discussion had reached a
conclusion yet.

Unless there's a strong reason to do otherwise, I prefer to stick with
the strbufs in this series for now.  I prefer strbuf for this purpose
over char[1024].  And switching later doesn't seem hard.

But if others prefer char[1024], I can update the existing code that
uses strbuf from the transaction API to use that and reroll this
series with that convention.  A part of me dislikes that but I'll go
along.

Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

2014-12-04 Thread Jonathan Nieder
Junio C Hamano wrote:
> Jonathan Nieder  writes:

>> Here's a draft for documentation on that.
>
> Thanks; looks reasonable; even if the discussion between you and
> Peff took us to a slightly different direction than what you
> described here, the earlier description of long established practice
> is a welcome addition.

I think I see what I misunderstood.  Do you mean "even if we settle on
a different API, this documentation of what you started with should be
easy to adapt and will make life easier"?

In other words, did you mean to say that things are still up in the
air (which I agree with) or that the project has already settled on a
different direction (which I do not)?

Sorry for the confusion,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 05/23] expire_reflog(): rename "ref" parameter to "refname"

2014-12-04 Thread Jonathan Nieder
Michael Haggerty wrote:

> Signed-off-by: Michael Haggerty 
> ---
>  builtin/reflog.c | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Jonathan Nieder 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

2014-12-04 Thread Jeff King
On Thu, Dec 04, 2014 at 03:41:47PM -0800, Jonathan Nieder wrote:

> Junio C Hamano wrote:
> > Jonathan Nieder  writes:
> 
> >> Here's a draft for documentation on that.
> >
> > Thanks; looks reasonable; even if the discussion between you and
> > Peff took us to a slightly different direction than what you
> > described here, the earlier description of long established practice
> > is a welcome addition.
> 
> I think I see what I misunderstood.  Do you mean "even if we settle on
> a different API, this documentation of what you started with should be
> easy to adapt and will make life easier"?
> 
> In other words, did you mean to say that things are still up in the
> air (which I agree with) or that the project has already settled on a
> different direction (which I do not)?

FWIW, that is how I read it (up in the air), and where I thought our
discussion was.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/23] Add reflog_expire() to the references API

2014-12-04 Thread Junio C Hamano
Michael Haggerty  writes:

> I propose this patch series as an alternative to Ronnie's "reflog
> transactions" series.

After coasting my eyes over individual patches, it was delightful to
look at the endgame state of the expiry codepath in builtin/reflog.c
that is left ;-)


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 06/23] expire_reflog(): exit early if the reference has no reflog

2014-12-04 Thread Jonathan Nieder
Michael Haggerty wrote:

> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -368,9 +368,11 @@ static int expire_reflog(const char *refname, const 
> unsigned char *sha1, void *c
>   lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
>   if (!lock)
>   return error("cannot lock ref '%s'", refname);
> + if (!reflog_exists(refname)) {
> + unlock_ref(lock);
> + return 0;
> + }
>   log_file = git_pathdup("logs/%s", refname);
> - if (!reflog_exists(refname))
> - goto finish;

Okay.

Reviewed-by: Jonathan Nieder 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

2014-12-04 Thread Junio C Hamano
Yeah, that is what I meant. The earlier part will not go to waste no matter
what happens to the discussion.

I am not a fan of char[1024], if only because our error message may have
to mention things whose length is not under our control, e.g. a filename
in the working tree, but I do share your concern that "strbuf"-approach
calls for more boilerplate. I offhand do not have a magic silver bullet for
it, though.

On Thu, Dec 4, 2014 at 3:44 PM, Jeff King  wrote:
> On Thu, Dec 04, 2014 at 03:41:47PM -0800, Jonathan Nieder wrote:
>
>> Junio C Hamano wrote:
>> > Jonathan Nieder  writes:
>>
>> >> Here's a draft for documentation on that.
>> >
>> > Thanks; looks reasonable; even if the discussion between you and
>> > Peff took us to a slightly different direction than what you
>> > described here, the earlier description of long established practice
>> > is a welcome addition.
>>
>> I think I see what I misunderstood.  Do you mean "even if we settle on
>> a different API, this documentation of what you started with should be
>> easy to adapt and will make life easier"?
>>
>> In other words, did you mean to say that things are still up in the
>> air (which I agree with) or that the project has already settled on a
>> different direction (which I do not)?
>
> FWIW, that is how I read it (up in the air), and where I thought our
> discussion was.
>
> -Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 06/23] expire_reflog(): exit early if the reference has no reflog

2014-12-04 Thread Jonathan Nieder
Michael Haggerty wrote:

> [Subject: expire_reflog(): exit early if the reference has no reflog]

The caller moves on to expire other reflogs, so it's not exiting.
"return early", maybe?

Except the function already returned early.  I think the purpose of
this patch is to simplify the no-reflog case by handling it separately.

Anyway, that's just nitpicking about the subject line.  With
s/exit/return/ it should be clear that this is a refactoring change,
which for someone looking at the shortlog is the important thing.

Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Git's configure script --mandir doesn't work

2014-12-04 Thread Stephen Fisher
I'm installing Git 2.2.0 from source distribution on NetBSD 6.1.5 
(amd64) and when I specify --mandir=/usr/local/man, it still installs 
man pages in the default /usr/local/share/man directory.  Is there a fix 
available for this?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

2014-12-04 Thread Jeff King
On Thu, Dec 04, 2014 at 03:52:45PM -0800, Junio C Hamano wrote:

> Yeah, that is what I meant. The earlier part will not go to waste no matter
> what happens to the discussion.
> 
> I am not a fan of char[1024], if only because our error message may have
> to mention things whose length is not under our control, e.g. a filename
> in the working tree, but I do share your concern that "strbuf"-approach
> calls for more boilerplate. I offhand do not have a magic silver bullet for
> it, though.

The only downside I can think of is that we may truncate the message in
exceptional circumstances. But is it really any less helpful to say:

  error: unable to open file: some-incredibly-long-filename-aa...

than printing out an extra 100 lines of "a"? And I mean the "..."
literally. I think mkerror() should indicate the truncation with a
"...", just so that it is clear to the user. It should almost never
happen, but when it does, it can be helpful to show the user that yes,
we know we are truncating the message, and it is not that git truncated
your filename during the operation.

Is this truncation really a concern, and/or is there some other downside
I'm not thinking of?

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file

2014-12-04 Thread Jonathan Nieder
Michael Haggerty wrote:

> We don't actually need the locking functionality, because we already
> hold the lock on the reference itself, which is how the reflog file is
> locked. But the lock_file code still does some of the bookkeeping for
> us and is more careful than the old code here was.

As you say, the ref lock takes care of mutual exclusion, so we do not
have to be too careful about compatibility with other tools that might
not know to lock the reflog.  And this is not tying our hands for a
future when I might want to lock logs/refs/heads/topic/1 while
logs/refs/heads/topic still exists as part of the implementation of
"git mv topic/1 topic".

Stefan and I had forgotten about that guarantee when looking at that
kind of operation --- thanks for the reminder.

Should updates to the HEAD reflog acquire HEAD.lock?  (They don't
currently.)

[...]
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const 
> unsigned char *sha1, int
>   return 0;
>  }
>  
> +static struct lock_file reflog_lock;

If this lockfile is only used in that one function, it can be declared
inside the function.

If it is meant to be used throughout the 'git reflog' command, then it
can go near the top of the file.

> +
>  static int expire_reflog(const char *refname, const unsigned char *sha1, 
> void *cb_data)
>  {
>   struct cmd_reflog_expire_cb *cmd = cb_data;
>   struct expire_reflog_cb cb;
>   struct ref_lock *lock;
> - char *log_file, *newlog_path = NULL;
> + char *log_file;
>   struct commit *tip_commit;
>   struct commit_list *tips;
>   int status = 0;
> @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const 
> unsigned char *sha1, void *c
>   unlock_ref(lock);
>   return 0;
>   }
> +
>   log_file = git_pathdup("logs/%s", refname);
>   if (!cmd->dry_run) {
> - newlog_path = git_pathdup("logs/%s.lock", refname);
> - cb.newlog = fopen(newlog_path, "w");
> + if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
> + goto failure;

hold_lock_file_for_update doesn't print a message.  Code to print one
looks like

if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
unable_to_lock_message(log_file, errno, &err);
error("%s", err.buf);
goto failure;
}

(A patch in flight changes that to

if (hold_lock_file_for_update(&reflog_lock, log_file, 0, &err) < 0) {
error("%s", err.buf);
goto failure;
}

)

> + cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> + if (!cb.newlog)
> + goto failure;

Hm.  lockfile.c::fdopen_lock_file ought to use xfdopen to make this
case impossible.  And xfdopen should use try_to_free_routine() and
try again on failure.

[...]
> @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const 
> unsigned char *sha1, void *c
>   }
>  
>   if (cb.newlog) {
> - if (fclose(cb.newlog)) {
> - status |= error("%s: %s", strerror(errno),
> - newlog_path);
> - unlink(newlog_path);
> + if (close_lock_file(&reflog_lock)) {
> + status |= error("Couldn't write %s: %s", log_file,
> + strerror(errno));

Style nit: error messages usually start with a lowercase letter
(though I realize nearby examples are already inconsistent).

commit_lock_file() can take care of the close_lock_file automatically.

[...]
> @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const 
> unsigned char *sha1, void *c
>close_ref(lock) < 0)) {
>   status |= error("Couldn't write %s",
>   lock->lk->filename.buf);
> - unlink(newlog_path);
> - } else if (rename(newlog_path, log_file)) {
> - status |= error("cannot rename %s to %s",
> - newlog_path, log_file);
> - unlink(newlog_path);
> + rollback_lock_file(&reflog_lock);
> + } else if (commit_lock_file(&reflog_lock)) {
> + status |= error("cannot rename %s.lock to %s",
> + log_file, log_file);

Most callers say "unable to commit reflog '%s'", log_file to hedge their
bets in case the close failed (which may be what you were avoiding
above.

errno is meaningful when commit_lock_file fails, making a more
detailed diagnosis from strerror(errno) possible.

Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] for_each_reflog_ent_reverse: fix newlines on block boundaries

2014-12-04 Thread Jeff King
On Thu, Dec 04, 2014 at 02:14:50PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I think the bug is in the reverse-reflog reader in
> > for_each_reflog_ent_reverse. It reads BUFSIZ chunks of the file in
> > reverse order, and then parses them individually. If the trailing
> > newline for a line falls directly on the block boundary, we may not have
> > it in our current block, and pass the line to show_one_reflog_ent
> > without a trailing newline.
> 
> Ahh, thanks for helping spot it.  A code that uses BUFSIZ explains
> why a single reproduction recipe is platform dependent.

It also makes writing portable tests annoying, but I think I managed it
in the patch below. :)

> > So this is a long-standing bug in for_each_reflog_ent_reverse. It just
> > showed up recently because we started using that function for
> > read_ref_at_ent.
> > ...
> > The above is a workaround. I think the right solution is probably to
> > teach for_each_reflog_ent_reverse to makes sure the trailing newline is
> > included (either by tweaking the reverse code, or conditionally adding
> > it to the parsed buffer).
> 
> Sounds correct.  Unfortunately I no longer remember how I decided to
> deal with a line that spans the block boundary in that piece of code
> X-<.

Here's my proposed fix.

-- >8 --
When we read a reflog file in reverse, we read whole chunks
of BUFSIZ bytes, then loop over the buffer, parsing any
lines we find. We find the beginning of each line by looking
for the newline from the previous line. If we don't find
one, we know that we are either at the beginning of
the file, or that we have to read another block.

In the latter case, we stuff away what we have into a
strbuf, read another block, and continue our parse. But we
missed one case here. If we did find a newline, and it is at
the beginning of the block, we must also stuff that newline
into the strbuf, as it belongs to the block we are about to
read.

The minimal fix here would be to add this special case to
the conditional that checks whether we found a newline.
But we can make the flow a little clearer by rearranging a
bit: we first handle lines that we are going to show, and
then at the end of each loop, stuff away any leftovers if
necessary. That lets us fold this special-case in with the
more common "we ended in the middle of a line" case.

Signed-off-by: Jeff King 
---
I really needed this rearranging to figure out what the fix
_should_ be. Now that I did that, I was able to write the above
paragraph explaining what the minimal fix would be. And I can do that if
we prefer. But I think the fact that I had to go through the untangling
step first is an indication that maybe the end result is better. Of
course that's all subjective. :)

I waffled on the test script between generating it on the fly (as
below), and just including a complete 8K example that provokes the
failure. I don't care about the size that much, but rather on which is
more readable. My goal with showing the generation steps was to make it
clear how we arrived there, but I fear it may have ended up too
convoluted to do anyone any good. Opinions welcome.

 refs.c| 47 ---
 t/t1410-reflog.sh | 30 ++
 2 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/refs.c b/refs.c
index 5ff457e..ccb8834 100644
--- a/refs.c
+++ b/refs.c
@@ -3404,24 +3404,49 @@ int for_each_reflog_ent_reverse(const char *refname, 
each_reflog_ent_fn fn, void
 
bp = find_beginning_of_line(buf, scanp);
 
-   if (*bp != '\n') {
-   strbuf_splice(&sb, 0, 0, buf, endp - buf);
-   if (pos)
-   break; /* need to fill another block */
-   scanp = buf - 1; /* leave loop */
-   } else {
+   if (*bp == '\n') {
/*
-* (bp + 1) thru endp is the beginning of the
-* current line we have in sb
+* The newline is the end of the previous line,
+* so we know we have complete line starting
+* at (bp + 1). Prefix it onto any prior data
+* we collected for the line and process it.
 */
strbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 
1));
scanp = bp;
endp = bp + 1;
+   ret = show_one_reflog_ent(&sb, fn, cb_data);
+   strbuf_reset(&sb);
+   if (ret)
+   break;
+   } else if (!pos) {
+   /*
+* We are at the st

[PATCH] for_each_reflog_ent_reverse: turn leftover check into assertion

2014-12-04 Thread Jeff King
On Thu, Dec 04, 2014 at 08:28:54PM -0500, Jeff King wrote:

> The minimal fix here would be to add this special case to
> the conditional that checks whether we found a newline.
> But we can make the flow a little clearer by rearranging a
> bit: we first handle lines that we are going to show, and
> then at the end of each loop, stuff away any leftovers if
> necessary. That lets us fold this special-case in with the
> more common "we ended in the middle of a line" case.
> 
> Signed-off-by: Jeff King 
> ---
> I really needed this rearranging to figure out what the fix
> _should_ be. Now that I did that, I was able to write the above
> paragraph explaining what the minimal fix would be. And I can do that if
> we prefer. But I think the fact that I had to go through the untangling
> step first is an indication that maybe the end result is better. Of
> course that's all subjective. :)

I _think_ the patch below is also applicable to the code before my
boundary-fixing patch. But the rearranging also made me more confident
in it.

-- >8 --
Subject: for_each_reflog_ent_reverse: turn leftover check into assertion

Our loop should always process all lines, even if we hit the
beginning of the file. We have a conditional after the loop
ends to double-check that there is nothing left and to
process it. But this should never happen, and is a sign of a
logic bug in the loop. Let's turn it into a BUG assertion.

Signed-off-by: Jeff King 
---
Of course I cannot say something like "this can never happen; the old
code was wrong to handle this case" without a nagging feeling that I am
missing something, so extra careful eyes are appreciated (and are why I
would rather have an assert here than removing the code and silently
dropping lines if I am wrong).

 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index ccb8834..1f77fa6 100644
--- a/refs.c
+++ b/refs.c
@@ -3451,7 +3451,7 @@ int for_each_reflog_ent_reverse(const char *refname, 
each_reflog_ent_fn fn, void
 
}
if (!ret && sb.len)
-   ret = show_one_reflog_ent(&sb, fn, cb_data);
+   die("BUG: reverse reflog parser had leftover data");
 
fclose(logfp);
strbuf_release(&sb);
-- 
2.2.0.390.gf60752d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH/RFC v2] Squashed changes for multiple worktrees vs. submodules

2014-12-04 Thread Duy Nguyen
On Fri, Dec 5, 2014 at 3:06 AM, Jens Lehmann  wrote:
> Wow, so the .git/config is shared between all worktrees? I
> suspect you have very good reasons for that,

most of config vars are at repo-level, not worktree-level, except
maybe submodule.* and something else. Technically we could use
"include.path" to point to a non-shared file, where we store
worktree-specific config.

> but I believe
> that'll make multiple work trees surprise the user from time
> to time when used with submodules. Because initializing a
> submodule in one worktree initializes it in all other
> worktrees too (I suspect other users regard "git submodule
> init" being a worktree local command too). And setting
> "submodule..update" to "none" will also affect all
> other worktrees. But I'd need to have separate settings for
> our CI server, e.g. to checkout the sources without the
> largish documentation submodule in one test job (=worktree)
> while checking out the whole documentation for another job
> building the setup in another worktree.
>
> And if I understand the "checkout: reject if the branch is
> already checked out elsewhere" thread correctly, I won't be
> able to build "master" in two jobs at the same time?

If you do "git checkout --to ... master^{}", it should run fine. I'm
still considering doing something better with the read-only refs, but
haven't found time to really think it through yet.

> Thanks. But I changed my mind about the details (now that I
> know about .git/config and multiple worktrees). I think you'd
> have to connect a .git directory in the submodule to the
> common git dir directly, as you cannot use the core.worktree
> setting (which could be different between commits due to
> renames) when putting it into /.git/modules. And
> then you couldn't remove or rename a submodule anymore,
> because that fails when it contains a .git directory.
>
> Seems like we should put a "Warning: may do unexpected things
> when used with submodules" (with some examples about what might
> happen) in the multiple worktrees documentation. And I don't
> believe anymore that teaching submodules to use the common git
> dir makes that much sense after I know about the restrictions
> it imposes.

I'm ok with such a warning fwiw.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[GIT PULL] l10n update on maint branch

2014-12-04 Thread Jiang Xin
Hi Junio,

Please pull l10n update to maint branch.

The following changes since commit b260d265e189728b26e50506ac6ffab6a7d588da:

  Git 2.2 (2014-11-26 13:18:34 -0800)

are available in the git repository at:

  git://github.com/git-l10n/git-po maint

for you to fetch changes up to ff51f5619d52410e4343c7cd75889e7d07f5659e:

  Merge branch 'master' of https://github.com/ralfth/git-po-de
(2014-11-29 10:44:48 +0800)


Hartmut Henkel (1):
  l10n: de.po: fix typos

Jiang Xin (1):
  Merge branch 'master' of https://github.com/ralfth/git-po-de

 po/de.po | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file

2014-12-04 Thread Stefan Beller
On Thu, Dec 04, 2014 at 04:23:31PM -0800, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
> > We don't actually need the locking functionality, because we already
> > hold the lock on the reference itself, which is how the reflog file is
> > locked. But the lock_file code still does some of the bookkeeping for
> > us and is more careful than the old code here was.
> 
> As you say, the ref lock takes care of mutual exclusion, so we do not
> have to be too careful about compatibility with other tools that might
> not know to lock the reflog.  And this is not tying our hands for a
> future when I might want to lock logs/refs/heads/topic/1 while
> logs/refs/heads/topic still exists as part of the implementation of
> "git mv topic/1 topic".
> 
> Stefan and I had forgotten about that guarantee when looking at that
> kind of operation --- thanks for the reminder.

I did not forget about it, I did not know about that in the first hand.
We don't seem to have documentation on it?

So sorry for heading in a direction, which would have been avoidable.

Thanks,
Stefan

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] introduce git root

2014-12-04 Thread Christian Couder
Jeff King  wrote:

> Some of the discussion has involved mixing config options into this
> kitchen sink, but that does not make any sense to me (and is why I
> find "git var -l" so odd). Config options are fundamentally different, in
> that they are set and retrieved, not computed (from other config
> variables, or from hard-coded values). And we already have a nice
> tool for working with them (well...nice-ish, let's say).

Yeah, but "git config" cannot say which config option applies in some
context and why.
For example, to chose the editor all the following could apply:

GIT_SEQUENCE_EDITOR env variable
sequence.editor config variable
GIT_EDITOR env variable
core.editor config variable
VISUAL env variable
EDITOR env variable
editor configured at compile time

and the user or our own scripts right now cannot easily know which
editor should be used when editing the sequence list.

The best they can do is:

- first check if GIT_SEQUENCE_EDITOR is set, and if yes, use it
- then check if sequence.editor config variable is set, and if yes, use it
- then use "git var GIT_EDITOR" that will check the other options

I don't think it is very nice.

Jeff King  also wrote:

> My issue is only that "git --foo" has other options besides computables.
> So you need to name each option in a way that makes it clear it is
> reporting a computable and not doing something else.
>
> Take "git --pager" for instance. That would be a natural choice to
> replace "git var GIT_PAGER". But shouldn't "--pager" be the opposite of
> the existing "--no-pager"?
>
> So instead we probably need some namespace to indicate that it is a
> "showing" option. Like "--show-pager". And then for consistency, we
> would probably want to move "--exec-path" to "--show-exec-path",
> creating a new "--show-" namespace. Or we could call that namespace
> "git var". :)

I agree with that, but I think it could be better if there was also a
notion of context,

> I do not think "git var --exec-path" is a good idea, nor GIT_EXEC_PATH
> for the environment-variable confusion you mentioned. I was thinking of
> just creating a new namespace, like:
>
>   git var exec-path
>   git var author-ident

I agree that this is nice, but I wonder what we would do for the
sequence editor and the default editor.
Maybe:

git var sequence-editor
git var editor

That would already be nicer than what we have now, but maybe we should
consider the following instead:

git var sequence.editor
git var core.editor

(and maybe also some aliases to core.editor, like:

git var default.editor
git var editor)

I think "sequence.editor" and "core.editor" are better because:

- they use the same syntax as the config variables, so they are easier
to remember and to discover, and
- they provide a notion of context.

The notion of context is interesting because suppose that we later
introduce the "commit.editor" config variable. If we decide now that
"foo.editor" means just "core.editor" if we don't know about any
"editor" variable related to the "foo" context, then the scripts that
might later be written using "git var commit.editor" will not have to
worry about the fact that previous versions of Git didn't know about
"commit.editor".

People could even start using "git var commit.editor" now, because it
would work even if "commit.editor" is unused by git commit.

Of course when the user asks for "git var foo.editor" and we don't
know about any "editor" variable related to the "foo" context, we
first should check if "foo.editor" exists in the config file and we
should use that if it exists, before we default to "git var
core.editor".

Best,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 07/23] expire_reflog(): use a lock_file for rewriting the reflog file

2014-12-04 Thread ronnie sahlberg
On Thu, Dec 4, 2014 at 3:08 PM, Michael Haggerty  wrote:
> We don't actually need the locking functionality, because we already
> hold the lock on the reference itself,

No. You do need the lock.
The ref is locked only during transaction_commit()

If you don't want to lock the reflog file and instead rely on the lock
on the ref itself you will need to
rework your patches so that the lock on the ref is taken already
during, for example, transaction_update_ref() instead.

But without doing those changes and moving the ref locking from
_commit() to _update_ref() you will risk reflog corruption/surprises
if two operations collide and both rewrite the reflog without any lock held.


> which is how the reflog file is
> locked. But the lock_file code still does some of the bookkeeping for
> us and is more careful than the old code here was.
>
> For example:
>
> * Correctly handle the case that the reflog lock file already exists
>   for some reason or cannot be opened.
>
> * Correctly clean up the lockfile if the program dies.
>
> Signed-off-by: Michael Haggerty 
> ---
>  builtin/reflog.c | 37 ++---
>  1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index a282e60..d344d45 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -349,12 +349,14 @@ static int push_tip_to_list(const char *refname, const 
> unsigned char *sha1, int
> return 0;
>  }
>
> +static struct lock_file reflog_lock;
> +
>  static int expire_reflog(const char *refname, const unsigned char *sha1, 
> void *cb_data)
>  {
> struct cmd_reflog_expire_cb *cmd = cb_data;
> struct expire_reflog_cb cb;
> struct ref_lock *lock;
> -   char *log_file, *newlog_path = NULL;
> +   char *log_file;
> struct commit *tip_commit;
> struct commit_list *tips;
> int status = 0;
> @@ -372,10 +374,14 @@ static int expire_reflog(const char *refname, const 
> unsigned char *sha1, void *c
> unlock_ref(lock);
> return 0;
> }
> +
> log_file = git_pathdup("logs/%s", refname);
> if (!cmd->dry_run) {
> -   newlog_path = git_pathdup("logs/%s.lock", refname);
> -   cb.newlog = fopen(newlog_path, "w");
> +   if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0)
> +   goto failure;
> +   cb.newlog = fdopen_lock_file(&reflog_lock, "w");
> +   if (!cb.newlog)
> +   goto failure;
> }
>
> cb.cmd = cmd;
> @@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const 
> unsigned char *sha1, void *c
> }
>
> if (cb.newlog) {
> -   if (fclose(cb.newlog)) {
> -   status |= error("%s: %s", strerror(errno),
> -   newlog_path);
> -   unlink(newlog_path);
> +   if (close_lock_file(&reflog_lock)) {
> +   status |= error("Couldn't write %s: %s", log_file,
> +   strerror(errno));
> } else if (cmd->updateref &&
> (write_in_full(lock->lock_fd,
> sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
> @@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const 
> unsigned char *sha1, void *c
>  close_ref(lock) < 0)) {
> status |= error("Couldn't write %s",
> lock->lk->filename.buf);
> -   unlink(newlog_path);
> -   } else if (rename(newlog_path, log_file)) {
> -   status |= error("cannot rename %s to %s",
> -   newlog_path, log_file);
> -   unlink(newlog_path);
> +   rollback_lock_file(&reflog_lock);
> +   } else if (commit_lock_file(&reflog_lock)) {
> +   status |= error("cannot rename %s.lock to %s",
> +   log_file, log_file);
> } else if (cmd->updateref && commit_ref(lock)) {
> status |= error("Couldn't set %s", lock->ref_name);
> -   } else {
> -   adjust_shared_perm(log_file);
> }
> }
> -   free(newlog_path);
> free(log_file);
> unlock_ref(lock);
> return status;
> +
> + failure:
> +   rollback_lock_file(&reflog_lock);
> +   free(log_file);
> +   unlock_ref(lock);
> +   return -1;
>  }
>
>  static int collect_reflog(const char *ref, const unsigned char *sha1, int 
> unused, void *cb_data)
> --
> 2.1.3
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


error: core.autocrlf=input conflicts with core.eol=crlf

2014-12-04 Thread Alex Stangl
Hi,

There seems to be problems with the checks in the git code for conflicts
between config values of core.autocrlf and core.eol. Because the various
config files are read in separate passes, and the conflict check happens
on the fly, it creates a situation where the order of the config file
entries matters. This seems like a bug or at least a POLA violation --
ordering of lines within a section of a config file is not usually
significant.

Example: User has core.autocrlf=input in his ~/.gitconfig. In his
project-level .git/config he wants to set core.autocrlf=false and
core.eol=crlf. If the core.autocrlf=false comes first, then all is
well and no conflict is reported. If the core.eol=crlf line comes
first, then at the time this line is getting parsed, core.autocrlf
is still set at "input" from ~/.gitconfig, and execution aborts:

error: core.autocrlf=input conflicts with core.eol=crlf

It seems like the conflict check should be made at the end of the
config file parsing, not on the fly. I was tempted to create a patch,
however I am unfamilar with the codebase, and didn't understand all
the places where the config file parsing is called, so I'm not sure
of the ramifications of any proposed change. A benefit of the current
approach is that it reports the line number where it aborted in the
config file. Trying to retain this and at the same time defer the
check until the end could get complicated.

Besides interaction between multiple levels of config files, the
same sort of ordering issue can arise in conjunction with command-line
config overrides.

Example: User has core.autocrlf=input in his project-level .git/config.
This command works fine:
$ git -c core.autocrlf=false -c core.eol=crlf status
This command blows up with conflict error:
$ git -c core.eol=crlf -c core.autocrlf=false status

I tested with git versions 1.9.3 and 2.1.0.

Alex

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


bug report on update-index --assume-unchanged

2014-12-04 Thread Sérgio Basto
Hi,

I add 2 files that I want ignore on commits 
git update-index --assume-unchanged configurations/local.defs
git update-index --assume-unchanged processor/default.defs

git diff -a 
is clean 
git diff .
is clean
git commit -a 

nothing added to commit

but 

git commit . 
# Changes to be committed:
#   modified:   configurations/local.defs
#   modified:   processor/default.defs

this is a bug .

thanks
-- 
Sérgio M. B.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH/RFC v2] Squashed changes for multiple worktrees vs. submodules

2014-12-04 Thread Max Kirillov
On Thu, Dec 4, 2014 at 10:06 PM, Jens Lehmann  wrote:
> But I'd need to have separate settings for
> our CI server, e.g. to checkout the sources without the
> largish documentation submodule in one test job (=worktree)
> while checking out the whole documentation for another job
> building the setup in another worktree.

Currently I'm estimating approach when submodules which have .git
file or directory inside are updated, and those which do not have it are not.
I have added a config variable submodule.updateIgnoringConfigUrl (because
usually the submodule..url is what turns on the update). It looks working,
maybe I even add setting the variable when chackout --to is used.

> And if I understand the "checkout: reject if the branch is
> already checked out elsewhere" thread correctly, I won't be
> able to build "master" in two jobs at the same time?

You are alerady second person complaining about it, but I don't really see
how this can be a problem. Make a branch 'master2', it's another 40 bytes.

> So two reasons against using multiple worktrees on our CI
> server to save quite some disk space :-(

My use is not to save space (working tree files often takes more than
the repository
itself), but for development, I have like 3-5 checkouts usually, which
used to be local
clones, but not having to keep synching them is really life changing.

> Thanks. But I changed my mind about the details (now that I
> know about .git/config and multiple worktrees). I think you'd
> have to connect a .git directory in the submodule to the
> common git dir directly, as you cannot use the core.worktree
> setting (which could be different between commits due to
> renames) when putting it into /.git/modules. And
> then you couldn't remove or rename a submodule anymore,
> because that fails when it contains a .git directory.

I need to think more about it.

> Seems like we should put a "Warning: may do unexpected things
> when used with submodules" (with some examples about what might
> happen) in the multiple worktrees documentation. And I don't
> believe anymore that teaching submodules to use the common git
> dir makes that much sense after I know about the restrictions
> it imposes.

btw, I thought even about making it an error to add/remove/(de)initialize
submodule not in the main working tree. But I'm afraid it would not be
considered appropriate for merging.

-- 
Max
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: bug report on update-index --assume-unchanged

2014-12-04 Thread Johannes Sixt

Am 05.12.2014 07:12, schrieb Sérgio Basto:

Hi,

I add 2 files that I want ignore on commits
git update-index --assume-unchanged configurations/local.defs
git update-index --assume-unchanged processor/default.defs

git diff -a
is clean
git diff .
is clean
git commit -a

nothing added to commit

but

git commit .
# Changes to be committed:
#   modified:   configurations/local.defs
#   modified:   processor/default.defs

this is a bug .


Actually, it's a user error. When you set --assume-unchanged, then you 
give a promise to git that you do not change the files, and git does not 
have to check itself whether there is a change.


But since you did not keep your promise, you get what you deserve. ;-)

So, to follow-up on your nearby post: --assume-unchanged is *not* a tool 
to avoid accidentally committing changes to files that are tracked.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


<    1   2