* Re: [PATCH v3 04/16] files-backend: replace *git_path*() with files_path()
From: Junio C Hamano @ 2017-02-17 19:27 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Michael Haggerty, Johannes Schindelin, Ramsay Jones,
Stefan Beller, novalis
In-Reply-To: <20170217140436.17336-5-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This centralizes all path rewriting of files-backend.c in one place so
> we have easier time removing the path rewriting later. There could be
> some hidden indirect git_path() though, I didn't audit the code to the
> bottom.
>
> Side note: set_worktree_head_symref() is a bad boy and should not be in
> files-backend.c (probably should not exist in the first place). But
> we'll leave it there until we have better multi-worktree support in refs
> before we update it.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs/files-backend.c | 185 ++++++++++++++++++++++++++-------------------------
> 1 file changed, 94 insertions(+), 91 deletions(-)
In this step, files_path() is still "if refs->submodule field is
there, then use that to call strbuf_git_path_submodule() and
otherwise call strbuf_git_path()." That is a very sensible
refactoring for things like packed-refs-file in this hunk:
> static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *refs)
> {
> char *packed_refs_file;
> + struct strbuf sb = STRBUF_INIT;
>
> - if (refs->submodule)
> - packed_refs_file = git_pathdup_submodule(refs->submodule,
> - "packed-refs");
> - else
> - packed_refs_file = git_pathdup("packed-refs");
> + files_path(refs, &sb, "packed-refs");
> + packed_refs_file = strbuf_detach(&sb, NULL);
But the original code of some other changes do not follow that
pattern, e.g.
> @@ -1585,7 +1578,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
> *lock_p = lock = xcalloc(1, sizeof(*lock));
>
> lock->ref_name = xstrdup(refname);
> - strbuf_git_path(&ref_file, "%s", refname);
> + files_path(refs, &ref_file, "%s", refname);
Is it the right way to review these changes to make sure that a
conversion from the original that is an unconditional
strbuf_git_path() to files_path() happens only if the function is
"files-assert-main-repository" clean? lock_raw_ref() certainly is
one of those functions where the caller should not have a non-empty
submodule field in refs.
> @@ -2052,7 +2045,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
> if (flags & REF_DELETING)
> resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
>
> - strbuf_git_path(&ref_file, "%s", refname);
> + files_path(refs, &ref_file, "%s", refname);
So is this one; lock_ref_sha1_basic() is protected with assert-main-repo.
> @@ -2343,7 +2336,7 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
> * Remove empty parents, but spare refs/ and immediate subdirs.
> * Note: munges *name.
> */
> -static void try_remove_empty_parents(char *name)
> +static void try_remove_empty_parents(struct files_ref_store *refs, char *name)
> {
> char *p, *q;
> int i;
> @@ -2368,7 +2361,7 @@ static void try_remove_empty_parents(char *name)
> if (q == p)
> break;
> *q = '\0';
> - strbuf_git_path(&sb, "%s", name);
> + files_path(refs, &sb, "%s", name);
But here it gets iffy. try_remove_empty_parents() itself does not
assert, and its sole caller prune_ref() does not, either. The sole
caller of prune_ref() which is prune_refs() does not. As we climb
the call chain up, we reach files_pack_refs(). Am I confused to
doubt that the method is inherently main-repo only?
... ah, OK, files_downcast() at the beginning of pack_refs
forbids submodule. So this is safe.
> @@ -2462,7 +2455,7 @@ static int repack_without_refs(struct files_ref_store *refs,
> if (lock_packed_refs(refs, 0)) {
> struct strbuf sb = STRBUF_INIT;
>
> - strbuf_git_path(&sb, "packed-refs");
> + files_path(refs, &sb, "packed-refs");
This is safe, as repack_without_refs() asserts that it is main-repo
only.
> @@ -2558,17 +2551,17 @@ static int files_delete_refs(struct ref_store *ref_store,
> */
> #define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log"
>
> -static int rename_tmp_log(const char *newrefname)
> +static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname)
> {
The sole caller files_rename_ref() is main-repo only and that is
guaranteed when downcast is done.
> -static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
> +static int log_ref_setup(struct files_ref_store *refs, const char *refname,
> + struct strbuf *logfile, struct strbuf *err,
> + int force_create)
> {
> int logfd, oflags = O_APPEND | O_WRONLY;
>
> - strbuf_git_path(logfile, "logs/%s", refname);
> + files_path(refs, logfile, "logs/%s", refname);
This and friends of log_ref_write() eventually rolls up to
commit_ref_update() that has the main-repo only assertion, so they
should be safe.
Another entry point files_create_symref() via create_symref_locked()
also reaches log_ref_write() and friends but the safety is guaranteed
via the downcast that asserts.
OK, overall I really like the loss of "Check the validity but we do
not need the result" with this step. The same checks are still done
but the code looks much less hacky.
^ permalink raw reply
* Re: [PATCH v3 11/16] refs.c: kill register_ref_store(), add register_submodule_ref_store()
From: Junio C Hamano @ 2017-02-17 19:29 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Michael Haggerty, Johannes Schindelin, Ramsay Jones,
Stefan Beller, novalis
In-Reply-To: <20170217140436.17336-12-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This is the last function in this code (besides public API) that takes
> submodule argument and handles both main/submodule cases. Break it down,
> move main store registration in get_main_ref_store() and keep the rest
> in register_submodule_ref_store().
Very nice.
^ permalink raw reply
* Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
From: Jeff King @ 2017-02-17 19:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kyle Meyer, git
In-Reply-To: <xmqqk28ou2o1.fsf@gitster.mtv.corp.google.com>
On Fri, Feb 17, 2017 at 09:50:54AM -0800, Junio C Hamano wrote:
> > I see we actually already have a "logmsg" parameter. It already says
> > "Branch: renamed %s to %s". Could we just reuse that? I know that this
> > step is technically just the deletion, but I think it more accurately
> > describes the whole operation that the deletion is part of.
>
> True, but stepping back a bit,...
>
> Do we even want these "internal" delete_ref() invocations to be
> logged in HEAD's reflog? I understand that this is inside the
> implementation of renaming an old ref to a new ref, and reflog
> message given to delete_ref() would matter only if the HEAD happens
> to be pointing at old ref---but then HEAD will be repointed to the
> new ref by somebody else [*1*] that called this function to rename
> old to new and it _will_ log it. So I am not sure if it is a good
> thing to describe the deletion more readably with a message (which
> is what this patch does) in the first place. If we can just say
> "don't log this deletion event in HEAD's reflog", wouldn't that be
> more desirable?
Yes. I think the options are basically (in order of decreasing
preference in my opinion):
1. Log a rename entry (same sha1, but note the rename in the free-form
text).
2. Log a delete (sha1 goes to null) followed by a creation (from null
back to the original sha1).
3. Log nothing at all for HEAD.
This does half of (2). If we do the second half, then I'd prefer it to
(3). But if we can do (1), that is better still (IMHO).
> *1* Is the reason why the code in files_rename_ref() we are looking
> at does not adjust HEAD to point at the new ref is because it is
> just handing one ref-store and obviouvious to symrefs in other
> backends?
I'm actually confused about which bit of code is updating HEAD. I do not
see it either in files_rename_ref() or in the caller. Yet it clearly
happens. But that is the code that would know enough to do (1) or the
second half of (2) above.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
From: Jeff King @ 2017-02-17 19:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kyle Meyer, git
In-Reply-To: <20170217194350.prhp5joh33cbvwsd@sigill.intra.peff.net>
On Fri, Feb 17, 2017 at 02:43:50PM -0500, Jeff King wrote:
> Yes. I think the options are basically (in order of decreasing
> preference in my opinion):
>
> 1. Log a rename entry (same sha1, but note the rename in the free-form
> text).
>
> 2. Log a delete (sha1 goes to null) followed by a creation (from null
> back to the original sha1).
>
> 3. Log nothing at all for HEAD.
>
> This does half of (2). If we do the second half, then I'd prefer it to
> (3). But if we can do (1), that is better still (IMHO).
>
> > *1* Is the reason why the code in files_rename_ref() we are looking
> > at does not adjust HEAD to point at the new ref is because it is
> > just handing one ref-store and obviouvious to symrefs in other
> > backends?
>
> I'm actually confused about which bit of code is updating HEAD. I do not
> see it either in files_rename_ref() or in the caller. Yet it clearly
> happens. But that is the code that would know enough to do (1) or the
> second half of (2) above.
Ah, I found it. It's in replace_each_worktree_head_symref() these days,
which does not bother to pass a log message.
So I think the second half of (2) is probably something like the patch
below.
Thinking on it more, we probably _do_ want two entries. Because the
operations are not atomic, it's possible that we may end up in a
half-way state after the first entry is written. And when debugging such
a case, I'd much rather see the first half of the operation logged than
nothing at all.
-Peff
---
diff --git a/branch.c b/branch.c
index b955d4f31..5c12036b0 100644
--- a/branch.c
+++ b/branch.c
@@ -345,7 +345,8 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)
branch, wt->path);
}
-int replace_each_worktree_head_symref(const char *oldref, const char *newref)
+int replace_each_worktree_head_symref(const char *oldref, const char *newref,
+ const char *logmsg)
{
int ret = 0;
struct worktree **worktrees = get_worktrees(0);
@@ -358,7 +359,7 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref)
continue;
if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
- newref)) {
+ newref, logmsg)) {
ret = -1;
error(_("HEAD of working tree %s is not updated"),
worktrees[i]->path);
diff --git a/branch.h b/branch.h
index 3103eb9ad..b07788558 100644
--- a/branch.h
+++ b/branch.h
@@ -71,6 +71,7 @@ extern void die_if_checked_out(const char *branch, int ignore_current_worktree);
* This will be used when renaming a branch. Returns 0 if successful, non-zero
* otherwise.
*/
-extern int replace_each_worktree_head_symref(const char *oldref, const char *newref);
+extern int replace_each_worktree_head_symref(const char *oldref, const char *newref,
+ const char *logmsg);
#endif
diff --git a/builtin/branch.c b/builtin/branch.c
index cbaa6d03c..383005912 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -471,14 +471,15 @@ static void rename_branch(const char *oldname, const char *newname, int force)
if (rename_ref(oldref.buf, newref.buf, logmsg.buf))
die(_("Branch rename failed"));
- strbuf_release(&logmsg);
if (recovery)
warning(_("Renamed a misnamed branch '%s' away"), oldref.buf + 11);
- if (replace_each_worktree_head_symref(oldref.buf, newref.buf))
+ if (replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
+ strbuf_release(&logmsg);
+
strbuf_addf(&oldsection, "branch.%s", oldref.buf + 11);
strbuf_release(&oldref);
strbuf_addf(&newsection, "branch.%s", newref.buf + 11);
diff --git a/refs.h b/refs.h
index 9fbff90e7..b33035c4a 100644
--- a/refs.h
+++ b/refs.h
@@ -334,7 +334,8 @@ int create_symref(const char *refname, const char *target, const char *logmsg);
* $GIT_DIR points to.
* Return 0 if successful, non-zero otherwise.
* */
-int set_worktree_head_symref(const char *gitdir, const char *target);
+int set_worktree_head_symref(const char *gitdir, const char *target,
+ const char *logmsg);
enum action_on_err {
UPDATE_REFS_MSG_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4f1a88f6d..fa8d08e3c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3024,7 +3024,7 @@ static int files_create_symref(struct ref_store *ref_store,
return ret;
}
-int set_worktree_head_symref(const char *gitdir, const char *target)
+int set_worktree_head_symref(const char *gitdir, const char *target, const char *logmsg)
{
static struct lock_file head_lock;
struct ref_lock *lock;
@@ -3052,7 +3052,7 @@ int set_worktree_head_symref(const char *gitdir, const char *target)
lock->lk = &head_lock;
lock->ref_name = xstrdup(head_rel);
- ret = create_symref_locked(lock, head_rel, target, NULL);
+ ret = create_symref_locked(lock, head_rel, target, logmsg);
unlock_ref(lock); /* will free lock */
strbuf_release(&head_path);
^ permalink raw reply related
* Re: body-CC-comment regression
From: Junio C Hamano @ 2017-02-17 20:05 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Larry Finger
In-Reply-To: <vpq7f4owtbi.fsf@anie.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Johan Hovold <johan@kernel.org> writes:
>>
>>> That's precisely what the patch I posted earlier in the thread did.
>>
>> That's good. I didn't see any patch yet
>
> It's here:
>
> http://public-inbox.org/git/20170217110642.GD2625@localhost/
>
> but as I explained, this removes a feature suported since several major
> releases and we have no idea how many users may use the "mupliple emails
> in one field". The approach I proposed does not suffer from this.
OK, so you are aiming higher to please both parties, i.e. those who
place a single address but cruft at the end would get the cruft
stripped when we grab a usable address from the field, and those who
write two or more addresses would get all of these addresses?
That approach may still constrain what those in the former camp can
write in the "cruft" part, like they cannot write comma or semicolon
as part of the "cruft", no? If that does not pose a practical problem,
then I can imagine it would work well for people in both camps.
^ permalink raw reply
* Re: body-CC-comment regression
From: Matthieu Moy @ 2017-02-17 20:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Larry Finger
In-Reply-To: <xmqqd1egshv1.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> That approach may still constrain what those in the former camp can
> write in the "cruft" part, like they cannot write comma or semicolon
> as part of the "cruft", no?
Right. Indeed, this may be a problem since the use of "#" for stable
seem to include commit message, and they may contain commas.
So, maybe Johan's patch is better indeed.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: git alias for options
From: Jeff King @ 2017-02-17 20:42 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: hIpPy, Git Mailing List
In-Reply-To: <CACBZZX4Zmwb8ZHGBXCpm6=yH_uxc-K1X1vv1jQ+wwnUPneqM4A@mail.gmail.com>
On Fri, Feb 17, 2017 at 02:50:23PM +0100, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Feb 17, 2017 at 9:23 AM, hIpPy <hippy2981@gmail.com> wrote:
> > Git has aliases for git commands. Is there a (an inbuilt) way to alias
> > options? If not, what is the reason?
>
> This has long been on my wishlist, because there's a lot of
> copy/pasted logic all over Git to make git foo --whatever aliased to
> foo.whatever in the config, but only for some options.
>
> It should ideally be part of something every option just supports, via
> the getopts struct.
>
> However, it can't allow you to modify whatever option you want,
> because some things like git-commit-tree should not be customized
> based on config, it would break things that rely on the plumbing
> commands.
>
> So it would have to be a whitelist for each option we allow to be
> configured like this via the getopts struct.
>
> Also there are surely other edge cases, like maybe the config option
> now doesn't 1=1 map to the name for the option in some cases, or the
> flag should be config-able but is has no long form (which we'd like
> for the config), then we'd want to add that etc.
I think your idea is roughly equivalent in functionality to just
allowing aliases to override command names. E.g., anything you could do
with:
[log]
follow = true
decorate = false
could be done with:
[alias]
log = "log --follow --no-decorate"
The reason we have historically not allowed that is for the
"commit-tree" plumbing reason you gave above. One option would be to
relax it for a whitelist of porcelain commands. Then your whitelist at
least only has to cover commands, and not each individual option.
I think there are a lot of corner cases in that whitelist, though. A lot
of commands serve dual porcelain/plumbing purposes. E.g., "log" and
"tag" are commonly used by both humans and by scripts.
A first step in that direction would probably be an environment variable
to tell Git to suppress command-aliases. Scripts would set that to
ensure a more vanilla experience. It doesn't fix _existing_ scripts, but
if that option were introduced, then over time scripts would start to
use it. Then eventually it would be safe(r) to introduce something like
command aliases.
But nobody has ever taken that first step, so here we are.
-Peff
^ permalink raw reply
* Re: dotfiles in git template dir are not copied
From: Jeff King @ 2017-02-17 20:44 UTC (permalink / raw)
To: greg0ire; +Cc: git
In-Reply-To: <ebd661c3-7d99-54d2-dda9-09c4a76cfe93@greg0ire.fr>
On Fri, Feb 17, 2017 at 10:31:37AM +0100, greg0ire wrote:
> I noticed yesterday that dotfiles inside the directory configured in
> init.templatedir are not copied when creating a new repository.
>
> Here is the line I think is responsible for this behavior :
> https://github.com/git/git/blob/master/builtin/init-db.c#L48
>
> Is it a bug or a feature?
I think it's probably a feature. You could, for example, have your
template directory itself be a git repository. You would not want to
copy the ".git" directory around.
I wouldn't be surprised if the documentation could be improved, though.
-Peff
^ permalink raw reply
* Re: [PATCH v2 00/16] Remove submodule from files-backend.c
From: Junio C Hamano @ 2017-02-17 20:49 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Michael Haggerty, Johannes Schindelin, Ramsay Jones,
Stefan Beller, novalis
In-Reply-To: <xmqqzihksm0i.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> I'll be sending two more follow-up series, if you are interested, soon:
>>
>> 1) v2 of nd/worktree-gc-protection
>> ...
>> 2) the real worktree-gc-protection
>>
>> This series adds a bunch of new refs API, enough for revision.c to
>> traverses all sorts of refs with a ref store instead of "submodule".
>> Many _submodule API are removed as a result because they no longer
>> have any callers (yay!). One of them remains though.
>
> Yay indeed.
Having said all that, I have a bad news.
I won't be able to queue this as-is, as this heavily conflicts with
mh/ref-remove-empty-directory that has already been cooking in
'next' for some time. I started trying conflict resolution myself
and it seemed doable, but stopped after finishing perhaps 40% of it,
as I cannot afford the same amount of time for conflict resolution
every time the series gets rerolled.
If the topic is rebased on the result of doing:
- checking out master^0
- merging mh/ref-remove-empty-directory on it
- mergeing mh/submodule-hash on the result
I suspect that we might be able start cooking it in 'pu' sooner, but
I cannot spend any more time on this topic today so I'd stop analyzing
the situation for now.
^ permalink raw reply
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Jeff King @ 2017-02-17 20:54 UTC (permalink / raw)
To: Michael Haggerty
Cc: Andreas Schwab, Junio C Hamano, Jáchym Barvínek, git
In-Reply-To: <64eedabd-c0de-a7e0-8d98-ad23a9625b45@alum.mit.edu>
On Fri, Feb 17, 2017 at 11:42:25AM +0100, Michael Haggerty wrote:
> On 02/17/2017 09:07 AM, Jeff King wrote:
> > [...]
> > That's similar to what I wrote earlier, but if we don't mind overwriting
> > errno unconditionally, I think just:
> >
> > errno = EIO; /* covers ferror(), overwritten by failing fclose() */
> > err |= ferror(fp);
> > err |= fclose(fp);
> >
> > does the same thing.
>
> True; I'd forgotten the convention that non-failing functions are
> allowed to change errno. Your solution is obviously simpler and faster.
I guess we are simultaneously assuming that it is OK to munge errno on
success in our function, but that fclose() will not do so. Which seems a
bit hypocritical. Maybe the "if" dance is better.
-Peff
^ permalink raw reply
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Jeff King @ 2017-02-17 21:07 UTC (permalink / raw)
To: Michael Haggerty
Cc: Andreas Schwab, Junio C Hamano, Jáchym Barvínek, git
In-Reply-To: <20170217205442.wnldfsxbj3dnnqvj@sigill.intra.peff.net>
On Fri, Feb 17, 2017 at 03:54:42PM -0500, Jeff King wrote:
> I guess we are simultaneously assuming that it is OK to munge errno on
> success in our function, but that fclose() will not do so. Which seems a
> bit hypocritical. Maybe the "if" dance is better.
So here's that patch with a justification.
At this point, this snippet of code would be appropriate to pull into
xfclose() if we wanted. But possibly that is the wrong direction, as it
encourages callers to do:
if (xfclose(fp))
err = error_errno("failure writing to ...");
when they could do:
if (ferror(fp))
err = error("failure writing to ...");
if (fclose(fp))
err = error_errno("failure writing to ...");
While longer, it's arguably better for them to distinguish the two
cases. It's only worth doing the errno magic when the close is deep
inside a callstack, and passing out the two cases is awkward.
-- >8 --
Subject: tempfile: set errno to a known value before calling ferror()
In close_tempfile(), we return an error if ferror()
indicated a previous failure, or if fclose() failed. In the
latter case, errno is set and it is useful for callers to
report it.
However, if _only_ ferror() triggers, then the value of
errno is based on whatever syscall happened to last fail,
which may not be related to our filehandle at all. A caller
cannot tell the difference between the two cases, and may
use "die_errno()" or similar to report a nonsense errno value.
One solution would be to actually pass back separate return
values for the two cases, so a caller can write a more
appropriate message for each case. But that makes the
interface clunky.
Instead, let's just set errno to the generic EIO in this case.
That's not as descriptive as we'd like, but at least it's
predictable. So it's better than the status quo in all cases
but one: when the last syscall really did involve a failure
on our filehandle, we'll be wiping that out. But that's a
fragile thing for us to rely on.
In any case, we'll let the errno result from fclose() take
precedence over our value, as we know that's recent and
accurate (and many I/O errors will persist through the
fclose anyway).
Signed-off-by: Jeff King <peff@peff.net>
---
tempfile.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/tempfile.c b/tempfile.c
index ffcc27237..684371067 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -247,8 +247,13 @@ int close_tempfile(struct tempfile *tempfile)
tempfile->fd = -1;
if (fp) {
tempfile->fp = NULL;
- err = ferror(fp);
- err |= fclose(fp);
+ if (ferror(fp)) {
+ err = -1;
+ if (!fclose(fp))
+ errno = EIO;
+ } else {
+ err = fclose(fp);
+ }
} else {
err = close(fd);
}
--
2.12.0.rc1.612.ga5f664feb
^ permalink raw reply related
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Junio C Hamano @ 2017-02-17 21:17 UTC (permalink / raw)
To: Jeff King
Cc: Michael Haggerty, Andreas Schwab, Jáchym Barvínek, git
In-Reply-To: <20170217205442.wnldfsxbj3dnnqvj@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Feb 17, 2017 at 11:42:25AM +0100, Michael Haggerty wrote:
>
>> On 02/17/2017 09:07 AM, Jeff King wrote:
>> > [...]
>> > That's similar to what I wrote earlier, but if we don't mind overwriting
>> > errno unconditionally, I think just:
>> >
>> > errno = EIO; /* covers ferror(), overwritten by failing fclose() */
>> > err |= ferror(fp);
>> > err |= fclose(fp);
>> >
>> > does the same thing.
>>
>> True; I'd forgotten the convention that non-failing functions are
>> allowed to change errno. Your solution is obviously simpler and faster.
>
> I guess we are simultaneously assuming that it is OK to munge errno on
> success in our function, but that fclose() will not do so. Which seems a
> bit hypocritical. Maybe the "if" dance is better.
Yes. When both ferror() and fclose() are successful, we would
prefer to see the original errno unmolested, so the "if" dance,
even though it looks uglier, is better. The ugliness is limited
to the implementation anyway ;-)
But it does look ugly, especially when nested inside the existing
code like so.
Stepping back a bit, would this be really needed? Even if the ferror()
does not update errno, the original stdio operation that failed
would have, no?
-- >8 --
Subject: close_tempfile(): set errno when ferror() notices a previous error
In close_tempfile(), we may notice that previous stdio operations
failed when we inspect ferror(tempfile->fp). As ferror() does not
set errno, and the caller of close_tempfile(), since it encountered
and ignored the original error, is likely to have called other
system library functions to cause errno to be modified, the caller
cannot really tell anything meaningful by looking at errno after we
return an error from here.
Set errno to an arbitrary value EIO when ferror() sees an error but
fclose() succeeds. If fclose() fails, we just let the caller see
errno from that failure.
---
tempfile.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/tempfile.c b/tempfile.c
index ffcc272375..d2c6de83a9 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -247,8 +247,20 @@ int close_tempfile(struct tempfile *tempfile)
tempfile->fd = -1;
if (fp) {
tempfile->fp = NULL;
- err = ferror(fp);
- err |= fclose(fp);
+ if (ferror(fp)) {
+ err = -1;
+ if (!fclose(fp))
+ /*
+ * There was some error detected by ferror()
+ * but it is likely that the true errno has
+ * long gone. Leave something generic to make
+ * it clear that the caller cannot rely on errno
+ * at this point.
+ */
+ errno = EIO;
+ } else {
+ err = fclose(fp);
+ }
} else {
err = close(fd);
}
^ permalink raw reply related
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Jeff King @ 2017-02-17 21:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, Andreas Schwab, Jáchym Barvínek, git
In-Reply-To: <xmqq37fcsejx.fsf@gitster.mtv.corp.google.com>
On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:
> > I guess we are simultaneously assuming that it is OK to munge errno on
> > success in our function, but that fclose() will not do so. Which seems a
> > bit hypocritical. Maybe the "if" dance is better.
>
> Yes. When both ferror() and fclose() are successful, we would
> prefer to see the original errno unmolested, so the "if" dance,
> even though it looks uglier, is better. The ugliness is limited
> to the implementation anyway ;-)
>
> But it does look ugly, especially when nested inside the existing
> code like so.
I think our emails crossed, but our patches are obviously quite similar.
My commit message maybe explains a bit more of my thinking.
> Stepping back a bit, would this be really needed? Even if the ferror()
> does not update errno, the original stdio operation that failed
> would have, no?
Sure, but we have no clue what happened in between.
-Peff
^ permalink raw reply
* git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Mike Crowe @ 2017-02-17 21:26 UTC (permalink / raw)
To: git
If "git diff --quiet" finds it necessary to compare actual file contents,
and a file requires CRLF conversion, then it incorrectly exits with an exit
code of 1 even if there have been no changes.
The patch below adds a test file that shows the problem.
The first test of diff without --quiet correctly has an exit status of zero
on both invocations.
The second test of diff with --quiet has an exit code of zero on the first
invocation, but an exit code of one on the second invocation. Further
invocations (not included in the test) may yield an exit code of 1. Calling
"git status" always fixes things.
(The touching with "tomorrow" was my attempt to avoid the sleep, but that
didn't seem to help - it appears that time must pass in order to ensure
that the cache is not used.)
The culprit would appear to be in diff_filespec_check_stat_unmatch where it
assumes that the files are different if their sizes are different:
if (!DIFF_FILE_VALID(p->one) || /* (1) */
!DIFF_FILE_VALID(p->two) ||
(p->one->oid_valid && p->two->oid_valid) ||
(p->one->mode != p->two->mode) ||
diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||
diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||
(p->one->size != p->two->size) ||
!diff_filespec_is_identical(p->one, p->two)) /* (2) */
p->skip_stat_unmatch_result = 1;
In the failing case p->one->size == 14 and p->two->size == 12.
Mike.
diff --git a/t/t4063-diff-converted.sh b/t/t4063-diff-converted.sh
new file mode 100755
index 0000000..a108dfb
--- /dev/null
+++ b/t/t4063-diff-converted.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+#
+# Copyright (c) 2017 Mike Crowe
+#
+
+test_description='git diff with files that require CRLF conversion'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ echo "* text=auto" > .gitattributes &&
+ printf "Hello\r\nWorld\r\n" > crlf.txt &&
+ git add .gitattributes crlf.txt &&
+ git commit -m "initial"
+'
+test_expect_success 'noisy diff works on file that requires CRLF conversion' '
+ git status >/dev/null &&
+ git diff >/dev/null &&
+ sleep 1 &&
+ touch --date=tomorrow crlf.txt &&
+ git diff >/dev/null
+'
+# The sleep is required for reasons I don't fully understand
+test_expect_success 'quiet diff works on file that requires CRLF conversion with no changes' '
+ git status &&
+ git diff --quiet &&
+ sleep 1 &&
+ touch --date=tomorrow crlf.txt &&
+ git diff --quiet
+'
+
+test_done
^ permalink raw reply related
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Junio C Hamano @ 2017-02-17 21:42 UTC (permalink / raw)
To: Jeff King
Cc: Michael Haggerty, Andreas Schwab, Jáchym Barvínek, git
In-Reply-To: <20170217212106.bew6krtb7pqpi3rr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:
>
>> Stepping back a bit, would this be really needed? Even if the ferror()
>> does not update errno, the original stdio operation that failed
>> would have, no?
>
> Sure, but we have no clue what happened in between.
Hmm, so we are protecting against somebody who does "errno = 0"
explicitly, because she knows that she's dealt with the error from
stdio earlier? Such a careful person would have called clearerr()
as well, I would guess.
So let's assume we only care about the case where some other error
was detected and errno was updated by a system library call.
> I think our emails crossed, but our patches are obviously quite similar.
> My commit message maybe explains a bit more of my thinking.
Yes, but ;-)
If we are trying to make sure that the caller would not say "failed
to close tempfile: ERRNO" with an ERRNO that is unrelated to any
stdio opration, I am not sure if the patch improves things. The
caller did not fail to close (most likely we successfully closed
it), and no matter what futzing we do to errno, the message supplied
by such a caller will not be improved.
If the caller used "noticed an earlier error while closing tempfile:
ERRNO", such a message would describe the situation more correctly,
but then ERRNO that is not about stdio is probably acceptable in the
context of that message (the original ERRNO might be ENOSPC that is
even more specific than EIO, FWIW). So I am not sure if the things
will improve from the status quo.
It's easy for us to either apply the patch and be done with it (or
drop and do the same), and in the bigger picture it wouldn't make
that much of a difference, I would think, so I can go either way.
^ permalink raw reply
* Re: [PATCH v2 00/19] object_id part 6
From: brian m. carlson @ 2017-02-17 21:45 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git, Jeff King
In-Reply-To: <3644df30-71e2-584b-ebed-ae117a5ded3f@alum.mit.edu>
[-- Attachment #1: Type: text/plain, Size: 2532 bytes --]
On Fri, Feb 17, 2017 at 10:55:03AM +0100, Michael Haggerty wrote:
> On 02/14/2017 03:31 AM, brian m. carlson wrote:
> > This is another series in the continuing conversion to struct object_id.
> >
> > This series converts more of the builtin directory and some of the refs
> > code to use struct object_id. Additionally, it implements an
> > nth_packed_object_oid function which provides a struct object_id version
> > of the nth_packed_object function, and a parse_oid_hex function that
> > makes parsing easier.
> >
> > The patch to use parse_oid_hex in the refs code has been split out into
> > its own patch, just because I'm wary of that code and potentially
> > breaking things, and I want it to be easy to revert in case things go
> > wrong. I have no reason to believe it is anything other than fully
> > functional, however.
> >
> > Changes from v1:
> > * Implement parse_oid_hex and use it.
> > * Make nth_packed_object_oid take a variable into which to store the
> > object ID. This avoids concerns about unsafe casts.
> > * Rebase on master.
>
> Thanks as always for working on this!
>
> I skimmed over the patches (looking more carefully at the refs-related
> ones) and left a few minor comments but didn't find anything serious.
I'll send out a new series shortly with those changes.
> I'm curious; what fraction of the overall convert-to-object_id campaign
> do you estimate is done so far? Are you getting close to the promised
> land yet?
So I think that the current scope left is best estimated by the
following command:
git grep -P 'unsigned char\s+(\*|.*20)' | grep -v '^Documentation'
So there are approximately 1200 call sites left, which is quite a bit of
work. I estimate between the work I've done and other people's
refactoring work (such as the refs backend refactor), we're about 40%
done.
I'm hoping to send out more smaller series in the future, since running
make test on a huge series takes a long time.
Part of the problem is that some places are almost completely
convertible, except for one or two components that rely on something
like sha1_array. I have patches somewhere that convert that to
oid_array, but I'm not sure how well they'll be received at this point.
I think converting a lot of shallow-related code requires that
conversion, though.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: Re: dotfiles in git template dir are not copied
From: Grégoire PARIS @ 2017-02-17 21:42 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170217204411.2yixhuazgczxmmxa@sigill.intra.peff.net>
> You could, for example, have your template directory itself be a git
repository.
I can and I do and indeed, that might be the reason behind this.
I made a PR to document this : https://github.com/git/git/pull/325
--
greg0ire
^ permalink raw reply
* Re: [PATCH] l10n: de.po: translate 241 messages
From: Phillip Sz @ 2017-02-17 21:56 UTC (permalink / raw)
To: Ralf Thielow, git
Cc: Thomas Rast, Jan Krüger, Christian Stimming,
Matthias Rüster, Magnus Görlitz
In-Reply-To: <20170217174132.8816-1-ralf.thielow@gmail.com>
Hey,
> @@ -916,17 +916,17 @@ msgstr ""
> msgid ""
> "The merge base %s is %s.\n"
> "This means the first '%s' commit is between %s and [%s].\n"
> msgstr ""
> "Die Merge-Basis %s ist %s.\n"
> "Das bedeutet, der erste '%s' Commit befindet sich zwischen %s und [%s]\n"
"Das bedeutet, der erste '%s' Commit befindet sich zwischen %s und [%s].\n"
Period is missing.
> #: remote.c:2092
> #, c-format
> msgid "Your branch is ahead of '%s' by %d commit.\n"
> msgid_plural "Your branch is ahead of '%s' by %d commits.\n"
> -msgstr[0] "Ihr Branch ist vor '%s' um %d Commit.\n"
> -msgstr[1] "Ihr Branch ist vor '%s' um %d Commits.\n"
> +msgstr[0] "Ihr Branch ist %2$d Commit vor '%1$s'.\n"
> +msgstr[1] "Ihr Branch ist %2$d Commits vor '%1$s'.\n"
>
Does this "%2$d" works and why not use '%s'?
> #: sequencer.c:840
> -#, fuzzy
> msgid "could not read HEAD's commit message"
> -msgstr "Konnte Commit-Beschreibung nicht lesen: %s"
> +msgstr "Konnte Commit-Beschreibung von HEAD nicht lesen"
>
> #: sequencer.c:846
> -#, fuzzy, c-format
> +#, c-format
> msgid "cannot write '%s'"
> -msgstr "kann '%s' nicht erstellen"
> +msgstr "kann '%s' nicht schreiben"
I think we should either use "kann" or "konnte".
We have used both and maybe we should unify it? What do you think?
> #: sequencer.c:1341
> -#, fuzzy
> msgid "please fix this using 'git rebase --edit-todo'."
> msgstr "Bitte beheben Sie das, indem Sie 'git rebase --edit-todo' ausführen."
Maybe: "Bitte beheben Sie dieses, indem Sie 'git rebase --edit-todo'
ausführen."
> #: git-add--interactive.perl:1074
> #, perl-format
> msgid ""
> "---\n"
> "To remove '%s' lines, make them ' ' lines (context).\n"
> "To remove '%s' lines, delete them.\n"
> "Lines starting with %s will be removed.\n"
> msgstr ""
> +"---\n"
> +"Um '%s' Zeilen zu entfernen, machen Sie aus diesen ' ' Zeilen (Kontext).\n"
> +"Um '%s' Zeilen zu entferenen, löschen Sie diese.\n"
> +"Zeilen, die mit %s beginnen, werden entfernt.\n"
>
"Um '%s' Zeilen zu entfernen, löschen Sie diese.\n"
Anyway thanks a lot for your awesome work!
Phillip
^ permalink raw reply
* Re: git diff --quiet exits with 1 on clean tree with CRLF conversions
From: Junio C Hamano @ 2017-02-17 22:05 UTC (permalink / raw)
To: Mike Crowe; +Cc: git
In-Reply-To: <20170217212633.GA24937@mcrowe.com>
Mike Crowe <mac@mcrowe.com> writes:
> If "git diff --quiet" finds it necessary to compare actual file contents,
> and a file requires CRLF conversion, then it incorrectly exits with an exit
> code of 1 even if there have been no changes.
>
> The patch below adds a test file that shows the problem.
If "git diff" does not show any output and "git diff --exit-code" or
"git diff --quiet" says there are differences, then it is a bug.
I would however have expected that any culprit would involve a code
that says "under QUICK option, we do not have to bother doing
this". The part you quoted:
> if (!DIFF_FILE_VALID(p->one) || /* (1) */
> !DIFF_FILE_VALID(p->two) ||
> (p->one->oid_valid && p->two->oid_valid) ||
> (p->one->mode != p->two->mode) ||
> diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||
> diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||
> (p->one->size != p->two->size) ||
> !diff_filespec_is_identical(p->one, p->two)) /* (2) */
> p->skip_stat_unmatch_result = 1;
is used by "git diff" with and without "--quiet", afacr, so I
suspect that the bug lies somewhere else.
^ permalink raw reply
* Re: dotfiles in git template dir are not copied
From: Junio C Hamano @ 2017-02-17 22:12 UTC (permalink / raw)
To: Grégoire PARIS; +Cc: Jeff King, git
In-Reply-To: <2bae8d8a-f0bf-fa8b-8ce4-6880d3490b43@greg0ire.fr>
Grégoire PARIS <postmaster@greg0ire.fr> writes:
>> You could, for example, have your template directory itself be a git
> repository.
>
> I can and I do and indeed, that might be the reason behind this.
An embedded .git was _not_ the reason behind the current behaviour
when we wrote it. The primary reason is because we did not ship and
did not plan to ship anything .dot in our standard template set and
there wasn't any reason to copy what we were not going to ever ship
;-).
As a justification after the fact, "you wouldn't want to copy .git
do you?" is a good one, though, and I do not think we should change
the behaviour to copy files whose names begin with a dot.
^ permalink raw reply
* Re: git alias for options
From: Jeff King @ 2017-02-17 22:13 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: hIpPy, Git Mailing List
In-Reply-To: <CACBZZX7nJkRmSxTdvhgXz5Zuk0KeovSZM1D_eGqbBn7i20+ePQ@mail.gmail.com>
On Fri, Feb 17, 2017 at 11:10:08PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > A first step in that direction would probably be an environment variable
> > to tell Git to suppress command-aliases. Scripts would set that to
> > ensure a more vanilla experience. It doesn't fix _existing_ scripts, but
> > if that option were introduced, then over time scripts would start to
> > use it. Then eventually it would be safe(r) to introduce something like
> > command aliases.
>
> The most gentle first step would be to try to turn the existing config
> options where you can override cli-options into some declarative thing
> from the current ad-hoc code we have for each one.
>
> That would be no change in behavior, but would make it easier to
> migrate more things in the future.
Yeah, I'd agree with that. It does not change anything for the users,
but it makes the implementation less annoying.
-Peff
^ permalink raw reply
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Jeff King @ 2017-02-17 22:10 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, Andreas Schwab, Jáchym Barvínek, git
In-Reply-To: <xmqqy3x4qyte.fsf@gitster.mtv.corp.google.com>
On Fri, Feb 17, 2017 at 01:42:21PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:
> >
> >> Stepping back a bit, would this be really needed? Even if the ferror()
> >> does not update errno, the original stdio operation that failed
> >> would have, no?
> >
> > Sure, but we have no clue what happened in between.
>
> Hmm, so we are protecting against somebody who does "errno = 0"
> explicitly, because she knows that she's dealt with the error from
> stdio earlier? Such a careful person would have called clearerr()
> as well, I would guess.
I'm not sure I understand what you are saying here. If somebody calls
clearerr(), our ferror() handling does not trigger at all, and do not
care what is in errno either way. They can reset errno or not when they
clearerr(), but it is not relevant.
If you are asking about somebody who sets errno to "0" and _doesn't_
call clearerr(), then I don't know what that person is trying to
accomplish. Setting errno to "0" is not the right way to clear an error.
And they certainly should not be relying on it not to get overwritten
before we make it to the final ferror()/fclose().
> So let's assume we only care about the case where some other error
> was detected and errno was updated by a system library call.
Right.
> > I think our emails crossed, but our patches are obviously quite similar.
> > My commit message maybe explains a bit more of my thinking.
>
> Yes, but ;-)
>
> If we are trying to make sure that the caller would not say "failed
> to close tempfile: ERRNO" with an ERRNO that is unrelated to any
> stdio opration, I am not sure if the patch improves things. The
> caller did not fail to close (most likely we successfully closed
> it), and no matter what futzing we do to errno, the message supplied
> by such a caller will not be improved.
Right. EIO is almost certainly _not_ the error we saw. But I would
rather consistently say "I/O error" and have the user scratch their
head, look up this thread, and say "ah, it was probably a deferred
error", as opposed to the alternative: the user sees something
nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,
may change from run to run based on what other code runs or fails in
between.
> If the caller used "noticed an earlier error while closing tempfile:
> ERRNO", such a message would describe the situation more correctly,
> but then ERRNO that is not about stdio is probably acceptable in the
> context of that message (the original ERRNO might be ENOSPC that is
> even more specific than EIO, FWIW). So I am not sure if the things
> will improve from the status quo.
Yes, that's I suggested that xfclose() is probably not a good direction.
The _best_ thing we can do is have the caller not report errno at all
(or even say "there was an earlier error, I have no idea what errno
was"). And xfclose() works in the opposite direction.
The only reason I do not think we should do so for close_tempfile() is
that the fclose is typically far away from the code that actually calls
error(). We'd have to pass the tristate (success, fail, fail-with-errno)
state up through the stack (most of the calls indirectly come from
commit_lock_file(), I would think).
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
From: Junio C Hamano @ 2017-02-17 22:18 UTC (permalink / raw)
To: Jeff King; +Cc: Kyle Meyer, git
In-Reply-To: <20170217195549.z6uyy7hbbhj5avh7@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Thinking on it more, we probably _do_ want two entries. Because the
> operations are not atomic, it's possible that we may end up in a
> half-way state after the first entry is written. And when debugging such
> a case, I'd much rather see the first half of the operation logged than
> nothing at all.
Yes, that sounds right.
^ permalink raw reply
* Re: git alias for options
From: Ævar Arnfjörð Bjarmason @ 2017-02-17 22:10 UTC (permalink / raw)
To: Jeff King; +Cc: hIpPy, Git Mailing List
In-Reply-To: <20170217204227.kreormjoo5lr6zu4@sigill.intra.peff.net>
On Fri, Feb 17, 2017 at 9:42 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Feb 17, 2017 at 02:50:23PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> On Fri, Feb 17, 2017 at 9:23 AM, hIpPy <hippy2981@gmail.com> wrote:
>> > Git has aliases for git commands. Is there a (an inbuilt) way to alias
>> > options? If not, what is the reason?
>>
>> This has long been on my wishlist, because there's a lot of
>> copy/pasted logic all over Git to make git foo --whatever aliased to
>> foo.whatever in the config, but only for some options.
>>
>> It should ideally be part of something every option just supports, via
>> the getopts struct.
>>
>> However, it can't allow you to modify whatever option you want,
>> because some things like git-commit-tree should not be customized
>> based on config, it would break things that rely on the plumbing
>> commands.
>>
>> So it would have to be a whitelist for each option we allow to be
>> configured like this via the getopts struct.
>>
>> Also there are surely other edge cases, like maybe the config option
>> now doesn't 1=1 map to the name for the option in some cases, or the
>> flag should be config-able but is has no long form (which we'd like
>> for the config), then we'd want to add that etc.
>
> I think your idea is roughly equivalent in functionality to just
> allowing aliases to override command names. E.g., anything you could do
> with:
>
> [log]
> follow = true
> decorate = false
>
> could be done with:
>
> [alias]
> log = "log --follow --no-decorate"
Indeed, exact same thing, different syntax. Mostly I like this
suggestion better, although the bad side of it is that it's not as
easy to introspect with a dump of git-config -l.
> The reason we have historically not allowed that is for the
> "commit-tree" plumbing reason you gave above. One option would be to
> relax it for a whitelist of porcelain commands. Then your whitelist at
> least only has to cover commands, and not each individual option.
>
> I think there are a lot of corner cases in that whitelist, though. A lot
> of commands serve dual porcelain/plumbing purposes. E.g., "log" and
> "tag" are commonly used by both humans and by scripts.
>
> A first step in that direction would probably be an environment variable
> to tell Git to suppress command-aliases. Scripts would set that to
> ensure a more vanilla experience. It doesn't fix _existing_ scripts, but
> if that option were introduced, then over time scripts would start to
> use it. Then eventually it would be safe(r) to introduce something like
> command aliases.
The most gentle first step would be to try to turn the existing config
options where you can override cli-options into some declarative thing
from the current ad-hoc code we have for each one.
That would be no change in behavior, but would make it easier to
migrate more things in the future.
Anyway, words are cheap. Just replied because to the extent that hIpPy
wants to work on this I thought I'd sprinkle some historical caveats
from memory. Other than that no point in keeping talking about this
without patches.
^ permalink raw reply
* Re: body-CC-comment regression
From: Junio C Hamano @ 2017-02-17 22:22 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Johan Hovold, git, Jeff King, Kevin Daudt, Larry Finger
In-Reply-To: <vpq1suwvab9.fsf@anie.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> That approach may still constrain what those in the former camp can
>> write in the "cruft" part, like they cannot write comma or semicolon
>> as part of the "cruft", no?
>
> Right. Indeed, this may be a problem since the use of "#" for stable
> seem to include commit message, and they may contain commas.
>
> So, maybe Johan's patch is better indeed.
OK, so I'll queue that one with your Ack for now so that we won't
forget. I guess we still want a few tests?
Thanks.
^ 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