Re: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref

[Phillip Wood <phillip.wood123@gmail.com>]

On 14/03/2022 21:19, Phillip Wood wrote:
> Hi Brian and Ævar

Sorry brian, I forgot you prefer a lowercase "b"

> Firstly I think this is a useful feature to add to git stash, thanks for 
> working on it Brian
> 
> On 11/03/2022 02:08, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Thu, Mar 10 2022, brian m. carlson wrote:
>>
>>> +    size_t author_len, committer_len;
>>> +    struct commit *this = NULL;
>>> +    const char *orig_author = NULL, *orig_committer = NULL;
>>> +    char *author = NULL, *committer = NULL;
>>> +    const char *buffer = NULL;
>>> +    unsigned long bufsize;
>>> +    const char *p;
>>> +    char *msg = NULL;
>>
>> These shouldn't be initialized unless they really need to..
>>
>>> +    this = lookup_commit_reference(the_repository, &info->w_commit);
>>
>> ..and some are clobbered right away here, so all of these should not 
>> be initializzed.
>>
>>> +    buffer = get_commit_buffer(this, &bufsize);
>>> +    orig_author = find_commit_header(buffer, "author", &author_len);
>>> +    orig_committer = find_commit_header(buffer, "committer", 
>>> &committer_len);
>>> +    p = memmem(buffer, bufsize, "\n\n", 2);
> 
> You could start searching from orig_committer rather than buffer but I'm 
> sure it doesn't make any real difference. The sequencer does something 
> similar to this to replay commits when rebasing - is there any scope for 
> sharing code between the two?

I had a look at sequencer.c this morning and I think the answer is "not 
easily". In principle it would be nice to have a split_commit_header() 
function that filled a struct of pointer, length pairs for the author, 
committer, message and optionally the subject that could be used here, 
in the sequencer and in fast-export but I think that is more work than 
is reasonable for this series.

Best Wishes

Phillip