From: Erick Mattos Date: Thu, 10 Jun 2010 16:48:40 GMT Subject: Re: [PATCH next v2] log_ref_setup: don't return stack-allocated array Message-ID: In-Reply-To: <47daf53b6b2cc25cc013c5f2183e309a671dc9d3.1276174233.git.trast@student.ethz.ch> Hi there, 2010/6/10 Thomas Rast > > 859c301 (refs: split log_ref_write logic into log_ref_setup, > 2010-05-21) refactors the stack allocation of the log_file array into > the new log_ref_setup() function, but passes it back to the caller. > > Since the original intent seems to have been to split the work between > log_ref_setup and log_ref_write, make it the caller's responsibility > to allocate the buffer. > > Signed-off-by: Thomas Rast > Reported-by: Ævar Arnfjörð Bjarmason > --- > > Sorry for the first one, that was completely botched and didn't even > compile. > > This one does, and as an added bonus also passes some tests. > >  builtin/checkout.c |    4 ++-- >  refs.c             |   26 ++++++++++++-------------- >  refs.h             |    2 +- >  3 files changed, 15 insertions(+), 17 deletions(-) > > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 5107eda..1994be9 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -496,12 +496,12 @@ static void update_refs_for_switch(struct checkout_opts *opts, >                if (opts->new_orphan_branch) { >                        if (opts->new_branch_log && !log_all_ref_updates) { >                                int temp; > -                               char *log_file; > +                               char log_file[PATH_MAX]; >                                char *ref_name = mkpath("refs/heads/%s", opts->new_orphan_branch); > >                                temp = log_all_ref_updates; >                                log_all_ref_updates = 1; > -                               if (log_ref_setup(ref_name, &log_file)) { > +                               if (log_ref_setup(ref_name, log_file, sizeof(log_file))) { >                                        fprintf(stderr, "Can not do reflog for '%s'\n", >                                            opts->new_orphan_branch); >                                        log_all_ref_updates = temp; > diff --git a/refs.c b/refs.c > index 3436649..6f486ae 100644 > --- a/refs.c > +++ b/refs.c > @@ -1262,43 +1262,41 @@ static int copy_msg(char *buf, const char *msg) >        return cp - buf; >  } > > -int log_ref_setup(const char *ref_name, char **log_file) > +int log_ref_setup(const char *ref_name, char *logfile, int bufsize) >  { >        int logfd, oflags = O_APPEND | O_WRONLY; > -       char logfile[PATH_MAX]; > > -       git_snpath(logfile, sizeof(logfile), "logs/%s", ref_name); > -       *log_file = logfile; > +       git_snpath(logfile, bufsize, "logs/%s", ref_name); >        if (log_all_ref_updates && >            (!prefixcmp(ref_name, "refs/heads/") || >             !prefixcmp(ref_name, "refs/remotes/") || >             !prefixcmp(ref_name, "refs/notes/") || >             !strcmp(ref_name, "HEAD"))) { > -               if (safe_create_leading_directories(*log_file) < 0) > +               if (safe_create_leading_directories(logfile) < 0) >                        return error("unable to create directory for %s", > -                                    *log_file); > +                                    logfile); >                oflags |= O_CREAT; >        } > > -       logfd = open(*log_file, oflags, 0666); > +       logfd = open(logfile, oflags, 0666); >        if (logfd < 0) { >                if (!(oflags & O_CREAT) && errno == ENOENT) >                        return 0; > >                if ((oflags & O_CREAT) && errno == EISDIR) { > -                       if (remove_empty_directories(*log_file)) { > +                       if (remove_empty_directories(logfile)) { >                                return error("There are still logs under '%s'", > -                                            *log_file); > +                                            logfile); >                        } > -                       logfd = open(*log_file, oflags, 0666); > +                       logfd = open(logfile, oflags, 0666); >                } > >                if (logfd < 0) >                        return error("Unable to append to %s: %s", > -                                    *log_file, strerror(errno)); > +                                    logfile, strerror(errno)); >        } > > -       adjust_shared_perm(*log_file); > +       adjust_shared_perm(logfile); >        close(logfd); >        return 0; >  } > @@ -1309,14 +1307,14 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1, >        int logfd, result, written, oflags = O_APPEND | O_WRONLY; >        unsigned maxlen, len; >        int msglen; > -       char *log_file; > +       char log_file[PATH_MAX]; >        char *logrec; >        const char *committer; > >        if (log_all_ref_updates < 0) >                log_all_ref_updates = !is_bare_repository(); > > -       result = log_ref_setup(ref_name, &log_file); > +       result = log_ref_setup(ref_name, log_file, sizeof(log_file)); >        if (result) >                return result; > > diff --git a/refs.h b/refs.h > index 594c9d9..762ce50 100644 > --- a/refs.h > +++ b/refs.h > @@ -69,7 +69,7 @@ extern void unlock_ref(struct ref_lock *lock); >  extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg); > >  /** Setup reflog before using. **/ > -int log_ref_setup(const char *ref_name, char **log_file); > +int log_ref_setup(const char *ref_name, char *logfile, int bufsize); > >  /** Reads log for the value of ref during at_time. **/ >  extern int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *sha1, char **msg, unsigned long *cutoff_time, int *cutoff_tz, int *cutoff_cnt); > -- > 1.7.1.553.ga798e I can't get your point. I don't see any improvement here.  Unless you want to get rid of using references on calling functions which is only going to add another buffer to the stack, sized PATH_MAX, once that log_file is going to be really allocated in the heap after git_snpath().  As folks use to say here: "changing six by half a dozen". Even though I think you will need to turn static log_file or to call git_snpath again in the calling functions for this proposed change, don't you? > Causes t5516 to fail, but only if I run it under valgrind.  (Ævar > managed to trigger it in other ways apparently.) I haven't ever seen this happening so I think you have found some particularity of valgrind which could route a patch to it. Kind regards